[PATCH 0/6] submodule: improve remote lookup logic

[PATCH 0/6] submodule: improve remote lookup logic

[Jacob Keller]

This series improves the git submodule remote lookup logic implemented in
submodule--helper.

A few cleanups are done first:

* The starts_with_dot(_dot)_slash helper functions are moved to dir.h for
  re-use, as these are used both within submodule--helper.c and
  submodule-config.c

* Several remote.c helper functions are refactored to take repository
  pointers, enabling use with a submodule repository pointer.

* The branch_release logic is fixed so that it won't dereference
  branch->merge if it was never setup. This appeared to trigger in some
  cases if a submodule repository was passed into the read_config()
  function.

Next, the submodule--helper.c logic in repo_get_default_remote() is
refactored to remote.c helper functions. A new repo_default_remote() helper
function is added which will try to find a default remote. This helper
first tries to look up the remote from the checked out branch, then falls
back to the only remote (if there is exactly one remote) before finally
falling back to "origin".

This improved logic is a good first step, but won't handle cases where
there are multiple remotes, and when remotes have been renamed.

For the final improvement, notice that the parent project already stores
the URL for the submodule in its .git/config or .gitmodules file. This URL
is what we use to set the remote in the first place when cloning.

Add a repo_remote_from_url() helper which will iterate through the remotes
and find the first remote with that URL. Use this in
repo_get_default_remote() to first try and find a remote by its URL. If
unsuccessful, we still keep the original fallback logic, in the off chance
that the user has changed the URL from within the submodule.

This method is more robust as it is less likely that the user has manually
changed the submodule URL within the submodule but not also within the
.git/config.

With this change, all commands which need the submodule remote will first
look up by URL before trying to use the fallback logic, and should now be
able to find a suitable remote regardless of now they are renamed.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Jacob Keller (6):
      dir: move starts_with_dot(_dot)_slash to dir.h
      remote: remove the_repository from some functions
      remote: check branch->merge before access in branch_release
      submodule--helper: improve logic for fallback remote name
      submodule: move get_default_remote_submodule()
      submodule: look up remotes by URL first

 dir.h                       |  23 ++++++++++
 remote.h                    |   3 ++
 builtin/submodule--helper.c | 101 ++++++++++++++++++++++--------------------
 remote.c                    | 104 ++++++++++++++++++++++++++++----------------
 submodule-config.c          |  12 -----
 t/t7406-submodule-update.sh |  61 ++++++++++++++++++++++++++
 6 files changed, 207 insertions(+), 97 deletions(-)
---
base-commit: 4c0e625c091d4c648cec7319bafaed3cc81658e5
change-id: 20250610-jk-submodule-helper-use-url-e55d3c379faf

Best regards,
-- 
Jacob Keller <jacob.keller@gmail.com>

[PATCH 1/6] dir: move starts_with_dot(_dot)_slash to dir.h

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Both submodule--helper.c and submodule-config.c have an implementation
of starts_with_dot_slash and starts_with_dot_dot_slash. The dir.h header
has starts_with_dot(_dot)_slash_native, which sets PATH_MATCH_NATIVE.

Move the helpers to dir.h as static inlines. I thought about renaming
them to postfix with _platform but that felt too long and ugly. On the
other hand it might be slightly confusing with _native.

This simplifies a submodule refactor which wants to use the helpers
earlier in the submodule--helper.c file.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 dir.h                       | 23 +++++++++++++++++++++++
 builtin/submodule--helper.c | 12 ------------
 submodule-config.c          | 12 ------------
 3 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/dir.h b/dir.h
index d7e71aa8daa7d833e4c05e6875b997bc321c6070..fc9be7b427a134e46bcd66c8df42375db47727fc 100644
--- a/dir.h
+++ b/dir.h
@@ -676,4 +676,27 @@ static inline int starts_with_dot_dot_slash_native(const char *const path)
 	return path_match_flags(path, what | PATH_MATCH_NATIVE);
 }
 
+/**
+ * starts_with_dot_slash: convenience wrapper for
+ * patch_match_flags() with PATH_MATCH_STARTS_WITH_DOT_SLASH and
+ * PATH_MATCH_XPLATFORM.
+ */
+static inline int starts_with_dot_slash(const char *const path)
+{
+	const enum path_match_flags what = PATH_MATCH_STARTS_WITH_DOT_SLASH;
+
+	return path_match_flags(path, what | PATH_MATCH_XPLATFORM);
+}
+
+/**
+ * starts_with_dot_dot_slash: convenience wrapper for
+ * patch_match_flags() with PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH and
+ * PATH_MATCH_XPLATFORM.
+ */
+static inline int starts_with_dot_dot_slash(const char *const path)
+{
+	const enum path_match_flags what = PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH;
+
+	return path_match_flags(path, what | PATH_MATCH_XPLATFORM);
+}
 #endif
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 53da2116ddf576bc565b29f043e8b703b8b1563b..9e8cdfe1b2a8c2985d9c1b8ad6f1b0d1f9401714 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -438,18 +438,6 @@ static int module_foreach(int argc, const char **argv, const char *prefix,
 	return ret;
 }
 
-static int starts_with_dot_slash(const char *const path)
-{
-	return path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_SLASH |
-				PATH_MATCH_XPLATFORM);
-}
-
-static int starts_with_dot_dot_slash(const char *const path)
-{
-	return path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH |
-				PATH_MATCH_XPLATFORM);
-}
-
 struct init_cb {
 	const char *prefix;
 	const char *super_prefix;
diff --git a/submodule-config.c b/submodule-config.c
index 8630e27947d3943e1980eb7a53bd41a546842503..d64438b2a18ed2123cc5e18f739539209032d3e9 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -235,18 +235,6 @@ int check_submodule_name(const char *name)
 	return 0;
 }
 
-static int starts_with_dot_slash(const char *const path)
-{
-	return path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_SLASH |
-				PATH_MATCH_XPLATFORM);
-}
-
-static int starts_with_dot_dot_slash(const char *const path)
-{
-	return path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH |
-				PATH_MATCH_XPLATFORM);
-}
-
 static int submodule_url_is_relative(const char *url)
 {
 	return starts_with_dot_slash(url) || starts_with_dot_dot_slash(url);

-- 
2.48.1.397.gec9d649cc640

[PATCH 2/6] remote: remove the_repository from some functions

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

The remotes_remote_get_1 (and its caller, remotes_remote_get, have an
implicit dependency on the_repository due to calling
read_branches_file() and read_remotes_file(), both of which use
the_repository. The branch_get() function calls set_merge() which has an
implicit dependency on the_repository as well.

Because of this use of the_repository, the helper functions cannot be
used in code paths which operate on other repositories. A future
refactor of the submodule--helper will want to make use of some of these
functions.

Refactor to break the dependency by passing struct repository *repo
instead of struct remote_state *remote_state in a few places.

The public callers and many other helper functions still depend on
the_repository. A repo-aware function will be exposed in a following
change for git submodule--helper.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 remote.c | 58 ++++++++++++++++++++++++++++------------------------------
 1 file changed, 28 insertions(+), 30 deletions(-)

diff --git a/remote.c b/remote.c
index 4099183cacdc8a607a8b5eaec86e456b2ef46b48..7092b12209d93e20ce1ed3b7d9e4cbac058c57ff 100644
--- a/remote.c
+++ b/remote.c
@@ -317,11 +317,10 @@ static void warn_about_deprecated_remote_type(const char *type,
 		type, remote->name, remote->name, remote->name);
 }
 
-static void read_remotes_file(struct remote_state *remote_state,
-			      struct remote *remote)
+static void read_remotes_file(struct repository *repo, struct remote *remote)
 {
 	struct strbuf buf = STRBUF_INIT;
-	FILE *f = fopen_or_warn(repo_git_path_append(the_repository, &buf,
+	FILE *f = fopen_or_warn(repo_git_path_append(repo, &buf,
 						     "remotes/%s", remote->name), "r");
 
 	if (!f)
@@ -337,7 +336,7 @@ static void read_remotes_file(struct remote_state *remote_state,
 		strbuf_rtrim(&buf);
 
 		if (skip_prefix(buf.buf, "URL:", &v))
-			add_url_alias(remote_state, remote,
+			add_url_alias(repo->remote_state, remote,
 				      skip_spaces(v));
 		else if (skip_prefix(buf.buf, "Push:", &v))
 			refspec_append(&remote->push, skip_spaces(v));
@@ -350,12 +349,11 @@ static void read_remotes_file(struct remote_state *remote_state,
 	strbuf_release(&buf);
 }
 
-static void read_branches_file(struct remote_state *remote_state,
-			       struct remote *remote)
+static void read_branches_file(struct repository *repo, struct remote *remote)
 {
 	char *frag, *to_free = NULL;
 	struct strbuf buf = STRBUF_INIT;
-	FILE *f = fopen_or_warn(repo_git_path_append(the_repository, &buf,
+	FILE *f = fopen_or_warn(repo_git_path_append(repo, &buf,
 						     "branches/%s", remote->name), "r");
 
 	if (!f)
@@ -382,9 +380,9 @@ static void read_branches_file(struct remote_state *remote_state,
 	if (frag)
 		*(frag++) = '\0';
 	else
-		frag = to_free = repo_default_branch_name(the_repository, 0);
+		frag = to_free = repo_default_branch_name(repo, 0);
 
-	add_url_alias(remote_state, remote, buf.buf);
+	add_url_alias(repo->remote_state, remote, buf.buf);
 	refspec_appendf(&remote->fetch, "refs/heads/%s:refs/heads/%s",
 			frag, remote->name);
 
@@ -681,7 +679,7 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)
 					     branch, explicit);
 }
 
-static struct remote *remotes_remote_get(struct remote_state *remote_state,
+static struct remote *remotes_remote_get(struct repository *repo,
 					 const char *name);
 
 char *remote_ref_for_branch(struct branch *branch, int for_push)
@@ -700,7 +698,7 @@ char *remote_ref_for_branch(struct branch *branch, int for_push)
 					the_repository->remote_state, branch,
 					NULL);
 			struct remote *remote = remotes_remote_get(
-				the_repository->remote_state, remote_name);
+				the_repository, remote_name);
 
 			if (remote && remote->push.nr &&
 			    (dst = apply_refspecs(&remote->push,
@@ -757,10 +755,11 @@ static void validate_remote_url(struct remote *remote)
 }
 
 static struct remote *
-remotes_remote_get_1(struct remote_state *remote_state, const char *name,
+remotes_remote_get_1(struct repository *repo, const char *name,
 		     const char *(*get_default)(struct remote_state *,
 						struct branch *, int *))
 {
+	struct remote_state *remote_state = repo->remote_state;
 	struct remote *ret;
 	int name_given = 0;
 
@@ -774,9 +773,9 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,
 #ifndef WITH_BREAKING_CHANGES
 	if (valid_remote_nick(name) && have_git_dir()) {
 		if (!valid_remote(ret))
-			read_remotes_file(remote_state, ret);
+			read_remotes_file(repo, ret);
 		if (!valid_remote(ret))
-			read_branches_file(remote_state, ret);
+			read_branches_file(repo, ret);
 	}
 #endif /* WITH_BREAKING_CHANGES */
 	if (name_given && !valid_remote(ret))
@@ -790,35 +789,33 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,
 }
 
 static inline struct remote *
-remotes_remote_get(struct remote_state *remote_state, const char *name)
+remotes_remote_get(struct repository *repo, const char *name)
 {
-	return remotes_remote_get_1(remote_state, name,
-				    remotes_remote_for_branch);
+	return remotes_remote_get_1(repo, name, remotes_remote_for_branch);
 }
 
 struct remote *remote_get(const char *name)
 {
 	read_config(the_repository, 0);
-	return remotes_remote_get(the_repository->remote_state, name);
+	return remotes_remote_get(the_repository, name);
 }
 
 struct remote *remote_get_early(const char *name)
 {
 	read_config(the_repository, 1);
-	return remotes_remote_get(the_repository->remote_state, name);
+	return remotes_remote_get(the_repository, name);
 }
 
 static inline struct remote *
-remotes_pushremote_get(struct remote_state *remote_state, const char *name)
+remotes_pushremote_get(struct repository *repo, const char *name)
 {
-	return remotes_remote_get_1(remote_state, name,
-				    remotes_pushremote_for_branch);
+	return remotes_remote_get_1(repo, name, remotes_pushremote_for_branch);
 }
 
 struct remote *pushremote_get(const char *name)
 {
 	read_config(the_repository, 0);
-	return remotes_pushremote_get(the_repository->remote_state, name);
+	return remotes_pushremote_get(the_repository, name);
 }
 
 int remote_is_configured(struct remote *remote, int in_repo)
@@ -1722,7 +1719,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 	}
 }
 
-static void set_merge(struct remote_state *remote_state, struct branch *ret)
+static void set_merge(struct repository *repo, struct branch *ret)
 {
 	struct remote *remote;
 	char *ref;
@@ -1742,7 +1739,7 @@ static void set_merge(struct remote_state *remote_state, struct branch *ret)
 		return;
 	}
 
-	remote = remotes_remote_get(remote_state, ret->remote_name);
+	remote = remotes_remote_get(repo, ret->remote_name);
 
 	CALLOC_ARRAY(ret->merge, ret->merge_nr);
 	for (i = 0; i < ret->merge_nr; i++) {
@@ -1751,7 +1748,7 @@ static void set_merge(struct remote_state *remote_state, struct branch *ret)
 		if (!remote_find_tracking(remote, ret->merge[i]) ||
 		    strcmp(ret->remote_name, "."))
 			continue;
-		if (repo_dwim_ref(the_repository, ret->merge_name[i],
+		if (repo_dwim_ref(repo, ret->merge_name[i],
 				  strlen(ret->merge_name[i]), &oid, &ref,
 				  0) == 1)
 			ret->merge[i]->dst = ref;
@@ -1770,7 +1767,7 @@ struct branch *branch_get(const char *name)
 	else
 		ret = make_branch(the_repository->remote_state, name,
 				  strlen(name));
-	set_merge(the_repository->remote_state, ret);
+	set_merge(the_repository, ret);
 	return ret;
 }
 
@@ -1841,13 +1838,14 @@ static const char *tracking_for_push_dest(struct remote *remote,
 	return ret;
 }
 
-static const char *branch_get_push_1(struct remote_state *remote_state,
+static const char *branch_get_push_1(struct repository *repo,
 				     struct branch *branch, struct strbuf *err)
 {
+	struct remote_state *remote_state = repo->remote_state;
 	struct remote *remote;
 
 	remote = remotes_remote_get(
-		remote_state,
+		repo,
 		remotes_pushremote_for_branch(remote_state, branch, NULL));
 	if (!remote)
 		return error_buf(err,
@@ -1914,7 +1912,7 @@ const char *branch_get_push(struct branch *branch, struct strbuf *err)
 
 	if (!branch->push_tracking_ref)
 		branch->push_tracking_ref = branch_get_push_1(
-			the_repository->remote_state, branch, err);
+			the_repository, branch, err);
 	return branch->push_tracking_ref;
 }
 

-- 
2.48.1.397.gec9d649cc640

[PATCH 3/6] remote: check branch->merge before access in branch_release

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

The branch_release function doesn't check the merge field before
accessing it, based on an assumption that merge will be set if
branch->merge_nr is non-zero.

This is not always the case. It turns out that branch->merge is not
initialized until set_merge() is called, but branch->merge_nr can be
non-zero from handle_config() calling add_merge().

set_merge() is not called until branch_get(). This function does set
merge_nr to zero if merge is not initialized. However, branch_release is
called on every branch when tearing down a repository.

An upcoming change to submodule--helper will initialize the remote state
by calling read_config(). In some cases, this results in branches in the
remote_state which have a non-zero merge_nr but no merge array. This
results in a crash when tearing the repository down.

To fix this, lets simply check if merge is valid before attempting to
release its contents.

This makes it safe to initialize the remote_state for a submodule
repository without crashing on teardown.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 remote.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/remote.c b/remote.c
index 7092b12209d93e20ce1ed3b7d9e4cbac058c57ff..1035f0cd32d034cce05bd2a3d829ec90795ff4e2 100644
--- a/remote.c
+++ b/remote.c
@@ -253,9 +253,11 @@ static void branch_release(struct branch *branch)
 	free((char *)branch->refname);
 	free(branch->remote_name);
 	free(branch->pushremote_name);
-	for (int i = 0; i < branch->merge_nr; i++)
-		refspec_item_clear(branch->merge[i]);
-	free(branch->merge);
+	if (branch->merge) {
+		for (int i = 0; i < branch->merge_nr; i++)
+			refspec_item_clear(branch->merge[i]);
+		free(branch->merge);
+	}
 }
 
 static struct rewrite *make_rewrite(struct rewrites *r,

-- 
2.48.1.397.gec9d649cc640

[PATCH 4/6] submodule--helper: improve logic for fallback remote name

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

The repo_get_default_remote() function in submodule--helper currently
tries to figure out the proper remote name to use for a submodule based
on a few factors.

First, it tries to find the remote for the currently checked out branch.
This works if the submodule is configured to checkout to a branch
instead of a detached HEAD state.

In the detached HEAD state, the code calls back to using "origin", on
the assumption that this is the default remote name. Some users may
change this, such as by setting clone.defaultRemoteName, or by changing
the remote name manually within the submodule repository.

As a first step to improving this situation, refactor to reuse the logic
from remotes_remote_for_branch(). This function uses the remote from the
branch if it has one. If it doesn't then it checks to see if there is
exactly one remote. It uses this remote first before attempting to fall
back to "origin".

To allow using this helper function, introduce a repo_default_remote()
helper to remote.c which takes a repository structure. This helper will
load the remote configuration and get the "HEAD" branch. Then it will
call remotes_remote_for_branch to find the default remote.

This method allows re-using the same existing logic as other flows,
rather than duplicating it in submodule--helper.c.

This isn't a perfect solution for users who change remote names, but it
should help in cases where the remote name is changed but users haven't
added any additional remotes.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 remote.h                    |  2 ++
 builtin/submodule--helper.c | 18 +++---------------
 remote.c                    | 25 ++++++++++++++++++++-----
 t/t7406-submodule-update.sh | 29 +++++++++++++++++++++++++++++
 4 files changed, 54 insertions(+), 20 deletions(-)

diff --git a/remote.h b/remote.h
index 7e4943ae3a70ecefa3332d211084762ca30b59b6..ef0de4aa64e9ccd32cc2eea076c00386dcba1161 100644
--- a/remote.h
+++ b/remote.h
@@ -338,6 +338,8 @@ const char *remote_for_branch(struct branch *branch, int *explicit);
 const char *pushremote_for_branch(struct branch *branch, int *explicit);
 char *remote_ref_for_branch(struct branch *branch, int for_push);
 
+const char *repo_default_remote(struct repository *repo);
+
 /* returns true if the given branch has merge configuration given. */
 int branch_has_merge_config(struct branch *branch);
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9e8cdfe1b2a8c2985d9c1b8ad6f1b0d1f9401714..ef3ff65a80f398c5ac35660288290ad92c7132c7 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -43,8 +43,6 @@ typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
 
 static int repo_get_default_remote(struct repository *repo, char **default_remote)
 {
-	char *dest = NULL;
-	struct strbuf sb = STRBUF_INIT;
 	struct ref_store *store = get_main_ref_store(repo);
 	const char *refname = refs_resolve_ref_unsafe(store, "HEAD", 0, NULL,
 						      NULL);
@@ -52,23 +50,13 @@ static int repo_get_default_remote(struct repository *repo, char **default_remot
 	if (!refname)
 		return die_message(_("No such ref: %s"), "HEAD");
 
-	/* detached HEAD */
-	if (!strcmp(refname, "HEAD")) {
-		*default_remote = xstrdup("origin");
-		return 0;
-	}
-
-	if (!skip_prefix(refname, "refs/heads/", &refname))
+	if (strcmp(refname, "HEAD") &&
+	    !skip_prefix(refname, "refs/heads/", &refname))
 		return die_message(_("Expecting a full ref name, got %s"),
 				   refname);
 
-	strbuf_addf(&sb, "branch.%s.remote", refname);
-	if (repo_config_get_string(repo, sb.buf, &dest))
-		*default_remote = xstrdup("origin");
-	else
-		*default_remote = dest;
+	*default_remote = xstrdup(repo_default_remote(repo));
 
-	strbuf_release(&sb);
 	return 0;
 }
 
diff --git a/remote.c b/remote.c
index 1035f0cd32d034cce05bd2a3d829ec90795ff4e2..fcda185ecfab5102afbe8918fed65c74971ef8c2 100644
--- a/remote.c
+++ b/remote.c
@@ -1759,20 +1759,35 @@ static void set_merge(struct repository *repo, struct branch *ret)
 	}
 }
 
-struct branch *branch_get(const char *name)
+static struct branch *repo_branch_get(struct repository *repo, const char *name)
 {
 	struct branch *ret;
 
-	read_config(the_repository, 0);
+	read_config(repo, 0);
 	if (!name || !*name || !strcmp(name, "HEAD"))
-		ret = the_repository->remote_state->current_branch;
+		ret = repo->remote_state->current_branch;
 	else
-		ret = make_branch(the_repository->remote_state, name,
+		ret = make_branch(repo->remote_state, name,
 				  strlen(name));
-	set_merge(the_repository, ret);
+	set_merge(repo, ret);
 	return ret;
 }
 
+struct branch *branch_get(const char *name)
+{
+	return repo_branch_get(the_repository, name);
+}
+
+const char *repo_default_remote(struct repository *repo)
+{
+	struct branch *branch;
+
+	read_config(repo, 0);
+	branch = repo_branch_get(repo, "HEAD");
+
+	return remotes_remote_for_branch(repo->remote_state, branch, NULL);
+}
+
 int branch_has_merge_config(struct branch *branch)
 {
 	return branch && !!branch->merge;
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index c562bad042ab2d4d0f82cb8b57a1eadbe24044d1..748b529745a5121f121768bb4e0cbc11bc833ea4 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -1134,6 +1134,35 @@ test_expect_success 'setup clean recursive superproject' '
 	git clone --recurse-submodules top top-clean
 '
 
+test_expect_success 'submodule update with renamed remote' '
+	test_when_finished "rm -fr top-cloned" &&
+	cp -r top-clean top-cloned &&
+
+	# Create a commit in each repo, starting with bottom
+	test_commit -C bottom rename_commit &&
+	# Create middle commit
+	git -C middle/bottom fetch &&
+	git -C middle/bottom checkout -f FETCH_HEAD &&
+	git -C middle add bottom &&
+	git -C middle commit -m "rename_commit" &&
+	# Create top commit
+	git -C top/middle fetch &&
+	git -C top/middle checkout -f FETCH_HEAD &&
+	git -C top add middle &&
+	git -C top commit -m "rename_commit" &&
+
+	# rename the submodule remote
+	git -C top-cloned/middle remote rename origin upstream &&
+
+	# Make the update of "middle" a no-op, otherwise we error out
+	# because of its unmerged state
+	test_config -C top-cloned submodule.middle.update !true &&
+	git -C top-cloned submodule update --recursive 2>actual.err &&
+	cat >expect.err <<-\EOF &&
+	EOF
+	test_cmp expect.err actual.err
+'
+
 test_expect_success 'submodule update should skip unmerged submodules' '
 	test_when_finished "rm -fr top-cloned" &&
 	cp -r top-clean top-cloned &&

-- 
2.48.1.397.gec9d649cc640

[PATCH 5/6] submodule: move get_default_remote_submodule()

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

A future refactor got get_default_remote_submodule() is going to depend on
resolve_relative_url(). That function depends on get_default_remote().

Move get_default_remote_submodule() after resolve_relative_url() first
to make the additional functionality easier to review.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 builtin/submodule--helper.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ef3ff65a80f398c5ac35660288290ad92c7132c7..5542b403217b979d6da92c79d89d0991e980f692 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -60,21 +60,6 @@ static int repo_get_default_remote(struct repository *repo, char **default_remot
 	return 0;
 }
 
-static int get_default_remote_submodule(const char *module_path, char **default_remote)
-{
-	struct repository subrepo;
-	int ret;
-
-	if (repo_submodule_init(&subrepo, the_repository, module_path,
-				null_oid(the_hash_algo)) < 0)
-		return die_message(_("could not get a repository handle for submodule '%s'"),
-				   module_path);
-	ret = repo_get_default_remote(&subrepo, default_remote);
-	repo_clear(&subrepo);
-
-	return ret;
-}
-
 static char *get_default_remote(void)
 {
 	char *default_remote;
@@ -110,6 +95,21 @@ static char *resolve_relative_url(const char *rel_url, const char *up_path, int
 	return resolved_url;
 }
 
+static int get_default_remote_submodule(const char *module_path, char **default_remote)
+{
+	struct repository subrepo;
+	int ret;
+
+	if (repo_submodule_init(&subrepo, the_repository, module_path,
+				null_oid(the_hash_algo)) < 0)
+		return die_message(_("could not get a repository handle for submodule '%s'"),
+				   module_path);
+	ret = repo_get_default_remote(&subrepo, default_remote);
+	repo_clear(&subrepo);
+
+	return ret;
+}
+
 /* the result should be freed by the caller. */
 static char *get_submodule_displaypath(const char *path, const char *prefix,
 				       const char *super_prefix)

-- 
2.48.1.397.gec9d649cc640

[PATCH 6/6] submodule: look up remotes by URL first

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

The get_default_remote_submodule() function performs a lookup to find
the appropriate remote to use within a submodule. The function first
checks to see if it can find the remote for the current branch. If this
fails, it then checks to see if there is exactly one remote. It will use
this, before finally falling back to "origin" as the default.

If a user happens to rename their default remote from origin, either
manually or by setting something like clone.defaultRemoteName, this
fallback will not work.

In such cases, the submodule logic will try to use a non-existent
remote. This usually manifests as a failure to trigger the submodule
update.

The parent project already knows and stores the submodule URL in either
.gitmodules or its .git/config.

Add a new repo_remote_from_url() helper which will iterate over all the
remotes in a repository and return the first remote which has a matching
URL.

Refactor repo_get_default_remote to take an optional url parameter. If
its set, first attempt to use repo_remote_from_url(), before attempting
to use the existing logic.

Refactor get_default_remote_submodule to find the submodule and get its
URL. If a valid URL exists, then pass this to repo_get_default_remote,
ensuring that we prioritize using a remote with the matching URL if it
exists.

The fallback logic is kept in case for some reason the user has manually
changed the URL within the submodule. Additionally, we still try to use
a remote rather than directly passing the URL in the
fetch_in_submodule() logic. This ensures that an update will properly
update the remote refs within the submodule as expected, rather than
just fetching into FETCH_HEAD.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 remote.h                    |  1 +
 builtin/submodule--helper.c | 43 ++++++++++++++++++++++++++++++++++++-------
 remote.c                    | 15 +++++++++++++++
 t/t7406-submodule-update.sh | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 84 insertions(+), 7 deletions(-)

diff --git a/remote.h b/remote.h
index ef0de4aa64e9ccd32cc2eea076c00386dcba1161..8b8ece50b1cd684969fbb8a9d695fb3002a3f3f5 100644
--- a/remote.h
+++ b/remote.h
@@ -339,6 +339,7 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit);
 char *remote_ref_for_branch(struct branch *branch, int for_push);
 
 const char *repo_default_remote(struct repository *repo);
+const char *repo_remote_from_url(struct repository *repo, const char *url);
 
 /* returns true if the given branch has merge configuration given. */
 int branch_has_merge_config(struct branch *branch);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5542b403217b979d6da92c79d89d0991e980f692..432fe5d14f78756b64cf1770c174ce49bc2978f9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -41,15 +41,25 @@
 typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
 				  void *cb_data);
 
-static int repo_get_default_remote(struct repository *repo, char **default_remote)
+static int repo_get_default_remote(struct repository *repo,
+				   const char *url,
+				   char **default_remote)
 {
-	struct ref_store *store = get_main_ref_store(repo);
-	const char *refname = refs_resolve_ref_unsafe(store, "HEAD", 0, NULL,
-						      NULL);
+	struct ref_store *store;
+	const char *refname;
 
+	if (url) {
+		const char *remote_name = repo_remote_from_url(repo, url);
+		if (remote_name) {
+			*default_remote = xstrdup(remote_name);
+			return 0;
+		}
+	}
+
+	store = get_main_ref_store(repo);
+	refname = refs_resolve_ref_unsafe(store, "HEAD", 0, NULL, NULL);
 	if (!refname)
 		return die_message(_("No such ref: %s"), "HEAD");
-
 	if (strcmp(refname, "HEAD") &&
 	    !skip_prefix(refname, "refs/heads/", &refname))
 		return die_message(_("Expecting a full ref name, got %s"),
@@ -63,7 +73,7 @@ static int repo_get_default_remote(struct repository *repo, char **default_remot
 static char *get_default_remote(void)
 {
 	char *default_remote;
-	int code = repo_get_default_remote(the_repository, &default_remote);
+	int code = repo_get_default_remote(the_repository, NULL, &default_remote);
 
 	if (code)
 		exit(code);
@@ -97,16 +107,35 @@ static char *resolve_relative_url(const char *rel_url, const char *up_path, int
 
 static int get_default_remote_submodule(const char *module_path, char **default_remote)
 {
+	const struct submodule *sub;
 	struct repository subrepo;
+	char *url = NULL;
 	int ret;
 
+	sub = submodule_from_path(the_repository, null_oid(the_hash_algo), module_path);
+	if (sub && sub->url) {
+		url = xstrdup(sub->url);
+
+		/* Possibly a url relative to parent */
+		if (starts_with_dot_dot_slash(url) ||
+		    starts_with_dot_slash(url)) {
+			char *oldurl = url;
+
+			url = resolve_relative_url(oldurl, NULL, 1);
+			free(oldurl);
+		}
+	}
+
 	if (repo_submodule_init(&subrepo, the_repository, module_path,
 				null_oid(the_hash_algo)) < 0)
 		return die_message(_("could not get a repository handle for submodule '%s'"),
 				   module_path);
-	ret = repo_get_default_remote(&subrepo, default_remote);
+	ret = repo_get_default_remote(&subrepo, url, default_remote);
 	repo_clear(&subrepo);
 
+	if (url)
+		free(url);
+
 	return ret;
 }
 
diff --git a/remote.c b/remote.c
index fcda185ecfab5102afbe8918fed65c74971ef8c2..bf2ebfcf06a87ddf1fecadc3e408427d0a83c479 100644
--- a/remote.c
+++ b/remote.c
@@ -1788,6 +1788,21 @@ const char *repo_default_remote(struct repository *repo)
 	return remotes_remote_for_branch(repo->remote_state, branch, NULL);
 }
 
+const char *repo_remote_from_url(struct repository *repo, const char *url)
+{
+	read_config(repo, 0);
+
+	for (int i = 0; i < repo->remote_state->remotes_nr; i++) {
+		struct remote *remote = repo->remote_state->remotes[i];
+		if (!remote)
+			continue;
+
+		if (remote_has_url(remote, url))
+			return remote->name;
+	}
+	return NULL;
+}
+
 int branch_has_merge_config(struct branch *branch)
 {
 	return branch && !!branch->merge;
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 748b529745a5121f121768bb4e0cbc11bc833ea4..c09047b5f441a73a02a9fc4197e9a0ea8f39b529 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -1134,6 +1134,38 @@ test_expect_success 'setup clean recursive superproject' '
 	git clone --recurse-submodules top top-clean
 '
 
+test_expect_success 'submodule update with multiple remotes' '
+	test_when_finished "rm -fr top-cloned" &&
+	cp -r top-clean top-cloned &&
+
+	# Create a commit in each repo, starting with bottom
+	test_commit -C bottom multiple_remote_commit &&
+	# Create middle commit
+	git -C middle/bottom fetch &&
+	git -C middle/bottom checkout -f FETCH_HEAD &&
+	git -C middle add bottom &&
+	git -C middle commit -m "multiple_remote_commit" &&
+	# Create top commit
+	git -C top/middle fetch &&
+	git -C top/middle checkout -f FETCH_HEAD &&
+	git -C top add middle &&
+	git -C top commit -m "multiple_remote_commit" &&
+
+	# rename the submodule remote
+	git -C top-cloned/middle remote rename origin upstream &&
+
+	# Add another remote
+	git -C top-cloned/middle remote add other bogus &&
+
+	# Make the update of "middle" a no-op, otherwise we error out
+	# because of its unmerged state
+	test_config -C top-cloned submodule.middle.update !true &&
+	git -C top-cloned submodule update --recursive 2>actual.err &&
+	cat >expect.err <<-\EOF &&
+	EOF
+	test_cmp expect.err actual.err
+'
+
 test_expect_success 'submodule update with renamed remote' '
 	test_when_finished "rm -fr top-cloned" &&
 	cp -r top-clean top-cloned &&

-- 
2.48.1.397.gec9d649cc640

Re: [PATCH 0/6] submodule: improve remote lookup logic

[Jacob Keller]

On 6/10/2025 5:52 PM, Jacob Keller wrote:
> This series improves the git submodule remote lookup logic implemented in
> submodule--helper.
> 
> A few cleanups are done first:
> 
> * The starts_with_dot(_dot)_slash helper functions are moved to dir.h for
>   re-use, as these are used both within submodule--helper.c and
>   submodule-config.c
> 
> * Several remote.c helper functions are refactored to take repository
>   pointers, enabling use with a submodule repository pointer.
> 
> * The branch_release logic is fixed so that it won't dereference
>   branch->merge if it was never setup. This appeared to trigger in some
>   cases if a submodule repository was passed into the read_config()
>   function.
> 
> Next, the submodule--helper.c logic in repo_get_default_remote() is
> refactored to remote.c helper functions. A new repo_default_remote() helper
> function is added which will try to find a default remote. This helper
> first tries to look up the remote from the checked out branch, then falls
> back to the only remote (if there is exactly one remote) before finally
> falling back to "origin".
> 
> This improved logic is a good first step, but won't handle cases where
> there are multiple remotes, and when remotes have been renamed.
> 
> For the final improvement, notice that the parent project already stores
> the URL for the submodule in its .git/config or .gitmodules file. This URL
> is what we use to set the remote in the first place when cloning.
> 
> Add a repo_remote_from_url() helper which will iterate through the remotes
> and find the first remote with that URL. Use this in
> repo_get_default_remote() to first try and find a remote by its URL. If
> unsuccessful, we still keep the original fallback logic, in the off chance
> that the user has changed the URL from within the submodule.
> 
> This method is more robust as it is less likely that the user has manually
> changed the submodule URL within the submodule but not also within the
> .git/config.
> 
> With this change, all commands which need the submodule remote will first
> look up by URL before trying to use the fallback logic, and should now be
> able to find a suitable remote regardless of now they are renamed.
> 
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> Jacob Keller (6):
>       dir: move starts_with_dot(_dot)_slash to dir.h
>       remote: remove the_repository from some functions
>       remote: check branch->merge before access in branch_release
>       submodule--helper: improve logic for fallback remote name
>       submodule: move get_default_remote_submodule()
>       submodule: look up remotes by URL first
> 
>  dir.h                       |  23 ++++++++++
>  remote.h                    |   3 ++
>  builtin/submodule--helper.c | 101 ++++++++++++++++++++++--------------------
>  remote.c                    | 104 ++++++++++++++++++++++++++++----------------
>  submodule-config.c          |  12 -----
>  t/t7406-submodule-update.sh |  61 ++++++++++++++++++++++++++
>  6 files changed, 207 insertions(+), 97 deletions(-)
> ---
> base-commit: 4c0e625c091d4c648cec7319bafaed3cc81658e5
> change-id: 20250610-jk-submodule-helper-use-url-e55d3c379faf
> 
> Best regards,

Seems like there hasn't been any interest in this series? :(

Re: [PATCH 0/6] submodule: improve remote lookup logic

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> Seems like there hasn't been any interest in this series? :(

So far it seems, but we have been in pre-release freeze for a few
weeks combined with slower summer (in northern hemisphere anyway)
season, so it might have just been a bad timing.

Re: [PATCH 0/6] submodule: improve remote lookup logic

[Jacob Keller]

On 6/16/2025 3:41 PM, Junio C Hamano wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
> 
>> Seems like there hasn't been any interest in this series? :(
> 
> So far it seems, but we have been in pre-release freeze for a few
> weeks combined with slower summer (in northern hemisphere anyway)
> season, so it might have just been a bad timing.

Oh true. I can resend once release freeze ends :)

Re: [PATCH 4/6] submodule--helper: improve logic for fallback remote name

[Lidong Yan]

Jacob Keller <jacob.e.keller@intel.com> writes：
> 
> From: Jacob Keller <jacob.keller@gmail.com>
> 
> The repo_get_default_remote() function in submodule--helper currently
> tries to figure out the proper remote name to use for a submodule based
> on a few factors.
> 
> First, it tries to find the remote for the currently checked out branch.
> This works if the submodule is configured to checkout to a branch
> instead of a detached HEAD state.
> 
> In the detached HEAD state, the code calls back to using "origin", on
> the assumption that this is the default remote name. Some users may
> change this, such as by setting clone.defaultRemoteName, or by changing
> the remote name manually within the submodule repository.
> 
> As a first step to improving this situation, refactor to reuse the logic
> from remotes_remote_for_branch(). This function uses the remote from the
> branch if it has one. If it doesn't then it checks to see if there is
> exactly one remote. It uses this remote first before attempting to fall
> back to "origin".
> 
> To allow using this helper function, introduce a repo_default_remote()
> helper to remote.c which takes a repository structure. This helper will
> load the remote configuration and get the "HEAD" branch. Then it will
> call remotes_remote_for_branch to find the default remote.

Just a thought: since repo_default_remote() is only used within
repo_get_default_remote(), and the two have very similar names,
do you think it might be clearer to inline the former into the latter?

> This method allows re-using the same existing logic as other flows,
> rather than duplicating it in submodule--helper.c.
> 
> This isn't a perfect solution for users who change remote names, but it
> should help in cases where the remote name is changed but users haven't
> added any additional remotes.
> 
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> remote.h                    |  2 ++
> builtin/submodule--helper.c | 18 +++---------------
> remote.c                    | 25 ++++++++++++++++++++-----
> t/t7406-submodule-update.sh | 29 +++++++++++++++++++++++++++++
> 4 files changed, 54 insertions(+), 20 deletions(-)
> 
> diff --git a/remote.h b/remote.h
> index 7e4943ae3a70ecefa3332d211084762ca30b59b6..ef0de4aa64e9ccd32cc2eea076c00386dcba1161 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -338,6 +338,8 @@ const char *remote_for_branch(struct branch *branch, int *explicit);
> const char *pushremote_for_branch(struct branch *branch, int *explicit);
> char *remote_ref_for_branch(struct branch *branch, int for_push);
> 
> +const char *repo_default_remote(struct repository *repo);
> +
> /* returns true if the given branch has merge configuration given. */
> int branch_has_merge_config(struct branch *branch);
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 9e8cdfe1b2a8c2985d9c1b8ad6f1b0d1f9401714..ef3ff65a80f398c5ac35660288290ad92c7132c7 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -43,8 +43,6 @@ typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
> 
> static int repo_get_default_remote(struct repository *repo, char **default_remote)
> {
> - char *dest = NULL;
> - struct strbuf sb = STRBUF_INIT;
> struct ref_store *store = get_main_ref_store(repo);
> const char *refname = refs_resolve_ref_unsafe(store, "HEAD", 0, NULL,
>      NULL);
> @@ -52,23 +50,13 @@ static int repo_get_default_remote(struct repository *repo, char **default_remot
> if (!refname)
> return die_message(_("No such ref: %s"), "HEAD");
> 
> - /* detached HEAD */
> - if (!strcmp(refname, "HEAD")) {
> - *default_remote = xstrdup("origin");
> - return 0;
> - }
> -
> - if (!skip_prefix(refname, "refs/heads/", &refname))
> + if (strcmp(refname, "HEAD") &&
> +    !skip_prefix(refname, "refs/heads/", &refname))
> return die_message(_("Expecting a full ref name, got %s"),
>   refname);
> 
> - strbuf_addf(&sb, "branch.%s.remote", refname);
> - if (repo_config_get_string(repo, sb.buf, &dest))
> - *default_remote = xstrdup("origin");
> - else
> - *default_remote = dest;
> + *default_remote = xstrdup(repo_default_remote(repo));
> 
> - strbuf_release(&sb);
> return 0;
> }
> 
> diff --git a/remote.c b/remote.c
> index 1035f0cd32d034cce05bd2a3d829ec90795ff4e2..fcda185ecfab5102afbe8918fed65c74971ef8c2 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1759,20 +1759,35 @@ static void set_merge(struct repository *repo, struct branch *ret)
> }
> }
> 
> -struct branch *branch_get(const char *name)
> +static struct branch *repo_branch_get(struct repository *repo, const char *name)
> {
> struct branch *ret;
> 
> - read_config(the_repository, 0);
> + read_config(repo, 0);
> if (!name || !*name || !strcmp(name, "HEAD"))
> - ret = the_repository->remote_state->current_branch;
> + ret = repo->remote_state->current_branch;
> else
> - ret = make_branch(the_repository->remote_state, name,
> + ret = make_branch(repo->remote_state, name,
>  strlen(name));
> - set_merge(the_repository, ret);
> + set_merge(repo, ret);
> return ret;
> }
> 
> +struct branch *branch_get(const char *name)
> +{
> + return repo_branch_get(the_repository, name);
> +}
> +
> +const char *repo_default_remote(struct repository *repo)
> +{
> + struct branch *branch;
> +
> + read_config(repo, 0);
> + branch = repo_branch_get(repo, "HEAD");
> +
> + return remotes_remote_for_branch(repo->remote_state, branch, NULL);
> +}
> +
> int branch_has_merge_config(struct branch *branch)
> {
> return branch && !!branch->merge;
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index c562bad042ab2d4d0f82cb8b57a1eadbe24044d1..748b529745a5121f121768bb4e0cbc11bc833ea4 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -1134,6 +1134,35 @@ test_expect_success 'setup clean recursive superproject' '
> git clone --recurse-submodules top top-clean
> '
> 
> +test_expect_success 'submodule update with renamed remote' '
> + test_when_finished "rm -fr top-cloned" &&
> + cp -r top-clean top-cloned &&
> +
> + # Create a commit in each repo, starting with bottom
> + test_commit -C bottom rename_commit &&
> + # Create middle commit
> + git -C middle/bottom fetch &&
> + git -C middle/bottom checkout -f FETCH_HEAD &&
> + git -C middle add bottom &&
> + git -C middle commit -m "rename_commit" &&
> + # Create top commit
> + git -C top/middle fetch &&
> + git -C top/middle checkout -f FETCH_HEAD &&
> + git -C top add middle &&
> + git -C top commit -m "rename_commit" &&
> +
> + # rename the submodule remote
> + git -C top-cloned/middle remote rename origin upstream &&
> +
> + # Make the update of "middle" a no-op, otherwise we error out
> + # because of its unmerged state
> + test_config -C top-cloned submodule.middle.update !true &&
> + git -C top-cloned submodule update --recursive 2>actual.err &&
> + cat >expect.err <<-\EOF &&
> + EOF
> + test_cmp expect.err actual.err
> +'
> +
> test_expect_success 'submodule update should skip unmerged submodules' '
> test_when_finished "rm -fr top-cloned" &&
> cp -r top-clean top-cloned &&
> 
> -- 
> 2.48.1.397.gec9d649cc640
> 
> 

Re: [PATCH 4/6] submodule--helper: improve logic for fallback remote name

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> diff --git a/remote.h b/remote.h
> index 7e4943ae3a70ecefa3332d211084762ca30b59b6..ef0de4aa64e9ccd32cc2eea076c00386dcba1161 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -338,6 +338,8 @@ const char *remote_for_branch(struct branch *branch, int *explicit);
>  const char *pushremote_for_branch(struct branch *branch, int *explicit);
>  char *remote_ref_for_branch(struct branch *branch, int for_push);
>  
> +const char *repo_default_remote(struct repository *repo);
> +

This breaks "make hdr-check" because nobody up to this point
mentions "struct repository".

It seems doing an opaque forward declaration is in vogue at least in
this file?

 remote.h | 1 +
 1 file changed, 1 insertion(+)

diff --git c/remote.h w/remote.h
index ef0de4aa64..86020ae2d3 100644
--- c/remote.h
+++ w/remote.h
@@ -9,6 +9,7 @@
 
 struct option;
 struct transport_ls_refs_options;
+struct repository;
 
 /**
  * The API gives access to the configuration related to remotes. It handles

Re: [PATCH 0/6] submodule: improve remote lookup logic

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> On 6/16/2025 3:41 PM, Junio C Hamano wrote:
>> Jacob Keller <jacob.e.keller@intel.com> writes:
>> 
>>> Seems like there hasn't been any interest in this series? :(
>> 
>> So far it seems, but we have been in pre-release freeze for a few
>> weeks combined with slower summer (in northern hemisphere anyway)
>> season, so it might have just been a bad timing.
>
> Oh true. I can resend once release freeze ends :)

Before doing so, can you run with leaksanitizer?  When merged to
'jch', many tests fail and t1013-read-tree-submodule.sh #52 is one
of them.

Thanks.

not ok 52 - git_test_func: modified submodule does not update submodule work tree to invalid commit
#
#                       prolog &&
#                       reset_work_tree_to add_sub1 &&
#                       (
#                               cd submodule_update &&
#                               git branch -t invalid_sub1 origin/invalid_sub1 &&
#                               $command invalid_sub1 &&
#                               test_superproject_content origin/invalid_sub1 &&
#                               test_submodule_content sub1 origin/add_sub1 &&
#                               test_must_fail git submodule update &&
#                               test_submodule_content sub1 origin/add_sub1
#                       )
#
1..52

=================================================================
==git==1541109==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 768 byte(s) in 1 object(s) allocated from:
    #0 0x561ea1a821c7 in realloc (git+0x8a1c7) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)
    #1 0x561ea1da4126 in xrealloc wrapper.c:140:8
    #2 0x561ea1d181a8 in refspec_append refspec.c:209:2
    #3 0x561ea1d1e698 in handle_config remote.c:506:3
    #4 0x561ea1bf7878 in configset_iter config.c:2140:7
    #5 0x561ea1bf775a in repo_config config.c:2519:2
    #6 0x561ea1d195fd in read_config remote.c:615:2
    #7 0x561ea1d1b6ba in repo_remote_from_url remote.c:1794:2
    #8 0x561ea1b8ae07 in repo_get_default_remote builtin/submodule--helper.c:54:29
    #9 0x561ea1b8d475 in get_default_remote_submodule builtin/submodule--helper.c:135:8
    #10 0x561ea1b8d789 in fetch_in_submodule builtin/submodule--helper.c:2355:10
    #11 0x561ea1b8d98e in run_update_procedure builtin/submodule--helper.c:2481:7
    #12 0x561ea1b8c7b8 in update_submodule builtin/submodule--helper.c:2700:9
    #13 0x561ea1b8ba35 in update_submodules builtin/submodule--helper.c:2784:10
    #14 0x561ea1b87097 in module_update builtin/submodule--helper.c:2924:8
    #15 0x561ea1b84b9d in cmd_submodule__helper builtin/submodule--helper.c:3646:9
    #16 0x561ea1a86cfb in run_builtin git.c:480:11
    #17 0x561ea1a858a0 in handle_builtin git.c:746:9
    #18 0x561ea1a866a8 in run_argv git.c:813:4
    #19 0x561ea1a8562b in cmd_main git.c:953:19
    #20 0x561ea1ba41da in main common-main.c:9:11
    #21 0x7f60975e6ca7 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #22 0x7f60975e6d64 in __libc_start_main csu/../csu/libc-start.c:360:3
    #23 0x561ea1a52210 in _start (git+0x5a210) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)

DEDUP_TOKEN: __interceptor_realloc--xrealloc--refspec_append--handle_config--configset_iter--repo_config--read_config--repo_remote_from_url--repo_get_default_remote--get_default_remote_submodule--fetch_in_submodule--run_update_procedure--update_submodule--update_submodules--module_update--cmd_submodule__helper--run_builtin--handle_builtin--run_argv--cmd_main--main--__libc_start_call_main--__libc_start_main--_start
Direct leak of 192 byte(s) in 1 object(s) allocated from:
    #0 0x561ea1a821c7 in realloc (git+0x8a1c7) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)
    #1 0x561ea1da4126 in xrealloc wrapper.c:140:8
    #2 0x561ea1d1edb4 in add_merge remote.c:177:2
    #3 0x561ea1d1e23c in handle_config remote.c:432:4
    #4 0x561ea1bf7878 in configset_iter config.c:2140:7
    #5 0x561ea1bf775a in repo_config config.c:2519:2
    #6 0x561ea1d195fd in read_config remote.c:615:2
    #7 0x561ea1d1b6ba in repo_remote_from_url remote.c:1794:2
    #8 0x561ea1b8ae07 in repo_get_default_remote builtin/submodule--helper.c:54:29
    #9 0x561ea1b8d475 in get_default_remote_submodule builtin/submodule--helper.c:135:8
    #10 0x561ea1b8d789 in fetch_in_submodule builtin/submodule--helper.c:2355:10
    #11 0x561ea1b8d98e in run_update_procedure builtin/submodule--helper.c:2481:7
    #12 0x561ea1b8c7b8 in update_submodule builtin/submodule--helper.c:2700:9
    #13 0x561ea1b8ba35 in update_submodules builtin/submodule--helper.c:2784:10
    #14 0x561ea1b87097 in module_update builtin/submodule--helper.c:2924:8
    #15 0x561ea1b84b9d in cmd_submodule__helper builtin/submodule--helper.c:3646:9
    #16 0x561ea1a86cfb in run_builtin git.c:480:11
    #17 0x561ea1a858a0 in handle_builtin git.c:746:9
    #18 0x561ea1a866a8 in run_argv git.c:813:4
    #19 0x561ea1a8562b in cmd_main git.c:953:19
    #20 0x561ea1ba41da in main common-main.c:9:11
    #21 0x7f60975e6ca7 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #22 0x7f60975e6d64 in __libc_start_main csu/../csu/libc-start.c:360:3
    #23 0x561ea1a52210 in _start (git+0x5a210) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)

DEDUP_TOKEN: __interceptor_realloc--xrealloc--add_merge--handle_config--configset_iter--repo_config--read_config--repo_remote_from_url--repo_get_default_remote--get_default_remote_submodule--fetch_in_submodule--run_update_procedure--update_submodule--update_submodules--module_update--cmd_submodule__helper--run_builtin--handle_builtin--run_argv--cmd_main--main--__libc_start_call_main--__libc_start_main--_start
Indirect leak of 36 byte(s) in 1 object(s) allocated from:
    #0 0x561ea1a81d3d in malloc (git+0x89d3d) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)
    #1 0x7f60976667a9 in strdup string/strdup.c:42:15
    #2 0x561ea1da3e14 in xstrdup wrapper.c:43:14
    #3 0x561ea1d17f2b in refspec_item_init refspec.c:160:14
    #4 0x561ea1d17ef1 in refspec_item_init_fetch refspec.c:166:9
    #5 0x561ea1d180dd in refspec_append refspec.c:203:9
    #6 0x561ea1d1e698 in handle_config remote.c:506:3
    #7 0x561ea1bf7878 in configset_iter config.c:2140:7
    #8 0x561ea1bf775a in repo_config config.c:2519:2
    #9 0x561ea1d195fd in read_config remote.c:615:2
    #10 0x561ea1d1b6ba in repo_remote_from_url remote.c:1794:2
    #11 0x561ea1b8ae07 in repo_get_default_remote builtin/submodule--helper.c:54:29
    #12 0x561ea1b8d475 in get_default_remote_submodule builtin/submodule--helper.c:135:8
    #13 0x561ea1b8d789 in fetch_in_submodule builtin/submodule--helper.c:2355:10
    #14 0x561ea1b8d98e in run_update_procedure builtin/submodule--helper.c:2481:7
    #15 0x561ea1b8c7b8 in update_submodule builtin/submodule--helper.c:2700:9
    #16 0x561ea1b8ba35 in update_submodules builtin/submodule--helper.c:2784:10
    #17 0x561ea1b87097 in module_update builtin/submodule--helper.c:2924:8
    #18 0x561ea1b84b9d in cmd_submodule__helper builtin/submodule--helper.c:3646:9
    #19 0x561ea1a86cfb in run_builtin git.c:480:11
    #20 0x561ea1a858a0 in handle_builtin git.c:746:9
    #21 0x561ea1a866a8 in run_argv git.c:813:4
    #22 0x561ea1a8562b in cmd_main git.c:953:19
    #23 0x561ea1ba41da in main common-main.c:9:11
    #24 0x7f60975e6ca7 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #25 0x7f60975e6d64 in __libc_start_main csu/../csu/libc-start.c:360:3
    #26 0x561ea1a52210 in _start (git+0x5a210) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)

DEDUP_TOKEN: __interceptor_malloc--strdup--xstrdup--refspec_item_init--refspec_item_init_fetch--refspec_append--handle_config--configset_iter--repo_config--read_config--repo_remote_from_url--repo_get_default_remote--get_default_remote_submodule--fetch_in_submodule--run_update_procedure--update_submodule--update_submodules--module_update--cmd_submodule__helper--run_builtin--handle_builtin--run_argv--cmd_main--main--__libc_start_call_main--__libc_start_main--_start
Indirect leak of 22 byte(s) in 1 object(s) allocated from:
    #0 0x561ea1a81d3d in malloc (git+0x89d3d) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)
    #1 0x561ea1da3e92 in do_xmalloc wrapper.c:55:8
    #2 0x561ea1da3f94 in do_xmallocz wrapper.c:89:8
    #3 0x561ea1da3f26 in xmallocz wrapper.c:97:9
    #4 0x561ea1da3ff8 in xmemdupz wrapper.c:113:16
    #5 0x561ea1da4077 in xstrndup wrapper.c:119:9
    #6 0x561ea1d19118 in parse_refspec refspec.c:55:15
    #7 0x561ea1d17f46 in refspec_item_init refspec.c:161:9
    #8 0x561ea1d17ef1 in refspec_item_init_fetch refspec.c:166:9
    #9 0x561ea1d180dd in refspec_append refspec.c:203:9
    #10 0x561ea1d1e698 in handle_config remote.c:506:3
    #11 0x561ea1bf7878 in configset_iter config.c:2140:7
    #12 0x561ea1bf775a in repo_config config.c:2519:2
    #13 0x561ea1d195fd in read_config remote.c:615:2
    #14 0x561ea1d1b6ba in repo_remote_from_url remote.c:1794:2
    #15 0x561ea1b8ae07 in repo_get_default_remote builtin/submodule--helper.c:54:29
    #16 0x561ea1b8d475 in get_default_remote_submodule builtin/submodule--helper.c:135:8
    #17 0x561ea1b8d789 in fetch_in_submodule builtin/submodule--helper.c:2355:10
    #18 0x561ea1b8d98e in run_update_procedure builtin/submodule--helper.c:2481:7
    #19 0x561ea1b8c7b8 in update_submodule builtin/submodule--helper.c:2700:9
    #20 0x561ea1b8ba35 in update_submodules builtin/submodule--helper.c:2784:10
    #21 0x561ea1b87097 in module_update builtin/submodule--helper.c:2924:8
    #22 0x561ea1b84b9d in cmd_submodule__helper builtin/submodule--helper.c:3646:9
    #23 0x561ea1a86cfb in run_builtin git.c:480:11
    #24 0x561ea1a858a0 in handle_builtin git.c:746:9
    #25 0x561ea1a866a8 in run_argv git.c:813:4
    #26 0x561ea1a8562b in cmd_main git.c:953:19
    #27 0x561ea1ba41da in main common-main.c:9:11

DEDUP_TOKEN: __interceptor_malloc--do_xmalloc--do_xmallocz--xmallocz--xmemdupz--xstrndup--parse_refspec--refspec_item_init--refspec_item_init_fetch--refspec_append--handle_config--configset_iter--repo_config--read_config--repo_remote_from_url--repo_get_default_remote--get_default_remote_submodule--fetch_in_submodule--run_update_procedure--update_submodule--update_submodules--module_update--cmd_submodule__helper--run_builtin--handle_builtin--run_argv--cmd_main--main
Indirect leak of 18 byte(s) in 1 object(s) allocated from:
    #0 0x561ea1a81d3d in malloc (git+0x89d3d) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)
    #1 0x7f60976667a9 in strdup string/strdup.c:42:15
    #2 0x561ea1da3e14 in xstrdup wrapper.c:43:14
    #3 0x561ea1d1e22d in handle_config remote.c:432:22
    #4 0x561ea1bf7878 in configset_iter config.c:2140:7
    #5 0x561ea1bf775a in repo_config config.c:2519:2
    #6 0x561ea1d195fd in read_config remote.c:615:2
    #7 0x561ea1d1b6ba in repo_remote_from_url remote.c:1794:2
    #8 0x561ea1b8ae07 in repo_get_default_remote builtin/submodule--helper.c:54:29
    #9 0x561ea1b8d475 in get_default_remote_submodule builtin/submodule--helper.c:135:8
    #10 0x561ea1b8d789 in fetch_in_submodule builtin/submodule--helper.c:2355:10
    #11 0x561ea1b8d98e in run_update_procedure builtin/submodule--helper.c:2481:7
    #12 0x561ea1b8c7b8 in update_submodule builtin/submodule--helper.c:2700:9
    #13 0x561ea1b8ba35 in update_submodules builtin/submodule--helper.c:2784:10
    #14 0x561ea1b87097 in module_update builtin/submodule--helper.c:2924:8
    #15 0x561ea1b84b9d in cmd_submodule__helper builtin/submodule--helper.c:3646:9
    #16 0x561ea1a86cfb in run_builtin git.c:480:11
    #17 0x561ea1a858a0 in handle_builtin git.c:746:9
    #18 0x561ea1a866a8 in run_argv git.c:813:4
    #19 0x561ea1a8562b in cmd_main git.c:953:19
    #20 0x561ea1ba41da in main common-main.c:9:11
    #21 0x7f60975e6ca7 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #22 0x7f60975e6d64 in __libc_start_main csu/../csu/libc-start.c:360:3
    #23 0x561ea1a52210 in _start (git+0x5a210) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)

DEDUP_TOKEN: __interceptor_malloc--strdup--xstrdup--handle_config--configset_iter--repo_config--read_config--repo_remote_from_url--repo_get_default_remote--get_default_remote_submodule--fetch_in_submodule--run_update_procedure--update_submodule--update_submodules--module_update--cmd_submodule__helper--run_builtin--handle_builtin--run_argv--cmd_main--main--__libc_start_call_main--__libc_start_main--_start
Indirect leak of 13 byte(s) in 1 object(s) allocated from:
    #0 0x561ea1a81d3d in malloc (git+0x89d3d) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)
    #1 0x561ea1da3e92 in do_xmalloc wrapper.c:55:8
    #2 0x561ea1da3f94 in do_xmallocz wrapper.c:89:8
    #3 0x561ea1da3f26 in xmallocz wrapper.c:97:9
    #4 0x561ea1da3ff8 in xmemdupz wrapper.c:113:16
    #5 0x561ea1da4077 in xstrndup wrapper.c:119:9
    #6 0x561ea1d1923b in parse_refspec refspec.c:73:15
    #7 0x561ea1d17f46 in refspec_item_init refspec.c:161:9
    #8 0x561ea1d17ef1 in refspec_item_init_fetch refspec.c:166:9
    #9 0x561ea1d180dd in refspec_append refspec.c:203:9
    #10 0x561ea1d1e698 in handle_config remote.c:506:3
    #11 0x561ea1bf7878 in configset_iter config.c:2140:7
    #12 0x561ea1bf775a in repo_config config.c:2519:2
    #13 0x561ea1d195fd in read_config remote.c:615:2
    #14 0x561ea1d1b6ba in repo_remote_from_url remote.c:1794:2
    #15 0x561ea1b8ae07 in repo_get_default_remote builtin/submodule--helper.c:54:29
    #16 0x561ea1b8d475 in get_default_remote_submodule builtin/submodule--helper.c:135:8
    #17 0x561ea1b8d789 in fetch_in_submodule builtin/submodule--helper.c:2355:10
    #18 0x561ea1b8d98e in run_update_procedure builtin/submodule--helper.c:2481:7
    #19 0x561ea1b8c7b8 in update_submodule builtin/submodule--helper.c:2700:9
    #20 0x561ea1b8ba35 in update_submodules builtin/submodule--helper.c:2784:10
    #21 0x561ea1b87097 in module_update builtin/submodule--helper.c:2924:8
    #22 0x561ea1b84b9d in cmd_submodule__helper builtin/submodule--helper.c:3646:9
    #23 0x561ea1a86cfb in run_builtin git.c:480:11
    #24 0x561ea1a858a0 in handle_builtin git.c:746:9
    #25 0x561ea1a866a8 in run_argv git.c:813:4
    #26 0x561ea1a8562b in cmd_main git.c:953:19
    #27 0x561ea1ba41da in main common-main.c:9:11

DEDUP_TOKEN: __interceptor_malloc--do_xmalloc--do_xmallocz--xmallocz--xmemdupz--xstrndup--parse_refspec--refspec_item_init--refspec_item_init_fetch--refspec_append--handle_config--configset_iter--repo_config--read_config--repo_remote_from_url--repo_get_default_remote--get_default_remote_submodule--fetch_in_submodule--run_update_procedure--update_submodule--update_submodules--module_update--cmd_submodule__helper--run_builtin--handle_builtin--run_argv--cmd_main--main
SUMMARY: LeakSanitizer: 1049 byte(s) leaked in 6 allocation(s).
Our logs revealed a memory leak...

Re: [PATCH 0/6] submodule: improve remote lookup logic

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> On 6/16/2025 3:41 PM, Junio C Hamano wrote:
>>> Jacob Keller <jacob.e.keller@intel.com> writes:
>>> 
>>>> Seems like there hasn't been any interest in this series? :(
>>> 
>>> So far it seems, but we have been in pre-release freeze for a few
>>> weeks combined with slower summer (in northern hemisphere anyway)
>>> season, so it might have just been a bad timing.
>>
>> Oh true. I can resend once release freeze ends :)
>
> Before doing so, can you run with leaksanitizer?  When merged to
> 'jch', many tests fail and t1013-read-tree-submodule.sh #52 is one
> of them.
>
> Thanks.

https://github.com/git/git/actions/runs/15711453001/job/44270362056

is a log of running the CI suite on this topic alone (tentatively
pushed to 'seen').

Re: [PATCH 4/6] submodule--helper: improve logic for fallback remote name

[Jacob Keller]

On 6/16/2025 7:58 PM, Lidong Yan wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes：
>>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> The repo_get_default_remote() function in submodule--helper currently
>> tries to figure out the proper remote name to use for a submodule based
>> on a few factors.
>>
>> First, it tries to find the remote for the currently checked out branch.
>> This works if the submodule is configured to checkout to a branch
>> instead of a detached HEAD state.
>>
>> In the detached HEAD state, the code calls back to using "origin", on
>> the assumption that this is the default remote name. Some users may
>> change this, such as by setting clone.defaultRemoteName, or by changing
>> the remote name manually within the submodule repository.
>>
>> As a first step to improving this situation, refactor to reuse the logic
>> from remotes_remote_for_branch(). This function uses the remote from the
>> branch if it has one. If it doesn't then it checks to see if there is
>> exactly one remote. It uses this remote first before attempting to fall
>> back to "origin".
>>
>> To allow using this helper function, introduce a repo_default_remote()
>> helper to remote.c which takes a repository structure. This helper will
>> load the remote configuration and get the "HEAD" branch. Then it will
>> call remotes_remote_for_branch to find the default remote.
> 
> Just a thought: since repo_default_remote() is only used within
> repo_get_default_remote(), and the two have very similar names,
> do you think it might be clearer to inline the former into the latter?
> 
Probably, but I can't inline it within submodule--helper.c because it
needs access to static functions in remote.c which I don't think make
sense to export.

I could also either move repo_get_default_remote() from
submodule--helper.c into remote.c, or try to eliminate it entirely.

My original thought was that it does some submodule-specific checks
which I don't want to remove but which don't seem to make sense
in-context with how the other remote.c helpers were implemented. Thus,
it didn't make sense to move it into remote.c, but I can't put
repo_default_remote() contents into submodule--helper.c without exposing
the read_config() and other static functions in remote.c

I'll think about this while resolving the memory issues and working on
the next version.

Re: [PATCH 4/6] submodule--helper: improve logic for fallback remote name

[Jacob Keller]

On 6/16/2025 7:58 PM, Lidong Yan wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes：
>>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> The repo_get_default_remote() function in submodule--helper currently
>> tries to figure out the proper remote name to use for a submodule based
>> on a few factors.
>>
>> First, it tries to find the remote for the currently checked out branch.
>> This works if the submodule is configured to checkout to a branch
>> instead of a detached HEAD state.
>>
>> In the detached HEAD state, the code calls back to using "origin", on
>> the assumption that this is the default remote name. Some users may
>> change this, such as by setting clone.defaultRemoteName, or by changing
>> the remote name manually within the submodule repository.
>>
>> As a first step to improving this situation, refactor to reuse the logic
>> from remotes_remote_for_branch(). This function uses the remote from the
>> branch if it has one. If it doesn't then it checks to see if there is
>> exactly one remote. It uses this remote first before attempting to fall
>> back to "origin".
>>
>> To allow using this helper function, introduce a repo_default_remote()
>> helper to remote.c which takes a repository structure. This helper will
>> load the remote configuration and get the "HEAD" branch. Then it will
>> call remotes_remote_for_branch to find the default remote.
> 
> Just a thought: since repo_default_remote() is only used within
> repo_get_default_remote(), and the two have very similar names,
> do you think it might be clearer to inline the former into the latter?
> 
I will try to rename this function, but I think in context it makes
sense to keep a separate function in remote.c which does the generic-ish
stuff while the submodule--helper.c function does the submodule-specific
stuff. This is because the later patches add the URL look up support to
this function.

Re: [PATCH 0/6] submodule: improve remote lookup logic

[Jacob Keller]

On 6/17/2025 8:09 AM, Junio C Hamano wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
> 
>> On 6/16/2025 3:41 PM, Junio C Hamano wrote:
>>> Jacob Keller <jacob.e.keller@intel.com> writes:
>>>
>>>> Seems like there hasn't been any interest in this series? :(
>>>
>>> So far it seems, but we have been in pre-release freeze for a few
>>> weeks combined with slower summer (in northern hemisphere anyway)
>>> season, so it might have just been a bad timing.
>>
>> Oh true. I can resend once release freeze ends :)
> 
> Before doing so, can you run with leaksanitizer?  When merged to
> 'jch', many tests fail and t1013-read-tree-submodule.sh #52 is one
> of them.
> 
> Thanks.
> 
Turns out it seems that SANITIZE=address,undefined,leak doesn't actually
run the leak sanitizer.

I found several gaps in branch_release() and remote_clear() which I've
fixed in v2.