{"thread":{"id":"63515","subject":"[PATCH v7 0/2] Avoid submodule overwritten and skip redundant active entries","startedAt":"2025-05-24T07:36:35Z","lastAt":"2025-07-25T17:28:30Z","messageCount":19,"participants":["K Jayatheerth","Junio C Hamano","JAYATHEERTH K","D. Ben Knoble","Ben Knoble"],"isPatch":true,"patchVersion":7,"patchTotal":2},"messages":[{"id":"518837","messageId":"20250524073628.58944-1-jayatheerthkulkarni2005@gmail.com","threadId":"63515","inReplyTo":null,"subject":"[PATCH v7 0/2] Avoid submodule overwritten and skip redundant active entries","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-24T07:36:26Z","receivedAt":"2025-05-24T07:36:35Z","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.<name>.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\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":"518838","messageId":"20250524073628.58944-2-jayatheerthkulkarni2005@gmail.com","threadId":"63515","inReplyTo":"20250524073628.58944-1-jayatheerthkulkarni2005@gmail.com","subject":"[PATCH v7 1/2] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-24T07:36:27Z","receivedAt":"2025-05-24T07:36:37Z","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 <jayatheerthkulkarni2005@gmail.com>\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 <name><n> 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":"518839","messageId":"20250524073628.58944-3-jayatheerthkulkarni2005@gmail.com","threadId":"63515","inReplyTo":"20250524073628.58944-1-jayatheerthkulkarni2005@gmail.com","subject":"[PATCH v7 2/2] submodule: skip redundant active entries when pattern covers path","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-24T07:36:28Z","receivedAt":"2025-05-24T07:36:39Z","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.<name>.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 <jayatheerthkulkarni2005@gmail.com>\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":"518972","messageId":"xmqqplfurw3q.fsf@gitster.g","threadId":"63515","inReplyTo":"20250524073628.58944-2-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH v7 1/2] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-27T14:01:29Z","receivedAt":"2025-05-27T14:01:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:\n\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\nLooks quite straight-forward to me.  Great.\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> +\t\t}\n> +\n> +\t\t/* --force: build <name><n> 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"},{"id":"518992","messageId":"xmqqcyburu6z.fsf@gitster.g","threadId":"63515","inReplyTo":"20250524073628.58944-3-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH v7 2/2] submodule: skip redundant active entries when pattern covers path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-27T14:42:44Z","receivedAt":"2025-05-27T14:42:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:\n\n> configure_added_submodule always writes an explicit submodule.<name>.active\n> entry, even when the new path is already matched by submodule.active\n> patterns. This leads to unnecessary and cluttered configuration.\n\nIt might be less cluttered but is it a good thing?\n\nEarlier, if submodule.active were b* (as documented in the \"git help\nsubmodules\") and then you added submodule \"baz\", your \"baz\" would be\nkept active even after you reconfigured submodule.active to another\npattern.  With this change, that is no longer true, which to existing\nusers is a change in behaviour, and to some it may appear a regression.\n\nAccording to that documentation, presense of submodule.<name>.url is\nalso a signal that the submodule is activated.  If we are going to\nomit setting .active because its path matches submodule.active,\nshouldn't we also be checking if .url exists and omit setting\n.active as well?\n\n> Introduce a single helper to centralize wildmatch-based pattern lookup.\n> In configure_added_submodule, wrap the active-entry write in a conditional\n> that only fires when that helper reports no existing pattern covers the\n> submodule’s path.\n\nThe new helper is a maintenance burden to keep in sync with\nsubmodule.c:is_tree_submodule_active(); if we really want to go this\nroute, the patch should extract that \"ah, submodule.active is set so\nlet's turn it into pathspec and see if the path matches\" part of the\nlogic to make sure the logic is shared.  But I am wondering if we\ncan do this without any new helper.\n\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\nI.e.  Can we replace this if() condition with something like this?\n\n\t/*\n\t * Explicitly set 'submodule.<name>.active' only if it is not\n\t * 'active' due to other reasons.\n\t */\n\tif (!is_submodule_active(the_repository, add_data->sm_path)) {\n\nThat is, we ask if the submodule is already active (we are before\nadding submodule.<name>.active for this thing---if may be active due\nto submodule.active or submodule.<name>.url) and enter the block\nonly when it is not yet.\n\nThat way, this codepath does not have to worry about the exact logic\nthat determines if a submodule is 'active' even when its .active\nconfiguration variable is not set.\n\n>  \t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n>  \t\tgit_config_set_gently(key, \"true\");\n>  \t\tfree(key);\n"},{"id":"519133","messageId":"CA+rGoLfpj2tepMSWLfNeVkwXfzHZB7Vc8_GJ+_=bWkQSzZ+Sjg@mail.gmail.com","threadId":"63515","inReplyTo":"xmqqcyburu6z.fsf@gitster.g","subject":"Re: [PATCH v7 2/2] submodule: skip redundant active entries when pattern covers path","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-05-29T04:23:23Z","receivedAt":"2025-05-29T04:23:35Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"On Tue, May 27, 2025 at 8:12 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:\n>\n> > configure_added_submodule always writes an explicit submodule.<name>.active\n> > entry, even when the new path is already matched by submodule.active\n> > patterns. This leads to unnecessary and cluttered configuration.\n>\n> It might be less cluttered but is it a good thing?\n>\n> Earlier, if submodule.active were b* (as documented in the \"git help\n> submodules\") and then you added submodule \"baz\", your \"baz\" would be\n> kept active even after you reconfigured submodule.active to another\n> pattern.  With this change, that is no longer true, which to existing\n> users is a change in behaviour, and to some it may appear a regression.\n>\n> According to that documentation, presense of submodule.<name>.url is\n> also a signal that the submodule is activated.  If we are going to\n> omit setting .active because its path matches submodule.active,\n> shouldn't we also be checking if .url exists and omit setting\n> .active as well?\n>\n> > Introduce a single helper to centralize wildmatch-based pattern lookup.\n> > In configure_added_submodule, wrap the active-entry write in a conditional\n> > that only fires when that helper reports no existing pattern covers the\n> > submodule’s path.\n>\n> The new helper is a maintenance burden to keep in sync with\n> submodule.c:is_tree_submodule_active(); if we really want to go this\n> route, the patch should extract that \"ah, submodule.active is set so\n> let's turn it into pathspec and see if the path matches\" part of the\n> logic to make sure the logic is shared.  But I am wondering if we\n> can do this without any new helper.\n>\n\nI've actually done something like this, but I've wrapped the core logic within\nthe if else after checking [1]\n\n> > @@ -3370,17 +3390,7 @@ static void configure_added_submodule(struct add_data *add_data)\n> >        * is_submodule_active(), since that function needs to find\n> >        * out the value of \"submodule.active\" again anyway.\n> >        */\n> > -     if (!git_config_get(\"submodule.active\")) {\n> > -             /*\n> > -              * If the submodule being added isn't already covered by the\n> > -              * current configured pathspec, set the submodule's active flag\n> > -              */\n> > -             if (!is_submodule_active(the_repository, add_data->sm_path)) {\n> > -                     key = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n> > -                     git_config_set_gently(key, \"true\");\n> > -                     free(key);\n> > -             }\n> > -     } else {\n> > +     if (!submodule_active_matches_path(add_data->sm_path)) {\n>\n> I.e.  Can we replace this if() condition with something like this?\n>\n>         /*\n>          * Explicitly set 'submodule.<name>.active' only if it is not\n>          * 'active' due to other reasons.\n>          */\n>         if (!is_submodule_active(the_repository, add_data->sm_path)) {\n>\n> That is, we ask if the submodule is already active (we are before\n> adding submodule.<name>.active for this thing---if may be active due\n> to submodule.active or submodule.<name>.url) and enter the block\n> only when it is not yet.\n>\n> That way, this codepath does not have to worry about the exact logic\n> that determines if a submodule is 'active' even when its .active\n> configuration variable is not set.\n>\n> >               key = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n> >               git_config_set_gently(key, \"true\");\n> >               free(key);\n\n\nI've done the exact thing in a bit different way\nShould I add this in the second patch, because this passes the tests too\nincluding the new one created.\n\n1- https://lore.kernel.org/git/20250518075436.75139-1-jayatheerthkulkarni2005@gmail.com/\n"},{"id":"519631","messageId":"xmqqh60wf850.fsf@gitster.g","threadId":"63515","inReplyTo":"CA+rGoLfpj2tepMSWLfNeVkwXfzHZB7Vc8_GJ+_=bWkQSzZ+Sjg@mail.gmail.com","subject":"Re: [PATCH v7 2/2] submodule: skip redundant active entries when pattern covers path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-04T04:22:35Z","receivedAt":"2025-06-04T04:22:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"JAYATHEERTH K <jayatheerthkulkarni2005@gmail.com> writes:\n\n>> The new helper is a maintenance burden to keep in sync with\n>> submodule.c:is_tree_submodule_active(); if we really want to go this\n>> route, the patch should extract that \"ah, submodule.active is set so\n>> let's turn it into pathspec and see if the path matches\" part of the\n>> logic to make sure the logic is shared.  But I am wondering if we\n>> can do this without any new helper.\n>\n> I've actually done something like this, but I've wrapped the core logic within\n> the if else after checking [1]\n\nI do not think I follow what you want to say here.  Care to rephrase?\n\n>> I.e.  Can we replace this if() condition with something like this?\n>>\n>>         /*\n>>          * Explicitly set 'submodule.<name>.active' only if it is not\n>>          * 'active' due to other reasons.\n>>          */\n>>         if (!is_submodule_active(the_repository, add_data->sm_path)) {\n>>\n>> That is, we ask if the submodule is already active (we are before\n>> adding submodule.<name>.active for this thing---if may be active due\n>> to submodule.active or submodule.<name>.url) and enter the block\n>> only when it is not yet.\n>>\n>> That way, this codepath does not have to worry about the exact logic\n>> that determines if a submodule is 'active' even when its .active\n>> configuration variable is not set.\n>>\n>> >               key = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n>> >               git_config_set_gently(key, \"true\");\n>> >               free(key);\n>\n>\n> I've done the exact thing in a bit different way\n\nI am confused again, as this reads as if it is an oxymoron --- did\nyou write the exactly the same thing as suggested above, or did you\nwrite something different?\n\n"},{"id":"519851","messageId":"CA+rGoLdTT3kdELUyHdZLWyy8e6AbfRU7kDFcVUdCmVtDi11hMw@mail.gmail.com","threadId":"63515","inReplyTo":"xmqqh60wf850.fsf@gitster.g","subject":"Re: [PATCH v7 2/2] submodule: skip redundant active entries when pattern covers path","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-06-06T14:41:42Z","receivedAt":"2025-06-06T14:41:55Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"On Wed, Jun 4, 2025 at 9:52 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> JAYATHEERTH K <jayatheerthkulkarni2005@gmail.com> writes:\n>\n> >> The new helper is a maintenance burden to keep in sync with\n> >> submodule.c:is_tree_submodule_active(); if we really want to go this\n> >> route, the patch should extract that \"ah, submodule.active is set so\n> >> let's turn it into pathspec and see if the path matches\" part of the\n> >> logic to make sure the logic is shared.  But I am wondering if we\n> >> can do this without any new helper.\n> >\n> > I've actually done something like this, but I've wrapped the core logic within\n> > the if else after checking [1]\n>\n> I do not think I follow what you want to say here.  Care to rephrase?\n>\n> >> I.e.  Can we replace this if() condition with something like this?\n> >>\n> >>         /*\n> >>          * Explicitly set 'submodule.<name>.active' only if it is not\n> >>          * 'active' due to other reasons.\n> >>          */\n> >>         if (!is_submodule_active(the_repository, add_data->sm_path)) {\n> >>\n> >> That is, we ask if the submodule is already active (we are before\n> >> adding submodule.<name>.active for this thing---if may be active due\n> >> to submodule.active or submodule.<name>.url) and enter the block\n> >> only when it is not yet.\n> >>\n> >> That way, this codepath does not have to worry about the exact logic\n> >> that determines if a submodule is 'active' even when its .active\n> >> configuration variable is not set.\n> >>\n> >> >               key = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n> >> >               git_config_set_gently(key, \"true\");\n> >> >               free(key);\n> >\n> >\n> > I've done the exact thing in a bit different way\n>\n> I am confused again, as this reads as if it is an oxymoron --- did\n> you write the exactly the same thing as suggested above, or did you\n> write something different?\n>\n\n\nOk so I'm expand on why I thought this would work;\n\n- if (!git_config_get(\"submodule.active\")) {\n- /*\n- * If the submodule being added isn't already covered by the\n- * current configured pathspec, set the submodule's active flag\n- */\n- if (!is_submodule_active(the_repository, add_data->sm_path)) {\n+ if (git_config_get(\"submodule.active\") || /* key absent */\n+   git_config_get_string_multi(\"submodule.active\", &values)) {\n+ /* submodule.active is missing -> force-enable */\n+ key = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n+ git_config_set_gently(key, \"true\");\n+ free(key);\n+ } else {\n+ for (i = 0; i < values->nr; i++) {\n+ const char *pat = values->items[i].string;\n+ if (!wildmatch(pat, add_data->sm_path, 0)) { /* match found */\n+ matched = 1;\n+ break;\n+ }\n+ }\n+ if (!matched) { /* no pattern matched -> force-enable */\n  key = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n  git_config_set_gently(key, \"true\");\n  free(key);\n  }\n- } else {\n- key = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n- git_config_set_gently(key, \"true\");\n- free(key);\n  }\n }\n\nTo clarify: My aim with the current patch wasn't to create a\ngeneral-purpose helper, but to very precisely control the conditions\nunder which submodule.<name>.active = true is written during a git\nsubmodule add operation. But I sure did have a thought that something\nlike this should exist.\n\nThe goals I considerat core when adding a new submodules should be\nsomething like\na. The submodule should become active. This is the primary user expectation.\nb.The configuration should be as clean and intentional as possible.\nAvoid redundant entries if the global submodule.active already covers\nthe new submodule.\nb. The configuration should explicitly reflect the active state if\nthere's no global policy, or if the global policy.\n\nThis was my underlying goal to be very specific.\n\nYour suggestion to use if (!is_submodule_active(the_repository,\nadd_data->sm_path)) is elegant in its reuse of existing logic.\nHowever, I find these following things\n\ni. Scenario: submodule.active is globally ABSENT.\nis_submodule_active() will return true (as submodules are active by\ndefault in this case).\n\nTherefore, !is_submodule_active() will be false.\n\nResult with your suggestion: submodule.<name>.active = true would not\nbe written. The submodule is active by default, but this isn't\nexplicitly recorded for the submodule itself. Also a reason why the\ntest failed after digging a bit deeper this is the possible\nexplanation I found.\nSpecifically Test case 9 from t7413\n\ntest_expect_success 'is-active, submodule.active and submodule add' '\ntest_when_finished \"rm -rf super2\" &&\ngit init super2 &&\ntest_commit -C super2 initial &&\ngit -C super2 config --add submodule.active \"sub*\" &&\n\n# submodule add should only add submodule.<name>.active\n# to the config if not matched by the pathspec\ngit -C super2 submodule add ../sub sub1 &&\ntest_must_fail git -C super2 config --get submodule.sub1.active &&\n\ngit -C super2 submodule add ../sub mod &&\ngit -C super2 config --get submodule.mod.active\n'\n\nii. My patch's behavior in this scenario (And not as a general purpose helper)\n\nThe first condition git_config_get(\"submodule.active\") ||\ngit_config_get_string_multi(\"submodule.active\", &values) evaluates to\ntrue because submodule.active is absent.\n\nResult with my patch: submodule.<name>.active = true is explicitly written.\n\nLet me also clarify on what I mean by the _similar_ and _different_\nstatements I sent above.\nThe outcome is what I meant was _similar_ the patch link I sent in the\nabove mail, explicitly adds only when it doesn't find\nnew submodule to be active, and I verified it with a documented test\nadded as the tenth test in t7413 where it shouldn't add\nan active twice and the outcome is what I found _similar_, the\n_difference_ however are in the methods of getting to the outcome,\nand adding guardrails. That's the underlying thought with that statement.\n\nTo reinforce the above ideas her e aer some points I would like to make:\n\n\nThe logic implemented in the else block (reading submodule.active and\niterating with wildmatch) is indeed a partial re-implementation of\nwhat is_submodule_active might do internally. However:\n\nIt's highly localized to this specific decision point in git submodule add.\nIt's concerned only with the submodule.active pathspecs.\nis_submodule_active might have broader considerations (as you hinted,\ne.g., submodule.<name>.url's presence, though that's less relevant for\nthe .active flag itself).\nThe benefit of precise control over config generation for add might\noutweigh the risk of this small, localized logic diverging, especially\nsince submodule.active's pathspec interpretation is fairly stable.\n\nThe core distinction is that is_submodule_active() answers \"Is this\nsubmodule considered active right now based on all existing rules and\ndefaults?\". My patch attempts to answer a slightly different question\nfor git add: \"Given the current global submodule.active policy, do we\nneed to explicitly set submodule.<name>.active = true for this new\nsubmodule to ensure it's active and clearly recorded as such?\"\nAnd that is the reason why I had to mention the _different_ and _same_\nstatement within the same statement.\n\nMy approach aims to set submodule.<name>.active = true unless an\nexisting, valid, and matching global submodule.active pattern already\nmakes it active. This leads to what I feel is a more robust and\nexplicit configuration outcome from git submodule add.\n\nPerhaps the ideal long-term solution would involve a more nuanced\nhelper function (which I'm more than happy to write, if I get the\ngreen flag.), but for the immediate improvement to git add's behavior,\nI believe this patch strikes a good balance, prioritizing explicit\nactivation and config clarity for newly added submodules.\n\n\n\nAlso to clarify :)\nI tried to dig and clarify as much as I can\nI maybe wrong in some places cause obviously I'm still relatively much\nnewer to the source code.\n\nBut I thought providing a huge para with what I thought would be\nhelpful for corrections.\n\nThank you for reading all the blab : )\n\n- Jayatheerth\n"},{"id":"519904","messageId":"20250608032705.11990-1-jayatheerthkulkarni2005@gmail.com","threadId":"63515","inReplyTo":"CA+rGoLdTT3kdELUyHdZLWyy8e6AbfRU7kDFcVUdCmVtDi11hMw@mail.gmail.com","subject":"[PATCH v8 0/2] Avoid submodule overwritten and skip redundant active entries","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-06-08T03:27:03Z","receivedAt":"2025-06-08T03:27:35Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"The first patch i.e. prevent overwriting .gitmodules entry on path reuse\nis exactly the same \n\nThe second patch however i.e. \nskip redundant active entries when pattern covers path\nhas changed logic \nwith the helper function being code maintainance because \nof the duplicated logic. I've tried to wrap it as much as \npossible to it's own core need without having to change it\nunless the core way of submodules addition and active status\nitself changes.\n\nThe CI was tested in which \nthe only compiler error I found was \nosx-gcc\nWhich I suppose is already addressed,\napart from this all of the other tests \nran successfully which includes the \nt7413 (prvious 9) and the new one.\n\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":"519905","messageId":"20250608032705.11990-2-jayatheerthkulkarni2005@gmail.com","threadId":"63515","inReplyTo":"20250608032705.11990-1-jayatheerthkulkarni2005@gmail.com","subject":"[PATCH v8 1/2] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-06-08T03:27:04Z","receivedAt":"2025-06-08T03:27:43Z","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 <jayatheerthkulkarni2005@gmail.com>\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 <name><n> 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":"519906","messageId":"20250608032705.11990-3-jayatheerthkulkarni2005@gmail.com","threadId":"63515","inReplyTo":"20250608032705.11990-1-jayatheerthkulkarni2005@gmail.com","subject":"[PATCH v8 2/2] submodule: skip redundant active entries when pattern covers path","fromName":"K Jayatheerth","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-06-08T03:27:05Z","receivedAt":"2025-06-08T03:27:51Z","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.<name>.active\nentry, even when the new path is already matched by submodule.active\npatterns. This leads to unnecessary and cluttered configuration.\n\nchange the logic 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 <jayatheerthkulkarni2005@gmail.com>\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..514abe480e 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@@ -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-\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 (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}\n }\n \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":"521479","messageId":"xmqq4ivn3a1w.fsf@gitster.g","threadId":"63515","inReplyTo":"20250608032705.11990-1-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH v8 0/2] Avoid submodule overwritten and skip redundant active entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-07T22:34:03Z","receivedAt":"2025-07-07T22:34:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:\n\n> The first patch i.e. prevent overwriting .gitmodules entry on path reuse\n> is exactly the same \n>\n> The second patch however i.e. \n> skip redundant active entries when pattern covers path\n> has changed logic \n> with the helper function being code maintainance because \n> of the duplicated logic. I've tried to wrap it as much as \n> possible to it's own core need without having to change it\n> unless the core way of submodules addition and active status\n> itself changes.\n>\n> The CI was tested in which \n> the only compiler error I found was \n> osx-gcc\n> Which I suppose is already addressed,\n> apart from this all of the other tests \n> ran successfully which includes the \n> t7413 (prvious 9) and the new one.\n>\n>\n> K Jayatheerth (2):\n>   submodule: prevent overwriting .gitmodules entry on path reuse\n>   submodule: skip redundant active entries when pattern covers path\n\nHaven't seen any comment from others on this topic for quite a\nwhile.  How does this one look to those who do use submodules (I am\nnot one of them)?\n\nThanks.\n"},{"id":"521605","messageId":"CALnO6CAXermmya0UjTHU2gPoEAb1m32fOt7Uzf6CL2mSJ=RkTg@mail.gmail.com","threadId":"63515","inReplyTo":"20250608032705.11990-3-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH v8 2/2] submodule: skip redundant active entries when pattern covers path","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-07-09T02:50:23Z","receivedAt":"2025-07-09T02:50:36Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Sat, Jun 7, 2025 at 11:28 PM K Jayatheerth\n<jayatheerthkulkarni2005@gmail.com> wrote:\n>\n> configure_added_submodule always writes an explicit submodule.<name>.active\n> entry, even when the new path is already matched by submodule.active\n> patterns. This leads to unnecessary and cluttered configuration.\n>\n> change the logic to centralize wildmatch-based pattern lookup,\n> in configure_added_submodule. Wrap the active-entry write in a conditional\n> that only fires when that helper reports no existing pattern covers the\n> submodule’s path.\n\nI use submodules, but am not too familiar with their internals, so I\nfind it hard to follow the details here. Perhaps some examples showing\nwhen the configuration becomes cluttered and what about it is\ncluttered would help?\n\nWhen I check a repo of mine that has ~50 submodules:\n\n    git config get submodule.active\n    .\n    git config get --show-names --regexp 'submodule.*active'\n    submodule.active .\n\nSo I'm not seeing this in practice, though I didn't try with the test\ncase in the patch.\n\n[Nit: s/change/Change]\n\n>\n> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>\n> ---\n>  builtin/submodule--helper.c    | 32 +++++++++++++++++++++-----------\n>  t/t7413-submodule-is-active.sh | 15 +++++++++++++++\n>  2 files changed, 36 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 9f6df833f0..514abe480e 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> @@ -3328,6 +3330,9 @@ static void configure_added_submodule(struct add_data *add_data)\n>         char *key;\n>         struct child_process add_submod = CHILD_PROCESS_INIT;\n>         struct child_process add_gitmodules = CHILD_PROCESS_INIT;\n> +       const struct string_list *values;\n> +       size_t i;\n> +       int matched = 0;\n>\n>         key = xstrfmt(\"submodule.%s.url\", add_data->sm_name);\n>         git_config_set_gently(key, add_data->realrepo);\n> @@ -3370,20 +3375,25 @@ static void configure_added_submodule(struct add_data *add_data)\n>          * is_submodule_active(), since that function needs to find\n>          * out the value of \"submodule.active\" again anyway.\n>          */\n> -       if (!git_config_get(\"submodule.active\")) {\n> -               /*\n> -                * If the submodule being added isn't already covered by the\n> -                * current configured pathspec, set the submodule's active flag\n> -                */\n\nDo we want to lose this comment? The replacement below (\"… ->\nforce-enable\") is a bit terse for me and rather loses some\ninformation.\n\n> -               if (!is_submodule_active(the_repository, add_data->sm_path)) {\n> -                       key = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n> -                       git_config_set_gently(key, \"true\");\n> -                       free(key);\n> -               }\n> -       } else {\n> +       if (git_config_get(\"submodule.active\") || /* key absent */\n> +           git_config_get_string_multi(\"submodule.active\", &values)) {\n> +               /* submodule.active is missing -> force-enable */\n>                 key = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n>                 git_config_set_gently(key, \"true\");\n>                 free(key);\n> +       } else {\n> +               for (i = 0; i < values->nr; i++) {\n> +                       const char *pat = values->items[i].string;\n> +                       if (!wildmatch(pat, add_data->sm_path, 0)) { /* match found */\n> +                               matched = 1;\n> +                               break;\n> +                       }\n> +               }\n> +               if (!matched) { /* no pattern matched -> force-enable */\n> +                       key = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n> +                       git_config_set_gently(key, \"true\");\n> +                       free(key);\n> +               }\n>         }\n>  }\n>\n> diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh\n> index 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>         git -C super2 config --get submodule.mod.active\n>  '\n>\n> +test_expect_success 'submodule add skips redundant active entry' '\n> +       git init repo &&\n> +       (\n> +               cd repo &&\n> +               git config submodule.active \"lib/*\" &&\n> +               git commit --allow-empty -m init &&\n> +\n> +               git init ../lib-origin &&\n> +               git -C ../lib-origin commit --allow-empty -m init &&\n> +\n> +               git submodule add ../lib-origin lib/foo &&\n> +               ! git config --get submodule.lib/foo.active\n\n(Not my area of expertise) Should this be test_must_fail?\n\n> +       )\n> +'\n> +\n>  test_done\n> --\n> 2.49.GIT\n>\n>\n\n\n-- \nD. Ben Knoble\n"},{"id":"521606","messageId":"CALnO6CB5VezYHtp22RNihNtzLXQkP9Ly=hf0V=3A1EsTvKcFgg@mail.gmail.com","threadId":"63515","inReplyTo":"20250608032705.11990-2-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH v8 1/2] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-07-09T02:50:34Z","receivedAt":"2025-07-09T02:50:48Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Sat, Jun 7, 2025 at 11:28 PM K Jayatheerth\n<jayatheerthkulkarni2005@gmail.com> wrote:\n>\n> 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\n> original submodule was only moved (e.g., to 'child_old') and not renamed,\n> this silently overwrites its configuration in .gitmodules.\n>\n> This behavior loses user configuration and causes confusion when the\n> original submodule is expected to remain intact. It assumes that the\n> path-derived name is always safe to reuse, even though the name might\n> still be in use elsewhere in the repository.\n>\n> Teach `module_add()` to check if the computed submodule name already\n> exists in the repository's submodule config, and if so, refuse the\n> operation unless the user explicitly renames or uses force to auto increment.\n\nI had to read the patch to figure out what \"auto increment\"\nmeant—perhaps some accompanying docs in `git help submodule`?\n\n>\n> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>\n> ---\n>  builtin/submodule--helper.c | 28 ++++++++++++++++++++++++++++\n>  t/t7400-submodule-basic.sh  | 23 +++++++++++++++++++++++\n>  2 files changed, 51 insertions(+)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 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>         struct add_data add_data = ADD_DATA_INIT;\n>         const char *ref_storage_format = NULL;\n>         char *to_free = NULL;\n> +       const struct submodule *existing;\n> +       struct strbuf buf = STRBUF_INIT;\n> +       int i;\n> +       char *sm_name_to_free = NULL;\n>         struct option options[] = {\n>                 OPT_STRING('b', \"branch\", &add_data.branch, N_(\"branch\"),\n>                            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>         if(!add_data.sm_name)\n>                 add_data.sm_name = add_data.sm_path;\n>\n> +       existing = submodule_from_name(the_repository,\n> +                                       null_oid(the_hash_algo),\n> +                                       add_data.sm_name);\n> +\n> +       if (existing && strcmp(existing->path, add_data.sm_path)) {\n> +               if (!force) {\n> +                       die(_(\"submodule name '%s' already used for path '%s'\"),\n> +                       add_data.sm_name, existing->path);\n> +               }\n> +\n> +               /* --force: build <name><n> until unique */\n> +               for (i = 1; ; i++) {\n> +                       strbuf_reset(&buf);\n> +                       strbuf_addf(&buf, \"%s%d\", add_data.sm_name, i);\n> +                       if (!submodule_from_name(the_repository,\n> +                                               null_oid(the_hash_algo),\n> +                                               buf.buf)) {\n> +                               break;\n> +                       }\n> +               }\n\nThis isn't typically what I'd expect --force to do, personally, though\nin this case it allows me to proceed with an operation that wasn't\nallowed otherwise.\n\nStill, I wonder if a user might be confused by \"I said 'child' and got\n'child2'?\"\n\n> +\n> +               add_data.sm_name = sm_name_to_free = strbuf_detach(&buf, NULL);\n> +       }\n>         if (check_submodule_name(add_data.sm_name))\n>                 die(_(\"'%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>         ret = 0;\n>  cleanup:\n> +       free(sm_name_to_free);\n>         free(add_data.sm_path);\n>         free(to_free);\n>         strbuf_release(&sb);\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 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>         )\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\nThis makes sense, though I was hoping (when I'd only skimmed the\nmessage and not seen \"refuse\") that this would be permitted by some\nclever trick. Oh well.\n\n\n> +       )\n> +'\n> +\n> +\n>  test_done\n> --\n> 2.49.GIT\n>\n>\n\n\n--\nD. Ben Knoble\n"},{"id":"521681","messageId":"xmqqqzyppgbi.fsf@gitster.g","threadId":"63515","inReplyTo":"CALnO6CAXermmya0UjTHU2gPoEAb1m32fOt7Uzf6CL2mSJ=RkTg@mail.gmail.com","subject":"Re: [PATCH v8 2/2] submodule: skip redundant active entries when pattern covers path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-09T14:52:17Z","receivedAt":"2025-07-09T14:52:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"D. Ben Knoble\" <ben.knoble@gmail.com> writes:\n\n>> +test_expect_success 'submodule add skips redundant active entry' '\n>> +       git init repo &&\n>> +       (\n>> +               cd repo &&\n>> +               git config submodule.active \"lib/*\" &&\n>> +               git commit --allow-empty -m init &&\n>> +\n>> +               git init ../lib-origin &&\n>> +               git -C ../lib-origin commit --allow-empty -m init &&\n>> +\n>> +               git submodule add ../lib-origin lib/foo &&\n>> +               ! git config --get submodule.lib/foo.active\n>\n> (Not my area of expertise) Should this be test_must_fail?\n\nThat is certainly better.  \"! git config ...\" would succeed even\nwhen that command segfaults and dumps core, which we may want to\nnotice.\n"},{"id":"522217","messageId":"xmqqa5527nzk.fsf@gitster.g","threadId":"63515","inReplyTo":"20250608032705.11990-3-jayatheerthkulkarni2005@gmail.com","subject":"Re: [PATCH v8 2/2] submodule: skip redundant active entries when pattern covers path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-17T16:58:55Z","receivedAt":"2025-07-17T16:58:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:\n\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\nSomehow these lines begin with SP and then HT.  If you are going to\nsend an updated version, please make sure to fix the whitespace\nissues around here.\n\nIn the meantime, I'll tweak the version I have in 'seen'.\n\nThanks.\n"},{"id":"522280","messageId":"CA+rGoLfygOAxXNxn_TComuef6q+=v1wOoUSfKh4M6Ywk-0McHQ@mail.gmail.com","threadId":"63515","inReplyTo":"xmqqa5527nzk.fsf@gitster.g","subject":"Re: [PATCH v8 2/2] submodule: skip redundant active entries when pattern covers path","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-07-18T14:24:01Z","receivedAt":"2025-07-18T14:24:15Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"On Thu, Jul 17, 2025 at 10:28 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:\n>\n> > +             if (!matched) { /* no pattern matched -> force-enable */\n> > +                     key = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n> > +                     git_config_set_gently(key, \"true\");\n> > +                     free(key);\n> > +             }\n>\n> Somehow these lines begin with SP and then HT.  If you are going to\n> send an updated version, please make sure to fix the whitespace\n> issues around here.\n>\n> In the meantime, I'll tweak the version I have in 'seen'.\n>\n> Thanks.\n\n\nDamn, I missed the previous emails,\nActually my fault it is, I made some tweaks to my email\nBut anyways\nI will go through and send an updated version\n\nThank you\n\n- Jayatheerth\n"},{"id":"522339","messageId":"CA+rGoLfi9=h0Z86QZ2Y_HQqd+ugrMgkBzLaNSwivvkcDwmT=rg@mail.gmail.com","threadId":"63515","inReplyTo":"CALnO6CB5VezYHtp22RNihNtzLXQkP9Ly=hf0V=3A1EsTvKcFgg@mail.gmail.com","subject":"Re: [PATCH v8 1/2] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"JAYATHEERTH K","fromEmail":"jayatheerthkulkarni2005@gmail.com","sentAt":"2025-07-20T12:25:11Z","receivedAt":"2025-07-20T12:25:24Z","isPatch":true,"sender":{"key":"jayatheerthkulkarni2005@gmail.com","avatar":"https://avatars.githubusercontent.com/u/148841023?v=4"},"body":"On Wed, Jul 9, 2025 at 8:20 AM D. Ben Knoble <ben.knoble@gmail.com> wrote:\n>\n> On Sat, Jun 7, 2025 at 11:28 PM K Jayatheerth\n> <jayatheerthkulkarni2005@gmail.com> wrote:\n> >\n> > 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\n> > original submodule was only moved (e.g., to 'child_old') and not renamed,\n> > this silently overwrites its configuration in .gitmodules.\n> >\n> > This behavior loses user configuration and causes confusion when the\n> > original submodule is expected to remain intact. It assumes that the\n> > path-derived name is always safe to reuse, even though the name might\n> > still be in use elsewhere in the repository.\n> >\n> > Teach `module_add()` to check if the computed submodule name already\n> > exists in the repository's submodule config, and if so, refuse the\n> > operation unless the user explicitly renames or uses force to auto increment.\n>\n> I had to read the patch to figure out what \"auto increment\"\n> meant—perhaps some accompanying docs in `git help submodule`?\n>\n> >\n> > Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>\n> > ---\n> >  builtin/submodule--helper.c | 28 ++++++++++++++++++++++++++++\n> >  t/t7400-submodule-basic.sh  | 23 +++++++++++++++++++++++\n> >  2 files changed, 51 insertions(+)\n> >\n> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> > index 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> >         struct add_data add_data = ADD_DATA_INIT;\n> >         const char *ref_storage_format = NULL;\n> >         char *to_free = NULL;\n> > +       const struct submodule *existing;\n> > +       struct strbuf buf = STRBUF_INIT;\n> > +       int i;\n> > +       char *sm_name_to_free = NULL;\n> >         struct option options[] = {\n> >                 OPT_STRING('b', \"branch\", &add_data.branch, N_(\"branch\"),\n> >                            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> >         if(!add_data.sm_name)\n> >                 add_data.sm_name = add_data.sm_path;\n> >\n> > +       existing = submodule_from_name(the_repository,\n> > +                                       null_oid(the_hash_algo),\n> > +                                       add_data.sm_name);\n> > +\n> > +       if (existing && strcmp(existing->path, add_data.sm_path)) {\n> > +               if (!force) {\n> > +                       die(_(\"submodule name '%s' already used for path '%s'\"),\n> > +                       add_data.sm_name, existing->path);\n> > +               }\n> > +\n> > +               /* --force: build <name><n> until unique */\n> > +               for (i = 1; ; i++) {\n> > +                       strbuf_reset(&buf);\n> > +                       strbuf_addf(&buf, \"%s%d\", add_data.sm_name, i);\n> > +                       if (!submodule_from_name(the_repository,\n> > +                                               null_oid(the_hash_algo),\n> > +                                               buf.buf)) {\n> > +                               break;\n> > +                       }\n> > +               }\n>\n> This isn't typically what I'd expect --force to do, personally, though\n> in this case it allows me to proceed with an operation that wasn't\n> allowed otherwise.\n>\n> Still, I wonder if a user might be confused by \"I said 'child' and got\n> 'child2'?\"\n>\n\n\nOk so while fixing the previous versions of my submissions\nI got stumped at this, I found child<incremented val> to be intuitive\nat that time\nbut I can see why it may not be intuitive too, I mean I could just\nremove the previous child and add\nthe current data as the new child because that feels intuitive for force.\nIf that is something which is in the interest I could send the new\npatches as soon as possible.\n\nThank You\n\n- Jayatheerth\n"},{"id":"522760","messageId":"E4F5EF8F-4146-46BA-A498-8493706238A7@gmail.com","threadId":"63515","inReplyTo":"CA+rGoLfi9=h0Z86QZ2Y_HQqd+ugrMgkBzLaNSwivvkcDwmT=rg@mail.gmail.com","subject":"Re: [PATCH v8 1/2] submodule: prevent overwriting .gitmodules entry on path reuse","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-07-25T17:28:17Z","receivedAt":"2025-07-25T17:28:30Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"\n> Le 20 juil. 2025 à 08:25, JAYATHEERTH K <jayatheerthkulkarni2005@gmail.com> a écrit :\n> \n> On Wed, Jul 9, 2025 at 8:20 AM D. Ben Knoble <ben.knoble@gmail.com> wrote:\n>> \n>>> On Sat, Jun 7, 2025 at 11:28 PM K Jayatheerth\n>>> <jayatheerthkulkarni2005@gmail.com> wrote:\n>>> \n>>> 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\n>>> original submodule was only moved (e.g., to 'child_old') and not renamed,\n>>> this silently overwrites its configuration in .gitmodules.\n>>> \n>>> This behavior loses user configuration and causes confusion when the\n>>> original submodule is expected to remain intact. It assumes that the\n>>> path-derived name is always safe to reuse, even though the name might\n>>> still be in use elsewhere in the repository.\n>>> \n>>> Teach `module_add()` to check if the computed submodule name already\n>>> exists in the repository's submodule config, and if so, refuse the\n>>> operation unless the user explicitly renames or uses force to auto increment.\n>> \n>> I had to read the patch to figure out what \"auto increment\"\n>> meant—perhaps some accompanying docs in `git help submodule`?\n>> \n>>> \n>>> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>\n>>> ---\n>>> builtin/submodule--helper.c | 28 ++++++++++++++++++++++++++++\n>>> t/t7400-submodule-basic.sh  | 23 +++++++++++++++++++++++\n>>> 2 files changed, 51 insertions(+)\n>>> \n>>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>>> index 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>>>        struct add_data add_data = ADD_DATA_INIT;\n>>>        const char *ref_storage_format = NULL;\n>>>        char *to_free = NULL;\n>>> +       const struct submodule *existing;\n>>> +       struct strbuf buf = STRBUF_INIT;\n>>> +       int i;\n>>> +       char *sm_name_to_free = NULL;\n>>>        struct option options[] = {\n>>>                OPT_STRING('b', \"branch\", &add_data.branch, N_(\"branch\"),\n>>>                           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>>>        if(!add_data.sm_name)\n>>>                add_data.sm_name = add_data.sm_path;\n>>> \n>>> +       existing = submodule_from_name(the_repository,\n>>> +                                       null_oid(the_hash_algo),\n>>> +                                       add_data.sm_name);\n>>> +\n>>> +       if (existing && strcmp(existing->path, add_data.sm_path)) {\n>>> +               if (!force) {\n>>> +                       die(_(\"submodule name '%s' already used for path '%s'\"),\n>>> +                       add_data.sm_name, existing->path);\n>>> +               }\n>>> +\n>>> +               /* --force: build <name><n> until unique */\n>>> +               for (i = 1; ; i++) {\n>>> +                       strbuf_reset(&buf);\n>>> +                       strbuf_addf(&buf, \"%s%d\", add_data.sm_name, i);\n>>> +                       if (!submodule_from_name(the_repository,\n>>> +                                               null_oid(the_hash_algo),\n>>> +                                               buf.buf)) {\n>>> +                               break;\n>>> +                       }\n>>> +               }\n>> \n>> This isn't typically what I'd expect --force to do, personally, though\n>> in this case it allows me to proceed with an operation that wasn't\n>> allowed otherwise.\n>> \n>> Still, I wonder if a user might be confused by \"I said 'child' and got\n>> 'child2'?\"\n>> \n> \n> \n> Ok so while fixing the previous versions of my submissions\n> I got stumped at this, I found child<incremented val> to be intuitive\n> at that time\n> but I can see why it may not be intuitive too, I mean I could just\n> remove the previous child and add\n> the current data as the new child because that feels intuitive for force.\n> If that is something which is in the interest I could send the new\n> patches as soon as possible.\n\nThat sounds more like a typical forceful operation to me."}]}