{"thread":{"id":"63662","subject":"[PATCH v2 0/6] submodule: improve remote lookup logic","startedAt":"2025-06-17T21:30:53Z","lastAt":"2025-06-18T17:41:59Z","messageCount":18,"participants":["Jacob Keller","Junio C Hamano","Lidong Yan"],"isPatch":true,"patchVersion":2,"patchTotal":6},"messages":[{"id":"520332","messageId":"20250617-jk-submodule-helper-use-url-v2-0-04cbb003177d@gmail.com","threadId":"63662","inReplyTo":null,"subject":"[PATCH v2 0/6] submodule: improve remote lookup logic","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-17T21:30:40Z","receivedAt":"2025-06-17T21:30:53Z","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* Both the branch_release() function and remote_clear() functions do not\n completely free all memory pointed to by their associated structures. In\n addition, branch_release() potentially dereferences branch->merge even\n when its NULL. Fix these inconsistencies to ensure that the remote logic\n works for a submodule repository pointer.\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\nNext, the submodule--helper.c logic replaces the repo_get_default_remote()\nfunction with a repo_default_remote() function in remote.c, which is based\non the more robust configuration reading logic. This helper uses similar\nlogic but also allows returning the only valid remote in the case where a\nrepository has exactly one remote. This way we do not fall back to \"origin\"\nif a user has renamed the remote without adding another.\n\nThis improved logic is a good first step, but won't handle cases where\nthere are multiple remotes, with none of them being named \"origin\".\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\nget_default_remote_submodule() to first try and find a remote by its URL.\nIf unsuccessful, we still keep the original fallback logic, in the off\nchance that 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 \n---\nChanges in v2:\n- Remove repo_get_default_remote() entirely. The extra checks it does are\n really only necessary if you're doing manual configuration lookup. This\n avoids the confusion of similarly named functions and is less code.\n- Fix leaks in branch_release() and remote_clear().\n- Add a forward declaration of struct repository.\n- Verified tests pass with leak sanitizer now.\n- Link to v1: https://lore.kernel.org/r/20250610-jk-submodule-helper-use-url-v1-0-6d14c1504e91@gmail.com\n\n---\nJacob Keller (6):\n remote: fix tear down of struct branch and struct remote\n dir: move starts_with_dot(_dot)_slash to dir.h\n remote: remove the_repository from some functions\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 | 6 ++-\n builtin/submodule--helper.c | 106 ++++++++++++++++------------------------\n remote.c | 114 +++++++++++++++++++++++++++++---------------\n submodule-config.c | 12 -----\n t/t7406-submodule-update.sh | 61 ++++++++++++++++++++++++\n 6 files changed, 206 insertions(+), 116 deletions(-)\n---\nbase-commit: 16bd9f20a403117f2e0d9bcda6c6e621d3763e77\nchange-id: 20250610-jk-submodule-helper-use-url-e55d3c379faf\n\nBest regards,\n-- \nJacob Keller \n\n"},{"id":"520333","messageId":"20250617-jk-submodule-helper-use-url-v2-1-04cbb003177d@gmail.com","threadId":"63662","inReplyTo":"20250617-jk-submodule-helper-use-url-v2-0-04cbb003177d@gmail.com","subject":"[PATCH v2 1/6] remote: fix tear down of struct branch and struct remote","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-17T21:30:41Z","receivedAt":"2025-06-17T21:30:55Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nThe branch_release() and remote_clear() functions fail to completely\nrelease all of the memory they point to. This results in leaked memory\nif you read the configuration for an arbitrary repository and then\nrelease that repository.\n\nThis should be caught by the leak sanitizer. However, for callers which\nuse ``the_repository``, the values never go out of scope and the\nsanitizer won't complain.\n\nA future change is going to add a caller of read_config() for a\nsubmodule repository structure. Doing so reveals one immediate issue due\nto a bad NULL pointer access, as well as the mentioned leaks.\n\n * The branch->merge array is accessed without checking if its NULL.\n Since this array is only setup by calling set_merge, it may in fact\n not be initialized even though merge_nr is non-zero.\n\n * The remote->push and remote->fetch refspecs are never cleared.\n\n * The branch->merge_name array is never cleared.\n\n * The individual elements of branch->merge are not released.\n\nAdd a check against branch->merge before accessing it and calling\nrefspec_item_clear. Update remote_clear() with calls to refspec_clear()\nfor both the push and fetch refspecs. Add a release of the merge_name\nitems as well as a final release of the merge_name array.\n\nFreeing merge_name elements results in a warning because we discard the\nconst qualifier on the parameter name. These values come from a call to\nadd_merge() in handle_config(), which always copies the names with\nxstrdup. This makes ownership of the memory difficult to track in the\ncode.\n\nMove the call to xstrdup inside add_merge() so that its clear that the\nmemory is duplicated here and must be released when the merge_name array\nis released. Drop the const qualifier on the branch structure to allow\ncalling free without an explicit cast.\n\nThese changes make it safe to call read_config() on a submodule\nrepository without crashing or leaking any of the memory when the\nsubmodule repository is released.\n\nThere is still some confusion with the difference between branch->merge\nand branch->merge_name, and the confusion of using branch->merge_nr for\nboth. That could probably use some future cleanup.\n\nSigned-off-by: Jacob Keller \n---\n remote.h | 2 +-\n remote.c | 18 ++++++++++++++----\n 2 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/remote.h b/remote.h\nindex 7e4943ae3a70ecefa3332d211084762ca30b59b6..0fca14348fd3d0734c599b13278afef2e00107b9 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -316,7 +316,7 @@ struct branch {\n \tchar *pushremote_name;\n \n \t/* An array of the \"merge\" lines in the configuration. */\n-\tconst char **merge_name;\n+\tchar **merge_name;\n \n \t/**\n \t * An array of the struct refspecs used for the merge lines. That is,\ndiff --git a/remote.c b/remote.c\nindex 4099183cacdc8a607a8b5eaec86e456b2ef46b48..538d0d24c832ffeb1cc14684eb166c62c42d719d 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -165,6 +165,9 @@ static void remote_clear(struct remote *remote)\n \tstrvec_clear(&remote->url);\n \tstrvec_clear(&remote->pushurl);\n \n+\trefspec_clear(&remote->push);\n+\trefspec_clear(&remote->fetch);\n+\n \tfree((char *)remote->receivepack);\n \tfree((char *)remote->uploadpack);\n \tFREE_AND_NULL(remote->http_proxy);\n@@ -176,7 +179,7 @@ static void add_merge(struct branch *branch, const char *name)\n {\n \tALLOC_GROW(branch->merge_name, branch->merge_nr + 1,\n \t\t branch->merge_alloc);\n-\tbranch->merge_name[branch->merge_nr++] = name;\n+\tbranch->merge_name[branch->merge_nr++] = xstrdup(name);\n }\n \n struct branches_hash_key {\n@@ -253,9 +256,16 @@ 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+\tfor (int i = 0; i < branch->merge_nr; i++) {\n+\t\tif (branch->merge) {\n+\t\t\trefspec_item_clear(branch->merge[i]);\n+\t\t\tfree(branch->merge[i]);\n+\t\t}\n+\t\tif (branch->merge_name)\n+\t\t\tfree(branch->merge_name[i]);\n+\t}\n \tfree(branch->merge);\n+\tfree(branch->merge_name);\n }\n \n static struct rewrite *make_rewrite(struct rewrites *r,\n@@ -429,7 +439,7 @@ static int handle_config(const char *key, const char *value,\n \t\t} else if (!strcmp(subkey, \"merge\")) {\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n-\t\t\tadd_merge(branch, xstrdup(value));\n+\t\t\tadd_merge(branch, value);\n \t\t}\n \t\treturn 0;\n \t}\n\n-- \n2.48.1.397.gec9d649cc640\n\n"},{"id":"520334","messageId":"20250617-jk-submodule-helper-use-url-v2-2-04cbb003177d@gmail.com","threadId":"63662","inReplyTo":"20250617-jk-submodule-helper-use-url-v2-0-04cbb003177d@gmail.com","subject":"[PATCH v2 2/6] dir: move starts_with_dot(_dot)_slash to dir.h","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-17T21:30:42Z","receivedAt":"2025-06-17T21:30:55Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \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 \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":"520335","messageId":"20250617-jk-submodule-helper-use-url-v2-3-04cbb003177d@gmail.com","threadId":"63662","inReplyTo":"20250617-jk-submodule-helper-use-url-v2-0-04cbb003177d@gmail.com","subject":"[PATCH v2 3/6] remote: remove the_repository from some functions","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-17T21:30:43Z","receivedAt":"2025-06-17T21:30:57Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \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 \n---\n remote.c | 58 ++++++++++++++++++++++++++++------------------------------\n 1 file changed, 28 insertions(+), 30 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 538d0d24c832ffeb1cc14684eb166c62c42d719d..b3a9881a6eacf90bee71d6760858b37d68263502 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -327,11 +327,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@@ -347,7 +346,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@@ -360,12 +359,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@@ -392,9 +390,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@@ -691,7 +689,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@@ -710,7 +708,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@@ -767,10 +765,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@@ -784,9 +783,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@@ -800,35 +799,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@@ -1732,7 +1729,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@@ -1752,7 +1749,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@@ -1761,7 +1758,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@@ -1780,7 +1777,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@@ -1851,13 +1848,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@@ -1924,7 +1922,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":"520336","messageId":"20250617-jk-submodule-helper-use-url-v2-4-04cbb003177d@gmail.com","threadId":"63662","inReplyTo":"20250617-jk-submodule-helper-use-url-v2-0-04cbb003177d@gmail.com","subject":"[PATCH v2 4/6] submodule--helper: improve logic for fallback remote name","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-17T21:30:44Z","receivedAt":"2025-06-17T21:30:57Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \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\nReplace calls of repo_get_default_remote() with the calls to this new\nfunction. To maintain consistency with the existing callers, continue\ncopying the returned string with xstrdup.\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 \n---\n remote.h | 3 +++\n builtin/submodule--helper.c | 46 +++++----------------------------------------\n remote.c | 25 +++++++++++++++++++-----\n t/t7406-submodule-update.sh | 29 ++++++++++++++++++++++++++++\n 4 files changed, 57 insertions(+), 46 deletions(-)\n\ndiff --git a/remote.h b/remote.h\nindex 0fca14348fd3d0734c599b13278afef2e00107b9..672e8e893b8243c09145c340f412aa0eae8d562f 100644\n--- a/remote.h\n+++ b/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@@ -338,6 +339,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..4aa237033a526fca29cce2926419462179d40ee3 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -41,61 +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-{\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-\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-\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-\n-\tstrbuf_release(&sb);\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+\n+\t*default_remote = xstrdup(repo_default_remote(&subrepo));\n+\n \trepo_clear(&subrepo);\n \n-\treturn ret;\n+\treturn 0;\n }\n \n static char *get_default_remote(void)\n {\n-\tchar *default_remote;\n-\tint code = repo_get_default_remote(the_repository, &default_remote);\n-\n-\tif (code)\n-\t\texit(code);\n-\n-\treturn default_remote;\n+\treturn xstrdup(repo_default_remote(the_repository));\n }\n \n static char *resolve_relative_url(const char *rel_url, const char *up_path, int quiet)\ndiff --git a/remote.c b/remote.c\nindex b3a9881a6eacf90bee71d6760858b37d68263502..94b31f4c23057a247a968fc0ebe2e5170e99614d 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1767,20 +1767,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":"520337","messageId":"20250617-jk-submodule-helper-use-url-v2-5-04cbb003177d@gmail.com","threadId":"63662","inReplyTo":"20250617-jk-submodule-helper-use-url-v2-0-04cbb003177d@gmail.com","subject":"[PATCH v2 5/6] submodule: move get_default_remote_submodule()","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-17T21:30:45Z","receivedAt":"2025-06-17T21:30:57Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \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 \n---\n builtin/submodule--helper.c | 32 ++++++++++++++++----------------\n 1 file changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 4aa237033a526fca29cce2926419462179d40ee3..1aa87435c2000e94f43da94c5ef88a307f6f3f4a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -41,22 +41,6 @@\n typedef void (*each_submodule_fn)(const struct cache_entry *list_item,\n \t\t\t\t void *cb_data);\n \n-static int get_default_remote_submodule(const char *module_path, char **default_remote)\n-{\n-\tstruct repository subrepo;\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-\n-\t*default_remote = xstrdup(repo_default_remote(&subrepo));\n-\n-\trepo_clear(&subrepo);\n-\n-\treturn 0;\n-}\n-\n static char *get_default_remote(void)\n {\n \treturn xstrdup(repo_default_remote(the_repository));\n@@ -86,6 +70,22 @@ 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+\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+\n+\t*default_remote = xstrdup(repo_default_remote(&subrepo));\n+\n+\trepo_clear(&subrepo);\n+\n+\treturn 0;\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":"520338","messageId":"20250617-jk-submodule-helper-use-url-v2-6-04cbb003177d@gmail.com","threadId":"63662","inReplyTo":"20250617-jk-submodule-helper-use-url-v2-0-04cbb003177d@gmail.com","subject":"[PATCH v2 6/6] submodule: look up remotes by URL first","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-17T21:30:46Z","receivedAt":"2025-06-17T21:30:59Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \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 get_default_remote_submodule to find the submodule and get its\nURL. If a valid URL exists, first try to obtain a remote using the new\nrepo_remote_from_url(). Fall back to the repo_default_remote()\notherwise.\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 \n---\n remote.h | 1 +\n builtin/submodule--helper.c | 26 +++++++++++++++++++++++++-\n remote.c | 15 +++++++++++++++\n t/t7406-submodule-update.sh | 32 ++++++++++++++++++++++++++++++++\n 4 files changed, 73 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.h b/remote.h\nindex 672e8e893b8243c09145c340f412aa0eae8d562f..59033d5d82dd81d79174f265c1d501ee5653a6ec 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -340,6 +340,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 1aa87435c2000e94f43da94c5ef88a307f6f3f4a..84a96d300d9489706fb16280f03aea02f87f1657 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -72,16 +72,40 @@ 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+\tconst char *remote_name = NULL;\n+\tchar *url = NULL;\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 \n-\t*default_remote = xstrdup(repo_default_remote(&subrepo));\n+\t/* Look up by URL first */\n+\tif (url)\n+\t\tremote_name = repo_remote_from_url(&subrepo, url);\n+\tif (!remote_name)\n+\t\tremote_name = repo_default_remote(&subrepo);\n+\n+\t*default_remote = xstrdup(remote_name);\n \n \trepo_clear(&subrepo);\n+\tfree(url);\n \n \treturn 0;\n }\ndiff --git a/remote.c b/remote.c\nindex 94b31f4c23057a247a968fc0ebe2e5170e99614d..706c25af0c27aaa9090197e93e87c2a612497172 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1796,6 +1796,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":"520339","messageId":"xmqqcyb2uhth.fsf@gitster.g","threadId":"63662","inReplyTo":"20250617-jk-submodule-helper-use-url-v2-1-04cbb003177d@gmail.com","subject":"Re: [PATCH v2 1/6] remote: fix tear down of struct branch and struct remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-17T22:25:14Z","receivedAt":"2025-06-17T22:25:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> From: Jacob Keller \n>\n> The branch_release() and remote_clear() functions fail to completely\n> release all of the memory they point to. This results in leaked memory\n> if you read the configuration for an arbitrary repository and then\n> release that repository.\n>\n> This should be caught by the leak sanitizer. However, for callers which\n> use ``the_repository``, the values never go out of scope and the\n> sanitizer won't complain.\n>\n> A future change is going to add a caller of read_config() for a\n> submodule repository structure. Doing so reveals one immediate issue due\n> to a bad NULL pointer access, as well as the mentioned leaks.\n>\n> * The branch->merge array is accessed without checking if its NULL.\n> Since this array is only setup by calling set_merge, it may in fact\n> not be initialized even though merge_nr is non-zero.\n>\n> * The remote->push and remote->fetch refspecs are never cleared.\n>\n> * The branch->merge_name array is never cleared.\n>\n> * The individual elements of branch->merge are not released.\n>\n> Add a check against branch->merge before accessing it and calling\n> refspec_item_clear. Update remote_clear() with calls to refspec_clear()\n> for both the push and fetch refspecs. Add a release of the merge_name\n> items as well as a final release of the merge_name array.\n>\n> Freeing merge_name elements results in a warning because we discard the\n> const qualifier on the parameter name. These values come from a call to\n> add_merge() in handle_config(), which always copies the names with\n> xstrdup. This makes ownership of the memory difficult to track in the\n> code.\n>\n> Move the call to xstrdup inside add_merge() so that its clear that the\n> memory is duplicated here and must be released when the merge_name array\n> is released. Drop the const qualifier on the branch structure to allow\n> calling free without an explicit cast.\n>\n> These changes make it safe to call read_config() on a submodule\n> repository without crashing or leaking any of the memory when the\n> submodule repository is released.\n>\n> There is still some confusion with the difference between branch->merge\n> and branch->merge_name, and the confusion of using branch->merge_nr for\n> both. That could probably use some future cleanup.\n\n\nNicely described. One thing that puzzles me is this part:\n\n> @@ -253,9 +256,16 @@ 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> +\tfor (int i = 0; i < branch->merge_nr; i++) {\n> +\t\tif (branch->merge) {\n> +\t\t\trefspec_item_clear(branch->merge[i]);\n> +\t\t\tfree(branch->merge[i]);\n> +\t\t}\n> +\t\tif (branch->merge_name)\n> +\t\t\tfree(branch->merge_name[i]);\n> +\t}\n> \tfree(branch->merge);\n> +\tfree(branch->merge_name);\n> }\n\nwhere we iterate over branch->merge[] and branch->merge_name[]\nfor branch->nr times and each time we check the NULL-ness of these\ntwo pointers.\n\nmerge_nr is only incremented inside add_merge() when merge_name[]\narray is grown by the same amount. Do we need to check for the\nNULL-ness of branch->merge_name?\n\nNear the beginning of set_merge() we allocate branch->merge[] \nonly when branch->merge_nr is non-zero (the assumption seems to be\nthat we accumulate in the merge_names[] while reading the config,\nso as long as branch->merge is initialized properly to NULL, it will\nnever be NULL if merge_nr is not 0, no?\n\nThanks.\n"},{"id":"520340","messageId":"xmqq7c1auguv.fsf@gitster.g","threadId":"63662","inReplyTo":"20250617-jk-submodule-helper-use-url-v2-3-04cbb003177d@gmail.com","subject":"Re: [PATCH v2 3/6] remote: remove the_repository from some functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-17T22:46:00Z","receivedAt":"2025-06-17T22:46:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> From: Jacob Keller \n>\n> The remotes_remote_get_1 (and its caller, remotes_remote_get, have an\n> implicit dependency on the_repository due to calling\n> read_branches_file() and read_remotes_file(), both of which use\n> the_repository. The branch_get() function calls set_merge() which has an\n> implicit dependency on the_repository as well.\n>\n> Because of this use of the_repository, the helper functions cannot be\n> used in code paths which operate on other repositories. A future\n> refactor of the submodule--helper will want to make use of some of these\n> functions.\n>\n> Refactor to break the dependency by passing struct repository *repo\n> instead of struct remote_state *remote_state in a few places.\n>\n> The public callers and many other helper functions still depend on\n> the_repository. A repo-aware function will be exposed in a following\n> change for git submodule--helper.\n>\n> Signed-off-by: Jacob Keller \n> ---\n> remote.c | 58 ++++++++++++++++++++++++++++------------------------------\n> 1 file changed, 28 insertions(+), 30 deletions(-)\n\nAs exactly one remote_state instance belongs to each repository ever\nsince fd3cb050 (remote: move static variables into per-repository\nstruct, 2021-11-17) defined the former, and remote_state is not\nshared across repository, passing the repository instance that owns\na remote_state instance and pick up the .remote_state member out of\nit as needed would give us the right remote_state, and gives us\naccess to the repository instance it owns it.\n\nMakes perfect sense, \n"},{"id":"520341","messageId":"xmqqv7out11r.fsf@gitster.g","threadId":"63662","inReplyTo":"20250617-jk-submodule-helper-use-url-v2-4-04cbb003177d@gmail.com","subject":"Re: [PATCH v2 4/6] submodule--helper: improve logic for fallback remote name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-17T23:12:48Z","receivedAt":"2025-06-17T23:12:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> From: Jacob Keller \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\n\"calls back\" -> \"falls back\".\n\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\nDesigned wtih good taste.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 9e8cdfe1b2a8c2985d9c1b8ad6f1b0d1f9401714..4aa237033a526fca29cce2926419462179d40ee3 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -41,61 +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> -{\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> -\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> -\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> -\n> -\tstrbuf_release(&sb);\n> -\treturn 0;\n> -}\n\nWe will lose two callers of this function, so we can safely remove\nit.\n\n> static int get_default_remote_submodule(const char *module_path, char **default_remote)\n> static char *get_default_remote(void)\n\nThese callers that used to call the removed helper now call\nrepo_default_remote() instread. Good.\n\n\n> diff --git a/remote.c b/remote.c\n> index b3a9881a6eacf90bee71d6760858b37d68263502..94b31f4c23057a247a968fc0ebe2e5170e99614d 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1767,20 +1767,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\nNice to see how the dependency to the_repository is lifted for new\ncallers while retaining the same interface for existing ones.\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\nOK. read_config() is a safe no-op if repo has already been\ninitialized, so this would give us what we want. Nicely done.\n\n"},{"id":"520342","messageId":"a0fdbc99-f5c5-4399-a82e-0bf37292868d@intel.com","threadId":"63662","inReplyTo":"xmqq7c1auguv.fsf@gitster.g","subject":"Re: [PATCH v2 3/6] remote: remove the_repository from some functions","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-17T23:19:54Z","receivedAt":"2025-06-17T23:20: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 3:46 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n> \n>> From: Jacob Keller \n>>\n>> The remotes_remote_get_1 (and its caller, remotes_remote_get, have an\n>> implicit dependency on the_repository due to calling\n>> read_branches_file() and read_remotes_file(), both of which use\n>> the_repository. The branch_get() function calls set_merge() which has an\n>> implicit dependency on the_repository as well.\n>>\n>> Because of this use of the_repository, the helper functions cannot be\n>> used in code paths which operate on other repositories. A future\n>> refactor of the submodule--helper will want to make use of some of these\n>> functions.\n>>\n>> Refactor to break the dependency by passing struct repository *repo\n>> instead of struct remote_state *remote_state in a few places.\n>>\n>> The public callers and many other helper functions still depend on\n>> the_repository. A repo-aware function will be exposed in a following\n>> change for git submodule--helper.\n>>\n>> Signed-off-by: Jacob Keller \n>> ---\n>> remote.c | 58 ++++++++++++++++++++++++++++------------------------------\n>> 1 file changed, 28 insertions(+), 30 deletions(-)\n> \n> As exactly one remote_state instance belongs to each repository ever\n> since fd3cb050 (remote: move static variables into per-repository\n> struct, 2021-11-17) defined the former, and remote_state is not\n> shared across repository, passing the repository instance that owns\n> a remote_state instance and pick up the .remote_state member out of\n> it as needed would give us the right remote_state, and gives us\n> access to the repository instance it owns it.\n> \n> Makes perfect sense, \n\nYep. It looks like most of the use of the repository structure actually\ncomes from the to-be-removed logic for handling the remotes_from_file\nstuff.. but I don't really want to delay these fixes and improvements to\n3.0. :(\n"},{"id":"520343","messageId":"5bdecb99-8ac6-4dc4-8070-ddc747ab40b3@intel.com","threadId":"63662","inReplyTo":"xmqqv7out11r.fsf@gitster.g","subject":"Re: [PATCH v2 4/6] submodule--helper: improve logic for fallback remote name","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-17T23:21:55Z","receivedAt":"2025-06-17T23:22: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 4:12 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n> \n>> From: Jacob Keller \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> \n> \"calls back\" -> \"falls back\".\n> \n\nYep.\n\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> Designed wtih good taste.\n> \n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index 9e8cdfe1b2a8c2985d9c1b8ad6f1b0d1f9401714..4aa237033a526fca29cce2926419462179d40ee3 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -41,61 +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>> -{\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>> -\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>> -\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>> -\n>> -\tstrbuf_release(&sb);\n>> -\treturn 0;\n>> -}\n> \n> We will lose two callers of this function, so we can safely remove\n> it.\n> \n\nYep, we're removing both callers.\n\n>> static int get_default_remote_submodule(const char *module_path, char **default_remote)\n>> static char *get_default_remote(void)\n> \n> These callers that used to call the removed helper now call\n> repo_default_remote() instread. Good.\n> \n> \n>> diff --git a/remote.c b/remote.c\n>> index b3a9881a6eacf90bee71d6760858b37d68263502..94b31f4c23057a247a968fc0ebe2e5170e99614d 100644\n>> --- a/remote.c\n>> +++ b/remote.c\n>> @@ -1767,20 +1767,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> Nice to see how the dependency to the_repository is lifted for new\n> callers while retaining the same interface for existing ones.\n> \n\nYea, I figured there's little reason to go poke all the existing callers\nright away. Those can be updated if/when they have a repository\nparameter passed in.\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> OK. read_config() is a safe no-op if repo has already been\n> initialized, so this would give us what we want. Nicely done.\n> \n> \n\nYep, this matches how most of the other remote logic works.\n\n"},{"id":"520344","messageId":"d72fb411-2e05-441e-aee4-d8a26d652fea@intel.com","threadId":"63662","inReplyTo":"xmqqcyb2uhth.fsf@gitster.g","subject":"Re: [PATCH v2 1/6] remote: fix tear down of struct branch and struct remote","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-17T23:25:16Z","receivedAt":"2025-06-17T23:25:24Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 6/17/2025 3:25 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n> \n>> From: Jacob Keller \n>>\n>> The branch_release() and remote_clear() functions fail to completely\n>> release all of the memory they point to. This results in leaked memory\n>> if you read the configuration for an arbitrary repository and then\n>> release that repository.\n>>\n>> This should be caught by the leak sanitizer. However, for callers which\n>> use ``the_repository``, the values never go out of scope and the\n>> sanitizer won't complain.\n>>\n>> A future change is going to add a caller of read_config() for a\n>> submodule repository structure. Doing so reveals one immediate issue due\n>> to a bad NULL pointer access, as well as the mentioned leaks.\n>>\n>> * The branch->merge array is accessed without checking if its NULL.\n>> Since this array is only setup by calling set_merge, it may in fact\n>> not be initialized even though merge_nr is non-zero.\n>>\n>> * The remote->push and remote->fetch refspecs are never cleared.\n>>\n>> * The branch->merge_name array is never cleared.\n>>\n>> * The individual elements of branch->merge are not released.\n>>\n>> Add a check against branch->merge before accessing it and calling\n>> refspec_item_clear. Update remote_clear() with calls to refspec_clear()\n>> for both the push and fetch refspecs. Add a release of the merge_name\n>> items as well as a final release of the merge_name array.\n>>\n>> Freeing merge_name elements results in a warning because we discard the\n>> const qualifier on the parameter name. These values come from a call to\n>> add_merge() in handle_config(), which always copies the names with\n>> xstrdup. This makes ownership of the memory difficult to track in the\n>> code.\n>>\n>> Move the call to xstrdup inside add_merge() so that its clear that the\n>> memory is duplicated here and must be released when the merge_name array\n>> is released. Drop the const qualifier on the branch structure to allow\n>> calling free without an explicit cast.\n>>\n>> These changes make it safe to call read_config() on a submodule\n>> repository without crashing or leaking any of the memory when the\n>> submodule repository is released.\n>>\n>> There is still some confusion with the difference between branch->merge\n>> and branch->merge_name, and the confusion of using branch->merge_nr for\n>> both. That could probably use some future cleanup.\n> \n> \n> Nicely described. One thing that puzzles me is this part:\n> \n>> @@ -253,9 +256,16 @@ 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>> +\tfor (int i = 0; i < branch->merge_nr; i++) {\n>> +\t\tif (branch->merge) {\n>> +\t\t\trefspec_item_clear(branch->merge[i]);\n>> +\t\t\tfree(branch->merge[i]);\n>> +\t\t}\n>> +\t\tif (branch->merge_name)\n>> +\t\t\tfree(branch->merge_name[i]);\n>> +\t}\n>> \tfree(branch->merge);\n>> +\tfree(branch->merge_name);\n>> }\n> \n> where we iterate over branch->merge[] and branch->merge_name[]\n> for branch->nr times and each time we check the NULL-ness of these\n> two pointers.\n> \n> merge_nr is only incremented inside add_merge() when merge_name[]\n> array is grown by the same amount. Do we need to check for the\n> NULL-ness of branch->merge_name?\n> \n\nProbably not.\n\n> Near the beginning of set_merge() we allocate branch->merge[] \n> only when branch->merge_nr is non-zero (the assumption seems to be\n> that we accumulate in the merge_names[] while reading the config,\n> so as long as branch->merge is initialized properly to NULL, it will\n> never be NULL if merge_nr is not 0, no?\n> \n\nWe initialize branch->merge with set_merge() which is called by\nbranch_get() and which is the only way for callers external to remote.c\nof getting a branch structure.\n\nThe issue is that merge_nr can be non-zero because if no caller has done\na branch_get() on the given branch, we still have merge_nr is non-zero\nand merge is NULL. I suspect you're right that merge_name will never be\nNULL while merge_nr is non-zero.\n\nReally, I think we should find a better way to handle this than having\ntwo separate arrays which interact with merge_nr. I especially just\nnoticed that set_merge will assign merge_nr to 0 if remote_name is bad,\nbut it doesn't release the memory, so we'd leak on teardown in that case...\n\nI wonder if it is possible to just get rid of merge_names entirely and\nonly have the merge array?\n\n> Thanks.\n\n"},{"id":"520345","messageId":"c61c0c6f-6d54-466b-9592-e4f3071ff7f0@intel.com","threadId":"63662","inReplyTo":"xmqqcyb2uhth.fsf@gitster.g","subject":"Re: [PATCH v2 1/6] remote: fix tear down of struct branch and struct remote","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-17T23:45:59Z","receivedAt":"2025-06-17T23:46:18Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 6/17/2025 3:25 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n> \n>> From: Jacob Keller \n> \n> where we iterate over branch->merge[] and branch->merge_name[]\n> for branch->nr times and each time we check the NULL-ness of these\n> two pointers.\n> \n> merge_nr is only incremented inside add_merge() when merge_name[]\n> array is grown by the same amount. Do we need to check for the\n> NULL-ness of branch->merge_name?\n> \n> Near the beginning of set_merge() we allocate branch->merge[] \n> only when branch->merge_nr is non-zero (the assumption seems to be\n> that we accumulate in the merge_names[] while reading the config,\n> so as long as branch->merge is initialized properly to NULL, it will\n> never be NULL if merge_nr is not 0, no?\n> \n> Thanks.\n\nPerhaps something like this might be a better solution so that we don't\nneed to worry about merge being NULL and tracking the difference between\nmerge_name and merge. Its a bit ugly still with a partially initialized\nrefspec_item so I am not 100% sure but I think this is overall a bit\nbetter than the current situation:\n\n> diff --git i/remote.h w/remote.h\n> index 59033d5d82dd..0ca399e1835b 100644\n> --- i/remote.h\n> +++ w/remote.h\n> @@ -316,8 +316,8 @@ struct branch {\n> \n> char *pushremote_name;\n> \n> - /* An array of the \"merge\" lines in the configuration. */\n> - char **merge_name;\n> + /* True if set_merge() has been called to finalize the merge array */\n> + int set_merge;\n> \n> /**\n> * An array of the struct refspecs used for the merge lines. That is,\n> diff --git i/branch.c w/branch.c\n> index 6d01d7d6bdb2..93f5b4e8dd9f 100644\n> --- i/branch.c\n> +++ w/branch.c\n> @@ -230,7 +230,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)\n> return -1;\n> }\n> \n> - if (branch->merge_nr < 1 || !branch->merge_name || !branch->merge_name[0]) {\n> + if (branch->merge_nr < 1 || !branch->merge || !branch->merge[0] || !branch->merge[0]->src) {\n> warning(_(\"asked to inherit tracking from '%s', but no merge configuration is set\"),\n> bare_ref);\n> return -1;\n> @@ -238,7 +238,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)\n> \n> tracking->remote = branch->remote_name;\n> for (i = 0; i < branch->merge_nr; i++)\n> - string_list_append(tracking->srcs, branch->merge_name[i]);\n> + string_list_append(tracking->srcs, branch->merge[i]->src);\n> return 0;\n> }\n> \n> diff --git i/builtin/pull.c w/builtin/pull.c\n> index a1ebc6ad3328..f4556ae155ce 100644\n> --- i/builtin/pull.c\n> +++ w/builtin/pull.c\n> @@ -487,7 +487,7 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs\n> } else\n> fprintf_ln(stderr, _(\"Your configuration specifies to merge with the ref '%s'\\n\"\n> \"from the remote, but no such ref was fetched.\"),\n> - *curr_branch->merge_name);\n> + curr_branch->merge[0]->src);\n> exit(1);\n> }\n> \n> diff --git i/remote.c w/remote.c\n> index 706c25af0c27..1850e8fa4e42 100644\n> --- i/remote.c\n> +++ w/remote.c\n> @@ -177,9 +177,15 @@ static void remote_clear(struct remote *remote)\n> \n> static void add_merge(struct branch *branch, const char *name)\n> {\n> - ALLOC_GROW(branch->merge_name, branch->merge_nr + 1,\n> + struct refspec_item *merge;\n> +\n> + ALLOC_GROW(branch->merge, branch->merge_nr + 1,\n> branch->merge_alloc);\n> - branch->merge_name[branch->merge_nr++] = xstrdup(name);\n> +\n> + merge = xcalloc(1, sizeof(*merge));\n> + merge->src = xstrdup(name);\n> +\n> + branch->merge[branch->merge_nr++] = merge;\n> }\n> \n> struct branches_hash_key {\n> @@ -250,22 +256,23 @@ static struct branch *make_branch(struct remote_state *remote_state,\n> return ret;\n> }\n> \n> +static void merge_clear(struct branch *branch)\n> +{\n> + for (int i = 0; i < branch->merge_nr; i++) {\n> + refspec_item_clear(branch->merge[i]);\n> + free(branch->merge[i]);\n> + }\n> + free(branch->merge);\n> + branch->merge_nr = 0;\n> +}\n> +\n> static void branch_release(struct branch *branch)\n> {\n> free((char *)branch->name);\n> free((char *)branch->refname);\n> free(branch->remote_name);\n> free(branch->pushremote_name);\n> - for (int i = 0; i < branch->merge_nr; i++) {\n> - if (branch->merge) {\n> - refspec_item_clear(branch->merge[i]);\n> - free(branch->merge[i]);\n> - }\n> - if (branch->merge_name)\n> - free(branch->merge_name[i]);\n> - }\n> - free(branch->merge);\n> - free(branch->merge_name);\n> + merge_clear(branch);\n> }\n> \n> static struct rewrite *make_rewrite(struct rewrites *r,\n> @@ -700,7 +707,7 @@ char *remote_ref_for_branch(struct branch *branch, int for_push)\n> if (branch) {\n> if (!for_push) {\n> if (branch->merge_nr) {\n> - return xstrdup(branch->merge_name[0]);\n> + return xstrdup(branch->merge[0]->src);\n> }\n> } else {\n> char *dst;\n> @@ -1738,32 +1745,30 @@ static void set_merge(struct repository *repo, struct branch *ret)\n> \n> if (!ret)\n> return; /* no branch */\n> - if (ret->merge)\n> + if (ret->set_merge)\n> return; /* already run */\n> if (!ret->remote_name || !ret->merge_nr) {\n> /*\n> * no merge config; let's make sure we don't confuse callers\n> * with a non-zero merge_nr but a NULL merge\n> */\n> - ret->merge_nr = 0;\n> + merge_clear(ret);\n> return;\n> }\n> + ret->set_merge = 1;\n> \n> remote = remotes_remote_get(repo, ret->remote_name);\n> \n> - CALLOC_ARRAY(ret->merge, ret->merge_nr);\n> for (i = 0; i < ret->merge_nr; i++) {\n> - ret->merge[i] = xcalloc(1, sizeof(**ret->merge));\n> - ret->merge[i]->src = xstrdup(ret->merge_name[i]);\n> if (!remote_find_tracking(remote, ret->merge[i]) ||\n> strcmp(ret->remote_name, \".\"))\n> continue;\n> - if (repo_dwim_ref(repo, ret->merge_name[i],\n> - strlen(ret->merge_name[i]), &oid, &ref,\n> + if (repo_dwim_ref(repo, ret->merge[i]->src,\n> + strlen(ret->merge[i]->src), &oid, &ref,\n> 0) == 1)\n> ret->merge[i]->dst = ref;\n> else\n> - ret->merge[i]->dst = xstrdup(ret->merge_name[i]);\n> + ret->merge[i]->dst = xstrdup(ret->merge[i]->src);\n> }\n> }\n> \n\n\n"},{"id":"520346","messageId":"xmqqldppu98s.fsf@gitster.g","threadId":"63662","inReplyTo":"d72fb411-2e05-441e-aee4-d8a26d652fea@intel.com","subject":"Re: [PATCH v2 1/6] remote: fix tear down of struct branch and struct remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-18T01:30:27Z","receivedAt":"2025-06-18T01:30:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> We initialize branch->merge with set_merge() which is called by\n> branch_get() and which is the only way for callers external to remote.c\n> of getting a branch structure.\n>\n> The issue is that merge_nr can be non-zero because if no caller has done\n> a branch_get() on the given branch, we still have merge_nr is non-zero\n> and merge is NULL.\n\nMeaning merge_nr and merge are both uninitialized and unlikely to be\n0 and NULL? What values do they have, and if they are left\nuninitialized, shouldn't we be initializing them to predictable\nvalues?\n"},{"id":"520347","messageId":"16C5347A-3EB4-4629-9C7A-575F60B86CA3@gmail.com","threadId":"63662","inReplyTo":"d72fb411-2e05-441e-aee4-d8a26d652fea@intel.com","subject":"Re: [PATCH v2 1/6] remote: fix tear down of struct branch and struct remote","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-06-18T02:44:51Z","receivedAt":"2025-06-18T02:45:10Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Jacob Keller writes：\n> \n> We initialize branch->merge with set_merge() which is called by\n> branch_get() and which is the only way for callers external to remote.c\n> of getting a branch structure.\n> \n> The issue is that merge_nr can be non-zero because if no caller has done\n> a branch_get() on the given branch, we still have merge_nr is non-zero\n> and merge is NULL. I suspect you're right that merge_name will never be\n> NULL while merge_nr is non-zero.\n\nMaybe we can initialize each branch->merge in read_config(). Then branch->merge\nwill always as large as branch->merge_nr. Do you think this would be a good idea?\n\n---\ndiff --git a/remote.c b/remote.c\nindex 4099183cac..835939c59e 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -596,6 +596,8 @@ static void alias_all_urls(struct remote_state *remote_state)\n \n static void read_config(struct repository *repo, int early)\n {\n+ struct hashmap_iter iter;\n+ struct branch *b;\n int flag;\n \n if (repo->remote_state->initialized)\n@@ -614,6 +616,9 @@ static void read_config(struct repository *repo, int early)\n }\n repo_config(repo, handle_config, repo->remote_state);\n alias_all_urls(repo->remote_state);\n+ hashmap_for_each_entry(&repo->remote_state->branches_hash, &item, b, ent) {\n+ set_merge(repo->remote_state, b);\n+ }\n }\n \n #ifndef WITH_BREAKING_CHANGES\n---\n\n"},{"id":"520351","messageId":"xmqq1prhguu3.fsf@gitster.g","threadId":"63662","inReplyTo":"xmqqldppu98s.fsf@gitster.g","subject":"Re: [PATCH v2 1/6] remote: fix tear down of struct branch and struct remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-18T11:20:04Z","receivedAt":"2025-06-18T11:20:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jacob Keller writes:\n>\n>> We initialize branch->merge with set_merge() which is called by\n>> branch_get() and which is the only way for callers external to remote.c\n>> of getting a branch structure.\n>>\n>> The issue is that merge_nr can be non-zero because if no caller has done\n>> a branch_get() on the given branch, we still have merge_nr is non-zero\n>> and merge is NULL.\n>\n> Meaning merge_nr and merge are both uninitialized and unlikely to be\n> 0 and NULL? What values do they have, and if they are left\n> uninitialized, shouldn't we be initializing them to predictable\n> values?\n\nAh, no, I was just being stupid.\n\nWe read configuration files and accumulate things in .merge_name[]\nwhile incrementing .merge_nr but until set_merge() is called, .merge\nis NULL. We need to clear .merge[] only when it is not NULL, of\ncourse. And for that, it may be more readable if we had two loops.\n\nThanks.\n\n\n\n"},{"id":"520355","messageId":"119590ce-c994-4875-addc-6f395990d708@intel.com","threadId":"63662","inReplyTo":"xmqq1prhguu3.fsf@gitster.g","subject":"Re: [PATCH v2 1/6] remote: fix tear down of struct branch and struct remote","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-06-18T17:41:52Z","receivedAt":"2025-06-18T17:41:59Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 6/18/2025 4:20 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Jacob Keller writes:\n>>\n>>> We initialize branch->merge with set_merge() which is called by\n>>> branch_get() and which is the only way for callers external to remote.c\n>>> of getting a branch structure.\n>>>\n>>> The issue is that merge_nr can be non-zero because if no caller has done\n>>> a branch_get() on the given branch, we still have merge_nr is non-zero\n>>> and merge is NULL.\n>>\n>> Meaning merge_nr and merge are both uninitialized and unlikely to be\n>> 0 and NULL? What values do they have, and if they are left\n>> uninitialized, shouldn't we be initializing them to predictable\n>> values?\n> \n> Ah, no, I was just being stupid.\n> \n> We read configuration files and accumulate things in .merge_name[]\n> while incrementing .merge_nr but until set_merge() is called, .merge\n> is NULL. We need to clear .merge[] only when it is not NULL, of\n> course. And for that, it may be more readable if we had two loops.\n> \n> Thanks.\n> \n> \n> \n\nRight. I think I like my solution of just eliminating merge_names\nentirely. Unfortunately it looks like my mailer marked that content as\nquoted when I pasted it in as an inline diff.\n"}]}