Repository navigation
Commit 0a5938d
committed
review
It's basically a rubber-stamp of the review comments, hoping this code
will be nothing more than interesting tests for the `gix-backend` branch,
so the implementation here won't matter.
<!-- agent -->
- [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.1 parent 833c8d8 commit 0a5938d
3 files changed
Lines changed: 196 additions & 49 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -56,6 +56,8 @@ | |||
| 56 | 56 | ||
| 57 | 57 | from .util import ( | |
| 58 | 58 | SubmoduleConfigParser, | |
| 59 | + _is_absolute_path, | ||
| 60 | + _url_is_local_not_ssh, | ||
| 59 | 61 | find_first_remote_branch, | |
| 60 | 62 | is_relative_url, | |
| 61 | 63 | mkhead, | |
@@ -69,7 +71,7 @@ | |||
| 69 | 71 | if TYPE_CHECKING: | |
| 70 | 72 | from git.index import IndexFile | |
| 71 | 73 | from git.objects.commit import Commit | |
| 72 | - from git.refs import Head, RemoteReference | ||
| 74 | + from git.refs import Head | ||
| 73 | 75 | from git.repo import Repo | |
| 74 | 76 | ||
| 75 | 77 | # ----------------------------------------------------------------------------- | |
@@ -429,13 +431,8 @@ def _clone_repo( | |||
| 429 | 431 | if not osp.isdir(module_abspath_dir): | |
| 430 | 432 | os.makedirs(module_abspath_dir) | |
| 431 | 433 | ||
| 432 | - if is_relative_url(url): | ||
| 433 | - remote_name = cast("RemoteReference", repo.active_branch.tracking_branch()).remote_name | ||
| 434 | - repo_remote_url = repo.remote(remote_name).url | ||
| 435 | - url = resolve_relative_url(repo_remote_url, url) | ||
| 436 | - | ||
| 437 | 434 | clone = git.Repo.clone_from( | |
| 438 | - url, | ||
| 435 | + cls._resolve_url(repo, url), | ||
| 439 | 436 | module_checkout_path, | |
| 440 | 437 | allow_unsafe_options=allow_unsafe_options, | |
| 441 | 438 | allow_unsafe_protocols=allow_unsafe_protocols, | |
@@ -747,8 +744,9 @@ def add( | |||
| 747 | 744 | ) | |
| 748 | 745 | # END verify url | |
| 749 | 746 | ||
| 750 | - ## See #525 for ensuring git URLs in config-files are valid under Windows. | ||
| 751 | - url = Git.polish_url(url, expand_vars=False) | ||
| 747 | + # Relative submodule URLs must keep their prefix to resolve against the parent remote. | ||
| 748 | + if not is_relative_url(url): | ||
| 749 | + url = Git.polish_url(url, expand_vars=False) | ||
| 752 | 750 | ||
| 753 | 751 | # It's important to add the URL to the parent config, to let `git submodule` know. | |
| 754 | 752 | # Otherwise there is a '-' character in front of the submodule listing: | |
@@ -985,7 +983,7 @@ def fetch_remotes(module_repo: "Repo") -> None: | |||
| 985 | 983 | raise ValueError("Missing module at %r but fetching is disabled" % self.path) from None | |
| 986 | 984 | mrepo = self._clone_repo( | |
| 987 | 985 | self.repo, | |
| 988 | - self._resolved_url(), | ||
| 986 | + self.url, | ||
| 989 | 987 | self.path, | |
| 990 | 988 | self.name, | |
| 991 | 989 | n=True, | |
@@ -1735,12 +1733,30 @@ def url(self) -> str: | |||
| 1735 | 1733 | """:return: The url to the repository our submodule's repository refers to""" | |
| 1736 | 1734 | return self._url | |
| 1737 | 1735 | ||
| 1736 | + @staticmethod | ||
| 1737 | + def _resolve_url(repo: "Repo", url: str) -> str: | ||
| 1738 | + """Resolve relative URLs against the tracking remote, then ``origin``, | ||
| 1739 | + falling back to the superproject's directory the way git does.""" | ||
| 1740 | + if not is_relative_url(url): | ||
| 1741 | + return url | ||
| 1742 | + remote_name = "origin" | ||
| 1743 | + if not repo.head.is_detached: | ||
| 1744 | + with repo.active_branch.config_reader() as reader: | ||
| 1745 | + remote_name = reader.get("remote", fallback="origin") | ||
| 1746 | + try: | ||
| 1747 | + remote_url = repo.remote(remote_name).url | ||
| 1748 | + except ValueError: | ||
| 1749 | + remote_url = os.fspath(repo.working_dir) | ||
| 1750 | + if remote_url == ".": | ||
| 1751 | + remote_url = os.fspath(repo.working_dir) | ||
| 1752 | + resolved = resolve_relative_url(remote_url, url) | ||
| 1753 | + if resolved and _url_is_local_not_ssh(resolved) and not _is_absolute_path(resolved): | ||
| 1754 | + resolved = os.path.join(repo.working_dir, resolved) | ||
| 1755 | + return resolved | ||
| 1756 | + | ||
| 1738 | 1757 | def _resolved_url(self) -> str: | |
| 1739 | - """:return: :attr:`url`, with a ``./`` or ``../`` prefix resolved against the | ||
| 1740 | - url of the superproject's ``origin`` remote the way git does""" | ||
| 1741 | - if not is_relative_url(self.url): | ||
| 1742 | - return self.url | ||
| 1743 | - return resolve_relative_url(self.repo.remotes.origin.url, self.url) | ||
| 1758 | + """:return: :attr:`url` resolved against the superproject's default remote""" | ||
| 1759 | + return self._resolve_url(self.repo, self.url) | ||
| 1744 | 1760 | ||
| 1745 | 1761 | @property | |
| 1746 | 1762 | def parent_commit(self) -> "Commit": | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -267,7 +267,6 @@ def update( # type: ignore[override] | |||
| 267 | 267 | # Relative urls are resolved against the superproject's remote, | |
| 268 | 268 | # which is what the module's remotes hold. | |
| 269 | 269 | sm_url = sm._resolved_url() | |
| 270 | - psm_url = psm._resolved_url() | ||
| 271 | 270 | ||
| 272 | 271 | # Don't do anything if we already have the url we search in | |
| 273 | 272 | # place. | |
@@ -281,28 +280,21 @@ def update( # type: ignore[override] | |||
| 281 | 280 | ||
| 282 | 281 | if not dry_run: | |
| 283 | 282 | previous_remote = None | |
| 284 | - for remote in rmts: | ||
| 285 | - if remote.url == psm_url: | ||
| 286 | - previous_remote = remote | ||
| 287 | - break | ||
| 288 | - # END if urls match | ||
| 289 | - # END for each remote | ||
| 283 | + if len(rmts) == 1: | ||
| 284 | + previous_remote = rmts[0] | ||
| 285 | + else: | ||
| 286 | + psm_url = psm._resolved_url() | ||
| 287 | + for remote in rmts: | ||
| 288 | + if remote.url == psm_url: | ||
| 289 | + previous_remote = remote | ||
| 290 | + break | ||
| 291 | + # END if urls match | ||
| 292 | + # END for each remote | ||
| 290 | 293 | ||
| 291 | - # If we didn't find a matching remote, but have exactly | ||
| 292 | - # one, we can safely use this one. | ||
| 293 | 294 | if previous_remote is None: | |
| 294 | - if len(rmts) == 1: | ||
| 295 | - previous_remote = rmts[0] | ||
| 296 | - else: | ||
| 297 | - # If we have not found any remote with the | ||
| 298 | - # original URL we may not have a name. This is a | ||
| 299 | - # special case, and its okay to fail here. | ||
| 300 | - # Alternatively we could just generate a unique | ||
| 301 | - # name and leave all existing ones in place. | ||
| 302 | - raise InvalidGitRepositoryError( | ||
| 303 | - "Couldn't find original remote-repo at url %r" % psm.url | ||
| 304 | - ) | ||
| 305 | - # END handle one single remote | ||
| 295 | + raise InvalidGitRepositoryError( | ||
| 296 | + "Couldn't find original remote-repo at url %r" % psm.url | ||
| 297 | + ) | ||
| 306 | 298 | # END handle check we found a remote | |
| 307 | 299 | ||
| 308 | 300 | if no_fetch: | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -53,6 +53,138 @@ def _patch_git_config(name, value): | |||
| 53 | 53 | yield | |
| 54 | 54 | ||
| 55 | 55 | ||
| 56 | + @pytest.mark.parametrize("state", ["unborn", "untracked", "detached"]) | ||
| 57 | + @pytest.mark.parametrize("remote", [None, ".", "origin"], ids=["working-tree", "local-origin", "origin"]) | ||
| 58 | + def test_add_relative_submodule_without_tracking_remote(tmp_path, state, remote): | ||
| 59 | + with git.Repo.init(tmp_path / "parent") as parent: | ||
| 60 | + if state != "unborn": | ||
| 61 | + parent.index.commit("Initial commit") | ||
| 62 | + if state == "detached": | ||
| 63 | + parent.head.reference = parent.head.commit | ||
| 64 | + base = tmp_path / "remote" if remote == "origin" else tmp_path / "parent" | ||
| 65 | + if remote is not None: | ||
| 66 | + parent.create_remote("origin", str(base) if remote == "origin" else ".") | ||
| 67 | + with git.Repo.init(base / "source") as source: | ||
| 68 | + source.git.symbolic_ref("HEAD", "refs/heads/master") | ||
| 69 | + source.index.commit("Initial commit") | ||
| 70 | + submodule = parent.create_submodule("module", "module", "./source") | ||
| 71 | + assert submodule.url == "./source" | ||
| 72 | + with submodule.module() as module: | ||
| 73 | + assert module.head.commit == source.head.commit | ||
| 74 | + assert Path(module.remotes.origin.url).samefile(source.working_tree_dir) | ||
| 75 | + parent.index.commit("Add submodule") | ||
| 76 | + submodule.remove(configuration=False) | ||
| 77 | + submodule.update(init=True) | ||
| 78 | + with submodule.module() as module: | ||
| 79 | + assert module.head.commit == source.head.commit | ||
| 80 | + assert Path(module.remotes.origin.url).samefile(source.working_tree_dir) | ||
| 81 | + | ||
| 82 | + | ||
| 83 | + @pytest.mark.parametrize("url", ["./source", "../source", "./../source"]) | ||
| 84 | + def test_add_preserves_relative_url_on_cygwin(tmp_path, url): | ||
| 85 | + with git.Repo.init(tmp_path / "parent") as parent, git.Repo.init(tmp_path / "source") as source: | ||
| 86 | + source.index.commit("Initial commit") | ||
| 87 | + with mock.patch.object(Submodule, "_clone_repo", return_value=source): | ||
| 88 | + with mock.patch.object(Git, "is_cygwin", return_value=True): | ||
| 89 | + submodule = parent.create_submodule("module", "module", url) | ||
| 90 | + assert submodule.url == url | ||
| 91 | + with submodule.config_reader() as reader: | ||
| 92 | + assert reader.get_value("url") == url | ||
| 93 | + with parent.config_reader() as reader: | ||
| 94 | + assert reader.get_value('submodule "module"', "url") == url | ||
| 95 | + | ||
| 96 | + | ||
| 97 | + def test_update_resolves_local_relative_remote_once(tmp_path): | ||
| 98 | + with git.Repo.init(tmp_path / "parent") as parent, git.Repo.init(tmp_path / "repos" / "child.git") as source: | ||
| 99 | + source.git.symbolic_ref("HEAD", "refs/heads/master") | ||
| 100 | + source.index.commit("Initial commit") | ||
| 101 | + parent.create_remote("origin", "../repos/super.git") | ||
| 102 | + parent.head.reference.set_tracking_branch(git.RemoteReference(parent, "refs/remotes/origin/master")) | ||
| 103 | + submodule = Submodule( | ||
| 104 | + parent, | ||
| 105 | + source.head.commit.binsha, | ||
| 106 | + name="module", | ||
| 107 | + path="module", | ||
| 108 | + url="../child.git", | ||
| 109 | + branch_path="refs/heads/master", | ||
| 110 | + ) | ||
| 111 | + submodule.update(init=True) | ||
| 112 | + with submodule.module() as module: | ||
| 113 | + assert module.head.commit == source.head.commit | ||
| 114 | + assert Path(module.remotes.origin.url).samefile(source.working_tree_dir) | ||
| 115 | + module.remotes.origin.fetch() | ||
| 116 | + | ||
| 117 | + | ||
| 118 | + @pytest.mark.parametrize("no_fetch", [False, True]) | ||
| 119 | + def test_root_update_anchors_relative_remote_url(tmp_path, no_fetch): | ||
| 120 | + with git.Repo.init(tmp_path / "repos" / "source.git") as source, git.Repo.init(tmp_path / "parent") as parent: | ||
| 121 | + source.git.symbolic_ref("HEAD", "refs/heads/master") | ||
| 122 | + source.index.commit("Initial commit") | ||
| 123 | + with git.Repo.clone_from(source.working_tree_dir, tmp_path / "repos" / "replacement.git") as replacement: | ||
| 124 | + parent.create_remote("origin", "../repos/super.git") | ||
| 125 | + submodule = parent.create_submodule("module", "module", source.working_tree_dir) | ||
| 126 | + with submodule.config_writer() as writer: | ||
| 127 | + writer.set_value("url", "../source.git") | ||
| 128 | + previous = parent.index.commit("Record relative URL") | ||
| 129 | + with submodule.config_writer() as writer: | ||
| 130 | + writer.set_value("url", "../replacement.git") | ||
| 131 | + parent.index.commit("Change relative URL") | ||
| 132 | + | ||
| 133 | + RootModule(parent).update(previous_commit=previous, no_fetch=no_fetch) | ||
| 134 | + | ||
| 135 | + with submodule.module() as module: | ||
| 136 | + assert Path(module.remotes.origin.url).samefile(replacement.working_tree_dir) | ||
| 137 | + assert module.head.commit == replacement.head.commit | ||
| 138 | + module.remotes.origin.fetch() | ||
| 139 | + | ||
| 140 | + | ||
| 141 | + @pytest.mark.parametrize("no_fetch", [False, True]) | ||
| 142 | + def test_root_update_uses_single_remote_without_resolving_old_url(tmp_path, no_fetch): | ||
| 143 | + with git.Repo.init(tmp_path / "source") as source, git.Repo.init(tmp_path / "parent") as parent: | ||
| 144 | + source.git.symbolic_ref("HEAD", "refs/heads/master") | ||
| 145 | + source.index.commit("Initial commit") | ||
| 146 | + with git.Repo.clone_from(source.working_tree_dir, tmp_path / "replacement") as replacement: | ||
| 147 | + parent.create_remote("origin", "../super.git") | ||
| 148 | + submodule = parent.create_submodule("module", "module", source.working_tree_dir) | ||
| 149 | + with submodule.config_writer() as writer: | ||
| 150 | + writer.set_value("url", "../../lib.git") | ||
| 151 | + previous = parent.index.commit("Record historical URL") | ||
| 152 | + with submodule.config_writer() as writer: | ||
| 153 | + writer.set_value("url", replacement.working_tree_dir) | ||
| 154 | + parent.index.commit("Replace historical URL") | ||
| 155 | + | ||
| 156 | + RootModule(parent).update(previous_commit=previous, no_fetch=no_fetch) | ||
| 157 | + | ||
| 158 | + with submodule.module() as module: | ||
| 159 | + assert Path(module.remotes.origin.url).samefile(replacement.working_tree_dir) | ||
| 160 | + assert module.head.commit == replacement.head.commit | ||
| 161 | + | ||
| 162 | + | ||
| 163 | + @pytest.mark.parametrize("dry_run", [False, True]) | ||
| 164 | + def test_root_update_skips_old_url_resolution_when_remote_matches(tmp_path, dry_run): | ||
| 165 | + with git.Repo.init(tmp_path / "source") as source, git.Repo.init(tmp_path / "parent") as parent: | ||
| 166 | + source.index.commit("Initial commit") | ||
| 167 | + submodule = parent.create_submodule("module", "module", source.working_tree_dir) | ||
| 168 | + with submodule.config_writer() as writer: | ||
| 169 | + writer.set_value("url", "../source") | ||
| 170 | + previous = parent.index.commit("Record relative submodule URL") | ||
| 171 | + with submodule.config_writer() as writer: | ||
| 172 | + writer.set_value("url", Git.polish_url(source.working_tree_dir)) | ||
| 173 | + parent.index.commit("Use existing remote URL") | ||
| 174 | + | ||
| 175 | + resolve_url = Submodule._resolved_url | ||
| 176 | + | ||
| 177 | + def resolve_current_url(sm): | ||
| 178 | + assert sm.url != "../source", "Previous URL does not need resolving" | ||
| 179 | + return resolve_url(sm) | ||
| 180 | + | ||
| 181 | + with mock.patch.object(Submodule, "_resolved_url", autospec=True, side_effect=resolve_current_url): | ||
| 182 | + RootModule(parent).update(previous_commit=previous, no_fetch=True, dry_run=dry_run) | ||
| 183 | + with submodule.module() as module: | ||
| 184 | + assert module.remotes.origin.url == Git.polish_url(source.working_tree_dir) | ||
| 185 | + assert module.head.commit == source.head.commit | ||
| 186 | + | ||
| 187 | + | ||
| 56 | 188 | @pytest.mark.parametrize( | |
| 57 | 189 | "name", ["module", "$GITPYTHON_TEST_SECRET", "prefix-${GITPYTHON_TEST_SECRET}-suffix", "%GITPYTHON_TEST_SECRET%"] | |
| 58 | 190 | ) | |
@@ -2437,6 +2569,9 @@ def test_resolve_relative_url_like_git(self): | |||
| 2437 | 2569 | ("//share/super", "../../sub", "//sub"), | |
| 2438 | 2570 | ("foo/bar", "../sub", "foo/sub"), | |
| 2439 | 2571 | ("./foo", "../sub", "sub"), | |
| 2572 | + ("./super.git", "../", ""), | ||
| 2573 | + ("ssh://host/org/super.git", "./", "ssh://host/org/super.git/"), | ||
| 2574 | + ("ssh://host/org/super.git", "../", "ssh://host/org/"), | ||
| 2440 | 2575 | ("../foo/bar", "../sub", "../foo/sub"), | |
| 2441 | 2576 | ("ssh://host/org/super.git", ".sub", ".sub"), | |
| 2442 | 2577 | ("ssh://host/org/super.git", "https://other/sub", "https://other/sub"), | |
@@ -2459,23 +2594,27 @@ def test_update_resolves_relative_url_against_remote(self, rwdir): | |||
| 2459 | 2594 | ("../planted", "ssh://git@host/org/planted"), | |
| 2460 | 2595 | ]: | |
| 2461 | 2596 | submodule = Submodule(parent, Submodule.NULL_BIN_SHA, name="module", path="module", url=url) | |
| 2462 | - with mock.patch.object(Submodule, "_clone_repo", side_effect=AssertionError("stop")) as clone_repo: | ||
| 2597 | + with mock.patch.object(git.Repo, "clone_from", side_effect=AssertionError("stop")) as clone_from: | ||
| 2463 | 2598 | with pytest.raises(AssertionError, match="stop"): | |
| 2464 | 2599 | submodule.update(init=True) | |
| 2465 | - assert clone_repo.call_args.args[1] == expected | ||
| 2600 | + assert clone_from.call_args.args[0] == expected | ||
| 2466 | 2601 | ||
| 2467 | 2602 | @with_rw_directory | |
| 2468 | 2603 | def test_clone_repo_resolves_relative_url_against_tracking_remote(self, rwdir): | |
| 2469 | 2604 | parent = git.Repo.init(osp.join(rwdir, "parent")) | |
| 2470 | 2605 | parent.index.commit("initial") | |
| 2471 | - parent.create_remote("origin", "git@host:org/super.git") | ||
| 2472 | - parent.head.reference.set_tracking_branch(git.RemoteReference(parent, "refs/remotes/origin/main")) | ||
| 2473 | - | ||
| 2474 | - for url, expected in [ | ||
| 2475 | - ("./planted", "git@host:org/super.git/planted"), | ||
| 2476 | - ("../planted", "git@host:org/planted"), | ||
| 2477 | - ]: | ||
| 2478 | - with mock.patch.object(git.Repo, "clone_from", side_effect=AssertionError("stop")) as clone_from: | ||
| 2479 | - with pytest.raises(AssertionError, match="stop"): | ||
| 2480 | - Submodule._clone_repo(parent, url, "module", "module") | ||
| 2481 | - assert clone_from.call_args.args[0] == expected | ||
| 2606 | + parent.create_remote("origin", "git@host:other/super.git") | ||
| 2607 | + for remote_name in ("upstream", "team/upstream"): | ||
| 2608 | + parent.create_remote(remote_name, "git@host:org/super.git") | ||
| 2609 | + with parent.head.reference.config_writer() as writer: | ||
| 2610 | + writer.set_value("remote", remote_name) | ||
| 2611 | + writer.set_value("merge", "refs/heads/main") | ||
| 2612 | + | ||
| 2613 | + for url, expected in [ | ||
| 2614 | + ("./planted", "git@host:org/super.git/planted"), | ||
| 2615 | + ("../planted", "git@host:org/planted"), | ||
| 2616 | + ]: | ||
| 2617 | + with mock.patch.object(git.Repo, "clone_from", side_effect=AssertionError("stop")) as clone_from: | ||
| 2618 | + with pytest.raises(AssertionError, match="stop"): | ||
| 2619 | + Submodule._clone_repo(parent, url, "module", "module") | ||
| 2620 | + assert clone_from.call_args.args[0] == expected | ||
0 commit comments