Skip to content

Commit 580fd90

Browse files
committed
review
<!-- 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 580fd90

3 files changed

Lines changed: 117 additions & 20 deletions

File tree

‎git/objects/submodule/base.py‎

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@
6969
if TYPE_CHECKING:
7070
from git.index import IndexFile
7171
from git.objects.commit import Commit
72-
from git.refs import Head, RemoteReference
72+
from git.refs import Head
7373
from git.repo import Repo
7474

7575
# -----------------------------------------------------------------------------
@@ -429,13 +429,8 @@ def _clone_repo(
429429
if not osp.isdir(module_abspath_dir):
430430
os.makedirs(module_abspath_dir)
431431

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-
437432
clone = git.Repo.clone_from(
438-
url,
433+
cls._resolve_url(repo, url),
439434
module_checkout_path,
440435
allow_unsafe_options=allow_unsafe_options,
441436
allow_unsafe_protocols=allow_unsafe_protocols,
@@ -747,8 +742,9 @@ def add(
747742
)
748743
# END verify url
749744

750-
## See #525 for ensuring git URLs in config-files are valid under Windows.
751-
url = Git.polish_url(url, expand_vars=False)
745+
# Relative submodule URLs must keep their prefix to resolve against the parent remote.
746+
if not is_relative_url(url):
747+
url = Git.polish_url(url, expand_vars=False)
752748

753749
# It's important to add the URL to the parent config, to let `git submodule` know.
754750
# Otherwise there is a '-' character in front of the submodule listing:
@@ -985,7 +981,7 @@ def fetch_remotes(module_repo: "Repo") -> None:
985981
raise ValueError("Missing module at %r but fetching is disabled" % self.path) from None
986982
mrepo = self._clone_repo(
987983
self.repo,
988-
self._resolved_url(),
984+
self.url,
989985
self.path,
990986
self.name,
991987
n=True,
@@ -1735,12 +1731,25 @@ def url(self) -> str:
17351731
""":return: The url to the repository our submodule's repository refers to"""
17361732
return self._url
17371733

1734+
@staticmethod
1735+
def _resolve_url(repo: "Repo", url: str) -> str:
1736+
"""Resolve relative URLs against the tracking remote, then ``origin``,
1737+
falling back to the superproject's directory the way git does."""
1738+
if not is_relative_url(url):
1739+
return url
1740+
tracking_branch = None if repo.head.is_detached else repo.active_branch.tracking_branch()
1741+
remote_name = tracking_branch.remote_name if tracking_branch is not None else "origin"
1742+
try:
1743+
remote_url = repo.remote(remote_name).url
1744+
except ValueError:
1745+
remote_url = os.fspath(repo.working_dir)
1746+
if remote_url == ".":
1747+
remote_url = os.fspath(repo.working_dir)
1748+
return resolve_relative_url(remote_url, url)
1749+
17381750
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)
1751+
""":return: :attr:`url` resolved against the superproject's default remote"""
1752+
return self._resolve_url(self.repo, self.url)
17441753

17451754
@property
17461755
def parent_commit(self) -> "Commit":

‎git/objects/submodule/root.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,6 @@ def update( # type: ignore[override]
267267
# Relative urls are resolved against the superproject's remote,
268268
# which is what the module's remotes hold.
269269
sm_url = sm._resolved_url()
270-
psm_url = psm._resolved_url()
271270

272271
# Don't do anything if we already have the url we search in
273272
# place.
@@ -280,6 +279,7 @@ def update( # type: ignore[override]
280279
)
281280

282281
if not dry_run:
282+
psm_url = psm._resolved_url()
283283
previous_remote = None
284284
for remote in rmts:
285285
if remote.url == psm_url:

‎test/test_submodule.py‎

Lines changed: 92 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,93 @@ def _patch_git_config(name, value):
5353
yield
5454

5555

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+
with cwd(parent.working_tree_dir):
112+
submodule.update(init=True)
113+
with submodule.module() as module:
114+
assert module.head.commit == source.head.commit
115+
assert Path(module.remotes.origin.url).samefile(source.working_tree_dir)
116+
117+
118+
@pytest.mark.parametrize("dry_run", [False, True])
119+
def test_root_update_skips_old_url_resolution_when_remote_matches(tmp_path, dry_run):
120+
with git.Repo.init(tmp_path / "source") as source, git.Repo.init(tmp_path / "parent") as parent:
121+
source.index.commit("Initial commit")
122+
submodule = parent.create_submodule("module", "module", source.working_tree_dir)
123+
with submodule.config_writer() as writer:
124+
writer.set_value("url", "../source")
125+
previous = parent.index.commit("Record relative submodule URL")
126+
with submodule.config_writer() as writer:
127+
writer.set_value("url", Git.polish_url(source.working_tree_dir))
128+
parent.index.commit("Use existing remote URL")
129+
130+
resolve_url = Submodule._resolved_url
131+
132+
def resolve_current_url(sm):
133+
assert sm.url != "../source", "Previous URL does not need resolving"
134+
return resolve_url(sm)
135+
136+
with mock.patch.object(Submodule, "_resolved_url", autospec=True, side_effect=resolve_current_url):
137+
RootModule(parent).update(previous_commit=previous, no_fetch=True, dry_run=dry_run)
138+
with submodule.module() as module:
139+
assert module.remotes.origin.url == Git.polish_url(source.working_tree_dir)
140+
assert module.head.commit == source.head.commit
141+
142+
56143
@pytest.mark.parametrize(
57144
"name", ["module", "$GITPYTHON_TEST_SECRET", "prefix-${GITPYTHON_TEST_SECRET}-suffix", "%GITPYTHON_TEST_SECRET%"]
58145
)
@@ -2459,17 +2546,18 @@ def test_update_resolves_relative_url_against_remote(self, rwdir):
24592546
("../planted", "ssh://git@host/org/planted"),
24602547
]:
24612548
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:
2549+
with mock.patch.object(git.Repo, "clone_from", side_effect=AssertionError("stop")) as clone_from:
24632550
with pytest.raises(AssertionError, match="stop"):
24642551
submodule.update(init=True)
2465-
assert clone_repo.call_args.args[1] == expected
2552+
assert clone_from.call_args.args[0] == expected
24662553

24672554
@with_rw_directory
24682555
def test_clone_repo_resolves_relative_url_against_tracking_remote(self, rwdir):
24692556
parent = git.Repo.init(osp.join(rwdir, "parent"))
24702557
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"))
2558+
parent.create_remote("origin", "git@host:other/super.git")
2559+
parent.create_remote("upstream", "git@host:org/super.git")
2560+
parent.head.reference.set_tracking_branch(git.RemoteReference(parent, "refs/remotes/upstream/main"))
24732561

24742562
for url, expected in [
24752563
("./planted", "git@host:org/super.git/planted"),

0 commit comments

Comments
 (0)