Commit cdda97b
Add jobs to reproduce Cygwin safe.directory submodule issue
Remove the Cygwin xfail decorations from `test_submodules` in
`test/test_docs.py` and `test/test_repo.py`, and from `test_root_module`
in `test/test_submodule.py`, so the tests surface the underlying
failure directly.
Temporarily add 256 `reproduce-safe-dir` matrix jobs to
`.github/workflows/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. The
anchors will likely be removed when the `reproduce-safe-dir` jobs are.
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` 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` 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`
(outside `name_to_object`'s try/except for
`ValueError`-from-`dereference_recursive`) up through
`repo.commit("HEAD")` into `iter_items` in
`git/objects/submodule/base.py`. That function's
`except (IOError, BadName)` clause 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`.
- `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.
- `test_submodule::test_root_module` does `assert len(rsmsp) >= 2` on
a similar traversal result, failing.
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 matches
the per-test prediction. `ValueError` accounts for ~98.7%; the
race-win exception types from the list above account for the
remainder. 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 8e8b5f6 commit cdda97b
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 | + | |
| 44 | + | |
42 | 45 | | |
43 | 46 | | |
44 | 47 | | |
45 | 48 | | |
46 | 49 | | |
47 | | - | |
| 50 | + | |
| 51 | + | |
48 | 52 | | |
49 | 53 | | |
50 | 54 | | |
51 | 55 | | |
52 | | - | |
| 56 | + | |
| 57 | + | |
53 | 58 | | |
54 | 59 | | |
55 | 60 | | |
56 | 61 | | |
57 | 62 | | |
58 | | - | |
| 63 | + | |
| 64 | + | |
59 | 65 | | |
60 | 66 | | |
61 | 67 | | |
62 | | - | |
| 68 | + | |
| 69 | + | |
63 | 70 | | |
64 | 71 | | |
65 | 72 | | |
66 | 73 | | |
67 | 74 | | |
68 | 75 | | |
69 | 76 | | |
70 | | - | |
| 77 | + | |
| 78 | + | |
71 | 79 | | |
72 | 80 | | |
73 | 81 | | |
74 | 82 | | |
75 | | - | |
| 83 | + | |
| 84 | + | |
76 | 85 | | |
77 | 86 | | |
78 | 87 | | |
79 | | - | |
| 88 | + | |
| 89 | + | |
80 | 90 | | |
81 | 91 | | |
82 | 92 | | |
| |||
91 | 101 | | |
92 | 102 | | |
93 | 103 | | |
| 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 | + | |
| 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