Merge pull request #2289 from Keerthana-64/submodule-relative-url

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

Author: Byron

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
@@ -57,8 +56,12 @@
 
 from .util import (
     SubmoduleConfigParser,
+    _is_absolute_path,
+    _url_is_local_not_ssh,
     find_first_remote_branch,
+    is_relative_url,
     mkhead,
+    resolve_relative_url,
     sm_name,
     sm_section,
 )
@@ -68,7 +71,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 +431,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,
@@ -746,8 +744,9 @@ def add(
             )
         # END verify url
 
-        ## See #525 for ensuring git URLs in config-files are valid under Windows.
-        url = Git.polish_url(url, expand_vars=False)
+        # Relative submodule URLs must keep their prefix to resolve against the parent remote.
+        if not is_relative_url(url):
+            url = Git.polish_url(url, expand_vars=False)
 
         # It's important to add the URL to the parent config, to let `git submodule` know.
         # Otherwise there is a '-' character in front of the submodule listing:
@@ -982,13 +981,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 +1733,31 @@ 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
+        remote_name = "origin"
+        if not repo.head.is_detached:
+            with repo.active_branch.config_reader() as reader:
+                remote_name = reader.get("remote", fallback="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)
+        resolved = resolve_relative_url(remote_url, url)
+        if resolved and _url_is_local_not_ssh(resolved) and not _is_absolute_path(resolved):
+            resolved = os.path.join(repo.working_dir, resolved)
+        return resolved
+
+    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,
@@ -277,38 +280,31 @@ def update(  # type: ignore[override]
 
                             if not dry_run:
                                 previous_remote = None
-                                for remote in rmts:
-                                    if remote.url == psm.url:
-                                        previous_remote = remote
-                                        break
-                                    # END if urls match
-                                # END for each remote
+                                if len(rmts) == 1:
+                                    previous_remote = rmts[0]
+                                else:
+                                    psm_url = psm._resolved_url()
+                                    for remote in rmts:
+                                        if remote.url == psm_url:
+                                            previous_remote = remote
+                                            break
+                                        # END if urls match
+                                    # END for each remote
 
-                                # If we didn't find a matching remote, but have exactly
-                                # one, we can safely use this one.
                                 if previous_remote is None:
-                                    if len(rmts) == 1:
-                                        previous_remote = rmts[0]
-                                    else:
-                                        # If we have not found any remote with the
-                                        # original URL we may not have a name. This is a
-                                        # special case, and its okay to fail here.
-                                        # Alternatively we could just generate a unique
-                                        # name and leave all existing ones in place.
-                                        raise InvalidGitRepositoryError(
-                                            "Couldn't find original remote-repo at url %r" % psm.url
-                                        )
-                                    # END handle one single remote
+                                    raise InvalidGitRepositoryError(
+                                        "Couldn't find original remote-repo at url %r" % psm.url
+                                    )
                                 # END handle check we found a remote
 
                                 if no_fetch:
                                     # 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