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.

Author: Byron

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,
@@ -985,7 +980,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 +1730,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":

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:

test/test_submodule.py

@@ -53,6 +53,72 @@ 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.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)
+
+
+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", 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 +2525,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"),