Reject submodule move destinations through intermediate symlinks by Byron · Pull Request #2232 · gitpython-developers/GitPython · GitHub

Tasks

This section is for Byron only. Models continuing this PR must not add, remove, check, uncheck, rename, or reorder checkboxes here.

refackiew

Everything below this line was generated by Codex GPT-6.

Created by Codex on behalf of Byron. Byron will review before this is ready to merge.

Submodule moves now validate intermediate destination components before changing the checkout, index, or configuration. The check shares the existing checkout-path validation and preserves ordinary moves, no-op moves, and final-component symlink behavior.

Advisory summary

GHSA-gq48-pqfc-9p58: medium severity; GitPython (pip), reported affected version = 3.1.62. No patched version or CVE is currently assigned. Reproducer details are omitted from this description.

Validation

Regression reproduced before the fix; all 30 new parameterized cases pass.
Complete submodule suite: 75 passed, 3 skipped, 1 expected failure, with commit signing disabled only for the test process.
Ruff lint/format, mypy (45 source files), and git diff --check pass.
Codex reviewed commits 5a1b6f38 and d7efa37d once each after creation and found no actionable regressions.
Git reference: 1630431f326e15fcde608827b5ff38422528eb59, builtin/mv.c and t/t7001-mv.sh, which reject intermediate symlinks without changing the index.

This check addresses existing symlinks; concurrent directory replacement remains outside its scope.

Windows CI exposed open clone handles in the normal-move test fixture. Follow-up d7efa37d closes the fixture module repository before yielding it, using the existing Windows cleanup in Repo.close(). All 30 focused cases pass locally after this test-only adjustment.

Also, sloppy review of the tests which are assumpted to not make things worse.  Submodule.move() checked lexical containment but did not validate intermediate destination components before filesystem and repository updates. GHSA-gq48-pqfc-9p58 identifies the resulting checkout-path boundary violation. The new regression failed before the fix because move() returned successfully. Share the existing abspath component walk with move() and validate the normalized destination before any mutation, including configuration-only and module-only calls. Preserve the no-op early return and existing final-component symlink handling; abspath still rejects every symlink component. This addresses pre-existing links, not concurrent directory replacement races. The Git reference checkout at 1630431f326e15fcde608827b5ff38422528eb59 uses has_symlink_leading_path() in builtin/mv.c and tests rejection without index changes in t/t7001-mv.sh. The fix follows that intermediate-component rule while retaining GitPython leaf-link compatibility. Validation: the 30 new parameterized cases pass, covering relative and absolute destinations and link targets, internal and dangling links, all move flag combinations, unchanged repository state after rejection, ordinary and no-op moves, and leaf-link compatibility. The complete test/test_submodule.py suite passes: 75 passed, 3 skipped, 1 xfailed. Test-process commit.gpgsign=false avoids sandbox GPG failures. Ruff lint and format checks, mypy (45 source files), and git diff --check pass. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 <codex@openai.com>

A bit of a sloppy review, rubber-stamping the tests based on the assumption that they are validating it's conforming to Git, probably also while increasing coverage.  Submodule.add() could clone through a checkout symlink after module_exists() swallowed the validation error. Metadata paths had a similar gap: locally planted symlinks under .git/modules could redirect cloning, reconnecting, renaming, updating, or removing a submodule. Some failures were detected only after changing configuration or moving or removing checkout directories. Reuse the checkout component check for metadata paths and validate checkout paths in the shared clone helper, including legacy embedded repositories. Check .gitfiles, submodule configuration files, and the actual repository path named by a gitfile, which can differ from .git/modules/<name>. Reject symlinked .gitmodules files as well. Preflight move and rename sources and destinations before mutation, including the implicit metadata rename when a default-named submodule moves. Keep module_exists()'s boolean contract and the existing supported replacement of a leaf symlink during a move. Add 56 regression cases covering checkout and metadata links, dangling links, redirected gitfiles, legacy clone layouts, and rejected operations preserving external targets, configuration, the index, and an empty move destination. The initial 36 cases reproduced failures before the fix. These checks reject existing symlinks; they do not prevent concurrent filesystem replacement between validation and use. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 <codex@openai.com>

🟡 Changes recommended

Two critical and two moderate findings remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This pull request hardens submodule operations against intermediate symlink traversal and adds regression coverage.

Changes:

Adds centralized checkout-path symlink validation.
Adds symlink and move regression tests.
Documents the security fix as version 3.1.63.

File summaries

File	Summary and findings
test/test_submodule.py	Adds symlink-safety tests. Moderate findings: missing .gitmodules preflight in an initialized update() path (2 votes), and inconsistent dangling-final-symlink expectation (2 votes).
git/objects/submodule/base.py	Implements path validation. Critical finding: forced removal may continue after a symlink-related ValueError instead of rejecting before mutation (2 votes).
doc/source/changes.rst	Adds the 3.1.63 changelog entry. Critical finding: VERSION remains 3.1.62, breaking release validation and publication consistency (1 vote).

Review details

Files reviewed: 3/3 changed files
Comments generated: 4
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

🟡 Changes recommended

Unresolved findings can bypass validation or cause incorrect repository state changes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

git/objects/submodule/base.py:1237

When module=True and the checkout path is symlinked, module_exists() below catches the ValueError from self.abspath and returns False. Removal then skips the checkout deletion but continues deleting the index and configuration, so remove(force=True) partially unregisters the submodule instead of rejecting it. Validate the checkout path before calling module_exists().

        self._validated_name(self.name)
        if configuration:
            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

git/objects/submodule/base.py:1103

Allowing the final symlink here leaves dangling destinations unchecked because the following osp.exists() test returns false for them. On POSIX, _renames() then calls os.rename, which replaces the dangling link and lets the move update repository metadata, contradicting the new test's required OSError and no-side-effects behavior. Reject a dangling final link before the destination-removal branch.

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

test/test_submodule.py:168

Because allow_final_symlink=True is used for destination validation, a dangling final symlink is not seen by osp.exists() or osp.isfile(), so move() reaches _renames() and os.rename() replaces the dangling link on POSIX. This branch therefore does not raise OSError and mutates repository state, causing the new test to fail. If dangling final links must be rejected, check lexists before removal; otherwise adjust the expectation to preserve the existing behavior.

        with pytest.raises(OSError if kind == "dangling" else ValueError):
            submodule.move("destination")

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Lite

🔵 Needs a closer look

Unresolved moderate findings affect update, forced removal, and dangling symlink handling.

Review details

Suppressed comments (5)

git/objects/submodule/base.py:258

This new guard only runs when a caller enters _config_parser. update() starts with self.module() and remote fetching and, for an already initialized checkout at the requested revision, can return without opening .gitmodules, so the gitmodules/update case added in this PR does not raise for the symlink. Add the .gitmodules preflight at the start of update(), before fetching.

            fp_module = cls._checked_abspath(repo.working_tree_dir, cls.k_modules_file)

git/objects/submodule/base.py:1239

These new checks cover only .gitmodules; when module=True, the module_exists() call below invokes self.module(), whose abspath now raises for a symlinked checkout, but module_exists() catches every exception and returns False. remove(force=True) therefore skips checkout deletion and continues deleting the index/configuration, so the checkout/remove case added in this PR is not rejected and can partially apply. Validate the checkout path before calling module_exists().

        if configuration:
            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

git/objects/submodule/base.py:1105

allow_final_symlink=True does not handle the new dangling-link case: osp.exists() is false for a dangling symlink, so the removal block is skipped and _renames() calls os.rename(), which replaces that link on POSIX. The kind == "dangling" assertion at test/test_submodule.py:167-168 will therefore fail and the destination link is changed; reject a lexists-but-not-exists destination before renaming.

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

test/test_submodule.py:167

On POSIX this branch does not raise for a dangling leaf link: allow_final_symlink=True lets it through, osp.isfile and osp.exists are false, and _renames uses os.rename, which replaces the dangling link. That contradicts this test and the stated preservation of final-component behavior; treat dangling like empty (or add an explicit lexists rejection if rejection is intended).

        with pytest.raises(OSError if kind == "dangling" else ValueError):

test/test_submodule.py:234

The update branch does not consult .gitmodules when the checkout is already initialized: it calls self.module(), fetches, and can return at the same commit without reaching _config_parser. A symlinked .gitmodules therefore is not rejected and this pytest.raises case fails; add the shared .gitmodules preflight in update() before fetching.

        if operation == "update":
            sm.update()

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Lite

A quick rubber-stamp, admittedly. V4 will probably review all tests and make it more proper, if there can be such a thing in python anyway.  Both Windows failures came from os.renames() pruning a directory symlink above the source after the rename succeeded. Unlike POSIX, Windows rmdir() can remove a directory symlink even when its target is nonempty. Moving a checkout through a worktree alias therefore deleted the alias and broke configuration updates and rollback. Renaming metadata through a linked .git/modules directory deleted that link and broke config.lock creation. Route checkout moves, rollback, and metadata renames through one helper. It creates destination parents and renames the source, then prunes empty source parents only until it reaches a symlink or a directory it cannot remove. This keeps ordinary empty-directory cleanup while preserving parent links and their targets, including targets that become empty. Leaf symlinks continue to move as links rather than moving their targets. Exercise Windows directory-symlink removal semantics on POSIX in the existing compatibility tests, and use native behavior on Windows. Both reported failures reproduced locally before the fix. Strengthen assertions that worktree and metadata parent aliases survive, their targets remain directories, and leaf metadata symlinks move without moving their targets. A further Windows run exposed a separate sharing violation during the checkout move. Submodule.add() read HEAD through its temporary Repo but left that Repo's persistent cat-file processes open. Those processes can hold the checkout as their current directory and prevent its rename. Close the owned Repo with a context manager when reading HEAD, including on read failure, instead of waiting for garbage collection. Add a regression that observes the real cat-file processes started for the new checkout and requires them to have exited before add() returns. It failed before the fix and now passes, along with the immediate move. The remaining metadata failures also reproduce with Python 3.7's Windows path semantics: ntpath.realpath is an alias of abspath and does not resolve symlinks. Relative core.worktree values were calculated from the metadata alias instead of the repository directory Git actually opens. This broke add/reconnect HEAD reads and made moves and renames point at nonexistent worktrees. The SHA/dubious-ownership message was a secondary read failure. Use pathlib.Path.resolve(), which resolves Windows symlinks on Python 3.7, for both endpoints of gitfile/config rewrites and for metadata removal. Run metadata and worktree-alias tests with native and simulated Windows 3.7 realpath behavior. The simulation reproduced all eight reported failures plus a leaf-symlink removal failure before this change. Reference: https://github.com/python/cpython/blob/3.7/Lib/ntpath.py and https://github.com/python/cpython/blob/3.7/Lib/pathlib.py. Validation: 183 passed, 3 skipped, and 1 expected failure across the submodule and diff suites plus the commit-message hook success test on macOS. Ruff lint and formatting, mypy for the changed module, and git diff --check passed. Native Windows validation remains for CI. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 <codex@openai.com>

🟡 Changes recommended

Three moderate review findings remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

git/objects/submodule/base.py:1241

This preflight validates .gitmodules but not the checkout. When the checkout is a symlink, module_exists() calls self.module(), catches the ValueError from self.abspath, and returns False; remove() then continues to delete the index and configuration below instead of rejecting the operation. Validate the checkout path before calling module_exists() whenever module is enabled.

        if configuration:
            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

test/test_submodule.py:234

The gitmodules/update parameter does not reach a rejecting path for an initialized submodule whose HEAD already matches the index: update() can use self.module(), fetch, and finish without invoking _config_parser. Consequently this pytest.raises block fails for that combination. Add an explicit .gitmodules preflight at the start of update() before fetching so the regression case is implemented rather than only asserted.

    with pytest.raises(ValueError, match="contains a symbolic link"):
        if operation == "update":
            sm.update()

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Lite

🟡 Changes recommended

Unresolved review findings remain in path validation, metadata cleanup, and regression coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

git/objects/submodule/base.py:1241

When the checkout itself is symlinked, this preflight only checks .gitmodules. The module_exists() call below catches the ValueError raised by self.abspath and returns False, so remove(force=True) skips physical deletion and still deletes the index/config entries. Validate the checkout path whenever module is true before recursive/removal work.

        if configuration:
            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

git/objects/submodule/base.py:1107

Because allow_final_symlink=True skips the final component, a dangling destination is false for both osp.isfile() and osp.exists() and falls through to _renames, which replaces the dangling link on POSIX. The new kind == "dangling" branch therefore will not raise as asserted; use osp.lexists() for the destination existence check if dangling leaf links are meant to remain rejected.

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

test/test_submodule.py:235

The gitmodules + update combination is not rejected when the module is already initialized: update() calls self.module() and fetches the child repository, but does not read .gitmodules, so _config_parser's symlink check is never reached. This parameterized case will fail; add an explicit .gitmodules preflight to update(), or remove this expectation if update() is intentionally allowed to use a symlinked file.

    with pytest.raises(ValueError, match="contains a symbolic link"):
        if operation == "update":
            sm.update()

test/test_submodule.py:169

On POSIX this expectation does not hold: allow_final_symlink=True lets a dangling leaf through, osp.exists() is false, and _renames() calls os.rename, which replaces the dangling link and completes the move. The assertion therefore fails and the repository state changes; either reject dangling leaf links before the rename (for example with lexists) or update this case to match the intended final-link compatibility behavior.

        with pytest.raises(OSError if kind == "dangling" else ValueError):
            submodule.move("destination")

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Lite

rubber stamp  The Python 3.7 Windows path simulation wrapped the entire os.path module in Mock. On Windows, all six simulated removal cases hit sharing violations that the default error handling converted into skipped tests. Use a SimpleNamespace copy of the path module and replace only realpath with abspath. Patch only the submodule module's osp binding so pathlib keeps its own resolver and other path operations remain ordinary calls. This retains Python 3.7 compatibility and allows the removal cases to run successfully without Windows permission-error suppression. Before removing a submodule, resolve and verify its metadata directory, then assert that removal deletes it as well as the checkout. Checking only the checkout could miss metadata left behind through a directory symlink. Validation: all 57 focused submodule compatibility and process-cleanup tests passed on Windows with Python 3.10 and HIDE_WINDOWS_KNOWN_ERRORS=0. Ruff 0.16.5 lint and formatting checks and git diff --check passed. Native Python 3.7 was unavailable; its realpath behavior is simulated. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 <codex@openai.com>

🔵 Needs a closer look

Two moderate findings remain regarding dangling destination symlinks and an ineffective gitmodules update test case.

Review details

Suppressed comments (2)

git/objects/submodule/base.py:1107

Allowing the final component here leaves a dangling destination unhandled: both osp.isfile() above and the osp.exists() check below follow links, so a dangling symlink falls through to _renames(), whose os.rename() replaces that link on POSIX. The new kind == "dangling" test therefore fails instead of raising; detect the destination with osp.lexists() (while retaining the existing empty-leaf replacement behavior).

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

test/test_submodule.py:233

The gitmodules + update case does not reach the new guard: for this already initialized checkout, update() proceeds through self.module() and fetch_remotes() without reading .gitmodules, so _config_parser is never called and no ValueError is raised. This parameterized case will fail; either preflight .gitmodules before fetching or remove/adjust this expectation if matching Git's less restrictive behavior is intentional.

    with pytest.raises(ValueError, match="contains a symbolic link"):

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Lite

🔵 Needs a closer look

Three unresolved moderate findings affect symlink validation and expected test behavior.

Review details

Suppressed comments (3)

git/objects/submodule/base.py:1107

allow_final_symlink=True still treats a dangling destination as absent: osp.exists() below is false, so _renames() replaces the dangling link and the move succeeds on POSIX. The new test_move_leaf_symlink_compatibility case expects an OSError; use osp.lexists() for the destination check (or explicitly reject dangling final links) before moving.

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

git/objects/submodule/base.py:1247

This preflight only validates .gitmodules. For a symlinked checkout, module_exists() catches the ValueError raised by self.abspath and returns False, so remove(force=True) skips the checkout deletion and continues deleting the index/configuration. That contradicts the new checkout-symlink case at test/test_submodule.py:241; validate the checkout path before calling module_exists().

        if configuration:
            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

git/objects/submodule/base.py:259

This guard is only reached when a config parser is opened. An initialized update() loads the module, fetches, and can finish without invoking _config_parser, so a symlinked working-tree .gitmodules is accepted while the new matrix at test/test_submodule.py:233 expects ValueError. Add the same .gitmodules preflight to update() before module access, inside its existing error-handling scope.

            fp_module = cls._checked_abspath(repo.working_tree_dir, cls.k_modules_file)

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Lite

Ubuntu CI reported FileNotFoundError for maintenance.lock while running test_timeout_funcs. The test performs normal pull/fetch calls before its forced timeouts, and Git can launch detached automatic maintenance from those operations. Maintenance can then race with the fixture's recursive removal of the temporary repository, removing a lock file after cleanup has enumerated it. Disable maintenance.auto in this test's temporary repository and set gc.auto to zero for older Git versions that use automatic garbage collection. This removes background housekeeping unrelated to the timeout assertions without weakening repository cleanup or changing library behavior. Use mock.patch.object for the global forced termination status so an assertion failure cannot leak the override into subsequent tests. Validation: Git Trace2 recorded three detached maintenance launches in the original test and none with the fix. The timeout test then passed 20 consecutive runs locally on macOS. Ruff lint and formatting checks and git diff --check passed. The original Ubuntu cleanup exception was not reproduced locally; tracing verified removal of the suspected race source. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 <codex@openai.com>

🔵 Needs a closer look

Unresolved implementation and regression-test issues affect symlink safety and side-effect-free behavior.

Review details

Suppressed comments (7)

Previously missed (1) — in code that hasn't changed since the last review.

test/test_submodule.py:322

Patching git.objects.submodule.base.osp does not affect pathlib.Path.resolve(), which uses pathlib's own os.path.realpath. _write_git_file_and_module_config() now calls Path.resolve() directly at base.py:487-488, so both fixture parameters exercise the same resolver and the windows37-realpath case does not model the Python 3.7 Windows behavior it claims to cover. Patch the resolver used by pathlib or make the resolution helper injectable.

git/objects/submodule/base.py:1107

allow_final_symlink=True lets a dangling destination skip the component check, but the later osp.exists() is also false for a dangling link. On POSIX, _renames() then calls os.rename(source, destination), which replaces that dangling symlink, so the new kind == "dangling" test succeeds instead of raising and the move mutates repository state. Treat the destination as present with lexists() before inspecting or replacing it.

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

git/objects/submodule/base.py:1247

With a symlinked checkout and module=True, module_exists() below catches the ValueError raised by self.module()/self.abspath and returns False. Forced removal then skips checkout deletion but still deletes the index and configuration, so the added checkout/remove case cannot pass and the rejected operation is not side-effect free. Validate the checkout path before calling module_exists(), or remove this case if that compatibility is intentional.

            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

test/test_submodule.py:235

This initialized-module update case cannot pass for link_kind == "gitmodules": update() calls self.module() and fetch_remotes() directly and never enters _config_parser, so replacing .gitmodules with a symlink is not observed and sm.update() completes without the expected ValueError. Either add an explicit .gitmodules preflight to update(), or remove this operation from the rejection matrix if Git-compatible initialized updates are intentionally allowed.

            sm.update()

test/test_submodule.py:241

The checkout/remove case does not take this rejection path. With .gitmodules intact, remove() calls module_exists(), which catches the ValueError raised by self.abspath for the symlinked checkout and returns False; removal then continues into index and configuration deletion. The new pytest.raises at test/test_submodule.py:241 therefore fails and the operation is not side-effect-free. If Git-compatible removal is intentional, remove "remove" from this matrix; otherwise preflight the checkout before module_exists().

            sm.remove(force=True)

test/test_submodule.py:308

git.Repo(alias) resolves a Path input before storing working_tree_dir (git/util.py:592-601), so this test operates on the real parent directory rather than through the alias symlink. It therefore does not exercise the claimed symlink-above-worktree case or the new root-handling path; preserve the symlink in the repository's working-tree path before running the operations.

    with git.Repo(alias) as parent:
        added = Submodule.add(parent, "new", "new", sm.url)

test/test_submodule.py:220

sm.rename("nested/module") has already moved the checkout from root / "module" to root / "nested/module". For link_kind == "checkout", this path.rename(target) therefore raises FileNotFoundError before the operation under test runs; point the checkout case at the renamed path.

    path = root / (".gitmodules" if link_kind == "gitmodules" else "module")

Files reviewed: 4/4 changed files
Comments generated: 0 new
Review effort level: Lite

🔵 Needs a closer look

Moderate unresolved issues affect update, removal, and dangling-symlink handling.

Review details

Suppressed comments (5)

git/objects/submodule/base.py:259

_config_parser is not reached by update() when the checkout is already initialized: update() calls self.module(), fetches remotes, and can finish without reading .gitmodules. Consequently the new gitmodules/update case in test_submodule_rejects_checkout_and_gitmodules_symlinks returns successfully instead of raising before side effects. Add an explicit .gitmodules preflight to update(), or remove that expectation if this operation is intentionally allowed.

            fp_module = cls._checked_abspath(repo.working_tree_dir, cls.k_modules_file)

git/objects/submodule/base.py:1259

module_exists() below calls self.module() and swallows the ValueError from self.abspath when the checkout itself is symlinked. With module=True, removal therefore skips deleting the checkout and proceeds to index/configuration deletion; for a normal .gitmodules this succeeds, and for a symlinked .gitmodules it fails only after the index and parent config have changed. Validate the checkout path before module_exists() so removal rejects without partial state changes.

        if configuration:
            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

git/objects/submodule/base.py:1119

allow_final_symlink=True skips validation of the destination leaf, but the later osp.exists(module_checkout_abspath) is false for a dangling leaf symlink. _renames() then invokes os.rename and replaces that link on POSIX, so the new kind == "dangling" test succeeds and changes repository state instead of raising. Use osp.lexists() for the destination existence check and reject the dangling case before removing or moving anything.

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

test/test_submodule.py:169

On POSIX, the new dangling case does not raise: allow_final_symlink=True lets the destination through, osp.exists() is false for a dangling link, and _renames() calls os.rename, which replaces that link. This test therefore fails and conflicts with the stated preservation of final-component behavior; either assert the successful replacement for this case or add an explicit lexists rejection in move if rejection is intended.

        with pytest.raises(OSError if kind == "dangling" else ValueError):
            submodule.move("destination")

test/test_submodule.py:235

An initialized update() follows self.module(), fetches remotes, and can reset the checkout without invoking _config_parser, so a symlinked .gitmodules is not rejected by this branch. The gitmodules + update parameter therefore completes instead of raising and makes this new regression test fail; narrow the case to operations that read configuration, or add an explicit update preflight only if rejecting this Git-compatible behavior is intended.

    with pytest.raises(ValueError, match="contains a symbolic link"):
        if operation == "update":
            sm.update()

Files reviewed: 4/4 changed files
Comments generated: 0 new
Review effort level: Lite

rubber stamp  Removing a submodule retains its metadata alias but deletes the target. Adding the same submodule again then passes a dangling directory symlink to git clone --separate-git-dir. Git for Windows fails while copying its template files through that alias. Both native-realpath and simulated Windows 3.7 remove-leaf cases reproduced this failure locally. When the metadata destination is a leaf symlink, pass its target to Git and leave the alias intact. Resolve relative targets against the link's parent, create missing target parents through the existing clone setup, and let Git create the repository directory itself. Precreating that directory is insufficient because Git rejects an existing separate git repository destination. Read the link explicitly because Python 3.7 on Windows cannot resolve a dangling link with Path.resolve(). Normalize the Windows namespace prefix returned by newer os.readlink implementations, including UNC targets, and use forward slashes before passing the path through Git's URL logic. The shared clone helper covers add() and initialization through update(). Add regression coverage for direct cloning through absolute and relative dangling metadata links with missing target parents. Verify that the link and its stored target are retained, metadata is created at the target, and the resulting checkout works under both realpath modes. Validation: 61 focused tests passed on Windows/Python 3.10 with HIDE_WINDOWS_KNOWN_ERRORS=0, including both reported failures. Ruff 0.16.5 lint and formatting checks and git diff --check passed. Native Python 3.7 and UNC network shares were not available for execution. A broader run also exposed sharing violations in sibling-reinitialization tests before cloning; a representative case also failed with the unchanged HEAD clone helper loaded in memory. That separate removal issue is not changed here. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 <codex@openai.com>

rubber stamp, just get this through CI OMG  The dangling-metadata regression passed pathlib.Path objects to os.readlink() when capturing and checking the symlink target. Windows Python 3.7 requires a string argument, so all four parameter combinations failed with TypeError before exercising the clone fix. Convert the path to str at both calls. The production clone helper already passes a string and needs no change. Keep the target-preservation assertions and the absolute/relative and realpath-mode coverage intact. Validation: reproduced all four TypeErrors on Windows/Python 3.10 with an in-memory readlink wrapper enforcing the Python 3.7 string requirement. After the conversions, all four cases passed with the same wrapper and Windows permission-error suppression disabled. Ruff 0.16.5 lint and formatting checks and git diff --check passed. Native Python 3.7 was not available locally. Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 <codex@openai.com>

🔵 Needs a closer look

Unresolved moderate findings affect dangling symlink handling, forced removal, and update validation.

Review details

Suppressed comments (5)

git/objects/submodule/base.py:1119

allow_final_symlink=True lets a dangling destination reach the osp.exists() check below, which returns false for dangling links; _renames() then calls os.rename and replaces the link on POSIX. The new kind == "dangling" case therefore does not raise and the move mutates the index/configuration. Use osp.lexists(module_checkout_abspath) for the destination-presence check so dangling links are rejected before the move.

        module_checkout_abspath = self._checkout_abspath(module_checkout_path, allow_final_symlink=True)

git/objects/submodule/base.py:1259

module_exists() below catches every exception from self.module(), including the ValueError raised by self.abspath for a symlinked checkout. Consequently remove(force=True) skips the module branch and continues to delete the index/configuration instead of rejecting the operation; the new (checkout, remove) regression case fails and can leave partial state. Validate the checkout path before calling module_exists() when module is true.

        if configuration:
            self._checked_abspath(self.repo.working_tree_dir, self.k_modules_file)

git/objects/submodule/base.py:259

This guard is not reached by an initialized update(): it first opens the module and fetches, and when the module is already at self.binsha it never reads .gitmodules. Consequently the new gitmodules + update parameterization does not raise and the test suite fails; either add an update preflight if rejection is intended, or remove that case if Git-compatible behavior is intended.

            fp_module = cls._checked_abspath(repo.working_tree_dir, cls.k_modules_file)

test/test_submodule.py:235

For an initialized submodule, update() calls self.module() and fetches/resets the child repository without opening the working-tree .gitmodules; therefore the (link_kind="gitmodules", operation="update") combination returns successfully instead of entering this pytest.raises. Either add an explicit preflight if rejecting this case is required, or exclude this pair; as written the new test fails.

    with pytest.raises(ValueError, match="contains a symbolic link"):
        if operation == "update":
            sm.update()

test/test_submodule.py:169

On POSIX this branch cannot raise for a dangling final link: allow_final_symlink=True permits it, both osp.isfile() and the later osp.exists() check return false, and _renames() replaces the dangling link with os.rename(). The added kind == "dangling" case therefore succeeds and mutates repository state instead of entering this exception block; update the expectation to preserve the existing replacement behavior, or add an explicit dangling-link rejection.

        with pytest.raises(OSError if kind == "dangling" else ValueError):
            submodule.move("destination")

Files reviewed: 4/4 changed files
Comments generated: 0 new
Review effort level: Lite

Byron force-pushed the fix-submodule-move branch 3 times, most recently from 853da83 to 340c895 Compare September 10, 2026 05:50

Byron and others added 2 commits September 10, 2026 07:51

Byron force-pushed the fix-submodule-move branch from 340c895 to 43a43cd Compare September 10, 2026 05:51

Byron marked this pull request as ready for review September 10, 2026 06:01

Copilot AI lite review requested due to automatic review settings September 10, 2026 06:01

Copilot started reviewing on behalf of Byron September 10, 2026 06:04

View session