{"thread":{"id":"63620","subject":"[PATCH 0/6] submodule: improve remote lookup logic","startedAt":"2025-06-11T00:52:26Z","lastAt":"2025-06-17T21:29:05Z","messageCount":17,"participants":["Jacob Keller","Junio C Hamano","Lidong Yan"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"520065","messageId":"20250610-jk-submodule-helper-use-url-v1-0-6d14c1504e91@gmail.com","threadId":"63620","inReplyTo":null,"subject":"[PATCH 0/6] submodule: improve remote lookup logic","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-11T00:52:14Z","receivedAt":"2025-06-11T00:52:26Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"This series improves the git submodule remote lookup logic implemented in\nsubmodule--helper.\n\nA few cleanups are done first:\n\n* The starts_with_dot(_dot)_slash helper functions are moved to dir.h for\n  re-use, as these are used both within submodule--helper.c and\n  submodule-config.c\n\n* Several remote.c helper functions are refactored to take repository\n  pointers, enabling use with a submodule repository pointer.\n\n* The branch_release logic is fixed so that it won't dereference\n  branch->merge if it was never setup. This appeared to trigger in some\n  cases if a submodule repository was passed into the read_config()\n  function.\n\nNext, the submodule--helper.c logic in repo_get_default_remote() is\nrefactored to remote.c helper functions. A new repo_default_remote() helper\nfunction is added which will try to find a default remote. This helper\nfirst tries to look up the remote from the checked out branch, then falls\nback to the only remote (if there is exactly one remote) before finally\nfalling back to \"origin\".\n\nThis improved logic is a good first step, but won't handle cases where\nthere are multiple remotes, and when remotes have been renamed.\n\nFor the final improvement, notice that the parent project already stores\nthe URL for the submodule in its .git/config or .gitmodules file. This URL\nis what we use to set the remote in the first place when cloning.\n\nAdd a repo_remote_from_url() helper which will iterate through the remotes\nand find the first remote with that URL. Use this in\nrepo_get_default_remote() to first try and find a remote by its URL. If\nunsuccessful, we still keep the original fallback logic, in the off chance\nthat the user has changed the URL from within the submodule.\n\nThis method is more robust as it is less likely that the user has manually\nchanged the submodule URL within the submodule but not also within the\n.git/config.\n\nWith this change, all commands which need the submodule remote will first\nlook up by URL before trying to use the fallback logic, and should now be\nable to find a suitable remote regardless of now they are renamed.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\nJacob Keller (6):\n      dir: move starts_with_dot(_dot)_slash to dir.h\n      remote: remove the_repository from some functions\n      remote: check branch->merge before access in branch_release\n      submodule--helper: improve logic for fallback remote name\n      submodule: move get_default_remote_submodule()\n      submodule: look up remotes by URL first\n\n dir.h                       |  23 ++++++++++\n remote.h                    |   3 ++\n builtin/submodule--helper.c | 101 ++++++++++++++++++++++--------------------\n remote.c                    | 104 ++++++++++++++++++++++++++++----------------\n submodule-config.c          |  12 -----\n t/t7406-submodule-update.sh |  61 ++++++++++++++++++++++++++\n 6 files changed, 207 insertions(+), 97 deletions(-)\n---\nbase-commit: 4c0e625c091d4c648cec7319bafaed3cc81658e5\nchange-id: 20250610-jk-submodule-helper-use-url-e55d3c379faf\n\nBest regards,\n-- \nJacob Keller <jacob.keller@gmail.com>\n\n"},{"id":"520066","messageId":"20250610-jk-submodule-helper-use-url-v1-1-6d14c1504e91@gmail.com","threadId":"63620","inReplyTo":"20250610-jk-submodule-helper-use-url-v1-0-6d14c1504e91@gmail.com","subject":"[PATCH 1/6] dir: move starts_with_dot(_dot)_slash to dir.h","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-11T00:52:15Z","receivedAt":"2025-06-11T00:52:27Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nBoth submodule--helper.c and submodule-config.c have an implementation\nof starts_with_dot_slash and starts_with_dot_dot_slash. The dir.h header\nhas starts_with_dot(_dot)_slash_native, which sets PATH_MATCH_NATIVE.\n\nMove the helpers to dir.h as static inlines. I thought about renaming\nthem to postfix with _platform but that felt too long and ugly. On the\nother hand it might be slightly confusing with _native.\n\nThis simplifies a submodule refactor which wants to use the helpers\nearlier in the submodule--helper.c file.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n dir.h                       | 23 +++++++++++++++++++++++\n builtin/submodule--helper.c | 12 ------------\n submodule-config.c          | 12 ------------\n 3 files changed, 23 insertions(+), 24 deletions(-)\n\ndiff --git a/dir.h b/dir.h\nindex d7e71aa8daa7d833e4c05e6875b997bc321c6070..fc9be7b427a134e46bcd66c8df42375db47727fc 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -676,4 +676,27 @@ static inline int starts_with_dot_dot_slash_native(const char *const path)\n \treturn path_match_flags(path, what | PATH_MATCH_NATIVE);\n }\n \n+/**\n+ * starts_with_dot_slash: convenience wrapper for\n+ * patch_match_flags() with PATH_MATCH_STARTS_WITH_DOT_SLASH and\n+ * PATH_MATCH_XPLATFORM.\n+ */\n+static inline int starts_with_dot_slash(const char *const path)\n+{\n+\tconst enum path_match_flags what = PATH_MATCH_STARTS_WITH_DOT_SLASH;\n+\n+\treturn path_match_flags(path, what | PATH_MATCH_XPLATFORM);\n+}\n+\n+/**\n+ * starts_with_dot_dot_slash: convenience wrapper for\n+ * patch_match_flags() with PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH and\n+ * PATH_MATCH_XPLATFORM.\n+ */\n+static inline int starts_with_dot_dot_slash(const char *const path)\n+{\n+\tconst enum path_match_flags what = PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH;\n+\n+\treturn path_match_flags(path, what | PATH_MATCH_XPLATFORM);\n+}\n #endif\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 53da2116ddf576bc565b29f043e8b703b8b1563b..9e8cdfe1b2a8c2985d9c1b8ad6f1b0d1f9401714 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -438,18 +438,6 @@ static int module_foreach(int argc, const char **argv, const char *prefix,\n \treturn ret;\n }\n \n-static int starts_with_dot_slash(const char *const path)\n-{\n-\treturn path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_SLASH |\n-\t\t\t\tPATH_MATCH_XPLATFORM);\n-}\n-\n-static int starts_with_dot_dot_slash(const char *const path)\n-{\n-\treturn path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH |\n-\t\t\t\tPATH_MATCH_XPLATFORM);\n-}\n-\n struct init_cb {\n \tconst char *prefix;\n \tconst char *super_prefix;\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 8630e27947d3943e1980eb7a53bd41a546842503..d64438b2a18ed2123cc5e18f739539209032d3e9 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -235,18 +235,6 @@ int check_submodule_name(const char *name)\n \treturn 0;\n }\n \n-static int starts_with_dot_slash(const char *const path)\n-{\n-\treturn path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_SLASH |\n-\t\t\t\tPATH_MATCH_XPLATFORM);\n-}\n-\n-static int starts_with_dot_dot_slash(const char *const path)\n-{\n-\treturn path_match_flags(path, PATH_MATCH_STARTS_WITH_DOT_DOT_SLASH |\n-\t\t\t\tPATH_MATCH_XPLATFORM);\n-}\n-\n static int submodule_url_is_relative(const char *url)\n {\n \treturn starts_with_dot_slash(url) || starts_with_dot_dot_slash(url);\n\n-- \n2.48.1.397.gec9d649cc640\n\n"},{"id":"520067","messageId":"20250610-jk-submodule-helper-use-url-v1-2-6d14c1504e91@gmail.com","threadId":"63620","inReplyTo":"20250610-jk-submodule-helper-use-url-v1-0-6d14c1504e91@gmail.com","subject":"[PATCH 2/6] remote: remove the_repository from some functions","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-11T00:52:16Z","receivedAt":"2025-06-11T00:52:28Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nThe remotes_remote_get_1 (and its caller, remotes_remote_get, have an\nimplicit dependency on the_repository due to calling\nread_branches_file() and read_remotes_file(), both of which use\nthe_repository. The branch_get() function calls set_merge() which has an\nimplicit dependency on the_repository as well.\n\nBecause of this use of the_repository, the helper functions cannot be\nused in code paths which operate on other repositories. A future\nrefactor of the submodule--helper will want to make use of some of these\nfunctions.\n\nRefactor to break the dependency by passing struct repository *repo\ninstead of struct remote_state *remote_state in a few places.\n\nThe public callers and many other helper functions still depend on\nthe_repository. A repo-aware function will be exposed in a following\nchange for git submodule--helper.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n remote.c | 58 ++++++++++++++++++++++++++++------------------------------\n 1 file changed, 28 insertions(+), 30 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 4099183cacdc8a607a8b5eaec86e456b2ef46b48..7092b12209d93e20ce1ed3b7d9e4cbac058c57ff 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -317,11 +317,10 @@ static void warn_about_deprecated_remote_type(const char *type,\n \t\ttype, remote->name, remote->name, remote->name);\n }\n \n-static void read_remotes_file(struct remote_state *remote_state,\n-\t\t\t      struct remote *remote)\n+static void read_remotes_file(struct repository *repo, struct remote *remote)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tFILE *f = fopen_or_warn(repo_git_path_append(the_repository, &buf,\n+\tFILE *f = fopen_or_warn(repo_git_path_append(repo, &buf,\n \t\t\t\t\t\t     \"remotes/%s\", remote->name), \"r\");\n \n \tif (!f)\n@@ -337,7 +336,7 @@ static void read_remotes_file(struct remote_state *remote_state,\n \t\tstrbuf_rtrim(&buf);\n \n \t\tif (skip_prefix(buf.buf, \"URL:\", &v))\n-\t\t\tadd_url_alias(remote_state, remote,\n+\t\t\tadd_url_alias(repo->remote_state, remote,\n \t\t\t\t      skip_spaces(v));\n \t\telse if (skip_prefix(buf.buf, \"Push:\", &v))\n \t\t\trefspec_append(&remote->push, skip_spaces(v));\n@@ -350,12 +349,11 @@ static void read_remotes_file(struct remote_state *remote_state,\n \tstrbuf_release(&buf);\n }\n \n-static void read_branches_file(struct remote_state *remote_state,\n-\t\t\t       struct remote *remote)\n+static void read_branches_file(struct repository *repo, struct remote *remote)\n {\n \tchar *frag, *to_free = NULL;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tFILE *f = fopen_or_warn(repo_git_path_append(the_repository, &buf,\n+\tFILE *f = fopen_or_warn(repo_git_path_append(repo, &buf,\n \t\t\t\t\t\t     \"branches/%s\", remote->name), \"r\");\n \n \tif (!f)\n@@ -382,9 +380,9 @@ static void read_branches_file(struct remote_state *remote_state,\n \tif (frag)\n \t\t*(frag++) = '\\0';\n \telse\n-\t\tfrag = to_free = repo_default_branch_name(the_repository, 0);\n+\t\tfrag = to_free = repo_default_branch_name(repo, 0);\n \n-\tadd_url_alias(remote_state, remote, buf.buf);\n+\tadd_url_alias(repo->remote_state, remote, buf.buf);\n \trefspec_appendf(&remote->fetch, \"refs/heads/%s:refs/heads/%s\",\n \t\t\tfrag, remote->name);\n \n@@ -681,7 +679,7 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)\n \t\t\t\t\t     branch, explicit);\n }\n \n-static struct remote *remotes_remote_get(struct remote_state *remote_state,\n+static struct remote *remotes_remote_get(struct repository *repo,\n \t\t\t\t\t const char *name);\n \n char *remote_ref_for_branch(struct branch *branch, int for_push)\n@@ -700,7 +698,7 @@ char *remote_ref_for_branch(struct branch *branch, int for_push)\n \t\t\t\t\tthe_repository->remote_state, branch,\n \t\t\t\t\tNULL);\n \t\t\tstruct remote *remote = remotes_remote_get(\n-\t\t\t\tthe_repository->remote_state, remote_name);\n+\t\t\t\tthe_repository, remote_name);\n \n \t\t\tif (remote && remote->push.nr &&\n \t\t\t    (dst = apply_refspecs(&remote->push,\n@@ -757,10 +755,11 @@ static void validate_remote_url(struct remote *remote)\n }\n \n static struct remote *\n-remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n+remotes_remote_get_1(struct repository *repo, const char *name,\n \t\t     const char *(*get_default)(struct remote_state *,\n \t\t\t\t\t\tstruct branch *, int *))\n {\n+\tstruct remote_state *remote_state = repo->remote_state;\n \tstruct remote *ret;\n \tint name_given = 0;\n \n@@ -774,9 +773,9 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n #ifndef WITH_BREAKING_CHANGES\n \tif (valid_remote_nick(name) && have_git_dir()) {\n \t\tif (!valid_remote(ret))\n-\t\t\tread_remotes_file(remote_state, ret);\n+\t\t\tread_remotes_file(repo, ret);\n \t\tif (!valid_remote(ret))\n-\t\t\tread_branches_file(remote_state, ret);\n+\t\t\tread_branches_file(repo, ret);\n \t}\n #endif /* WITH_BREAKING_CHANGES */\n \tif (name_given && !valid_remote(ret))\n@@ -790,35 +789,33 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n }\n \n static inline struct remote *\n-remotes_remote_get(struct remote_state *remote_state, const char *name)\n+remotes_remote_get(struct repository *repo, const char *name)\n {\n-\treturn remotes_remote_get_1(remote_state, name,\n-\t\t\t\t    remotes_remote_for_branch);\n+\treturn remotes_remote_get_1(repo, name, remotes_remote_for_branch);\n }\n \n struct remote *remote_get(const char *name)\n {\n \tread_config(the_repository, 0);\n-\treturn remotes_remote_get(the_repository->remote_state, name);\n+\treturn remotes_remote_get(the_repository, name);\n }\n \n struct remote *remote_get_early(const char *name)\n {\n \tread_config(the_repository, 1);\n-\treturn remotes_remote_get(the_repository->remote_state, name);\n+\treturn remotes_remote_get(the_repository, name);\n }\n \n static inline struct remote *\n-remotes_pushremote_get(struct remote_state *remote_state, const char *name)\n+remotes_pushremote_get(struct repository *repo, const char *name)\n {\n-\treturn remotes_remote_get_1(remote_state, name,\n-\t\t\t\t    remotes_pushremote_for_branch);\n+\treturn remotes_remote_get_1(repo, name, remotes_pushremote_for_branch);\n }\n \n struct remote *pushremote_get(const char *name)\n {\n \tread_config(the_repository, 0);\n-\treturn remotes_pushremote_get(the_repository->remote_state, name);\n+\treturn remotes_pushremote_get(the_repository, name);\n }\n \n int remote_is_configured(struct remote *remote, int in_repo)\n@@ -1722,7 +1719,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t}\n }\n \n-static void set_merge(struct remote_state *remote_state, struct branch *ret)\n+static void set_merge(struct repository *repo, struct branch *ret)\n {\n \tstruct remote *remote;\n \tchar *ref;\n@@ -1742,7 +1739,7 @@ static void set_merge(struct remote_state *remote_state, struct branch *ret)\n \t\treturn;\n \t}\n \n-\tremote = remotes_remote_get(remote_state, ret->remote_name);\n+\tremote = remotes_remote_get(repo, ret->remote_name);\n \n \tCALLOC_ARRAY(ret->merge, ret->merge_nr);\n \tfor (i = 0; i < ret->merge_nr; i++) {\n@@ -1751,7 +1748,7 @@ static void set_merge(struct remote_state *remote_state, struct branch *ret)\n \t\tif (!remote_find_tracking(remote, ret->merge[i]) ||\n \t\t    strcmp(ret->remote_name, \".\"))\n \t\t\tcontinue;\n-\t\tif (repo_dwim_ref(the_repository, ret->merge_name[i],\n+\t\tif (repo_dwim_ref(repo, ret->merge_name[i],\n \t\t\t\t  strlen(ret->merge_name[i]), &oid, &ref,\n \t\t\t\t  0) == 1)\n \t\t\tret->merge[i]->dst = ref;\n@@ -1770,7 +1767,7 @@ struct branch *branch_get(const char *name)\n \telse\n \t\tret = make_branch(the_repository->remote_state, name,\n \t\t\t\t  strlen(name));\n-\tset_merge(the_repository->remote_state, ret);\n+\tset_merge(the_repository, ret);\n \treturn ret;\n }\n \n@@ -1841,13 +1838,14 @@ static const char *tracking_for_push_dest(struct remote *remote,\n \treturn ret;\n }\n \n-static const char *branch_get_push_1(struct remote_state *remote_state,\n+static const char *branch_get_push_1(struct repository *repo,\n \t\t\t\t     struct branch *branch, struct strbuf *err)\n {\n+\tstruct remote_state *remote_state = repo->remote_state;\n \tstruct remote *remote;\n \n \tremote = remotes_remote_get(\n-\t\tremote_state,\n+\t\trepo,\n \t\tremotes_pushremote_for_branch(remote_state, branch, NULL));\n \tif (!remote)\n \t\treturn error_buf(err,\n@@ -1914,7 +1912,7 @@ const char *branch_get_push(struct branch *branch, struct strbuf *err)\n \n \tif (!branch->push_tracking_ref)\n \t\tbranch->push_tracking_ref = branch_get_push_1(\n-\t\t\tthe_repository->remote_state, branch, err);\n+\t\t\tthe_repository, branch, err);\n \treturn branch->push_tracking_ref;\n }\n \n\n-- \n2.48.1.397.gec9d649cc640\n\n"},{"id":"520068","messageId":"20250610-jk-submodule-helper-use-url-v1-3-6d14c1504e91@gmail.com","threadId":"63620","inReplyTo":"20250610-jk-submodule-helper-use-url-v1-0-6d14c1504e91@gmail.com","subject":"[PATCH 3/6] remote: check branch->merge before access in branch_release","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-11T00:52:17Z","receivedAt":"2025-06-11T00:52:29Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nThe branch_release function doesn't check the merge field before\naccessing it, based on an assumption that merge will be set if\nbranch->merge_nr is non-zero.\n\nThis is not always the case. It turns out that branch->merge is not\ninitialized until set_merge() is called, but branch->merge_nr can be\nnon-zero from handle_config() calling add_merge().\n\nset_merge() is not called until branch_get(). This function does set\nmerge_nr to zero if merge is not initialized. However, branch_release is\ncalled on every branch when tearing down a repository.\n\nAn upcoming change to submodule--helper will initialize the remote state\nby calling read_config(). In some cases, this results in branches in the\nremote_state which have a non-zero merge_nr but no merge array. This\nresults in a crash when tearing the repository down.\n\nTo fix this, lets simply check if merge is valid before attempting to\nrelease its contents.\n\nThis makes it safe to initialize the remote_state for a submodule\nrepository without crashing on teardown.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n remote.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 7092b12209d93e20ce1ed3b7d9e4cbac058c57ff..1035f0cd32d034cce05bd2a3d829ec90795ff4e2 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -253,9 +253,11 @@ static void branch_release(struct branch *branch)\n \tfree((char *)branch->refname);\n \tfree(branch->remote_name);\n \tfree(branch->pushremote_name);\n-\tfor (int i = 0; i < branch->merge_nr; i++)\n-\t\trefspec_item_clear(branch->merge[i]);\n-\tfree(branch->merge);\n+\tif (branch->merge) {\n+\t\tfor (int i = 0; i < branch->merge_nr; i++)\n+\t\t\trefspec_item_clear(branch->merge[i]);\n+\t\tfree(branch->merge);\n+\t}\n }\n \n static struct rewrite *make_rewrite(struct rewrites *r,\n\n-- \n2.48.1.397.gec9d649cc640\n\n"},{"id":"520069","messageId":"20250610-jk-submodule-helper-use-url-v1-4-6d14c1504e91@gmail.com","threadId":"63620","inReplyTo":"20250610-jk-submodule-helper-use-url-v1-0-6d14c1504e91@gmail.com","subject":"[PATCH 4/6] submodule--helper: improve logic for fallback remote name","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-11T00:52:18Z","receivedAt":"2025-06-11T00:52:29Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nThe repo_get_default_remote() function in submodule--helper currently\ntries to figure out the proper remote name to use for a submodule based\non a few factors.\n\nFirst, it tries to find the remote for the currently checked out branch.\nThis works if the submodule is configured to checkout to a branch\ninstead of a detached HEAD state.\n\nIn the detached HEAD state, the code calls back to using \"origin\", on\nthe assumption that this is the default remote name. Some users may\nchange this, such as by setting clone.defaultRemoteName, or by changing\nthe remote name manually within the submodule repository.\n\nAs a first step to improving this situation, refactor to reuse the logic\nfrom remotes_remote_for_branch(). This function uses the remote from the\nbranch if it has one. If it doesn't then it checks to see if there is\nexactly one remote. It uses this remote first before attempting to fall\nback to \"origin\".\n\nTo allow using this helper function, introduce a repo_default_remote()\nhelper to remote.c which takes a repository structure. This helper will\nload the remote configuration and get the \"HEAD\" branch. Then it will\ncall remotes_remote_for_branch to find the default remote.\n\nThis method allows re-using the same existing logic as other flows,\nrather than duplicating it in submodule--helper.c.\n\nThis isn't a perfect solution for users who change remote names, but it\nshould help in cases where the remote name is changed but users haven't\nadded any additional remotes.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n remote.h                    |  2 ++\n builtin/submodule--helper.c | 18 +++---------------\n remote.c                    | 25 ++++++++++++++++++++-----\n t/t7406-submodule-update.sh | 29 +++++++++++++++++++++++++++++\n 4 files changed, 54 insertions(+), 20 deletions(-)\n\ndiff --git a/remote.h b/remote.h\nindex 7e4943ae3a70ecefa3332d211084762ca30b59b6..ef0de4aa64e9ccd32cc2eea076c00386dcba1161 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -338,6 +338,8 @@ const char *remote_for_branch(struct branch *branch, int *explicit);\n const char *pushremote_for_branch(struct branch *branch, int *explicit);\n char *remote_ref_for_branch(struct branch *branch, int for_push);\n \n+const char *repo_default_remote(struct repository *repo);\n+\n /* returns true if the given branch has merge configuration given. */\n int branch_has_merge_config(struct branch *branch);\n \ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 9e8cdfe1b2a8c2985d9c1b8ad6f1b0d1f9401714..ef3ff65a80f398c5ac35660288290ad92c7132c7 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -43,8 +43,6 @@ typedef void (*each_submodule_fn)(const struct cache_entry *list_item,\n \n static int repo_get_default_remote(struct repository *repo, char **default_remote)\n {\n-\tchar *dest = NULL;\n-\tstruct strbuf sb = STRBUF_INIT;\n \tstruct ref_store *store = get_main_ref_store(repo);\n \tconst char *refname = refs_resolve_ref_unsafe(store, \"HEAD\", 0, NULL,\n \t\t\t\t\t\t      NULL);\n@@ -52,23 +50,13 @@ static int repo_get_default_remote(struct repository *repo, char **default_remot\n \tif (!refname)\n \t\treturn die_message(_(\"No such ref: %s\"), \"HEAD\");\n \n-\t/* detached HEAD */\n-\tif (!strcmp(refname, \"HEAD\")) {\n-\t\t*default_remote = xstrdup(\"origin\");\n-\t\treturn 0;\n-\t}\n-\n-\tif (!skip_prefix(refname, \"refs/heads/\", &refname))\n+\tif (strcmp(refname, \"HEAD\") &&\n+\t    !skip_prefix(refname, \"refs/heads/\", &refname))\n \t\treturn die_message(_(\"Expecting a full ref name, got %s\"),\n \t\t\t\t   refname);\n \n-\tstrbuf_addf(&sb, \"branch.%s.remote\", refname);\n-\tif (repo_config_get_string(repo, sb.buf, &dest))\n-\t\t*default_remote = xstrdup(\"origin\");\n-\telse\n-\t\t*default_remote = dest;\n+\t*default_remote = xstrdup(repo_default_remote(repo));\n \n-\tstrbuf_release(&sb);\n \treturn 0;\n }\n \ndiff --git a/remote.c b/remote.c\nindex 1035f0cd32d034cce05bd2a3d829ec90795ff4e2..fcda185ecfab5102afbe8918fed65c74971ef8c2 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1759,20 +1759,35 @@ static void set_merge(struct repository *repo, struct branch *ret)\n \t}\n }\n \n-struct branch *branch_get(const char *name)\n+static struct branch *repo_branch_get(struct repository *repo, const char *name)\n {\n \tstruct branch *ret;\n \n-\tread_config(the_repository, 0);\n+\tread_config(repo, 0);\n \tif (!name || !*name || !strcmp(name, \"HEAD\"))\n-\t\tret = the_repository->remote_state->current_branch;\n+\t\tret = repo->remote_state->current_branch;\n \telse\n-\t\tret = make_branch(the_repository->remote_state, name,\n+\t\tret = make_branch(repo->remote_state, name,\n \t\t\t\t  strlen(name));\n-\tset_merge(the_repository, ret);\n+\tset_merge(repo, ret);\n \treturn ret;\n }\n \n+struct branch *branch_get(const char *name)\n+{\n+\treturn repo_branch_get(the_repository, name);\n+}\n+\n+const char *repo_default_remote(struct repository *repo)\n+{\n+\tstruct branch *branch;\n+\n+\tread_config(repo, 0);\n+\tbranch = repo_branch_get(repo, \"HEAD\");\n+\n+\treturn remotes_remote_for_branch(repo->remote_state, branch, NULL);\n+}\n+\n int branch_has_merge_config(struct branch *branch)\n {\n \treturn branch && !!branch->merge;\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex c562bad042ab2d4d0f82cb8b57a1eadbe24044d1..748b529745a5121f121768bb4e0cbc11bc833ea4 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -1134,6 +1134,35 @@ test_expect_success 'setup clean recursive superproject' '\n \tgit clone --recurse-submodules top top-clean\n '\n \n+test_expect_success 'submodule update with renamed remote' '\n+\ttest_when_finished \"rm -fr top-cloned\" &&\n+\tcp -r top-clean top-cloned &&\n+\n+\t# Create a commit in each repo, starting with bottom\n+\ttest_commit -C bottom rename_commit &&\n+\t# Create middle commit\n+\tgit -C middle/bottom fetch &&\n+\tgit -C middle/bottom checkout -f FETCH_HEAD &&\n+\tgit -C middle add bottom &&\n+\tgit -C middle commit -m \"rename_commit\" &&\n+\t# Create top commit\n+\tgit -C top/middle fetch &&\n+\tgit -C top/middle checkout -f FETCH_HEAD &&\n+\tgit -C top add middle &&\n+\tgit -C top commit -m \"rename_commit\" &&\n+\n+\t# rename the submodule remote\n+\tgit -C top-cloned/middle remote rename origin upstream &&\n+\n+\t# Make the update of \"middle\" a no-op, otherwise we error out\n+\t# because of its unmerged state\n+\ttest_config -C top-cloned submodule.middle.update !true &&\n+\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n+\tcat >expect.err <<-\\EOF &&\n+\tEOF\n+\ttest_cmp expect.err actual.err\n+'\n+\n test_expect_success 'submodule update should skip unmerged submodules' '\n \ttest_when_finished \"rm -fr top-cloned\" &&\n \tcp -r top-clean top-cloned &&\n\n-- \n2.48.1.397.gec9d649cc640\n\n"},{"id":"520070","messageId":"20250610-jk-submodule-helper-use-url-v1-5-6d14c1504e91@gmail.com","threadId":"63620","inReplyTo":"20250610-jk-submodule-helper-use-url-v1-0-6d14c1504e91@gmail.com","subject":"[PATCH 5/6] submodule: move get_default_remote_submodule()","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-11T00:52:19Z","receivedAt":"2025-06-11T00:52:30Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nA future refactor got get_default_remote_submodule() is going to depend on\nresolve_relative_url(). That function depends on get_default_remote().\n\nMove get_default_remote_submodule() after resolve_relative_url() first\nto make the additional functionality easier to review.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n builtin/submodule--helper.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ef3ff65a80f398c5ac35660288290ad92c7132c7..5542b403217b979d6da92c79d89d0991e980f692 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -60,21 +60,6 @@ static int repo_get_default_remote(struct repository *repo, char **default_remot\n \treturn 0;\n }\n \n-static int get_default_remote_submodule(const char *module_path, char **default_remote)\n-{\n-\tstruct repository subrepo;\n-\tint ret;\n-\n-\tif (repo_submodule_init(&subrepo, the_repository, module_path,\n-\t\t\t\tnull_oid(the_hash_algo)) < 0)\n-\t\treturn die_message(_(\"could not get a repository handle for submodule '%s'\"),\n-\t\t\t\t   module_path);\n-\tret = repo_get_default_remote(&subrepo, default_remote);\n-\trepo_clear(&subrepo);\n-\n-\treturn ret;\n-}\n-\n static char *get_default_remote(void)\n {\n \tchar *default_remote;\n@@ -110,6 +95,21 @@ static char *resolve_relative_url(const char *rel_url, const char *up_path, int\n \treturn resolved_url;\n }\n \n+static int get_default_remote_submodule(const char *module_path, char **default_remote)\n+{\n+\tstruct repository subrepo;\n+\tint ret;\n+\n+\tif (repo_submodule_init(&subrepo, the_repository, module_path,\n+\t\t\t\tnull_oid(the_hash_algo)) < 0)\n+\t\treturn die_message(_(\"could not get a repository handle for submodule '%s'\"),\n+\t\t\t\t   module_path);\n+\tret = repo_get_default_remote(&subrepo, default_remote);\n+\trepo_clear(&subrepo);\n+\n+\treturn ret;\n+}\n+\n /* the result should be freed by the caller. */\n static char *get_submodule_displaypath(const char *path, const char *prefix,\n \t\t\t\t       const char *super_prefix)\n\n-- \n2.48.1.397.gec9d649cc640\n\n"},{"id":"520071","messageId":"20250610-jk-submodule-helper-use-url-v1-6-6d14c1504e91@gmail.com","threadId":"63620","inReplyTo":"20250610-jk-submodule-helper-use-url-v1-0-6d14c1504e91@gmail.com","subject":"[PATCH 6/6] submodule: look up remotes by URL first","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-11T00:52:20Z","receivedAt":"2025-06-11T00:52:30Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nThe get_default_remote_submodule() function performs a lookup to find\nthe appropriate remote to use within a submodule. The function first\nchecks to see if it can find the remote for the current branch. If this\nfails, it then checks to see if there is exactly one remote. It will use\nthis, before finally falling back to \"origin\" as the default.\n\nIf a user happens to rename their default remote from origin, either\nmanually or by setting something like clone.defaultRemoteName, this\nfallback will not work.\n\nIn such cases, the submodule logic will try to use a non-existent\nremote. This usually manifests as a failure to trigger the submodule\nupdate.\n\nThe parent project already knows and stores the submodule URL in either\n.gitmodules or its .git/config.\n\nAdd a new repo_remote_from_url() helper which will iterate over all the\nremotes in a repository and return the first remote which has a matching\nURL.\n\nRefactor repo_get_default_remote to take an optional url parameter. If\nits set, first attempt to use repo_remote_from_url(), before attempting\nto use the existing logic.\n\nRefactor get_default_remote_submodule to find the submodule and get its\nURL. If a valid URL exists, then pass this to repo_get_default_remote,\nensuring that we prioritize using a remote with the matching URL if it\nexists.\n\nThe fallback logic is kept in case for some reason the user has manually\nchanged the URL within the submodule. Additionally, we still try to use\na remote rather than directly passing the URL in the\nfetch_in_submodule() logic. This ensures that an update will properly\nupdate the remote refs within the submodule as expected, rather than\njust fetching into FETCH_HEAD.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n remote.h                    |  1 +\n builtin/submodule--helper.c | 43 ++++++++++++++++++++++++++++++++++++-------\n remote.c                    | 15 +++++++++++++++\n t/t7406-submodule-update.sh | 32 ++++++++++++++++++++++++++++++++\n 4 files changed, 84 insertions(+), 7 deletions(-)\n\ndiff --git a/remote.h b/remote.h\nindex ef0de4aa64e9ccd32cc2eea076c00386dcba1161..8b8ece50b1cd684969fbb8a9d695fb3002a3f3f5 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -339,6 +339,7 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit);\n char *remote_ref_for_branch(struct branch *branch, int for_push);\n \n const char *repo_default_remote(struct repository *repo);\n+const char *repo_remote_from_url(struct repository *repo, const char *url);\n \n /* returns true if the given branch has merge configuration given. */\n int branch_has_merge_config(struct branch *branch);\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 5542b403217b979d6da92c79d89d0991e980f692..432fe5d14f78756b64cf1770c174ce49bc2978f9 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -41,15 +41,25 @@\n typedef void (*each_submodule_fn)(const struct cache_entry *list_item,\n \t\t\t\t  void *cb_data);\n \n-static int repo_get_default_remote(struct repository *repo, char **default_remote)\n+static int repo_get_default_remote(struct repository *repo,\n+\t\t\t\t   const char *url,\n+\t\t\t\t   char **default_remote)\n {\n-\tstruct ref_store *store = get_main_ref_store(repo);\n-\tconst char *refname = refs_resolve_ref_unsafe(store, \"HEAD\", 0, NULL,\n-\t\t\t\t\t\t      NULL);\n+\tstruct ref_store *store;\n+\tconst char *refname;\n \n+\tif (url) {\n+\t\tconst char *remote_name = repo_remote_from_url(repo, url);\n+\t\tif (remote_name) {\n+\t\t\t*default_remote = xstrdup(remote_name);\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n+\tstore = get_main_ref_store(repo);\n+\trefname = refs_resolve_ref_unsafe(store, \"HEAD\", 0, NULL, NULL);\n \tif (!refname)\n \t\treturn die_message(_(\"No such ref: %s\"), \"HEAD\");\n-\n \tif (strcmp(refname, \"HEAD\") &&\n \t    !skip_prefix(refname, \"refs/heads/\", &refname))\n \t\treturn die_message(_(\"Expecting a full ref name, got %s\"),\n@@ -63,7 +73,7 @@ static int repo_get_default_remote(struct repository *repo, char **default_remot\n static char *get_default_remote(void)\n {\n \tchar *default_remote;\n-\tint code = repo_get_default_remote(the_repository, &default_remote);\n+\tint code = repo_get_default_remote(the_repository, NULL, &default_remote);\n \n \tif (code)\n \t\texit(code);\n@@ -97,16 +107,35 @@ static char *resolve_relative_url(const char *rel_url, const char *up_path, int\n \n static int get_default_remote_submodule(const char *module_path, char **default_remote)\n {\n+\tconst struct submodule *sub;\n \tstruct repository subrepo;\n+\tchar *url = NULL;\n \tint ret;\n \n+\tsub = submodule_from_path(the_repository, null_oid(the_hash_algo), module_path);\n+\tif (sub && sub->url) {\n+\t\turl = xstrdup(sub->url);\n+\n+\t\t/* Possibly a url relative to parent */\n+\t\tif (starts_with_dot_dot_slash(url) ||\n+\t\t    starts_with_dot_slash(url)) {\n+\t\t\tchar *oldurl = url;\n+\n+\t\t\turl = resolve_relative_url(oldurl, NULL, 1);\n+\t\t\tfree(oldurl);\n+\t\t}\n+\t}\n+\n \tif (repo_submodule_init(&subrepo, the_repository, module_path,\n \t\t\t\tnull_oid(the_hash_algo)) < 0)\n \t\treturn die_message(_(\"could not get a repository handle for submodule '%s'\"),\n \t\t\t\t   module_path);\n-\tret = repo_get_default_remote(&subrepo, default_remote);\n+\tret = repo_get_default_remote(&subrepo, url, default_remote);\n \trepo_clear(&subrepo);\n \n+\tif (url)\n+\t\tfree(url);\n+\n \treturn ret;\n }\n \ndiff --git a/remote.c b/remote.c\nindex fcda185ecfab5102afbe8918fed65c74971ef8c2..bf2ebfcf06a87ddf1fecadc3e408427d0a83c479 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1788,6 +1788,21 @@ const char *repo_default_remote(struct repository *repo)\n \treturn remotes_remote_for_branch(repo->remote_state, branch, NULL);\n }\n \n+const char *repo_remote_from_url(struct repository *repo, const char *url)\n+{\n+\tread_config(repo, 0);\n+\n+\tfor (int i = 0; i < repo->remote_state->remotes_nr; i++) {\n+\t\tstruct remote *remote = repo->remote_state->remotes[i];\n+\t\tif (!remote)\n+\t\t\tcontinue;\n+\n+\t\tif (remote_has_url(remote, url))\n+\t\t\treturn remote->name;\n+\t}\n+\treturn NULL;\n+}\n+\n int branch_has_merge_config(struct branch *branch)\n {\n \treturn branch && !!branch->merge;\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex 748b529745a5121f121768bb4e0cbc11bc833ea4..c09047b5f441a73a02a9fc4197e9a0ea8f39b529 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -1134,6 +1134,38 @@ test_expect_success 'setup clean recursive superproject' '\n \tgit clone --recurse-submodules top top-clean\n '\n \n+test_expect_success 'submodule update with multiple remotes' '\n+\ttest_when_finished \"rm -fr top-cloned\" &&\n+\tcp -r top-clean top-cloned &&\n+\n+\t# Create a commit in each repo, starting with bottom\n+\ttest_commit -C bottom multiple_remote_commit &&\n+\t# Create middle commit\n+\tgit -C middle/bottom fetch &&\n+\tgit -C middle/bottom checkout -f FETCH_HEAD &&\n+\tgit -C middle add bottom &&\n+\tgit -C middle commit -m \"multiple_remote_commit\" &&\n+\t# Create top commit\n+\tgit -C top/middle fetch &&\n+\tgit -C top/middle checkout -f FETCH_HEAD &&\n+\tgit -C top add middle &&\n+\tgit -C top commit -m \"multiple_remote_commit\" &&\n+\n+\t# rename the submodule remote\n+\tgit -C top-cloned/middle remote rename origin upstream &&\n+\n+\t# Add another remote\n+\tgit -C top-cloned/middle remote add other bogus &&\n+\n+\t# Make the update of \"middle\" a no-op, otherwise we error out\n+\t# because of its unmerged state\n+\ttest_config -C top-cloned submodule.middle.update !true &&\n+\tgit -C top-cloned submodule update --recursive 2>actual.err &&\n+\tcat >expect.err <<-\\EOF &&\n+\tEOF\n+\ttest_cmp expect.err actual.err\n+'\n+\n test_expect_success 'submodule update with renamed remote' '\n \ttest_when_finished \"rm -fr top-cloned\" &&\n \tcp -r top-clean top-cloned &&\n\n-- \n2.48.1.397.gec9d649cc640\n\n"},{"id":"520299","messageId":"f16e09e8-55f8-4fb7-825d-fe4c9b049d1c@intel.com","threadId":"63620","inReplyTo":"20250610-jk-submodule-helper-use-url-v1-0-6d14c1504e91@gmail.com","subject":"Re: [PATCH 0/6] submodule: improve remote lookup logic","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-16T22:27:03Z","receivedAt":"2025-06-16T22:27:26Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 6/10/2025 5:52 PM, Jacob Keller wrote:\n> This series improves the git submodule remote lookup logic implemented in\n> submodule--helper.\n> \n> A few cleanups are done first:\n> \n> * The starts_with_dot(_dot)_slash helper functions are moved to dir.h for\n>   re-use, as these are used both within submodule--helper.c and\n>   submodule-config.c\n> \n> * Several remote.c helper functions are refactored to take repository\n>   pointers, enabling use with a submodule repository pointer.\n> \n> * The branch_release logic is fixed so that it won't dereference\n>   branch->merge if it was never setup. This appeared to trigger in some\n>   cases if a submodule repository was passed into the read_config()\n>   function.\n> \n> Next, the submodule--helper.c logic in repo_get_default_remote() is\n> refactored to remote.c helper functions. A new repo_default_remote() helper\n> function is added which will try to find a default remote. This helper\n> first tries to look up the remote from the checked out branch, then falls\n> back to the only remote (if there is exactly one remote) before finally\n> falling back to \"origin\".\n> \n> This improved logic is a good first step, but won't handle cases where\n> there are multiple remotes, and when remotes have been renamed.\n> \n> For the final improvement, notice that the parent project already stores\n> the URL for the submodule in its .git/config or .gitmodules file. This URL\n> is what we use to set the remote in the first place when cloning.\n> \n> Add a repo_remote_from_url() helper which will iterate through the remotes\n> and find the first remote with that URL. Use this in\n> repo_get_default_remote() to first try and find a remote by its URL. If\n> unsuccessful, we still keep the original fallback logic, in the off chance\n> that the user has changed the URL from within the submodule.\n> \n> This method is more robust as it is less likely that the user has manually\n> changed the submodule URL within the submodule but not also within the\n> .git/config.\n> \n> With this change, all commands which need the submodule remote will first\n> look up by URL before trying to use the fallback logic, and should now be\n> able to find a suitable remote regardless of now they are renamed.\n> \n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> ---\n> Jacob Keller (6):\n>       dir: move starts_with_dot(_dot)_slash to dir.h\n>       remote: remove the_repository from some functions\n>       remote: check branch->merge before access in branch_release\n>       submodule--helper: improve logic for fallback remote name\n>       submodule: move get_default_remote_submodule()\n>       submodule: look up remotes by URL first\n> \n>  dir.h                       |  23 ++++++++++\n>  remote.h                    |   3 ++\n>  builtin/submodule--helper.c | 101 ++++++++++++++++++++++--------------------\n>  remote.c                    | 104 ++++++++++++++++++++++++++++----------------\n>  submodule-config.c          |  12 -----\n>  t/t7406-submodule-update.sh |  61 ++++++++++++++++++++++++++\n>  6 files changed, 207 insertions(+), 97 deletions(-)\n> ---\n> base-commit: 4c0e625c091d4c648cec7319bafaed3cc81658e5\n> change-id: 20250610-jk-submodule-helper-use-url-e55d3c379faf\n> \n> Best regards,\n\nSeems like there hasn't been any interest in this series? :(\n"},{"id":"520302","messageId":"xmqqbjqnz4ve.fsf@gitster.g","threadId":"63620","inReplyTo":"f16e09e8-55f8-4fb7-825d-fe4c9b049d1c@intel.com","subject":"Re: [PATCH 0/6] submodule: improve remote lookup logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-16T22:41:25Z","receivedAt":"2025-06-16T22:41:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> Seems like there hasn't been any interest in this series? :(\n\nSo far it seems, but we have been in pre-release freeze for a few\nweeks combined with slower summer (in northern hemisphere anyway)\nseason, so it might have just been a bad timing.\n"},{"id":"520304","messageId":"83af9bc9-7884-4ce8-a343-28dbcded7954@intel.com","threadId":"63620","inReplyTo":"xmqqbjqnz4ve.fsf@gitster.g","subject":"Re: [PATCH 0/6] submodule: improve remote lookup logic","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-16T23:20:33Z","receivedAt":"2025-06-16T23:20:59Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 6/16/2025 3:41 PM, Junio C Hamano wrote:\n> Jacob Keller <jacob.e.keller@intel.com> writes:\n> \n>> Seems like there hasn't been any interest in this series? :(\n> \n> So far it seems, but we have been in pre-release freeze for a few\n> weeks combined with slower summer (in northern hemisphere anyway)\n> season, so it might have just been a bad timing.\n\nOh true. I can resend once release freeze ends :)\n"},{"id":"520306","messageId":"90B8957C-9469-4D46-9026-46407AD2EC1B@gmail.com","threadId":"63620","inReplyTo":"20250610-jk-submodule-helper-use-url-v1-4-6d14c1504e91@gmail.com","subject":"Re: [PATCH 4/6] submodule--helper: improve logic for fallback remote name","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-06-17T02:58:46Z","receivedAt":"2025-06-17T02:59:19Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes：\n> \n> From: Jacob Keller <jacob.keller@gmail.com>\n> \n> The repo_get_default_remote() function in submodule--helper currently\n> tries to figure out the proper remote name to use for a submodule based\n> on a few factors.\n> \n> First, it tries to find the remote for the currently checked out branch.\n> This works if the submodule is configured to checkout to a branch\n> instead of a detached HEAD state.\n> \n> In the detached HEAD state, the code calls back to using \"origin\", on\n> the assumption that this is the default remote name. Some users may\n> change this, such as by setting clone.defaultRemoteName, or by changing\n> the remote name manually within the submodule repository.\n> \n> As a first step to improving this situation, refactor to reuse the logic\n> from remotes_remote_for_branch(). This function uses the remote from the\n> branch if it has one. If it doesn't then it checks to see if there is\n> exactly one remote. It uses this remote first before attempting to fall\n> back to \"origin\".\n> \n> To allow using this helper function, introduce a repo_default_remote()\n> helper to remote.c which takes a repository structure. This helper will\n> load the remote configuration and get the \"HEAD\" branch. Then it will\n> call remotes_remote_for_branch to find the default remote.\n\nJust a thought: since repo_default_remote() is only used within\nrepo_get_default_remote(), and the two have very similar names,\ndo you think it might be clearer to inline the former into the latter?\n\n> This method allows re-using the same existing logic as other flows,\n> rather than duplicating it in submodule--helper.c.\n> \n> This isn't a perfect solution for users who change remote names, but it\n> should help in cases where the remote name is changed but users haven't\n> added any additional remotes.\n> \n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> ---\n> remote.h                    |  2 ++\n> builtin/submodule--helper.c | 18 +++---------------\n> remote.c                    | 25 ++++++++++++++++++++-----\n> t/t7406-submodule-update.sh | 29 +++++++++++++++++++++++++++++\n> 4 files changed, 54 insertions(+), 20 deletions(-)\n> \n> diff --git a/remote.h b/remote.h\n> index 7e4943ae3a70ecefa3332d211084762ca30b59b6..ef0de4aa64e9ccd32cc2eea076c00386dcba1161 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -338,6 +338,8 @@ const char *remote_for_branch(struct branch *branch, int *explicit);\n> const char *pushremote_for_branch(struct branch *branch, int *explicit);\n> char *remote_ref_for_branch(struct branch *branch, int for_push);\n> \n> +const char *repo_default_remote(struct repository *repo);\n> +\n> /* returns true if the given branch has merge configuration given. */\n> int branch_has_merge_config(struct branch *branch);\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 9e8cdfe1b2a8c2985d9c1b8ad6f1b0d1f9401714..ef3ff65a80f398c5ac35660288290ad92c7132c7 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -43,8 +43,6 @@ typedef void (*each_submodule_fn)(const struct cache_entry *list_item,\n> \n> static int repo_get_default_remote(struct repository *repo, char **default_remote)\n> {\n> - char *dest = NULL;\n> - struct strbuf sb = STRBUF_INIT;\n> struct ref_store *store = get_main_ref_store(repo);\n> const char *refname = refs_resolve_ref_unsafe(store, \"HEAD\", 0, NULL,\n>      NULL);\n> @@ -52,23 +50,13 @@ static int repo_get_default_remote(struct repository *repo, char **default_remot\n> if (!refname)\n> return die_message(_(\"No such ref: %s\"), \"HEAD\");\n> \n> - /* detached HEAD */\n> - if (!strcmp(refname, \"HEAD\")) {\n> - *default_remote = xstrdup(\"origin\");\n> - return 0;\n> - }\n> -\n> - if (!skip_prefix(refname, \"refs/heads/\", &refname))\n> + if (strcmp(refname, \"HEAD\") &&\n> +    !skip_prefix(refname, \"refs/heads/\", &refname))\n> return die_message(_(\"Expecting a full ref name, got %s\"),\n>   refname);\n> \n> - strbuf_addf(&sb, \"branch.%s.remote\", refname);\n> - if (repo_config_get_string(repo, sb.buf, &dest))\n> - *default_remote = xstrdup(\"origin\");\n> - else\n> - *default_remote = dest;\n> + *default_remote = xstrdup(repo_default_remote(repo));\n> \n> - strbuf_release(&sb);\n> return 0;\n> }\n> \n> diff --git a/remote.c b/remote.c\n> index 1035f0cd32d034cce05bd2a3d829ec90795ff4e2..fcda185ecfab5102afbe8918fed65c74971ef8c2 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1759,20 +1759,35 @@ static void set_merge(struct repository *repo, struct branch *ret)\n> }\n> }\n> \n> -struct branch *branch_get(const char *name)\n> +static struct branch *repo_branch_get(struct repository *repo, const char *name)\n> {\n> struct branch *ret;\n> \n> - read_config(the_repository, 0);\n> + read_config(repo, 0);\n> if (!name || !*name || !strcmp(name, \"HEAD\"))\n> - ret = the_repository->remote_state->current_branch;\n> + ret = repo->remote_state->current_branch;\n> else\n> - ret = make_branch(the_repository->remote_state, name,\n> + ret = make_branch(repo->remote_state, name,\n>  strlen(name));\n> - set_merge(the_repository, ret);\n> + set_merge(repo, ret);\n> return ret;\n> }\n> \n> +struct branch *branch_get(const char *name)\n> +{\n> + return repo_branch_get(the_repository, name);\n> +}\n> +\n> +const char *repo_default_remote(struct repository *repo)\n> +{\n> + struct branch *branch;\n> +\n> + read_config(repo, 0);\n> + branch = repo_branch_get(repo, \"HEAD\");\n> +\n> + return remotes_remote_for_branch(repo->remote_state, branch, NULL);\n> +}\n> +\n> int branch_has_merge_config(struct branch *branch)\n> {\n> return branch && !!branch->merge;\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index c562bad042ab2d4d0f82cb8b57a1eadbe24044d1..748b529745a5121f121768bb4e0cbc11bc833ea4 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -1134,6 +1134,35 @@ test_expect_success 'setup clean recursive superproject' '\n> git clone --recurse-submodules top top-clean\n> '\n> \n> +test_expect_success 'submodule update with renamed remote' '\n> + test_when_finished \"rm -fr top-cloned\" &&\n> + cp -r top-clean top-cloned &&\n> +\n> + # Create a commit in each repo, starting with bottom\n> + test_commit -C bottom rename_commit &&\n> + # Create middle commit\n> + git -C middle/bottom fetch &&\n> + git -C middle/bottom checkout -f FETCH_HEAD &&\n> + git -C middle add bottom &&\n> + git -C middle commit -m \"rename_commit\" &&\n> + # Create top commit\n> + git -C top/middle fetch &&\n> + git -C top/middle checkout -f FETCH_HEAD &&\n> + git -C top add middle &&\n> + git -C top commit -m \"rename_commit\" &&\n> +\n> + # rename the submodule remote\n> + git -C top-cloned/middle remote rename origin upstream &&\n> +\n> + # Make the update of \"middle\" a no-op, otherwise we error out\n> + # because of its unmerged state\n> + test_config -C top-cloned submodule.middle.update !true &&\n> + git -C top-cloned submodule update --recursive 2>actual.err &&\n> + cat >expect.err <<-\\EOF &&\n> + EOF\n> + test_cmp expect.err actual.err\n> +'\n> +\n> test_expect_success 'submodule update should skip unmerged submodules' '\n> test_when_finished \"rm -fr top-cloned\" &&\n> cp -r top-clean top-cloned &&\n> \n> -- \n> 2.48.1.397.gec9d649cc640\n> \n> \n\n"},{"id":"520319","messageId":"xmqqecvixzov.fsf@gitster.g","threadId":"63620","inReplyTo":"20250610-jk-submodule-helper-use-url-v1-4-6d14c1504e91@gmail.com","subject":"Re: [PATCH 4/6] submodule--helper: improve logic for fallback remote name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-17T13:30:56Z","receivedAt":"2025-06-17T13:30:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> diff --git a/remote.h b/remote.h\n> index 7e4943ae3a70ecefa3332d211084762ca30b59b6..ef0de4aa64e9ccd32cc2eea076c00386dcba1161 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -338,6 +338,8 @@ const char *remote_for_branch(struct branch *branch, int *explicit);\n>  const char *pushremote_for_branch(struct branch *branch, int *explicit);\n>  char *remote_ref_for_branch(struct branch *branch, int for_push);\n>  \n> +const char *repo_default_remote(struct repository *repo);\n> +\n\nThis breaks \"make hdr-check\" because nobody up to this point\nmentions \"struct repository\".\n\nIt seems doing an opaque forward declaration is in vogue at least in\nthis file?\n\n remote.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git c/remote.h w/remote.h\nindex ef0de4aa64..86020ae2d3 100644\n--- c/remote.h\n+++ w/remote.h\n@@ -9,6 +9,7 @@\n \n struct option;\n struct transport_ls_refs_options;\n+struct repository;\n \n /**\n  * The API gives access to the configuration related to remotes. It handles\n"},{"id":"520320","messageId":"xmqq4iwexv4n.fsf@gitster.g","threadId":"63620","inReplyTo":"83af9bc9-7884-4ce8-a343-28dbcded7954@intel.com","subject":"Re: [PATCH 0/6] submodule: improve remote lookup logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-17T15:09:28Z","receivedAt":"2025-06-17T15:09:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> On 6/16/2025 3:41 PM, Junio C Hamano wrote:\n>> Jacob Keller <jacob.e.keller@intel.com> writes:\n>> \n>>> Seems like there hasn't been any interest in this series? :(\n>> \n>> So far it seems, but we have been in pre-release freeze for a few\n>> weeks combined with slower summer (in northern hemisphere anyway)\n>> season, so it might have just been a bad timing.\n>\n> Oh true. I can resend once release freeze ends :)\n\nBefore doing so, can you run with leaksanitizer?  When merged to\n'jch', many tests fail and t1013-read-tree-submodule.sh #52 is one\nof them.\n\nThanks.\n\nnot ok 52 - git_test_func: modified submodule does not update submodule work tree to invalid commit\n#\n#                       prolog &&\n#                       reset_work_tree_to add_sub1 &&\n#                       (\n#                               cd submodule_update &&\n#                               git branch -t invalid_sub1 origin/invalid_sub1 &&\n#                               $command invalid_sub1 &&\n#                               test_superproject_content origin/invalid_sub1 &&\n#                               test_submodule_content sub1 origin/add_sub1 &&\n#                               test_must_fail git submodule update &&\n#                               test_submodule_content sub1 origin/add_sub1\n#                       )\n#\n1..52\n\n=================================================================\n==git==1541109==ERROR: LeakSanitizer: detected memory leaks\n\nDirect leak of 768 byte(s) in 1 object(s) allocated from:\n    #0 0x561ea1a821c7 in realloc (git+0x8a1c7) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)\n    #1 0x561ea1da4126 in xrealloc wrapper.c:140:8\n    #2 0x561ea1d181a8 in refspec_append refspec.c:209:2\n    #3 0x561ea1d1e698 in handle_config remote.c:506:3\n    #4 0x561ea1bf7878 in configset_iter config.c:2140:7\n    #5 0x561ea1bf775a in repo_config config.c:2519:2\n    #6 0x561ea1d195fd in read_config remote.c:615:2\n    #7 0x561ea1d1b6ba in repo_remote_from_url remote.c:1794:2\n    #8 0x561ea1b8ae07 in repo_get_default_remote builtin/submodule--helper.c:54:29\n    #9 0x561ea1b8d475 in get_default_remote_submodule builtin/submodule--helper.c:135:8\n    #10 0x561ea1b8d789 in fetch_in_submodule builtin/submodule--helper.c:2355:10\n    #11 0x561ea1b8d98e in run_update_procedure builtin/submodule--helper.c:2481:7\n    #12 0x561ea1b8c7b8 in update_submodule builtin/submodule--helper.c:2700:9\n    #13 0x561ea1b8ba35 in update_submodules builtin/submodule--helper.c:2784:10\n    #14 0x561ea1b87097 in module_update builtin/submodule--helper.c:2924:8\n    #15 0x561ea1b84b9d in cmd_submodule__helper builtin/submodule--helper.c:3646:9\n    #16 0x561ea1a86cfb in run_builtin git.c:480:11\n    #17 0x561ea1a858a0 in handle_builtin git.c:746:9\n    #18 0x561ea1a866a8 in run_argv git.c:813:4\n    #19 0x561ea1a8562b in cmd_main git.c:953:19\n    #20 0x561ea1ba41da in main common-main.c:9:11\n    #21 0x7f60975e6ca7 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n    #22 0x7f60975e6d64 in __libc_start_main csu/../csu/libc-start.c:360:3\n    #23 0x561ea1a52210 in _start (git+0x5a210) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)\n\nDEDUP_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\nDirect leak of 192 byte(s) in 1 object(s) allocated from:\n    #0 0x561ea1a821c7 in realloc (git+0x8a1c7) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)\n    #1 0x561ea1da4126 in xrealloc wrapper.c:140:8\n    #2 0x561ea1d1edb4 in add_merge remote.c:177:2\n    #3 0x561ea1d1e23c in handle_config remote.c:432:4\n    #4 0x561ea1bf7878 in configset_iter config.c:2140:7\n    #5 0x561ea1bf775a in repo_config config.c:2519:2\n    #6 0x561ea1d195fd in read_config remote.c:615:2\n    #7 0x561ea1d1b6ba in repo_remote_from_url remote.c:1794:2\n    #8 0x561ea1b8ae07 in repo_get_default_remote builtin/submodule--helper.c:54:29\n    #9 0x561ea1b8d475 in get_default_remote_submodule builtin/submodule--helper.c:135:8\n    #10 0x561ea1b8d789 in fetch_in_submodule builtin/submodule--helper.c:2355:10\n    #11 0x561ea1b8d98e in run_update_procedure builtin/submodule--helper.c:2481:7\n    #12 0x561ea1b8c7b8 in update_submodule builtin/submodule--helper.c:2700:9\n    #13 0x561ea1b8ba35 in update_submodules builtin/submodule--helper.c:2784:10\n    #14 0x561ea1b87097 in module_update builtin/submodule--helper.c:2924:8\n    #15 0x561ea1b84b9d in cmd_submodule__helper builtin/submodule--helper.c:3646:9\n    #16 0x561ea1a86cfb in run_builtin git.c:480:11\n    #17 0x561ea1a858a0 in handle_builtin git.c:746:9\n    #18 0x561ea1a866a8 in run_argv git.c:813:4\n    #19 0x561ea1a8562b in cmd_main git.c:953:19\n    #20 0x561ea1ba41da in main common-main.c:9:11\n    #21 0x7f60975e6ca7 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n    #22 0x7f60975e6d64 in __libc_start_main csu/../csu/libc-start.c:360:3\n    #23 0x561ea1a52210 in _start (git+0x5a210) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)\n\nDEDUP_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\nIndirect leak of 36 byte(s) in 1 object(s) allocated from:\n    #0 0x561ea1a81d3d in malloc (git+0x89d3d) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)\n    #1 0x7f60976667a9 in strdup string/strdup.c:42:15\n    #2 0x561ea1da3e14 in xstrdup wrapper.c:43:14\n    #3 0x561ea1d17f2b in refspec_item_init refspec.c:160:14\n    #4 0x561ea1d17ef1 in refspec_item_init_fetch refspec.c:166:9\n    #5 0x561ea1d180dd in refspec_append refspec.c:203:9\n    #6 0x561ea1d1e698 in handle_config remote.c:506:3\n    #7 0x561ea1bf7878 in configset_iter config.c:2140:7\n    #8 0x561ea1bf775a in repo_config config.c:2519:2\n    #9 0x561ea1d195fd in read_config remote.c:615:2\n    #10 0x561ea1d1b6ba in repo_remote_from_url remote.c:1794:2\n    #11 0x561ea1b8ae07 in repo_get_default_remote builtin/submodule--helper.c:54:29\n    #12 0x561ea1b8d475 in get_default_remote_submodule builtin/submodule--helper.c:135:8\n    #13 0x561ea1b8d789 in fetch_in_submodule builtin/submodule--helper.c:2355:10\n    #14 0x561ea1b8d98e in run_update_procedure builtin/submodule--helper.c:2481:7\n    #15 0x561ea1b8c7b8 in update_submodule builtin/submodule--helper.c:2700:9\n    #16 0x561ea1b8ba35 in update_submodules builtin/submodule--helper.c:2784:10\n    #17 0x561ea1b87097 in module_update builtin/submodule--helper.c:2924:8\n    #18 0x561ea1b84b9d in cmd_submodule__helper builtin/submodule--helper.c:3646:9\n    #19 0x561ea1a86cfb in run_builtin git.c:480:11\n    #20 0x561ea1a858a0 in handle_builtin git.c:746:9\n    #21 0x561ea1a866a8 in run_argv git.c:813:4\n    #22 0x561ea1a8562b in cmd_main git.c:953:19\n    #23 0x561ea1ba41da in main common-main.c:9:11\n    #24 0x7f60975e6ca7 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n    #25 0x7f60975e6d64 in __libc_start_main csu/../csu/libc-start.c:360:3\n    #26 0x561ea1a52210 in _start (git+0x5a210) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)\n\nDEDUP_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\nIndirect leak of 22 byte(s) in 1 object(s) allocated from:\n    #0 0x561ea1a81d3d in malloc (git+0x89d3d) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)\n    #1 0x561ea1da3e92 in do_xmalloc wrapper.c:55:8\n    #2 0x561ea1da3f94 in do_xmallocz wrapper.c:89:8\n    #3 0x561ea1da3f26 in xmallocz wrapper.c:97:9\n    #4 0x561ea1da3ff8 in xmemdupz wrapper.c:113:16\n    #5 0x561ea1da4077 in xstrndup wrapper.c:119:9\n    #6 0x561ea1d19118 in parse_refspec refspec.c:55:15\n    #7 0x561ea1d17f46 in refspec_item_init refspec.c:161:9\n    #8 0x561ea1d17ef1 in refspec_item_init_fetch refspec.c:166:9\n    #9 0x561ea1d180dd in refspec_append refspec.c:203:9\n    #10 0x561ea1d1e698 in handle_config remote.c:506:3\n    #11 0x561ea1bf7878 in configset_iter config.c:2140:7\n    #12 0x561ea1bf775a in repo_config config.c:2519:2\n    #13 0x561ea1d195fd in read_config remote.c:615:2\n    #14 0x561ea1d1b6ba in repo_remote_from_url remote.c:1794:2\n    #15 0x561ea1b8ae07 in repo_get_default_remote builtin/submodule--helper.c:54:29\n    #16 0x561ea1b8d475 in get_default_remote_submodule builtin/submodule--helper.c:135:8\n    #17 0x561ea1b8d789 in fetch_in_submodule builtin/submodule--helper.c:2355:10\n    #18 0x561ea1b8d98e in run_update_procedure builtin/submodule--helper.c:2481:7\n    #19 0x561ea1b8c7b8 in update_submodule builtin/submodule--helper.c:2700:9\n    #20 0x561ea1b8ba35 in update_submodules builtin/submodule--helper.c:2784:10\n    #21 0x561ea1b87097 in module_update builtin/submodule--helper.c:2924:8\n    #22 0x561ea1b84b9d in cmd_submodule__helper builtin/submodule--helper.c:3646:9\n    #23 0x561ea1a86cfb in run_builtin git.c:480:11\n    #24 0x561ea1a858a0 in handle_builtin git.c:746:9\n    #25 0x561ea1a866a8 in run_argv git.c:813:4\n    #26 0x561ea1a8562b in cmd_main git.c:953:19\n    #27 0x561ea1ba41da in main common-main.c:9:11\n\nDEDUP_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\nIndirect leak of 18 byte(s) in 1 object(s) allocated from:\n    #0 0x561ea1a81d3d in malloc (git+0x89d3d) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)\n    #1 0x7f60976667a9 in strdup string/strdup.c:42:15\n    #2 0x561ea1da3e14 in xstrdup wrapper.c:43:14\n    #3 0x561ea1d1e22d in handle_config remote.c:432:22\n    #4 0x561ea1bf7878 in configset_iter config.c:2140:7\n    #5 0x561ea1bf775a in repo_config config.c:2519:2\n    #6 0x561ea1d195fd in read_config remote.c:615:2\n    #7 0x561ea1d1b6ba in repo_remote_from_url remote.c:1794:2\n    #8 0x561ea1b8ae07 in repo_get_default_remote builtin/submodule--helper.c:54:29\n    #9 0x561ea1b8d475 in get_default_remote_submodule builtin/submodule--helper.c:135:8\n    #10 0x561ea1b8d789 in fetch_in_submodule builtin/submodule--helper.c:2355:10\n    #11 0x561ea1b8d98e in run_update_procedure builtin/submodule--helper.c:2481:7\n    #12 0x561ea1b8c7b8 in update_submodule builtin/submodule--helper.c:2700:9\n    #13 0x561ea1b8ba35 in update_submodules builtin/submodule--helper.c:2784:10\n    #14 0x561ea1b87097 in module_update builtin/submodule--helper.c:2924:8\n    #15 0x561ea1b84b9d in cmd_submodule__helper builtin/submodule--helper.c:3646:9\n    #16 0x561ea1a86cfb in run_builtin git.c:480:11\n    #17 0x561ea1a858a0 in handle_builtin git.c:746:9\n    #18 0x561ea1a866a8 in run_argv git.c:813:4\n    #19 0x561ea1a8562b in cmd_main git.c:953:19\n    #20 0x561ea1ba41da in main common-main.c:9:11\n    #21 0x7f60975e6ca7 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n    #22 0x7f60975e6d64 in __libc_start_main csu/../csu/libc-start.c:360:3\n    #23 0x561ea1a52210 in _start (git+0x5a210) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)\n\nDEDUP_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\nIndirect leak of 13 byte(s) in 1 object(s) allocated from:\n    #0 0x561ea1a81d3d in malloc (git+0x89d3d) (BuildId: 8e698d3663135d471ee5cb682ce22cd9ec7b4322)\n    #1 0x561ea1da3e92 in do_xmalloc wrapper.c:55:8\n    #2 0x561ea1da3f94 in do_xmallocz wrapper.c:89:8\n    #3 0x561ea1da3f26 in xmallocz wrapper.c:97:9\n    #4 0x561ea1da3ff8 in xmemdupz wrapper.c:113:16\n    #5 0x561ea1da4077 in xstrndup wrapper.c:119:9\n    #6 0x561ea1d1923b in parse_refspec refspec.c:73:15\n    #7 0x561ea1d17f46 in refspec_item_init refspec.c:161:9\n    #8 0x561ea1d17ef1 in refspec_item_init_fetch refspec.c:166:9\n    #9 0x561ea1d180dd in refspec_append refspec.c:203:9\n    #10 0x561ea1d1e698 in handle_config remote.c:506:3\n    #11 0x561ea1bf7878 in configset_iter config.c:2140:7\n    #12 0x561ea1bf775a in repo_config config.c:2519:2\n    #13 0x561ea1d195fd in read_config remote.c:615:2\n    #14 0x561ea1d1b6ba in repo_remote_from_url remote.c:1794:2\n    #15 0x561ea1b8ae07 in repo_get_default_remote builtin/submodule--helper.c:54:29\n    #16 0x561ea1b8d475 in get_default_remote_submodule builtin/submodule--helper.c:135:8\n    #17 0x561ea1b8d789 in fetch_in_submodule builtin/submodule--helper.c:2355:10\n    #18 0x561ea1b8d98e in run_update_procedure builtin/submodule--helper.c:2481:7\n    #19 0x561ea1b8c7b8 in update_submodule builtin/submodule--helper.c:2700:9\n    #20 0x561ea1b8ba35 in update_submodules builtin/submodule--helper.c:2784:10\n    #21 0x561ea1b87097 in module_update builtin/submodule--helper.c:2924:8\n    #22 0x561ea1b84b9d in cmd_submodule__helper builtin/submodule--helper.c:3646:9\n    #23 0x561ea1a86cfb in run_builtin git.c:480:11\n    #24 0x561ea1a858a0 in handle_builtin git.c:746:9\n    #25 0x561ea1a866a8 in run_argv git.c:813:4\n    #26 0x561ea1a8562b in cmd_main git.c:953:19\n    #27 0x561ea1ba41da in main common-main.c:9:11\n\nDEDUP_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\nSUMMARY: LeakSanitizer: 1049 byte(s) leaked in 6 allocation(s).\nOur logs revealed a memory leak...\n\n\n\n"},{"id":"520322","messageId":"xmqqv7ouuyw3.fsf@gitster.g","threadId":"63620","inReplyTo":"xmqq4iwexv4n.fsf@gitster.g","subject":"Re: [PATCH 0/6] submodule: improve remote lookup logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-17T16:16:28Z","receivedAt":"2025-06-17T16:16:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jacob Keller <jacob.e.keller@intel.com> writes:\n>\n>> On 6/16/2025 3:41 PM, Junio C Hamano wrote:\n>>> Jacob Keller <jacob.e.keller@intel.com> writes:\n>>> \n>>>> Seems like there hasn't been any interest in this series? :(\n>>> \n>>> So far it seems, but we have been in pre-release freeze for a few\n>>> weeks combined with slower summer (in northern hemisphere anyway)\n>>> season, so it might have just been a bad timing.\n>>\n>> Oh true. I can resend once release freeze ends :)\n>\n> Before doing so, can you run with leaksanitizer?  When merged to\n> 'jch', many tests fail and t1013-read-tree-submodule.sh #52 is one\n> of them.\n>\n> Thanks.\n\nhttps://github.com/git/git/actions/runs/15711453001/job/44270362056\n\nis a log of running the CI suite on this topic alone (tentatively\npushed to 'seen').\n"},{"id":"520326","messageId":"974293a0-6f5f-4077-9977-84c07fe431ca@intel.com","threadId":"63620","inReplyTo":"90B8957C-9469-4D46-9026-46407AD2EC1B@gmail.com","subject":"Re: [PATCH 4/6] submodule--helper: improve logic for fallback remote name","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-17T17:53:10Z","receivedAt":"2025-06-17T17:53:28Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 6/16/2025 7:58 PM, Lidong Yan wrote:\n> Jacob Keller <jacob.e.keller@intel.com> writes：\n>>\n>> From: Jacob Keller <jacob.keller@gmail.com>\n>>\n>> The repo_get_default_remote() function in submodule--helper currently\n>> tries to figure out the proper remote name to use for a submodule based\n>> on a few factors.\n>>\n>> First, it tries to find the remote for the currently checked out branch.\n>> This works if the submodule is configured to checkout to a branch\n>> instead of a detached HEAD state.\n>>\n>> In the detached HEAD state, the code calls back to using \"origin\", on\n>> the assumption that this is the default remote name. Some users may\n>> change this, such as by setting clone.defaultRemoteName, or by changing\n>> the remote name manually within the submodule repository.\n>>\n>> As a first step to improving this situation, refactor to reuse the logic\n>> from remotes_remote_for_branch(). This function uses the remote from the\n>> branch if it has one. If it doesn't then it checks to see if there is\n>> exactly one remote. It uses this remote first before attempting to fall\n>> back to \"origin\".\n>>\n>> To allow using this helper function, introduce a repo_default_remote()\n>> helper to remote.c which takes a repository structure. This helper will\n>> load the remote configuration and get the \"HEAD\" branch. Then it will\n>> call remotes_remote_for_branch to find the default remote.\n> \n> Just a thought: since repo_default_remote() is only used within\n> repo_get_default_remote(), and the two have very similar names,\n> do you think it might be clearer to inline the former into the latter?\n> \nProbably, but I can't inline it within submodule--helper.c because it\nneeds access to static functions in remote.c which I don't think make\nsense to export.\n\nI could also either move repo_get_default_remote() from\nsubmodule--helper.c into remote.c, or try to eliminate it entirely.\n\nMy original thought was that it does some submodule-specific checks\nwhich I don't want to remove but which don't seem to make sense\nin-context with how the other remote.c helpers were implemented. Thus,\nit didn't make sense to move it into remote.c, but I can't put\nrepo_default_remote() contents into submodule--helper.c without exposing\nthe read_config() and other static functions in remote.c\n\nI'll think about this while resolving the memory issues and working on\nthe next version.\n"},{"id":"520329","messageId":"4773e9a4-778d-453f-93b0-cf37a660145e@intel.com","threadId":"63620","inReplyTo":"90B8957C-9469-4D46-9026-46407AD2EC1B@gmail.com","subject":"Re: [PATCH 4/6] submodule--helper: improve logic for fallback remote name","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-17T18:57:13Z","receivedAt":"2025-06-17T18:57:30Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 6/16/2025 7:58 PM, Lidong Yan wrote:\n> Jacob Keller <jacob.e.keller@intel.com> writes：\n>>\n>> From: Jacob Keller <jacob.keller@gmail.com>\n>>\n>> The repo_get_default_remote() function in submodule--helper currently\n>> tries to figure out the proper remote name to use for a submodule based\n>> on a few factors.\n>>\n>> First, it tries to find the remote for the currently checked out branch.\n>> This works if the submodule is configured to checkout to a branch\n>> instead of a detached HEAD state.\n>>\n>> In the detached HEAD state, the code calls back to using \"origin\", on\n>> the assumption that this is the default remote name. Some users may\n>> change this, such as by setting clone.defaultRemoteName, or by changing\n>> the remote name manually within the submodule repository.\n>>\n>> As a first step to improving this situation, refactor to reuse the logic\n>> from remotes_remote_for_branch(). This function uses the remote from the\n>> branch if it has one. If it doesn't then it checks to see if there is\n>> exactly one remote. It uses this remote first before attempting to fall\n>> back to \"origin\".\n>>\n>> To allow using this helper function, introduce a repo_default_remote()\n>> helper to remote.c which takes a repository structure. This helper will\n>> load the remote configuration and get the \"HEAD\" branch. Then it will\n>> call remotes_remote_for_branch to find the default remote.\n> \n> Just a thought: since repo_default_remote() is only used within\n> repo_get_default_remote(), and the two have very similar names,\n> do you think it might be clearer to inline the former into the latter?\n> \nI will try to rename this function, but I think in context it makes\nsense to keep a separate function in remote.c which does the generic-ish\nstuff while the submodule--helper.c function does the submodule-specific\nstuff. This is because the later patches add the URL look up support to\nthis function.\n"},{"id":"520331","messageId":"7333eae0-91ec-4aec-a13f-530074f0d7b6@intel.com","threadId":"63620","inReplyTo":"xmqq4iwexv4n.fsf@gitster.g","subject":"Re: [PATCH 0/6] submodule: improve remote lookup logic","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-17T21:28:46Z","receivedAt":"2025-06-17T21:29:05Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 6/17/2025 8:09 AM, Junio C Hamano wrote:\n> Jacob Keller <jacob.e.keller@intel.com> writes:\n> \n>> On 6/16/2025 3:41 PM, Junio C Hamano wrote:\n>>> Jacob Keller <jacob.e.keller@intel.com> writes:\n>>>\n>>>> Seems like there hasn't been any interest in this series? :(\n>>>\n>>> So far it seems, but we have been in pre-release freeze for a few\n>>> weeks combined with slower summer (in northern hemisphere anyway)\n>>> season, so it might have just been a bad timing.\n>>\n>> Oh true. I can resend once release freeze ends :)\n> \n> Before doing so, can you run with leaksanitizer?  When merged to\n> 'jch', many tests fail and t1013-read-tree-submodule.sh #52 is one\n> of them.\n> \n> Thanks.\n> \nTurns out it seems that SANITIZE=address,undefined,leak doesn't actually\nrun the leak sanitizer.\n\nI found several gaps in branch_release() and remote_clear() which I've\nfixed in v2.\n"}]}