Skip to content

Commit 48f73f8

Browse files
authored
Merge pull request #2154 from EliahKagan/claude/cygwin-safe-directory
Run more submodule tests on Cygwin (fix flaky xfails)
2 parents 9c5b57b + 4dd89af commit 48f73f8

7 files changed

Lines changed: 270 additions & 20 deletions

File tree

.github/workflows/alpine-test.yml

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,29 @@ jobs:
6161
. .venv/bin/activate
6262
pip install '.[test]'
6363
64+
- name: Show POSIX file ownership
65+
run: |
66+
for p in \
67+
"$(pwd)" \
68+
"$(pwd)/.git" \
69+
"$(pwd)/git/ext/gitdb" \
70+
"$(pwd)/git/ext/gitdb/.git" \
71+
"$(pwd)/git/ext/gitdb/gitdb/ext/smmap" \
72+
"$(pwd)/git/ext/gitdb/gitdb/ext/smmap/.git" \
73+
"${HOME:?HOME is not set}/.gitconfig"
74+
do
75+
ls -ld -- "$p" 2>/dev/null || echo "(missing: $p)"
76+
done
77+
78+
- name: Show safe.directory entries
79+
# `actions/checkout`'s safe.directory add is only durable for the
80+
# checkout itself (it writes under a throwaway HOME override and
81+
# then discards it), so by the time this step runs the runner
82+
# user's `~/.gitconfig` has no entries -- and the Alpine container
83+
# chowns the workspace to runner:docker to match the test user, so
84+
# git accepts the ownership without one. Expected: `(none)`.
85+
run: git config --global --get-all safe.directory || echo "(none)"
86+
6487
- name: Show version and platform information
6588
run: |
6689
. .venv/bin/activate

.github/workflows/cygwin-test.yml

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,8 @@ jobs:
5353
run: |
5454
git config --global --add safe.directory "$(pwd)"
5555
git config --global --add safe.directory "$(pwd)/.git"
56+
git config --global --add safe.directory "$(pwd)/git/ext/gitdb"
57+
git config --global --add safe.directory "$(pwd)/git/ext/gitdb/gitdb/ext/smmap"
5658
git config --global core.autocrlf false
5759
5860
- name: Prepare this repo for tests
@@ -80,6 +82,60 @@ jobs:
8082
run: |
8183
pip install '.[test]'
8284
85+
- name: Show POSIX file ownership
86+
# Cygwin's `ls -ld` reports the NTFS Owner SID via Cygwin's SID-to-uid
87+
# mapping (well-known SIDs by their RID, machine-local accounts by
88+
# 0x30000+RID). That mapping is what Cygwin git's
89+
# `is_path_owned_by_current_user` reduces to, so this is the view that
90+
# determines whether `safe.directory` is consulted.
91+
run: |
92+
for p in \
93+
"$(pwd)" \
94+
"$(pwd)/.git" \
95+
"$(pwd)/git/ext/gitdb" \
96+
"$(pwd)/git/ext/gitdb/.git" \
97+
"$(pwd)/.git/modules/gitdb" \
98+
"$(pwd)/git/ext/gitdb/gitdb/ext/smmap" \
99+
"$(pwd)/git/ext/gitdb/gitdb/ext/smmap/.git" \
100+
"$(pwd)/.git/modules/gitdb/modules/smmap" \
101+
"${HOME:?HOME is not set}/.gitconfig"
102+
do
103+
ls -ld -- "$p" 2>/dev/null || echo "(missing: $p)"
104+
done
105+
106+
- name: Show NTFS file ownership
107+
# Authoritative NTFS Owner via Get-Acl, with no Cygwin SID-to-uid layer
108+
# in between -- useful for confirming what the Cygwin view reports as
109+
# "Administrators" is the BUILTIN\Administrators SID (S-1-5-32-544).
110+
shell: pwsh
111+
run: |
112+
$paths = @(
113+
"$pwd",
114+
"$pwd\.git",
115+
"$pwd\git\ext\gitdb",
116+
"$pwd\git\ext\gitdb\.git",
117+
"$pwd\.git\modules\gitdb",
118+
"$pwd\git\ext\gitdb\gitdb\ext\smmap",
119+
"$pwd\git\ext\gitdb\gitdb\ext\smmap\.git",
120+
"$pwd\.git\modules\gitdb\modules\smmap",
121+
"$env:USERPROFILE\.gitconfig"
122+
)
123+
foreach ($p in $paths) {
124+
if (Test-Path -LiteralPath $p) {
125+
try {
126+
$owner = (Get-Acl -LiteralPath $p).Owner
127+
} catch {
128+
$owner = "ERROR: $($_.Exception.Message)"
129+
}
130+
"{0,-44} {1}" -f $owner, $p
131+
} else {
132+
"(missing: $p)"
133+
}
134+
}
135+
136+
- name: Show safe.directory entries
137+
run: git config --global --get-all safe.directory
138+
83139
- name: Show version and platform information
84140
run: |
85141
uname -a

.github/workflows/pythonpackage.yml

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,64 @@ jobs:
8787
run: |
8888
pip install '.[test]'
8989
90+
- name: Show POSIX file ownership
91+
# Linux and macOS only. On Windows, Git Bash's `ls -ld` reports a
92+
# uniform uid+gid for every path regardless of NTFS Owner (MSYS2's
93+
# SID-to-uid mapping doesn't have Cygwin's fidelity), so it would
94+
# not be informative here. The NTFS Owner check below covers Windows.
95+
if: matrix.os-type != 'windows'
96+
run: |
97+
for p in \
98+
"$(pwd)" \
99+
"$(pwd)/.git" \
100+
"$(pwd)/git/ext/gitdb" \
101+
"$(pwd)/git/ext/gitdb/.git" \
102+
"$(pwd)/git/ext/gitdb/gitdb/ext/smmap" \
103+
"$(pwd)/git/ext/gitdb/gitdb/ext/smmap/.git" \
104+
"${HOME:?HOME is not set}/.gitconfig"
105+
do
106+
ls -ld -- "$p" 2>/dev/null || echo "(missing: $p)"
107+
done
108+
109+
- name: Show NTFS file ownership
110+
# Windows only. Reads NTFS Owner directly via Get-Acl, which is the
111+
# authoritative view for Windows-side ownership questions; the POSIX
112+
# view via Git Bash's MSYS2 layer is not a reliable proxy here.
113+
if: matrix.os-type == 'windows'
114+
shell: pwsh
115+
run: |
116+
$paths = @(
117+
"$pwd",
118+
"$pwd\.git",
119+
"$pwd\git\ext\gitdb",
120+
"$pwd\git\ext\gitdb\.git",
121+
"$pwd\git\ext\gitdb\gitdb\ext\smmap",
122+
"$pwd\git\ext\gitdb\gitdb\ext\smmap\.git",
123+
"$env:USERPROFILE\.gitconfig"
124+
)
125+
foreach ($p in $paths) {
126+
if (Test-Path -LiteralPath $p) {
127+
try {
128+
$owner = (Get-Acl -LiteralPath $p).Owner
129+
} catch {
130+
$owner = "ERROR: $($_.Exception.Message)"
131+
}
132+
"{0,-44} {1}" -f $owner, $p
133+
} else {
134+
"(missing: $p)"
135+
}
136+
}
137+
138+
- name: Show safe.directory entries
139+
# `actions/checkout`'s safe.directory add is only durable for the
140+
# checkout itself (it writes under a throwaway HOME override and
141+
# then discards it), so by the time this step runs the runner
142+
# user's `~/.gitconfig` has no entries -- and git accepts the
143+
# workspace's ownership anyway: Git for Windows via its
144+
# Admins-group exemption on the windows matrix; on Linux/macOS
145+
# the workspace is owned by the test user. Expected: `(none)`.
146+
run: git config --global --get-all safe.directory || echo "(none)"
147+
90148
- name: Show version and platform information
91149
run: |
92150
uname -a

test/test_docs.py

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,6 @@
66
import gc
77
import os
88
import os.path
9-
import sys
10-
11-
import pytest
129

1310
from test.lib import TestBase
1411
from test.lib.helper import with_rw_directory
@@ -478,11 +475,6 @@ def test_references_and_objects(self, rw_dir):
478475

479476
repo.git.clear_cache()
480477

481-
@pytest.mark.xfail(
482-
sys.platform == "cygwin",
483-
reason="Cygwin GitPython can't find SHA for submodule",
484-
raises=ValueError,
485-
)
486478
def test_submodules(self):
487479
# [1-test_submodules]
488480
repo = self.rorepo

test/test_fixture_health.py

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
# This module is part of GitPython and is released under the
2+
# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
3+
4+
"""Verify that fixture directories are usable by git.
5+
6+
If a fixture directory is missing, isn't an initialized git repository,
7+
or is rejected by git for "dubious ownership", dependent tests
8+
elsewhere in the suite fail in opaque ways. The checks here name the
9+
preconditions directly so a misconfigured environment is recognizable
10+
from the test output rather than from a cascade of unrelated-seeming
11+
failures.
12+
13+
These tests do not exercise GitPython's production code. They verify
14+
the conditions under which production code is exercised are valid.
15+
"""
16+
17+
import subprocess
18+
from pathlib import Path
19+
20+
import pytest
21+
22+
REPO_ROOT = Path(__file__).resolve().parent.parent
23+
24+
# Directories git must trust for the test suite to operate normally. The
25+
# current set is the GitPython working tree plus the working trees of its
26+
# gitdb submodule and the smmap submodule nested inside gitdb. New entries
27+
# should be added here whenever the test suite gains a dependency on git
28+
# accepting another directory.
29+
FIXTURE_DIRS = [
30+
pytest.param(REPO_ROOT, id="repo_root"),
31+
pytest.param(REPO_ROOT / "git" / "ext" / "gitdb", id="gitdb"),
32+
pytest.param(
33+
REPO_ROOT / "git" / "ext" / "gitdb" / "gitdb" / "ext" / "smmap",
34+
id="smmap",
35+
),
36+
]
37+
38+
# Submodule working trees that must be present and initialized for the
39+
# test suite to operate normally: gitdb at `git/ext/gitdb`, and smmap
40+
# nested inside gitdb at `git/ext/gitdb/gitdb/ext/smmap`. The paths
41+
# below are anchored at REPO_ROOT (the GitPython source tree), not at
42+
# any rorepo redirection target.
43+
SUBMODULE_DIRS = [
44+
pytest.param(REPO_ROOT / "git" / "ext" / "gitdb", id="gitdb"),
45+
pytest.param(
46+
REPO_ROOT / "git" / "ext" / "gitdb" / "gitdb" / "ext" / "smmap",
47+
id="smmap",
48+
),
49+
]
50+
51+
52+
@pytest.mark.parametrize("fixture_dir", FIXTURE_DIRS)
53+
def test_fixture_dir_is_trusted_by_git(fixture_dir: Path) -> None:
54+
"""git accepts ``fixture_dir`` as its own repository owned by a trusted user.
55+
56+
Run ``git -C <fixture_dir> rev-parse --show-toplevel`` and assert it
57+
succeeds and reports ``fixture_dir`` itself as the toplevel. Failure
58+
typically means the directory's on-disk ownership doesn't match the
59+
running user and the CI workflow's ``safe.directory`` list is missing
60+
an entry that would override the check.
61+
"""
62+
if not fixture_dir.exists():
63+
pytest.skip(f"{fixture_dir} not present (run `git submodule update --init --recursive` from the repo root)")
64+
if not (fixture_dir / ".git").exists():
65+
pytest.skip(
66+
f"{fixture_dir} has no .git marker "
67+
"(submodule not initialized; run "
68+
"`git submodule update --init --recursive` from the repo root)"
69+
)
70+
try:
71+
result = subprocess.run(
72+
["git", "-C", str(fixture_dir), "rev-parse", "--show-toplevel"],
73+
capture_output=True,
74+
text=True,
75+
check=False,
76+
)
77+
except FileNotFoundError:
78+
pytest.skip("git is not installed or not on PATH")
79+
assert result.returncode == 0, (
80+
f"git refuses to operate in {fixture_dir}.\n"
81+
f"stderr: {result.stderr.strip()}\n"
82+
"The directory's owner doesn't match the running user and no "
83+
"`safe.directory` entry overrides the check. On CI, the "
84+
"workflow's `safe.directory` list typically needs an entry for "
85+
"this path. Locally, this is unexpected and usually indicates "
86+
"an ownership problem worth investigating."
87+
)
88+
reported = Path(result.stdout.strip())
89+
assert reported.samefile(fixture_dir), (
90+
f"git reports the toplevel as {reported}, "
91+
f"not as {fixture_dir} itself. "
92+
"This usually means the directory is not an initialized git "
93+
"repository (its `.git` marker may be stale or pointing elsewhere)."
94+
)
95+
96+
97+
@pytest.mark.parametrize("submodule_dir", SUBMODULE_DIRS)
98+
def test_required_submodule_is_initialized(submodule_dir: Path) -> None:
99+
"""The submodule's working tree is present and initialized.
100+
101+
Failure means the source tree is a git clone but the submodule's
102+
working tree hasn't been populated. Skipped when the source tree
103+
itself isn't a git clone (e.g. an extracted release tarball), since
104+
``git submodule update`` cannot operate there; setups that handle
105+
submodules in a separately-prepared tree (via
106+
``GIT_PYTHON_TEST_GIT_REPO_BASE``) are exempted from this check.
107+
"""
108+
if not (REPO_ROOT / ".git").exists():
109+
pytest.skip(
110+
"Source tree is not a git clone (no .git in REPO_ROOT); submodules "
111+
"cannot be initialized via `git submodule update` here. Setups "
112+
"that prepare submodules in a separately-pointed tree (via "
113+
"GIT_PYTHON_TEST_GIT_REPO_BASE) are exempted from this check."
114+
)
115+
# The assertion messages below recommend `git submodule update --init
116+
# --recursive` rather than `init-tests-after-clone.sh`, even though the
117+
# latter is the documented entry point for first-time test setup. Two
118+
# reasons: the script performs `git reset --hard` operations that can
119+
# destroy local work, and #1713 showed the script itself can carry
120+
# submodule-init regressions, in which case recommending it would lead
121+
# developers in a circle. The direct git command is a safe minimal fix
122+
# for this test's specific failure mode and bypasses any such regression.
123+
assert submodule_dir.is_dir(), (
124+
f"Submodule working tree missing: {submodule_dir}.\n"
125+
"Run `git submodule update --init --recursive` from the repo root."
126+
)
127+
assert (submodule_dir / ".git").exists(), (
128+
f"Submodule directory exists but has no .git marker: {submodule_dir}.\n"
129+
"The submodule hasn't been initialized. "
130+
"Run `git submodule update --init --recursive` from the repo root."
131+
)

test/test_repo.py

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -877,11 +877,6 @@ def test_repo_odbtype(self):
877877
target_type = GitCmdObjectDB
878878
self.assertIsInstance(self.rorepo.odb, target_type)
879879

880-
@pytest.mark.xfail(
881-
sys.platform == "cygwin",
882-
reason="Cygwin GitPython can't find submodule SHA",
883-
raises=ValueError,
884-
)
885880
def test_submodules(self):
886881
self.assertEqual(len(self.rorepo.submodules), 1) # non-recursive
887882
self.assertGreaterEqual(len(list(self.rorepo.iter_submodules())), 2)

test/test_submodule.py

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -480,11 +480,6 @@ def test_base_rw(self, rwrepo):
480480
def test_base_bare(self, rwrepo):
481481
self._do_base_tests(rwrepo)
482482

483-
@pytest.mark.xfail(
484-
sys.platform == "cygwin",
485-
reason="Cygwin GitPython can't find submodule SHA",
486-
raises=ValueError,
487-
)
488483
@pytest.mark.xfail(
489484
HIDE_WINDOWS_KNOWN_ERRORS,
490485
reason=(
@@ -513,9 +508,9 @@ def test_root_module(self, rwrepo):
513508
with rm.config_writer():
514509
pass
515510

516-
# Deep traversal gitdb / async.
511+
# Deep traversal yields gitdb and its nested smmap.
517512
rsmsp = [sm.path for sm in rm.traverse()]
518-
assert len(rsmsp) >= 2 # gitdb and async [and smmap], async being a child of gitdb.
513+
assert rsmsp == ["git/ext/gitdb", "gitdb/ext/smmap"]
519514

520515
# Cannot set the parent commit as root module's path didn't exist.
521516
self.assertRaises(ValueError, rm.set_parent_commit, "HEAD")

0 commit comments

Comments
 (0)