Skip to content

Commit 9d74656

Browse files
codexByron
authored andcommitted
fix(submodule): bind writes to the validated checkout
The requested audit against Git's submodule path validation found that `Submodule.module()` checked the checkout path, then let `Repo` select a different working tree from `core.worktree`. A symlink in that configured path could redirect `update()` writes outside the checkout. Unforced `remove(configuration=False)` could delete the external working tree. Pass the checked checkout as `GIT_WORK_TREE` when opening the shared module handle. This keeps updates, removal and recursive traversal on the validated path without making repository queries rewrite configuration. Existing Git metadata symlinks remain supported. Git reference `6de20f6092dcf9bdb1c8efe03db4b70c82b423dd` validates checkout components in `submodule.c:validate_submodule_path()` and repairs configured worktrees before update in `builtin/submodule--helper.c:ensure_core_worktree()`. Git 2.54.0 preserves external targets in the new update/deinit comparisons. GitPython previously overwrote or deleted them. The regressions cover leaf and intermediate symlinks, and pass on both CLI and Gix backends after the fix. Record the independently observed GixPython 0.3.0 configured-worktree canonicalization difference under the still-open `GIX-14` ledger entry. The scoped submodule guard does not claim to fix native discovery upstream. Validation: eight focused regression cases per backend; full `mypy` and `basedpyright --warnings`; Ruff check/format; ShellCheck with sourced fixtures; `git diff --check`. Agent-Model: unknown
1 parent 00ae9a2 commit 9d74656

4 files changed

Lines changed: 63 additions & 1 deletion

File tree

‎doc/gix-backend.md‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -968,6 +968,18 @@ identify the historical fixture where needed.
968968
| GIX-33 | Bug (open); missing exact-name mode | With Git 2.54.0 and GixPython 0.3.0, `exact-refs.sh sha1 files symbolic` makes `refs/tags/REBASE_HEAD` a symbolic ref to another tag while the root `REBASE_HEAD` is absent. `Repository.try_find_reference("REBASE_HEAD")` returns that tag; native `_symbolic_ref` formerly returned its target, whereas `git symbolic-ref --quiet --no-recurse -- REBASE_HEAD` exits 1. GitPython's root-reference object read consequently returned a commit through Gix and raised `ValueError` through CLI. SHA-256 reproduces the same mismatch. | Expose an exact-name lookup mode, including root references, whose absence and error behavior matches Git. `_symbolic_ref()` compares the native reference's name with the requested bytes and selects CLI for a lookalike. `test_missing_root_reference_does_not_follow_symbolic_lookalike_tag` verifies parity and forced CLI fallback. This adapter guard does not close the upstream compatibility bug. |
969969
| GIX-34 | Bug (open) | With Git 2.54.0 and GixPython 0.3.0, an author, committer or tagger line with fields after its time zone, such as `A <a@example.com> 1112591163 +0200 1700000000 -0500`, decodes through `Commit.author()`/`committer()` and `Tag.tagger()` with the correct seconds but an offset of 0. Git reads only the first date and time zone after the last `>` (`split_ident_line`) and reports `1112591163 +0200` through `git show --format=%ad --date=raw` and `%(taggerdate:raw)`. `git fsck` reports `badTimezone` for such objects, but the default `transfer.fsckObjects=false` accepts them. The `trailing-date` read-only `commit.sh` fixture in `test_deserialization_reads_date_after_email` reproduces both object kinds. | Report the time zone Git reads for identity lines with trailing fields, or reject them consistently. `object_metadata()` selects CLI when the decoded signature does not reproduce the end of its raw header line. This adapter guard does not close the upstream compatibility bug. |
970970

971+
GIX-14 also covers configured worktree symlink resolution. On macOS with
972+
GixPython 0.3.0, CPython 3.12.14 and Git 2.54.0, set a submodule's
973+
`core.worktree` to `parent/redirect`, where `redirect` links to `outside`.
974+
`Repository.workdir()` retains `parent/redirect`; Git's
975+
`rev-parse --show-toplevel` returns `outside`. A trailing `/worktree` reproduces
976+
the same difference for an intermediate symlink. Opening the canonical gitfile
977+
or reopening the canonical native Git directory does not resolve it.
978+
`test_submodule_writes_use_checked_worktree` exercises both layouts: submodule
979+
handles now pass the validated checkout as `GIT_WORK_TREE` to prevent redirected
980+
writes in either backend. General native discovery still needs Git-compatible
981+
configured-worktree resolution; this guard does not close GIX-14.
982+
971983
The PR #2306 regression exposed an adapter ownership-check bypass in both
972984
backends: `Repo`'s automatic `GIT_DIR` made Git 2.54.0 treat hook execution as
973985
explicitly selecting the repository, so it ran a hook where an ordinary

‎git/objects/submodule/base.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1727,7 +1727,8 @@ def module(self) -> "Repo":
17271727
module_checkout_abspath = self.abspath
17281728
self._module_abspath(self.repo, self.path, self.name)
17291729
try:
1730-
repo = git.Repo(module_checkout_abspath)
1730+
# Keep core.worktree from redirecting writes past the validated checkout.
1731+
repo = git.Repo(module_checkout_abspath, _env={"GIT_WORK_TREE": os.fspath(module_checkout_abspath)})
17311732
if repo != self.repo:
17321733
return repo
17331734
# END handle repo uninitialized

‎test/test_submodule.py‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -949,6 +949,36 @@ def test_submodule_rejects_checkout_and_gitmodules_symlinks(configured_submodule
949949
assert (root / "moved").is_dir()
950950

951951

952+
@pytest.mark.parametrize("implementation", ["git", "gitpython"])
953+
@pytest.mark.parametrize("operation", ["update", "remove"])
954+
@pytest.mark.parametrize("kind", ["leaf", "intermediate"])
955+
def test_submodule_writes_use_checked_worktree(configured_submodule, implementation, operation, kind):
956+
"""A configured worktree must not redirect writes past the checked checkout."""
957+
sm = configured_submodule
958+
root = Path(sm.repo.working_tree_dir)
959+
outside = root.parent / "outside"
960+
if kind == "intermediate":
961+
outside /= "worktree"
962+
before = {path.name: path.read_bytes() for path in outside.iterdir()}
963+
if implementation == "git":
964+
if operation == "update":
965+
sm.repo.git.submodule("update", "--force", "--no-fetch", "--", sm.path)
966+
else:
967+
sm.repo.git.submodule("deinit", "--force", "--", sm.path)
968+
elif operation == "update":
969+
sm.update(force=True, no_fetch=True)
970+
else:
971+
sm.remove(configuration=False)
972+
973+
assert outside.is_dir()
974+
assert {path.name: path.read_bytes() for path in outside.iterdir()} == before
975+
assert (root / "redirect").is_symlink()
976+
if operation == "update":
977+
assert (root / "module/file").read_text() == "updated"
978+
else:
979+
assert not (root / "module/file").exists()
980+
981+
952982
def test_add_closes_checkout_processes(movable_submodule, monkeypatch):
953983
"""Adding a submodule must not leave a child process holding its checkout open."""
954984
# Native reads have no persistent process; keep exercising the CLI fallback.

‎tests/fixtures/submodule-symlinks.sh‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,25 @@ relative_target=${args[6]}
1010
modules="$root/parent/.git/modules"
1111
target="$root/outside"
1212
case "$scenario" in
13+
test_submodule_writes_use_checked_worktree)
14+
if test "${args[5]}" = update; then
15+
printf updated > parent/module/file
16+
git -C parent/module add file
17+
git -C parent/module commit -qm 'Advance submodule'
18+
git -C parent add module
19+
git -C parent commit -qm 'Record updated submodule'
20+
git -C parent/module reset -q --hard HEAD^
21+
fi
22+
mkdir "$target"
23+
ln -s "$target" parent/redirect
24+
worktree="$root/parent/redirect"
25+
if test "$kind" = intermediate; then
26+
mkdir "$target/worktree"
27+
worktree="$worktree/worktree"
28+
fi
29+
printf content > "$worktree/file"
30+
git -C parent/module config core.worktree "$worktree"
31+
;;
1332
test_move_leaf_symlink_compatibility)
1433
case "$kind" in
1534
empty|nonempty) mkdir "$target" ;;

0 commit comments

Comments
 (0)