From 833c8d8d3e5347e95f782cf1cc0b9fab60569110 Mon Sep 17 00:00:00 2001 From: Keerthana KT Date: Fri, 16 Oct 2026 14:03:21 +0530 Subject: [PATCH 1/2] fix(submodule): resolve relative submodule URLs the way git does `Submodule.update` resolved a `.gitmodules` URL starting with `./` or `../` with `urllib.parse.urljoin` against the superproject's `origin` URL, and `Submodule._clone_repo` appended a `../` URL to the tracking remote's URL with `os.path.join`. Neither follows git's `relative_url`, and `urljoin` only resolves against the schemes it knows: for an `ssh://` or `git://` origin it returns the relative URL unchanged, and for an scp-like `host:path` origin more than one `../` collapses to a bare path. `git clone` then received `./planted`, a path inside the superproject's checkout, where git resolves the same entry to `ssh://host/org/super.git/planted`. A submodule was therefore cloned from a directory committed by whoever authored the superproject, through git's local clone path: `git upload-pack` ran inside that directory and its objects were copied without the `transfer.fsckObjects` checks a transport fetch applies (an object that `git clone file://...` rejects as `badDate` is accepted this way). Relative submodule URLs are common in multi-repository setups, and the superproject is untrusted input to any service that updates submodules of repositories it is given. Add `resolve_relative_url`, a port of git's `relative_url`: each leading `../` strips one component of the remote URL, a path component first, then the scp-like host before the colon, and running out of components is an error; `./` prefixes are dropped and the rest is appended. It agrees with what `git submodule init` records over 272 remote and URL pairs, including the error cases. Use it in `Submodule.update`, `Submodule._clone_repo` (which now also resolves `./` URLs, previously passed to `git clone` verbatim), and the URL change path of `RootModule.update`, which created a remote from the raw relative URL and fetched it as a path relative to the module's working tree. The remote used for resolution is unchanged: `origin` in `update`, the tracking branch's remote in `_clone_repo`. Adds regression tests in `test/test_submodule.py`; the two behavioral ones fail before the change with `./planted` reaching the clone unchanged. --- git/objects/submodule/base.py | 20 +++++--- git/objects/submodule/root.py | 12 +++-- git/objects/submodule/util.py | 94 +++++++++++++++++++++++++++++++++++ test/test_submodule.py | 63 +++++++++++++++++++++++ 4 files changed, 177 insertions(+), 12 deletions(-) diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index 74fbd39fe..d17404ea3 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, ) @@ -428,10 +429,10 @@ def _clone_repo( if not osp.isdir(module_abspath_dir): os.makedirs(module_abspath_dir) - if url.startswith("../"): + if is_relative_url(url): 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) + url = resolve_relative_url(repo_remote_url, url) clone = git.Repo.clone_from( url, @@ -982,13 +983,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._resolved_url(), self.path, self.name, n=True, @@ -1738,6 +1735,13 @@ def url(self) -> str: """:return: The url to the repository our submodule's repository refers to""" return self._url + def _resolved_url(self) -> str: + """:return: :attr:`url`, with a ``./`` or ``../`` prefix resolved against the + url of the superproject's ``origin`` remote the way git does""" + if not is_relative_url(self.url): + return self.url + return resolve_relative_url(self.repo.remotes.origin.url, self.url) + @property def parent_commit(self) -> "Commit": """ diff --git a/git/objects/submodule/root.py b/git/objects/submodule/root.py index a41c50009..047f15727 100644 --- a/git/objects/submodule/root.py +++ b/git/objects/submodule/root.py @@ -264,10 +264,14 @@ 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() + psm_url = psm._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, @@ -278,7 +282,7 @@ def update( # type: ignore[override] if not dry_run: 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..97d51d6a0 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 @@ -2416,3 +2417,65 @@ 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(Submodule, "_clone_repo", side_effect=AssertionError("stop")) as clone_repo: + with pytest.raises(AssertionError, match="stop"): + submodule.update(init=True) + assert clone_repo.call_args.args[1] == 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:org/super.git") + parent.head.reference.set_tracking_branch(git.RemoteReference(parent, "refs/remotes/origin/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 From 580fd90daa7f8df9cf5addb76e3fa61642434efb Mon Sep 17 00:00:00 2001 From: Byron Date: Fri, 16 Oct 2026 11:18:29 +0200 Subject: [PATCH 2/2] review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - [P2] Fall back when a relative URL has no tracking remote — git/objects/submodule/base.py:432-435 For a locally initialized superproject without a tracking branch, `create_submodule(..., url='./source')` now enters this block and dereferences `None`, raising `AttributeError` even when `./source` is a valid repository. This addition succeeded before the patch; detached checkouts also fail at `active_branch`. Handle missing tracking branches by falling back to `origin` or the superproject directory, as Git does. - [P2] Resolve relative submodule URLs only once — git/objects/submodule/base.py:986-988 When `origin` is a local relative URL such as `../repos/super.git` and the branch tracks that remote, `_resolved_url()` turns `../child.git` into `../repos/child.git`. Because that result still starts with `../`, `_clone_repo()` resolves it again, producing `../repos/repos/child.git`. This can fail cloning or select the wrong repository. Ensure URLs already resolved by `update()` bypass the second resolution. - [P2] Defer old-URL resolution until a remote change is needed — git/objects/submodule/root.py:269-270 For an initialized superproject without `origin`, changing `.gitmodules` from `../source` to the absolute URL already used by the submodule's remote now raises `AttributeError` in `psm._resolved_url()`. Previously, the existing-remote check skipped URL handling and the update completed. Resolve the previous URL only after checking whether the new URL already matches a remote, so updates requiring no remote adjustment do not fail. --- git/objects/submodule/base.py | 39 ++++++++------ git/objects/submodule/root.py | 2 +- test/test_submodule.py | 96 +++++++++++++++++++++++++++++++++-- 3 files changed, 117 insertions(+), 20 deletions(-) diff --git a/git/objects/submodule/base.py b/git/objects/submodule/base.py index d17404ea3..6660afefd 100644 --- a/git/objects/submodule/base.py +++ b/git/objects/submodule/base.py @@ -69,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 # ----------------------------------------------------------------------------- @@ -429,13 +429,8 @@ def _clone_repo( if not osp.isdir(module_abspath_dir): os.makedirs(module_abspath_dir) - if is_relative_url(url): - remote_name = cast("RemoteReference", repo.active_branch.tracking_branch()).remote_name - repo_remote_url = repo.remote(remote_name).url - url = resolve_relative_url(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, @@ -747,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: @@ -985,7 +981,7 @@ def fetch_remotes(module_repo: "Repo") -> None: raise ValueError("Missing module at %r but fetching is disabled" % self.path) from None mrepo = self._clone_repo( self.repo, - self._resolved_url(), + self.url, self.path, self.name, n=True, @@ -1735,12 +1731,25 @@ 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`, with a ``./`` or ``../`` prefix resolved against the - url of the superproject's ``origin`` remote the way git does""" - if not is_relative_url(self.url): - return self.url - return resolve_relative_url(self.repo.remotes.origin.url, self.url) + """: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 047f15727..a5dd3b9e2 100644 --- a/git/objects/submodule/root.py +++ b/git/objects/submodule/root.py @@ -267,7 +267,6 @@ def update( # type: ignore[override] # Relative urls are resolved against the superproject's remote, # which is what the module's remotes hold. sm_url = sm._resolved_url() - psm_url = psm._resolved_url() # Don't do anything if we already have the url we search in # place. @@ -280,6 +279,7 @@ 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: diff --git a/test/test_submodule.py b/test/test_submodule.py index 97d51d6a0..edb3db84c 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -53,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%"] ) @@ -2459,17 +2546,18 @@ def test_update_resolves_relative_url_against_remote(self, rwdir): ("../planted", "ssh://git@host/org/planted"), ]: submodule = Submodule(parent, Submodule.NULL_BIN_SHA, name="module", path="module", url=url) - with mock.patch.object(Submodule, "_clone_repo", side_effect=AssertionError("stop")) as clone_repo: + 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_repo.call_args.args[1] == expected + 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:org/super.git") - parent.head.reference.set_tracking_branch(git.RemoteReference(parent, "refs/remotes/origin/main")) + 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"),