{"thread":{"id":"63433","subject":"[PATCH] submodule: prevent overwriting .gitmodules entry on path reuse","startedAt":"2025-05-10T05:45:49Z","lastAt":"2025-05-24T07:33:22Z","messageCount":24,"participants":["K Jayatheerth","JAYATHEERTH K","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"517723","messageId":"20250510054542.556401-1-jayatheerthkulkarni2005@gmail.com","threadId":"63433","inReplyTo":null,"subject":"[PATCH] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-10T05:45:42Z","receivedAt":"2025-05-10T05:45:49Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"When a submodule is added at a path that previously hosted another submodule\n(e.g., 'child'), Git reuses the submodule name derived from the path and\nupdates the corresponding entry in .gitmodules. This can silently overwrite\nexisting configuration if the old submodule was only moved (e.g., to\n'child_old') without renaming the submodule.\n\nThis patch improves the `module_add()` logic by checking whether the\nsubmodule name already exists in the config but maps to a different path.\nIn such a case, Git now errors out unless `--force` is specified, thus\npreventing accidental overwrites. To proceed safely, the user can provide\na new name via `--name` or use `--force`.\n\nSigned-off-by: K Jayatheerth \n---\n builtin/submodule--helper.c | 45 ++++++++++++++++++++++++++++---------\n t/t7400-submodule-basic.sh | 23 +++++++++++++++++++\n 2 files changed, 57 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 53da2116dd..0f98ef122b 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -32,6 +32,8 @@\n #include \"advice.h\"\n #include \"branch.h\"\n #include \"list-objects-filter-options.h\"\n+#include \"wildmatch.h\"\n+\n \n #define OPT_QUIET (1 << 0)\n #define OPT_CACHED (1 << 1)\n@@ -3323,6 +3325,23 @@ static int config_submodule_in_gitmodules(const char *name, const char *var, con\n \treturn ret;\n }\n \n+static int submodule_active_matches_path(const char *path)\n+{\n+\tconst struct string_list *values;\n+\tsize_t i;\n+\n+\tif (git_config_get_string_multi(\"submodule.active\", &values))\n+\t\treturn 0;\n+\n+\tfor (i = 0; i < values->nr; i++) {\n+\t\tconst char *pat = values->items[i].string;\n+\t\tif (!wildmatch(pat, path, 0))\n+\t\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n static void configure_added_submodule(struct add_data *add_data)\n {\n \tchar *key;\n@@ -3370,17 +3389,7 @@ static void configure_added_submodule(struct add_data *add_data)\n \t * is_submodule_active(), since that function needs to find\n \t * out the value of \"submodule.active\" again anyway.\n \t */\n-\tif (!git_config_get(\"submodule.active\")) {\n-\t\t/*\n-\t\t * If the submodule being added isn't already covered by the\n-\t\t * current configured pathspec, set the submodule's active flag\n-\t\t */\n-\t\tif (!is_submodule_active(the_repository, add_data->sm_path)) {\n-\t\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n-\t\t\tgit_config_set_gently(key, \"true\");\n-\t\t\tfree(key);\n-\t\t}\n-\t} else {\n+\tif (!submodule_active_matches_path(add_data->sm_path)) {\n \t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n \t\tgit_config_set_gently(key, \"true\");\n \t\tfree(key);\n@@ -3443,6 +3452,7 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \tint force = 0, quiet = 0, progress = 0, dissociate = 0;\n \tstruct add_data add_data = ADD_DATA_INIT;\n \tconst char *ref_storage_format = NULL;\n+\tconst struct submodule *existing;\n \tchar *to_free = NULL;\n \tstruct option options[] = {\n \t\tOPT_STRING('b', \"branch\", &add_data.branch, N_(\"branch\"),\n@@ -3546,6 +3556,19 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \tif(!add_data.sm_name)\n \t\tadd_data.sm_name = add_data.sm_path;\n \n+\texisting = submodule_from_name(the_repository,\n+\t\t\t\t\tnull_oid(the_hash_algo),\n+\t\t\t\t\tadd_data.sm_name);\n+\n+\tif (existing && strcmp(existing->path, add_data.sm_path)) {\n+\t\tif (!force) {\n+\t\t\tdie(_(\"submodule name '%s' already used for path '%s' \"\n+\t\t\t\"(use --name to choose another or --force to overwrite)\"),\n+\t\t\tadd_data.sm_name, existing->path);\n+\t\t}\n+\t\tadd_data.sm_name = xstrfmt(\"%s.%s\", add_data.sm_name, basename(add_data.sm_path));\n+\t}\n+\n \tif (check_submodule_name(add_data.sm_name))\n \t\tdie(_(\"'%s' is not a valid submodule name\"), add_data.sm_name);\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex d6a501d453..5c3f471338 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1482,4 +1482,27 @@ test_expect_success '`submodule init` and `init.templateDir`' '\n \t)\n '\n \n+test_expect_success 'submodule add fails when name is reused' '\n+\tgit init test-submodule &&\n+\t(\n+\t\tcd test-submodule &&\n+\t\tgit commit --allow-empty -m \"initial commit\" &&\n+\n+\t\tgit init ../child-origin &&\n+\t\tgit -C ../child-origin commit --allow-empty -m \"initial commit\" &&\n+\n+\t\tgit submodule add ../child-origin child &&\n+\t\tgit commit -m \"Add submodule child\" &&\n+\n+\t\tgit mv child child_old &&\n+\t\tgit commit -m \"Move child to child_old\" &&\n+\n+\t\t# Create another submodule repo\n+\t\tgit init ../child2-origin &&\n+\t\tgit -C ../child2-origin commit --allow-empty -m \"initial commit\" &&\n+\n+\t\ttest_must_fail git submodule add ../child2-origin child\n+\t)\n+'\n+\n test_done\n-- \n2.49.0.533.g80f4e02b4b\n\n"},{"id":"517724","messageId":"CA+rGoLc=S44mHwc-ZpagwfAo=V-BjcX58HCaQDn1fBJcknQ4Vw@mail.gmail.com","threadId":"63433","inReplyTo":"20250510054542.556401-1-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-10T05:57:22Z","receivedAt":"2025-05-10T05:57:35Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"> +++ b/builtin/submodule--helper.c\n> @@ -32,6 +32,8 @@\n> #include \"advice.h\"\n> #include \"branch.h\"\n> #include \"list-objects-filter-options.h\"\n> +#include \"wildmatch.h\"\n> +\n>\n> #define OPT_QUIET (1 << 0)\n> #define OPT_CACHED (1 << 1)\n> @@ -3323,6 +3325,23 @@ static int config_submodule_in_gitmodules(const char *name, const char *var, con\n> return ret;\n> }\n>\n> +static int submodule_active_matches_path(const char *path)\n> +{\n> + const struct string_list *values;\n> + size_t i;\n> +\n> + if (git_config_get_string_multi(\"submodule.active\", &values))\n> + return 0;\n> +\n> + for (i = 0; i < values->nr; i++) {\n> + const char *pat = values->items[i].string;\n> + if (!wildmatch(pat, path, 0))\n> + return 1;\n> + }\n> +\n> + return 0;\n> +}\n> +\n>\n\n\nI added this change in the same patch because\nt7413 was throwing errors at these specific lines of code\nI don't really have full understanding of these files yet\nbut after this t7413 perfectly passed all the tests\n\nI tried these changed because there was a comment about this in the file\n\n/*\n* NEEDSWORK: In a multi-working-tree world this needs to be\n* set in the per-worktree config.\n*/\n/*\n* NEEDSWORK: In the longer run, we need to get rid of this\n* pattern of querying \"submodule.active\" before calling\n* is_submodule_active(), since that function needs to find\n* out the value of \"submodule.active\" again anyway.\n*/\n\n\nI can separate it into different patches if there is a requirement.\nOr change based on the feedback.\n\n-Jayatheerth\n"},{"id":"517831","messageId":"xmqqbjryxb5j.fsf@gitster.g","threadId":"63433","inReplyTo":"20250510054542.556401-1-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-12T12:32:08Z","receivedAt":"2025-05-12T12:32:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"K Jayatheerth writes:\n\n> When a submodule is added at a path that previously hosted another submodule\n> (e.g., 'child'), Git reuses the submodule name derived from the path and\n> updates the corresponding entry in .gitmodules. This can silently overwrite\n> existing configuration if the old submodule was only moved (e.g., to\n> 'child_old') without renaming the submodule.\n>\n> This patch improves the `module_add()` logic by checking whether the\n> submodule name already exists in the config but maps to a different path.\n\nQuite sensible description of the problem and the proposed course of\nimprovement.\n\nI do not think `--force` that allows the same name to be reused a\ngood idea at all, though. We shouldn't encourage its use to resolve\nsuch a case. If what used to be called `child` now sits elsewhere,\nperhaps because the tree structure was reorganized due to mass\nrenaming, but if it still is being used in the project, there is no\ngood reason to nuke the configuration recorded for that existing\nmodule.\n\nThe module name used in .git/config is purely local so the user\nshould just give a new one a name that does not conflict, or even\nbetter yet, perhaps the tool should pick a unique and nonconflicting\nname automatically, no?\n\n> In such a case, Git now errors out unless `--force` is specified, thus\n> preventing accidental overwrites. To proceed safely, the user can provide\n> a new name via `--name` or use `--force`.\n\n\n>\n> Signed-off-by: K Jayatheerth \n> ---\n> builtin/submodule--helper.c | 45 ++++++++++++++++++++++++++++---------\n> t/t7400-submodule-basic.sh | 23 +++++++++++++++++++\n> 2 files changed, 57 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 53da2116dd..0f98ef122b 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -32,6 +32,8 @@\n> #include \"advice.h\"\n> #include \"branch.h\"\n> #include \"list-objects-filter-options.h\"\n> +#include \"wildmatch.h\"\n> +\n> \n> #define OPT_QUIET (1 << 0)\n> #define OPT_CACHED (1 << 1)\n> @@ -3323,6 +3325,23 @@ static int config_submodule_in_gitmodules(const char *name, const char *var, con\n> \treturn ret;\n> }\n> \n> +static int submodule_active_matches_path(const char *path)\n> +{\n> +\tconst struct string_list *values;\n> +\tsize_t i;\n> +\n> +\tif (git_config_get_string_multi(\"submodule.active\", &values))\n> +\t\treturn 0;\n> +\n> +\tfor (i = 0; i < values->nr; i++) {\n> +\t\tconst char *pat = values->items[i].string;\n> +\t\tif (!wildmatch(pat, path, 0))\n> +\t\t\treturn 1;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> static void configure_added_submodule(struct add_data *add_data)\n> {\n> \tchar *key;\n> @@ -3370,17 +3389,7 @@ static void configure_added_submodule(struct add_data *add_data)\n> \t * is_submodule_active(), since that function needs to find\n> \t * out the value of \"submodule.active\" again anyway.\n> \t */\n> -\tif (!git_config_get(\"submodule.active\")) {\n> -\t\t/*\n> -\t\t * If the submodule being added isn't already covered by the\n> -\t\t * current configured pathspec, set the submodule's active flag\n> -\t\t */\n> -\t\tif (!is_submodule_active(the_repository, add_data->sm_path)) {\n> -\t\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n> -\t\t\tgit_config_set_gently(key, \"true\");\n> -\t\t\tfree(key);\n> -\t\t}\n> -\t} else {\n> +\tif (!submodule_active_matches_path(add_data->sm_path)) {\n> \t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n> \t\tgit_config_set_gently(key, \"true\");\n> \t\tfree(key);\n> @@ -3443,6 +3452,7 @@ static int module_add(int argc, const char **argv, const char *prefix,\n> \tint force = 0, quiet = 0, progress = 0, dissociate = 0;\n> \tstruct add_data add_data = ADD_DATA_INIT;\n> \tconst char *ref_storage_format = NULL;\n> +\tconst struct submodule *existing;\n> \tchar *to_free = NULL;\n> \tstruct option options[] = {\n> \t\tOPT_STRING('b', \"branch\", &add_data.branch, N_(\"branch\"),\n> @@ -3546,6 +3556,19 @@ static int module_add(int argc, const char **argv, const char *prefix,\n> \tif(!add_data.sm_name)\n> \t\tadd_data.sm_name = add_data.sm_path;\n> \n> +\texisting = submodule_from_name(the_repository,\n> +\t\t\t\t\tnull_oid(the_hash_algo),\n> +\t\t\t\t\tadd_data.sm_name);\n> +\n> +\tif (existing && strcmp(existing->path, add_data.sm_path)) {\n> +\t\tif (!force) {\n> +\t\t\tdie(_(\"submodule name '%s' already used for path '%s' \"\n> +\t\t\t\"(use --name to choose another or --force to overwrite)\"),\n> +\t\t\tadd_data.sm_name, existing->path);\n> +\t\t}\n> +\t\tadd_data.sm_name = xstrfmt(\"%s.%s\", add_data.sm_name, basename(add_data.sm_path));\n> +\t}\n> +\n> \tif (check_submodule_name(add_data.sm_name))\n> \t\tdie(_(\"'%s' is not a valid submodule name\"), add_data.sm_name);\n> \n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index d6a501d453..5c3f471338 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -1482,4 +1482,27 @@ test_expect_success '`submodule init` and `init.templateDir`' '\n> \t)\n> '\n> \n> +test_expect_success 'submodule add fails when name is reused' '\n> +\tgit init test-submodule &&\n> +\t(\n> +\t\tcd test-submodule &&\n> +\t\tgit commit --allow-empty -m \"initial commit\" &&\n> +\n> +\t\tgit init ../child-origin &&\n> +\t\tgit -C ../child-origin commit --allow-empty -m \"initial commit\" &&\n> +\n> +\t\tgit submodule add ../child-origin child &&\n> +\t\tgit commit -m \"Add submodule child\" &&\n> +\n> +\t\tgit mv child child_old &&\n> +\t\tgit commit -m \"Move child to child_old\" &&\n> +\n> +\t\t# Create another submodule repo\n> +\t\tgit init ../child2-origin &&\n> +\t\tgit -C ../child2-origin commit --allow-empty -m \"initial commit\" &&\n> +\n> +\t\ttest_must_fail git submodule add ../child2-origin child\n> +\t)\n> +'\n> +\n> test_done\n"},{"id":"517843","messageId":"CA+rGoLdzT01jLigX=Z+_tw715O0BibgVA+Sj+65Qwee7i5gLfg@mail.gmail.com","threadId":"63433","inReplyTo":"xmqqbjryxb5j.fsf@gitster.g","subject":"Re: [PATCH] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-12T13:26:19Z","receivedAt":"2025-05-12T13:26:41Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"On Mon, May 12, 2025 at 6:02 PM Junio C Hamano wrote:\n>\n> K Jayatheerth writes:\n>\n> > When a submodule is added at a path that previously hosted another submodule\n> > (e.g., 'child'), Git reuses the submodule name derived from the path and\n> > updates the corresponding entry in .gitmodules. This can silently overwrite\n> > existing configuration if the old submodule was only moved (e.g., to\n> > 'child_old') without renaming the submodule.\n> >\n> > This patch improves the `module_add()` logic by checking whether the\n> > submodule name already exists in the config but maps to a different path.\n>\n> Quite sensible description of the problem and the proposed course of\n> improvement.\n>\n> I do not think `--force` that allows the same name to be reused a\n> good idea at all, though. We shouldn't encourage its use to resolve\n> such a case. If what used to be called `child` now sits elsewhere,\n> perhaps because the tree structure was reorganized due to mass\n> renaming, but if it still is being used in the project, there is no\n> good reason to nuke the configuration recorded for that existing\n> module.\n>\n> The module name used in .git/config is purely local so the user\n> should just give a new one a name that does not conflict, or even\n> better yet, perhaps the tool should pick a unique and nonconflicting\n> name automatically, no?\n>\n\n\nThat makes sense\nI see the point in using --force to resolve name conflicts might not be ideal,\nespecially when the previous submodule config may still be valid and in use.\n\nAs a potential improvement, I was thinking of mimicking how duplicate\nfilenames are handled: if the default submodule name (derived from the\npath) already exists and maps to a different path, we could\nautomatically append an incrementing number (foo, foo1, foo2, etc.)\nuntil a non-conflicting name is found.\n\n-Jayatheerth\n"},{"id":"517924","messageId":"20250513033403.91365-1-jayatheerthkulkarni2005@gmail.com","threadId":"63433","inReplyTo":"CA+rGoLdzT01jLigX=Z+_tw715O0BibgVA+Sj+65Qwee7i5gLfg@mail.gmail.com","subject":"[PATCH v2] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-13T03:34:03Z","receivedAt":"2025-05-13T03:34:15Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"When a submodule is added at a path that previously hosted another submodule\n(e.g., 'child'), Git reuses the submodule name derived from the path and\nupdates the corresponding entry in .gitmodules. This can silently overwrite\nexisting configuration if the old submodule was only moved (e.g., to\n'child_old') without renaming the submodule.\n\nThis patch improves the `module_add()` logic by checking whether the\nsubmodule name already exists in the config but maps to a different path.\nIn such a case, Git now errors out unless `--force` is specified, thus\npreventing accidental overwrites. To proceed safely, the user can provide\na new name via `--name` or use `--force`.\n\nSigned-off-by: K Jayatheerth \n---\n builtin/submodule--helper.c | 58 ++++++++++++++++++++++++++++++-------\n t/t7400-submodule-basic.sh | 23 +++++++++++++++\n 2 files changed, 70 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 53da2116dd..e70aa584f1 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -32,6 +32,8 @@\n #include \"advice.h\"\n #include \"branch.h\"\n #include \"list-objects-filter-options.h\"\n+#include \"wildmatch.h\"\n+#include \"strbuf.h\"\n \n #define OPT_QUIET (1 << 0)\n #define OPT_CACHED (1 << 1)\n@@ -3323,6 +3325,23 @@ static int config_submodule_in_gitmodules(const char *name, const char *var, con\n \treturn ret;\n }\n \n+static int submodule_active_matches_path(const char *path)\n+{\n+\tconst struct string_list *values;\n+\tsize_t i;\n+\n+\tif (git_config_get_string_multi(\"submodule.active\", &values))\n+\t\treturn 0;\n+\n+\tfor (i = 0; i < values->nr; i++) {\n+\t\tconst char *pat = values->items[i].string;\n+\t\tif (!wildmatch(pat, path, 0))\n+\t\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n static void configure_added_submodule(struct add_data *add_data)\n {\n \tchar *key;\n@@ -3370,17 +3389,7 @@ static void configure_added_submodule(struct add_data *add_data)\n \t * is_submodule_active(), since that function needs to find\n \t * out the value of \"submodule.active\" again anyway.\n \t */\n-\tif (!git_config_get(\"submodule.active\")) {\n-\t\t/*\n-\t\t * If the submodule being added isn't already covered by the\n-\t\t * current configured pathspec, set the submodule's active flag\n-\t\t */\n-\t\tif (!is_submodule_active(the_repository, add_data->sm_path)) {\n-\t\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n-\t\t\tgit_config_set_gently(key, \"true\");\n-\t\t\tfree(key);\n-\t\t}\n-\t} else {\n+\tif (!submodule_active_matches_path(add_data->sm_path)) {\n \t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n \t\tgit_config_set_gently(key, \"true\");\n \t\tfree(key);\n@@ -3443,6 +3452,7 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \tint force = 0, quiet = 0, progress = 0, dissociate = 0;\n \tstruct add_data add_data = ADD_DATA_INIT;\n \tconst char *ref_storage_format = NULL;\n+\tconst struct submodule *existing;\n \tchar *to_free = NULL;\n \tstruct option options[] = {\n \t\tOPT_STRING('b', \"branch\", &add_data.branch, N_(\"branch\"),\n@@ -3546,6 +3556,32 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \tif(!add_data.sm_name)\n \t\tadd_data.sm_name = add_data.sm_path;\n \n+\texisting = submodule_from_name(the_repository,\n+\t\t\t\t\tnull_oid(the_hash_algo),\n+\t\t\t\t\tadd_data.sm_name);\n+\n+\tif (existing && strcmp(existing->path, add_data.sm_path)) {\n+\t\tif (!force)\n+\t\t\tdie(_(\"submodule name '%s' already used for path '%s'\"),\n+\t\t\tadd_data.sm_name, existing->path);\n+\n+\t\t/* --force: build until unique */\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tstrbuf_addstr(&buf, add_data.sm_name);\n+\n+\t\tfor (int i = 1; ; i++) {\n+\t\t\tstrbuf_setlen(&buf, 0);\n+\t\t\tstrbuf_addf(&buf, \"%s%d\", add_data.sm_name, i);\n+\n+\t\t\tif (!submodule_from_name(the_repository,\n+\t\t\t\t\t\tnull_oid(the_hash_algo),\n+\t\t\t\t\t\tbuf.buf))\n+\t\t\t\tbreak;\n+\t\t}\n+\n+\t\tadd_data.sm_name = strbuf_detach(&buf, NULL);\n+\t}\n+\n \tif (check_submodule_name(add_data.sm_name))\n \t\tdie(_(\"'%s' is not a valid submodule name\"), add_data.sm_name);\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex d6a501d453..5c3f471338 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1482,4 +1482,27 @@ test_expect_success '`submodule init` and `init.templateDir`' '\n \t)\n '\n \n+test_expect_success 'submodule add fails when name is reused' '\n+\tgit init test-submodule &&\n+\t(\n+\t\tcd test-submodule &&\n+\t\tgit commit --allow-empty -m \"initial commit\" &&\n+\n+\t\tgit init ../child-origin &&\n+\t\tgit -C ../child-origin commit --allow-empty -m \"initial commit\" &&\n+\n+\t\tgit submodule add ../child-origin child &&\n+\t\tgit commit -m \"Add submodule child\" &&\n+\n+\t\tgit mv child child_old &&\n+\t\tgit commit -m \"Move child to child_old\" &&\n+\n+\t\t# Create another submodule repo\n+\t\tgit init ../child2-origin &&\n+\t\tgit -C ../child2-origin commit --allow-empty -m \"initial commit\" &&\n+\n+\t\ttest_must_fail git submodule add ../child2-origin child\n+\t)\n+'\n+\n test_done\n-- \n2.49.GIT\n\n"},{"id":"517985","messageId":"xmqq7c2kgp8e.fsf@gitster.g","threadId":"63433","inReplyTo":"20250513033403.91365-1-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH v2] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-13T21:44:33Z","receivedAt":"2025-05-13T21:44:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"K Jayatheerth writes:\n\n> When a submodule is added at a path that previously hosted another submodule\n> (e.g., 'child'), Git reuses the submodule name derived from the path and\n> updates the corresponding entry in .gitmodules. This can silently overwrite\n> existing configuration if the old submodule was only moved (e.g., to\n> 'child_old') without renaming the submodule.\n\nOK.\n\n> This patch improves the `module_add()` logic by checking whether the\n> submodule name already exists in the config but maps to a different path.\n\nWe frown upon a patch that says \"This patch does X\"; just give an\norder to the codebase to \"be like so\". I.e. \"Improve the module-add\nby doing X...\" is how we phrase a proposed change.\n\n> In such a case, Git now errors out unless `--force` is specified, thus\n> preventing accidental overwrites. To proceed safely, the user can provide\n> a new name via `--name` or use `--force`.\n\nThe above explains what happens in module_add() quite well.\n\nWhat is puzzling about this change is that the new helper function\nand changes to configure_added_submodule() is not described at all\nin the proposed log message. How are they relevant and why do we\nneed them?\n\n> @@ -3443,6 +3452,7 @@ static int module_add(int argc, const char **argv, const char *prefix,\n> \tint force = 0, quiet = 0, progress = 0, dissociate = 0;\n> \tstruct add_data add_data = ADD_DATA_INIT;\n> \tconst char *ref_storage_format = NULL;\n> +\tconst struct submodule *existing;\n> \tchar *to_free = NULL;\n> \tstruct option options[] = {\n> \t\tOPT_STRING('b', \"branch\", &add_data.branch, N_(\"branch\"),\n> @@ -3546,6 +3556,32 @@ static int module_add(int argc, const char **argv, const char *prefix,\n> \tif(!add_data.sm_name)\n> \t\tadd_data.sm_name = add_data.sm_path;\n> \n> +\texisting = submodule_from_name(the_repository,\n> +\t\t\t\t\tnull_oid(the_hash_algo),\n> +\t\t\t\t\tadd_data.sm_name);\n> +\n> +\tif (existing && strcmp(existing->path, add_data.sm_path)) {\n\nIf the name is in use, and the submodule with that name is at a\ndifferent path, then we are in trouble, OK.\n\n> +\t\tif (!force)\n> +\t\t\tdie(_(\"submodule name '%s' already used for path '%s'\"),\n> +\t\t\tadd_data.sm_name, existing->path);\n> +\n> +\t\t/* --force: build until unique */\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n\n\"-Wdeclaration-after-statement\"\n\n> +\t\tstrbuf_addstr(&buf, add_data.sm_name);\n> +\n> +\t\tfor (int i = 1; ; i++) {\n> +\t\t\tstrbuf_setlen(&buf, 0);\n> +\t\t\tstrbuf_addf(&buf, \"%s%d\", add_data.sm_name, i);\n> +\n> +\t\t\tif (!submodule_from_name(the_repository,\n> +\t\t\t\t\t\tnull_oid(the_hash_algo),\n> +\t\t\t\t\t\tbuf.buf))\n> +\t\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tadd_data.sm_name = strbuf_detach(&buf, NULL);\n> +\t}\n> +\n\nWhat is the memory ownership rule for add_data.sm_name? Earlier we\nsaw in a pre-context of a hunk that this was assigned from\nadd_data.sm_path, so in that codepath it is considered a borrowed\npiece of memory, but here the member has to be the one that owns the\nstring detached from the strbuf, which eventually must be freed by\nsomebody, or it would be a memory leak.\n"},{"id":"517991","messageId":"20250514020108.24396-1-jayatheerthkulkarni2005@gmail.com","threadId":"63433","inReplyTo":"xmqq7c2kgp8e.fsf@gitster.g","subject":"[PATCH v3] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-14T02:01:08Z","receivedAt":"2025-05-14T02:01:20Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"When a submodule is added at a path that previously hosted another submodule\n(e.g., 'child'), Git reuses the submodule name derived from the path and\nupdates the corresponding entry in .gitmodules. This can silently overwrite\nexisting configuration if the old submodule was only moved (e.g., to\n'child_old') without renaming the submodule.\n\nTeach `module_add()` to look up the chosen submodule name in the\nrepository existing submodule config. If that name is already in\nuse and points at a *different* path, we now die with an error,\nprompting the user to supply `--name`. Increment the name to an\nappropriate unique name (Like file system) when --force is called upon.\n\nAdd helper `submodule_active_matches_path()` so we can\nre-implement the old “is this path already covered by\nsubmodule.active?” logic without re-reading the config twice.\n\nSigned-off-by: K Jayatheerth \n---\n builtin/submodule--helper.c | 62 ++++++++++++++++++++++++++++++-------\n t/t7400-submodule-basic.sh | 23 ++++++++++++++\n 2 files changed, 74 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 53da2116dd..ef9e733cfb 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -32,6 +32,8 @@\n #include \"advice.h\"\n #include \"branch.h\"\n #include \"list-objects-filter-options.h\"\n+#include \"wildmatch.h\"\n+#include \"strbuf.h\"\n \n #define OPT_QUIET (1 << 0)\n #define OPT_CACHED (1 << 1)\n@@ -3323,6 +3325,23 @@ static int config_submodule_in_gitmodules(const char *name, const char *var, con\n \treturn ret;\n }\n \n+static int submodule_active_matches_path(const char *path)\n+{\n+\tconst struct string_list *values;\n+\tsize_t i;\n+\n+\tif (git_config_get_string_multi(\"submodule.active\", &values))\n+\t\treturn 0;\n+\n+\tfor (i = 0; i < values->nr; i++) {\n+\t\tconst char *pat = values->items[i].string;\n+\t\tif (!wildmatch(pat, path, 0))\n+\t\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n static void configure_added_submodule(struct add_data *add_data)\n {\n \tchar *key;\n@@ -3370,17 +3389,7 @@ static void configure_added_submodule(struct add_data *add_data)\n \t * is_submodule_active(), since that function needs to find\n \t * out the value of \"submodule.active\" again anyway.\n \t */\n-\tif (!git_config_get(\"submodule.active\")) {\n-\t\t/*\n-\t\t * If the submodule being added isn't already covered by the\n-\t\t * current configured pathspec, set the submodule's active flag\n-\t\t */\n-\t\tif (!is_submodule_active(the_repository, add_data->sm_path)) {\n-\t\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n-\t\t\tgit_config_set_gently(key, \"true\");\n-\t\t\tfree(key);\n-\t\t}\n-\t} else {\n+\tif (!submodule_active_matches_path(add_data->sm_path)) {\n \t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n \t\tgit_config_set_gently(key, \"true\");\n \t\tfree(key);\n@@ -3443,7 +3452,11 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \tint force = 0, quiet = 0, progress = 0, dissociate = 0;\n \tstruct add_data add_data = ADD_DATA_INIT;\n \tconst char *ref_storage_format = NULL;\n+\tconst struct submodule *existing;\n \tchar *to_free = NULL;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint i;\n+\tint allocated_sm_name = 0;\n \tstruct option options[] = {\n \t\tOPT_STRING('b', \"branch\", &add_data.branch, N_(\"branch\"),\n \t\t\t N_(\"branch of repository to add as submodule\")),\n@@ -3546,6 +3559,31 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \tif(!add_data.sm_name)\n \t\tadd_data.sm_name = add_data.sm_path;\n \n+\texisting = submodule_from_name(the_repository,\n+\t\t\t\t\tnull_oid(the_hash_algo),\n+\t\t\t\t\tadd_data.sm_name);\n+\n+\tif (existing && strcmp(existing->path, add_data.sm_path)) {\n+\t\tif (!force) {\n+\t\t\tdie(_(\"submodule name '%s' already used for path '%s'\"),\n+\t\t\tadd_data.sm_name, existing->path);\n+\t\t}\n+\n+\t\t/* --force: build until unique */\n+\t\tfor (i = 1; ; i++) {\n+\t\t\tstrbuf_reset(&buf);\n+\t\t\tstrbuf_addf(&buf, \"%s%d\", add_data.sm_name, i);\n+\t\t\tif (!submodule_from_name(the_repository,\n+\t\t\t\t\t\tnull_oid(the_hash_algo),\n+\t\t\t\t\t\tbuf.buf)) {\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tadd_data.sm_name = strbuf_detach(&buf, NULL);\n+\t\tallocated_sm_name = 1;\n+\t}\n+\n \tif (check_submodule_name(add_data.sm_name))\n \t\tdie(_(\"'%s' is not a valid submodule name\"), add_data.sm_name);\n \n@@ -3561,6 +3599,8 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \n \tret = 0;\n cleanup:\n+\tif (allocated_sm_name)\n+\t\tfree((char *)add_data.sm_name);\n \tfree(add_data.sm_path);\n \tfree(to_free);\n \tstrbuf_release(&sb);\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex d6a501d453..5c3f471338 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1482,4 +1482,27 @@ test_expect_success '`submodule init` and `init.templateDir`' '\n \t)\n '\n \n+test_expect_success 'submodule add fails when name is reused' '\n+\tgit init test-submodule &&\n+\t(\n+\t\tcd test-submodule &&\n+\t\tgit commit --allow-empty -m \"initial commit\" &&\n+\n+\t\tgit init ../child-origin &&\n+\t\tgit -C ../child-origin commit --allow-empty -m \"initial commit\" &&\n+\n+\t\tgit submodule add ../child-origin child &&\n+\t\tgit commit -m \"Add submodule child\" &&\n+\n+\t\tgit mv child child_old &&\n+\t\tgit commit -m \"Move child to child_old\" &&\n+\n+\t\t# Create another submodule repo\n+\t\tgit init ../child2-origin &&\n+\t\tgit -C ../child2-origin commit --allow-empty -m \"initial commit\" &&\n+\n+\t\ttest_must_fail git submodule add ../child2-origin child\n+\t)\n+'\n+\n test_done\n-- \n2.49.GIT\n\n"},{"id":"518078","messageId":"xmqqplga6c80.fsf@gitster.g","threadId":"63433","inReplyTo":"20250514020108.24396-1-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH v3] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-14T22:47:59Z","receivedAt":"2025-05-14T22:48:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"K Jayatheerth writes:\n\n> Add helper `submodule_active_matches_path()` so we can\n> re-implement the old “is this path already covered by\n> submodule.active?” logic without re-reading the config twice.\n\nHaving duplicated code to implement what is supposed to be the same\nthing is a bug waiting to happen by them diverging from each other.\n\nIsn't the fact that our configuration reading code reads things just\nonce and the caches the result good enough for the purpose of this\ncode path? Do we have a measurement that tells us that the extra\ncomplexity is worth the maintenance headache?\n\n> @@ -3443,7 +3452,11 @@ static int module_add(int argc, const char **argv, const char *prefix,\n> \tint force = 0, quiet = 0, progress = 0, dissociate = 0;\n> \tstruct add_data add_data = ADD_DATA_INIT;\n> \tconst char *ref_storage_format = NULL;\n> +\tconst struct submodule *existing;\n> \tchar *to_free = NULL;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tint i;\n> +\tint allocated_sm_name = 0;\n\nA separate flag is not wrong per-se, but the idiom used in this\nproject more often is to have an extra pointer variable that points\nat an allocated piece of memory (or NULL), and free the piece of\nmemory unconditionally.\n\n\"git grep -e to_free\" to see the idiom in action. Even better yet,\nthis codepath already uses the idiom.\n\nBy doing so\n\n> +\tif (allocated_sm_name)\n> +\t\tfree((char *)add_data.sm_name);\n\nbecomes\n\n\tfree(sm_name_to_free);\n\nand we can keep the \"add_data.sm_name is pointing at a borrowed\npiece of memory, and we will _never_ free anything through that\npointer\" memory ownership rule. We were borrowing from a separate\nvariable sm_name_to_free, and we may free it when add_data is\ngetting destroyed, or we may be borrowing from the .sm_path string,\nwhich we would free it when add_data is getting destroyed.\n\n> \tfree(add_data.sm_path);\n> \tfree(to_free);\n> \tstrbuf_release(&sb);\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index d6a501d453..5c3f471338 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -1482,4 +1482,27 @@ test_expect_success '`submodule init` and `init.templateDir`' '\n> \t)\n> '\n> \n> +test_expect_success 'submodule add fails when name is reused' '\n> +\tgit init test-submodule &&\n> +\t(\n> +\t\tcd test-submodule &&\n> +\t\tgit commit --allow-empty -m \"initial commit\" &&\n> +\n> +\t\tgit init ../child-origin &&\n> +\t\tgit -C ../child-origin commit --allow-empty -m \"initial commit\" &&\n> +\n> +\t\tgit submodule add ../child-origin child &&\n> +\t\tgit commit -m \"Add submodule child\" &&\n> +\n> +\t\tgit mv child child_old &&\n> +\t\tgit commit -m \"Move child to child_old\" &&\n> +\n> +\t\t# Create another submodule repo\n> +\t\tgit init ../child2-origin &&\n> +\t\tgit -C ../child2-origin commit --allow-empty -m \"initial commit\" &&\n> +\n> +\t\ttest_must_fail git submodule add ../child2-origin child\n> +\t)\n> +'\n> +\n> test_done\n"},{"id":"518217","messageId":"CA+rGoLfYLcx+tM6nif7E5vZDce7PeGyGJkTRraT7kF3ACAzBfA@mail.gmail.com","threadId":"63433","inReplyTo":"xmqqplga6c80.fsf@gitster.g","subject":"Re: [PATCH v3] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-16T08:51:56Z","receivedAt":"2025-05-16T08:52:08Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"On Thu, May 15, 2025 at 4:18 AM Junio C Hamano wrote:\n>\n> K Jayatheerth writes:\n>\n> > Add helper `submodule_active_matches_path()` so we can\n> > re-implement the old “is this path already covered by\n> > submodule.active?” logic without re-reading the config twice.\n>\n> Having duplicated code to implement what is supposed to be the same\n> thing is a bug waiting to happen by them diverging from each other.\n>\n> Isn't the fact that our configuration reading code reads things just\n> once and the caches the result good enough for the purpose of this\n> code path? Do we have a measurement that tells us that the extra\n> complexity is worth the maintenance headache?\n>\n\nWell when I first sent this patch I didn't quite understand why test\n4137 was failing\nthen I read the tests and I didn't have a lot of idea of the code. But\nI think it' better to remove helper as you said\nI will send a new patch with a different approach as I've spent some\ntime understanding this now.\n\n> > @@ -3443,7 +3452,11 @@ static int module_add(int argc, const char **argv, const char *prefix,\n> > int force = 0, quiet = 0, progress = 0, dissociate = 0;\n> > struct add_data add_data = ADD_DATA_INIT;\n> > const char *ref_storage_format = NULL;\n> > + const struct submodule *existing;\n> > char *to_free = NULL;\n> > + struct strbuf buf = STRBUF_INIT;\n> > + int i;\n> > + int allocated_sm_name = 0;\n>\n> A separate flag is not wrong per-se, but the idiom used in this\n> project more often is to have an extra pointer variable that points\n> at an allocated piece of memory (or NULL), and free the piece of\n> memory unconditionally.\n>\n> \"git grep -e to_free\" to see the idiom in action. Even better yet,\n> this codepath already uses the idiom.\n>\n> By doing so\n>\n> > + if (allocated_sm_name)\n> > + free((char *)add_data.sm_name);\n>\n> becomes\n>\n> free(sm_name_to_free);\n>\n> and we can keep the \"add_data.sm_name is pointing at a borrowed\n> piece of memory, and we will _never_ free anything through that\n> pointer\" memory ownership rule. We were borrowing from a separate\n> variable sm_name_to_free, and we may free it when add_data is\n> getting destroyed, or we may be borrowing from the .sm_path string,\n> which we would free it when add_data is getting destroyed.\n>\n\nInteresting, this is good, I'm going to copy this : )\n\nThank you,\n\n-Jayatheerth\n"},{"id":"518272","messageId":"20250516174934.45008-1-jayatheerthkulkarni2005@gmail.com","threadId":"63433","inReplyTo":"CA+rGoLfYLcx+tM6nif7E5vZDce7PeGyGJkTRraT7kF3ACAzBfA@mail.gmail.com","subject":"[PATCH v4] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-16T17:49:34Z","receivedAt":"2025-05-16T17:49:50Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"When adding a submodule, Git may write `submodule..active = true`\nto the repository configuration to explicitly mark the submodule as active.\nHowever, if the submodule's path is already matched by a pattern in the\n`submodule.active` setting, this explicit entry is redundant.\n\nThis change inlines the logic, using `wildmatch()` to check if\nthe added submodule path is already covered by one of the patterns defined\nin `submodule.active`.\n\nIf the path matches any pattern, Git now avoids writing the\n`submodule..active` entry. This prevents unnecessary configuration\nbloat and aligns the behavior with user expectations when using\npattern-based submodule activation.\n\nReported-by: Moritz \nSigned-off-by: K Jayatheerth \n---\n builtin/submodule--helper.c | 59 ++++++++++++++++++++++++++++++-------\n t/t7400-submodule-basic.sh | 23 +++++++++++++++\n 2 files changed, 72 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 53da2116dd..5a9c8bdc0c 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -32,6 +32,8 @@\n #include \"advice.h\"\n #include \"branch.h\"\n #include \"list-objects-filter-options.h\"\n+#include \"strbuf.h\"\n+#include \"wildmatch.h\"\n \n #define OPT_QUIET (1 << 0)\n #define OPT_CACHED (1 << 1)\n@@ -3328,6 +3330,9 @@ static void configure_added_submodule(struct add_data *add_data)\n \tchar *key;\n \tstruct child_process add_submod = CHILD_PROCESS_INIT;\n \tstruct child_process add_gitmodules = CHILD_PROCESS_INIT;\n+\tconst struct string_list *values;\n+\tsize_t i;\n+\tint matched = 0;\n \n \tkey = xstrfmt(\"submodule.%s.url\", add_data->sm_name);\n \tgit_config_set_gently(key, add_data->realrepo);\n@@ -3370,20 +3375,25 @@ static void configure_added_submodule(struct add_data *add_data)\n \t * is_submodule_active(), since that function needs to find\n \t * out the value of \"submodule.active\" again anyway.\n \t */\n-\tif (!git_config_get(\"submodule.active\")) {\n-\t\t/*\n-\t\t * If the submodule being added isn't already covered by the\n-\t\t * current configured pathspec, set the submodule's active flag\n-\t\t */\n-\t\tif (!is_submodule_active(the_repository, add_data->sm_path)) {\n+\tif (git_config_get(\"submodule.active\") || /* key absent */\n+\t git_config_get_string_multi(\"submodule.active\", &values)) {\n+\t\t/* submodule.active is missing -> force-enable */\n+\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n+\t\tgit_config_set_gently(key, \"true\");\n+\t\tfree(key);\n+\t} else {\n+\t\tfor (i = 0; i < values->nr; i++) {\n+\t\t\tconst char *pat = values->items[i].string;\n+\t\t\tif (!wildmatch(pat, add_data->sm_path, 0)) { /* match found */\n+\t\t\t\tmatched = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tif (!matched) { /* no pattern matched -> force-enable */\n \t\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n \t\t\tgit_config_set_gently(key, \"true\");\n \t\t\tfree(key);\n \t\t}\n-\t} else {\n-\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n-\t\tgit_config_set_gently(key, \"true\");\n-\t\tfree(key);\n \t}\n }\n \n@@ -3443,7 +3453,11 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \tint force = 0, quiet = 0, progress = 0, dissociate = 0;\n \tstruct add_data add_data = ADD_DATA_INIT;\n \tconst char *ref_storage_format = NULL;\n+\tconst struct submodule *existing;\n \tchar *to_free = NULL;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint i;\n+\tchar *sm_name_to_free = NULL;\n \tstruct option options[] = {\n \t\tOPT_STRING('b', \"branch\", &add_data.branch, N_(\"branch\"),\n \t\t\t N_(\"branch of repository to add as submodule\")),\n@@ -3546,6 +3560,30 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \tif(!add_data.sm_name)\n \t\tadd_data.sm_name = add_data.sm_path;\n \n+\texisting = submodule_from_name(the_repository,\n+\t\t\t\t\tnull_oid(the_hash_algo),\n+\t\t\t\t\tadd_data.sm_name);\n+\n+\tif (existing && strcmp(existing->path, add_data.sm_path)) {\n+\t\tif (!force) {\n+\t\t\tdie(_(\"submodule name '%s' already used for path '%s'\"),\n+\t\t\tadd_data.sm_name, existing->path);\n+\t\t}\n+\n+\t\t/* --force: build until unique */\n+\t\tfor (i = 1; ; i++) {\n+\t\t\tstrbuf_reset(&buf);\n+\t\t\tstrbuf_addf(&buf, \"%s%d\", add_data.sm_name, i);\n+\t\t\tif (!submodule_from_name(the_repository,\n+\t\t\t\t\t\tnull_oid(the_hash_algo),\n+\t\t\t\t\t\tbuf.buf)) {\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tadd_data.sm_name = sm_name_to_free = strbuf_detach(&buf, NULL);\n+\t}\n+\n \tif (check_submodule_name(add_data.sm_name))\n \t\tdie(_(\"'%s' is not a valid submodule name\"), add_data.sm_name);\n \n@@ -3561,6 +3599,7 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \n \tret = 0;\n cleanup:\n+\tfree(sm_name_to_free);\n \tfree(add_data.sm_path);\n \tfree(to_free);\n \tstrbuf_release(&sb);\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex d6a501d453..5c3f471338 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1482,4 +1482,27 @@ test_expect_success '`submodule init` and `init.templateDir`' '\n \t)\n '\n \n+test_expect_success 'submodule add fails when name is reused' '\n+\tgit init test-submodule &&\n+\t(\n+\t\tcd test-submodule &&\n+\t\tgit commit --allow-empty -m \"initial commit\" &&\n+\n+\t\tgit init ../child-origin &&\n+\t\tgit -C ../child-origin commit --allow-empty -m \"initial commit\" &&\n+\n+\t\tgit submodule add ../child-origin child &&\n+\t\tgit commit -m \"Add submodule child\" &&\n+\n+\t\tgit mv child child_old &&\n+\t\tgit commit -m \"Move child to child_old\" &&\n+\n+\t\t# Create another submodule repo\n+\t\tgit init ../child2-origin &&\n+\t\tgit -C ../child2-origin commit --allow-empty -m \"initial commit\" &&\n+\n+\t\ttest_must_fail git submodule add ../child2-origin child\n+\t)\n+'\n+\n test_done\n-- \n2.49.GIT\n\n"},{"id":"518273","messageId":"CA+rGoLfLA_s_xYN1N41w=vaOg0WPTNo_9TGKvG6ZQDqqWmzFPA@mail.gmail.com","threadId":"63433","inReplyTo":"20250516174934.45008-1-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH v4] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-16T17:53:27Z","receivedAt":"2025-05-16T17:53:41Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"I think the logic is almost similar as the helper function in this patch,\n\nBut I tried to wrap it within the if else block\n\nReason why this is needed is\nI initially thought that this was just a path issue\nTurned out tests like\nt4137 and t7413 failed\n\nThese failed tests were because I didn't take\nsubmodule active into consideration\n\nAnd at the end since this is the only place where the helper is needed\nI just wrapped it up for this specific thing\n\nMaybe if someone might need it in future or someone builds an helper\nfunction then using that might make sense\nbut for now I think this is the middle ground approach we could find.\n\n-Jayatheerth\n"},{"id":"518360","messageId":"20250518075436.75139-1-jayatheerthkulkarni2005@gmail.com","threadId":"63433","inReplyTo":"CA+rGoLfLA_s_xYN1N41w=vaOg0WPTNo_9TGKvG6ZQDqqWmzFPA@mail.gmail.com","subject":"[PATCH v5] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-18T07:54:35Z","receivedAt":"2025-05-18T07:54:52Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"When a submodule is added, Git writes submodule..active = true\nto the repository configuration to mark it as active. This happens even\nwhen the submodule path already matches a pattern in submodule.active.\nThis results in redundant configuration entries that are unnecessary\nand clutter the config, especially when pattern-based activation is used.\n\nAvoid writing the submodule..active entry if the path is already\ncovered by a pattern in submodule.active.\n\nReported-by: Moritz \nSigned-off-by: K Jayatheerth \n---\n builtin/submodule--helper.c | 59 ++++++++++++++++++++++++++++++-------\n t/t7400-submodule-basic.sh | 23 +++++++++++++++\n 2 files changed, 72 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 53da2116dd..5a9c8bdc0c 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -32,6 +32,8 @@\n #include \"advice.h\"\n #include \"branch.h\"\n #include \"list-objects-filter-options.h\"\n+#include \"strbuf.h\"\n+#include \"wildmatch.h\"\n \n #define OPT_QUIET (1 << 0)\n #define OPT_CACHED (1 << 1)\n@@ -3328,6 +3330,9 @@ static void configure_added_submodule(struct add_data *add_data)\n \tchar *key;\n \tstruct child_process add_submod = CHILD_PROCESS_INIT;\n \tstruct child_process add_gitmodules = CHILD_PROCESS_INIT;\n+\tconst struct string_list *values;\n+\tsize_t i;\n+\tint matched = 0;\n \n \tkey = xstrfmt(\"submodule.%s.url\", add_data->sm_name);\n \tgit_config_set_gently(key, add_data->realrepo);\n@@ -3370,20 +3375,25 @@ static void configure_added_submodule(struct add_data *add_data)\n \t * is_submodule_active(), since that function needs to find\n \t * out the value of \"submodule.active\" again anyway.\n \t */\n-\tif (!git_config_get(\"submodule.active\")) {\n-\t\t/*\n-\t\t * If the submodule being added isn't already covered by the\n-\t\t * current configured pathspec, set the submodule's active flag\n-\t\t */\n-\t\tif (!is_submodule_active(the_repository, add_data->sm_path)) {\n+\tif (git_config_get(\"submodule.active\") || /* key absent */\n+\t git_config_get_string_multi(\"submodule.active\", &values)) {\n+\t\t/* submodule.active is missing -> force-enable */\n+\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n+\t\tgit_config_set_gently(key, \"true\");\n+\t\tfree(key);\n+\t} else {\n+\t\tfor (i = 0; i < values->nr; i++) {\n+\t\t\tconst char *pat = values->items[i].string;\n+\t\t\tif (!wildmatch(pat, add_data->sm_path, 0)) { /* match found */\n+\t\t\t\tmatched = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tif (!matched) { /* no pattern matched -> force-enable */\n \t\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n \t\t\tgit_config_set_gently(key, \"true\");\n \t\t\tfree(key);\n \t\t}\n-\t} else {\n-\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n-\t\tgit_config_set_gently(key, \"true\");\n-\t\tfree(key);\n \t}\n }\n \n@@ -3443,7 +3453,11 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \tint force = 0, quiet = 0, progress = 0, dissociate = 0;\n \tstruct add_data add_data = ADD_DATA_INIT;\n \tconst char *ref_storage_format = NULL;\n+\tconst struct submodule *existing;\n \tchar *to_free = NULL;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint i;\n+\tchar *sm_name_to_free = NULL;\n \tstruct option options[] = {\n \t\tOPT_STRING('b', \"branch\", &add_data.branch, N_(\"branch\"),\n \t\t\t N_(\"branch of repository to add as submodule\")),\n@@ -3546,6 +3560,30 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \tif(!add_data.sm_name)\n \t\tadd_data.sm_name = add_data.sm_path;\n \n+\texisting = submodule_from_name(the_repository,\n+\t\t\t\t\tnull_oid(the_hash_algo),\n+\t\t\t\t\tadd_data.sm_name);\n+\n+\tif (existing && strcmp(existing->path, add_data.sm_path)) {\n+\t\tif (!force) {\n+\t\t\tdie(_(\"submodule name '%s' already used for path '%s'\"),\n+\t\t\tadd_data.sm_name, existing->path);\n+\t\t}\n+\n+\t\t/* --force: build until unique */\n+\t\tfor (i = 1; ; i++) {\n+\t\t\tstrbuf_reset(&buf);\n+\t\t\tstrbuf_addf(&buf, \"%s%d\", add_data.sm_name, i);\n+\t\t\tif (!submodule_from_name(the_repository,\n+\t\t\t\t\t\tnull_oid(the_hash_algo),\n+\t\t\t\t\t\tbuf.buf)) {\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tadd_data.sm_name = sm_name_to_free = strbuf_detach(&buf, NULL);\n+\t}\n+\n \tif (check_submodule_name(add_data.sm_name))\n \t\tdie(_(\"'%s' is not a valid submodule name\"), add_data.sm_name);\n \n@@ -3561,6 +3599,7 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \n \tret = 0;\n cleanup:\n+\tfree(sm_name_to_free);\n \tfree(add_data.sm_path);\n \tfree(to_free);\n \tstrbuf_release(&sb);\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex d6a501d453..5c3f471338 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1482,4 +1482,27 @@ test_expect_success '`submodule init` and `init.templateDir`' '\n \t)\n '\n \n+test_expect_success 'submodule add fails when name is reused' '\n+\tgit init test-submodule &&\n+\t(\n+\t\tcd test-submodule &&\n+\t\tgit commit --allow-empty -m \"initial commit\" &&\n+\n+\t\tgit init ../child-origin &&\n+\t\tgit -C ../child-origin commit --allow-empty -m \"initial commit\" &&\n+\n+\t\tgit submodule add ../child-origin child &&\n+\t\tgit commit -m \"Add submodule child\" &&\n+\n+\t\tgit mv child child_old &&\n+\t\tgit commit -m \"Move child to child_old\" &&\n+\n+\t\t# Create another submodule repo\n+\t\tgit init ../child2-origin &&\n+\t\tgit -C ../child2-origin commit --allow-empty -m \"initial commit\" &&\n+\n+\t\ttest_must_fail git submodule add ../child2-origin child\n+\t)\n+'\n+\n test_done\n-- \n2.49.GIT\n\n"},{"id":"518361","messageId":"CA+rGoLcUMzj1eaGct+r1wUF5K_nMLepFz3kbopVYyoQXLY_0WA@mail.gmail.com","threadId":"63433","inReplyTo":"20250518075436.75139-1-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH v5] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-18T07:58:15Z","receivedAt":"2025-05-18T07:58:27Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"No separate change\njust changed the commit message to the format\n\nAdded _order_ to the last line.\n\nThank you,\n\n-Jayatheerth\n"},{"id":"518427","messageId":"xmqqwmacobf0.fsf@gitster.g","threadId":"63433","inReplyTo":"20250518075436.75139-1-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH v5] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-19T15:41:39Z","receivedAt":"2025-05-19T15:41:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"K Jayatheerth writes:\n\n> When a submodule is added, Git writes submodule..active = true\n> to the repository configuration to mark it as active. This happens even\n> when the submodule path already matches a pattern in submodule.active.\n> This results in redundant configuration entries that are unnecessary\n> and clutter the config, especially when pattern-based activation is used.\n>\n> Avoid writing the submodule..active entry if the path is already\n> covered by a pattern in submodule.active.\n\nThis explains why the part of the change that deals the .active bit\nmakes sense.\n\nBut now do we drop the other fix from the patch, namely \"ouch, we\nare adding submodule at 'foo/' but the name 'foo' is taken by a\ndifferent submodule that used to live there and moved elsewhere\", or\nhave you forgotten to describe that fix in the proposed log message?\n\nStepping back a bit, perhaps this patch addresses two independent\nissues, both of which can trigger with\"submodule add\"? If so, would\nit make sense to have it in two separate patches?\n\n> +test_expect_success 'submodule add fails when name is reused' '\n> +\tgit init test-submodule &&\n> +\t(\n> +\t\tcd test-submodule &&\n> +\t\tgit commit --allow-empty -m \"initial commit\" &&\n> +\n> +\t\tgit init ../child-origin &&\n> +\t\tgit -C ../child-origin commit --allow-empty -m \"initial commit\" &&\n> +\n> +\t\tgit submodule add ../child-origin child &&\n> +\t\tgit commit -m \"Add submodule child\" &&\n> +\n> +\t\tgit mv child child_old &&\n> +\t\tgit commit -m \"Move child to child_old\" &&\n> +\n> +\t\t# Create another submodule repo\n> +\t\tgit init ../child2-origin &&\n> +\t\tgit -C ../child2-origin commit --allow-empty -m \"initial commit\" &&\n> +\n> +\t\ttest_must_fail git submodule add ../child2-origin child\n> +\t)\n> +'\n\nThe test seems to be about \"the other issue\". Shouldn't we also\nhave a test about \"we no longer add redundant configuration entries\"?\n\nThanks.\n"},{"id":"518462","messageId":"CA+rGoLdbmRkk5b3LJ+n3HG6OJv7oURJaLuWs3tTymDfsbBgwjA@mail.gmail.com","threadId":"63433","inReplyTo":"xmqqwmacobf0.fsf@gitster.g","subject":"Re: [PATCH v5] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-20T01:31:24Z","receivedAt":"2025-05-20T01:31:37Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"On Mon, May 19, 2025 at 9:11 PM Junio C Hamano wrote:\n>\n> K Jayatheerth writes:\n>\n> > When a submodule is added, Git writes submodule..active = true\n> > to the repository configuration to mark it as active. This happens even\n> > when the submodule path already matches a pattern in submodule.active.\n> > This results in redundant configuration entries that are unnecessary\n> > and clutter the config, especially when pattern-based activation is used.\n> >\n> > Avoid writing the submodule..active entry if the path is already\n> > covered by a pattern in submodule.active.\n>\n> This explains why the part of the change that deals the .active bit\n> makes sense.\n>\n\nHmm, I look at it this way,\nactive and path problem are not different,\nIt's just that this has to follow t7413\nsubmodule active too.\nTherefore the submodule..active = true\nlogic exists\n > >Avoid writing the submodule..active entry if the path is already\n > >covered by a pattern in submodule.active.\nI think this order makes sense to me, but I could change if you want me to.\n\nThe path is the core issue and the active is just like a neat wrapper\nof following all the\ntest cases in my opinion.\n\n> But now do we drop the other fix from the patch, namely \"ouch, we\n> are adding submodule at 'foo/' but the name 'foo' is taken by a\n> different submodule that used to live there and moved elsewhere\", or\n> have you forgotten to describe that fix in the proposed log message?\n>\n> Stepping back a bit, perhaps this patch addresses two independent\n> issues, both of which can trigger with\"submodule add\"? If so, would\n> it make sense to have it in two separate patches?\n>\n\nThis would be the case if I wrote a separate helper function I guess\nbut the core issue still lies at the if else block\nAnd the active part is just written to mark the submodule..active = true\nSo I think these are a part of the same problem.\n\n> > +test_expect_success 'submodule add fails when name is reused' '\n> > + git init test-submodule &&\n> > + (\n> > + cd test-submodule &&\n> > + git commit --allow-empty -m \"initial commit\" &&\n> > +\n> > + git init ../child-origin &&\n> > + git -C ../child-origin commit --allow-empty -m \"initial commit\" &&\n> > +\n> > + git submodule add ../child-origin child &&\n> > + git commit -m \"Add submodule child\" &&\n> > +\n> > + git mv child child_old &&\n> > + git commit -m \"Move child to child_old\" &&\n> > +\n> > + # Create another submodule repo\n> > + git init ../child2-origin &&\n> > + git -C ../child2-origin commit --allow-empty -m \"initial commit\" &&\n> > +\n> > + test_must_fail git submodule add ../child2-origin child\n> > + )\n> > +'\n>\n> The test seems to be about \"the other issue\". Shouldn't we also\n> have a test about \"we no longer add redundant configuration entries\"?\n>\n\nActually t7413 has a detailed coverage of the _active_ logic.\nI actually didn't consider submodule..active = true\nbut looking what failed in 7413 made me realise we have to\nsolve the core issue and then\nsubmodule..active = true it too.\nTherefore I didn't add extra test case too.\n\n> Thanks.\n\nI'm open to a review from your side on this\nI may have missed some logic here.\n\nThank you,\n\n-Jayatheerth\n"},{"id":"518501","messageId":"xmqqsekzjo7g.fsf@gitster.g","threadId":"63433","inReplyTo":"CA+rGoLdbmRkk5b3LJ+n3HG6OJv7oURJaLuWs3tTymDfsbBgwjA@mail.gmail.com","subject":"Re: [PATCH v5] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-20T15:28:51Z","receivedAt":"2025-05-20T15:28:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"JAYATHEERTH K writes:\n\n> On Mon, May 19, 2025 at 9:11 PM Junio C Hamano wrote:\n>>\n>> K Jayatheerth writes:\n>>\n>> > When a submodule is added, Git writes submodule..active = true\n>> > to the repository configuration to mark it as active. This happens even\n>> > when the submodule path already matches a pattern in submodule.active.\n>> > This results in redundant configuration entries that are unnecessary\n>> > and clutter the config, especially when pattern-based activation is used.\n>> >\n>> > Avoid writing the submodule..active entry if the path is already\n>> > covered by a pattern in submodule.active.\n>>\n>> This explains why the part of the change that deals the .active bit\n>> makes sense.\n>>\n>\n> Hmm, I look at it this way,\n> active and path problem are not different,\n> It's just that this has to follow t7413\n> submodule active too.\n> Therefore the submodule..active = true\n> logic exists\n> > >Avoid writing the submodule..active entry if the path is already\n> > >covered by a pattern in submodule.active.\n> I think this order makes sense to me, but I could change if you want me to.\n\nDo you mean the problem \"we add a submodule at 'foo/' but the name\n'foo' is taken already\" does *not* happen when submodule.active\npatterns are *not* used? If so, I understand that these two\nproblems are linked together, but I got an impression that it is not\nwhat is happening.\n\nAnd that is where this question ...\n\n>> Stepping back a bit, perhaps this patch addresses two independent\n>> issues, both of which can trigger with\"submodule add\"? If so, would\n>> it make sense to have it in two separate patches?\n\n... comes from. The added test does not use submodule.active\npattern, so I assumed that the pattern-based activation is a\nseparate issue.\n\nThe solution to pattern-based activation may have to happen by\nupdating the same code path and it may turn out to be that having a\nsingle helper function that deals with both is the cleanest\nsolution, but still if they are two different problems, a single\npatch should not try to solve both of them at the same time. Solve\none by introducing a helper function and make sure that the fix for\nthat one problem will not get broken by adding tests, and then on\ntop, solve the other, perhaps by modifying the same helper function,\nand make sure that the fix for the other problem will not get broken\nby adding further tests, prehaps?\n\n> This would be the case if I wrote a separate helper function I guess\n> but the core issue still lies at the if else block\n> And the active part is just written to mark the submodule..active = true\n> So I think these are a part of the same problem.\n>\n>> > +test_expect_success 'submodule add fails when name is reused' '\n>> > + git init test-submodule &&\n>> > + (\n>> > + cd test-submodule &&\n>> > + git commit --allow-empty -m \"initial commit\" &&\n>> > +\n>> > + git init ../child-origin &&\n>> > + git -C ../child-origin commit --allow-empty -m \"initial commit\" &&\n>> > +\n>> > + git submodule add ../child-origin child &&\n>> > + git commit -m \"Add submodule child\" &&\n>> > +\n>> > + git mv child child_old &&\n>> > + git commit -m \"Move child to child_old\" &&\n>> > +\n>> > + # Create another submodule repo\n>> > + git init ../child2-origin &&\n>> > + git -C ../child2-origin commit --allow-empty -m \"initial commit\" &&\n>> > +\n>> > + test_must_fail git submodule add ../child2-origin child\n>> > + )\n>> > +'\n>>\n>> The test seems to be about \"the other issue\". Shouldn't we also\n>> have a test about \"we no longer add redundant configuration entries\"?\n\n> Actually t7413 has a detailed coverage of the _active_ logic.\n> I actually didn't consider submodule..active = true\n> but looking what failed in 7413 made me realise we have to\n> solve the core issue and then\n> submodule..active = true it too.\n> Therefore I didn't add extra test case too.\n\nWhen we re-read what you wrote as a proposed log message, you said\nthat adding a new submodule that would match the \"submodule.active\"\npattern will result in redundant configuration entries that are\nunnecessary.\n\nIf you perceive it as a problem to be solved (and you do---otherwise\nyou wouldn't have written that there), the existing tests do *not*\nconsider it as a problem, so even if they have tests that does the\npattern-based activation, they would not have any tests that make\nsure they do not create redundant configuration entries, would they?\nIf we fixed that issue, then now we need to make sure that future\nchanges will not break the fix and start adding redundant\nconfiguration entries, but because there is no test that does so\n(otherwise, you wouldn't have found a need to fix that \"redundant\nconfiguration entries\" problem in the first place), the patch that\nfixes the problem needs to add tests to do so, no?\n\nWhat \"redundant\" entries do you exactly mean? If it is \"we end up\nwith the same submodule..active appearing twice\", then the\ntest should make sure the resulting .git/config file has only one\n(perhaps with \"git config --get-all\"), for example.\n\n"},{"id":"518829","messageId":"20250524064847.51900-1-jayatheerthkulkarni2005@gmail.com","threadId":"63433","inReplyTo":"xmqqsekzjo7g.fsf@gitster.g","subject":"[PATCH v6 0/2] Avoid submodule overwritten and skip redundant active entries","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-24T06:48:45Z","receivedAt":"2025-05-24T06:48:54Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"This series of patch covers mainly two areas\n\n1. The bug report where after submodule was moved and the path remained same\n when a new submodule was added then it directly was overwriting the \n moved submodule as the present submodule since the path matched.\n\n2. The configure_added_submodule was writing submodule..active\n entry, even when the new path is already matched by submodule.active\n patterns.\n\nBelow is a helper function and 2 new tests with fixes of the above problem.\n\nK Jayatheerth (2):\n submodule: prevent overwriting .gitmodules entry on path reuse\n submodule: skip redundant active entries when pattern covers path\n\n builtin/submodule--helper.c | 60 +++++++++++++++++++++++++++-------\n t/t7400-submodule-basic.sh | 23 +++++++++++++\n t/t7413-submodule-is-active.sh | 15 +++++++++\n 3 files changed, 87 insertions(+), 11 deletions(-)\n\n-- \n2.49.GIT\n\n"},{"id":"518830","messageId":"20250524064847.51900-2-jayatheerthkulkarni2005@gmail.com","threadId":"63433","inReplyTo":"20250524064847.51900-1-jayatheerthkulkarni2005@gmail.com","subject":"[PATCH v6 1/2] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-24T06:48:46Z","receivedAt":"2025-05-24T06:48:57Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"Adding a submodule at a path that previously hosted another submodule\n(e.g., 'child') reuses the submodule name derived from the path. If the\noriginal submodule was only moved (e.g., to 'child_old') and not renamed,\nthis silently overwrites its configuration in .gitmodules.\n\nThis behavior loses user configuration and causes confusion when the\noriginal submodule is expected to remain intact. It assumes that the\npath-derived name is always safe to reuse, even though the name might\nstill be in use elsewhere in the repository.\n\nTeach `module_add()` to check if the computed submodule name already\nexists in the repository's submodule config, and if so, refuse the\noperation unless the user explicitly renames or uses force to auto increment.\n\nSigned-off-by: K Jayatheerth \n---\n builtin/submodule--helper.c | 28 ++++++++++++++++++++++++++++\n t/t7400-submodule-basic.sh | 23 +++++++++++++++++++++++\n 2 files changed, 51 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 53da2116dd..9f6df833f0 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -3444,6 +3444,10 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \tstruct add_data add_data = ADD_DATA_INIT;\n \tconst char *ref_storage_format = NULL;\n \tchar *to_free = NULL;\n+\tconst struct submodule *existing;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint i;\n+\tchar *sm_name_to_free = NULL;\n \tstruct option options[] = {\n \t\tOPT_STRING('b', \"branch\", &add_data.branch, N_(\"branch\"),\n \t\t\t N_(\"branch of repository to add as submodule\")),\n@@ -3546,6 +3550,29 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \tif(!add_data.sm_name)\n \t\tadd_data.sm_name = add_data.sm_path;\n \n+\texisting = submodule_from_name(the_repository,\n+\t\t\t\t\tnull_oid(the_hash_algo),\n+\t\t\t\t\tadd_data.sm_name);\n+\n+\tif (existing && strcmp(existing->path, add_data.sm_path)) {\n+\t\tif (!force) {\n+\t\t\tdie(_(\"submodule name '%s' already used for path '%s'\"),\n+\t\t\tadd_data.sm_name, existing->path);\n+\t\t}\n+\n+\t\t/* --force: build until unique */\n+\t\tfor (i = 1; ; i++) {\n+\t\t\tstrbuf_reset(&buf);\n+\t\t\tstrbuf_addf(&buf, \"%s%d\", add_data.sm_name, i);\n+\t\t\tif (!submodule_from_name(the_repository,\n+\t\t\t\t\t\tnull_oid(the_hash_algo),\n+\t\t\t\t\t\tbuf.buf)) {\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tadd_data.sm_name = sm_name_to_free = strbuf_detach(&buf, NULL);\n+\t}\n \tif (check_submodule_name(add_data.sm_name))\n \t\tdie(_(\"'%s' is not a valid submodule name\"), add_data.sm_name);\n \n@@ -3561,6 +3588,7 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \n \tret = 0;\n cleanup:\n+\tfree(sm_name_to_free);\n \tfree(add_data.sm_path);\n \tfree(to_free);\n \tstrbuf_release(&sb);\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex d6a501d453..07951f0c6d 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1482,4 +1482,27 @@ test_expect_success '`submodule init` and `init.templateDir`' '\n \t)\n '\n \n+test_expect_success 'submodule add fails when name is reused' '\n+ git init test-submodule &&\n+ (\n+ cd test-submodule &&\n+ git commit --allow-empty -m init &&\n+\n+ git init ../child-origin &&\n+ git -C ../child-origin commit --allow-empty -m init &&\n+\n+ git submodule add ../child-origin child &&\n+ git commit -m \"Add submodule child\" &&\n+\n+ git mv child child_old &&\n+ git commit -m \"Move child to child_old\" &&\n+\n+ # Now adding a *new* repo at the old name must fail\n+ git init ../child2-origin &&\n+ git -C ../child2-origin commit --allow-empty -m init &&\n+ test_must_fail git submodule add ../child2-origin child\n+ )\n+'\n+\n+\n test_done\n-- \n2.49.GIT\n\n"},{"id":"518831","messageId":"20250524064847.51900-3-jayatheerthkulkarni2005@gmail.com","threadId":"63433","inReplyTo":"20250524064847.51900-1-jayatheerthkulkarni2005@gmail.com","subject":"[PATCH v6 2/2] submodule: skip redundant active entries when pattern covers path","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-24T06:48:47Z","receivedAt":"2025-05-24T06:49:00Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"configure_added_submodule always writes an explicit submodule..active\nentry, even when the new path is already matched by submodule.active\npatterns. This leads to unnecessary and cluttered configuration.\n\nIntroduce a single helper to centralize wildmatch-based pattern lookup.\nIn configure_added_submodule, wrap the active-entry write in a conditional\nthat only fires when that helper reports no existing pattern covers the\nsubmodule’s path.\n\nSigned-off-by: K Jayatheerth \n---\n builtin/submodule--helper.c | 32 +++++++++++++++++++++-----------\n t/t7413-submodule-is-active.sh | 15 +++++++++++++++\n 2 files changed, 36 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 9f6df833f0..8872c0fce3 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -32,6 +32,8 @@\n #include \"advice.h\"\n #include \"branch.h\"\n #include \"list-objects-filter-options.h\"\n+#include \"wildmatch.h\"\n+#include \"strbuf.h\"\n \n #define OPT_QUIET (1 << 0)\n #define OPT_CACHED (1 << 1)\n@@ -3323,6 +3325,24 @@ static int config_submodule_in_gitmodules(const char *name, const char *var, con\n \treturn ret;\n }\n \n+static int submodule_active_matches_path(const char *path)\n+{\n+\tconst struct string_list *values;\n+\tsize_t i;\n+\n+\tif (git_config_get_string_multi(\"submodule.active\", &values))\n+\t\treturn 0;\n+\n+\tfor (i = 0; i < values->nr; i++) {\n+\t\tconst char *pat = values->items[i].string;\n+\t\tif (!wildmatch(pat, path, 0))\n+\t\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+\n static void configure_added_submodule(struct add_data *add_data)\n {\n \tchar *key;\n@@ -3370,17 +3390,7 @@ static void configure_added_submodule(struct add_data *add_data)\n \t * is_submodule_active(), since that function needs to find\n \t * out the value of \"submodule.active\" again anyway.\n \t */\n-\tif (!git_config_get(\"submodule.active\")) {\n-\t\t/*\n-\t\t * If the submodule being added isn't already covered by the\n-\t\t * current configured pathspec, set the submodule's active flag\n-\t\t */\n-\t\tif (!is_submodule_active(the_repository, add_data->sm_path)) {\n-\t\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n-\t\t\tgit_config_set_gently(key, \"true\");\n-\t\t\tfree(key);\n-\t\t}\n-\t} else {\n+\tif (!submodule_active_matches_path(add_data->sm_path)) {\n \t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n \t\tgit_config_set_gently(key, \"true\");\n \t\tfree(key);\ndiff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh\nindex 9509dc18fd..a42060cac9 100755\n--- a/t/t7413-submodule-is-active.sh\n+++ b/t/t7413-submodule-is-active.sh\n@@ -124,4 +124,19 @@ test_expect_success 'is-active, submodule.active and submodule add' '\n \tgit -C super2 config --get submodule.mod.active\n '\n \n+test_expect_success 'submodule add skips redundant active entry' '\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config submodule.active \"lib/*\" &&\n+\t\tgit commit --allow-empty -m init &&\n+\n+\t\tgit init ../lib-origin &&\n+\t\tgit -C ../lib-origin commit --allow-empty -m init &&\n+\n+\t\tgit submodule add ../lib-origin lib/foo &&\n+\t\t! git config --get submodule.lib/foo.active\n+\t)\n+'\n+\n test_done\n-- \n2.49.GIT\n\n"},{"id":"518832","messageId":"CA+rGoLdbZvD_aMfB=J_cJay-4Ptgf+=R3es9vmdrsHkGhCtPmw@mail.gmail.com","threadId":"63433","inReplyTo":"20250524064847.51900-3-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH v6 2/2] submodule: skip redundant active entries when pattern covers path","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-24T06:54:53Z","receivedAt":"2025-05-24T06:55:05Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"Since I added the helper back\nI wrote the tests for the active part of the problem too and separated\nthe patches for clarity.\n\nI also ran the tests and it passed the test 9 in t7413 and added test 10.\n\nThank you,\n\n-Jayatheerth\n"},{"id":"518833","messageId":"20250524073055.58092-1-jayatheerthkulkarni2005@gmail.com","threadId":"63433","inReplyTo":"CA+rGoLdbZvD_aMfB=J_cJay-4Ptgf+=R3es9vmdrsHkGhCtPmw@mail.gmail.com","subject":"[PATCH v7 0/2] Avoid submodule overwritten and skip redundant active entries","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-24T07:30:53Z","receivedAt":"2025-05-24T07:31:07Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"This series of patch covers mainly two areas\n\n1. The bug report where after submodule was moved and the path remained same\n when a new submodule was added then it directly was overwriting the \n moved submodule as the present submodule since the path matched.\n\n2. The configure_added_submodule was writing submodule..active\n entry, even when the new path is already matched by submodule.active\n patterns.\n\nBelow is a helper function and 2 new tests with fixes of the above problem.\n\nV7 has added Tab spaces in the test rather than 4 spaces in V6\n\nJunio C Hamano (1):\n The seventeenth batch\n\nK Jayatheerth (1):\n submodule: prevent overwriting .gitmodules entry on path reuse\n\n Documentation/RelNotes/2.50.0.adoc | 19 +++++++++++++++++++\n builtin/submodule--helper.c | 28 ++++++++++++++++++++++++++++\n t/t7400-submodule-basic.sh | 23 +++++++++++++++++++++++\n 3 files changed, 70 insertions(+)\n\n-- \n2.49.GIT\n\n"},{"id":"518834","messageId":"20250524073055.58092-2-jayatheerthkulkarni2005@gmail.com","threadId":"63433","inReplyTo":"20250524073055.58092-1-jayatheerthkulkarni2005@gmail.com","subject":"[PATCH v7 1/2] The seventeenth batch","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-24T07:30:54Z","receivedAt":"2025-05-24T07:31:17Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"From: Junio C Hamano \n\nSigned-off-by: Junio C Hamano \n---\n Documentation/RelNotes/2.50.0.adoc | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/Documentation/RelNotes/2.50.0.adoc b/Documentation/RelNotes/2.50.0.adoc\nindex bf73de114e..f721ea350d 100644\n--- a/Documentation/RelNotes/2.50.0.adoc\n+++ b/Documentation/RelNotes/2.50.0.adoc\n@@ -72,6 +72,10 @@ UI, Workflows & Features\n * The `send-email` documentation has been updated with OAuth2.0\n related examples.\n \n+ * Two of the \"scalar\" subcommands that add a repository that hasn't\n+ been under \"scalar\"'s control are taught an option not to enable the\n+ scheduled maintenance on it.\n+\n \n Performance, Internal Implementation, Development Support etc.\n --------------------------------------------------------------\n@@ -157,6 +161,12 @@ Performance, Internal Implementation, Development Support etc.\n \n * Build performance fix.\n \n+ * Teach \"git send-email\" to also consult `hostname -f` for mail\n+ domain to compute the identity given to SMTP servers.\n+\n+ * The dependency on the_repository variable has been reduced from the\n+ code paths in \"git replay\".\n+\n \n Fixes since v2.49\n -----------------\n@@ -306,6 +316,15 @@ Fixes since v2.49\n * Use-after-free fix in the sequencer.\n (merge 5dbaec628d pw/sequencer-reflog-use-after-free later to maint).\n \n+ * win+Meson CI pipeline, unlike other pipelines for Windows,\n+ used to build artifacts in develper mode, which has been changed to\n+ build them in release mode for consistency.\n+ (merge 184abdcf05 js/ci-build-win-in-release-mode later to maint).\n+\n+ * CI settings at GitLab has been updated to run MSVC based Meson job\n+ automatically (as opposed to be done only upon manual request).\n+ (merge 6389579b2f ps/ci-gitlab-enable-msvc-meson-job later to maint).\n+\n * Other code cleanup, docfix, build fix, etc.\n (merge 227c4f33a0 ja/doc-block-delimiter-markup-fix later to maint).\n (merge 2bfd3b3685 ab/decorate-code-cleanup later to maint).\n-- \n2.49.GIT\n\n"},{"id":"518835","messageId":"20250524073055.58092-3-jayatheerthkulkarni2005@gmail.com","threadId":"63433","inReplyTo":"20250524073055.58092-1-jayatheerthkulkarni2005@gmail.com","subject":"[PATCH v7 2/2] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-24T07:30:55Z","receivedAt":"2025-05-24T07:31:33Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"Adding a submodule at a path that previously hosted another submodule\n(e.g., 'child') reuses the submodule name derived from the path. If the\noriginal submodule was only moved (e.g., to 'child_old') and not renamed,\nthis silently overwrites its configuration in .gitmodules.\n\nThis behavior loses user configuration and causes confusion when the\noriginal submodule is expected to remain intact. It assumes that the\npath-derived name is always safe to reuse, even though the name might\nstill be in use elsewhere in the repository.\n\nTeach `module_add()` to check if the computed submodule name already\nexists in the repository's submodule config, and if so, refuse the\noperation unless the user explicitly renames or uses force to auto increment.\n\nSigned-off-by: K Jayatheerth \n---\n builtin/submodule--helper.c | 28 ++++++++++++++++++++++++++++\n t/t7400-submodule-basic.sh | 23 +++++++++++++++++++++++\n 2 files changed, 51 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 53da2116dd..9f6df833f0 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -3444,6 +3444,10 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \tstruct add_data add_data = ADD_DATA_INIT;\n \tconst char *ref_storage_format = NULL;\n \tchar *to_free = NULL;\n+\tconst struct submodule *existing;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint i;\n+\tchar *sm_name_to_free = NULL;\n \tstruct option options[] = {\n \t\tOPT_STRING('b', \"branch\", &add_data.branch, N_(\"branch\"),\n \t\t\t N_(\"branch of repository to add as submodule\")),\n@@ -3546,6 +3550,29 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \tif(!add_data.sm_name)\n \t\tadd_data.sm_name = add_data.sm_path;\n \n+\texisting = submodule_from_name(the_repository,\n+\t\t\t\t\tnull_oid(the_hash_algo),\n+\t\t\t\t\tadd_data.sm_name);\n+\n+\tif (existing && strcmp(existing->path, add_data.sm_path)) {\n+\t\tif (!force) {\n+\t\t\tdie(_(\"submodule name '%s' already used for path '%s'\"),\n+\t\t\tadd_data.sm_name, existing->path);\n+\t\t}\n+\n+\t\t/* --force: build until unique */\n+\t\tfor (i = 1; ; i++) {\n+\t\t\tstrbuf_reset(&buf);\n+\t\t\tstrbuf_addf(&buf, \"%s%d\", add_data.sm_name, i);\n+\t\t\tif (!submodule_from_name(the_repository,\n+\t\t\t\t\t\tnull_oid(the_hash_algo),\n+\t\t\t\t\t\tbuf.buf)) {\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tadd_data.sm_name = sm_name_to_free = strbuf_detach(&buf, NULL);\n+\t}\n \tif (check_submodule_name(add_data.sm_name))\n \t\tdie(_(\"'%s' is not a valid submodule name\"), add_data.sm_name);\n \n@@ -3561,6 +3588,7 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \n \tret = 0;\n cleanup:\n+\tfree(sm_name_to_free);\n \tfree(add_data.sm_path);\n \tfree(to_free);\n \tstrbuf_release(&sb);\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex d6a501d453..f5514decab 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1482,4 +1482,27 @@ test_expect_success '`submodule init` and `init.templateDir`' '\n \t)\n '\n \n+test_expect_success 'submodule add fails when name is reused' '\n+\tgit init test-submodule &&\n+\t(\n+\t\tcd test-submodule &&\n+\t\tgit commit --allow-empty -m init &&\n+\n+\t\tgit init ../child-origin &&\n+\t\tgit -C ../child-origin commit --allow-empty -m init &&\n+\n+\t\tgit submodule add ../child-origin child &&\n+\t\tgit commit -m \"Add submodule child\" &&\n+\n+\t\tgit mv child child_old &&\n+\t\tgit commit -m \"Move child to child_old\" &&\n+\n+\t\t# Now adding a *new* repo at the old name must fail\n+\t\tgit init ../child2-origin &&\n+\t\tgit -C ../child2-origin commit --allow-empty -m init &&\n+\t\ttest_must_fail git submodule add ../child2-origin child\n+\t)\n+'\n+\n+\n test_done\n-- \n2.49.GIT\n\n"},{"id":"518836","messageId":"CA+rGoLdP+CPhqX8WCAPDUnKBUfQSZBpXeSJOXc3KuS4sVR-2ag@mail.gmail.com","threadId":"63433","inReplyTo":"20250524073055.58092-2-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH v7 1/2] The seventeenth batch","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-24T07:33:10Z","receivedAt":"2025-05-24T07:33:22Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"Oh my god\n\nI'm so clumsy today\n\nSorry bout that\nwill send in a new thread.\n\n-Jayatheerth\n"}]}