Commit 7fe568f
Add jobs to reproduce Cygwin safe.directory submodule issue
Remove the Cygwin xfail decorations from test_submodules in
test_docs.py and test_repo.py, and from test_root_module in
test_submodule.py, so the tests surface the underlying failure
directly. Add 256 reproduce-safe-dir matrix jobs to cygwin-test.yml,
each running these three tests under the current safe.directory
configuration. All or most of these reproduce-safe-dir jobs must be
removed before this work is integrated.
The existing test job's env, defaults, and setup steps gain YAML
anchors so the new job can reference them without duplication.
Root cause
----------
The CI workflow adds the main repo and its .git directory to
safe.directory but not the gitdb or smmap submodule working trees.
Git matches safe.directory by exact path. When GitPython opens a
submodule as a Repo and runs "git cat-file --batch-check" against
it, git rejects the repository for dubious ownership and exits.
The race
--------
GitPython caches the cat-file process. When git has exited, the
next call to __get_object_header (git/cmd.py:1697) hits one of
two outcomes depending on whether Python's cmd.stdin.flush() runs
before or after the kernel marks the read end of the pipe as
closed. (Same mechanism as the long-standing gitpython-developers#427, in which SIGINT
kills the cat-file process and the next flush() raises
BrokenPipeError.)
If Python wins the race, flush() succeeds (data goes into the
kernel buffer); the subsequent stdout.readline() returns b"" (EOF);
_parse_object_header at git/cmd.py:1659 raises a ValueError whose
message names the rejected directory.
If git wins, flush() raises BrokenPipeError, which is OSError, which
is IOError.
In both paths, the exception travels from Object.new_from_sha at
git/repo/fun.py:229 (outside name_to_object's try/except for
ValueError-from-dereference_recursive) up through repo.commit("HEAD")
into iter_items at git/objects/submodule/base.py. That function's
"except (IOError, BadName)" at base.py:1597 catches BrokenPipeError
but not ValueError. So Path 1 propagates ValueError all the way to
the test; Path 2 ends with iter_items returning early and the test
seeing an empty submodule list.
Per-test outcomes
-----------------
What the test sees in Path 2 is determined by what the test does
with the empty list:
- test_docs::test_submodules does sm.children()[0].name, so the
[0] on the empty list raises IndexError at git/util.py:1212.
- test_repo::test_submodules does
assertGreaterEqual(len(list(self.rorepo.iter_submodules())), 2).
The recursive traversal yields gitdb (its iter_items on the *main*
repo succeeds, because the main repo IS in safe.directory) but not
smmap, so length 1 fails the assertion at test_repo.py:882.
- test_submodule::test_root_module does "assert len(rsmsp) >= 2" on
a similar traversal result, failing at test_submodule.py:513.
The race itself is non-deterministic, but the mapping from race
outcome to exception type is deterministic per test, so each test
has exactly two possible failure types -- one per side of the race:
test_docs: ValueError (Python wins) or IndexError (git wins).
test_repo: ValueError (Python wins) or AssertionError (git wins).
test_submodule: ValueError (Python wins) or AssertionError (git wins).
In particular, test_docs never produces AssertionError, and
test_repo and test_submodule never produce IndexError.
Empirical confirmation
----------------------
Across 1057 reproduce-safe-dir jobs and 5 buggy-config test (fast)
jobs in the runs cited below, every one of 2403 test failures
traces to one of four source lines: git/cmd.py:1659 (ValueError,
~98.7%), git/util.py:1212 (IndexError, only test_docs),
test/test_repo.py:882 (AssertionError, only test_repo), or
test/test_submodule.py:513 (AssertionError, only test_submodule).
Zero violations of the per-test prediction.
Reproduce-safe-dir runs on the fork:
https://github.com/EliahKagan/GitPython/actions/runs/25454533092
https://github.com/EliahKagan/GitPython/actions/runs/25454836713
https://github.com/EliahKagan/GitPython/actions/runs/25472029324
https://github.com/EliahKagan/GitPython/actions/runs/25473762375/attempts/1
https://github.com/EliahKagan/GitPython/actions/runs/25473762375/attempts/2
The first run with the fix applied (256 jobs, 768 test outcomes,
all PASSED):
https://github.com/EliahKagan/GitPython/actions/runs/25473807645
The independently observed race in the field that prompted this
investigation, from a test (fast) job in a PR gitpython-developers#2143 CI run:
https://github.com/gitpython-developers/GitPython/actions/runs/25440735020/attempts/1
Plan
----
1. (this commit) Reproduce the failure under the current
safe.directory configuration, with xfails removed so failures
surface directly.
2. Apply the fix: extend the safe.directory configuration to cover
the gitdb submodule's working tree (and smmap's, recursively).
3. Remove the reproduce-safe-dir jobs and the YAML anchors that
only the reproduce job needed. The existing test job retains the
permanent fix.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>1 parent a714ed6 commit 7fe568f
4 files changed
Lines changed: 53 additions & 30 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
20 | 20 | | |
21 | 21 | | |
22 | 22 | | |
23 | | - | |
| 23 | + | |
24 | 24 | | |
25 | 25 | | |
26 | 26 | | |
27 | | - | |
| 27 | + | |
28 | 28 | | |
29 | 29 | | |
30 | 30 | | |
31 | 31 | | |
32 | | - | |
| 32 | + | |
| 33 | + | |
33 | 34 | | |
34 | 35 | | |
35 | 36 | | |
36 | 37 | | |
37 | | - | |
| 38 | + | |
| 39 | + | |
38 | 40 | | |
39 | 41 | | |
40 | 42 | | |
41 | 43 | | |
42 | | - | |
| 44 | + | |
| 45 | + | |
43 | 46 | | |
44 | 47 | | |
45 | 48 | | |
46 | 49 | | |
47 | 50 | | |
48 | | - | |
| 51 | + | |
| 52 | + | |
49 | 53 | | |
50 | 54 | | |
51 | 55 | | |
52 | 56 | | |
53 | | - | |
| 57 | + | |
| 58 | + | |
54 | 59 | | |
55 | 60 | | |
56 | 61 | | |
57 | 62 | | |
58 | 63 | | |
59 | | - | |
| 64 | + | |
| 65 | + | |
60 | 66 | | |
61 | 67 | | |
62 | 68 | | |
63 | | - | |
| 69 | + | |
| 70 | + | |
64 | 71 | | |
65 | 72 | | |
66 | 73 | | |
67 | 74 | | |
68 | 75 | | |
69 | 76 | | |
70 | 77 | | |
71 | | - | |
| 78 | + | |
| 79 | + | |
72 | 80 | | |
73 | 81 | | |
74 | 82 | | |
75 | 83 | | |
76 | | - | |
| 84 | + | |
| 85 | + | |
77 | 86 | | |
78 | 87 | | |
79 | 88 | | |
80 | | - | |
| 89 | + | |
| 90 | + | |
81 | 91 | | |
82 | 92 | | |
83 | 93 | | |
| |||
92 | 102 | | |
93 | 103 | | |
94 | 104 | | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
9 | | - | |
10 | | - | |
11 | | - | |
12 | 9 | | |
13 | 10 | | |
14 | 11 | | |
| |||
478 | 475 | | |
479 | 476 | | |
480 | 477 | | |
481 | | - | |
482 | | - | |
483 | | - | |
484 | | - | |
485 | | - | |
486 | 478 | | |
487 | 479 | | |
488 | 480 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
877 | 877 | | |
878 | 878 | | |
879 | 879 | | |
880 | | - | |
881 | | - | |
882 | | - | |
883 | | - | |
884 | | - | |
885 | 880 | | |
886 | 881 | | |
887 | 882 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
480 | 480 | | |
481 | 481 | | |
482 | 482 | | |
483 | | - | |
484 | | - | |
485 | | - | |
486 | | - | |
487 | | - | |
488 | 483 | | |
489 | 484 | | |
490 | 485 | | |
| |||
0 commit comments