diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index 74fbd39fe..6660afefd 100644 --- a/git/objects/submodule/base.py +++ b/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, @@ -746,8 +742,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 +979,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 +1731,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": """ diff --git a/git/objects/submodule/root.py b/git/objects/submodule/root.py index a41c50009..a5dd3b9e2 100644 --- a/git/objects/submodule/root.py +++ b/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 diff --git a/git/objects/submodule/util.py b/git/objects/submodule/util.py index c021510d8..9856e39ce 100644 --- a/git/objects/submodule/util.py +++ b/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 diff --git a/test/test_submodule.py b/test/test_submodule.py index 45b285c55..edb3db84c 100644 --- a/test/test_submodule.py +++ b/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,93 @@ 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.git.symbolic_ref("HEAD", "refs/heads/master") + 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) + parent.index.commit("Add submodule") + submodule.remove(configuration=False) + 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("url", ["./source", "../source", "./../source"]) +def test_add_preserves_relative_url_on_cygwin(tmp_path, url): + with git.Repo.init(tmp_path / "parent") as parent, git.Repo.init(tmp_path / "source") as source: + source.index.commit("Initial commit") + with mock.patch.object(Submodule, "_clone_repo", return_value=source): + with mock.patch.object(Git, "is_cygwin", return_value=True): + submodule = parent.create_submodule("module", "module", url) + assert submodule.url == url + with submodule.config_reader() as reader: + assert reader.get_value("url") == url + with parent.config_reader() as reader: + assert reader.get_value('submodule "module"', "url") == url + + +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 +2504,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