FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Clearly reproduce Cygwin safe.directory submodule bug · gitpython-developers/GitPython@ce68322 · GitHub

  1. gitpython-developers
  2. GitPython

Repository navigation

Commit ce68322

Browse files
EliahKagan
and
claude
committed
Clearly reproduce Cygwin safe.directory submodule bug
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. Also 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 shall be removed before the associated bugfix 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 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 specific paths (or wildcards, but we are not using wildcards). 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. (This is the same mechanism as the long-standing #427, in which SIGINT kills the cat-file process and the next `flush()` raises `BrokenPipeError`.) 1. 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. 2. If git wins, `flush()` raises `BrokenPipeError`, a subclass of `OSError` (a.k.a. `IOError`). In both paths, the exception travels from `Object.new_from_sha` (outside `name_to_object`'s `except ValueError`, which covers only the `dereference_recursive` call), 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, while 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 ---------------------- In 1024 `reproduce-safe-dir` jobs and 4 buggy-config "test (fast)" job runs, 100% of 3084 target-test failures match the per-test prediction. `ValueError` accounts for ~99.0%, while the race-win exception types from the list above account for the others. `reproduce-safe-dir` runs: https://github.com/EliahKagan/GitPython/actions/runs/25836741324 https://github.com/EliahKagan/GitPython/actions/runs/25836329241 https://github.com/EliahKagan/GitPython/actions/runs/25836334196 https://github.com/EliahKagan/GitPython/actions/runs/25836339345 Run on the fix commit (256 jobs, 768 test outcomes, all PASSED): https://github.com/EliahKagan/GitPython/actions/runs/25836344886 The race condition, from a "test (fast)" job in a PR #2143 CI run: https://github.com/gitpython-developers/GitPython/actions/runs/25440735020/attempts/1 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent d5fb280 commit ce68322

4 files changed

Lines changed: 53 additions & 30 deletions

File tree

‎.github/workflows/cygwin-test.yml‎

Lines changed: 53 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,63 +20,73 @@ jobs:
2020

2121
runs-on: windows-latest
2222

23-
env:
23+
env: &cygwin-env
2424
CHERE_INVOKING: "1"
2525
CYGWIN_NOWINPATH: "1"
2626

27-
defaults:
27+
defaults: &cygwin-defaults
2828
run:
2929
shell: C:\cygwin\bin\bash.exe --login --norc -eo pipefail -o igncr "{0}"
3030

3131
steps:
32-
- name: Force LF line endings
32+
- &force-lf
33+
name: Force LF line endings
3334
run: |
3435
git config --global core.autocrlf false # Affects the non-Cygwin git.
3536
shell: pwsh # Do this outside Cygwin, to affect actions/checkout.
3637

37-
- uses: actions/checkout@v6
38+
- &checkout
39+
uses: actions/checkout@v6
3840
with:
3941
fetch-depth: 0
4042

41-
- name: Install Cygwin
43+
- &install-cygwin
44+
name: Install Cygwin
4245
uses: cygwin/cygwin-install-action@v6
4346
with:
4447
packages: git python39 python-pip-wheel python-setuptools-wheel python-wheel-wheel
4548
add-to-path: false # No need to change $PATH outside the Cygwin environment.
4649

47-
- name: Arrange for verbose output
50+
- &verbose-output
51+
name: Arrange for verbose output
4852
run: |
4953
# Arrange for verbose output but without shell environment setup details.
5054
echo 'set -x' >~/.bash_profile
5155
52-
- name: Special configuration for Cygwin git
56+
- &safe-directory
57+
name: Special configuration for Cygwin git
5358
run: |
5459
git config --global --add safe.directory "$(pwd)"
5560
git config --global --add safe.directory "$(pwd)/.git"
5661
git config --global core.autocrlf false
5762
58-
- name: Prepare this repo for tests
63+
- &prepare-repo
64+
name: Prepare this repo for tests
5965
run: |
6066
./init-tests-after-clone.sh
6167
62-
- name: Set git user identity and command aliases for the tests
68+
- &git-identity
69+
name: Set git user identity and command aliases for the tests
6370
run: |
6471
git config --global user.email "travis@ci.com"
6572
git config --global user.name "Travis Runner"
6673
# If we rewrite the user's config by accident, we will mess it up
6774
# and cause subsequent tests to fail
6875
cat test/fixtures/.gitconfig >> ~/.gitconfig
6976
70-
- name: Set up virtual environment
77+
- &setup-venv
78+
name: Set up virtual environment
7179
run: |
7280
python3.9 -m venv .venv
7381
echo 'BASH_ENV=.venv/bin/activate' >>"$GITHUB_ENV"
7482
75-
- name: Update PyPA packages
83+
- &update-pypa
84+
name: Update PyPA packages
7685
run: |
7786
python -m pip install -U pip 'setuptools; python_version<"3.12"' wheel
7887
79-
- name: Install project and test dependencies
88+
- &install-deps
89+
name: Install project and test dependencies
8090
run: |
8191
pip install '.[test]'
8292
@@ -91,3 +101,34 @@ jobs:
91101
- name: Test with pytest (${{ matrix.additional-pytest-args }})
92102
run: |
93103
pytest --color=yes -p no:sugar --instafail -vv ${{ matrix.additional-pytest-args }}
104+
105+
reproduce-safe-dir:
106+
strategy:
107+
matrix:
108+
run: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 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, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 217, 218, 219, 220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 251, 252, 253, 254, 255, 256]
109+
fail-fast: false
110+
111+
runs-on: windows-latest
112+
113+
env: *cygwin-env
114+
115+
defaults: *cygwin-defaults
116+
117+
steps:
118+
- *force-lf
119+
- *checkout
120+
- *install-cygwin
121+
- *verbose-output
122+
- *safe-directory
123+
- *prepare-repo
124+
- *git-identity
125+
- *setup-venv
126+
- *update-pypa
127+
- *install-deps
128+
129+
- name: Run submodule tests
130+
run: |
131+
python -m pytest -vv \
132+
test/test_docs.py::Tutorials::test_submodules \
133+
test/test_repo.py::TestRepo::test_submodules \
134+
test/test_submodule.py::TestSubmodule::test_root_module

‎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_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: 0 additions & 5 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=(

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL