fix: Preserve submodule paths and release checkout handles on Windows

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.
<!-- agent -->
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.

Validation: 155 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>

Author: Byron

git/objects/submodule/base.py

@@ -444,6 +444,20 @@ def _checked_abspath(
                 raise ValueError("Submodule path %r contains a symbolic link" % relative_path)
         return path
 
+    @staticmethod
+    def _renames(source: PathLike, destination: PathLike) -> None:
+        os.makedirs(osp.dirname(destination), exist_ok=True)
+        os.rename(source, destination)
+        # Match renames() cleanup, but stop before directory symlinks: Windows
+        # rmdir() removes the link even when its target is nonempty.
+        parent = osp.dirname(source)
+        while parent and not osp.islink(parent):
+            try:
+                os.rmdir(parent)
+            except OSError:
+                break
+            parent = osp.dirname(parent)
+
     @classmethod
     def _write_git_file_and_module_config(cls, working_tree_dir: PathLike, module_abspath: PathLike) -> None:
         """Write a ``.git`` file containing a (preferably) relative path to the actual
@@ -685,7 +699,9 @@ def add(
 
         # We deliberately assume that our head matches our index!
         if mrepo:
-            sm.binsha = mrepo.head.commit.binsha
+            # Release cat-file processes before callers move the checkout on Windows.
+            with mrepo:
+                sm.binsha = mrepo.head.commit.binsha
         index.add([sm], write=True)
 
         return sm
@@ -1120,7 +1136,7 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool =
         # Move the module into place if possible.
         renamed_module = False
         if module and osp.exists(cur_path):
-            os.renames(cur_path, module_checkout_abspath)
+            self._renames(cur_path, module_checkout_abspath)
             renamed_module = True
 
             if osp.isfile(osp.join(module_checkout_abspath, ".git")):
@@ -1151,7 +1167,7 @@ def move(self, module_path: PathLike, configuration: bool = True, module: bool =
             # END handle configuration flag
         except Exception:
             if renamed_module:
-                os.renames(module_checkout_abspath, cur_path)
+                self._renames(module_checkout_abspath, cur_path)
             # END undo module renaming
             raise
         # END handle undo rename
@@ -1504,10 +1520,10 @@ def rename(self, new_name: str) -> "Submodule":
             # Let's be sure the submodule name is not so obviously tied to a directory.
             if str(destination_module_abspath).startswith(str(mod.git_dir)):
                 tmp_dir = self._module_abspath(self.repo, self.path, str(uuid.uuid4()))
-                os.renames(source_dir, tmp_dir)
+                self._renames(source_dir, tmp_dir)
                 source_dir = tmp_dir
             # END handle self-containment
-            os.renames(source_dir, destination_module_abspath)
+            self._renames(source_dir, destination_module_abspath)
             if mod.working_tree_dir:
                 self._write_git_file_and_module_config(mod.working_tree_dir, destination_module_abspath)
         # END move separate git repository

test/test_submodule.py

@@ -252,14 +252,59 @@ def test_submodule_rejects_checkout_and_gitmodules_symlinks(movable_submodule, t
     assert (root / "moved").is_dir()
 
 
-def test_submodule_allows_symlink_above_worktree(movable_submodule, tmp_path):
+def test_add_closes_checkout_processes(movable_submodule, monkeypatch):
+    """Adding a submodule must not leave a child process holding its checkout open."""
+    sm = movable_submodule
+    checkout = Path(sm.repo.working_tree_dir, "new")
+    execute = Git.execute
+    processes = []
+
+    def capture_process(self, command, *args, **kwargs):
+        result = execute(self, command, *args, **kwargs)
+        if (
+            kwargs.get("as_process")
+            and "cat-file" in command
+            and Path(self.working_dir).resolve() == checkout.resolve()
+        ):
+            processes.append(result.proc)
+        return result
+
+    monkeypatch.setattr(Git, "execute", capture_process)
+    try:
+        added = Submodule.add(sm.repo, "new", "new", sm.url)
+        assert processes, "The HEAD read must exercise a persistent cat-file process"
+        assert all(process.poll() is not None for process in processes)
+        added.move("moved")
+    finally:
+        for process in processes:
+            if process.poll() is None:
+                process.terminate()
+            process.wait()
+
+
+@pytest.fixture
+def windows_directory_symlink_removal(monkeypatch):
+    """Exercise Windows rmdir semantics on POSIX, where rmdir rejects symlinks."""
+    if sys.platform != "win32":
+        original_rmdir = os.rmdir
+
+        def rmdir(path, *args, **kwargs):
+            if osp.islink(path):
+                return os.unlink(path, *args, **kwargs)
+            return original_rmdir(path, *args, **kwargs)
+
+        monkeypatch.setattr(os, "rmdir", rmdir)
+
+
+def test_submodule_allows_symlink_above_worktree(movable_submodule, tmp_path, windows_directory_symlink_removal):
     """Allow adding and moving submodules when the parent is opened through a symlink."""
     sm = movable_submodule
     alias = tmp_path / "alias"
     alias.symlink_to(sm.repo.working_tree_dir, target_is_directory=True)
     with git.Repo(alias) as parent:
         added = Submodule.add(parent, "new", "new", sm.url)
         added.move("moved")
+        assert alias.is_symlink()
         with added.module() as module:
             assert Path(module.git.rev_parse("--show-toplevel")).resolve() == Path(added.abspath).resolve()
         assert Path(added.abspath, "file").read_text() == "content"
@@ -296,7 +341,9 @@ def test_submodule_allows_metadata_destination_symlinks(movable_submodule, tmp_p
 
 @pytest.mark.parametrize("kind", ["modules", "intermediate", "leaf", "gitfile", "config", "alias"])
 @pytest.mark.parametrize("operation", ["update", "move", "rename", "remove"])
-def test_submodule_allows_existing_metadata_symlinks(movable_submodule, tmp_path, kind, operation):
+def test_submodule_allows_existing_metadata_symlinks(
+    movable_submodule, tmp_path, kind, operation, windows_directory_symlink_removal
+):
     """Keep submodule operations compatible with existing symlinks in Git metadata.
 
     Cover linked metadata directories, gitfiles, configs, and internal aliases.
@@ -336,6 +383,12 @@ def test_submodule_allows_existing_metadata_symlinks(movable_submodule, tmp_path
         sm.move("moved")
     else:
         sm.rename("renamed")
+    if kind == "modules" or (operation == "rename" and kind in ("intermediate", "alias")):
+        assert link.is_symlink()
+        assert link.is_dir()
+    if operation == "rename" and kind == "leaf":
+        assert (modules / "renamed").is_symlink()
+        assert target.is_dir()
     with sm.module() as module:
         assert Path(module.git.rev_parse("--show-toplevel")).resolve() == Path(sm.abspath).resolve()
     assert Path(sm.abspath, "file").read_text() == "content"