gitpython-developers
diff --git a/‎git/objects/submodule/base.py‎
Lines changed: 26 additions & 8 deletions b/‎git/objects/submodule/base.py‎
Lines changed: 26 additions & 8 deletions
diff --git a/‎test/test_submodule.py‎
Lines changed: 69 additions & 3 deletions b/‎test/test_submodule.py‎
Lines changed: 69 additions & 3 deletions
@@ -9,6 +9,7 @@
 import ntpath
 import os
 import os.path as osp
+from pathlib import Path
 import shlex
 import stat
 import sys
@@ -444,6 +445,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
@@ -468,8 +483,9 @@ def _write_git_file_and_module_config(cls, working_tree_dir: PathLike, module_ab
             Absolute path to the bare repository.
         """
         # Git resolves metadata symlinks before interpreting core.worktree.
-        module_abspath = osp.realpath(module_abspath)
-        working_tree_dir = osp.realpath(working_tree_dir)
+        # Path.resolve() also handles Windows symlinks on Python 3.7.
+        module_abspath = str(Path(module_abspath).resolve())
+        working_tree_dir = str(Path(working_tree_dir).resolve())
         git_file = osp.join(working_tree_dir, ".git")
         module_config = osp.join(module_abspath, "config")
         rela_path = osp.relpath(module_abspath, start=working_tree_dir)
@@ -685,7 +701,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 +1138,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 +1169,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
@@ -1239,7 +1257,7 @@ def remove(
         ################################
         if module and self.module_exists():
             mod = self.module()
-            git_dir = osp.realpath(mod.git_dir)
+            git_dir = str(Path(mod.git_dir).resolve())
             if force:
                 # Take the fast lane and just delete everything in our module path.
                 # TODO: If we run into permission problems, we have a highly
@@ -1504,10 +1522,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
 
@@ -252,21 +252,79 @@ 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, metadata_realpath
+):
     """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"
 
 
+@pytest.fixture(params=[False, True], ids=["native-realpath", "windows37-realpath"])
+def metadata_realpath(request):
+    """Model Python 3.7 on Windows without altering pathlib's own resolver."""
+    if request.param:
+        with mock.patch("git.objects.submodule.base.osp", wraps=osp) as paths:
+            paths.realpath.side_effect = osp.abspath
+            yield
+    else:
+        yield
+
+
 @pytest.mark.parametrize("operation", ["add", "reconnect", "rename"])
-def test_submodule_allows_metadata_destination_symlinks(movable_submodule, tmp_path, operation):
+def test_submodule_allows_metadata_destination_symlinks(movable_submodule, tmp_path, operation, metadata_realpath):
     """Allow linked metadata destinations while keeping the checkout correctly connected.
 
     Adding, reconnecting after deinit, and renaming may store metadata outside the
@@ -296,7 +354,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, metadata_realpath
+):
     """Keep submodule operations compatible with existing symlinks in Git metadata.
 
     Cover linked metadata directories, gitfiles, configs, and internal aliases.
@@ -336,6 +396,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"