fix(submodule): resolve relative submodule URLs the way git does

git/objects/submodule/base.py

@@ -12,7 +12,6 @@
 import shlex
 import stat
 import sys
-import urllib.parse
 import uuid
 from io import BytesIO
 from pathlib import Path
@@ -58,7 +57,9 @@
 from .util import (
     SubmoduleConfigParser,
     find_first_remote_branch,
+    is_relative_url,
     mkhead,
+    resolve_relative_url,
     sm_name,
     sm_section,
 )
@@ -68,7 +69,7 @@
 if TYPE_CHECKING:
     from git.index import IndexFile
     from git.objects.commit import Commit
-    from git.refs import Head, RemoteReference
+    from git.refs import Head
     from git.repo import Repo
 
 # -----------------------------------------------------------------------------
@@ -428,13 +429,8 @@ def _clone_repo(
             if not osp.isdir(module_abspath_dir):
                 os.makedirs(module_abspath_dir)
 
-        if url.startswith("../"):
-            remote_name = cast("RemoteReference", repo.active_branch.tracking_branch()).remote_name
-            repo_remote_url = repo.remote(remote_name).url
-            url = os.path.join(repo_remote_url, url)
-
         clone = git.Repo.clone_from(
-            url,
+            cls._resolve_url(repo, url),
             module_checkout_path,
             allow_unsafe_options=allow_unsafe_options,
             allow_unsafe_protocols=allow_unsafe_protocols,
@@ -982,13 +978,9 @@ def fetch_remotes(module_repo: "Repo") -> None:
                     if not dry_run:
                         if no_fetch:
                             raise ValueError("Missing module at %r but fetching is disabled" % self.path) from None
-                        if self.url.startswith("."):
-                            url = urllib.parse.urljoin(self.repo.remotes.origin.url + "/", self.url)
-                        else:
-                            url = self.url
                         mrepo = self._clone_repo(
                             self.repo,
-                            url,
+                            self.url,
                             self.path,
                             self.name,
                             n=True,
@@ -1738,6 +1730,26 @@ def url(self) -> str:
         """:return: The url to the repository our submodule's repository refers to"""
         return self._url
 
+    @staticmethod
+    def _resolve_url(repo: "Repo", url: str) -> str:
+        """Resolve relative URLs against the tracking remote, then ``origin``,
+        falling back to the superproject's directory the way git does."""
+        if not is_relative_url(url):
+            return url
+        tracking_branch = None if repo.head.is_detached else repo.active_branch.tracking_branch()
+        remote_name = tracking_branch.remote_name if tracking_branch is not None else "origin"
+        try:
+            remote_url = repo.remote(remote_name).url
+        except ValueError:
+            remote_url = os.fspath(repo.working_dir)
+        if remote_url == ".":
+            remote_url = os.fspath(repo.working_dir)
+        return resolve_relative_url(remote_url, url)
+
+    def _resolved_url(self) -> str:
+        """:return: :attr:`url` resolved against the superproject's default remote"""
+        return self._resolve_url(self.repo, self.url)
+
     @property
     def parent_commit(self) -> "Commit":
         """

git/objects/submodule/root.py

@@ -264,10 +264,13 @@ def update(  # type: ignore[override]
                         nn = "__new_origin__"
                         smm = sm.module()
                         rmts = smm.remotes
+                        # Relative urls are resolved against the superproject's remote,
+                        # which is what the module's remotes hold.
+                        sm_url = sm._resolved_url()
 
                         # Don't do anything if we already have the url we search in
                         # place.
-                        if len([r for r in rmts if r.url == sm.url]) == 0:
+                        if len([r for r in rmts if r.url == sm_url]) == 0:
                             progress.update(
                                 BEGIN | URLCHANGE,
                                 i,
@@ -276,9 +279,10 @@ def update(  # type: ignore[override]
                             )
 
                             if not dry_run:
+                                psm_url = psm._resolved_url()
                                 previous_remote = None
                                 for remote in rmts:
-                                    if remote.url == psm.url:
+                                    if remote.url == psm_url:
                                         previous_remote = remote
                                         break
                                     # END if urls match
@@ -305,10 +309,10 @@ def update(  # type: ignore[override]
                                     # A new remote would have no cached refs. Preserve
                                     # the existing refs and tracking configuration for
                                     # offline updates instead of replacing the remote.
-                                    previous_remote.set_url(git.Git.polish_url(sm.url, expand_vars=False))
+                                    previous_remote.set_url(git.Git.polish_url(sm_url, expand_vars=False))
                                 else:
                                     assert nn not in [r.name for r in rmts]
-                                    smr = smm.create_remote(nn, sm.url)
+                                    smr = smm.create_remote(nn, sm_url)
                                     smr.fetch(progress=progress)
 
                                     # If we have a tracking branch, it should be available

git/objects/submodule/util.py

@@ -6,10 +6,13 @@
     "sm_name",
     "mkhead",
     "find_first_remote_branch",
+    "is_relative_url",
+    "resolve_relative_url",
     "SubmoduleConfigParser",
 ]
 
 from io import BytesIO
+import sys
 import weakref
 
 import git
@@ -63,6 +66,97 @@ def find_first_remote_branch(remotes: Sequence["Remote"], branch_name: str) -> "
     raise InvalidGitRepositoryError("Didn't find remote branch '%r' in any of the given remotes" % branch_name)
 
 
+def _is_dir_sep(char: str) -> bool:
+    return char == "/" or (sys.platform == "win32" and char == "\\")
+
+
+def _starts_with_dot_slash(url: str) -> bool:
+    return len(url) >= 2 and url[0] == "." and _is_dir_sep(url[1])
+
+
+def _starts_with_dot_dot_slash(url: str) -> bool:
+    return len(url) >= 3 and url[0:2] == ".." and _is_dir_sep(url[2])
+
+
+def _has_dos_drive_prefix(url: str) -> bool:
+    return sys.platform == "win32" and len(url) >= 2 and url[0].isalpha() and url[1] == ":"
+
+
+def _is_absolute_path(url: str) -> bool:
+    return (len(url) >= 1 and _is_dir_sep(url[0])) or _has_dos_drive_prefix(url)
+
+
+def _url_is_local_not_ssh(url: str) -> bool:
+    colon = url.find(":")
+    slash = url.find("/")
+    return colon == -1 or (slash != -1 and slash < colon) or _has_dos_drive_prefix(url)
+
+
+def _find_last_dir_sep(url: str) -> int:
+    for index in range(len(url) - 1, -1, -1):
+        if _is_dir_sep(url[index]):
+            return index
+    return -1
+
+
+def is_relative_url(url: str) -> bool:
+    """:return: ``True`` if `url` starts with ``./`` or ``../``, which git resolves
+    against the superproject's remote rather than treating as a path"""
+    return _starts_with_dot_slash(url) or _starts_with_dot_dot_slash(url)
+
+
+def resolve_relative_url(remote_url: str, url: str) -> str:
+    """Resolve a submodule URL that starts with ``./`` or ``../`` against the
+    superproject's remote URL, the way git's ``relative_url`` does.
+
+    Git treats a relative submodule URL like a relative directory: each leading
+    ``../`` removes the last component of `remote_url`, whether that is a path
+    component, the host of a URL, or the host of an scp-like ``host:path`` address.
+    The remainder is appended with ``/``, or with ``:`` once an scp-like host was
+    reached. A URL that is not relative is returned unchanged.
+
+    :raise ValueError:
+        If more components are stripped than `remote_url` has, which git rejects with
+        "cannot strip one component off url".
+    """
+    if not is_relative_url(url):
+        return url
+
+    remote = remote_url
+    if remote and _is_dir_sep(remote[-1]):
+        remote = remote[:-1]
+
+    is_relative = _url_is_local_not_ssh(remote) and not _is_absolute_path(remote)
+    if is_relative and not _starts_with_dot_slash(remote) and not _starts_with_dot_dot_slash(remote):
+        remote = "./" + remote
+
+    colonsep = False
+    while url:
+        if _starts_with_dot_dot_slash(url):
+            url = url[3:]
+            separator = _find_last_dir_sep(remote)
+            if separator != -1:
+                remote = remote[:separator]
+            elif ":" in remote:
+                remote = remote[: remote.rfind(":")]
+                colonsep = True
+            elif is_relative or remote == ".":
+                raise ValueError("cannot strip one component off url %r" % remote)
+            else:
+                remote = "."
+        elif _starts_with_dot_slash(url):
+            url = url[2:]
+        else:
+            break
+
+    resolved = remote + (":" if colonsep else "/") + url
+    if url.endswith("/"):
+        resolved = resolved[:-1]
+    if _starts_with_dot_slash(resolved):
+        resolved = resolved[2:]
+    return resolved
+
+
 # } END utilities
 
 # { Classes

test/test_submodule.py

@@ -26,6 +26,7 @@
 )
 from git.objects.submodule.base import Submodule
 from git.objects.submodule.root import RootModule, RootUpdateProgress
+from git.objects.submodule.util import resolve_relative_url
 from git.repo.fun import find_submodule_git_dir, touch
 from git.util import HIDE_WINDOWS_KNOWN_ERRORS, cwd, join_path_native, to_native_path_linux
 
@@ -52,6 +53,72 @@ def _patch_git_config(name, value):
         yield
 
 
+@pytest.mark.parametrize("state", ["unborn", "untracked", "detached"])
+@pytest.mark.parametrize("remote", [None, ".", "origin"], ids=["working-tree", "local-origin", "origin"])
+def test_add_relative_submodule_without_tracking_remote(tmp_path, state, remote):
+    with git.Repo.init(tmp_path / "parent") as parent:
+        if state != "unborn":
+            parent.index.commit("Initial commit")
+        if state == "detached":
+            parent.head.reference = parent.head.commit
+        base = tmp_path / "remote" if remote == "origin" else tmp_path / "parent"
+        if remote is not None:
+            parent.create_remote("origin", str(base) if remote == "origin" else ".")
+        with git.Repo.init(base / "source") as source:
+            source.index.commit("Initial commit")
+            submodule = parent.create_submodule("module", "module", "./source")
+            assert submodule.url == "./source"
+            with submodule.module() as module:
+                assert module.head.commit == source.head.commit
+                assert Path(module.remotes.origin.url).samefile(source.working_tree_dir)
+
+
+def test_update_resolves_local_relative_remote_once(tmp_path):
+    with git.Repo.init(tmp_path / "parent") as parent, git.Repo.init(tmp_path / "repos" / "child.git") as source:
+        source.git.symbolic_ref("HEAD", "refs/heads/master")
+        source.index.commit("Initial commit")
+        parent.create_remote("origin", "../repos/super.git")
+        parent.head.reference.set_tracking_branch(git.RemoteReference(parent, "refs/remotes/origin/master"))
+        submodule = Submodule(
+            parent,
+            source.head.commit.binsha,
+            name="module",
+            path="module",
+            url="../child.git",
+            branch_path="refs/heads/master",
+        )
+        with cwd(parent.working_tree_dir):
+            submodule.update(init=True)
+        with submodule.module() as module:
+            assert module.head.commit == source.head.commit
+            assert Path(module.remotes.origin.url).samefile(source.working_tree_dir)
+
+
+@pytest.mark.parametrize("dry_run", [False, True])
+def test_root_update_skips_old_url_resolution_when_remote_matches(tmp_path, dry_run):
+    with git.Repo.init(tmp_path / "source") as source, git.Repo.init(tmp_path / "parent") as parent:
+        source.index.commit("Initial commit")
+        submodule = parent.create_submodule("module", "module", source.working_tree_dir)
+        with submodule.config_writer() as writer:
+            writer.set_value("url", "../source")
+        previous = parent.index.commit("Record relative submodule URL")
+        with submodule.config_writer() as writer:
+            writer.set_value("url", Git.polish_url(source.working_tree_dir))
+        parent.index.commit("Use existing remote URL")
+
+        resolve_url = Submodule._resolved_url
+
+        def resolve_current_url(sm):
+            assert sm.url != "../source", "Previous URL does not need resolving"
+            return resolve_url(sm)
+
+        with mock.patch.object(Submodule, "_resolved_url", autospec=True, side_effect=resolve_current_url):
+            RootModule(parent).update(previous_commit=previous, no_fetch=True, dry_run=dry_run)
+        with submodule.module() as module:
+            assert module.remotes.origin.url == Git.polish_url(source.working_tree_dir)
+            assert module.head.commit == source.head.commit
+
+
 @pytest.mark.parametrize(
     "name", ["module", "$GITPYTHON_TEST_SECRET", "prefix-${GITPYTHON_TEST_SECRET}-suffix", "%GITPYTHON_TEST_SECRET%"]
 )
@@ -2416,3 +2483,66 @@ def test_submodule_update_relative_url(self, rwdir):
 
         has_module = any(sm.name == "module" for sm in cloned_repo.submodules)
         assert has_module, "Relative submodule was not updated properly"
+
+    def test_resolve_relative_url_like_git(self):
+        # Expected values are what `git submodule init` records for the same pair.
+        cases = [
+            ("ssh://host/org/super.git", "./sub", "ssh://host/org/super.git/sub"),
+            ("ssh://host/org/super.git", "../sub", "ssh://host/org/sub"),
+            ("ssh://host:22/org/super.git", "../sub", "ssh://host:22/org/sub"),
+            ("git@host:org/super.git", "./sub", "git@host:org/super.git/sub"),
+            ("git@host:org/super.git", "../sub", "git@host:org/sub"),
+            ("git@host:org/super.git", "../../sub", "git@host:sub"),
+            ("https://host/org/super.git", "./sub/", "https://host/org/super.git/sub"),
+            ("https://host/org/super.git/", "././sub", "https://host/org/super.git/sub"),
+            ("https://host/org/super.git", ".././sub", "https://host/org/sub"),
+            ("https://host/org/super.git", "../../../sub", "https://sub"),
+            ("file:///srv/git/super.git", "../sub", "file:///srv/git/sub"),
+            ("/srv/git/super", "../sub", "/srv/git/sub"),
+            ("/srv/git/super", "../../../../sub", "sub"),
+            ("//share/super", "../../sub", "//sub"),
+            ("foo/bar", "../sub", "foo/sub"),
+            ("./foo", "../sub", "sub"),
+            ("../foo/bar", "../sub", "../foo/sub"),
+            ("ssh://host/org/super.git", ".sub", ".sub"),
+            ("ssh://host/org/super.git", "https://other/sub", "https://other/sub"),
+        ]
+        for remote_url, url, expected in cases:
+            assert resolve_relative_url(remote_url, url) == expected, (remote_url, url)
+
+        for remote_url, url in [("foo", "../../sub"), ("git@host:super.git", "../../../sub"), ("../foo", "../../sub")]:
+            with self.assertRaisesRegex(ValueError, "cannot strip one component"):
+                resolve_relative_url(remote_url, url)
+
+    @with_rw_directory
+    def test_update_resolves_relative_url_against_remote(self, rwdir):
+        parent = git.Repo.init(osp.join(rwdir, "parent"))
+        parent.create_remote("origin", "ssh://git@host/org/super.git")
+        os.mkdir(osp.join(parent.working_tree_dir, "planted"))
+
+        for url, expected in [
+            ("./planted", "ssh://git@host/org/super.git/planted"),
+            ("../planted", "ssh://git@host/org/planted"),
+        ]:
+            submodule = Submodule(parent, Submodule.NULL_BIN_SHA, name="module", path="module", url=url)
+            with mock.patch.object(git.Repo, "clone_from", side_effect=AssertionError("stop")) as clone_from:
+                with pytest.raises(AssertionError, match="stop"):
+                    submodule.update(init=True)
+            assert clone_from.call_args.args[0] == expected
+
+    @with_rw_directory
+    def test_clone_repo_resolves_relative_url_against_tracking_remote(self, rwdir):
+        parent = git.Repo.init(osp.join(rwdir, "parent"))
+        parent.index.commit("initial")
+        parent.create_remote("origin", "git@host:other/super.git")
+        parent.create_remote("upstream", "git@host:org/super.git")
+        parent.head.reference.set_tracking_branch(git.RemoteReference(parent, "refs/remotes/upstream/main"))
+
+        for url, expected in [
+            ("./planted", "git@host:org/super.git/planted"),
+            ("../planted", "git@host:org/planted"),
+        ]:
+            with mock.patch.object(git.Repo, "clone_from", side_effect=AssertionError("stop")) as clone_from:
+                with pytest.raises(AssertionError, match="stop"):
+                    Submodule._clone_repo(parent, url, "module", "module")
+            assert clone_from.call_args.args[0] == expected