[PATCH v7 0/2] Avoid submodule overwritten and skip redundant active entries

[PATCH v7 0/2] Avoid submodule overwritten and skip redundant active entries

[K Jayatheerth]

This series of patch covers mainly two areas

1. The bug report where after submodule was moved and the path remained same
   when a new submodule was added then it directly was overwriting the 
   moved submodule as the present submodule since the path matched.

2. The configure_added_submodule was writing submodule.<name>.active
   entry, even when the new path is already matched by submodule.active
   patterns.

Below is a helper function and 2 new tests with fixes of the above problem.


K Jayatheerth (2):
  submodule: prevent overwriting .gitmodules entry on path reuse
  submodule: skip redundant active entries when pattern covers path

 builtin/submodule--helper.c    | 60 +++++++++++++++++++++++++++-------
 t/t7400-submodule-basic.sh     | 23 +++++++++++++
 t/t7413-submodule-is-active.sh | 15 +++++++++
 3 files changed, 87 insertions(+), 11 deletions(-)

-- 
2.49.GIT

[PATCH v7 1/2] submodule: prevent overwriting .gitmodules entry on path reuse

[K Jayatheerth]

Adding a submodule at a path that previously hosted another submodule
(e.g., 'child') reuses the submodule name derived from the path. If the
original submodule was only moved (e.g., to 'child_old') and not renamed,
this silently overwrites its configuration in .gitmodules.

This behavior loses user configuration and causes confusion when the
original submodule is expected to remain intact. It assumes that the
path-derived name is always safe to reuse, even though the name might
still be in use elsewhere in the repository.

Teach `module_add()` to check if the computed submodule name already
exists in the repository's submodule config, and if so, refuse the
operation unless the user explicitly renames or uses force to auto increment.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 builtin/submodule--helper.c | 28 ++++++++++++++++++++++++++++
 t/t7400-submodule-basic.sh  | 23 +++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 53da2116dd..9f6df833f0 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -3444,6 +3444,10 @@ static int module_add(int argc, const char **argv, const char *prefix,
 	struct add_data add_data = ADD_DATA_INIT;
 	const char *ref_storage_format = NULL;
 	char *to_free = NULL;
+	const struct submodule *existing;
+	struct strbuf buf = STRBUF_INIT;
+	int i;
+	char *sm_name_to_free = NULL;
 	struct option options[] = {
 		OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
 			   N_("branch of repository to add as submodule")),
@@ -3546,6 +3550,29 @@ static int module_add(int argc, const char **argv, const char *prefix,
 	if(!add_data.sm_name)
 		add_data.sm_name = add_data.sm_path;
 
+	existing = submodule_from_name(the_repository,
+					null_oid(the_hash_algo),
+					add_data.sm_name);
+
+	if (existing && strcmp(existing->path, add_data.sm_path)) {
+		if (!force) {
+			die(_("submodule name '%s' already used for path '%s'"),
+			add_data.sm_name, existing->path);
+		}
+
+		/* --force: build <name><n> until unique */
+		for (i = 1; ; i++) {
+			strbuf_reset(&buf);
+			strbuf_addf(&buf, "%s%d", add_data.sm_name, i);
+			if (!submodule_from_name(the_repository,
+						null_oid(the_hash_algo),
+						buf.buf)) {
+				break;
+			}
+		}
+
+		add_data.sm_name = sm_name_to_free = strbuf_detach(&buf, NULL);
+	}
 	if (check_submodule_name(add_data.sm_name))
 		die(_("'%s' is not a valid submodule name"), add_data.sm_name);
 
@@ -3561,6 +3588,7 @@ static int module_add(int argc, const char **argv, const char *prefix,
 
 	ret = 0;
 cleanup:
+	free(sm_name_to_free);
 	free(add_data.sm_path);
 	free(to_free);
 	strbuf_release(&sb);
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index d6a501d453..f5514decab 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1482,4 +1482,27 @@ test_expect_success '`submodule init` and `init.templateDir`' '
 	)
 '
 
+test_expect_success 'submodule add fails when name is reused' '
+	git init test-submodule &&
+	(
+		cd test-submodule &&
+		git commit --allow-empty -m init &&
+
+		git init ../child-origin &&
+		git -C ../child-origin commit --allow-empty -m init &&
+
+		git submodule add ../child-origin child &&
+		git commit -m "Add submodule child" &&
+
+		git mv child child_old &&
+		git commit -m "Move child to child_old" &&
+
+		# Now adding a *new* repo at the old name must fail
+		git init ../child2-origin &&
+		git -C ../child2-origin commit --allow-empty -m init &&
+		test_must_fail git submodule add ../child2-origin child
+	)
+'
+
+
 test_done
-- 
2.49.GIT

Re: [PATCH v7 1/2] submodule: prevent overwriting .gitmodules entry on path reuse

[Junio C Hamano]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

> @@ -3546,6 +3550,29 @@ static int module_add(int argc, const char **argv, const char *prefix,
>  	if(!add_data.sm_name)
>  		add_data.sm_name = add_data.sm_path;
>  
> +	existing = submodule_from_name(the_repository,
> +					null_oid(the_hash_algo),
> +					add_data.sm_name);
> +
> +	if (existing && strcmp(existing->path, add_data.sm_path)) {

Looks quite straight-forward to me.  Great.

> +		if (!force) {
> +			die(_("submodule name '%s' already used for path '%s'"),
> +			add_data.sm_name, existing->path);
> +		}
> +
> +		/* --force: build <name><n> until unique */
> +		for (i = 1; ; i++) {
> +			strbuf_reset(&buf);
> +			strbuf_addf(&buf, "%s%d", add_data.sm_name, i);
> +			if (!submodule_from_name(the_repository,
> +						null_oid(the_hash_algo),
> +						buf.buf)) {
> +				break;
> +			}
> +		}
> +
> +		add_data.sm_name = sm_name_to_free = strbuf_detach(&buf, NULL);
> +	}

[PATCH v7 2/2] submodule: skip redundant active entries when pattern covers path

[K Jayatheerth]

configure_added_submodule always writes an explicit submodule.<name>.active
entry, even when the new path is already matched by submodule.active
patterns. This leads to unnecessary and cluttered configuration.

Introduce a single helper to centralize wildmatch-based pattern lookup.
In configure_added_submodule, wrap the active-entry write in a conditional
that only fires when that helper reports no existing pattern covers the
submodule’s path.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 builtin/submodule--helper.c    | 32 +++++++++++++++++++++-----------
 t/t7413-submodule-is-active.sh | 15 +++++++++++++++
 2 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9f6df833f0..8872c0fce3 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -32,6 +32,8 @@
 #include "advice.h"
 #include "branch.h"
 #include "list-objects-filter-options.h"
+#include "wildmatch.h"
+#include "strbuf.h"
 
 #define OPT_QUIET (1 << 0)
 #define OPT_CACHED (1 << 1)
@@ -3323,6 +3325,24 @@ static int config_submodule_in_gitmodules(const char *name, const char *var, con
 	return ret;
 }
 
+static int submodule_active_matches_path(const char *path)
+{
+	const struct string_list *values;
+	size_t i;
+
+	if (git_config_get_string_multi("submodule.active", &values))
+		return 0;
+
+	for (i = 0; i < values->nr; i++) {
+		const char *pat = values->items[i].string;
+		if (!wildmatch(pat, path, 0))
+			return 1;
+	}
+
+	return 0;
+}
+
+
 static void configure_added_submodule(struct add_data *add_data)
 {
 	char *key;
@@ -3370,17 +3390,7 @@ static void configure_added_submodule(struct add_data *add_data)
 	 * is_submodule_active(), since that function needs to find
 	 * out the value of "submodule.active" again anyway.
 	 */
-	if (!git_config_get("submodule.active")) {
-		/*
-		 * If the submodule being added isn't already covered by the
-		 * current configured pathspec, set the submodule's active flag
-		 */
-		if (!is_submodule_active(the_repository, add_data->sm_path)) {
-			key = xstrfmt("submodule.%s.active", add_data->sm_name);
-			git_config_set_gently(key, "true");
-			free(key);
-		}
-	} else {
+	if (!submodule_active_matches_path(add_data->sm_path)) {
 		key = xstrfmt("submodule.%s.active", add_data->sm_name);
 		git_config_set_gently(key, "true");
 		free(key);
diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh
index 9509dc18fd..a42060cac9 100755
--- a/t/t7413-submodule-is-active.sh
+++ b/t/t7413-submodule-is-active.sh
@@ -124,4 +124,19 @@ test_expect_success 'is-active, submodule.active and submodule add' '
 	git -C super2 config --get submodule.mod.active
 '
 
+test_expect_success 'submodule add skips redundant active entry' '
+	git init repo &&
+	(
+		cd repo &&
+		git config submodule.active "lib/*" &&
+		git commit --allow-empty -m init &&
+
+		git init ../lib-origin &&
+		git -C ../lib-origin commit --allow-empty -m init &&
+
+		git submodule add ../lib-origin lib/foo &&
+		! git config --get submodule.lib/foo.active
+	)
+'
+
 test_done
-- 
2.49.GIT

Re: [PATCH v7 2/2] submodule: skip redundant active entries when pattern covers path

[Junio C Hamano]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

> configure_added_submodule always writes an explicit submodule.<name>.active
> entry, even when the new path is already matched by submodule.active
> patterns. This leads to unnecessary and cluttered configuration.

It might be less cluttered but is it a good thing?

Earlier, if submodule.active were b* (as documented in the "git help
submodules") and then you added submodule "baz", your "baz" would be
kept active even after you reconfigured submodule.active to another
pattern.  With this change, that is no longer true, which to existing
users is a change in behaviour, and to some it may appear a regression.

According to that documentation, presense of submodule.<name>.url is
also a signal that the submodule is activated.  If we are going to
omit setting .active because its path matches submodule.active,
shouldn't we also be checking if .url exists and omit setting
.active as well?

> Introduce a single helper to centralize wildmatch-based pattern lookup.
> In configure_added_submodule, wrap the active-entry write in a conditional
> that only fires when that helper reports no existing pattern covers the
> submodule’s path.

The new helper is a maintenance burden to keep in sync with
submodule.c:is_tree_submodule_active(); if we really want to go this
route, the patch should extract that "ah, submodule.active is set so
let's turn it into pathspec and see if the path matches" part of the
logic to make sure the logic is shared.  But I am wondering if we
can do this without any new helper.

> @@ -3370,17 +3390,7 @@ static void configure_added_submodule(struct add_data *add_data)
>  	 * is_submodule_active(), since that function needs to find
>  	 * out the value of "submodule.active" again anyway.
>  	 */
> -	if (!git_config_get("submodule.active")) {
> -		/*
> -		 * If the submodule being added isn't already covered by the
> -		 * current configured pathspec, set the submodule's active flag
> -		 */
> -		if (!is_submodule_active(the_repository, add_data->sm_path)) {
> -			key = xstrfmt("submodule.%s.active", add_data->sm_name);
> -			git_config_set_gently(key, "true");
> -			free(key);
> -		}
> -	} else {
> +	if (!submodule_active_matches_path(add_data->sm_path)) {

I.e.  Can we replace this if() condition with something like this?

	/*
	 * Explicitly set 'submodule.<name>.active' only if it is not
	 * 'active' due to other reasons.
	 */
	if (!is_submodule_active(the_repository, add_data->sm_path)) {

That is, we ask if the submodule is already active (we are before
adding submodule.<name>.active for this thing---if may be active due
to submodule.active or submodule.<name>.url) and enter the block
only when it is not yet.

That way, this codepath does not have to worry about the exact logic
that determines if a submodule is 'active' even when its .active
configuration variable is not set.

>  		key = xstrfmt("submodule.%s.active", add_data->sm_name);
>  		git_config_set_gently(key, "true");
>  		free(key);

Re: [PATCH v7 2/2] submodule: skip redundant active entries when pattern covers path

[JAYATHEERTH K]

On Tue, May 27, 2025 at 8:12 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
>
> > configure_added_submodule always writes an explicit submodule.<name>.active
> > entry, even when the new path is already matched by submodule.active
> > patterns. This leads to unnecessary and cluttered configuration.
>
> It might be less cluttered but is it a good thing?
>
> Earlier, if submodule.active were b* (as documented in the "git help
> submodules") and then you added submodule "baz", your "baz" would be
> kept active even after you reconfigured submodule.active to another
> pattern.  With this change, that is no longer true, which to existing
> users is a change in behaviour, and to some it may appear a regression.
>
> According to that documentation, presense of submodule.<name>.url is
> also a signal that the submodule is activated.  If we are going to
> omit setting .active because its path matches submodule.active,
> shouldn't we also be checking if .url exists and omit setting
> .active as well?
>
> > Introduce a single helper to centralize wildmatch-based pattern lookup.
> > In configure_added_submodule, wrap the active-entry write in a conditional
> > that only fires when that helper reports no existing pattern covers the
> > submodule’s path.
>
> The new helper is a maintenance burden to keep in sync with
> submodule.c:is_tree_submodule_active(); if we really want to go this
> route, the patch should extract that "ah, submodule.active is set so
> let's turn it into pathspec and see if the path matches" part of the
> logic to make sure the logic is shared.  But I am wondering if we
> can do this without any new helper.
>

I've actually done something like this, but I've wrapped the core logic within
the if else after checking [1]

> > @@ -3370,17 +3390,7 @@ static void configure_added_submodule(struct add_data *add_data)
> >        * is_submodule_active(), since that function needs to find
> >        * out the value of "submodule.active" again anyway.
> >        */
> > -     if (!git_config_get("submodule.active")) {
> > -             /*
> > -              * If the submodule being added isn't already covered by the
> > -              * current configured pathspec, set the submodule's active flag
> > -              */
> > -             if (!is_submodule_active(the_repository, add_data->sm_path)) {
> > -                     key = xstrfmt("submodule.%s.active", add_data->sm_name);
> > -                     git_config_set_gently(key, "true");
> > -                     free(key);
> > -             }
> > -     } else {
> > +     if (!submodule_active_matches_path(add_data->sm_path)) {
>
> I.e.  Can we replace this if() condition with something like this?
>
>         /*
>          * Explicitly set 'submodule.<name>.active' only if it is not
>          * 'active' due to other reasons.
>          */
>         if (!is_submodule_active(the_repository, add_data->sm_path)) {
>
> That is, we ask if the submodule is already active (we are before
> adding submodule.<name>.active for this thing---if may be active due
> to submodule.active or submodule.<name>.url) and enter the block
> only when it is not yet.
>
> That way, this codepath does not have to worry about the exact logic
> that determines if a submodule is 'active' even when its .active
> configuration variable is not set.
>
> >               key = xstrfmt("submodule.%s.active", add_data->sm_name);
> >               git_config_set_gently(key, "true");
> >               free(key);


I've done the exact thing in a bit different way
Should I add this in the second patch, because this passes the tests too
including the new one created.

1- https://lore.kernel.org/git/20250518075436.75139-1-jayatheerthkulkarni2005@gmail.com/

Re: [PATCH v7 2/2] submodule: skip redundant active entries when pattern covers path

[Junio C Hamano]

JAYATHEERTH K <jayatheerthkulkarni2005@gmail.com> writes:

>> The new helper is a maintenance burden to keep in sync with
>> submodule.c:is_tree_submodule_active(); if we really want to go this
>> route, the patch should extract that "ah, submodule.active is set so
>> let's turn it into pathspec and see if the path matches" part of the
>> logic to make sure the logic is shared.  But I am wondering if we
>> can do this without any new helper.
>
> I've actually done something like this, but I've wrapped the core logic within
> the if else after checking [1]

I do not think I follow what you want to say here.  Care to rephrase?

>> I.e.  Can we replace this if() condition with something like this?
>>
>>         /*
>>          * Explicitly set 'submodule.<name>.active' only if it is not
>>          * 'active' due to other reasons.
>>          */
>>         if (!is_submodule_active(the_repository, add_data->sm_path)) {
>>
>> That is, we ask if the submodule is already active (we are before
>> adding submodule.<name>.active for this thing---if may be active due
>> to submodule.active or submodule.<name>.url) and enter the block
>> only when it is not yet.
>>
>> That way, this codepath does not have to worry about the exact logic
>> that determines if a submodule is 'active' even when its .active
>> configuration variable is not set.
>>
>> >               key = xstrfmt("submodule.%s.active", add_data->sm_name);
>> >               git_config_set_gently(key, "true");
>> >               free(key);
>
>
> I've done the exact thing in a bit different way

I am confused again, as this reads as if it is an oxymoron --- did
you write the exactly the same thing as suggested above, or did you
write something different?

Re: [PATCH v7 2/2] submodule: skip redundant active entries when pattern covers path

[JAYATHEERTH K]

On Wed, Jun 4, 2025 at 9:52 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> JAYATHEERTH K <jayatheerthkulkarni2005@gmail.com> writes:
>
> >> The new helper is a maintenance burden to keep in sync with
> >> submodule.c:is_tree_submodule_active(); if we really want to go this
> >> route, the patch should extract that "ah, submodule.active is set so
> >> let's turn it into pathspec and see if the path matches" part of the
> >> logic to make sure the logic is shared.  But I am wondering if we
> >> can do this without any new helper.
> >
> > I've actually done something like this, but I've wrapped the core logic within
> > the if else after checking [1]
>
> I do not think I follow what you want to say here.  Care to rephrase?
>
> >> I.e.  Can we replace this if() condition with something like this?
> >>
> >>         /*
> >>          * Explicitly set 'submodule.<name>.active' only if it is not
> >>          * 'active' due to other reasons.
> >>          */
> >>         if (!is_submodule_active(the_repository, add_data->sm_path)) {
> >>
> >> That is, we ask if the submodule is already active (we are before
> >> adding submodule.<name>.active for this thing---if may be active due
> >> to submodule.active or submodule.<name>.url) and enter the block
> >> only when it is not yet.
> >>
> >> That way, this codepath does not have to worry about the exact logic
> >> that determines if a submodule is 'active' even when its .active
> >> configuration variable is not set.
> >>
> >> >               key = xstrfmt("submodule.%s.active", add_data->sm_name);
> >> >               git_config_set_gently(key, "true");
> >> >               free(key);
> >
> >
> > I've done the exact thing in a bit different way
>
> I am confused again, as this reads as if it is an oxymoron --- did
> you write the exactly the same thing as suggested above, or did you
> write something different?
>


Ok so I'm expand on why I thought this would work;

- if (!git_config_get("submodule.active")) {
- /*
- * If the submodule being added isn't already covered by the
- * current configured pathspec, set the submodule's active flag
- */
- if (!is_submodule_active(the_repository, add_data->sm_path)) {
+ if (git_config_get("submodule.active") || /* key absent */
+   git_config_get_string_multi("submodule.active", &values)) {
+ /* submodule.active is missing -> force-enable */
+ key = xstrfmt("submodule.%s.active", add_data->sm_name);
+ git_config_set_gently(key, "true");
+ free(key);
+ } else {
+ for (i = 0; i < values->nr; i++) {
+ const char *pat = values->items[i].string;
+ if (!wildmatch(pat, add_data->sm_path, 0)) { /* match found */
+ matched = 1;
+ break;
+ }
+ }
+ if (!matched) { /* no pattern matched -> force-enable */
  key = xstrfmt("submodule.%s.active", add_data->sm_name);
  git_config_set_gently(key, "true");
  free(key);
  }
- } else {
- key = xstrfmt("submodule.%s.active", add_data->sm_name);
- git_config_set_gently(key, "true");
- free(key);
  }
 }

To clarify: My aim with the current patch wasn't to create a
general-purpose helper, but to very precisely control the conditions
under which submodule.<name>.active = true is written during a git
submodule add operation. But I sure did have a thought that something
like this should exist.

The goals I considerat core when adding a new submodules should be
something like
a. The submodule should become active. This is the primary user expectation.
b.The configuration should be as clean and intentional as possible.
Avoid redundant entries if the global submodule.active already covers
the new submodule.
b. The configuration should explicitly reflect the active state if
there's no global policy, or if the global policy.

This was my underlying goal to be very specific.

Your suggestion to use if (!is_submodule_active(the_repository,
add_data->sm_path)) is elegant in its reuse of existing logic.
However, I find these following things

i. Scenario: submodule.active is globally ABSENT.
is_submodule_active() will return true (as submodules are active by
default in this case).

Therefore, !is_submodule_active() will be false.

Result with your suggestion: submodule.<name>.active = true would not
be written. The submodule is active by default, but this isn't
explicitly recorded for the submodule itself. Also a reason why the
test failed after digging a bit deeper this is the possible
explanation I found.
Specifically Test case 9 from t7413

test_expect_success 'is-active, submodule.active and submodule add' '
test_when_finished "rm -rf super2" &&
git init super2 &&
test_commit -C super2 initial &&
git -C super2 config --add submodule.active "sub*" &&

# submodule add should only add submodule.<name>.active
# to the config if not matched by the pathspec
git -C super2 submodule add ../sub sub1 &&
test_must_fail git -C super2 config --get submodule.sub1.active &&

git -C super2 submodule add ../sub mod &&
git -C super2 config --get submodule.mod.active
'

ii. My patch's behavior in this scenario (And not as a general purpose helper)

The first condition git_config_get("submodule.active") ||
git_config_get_string_multi("submodule.active", &values) evaluates to
true because submodule.active is absent.

Result with my patch: submodule.<name>.active = true is explicitly written.

Let me also clarify on what I mean by the _similar_ and _different_
statements I sent above.
The outcome is what I meant was _similar_ the patch link I sent in the
above mail, explicitly adds only when it doesn't find
new submodule to be active, and I verified it with a documented test
added as the tenth test in t7413 where it shouldn't add
an active twice and the outcome is what I found _similar_, the
_difference_ however are in the methods of getting to the outcome,
and adding guardrails. That's the underlying thought with that statement.

To reinforce the above ideas her e aer some points I would like to make:


The logic implemented in the else block (reading submodule.active and
iterating with wildmatch) is indeed a partial re-implementation of
what is_submodule_active might do internally. However:

It's highly localized to this specific decision point in git submodule add.
It's concerned only with the submodule.active pathspecs.
is_submodule_active might have broader considerations (as you hinted,
e.g., submodule.<name>.url's presence, though that's less relevant for
the .active flag itself).
The benefit of precise control over config generation for add might
outweigh the risk of this small, localized logic diverging, especially
since submodule.active's pathspec interpretation is fairly stable.

The core distinction is that is_submodule_active() answers "Is this
submodule considered active right now based on all existing rules and
defaults?". My patch attempts to answer a slightly different question
for git add: "Given the current global submodule.active policy, do we
need to explicitly set submodule.<name>.active = true for this new
submodule to ensure it's active and clearly recorded as such?"
And that is the reason why I had to mention the _different_ and _same_
statement within the same statement.

My approach aims to set submodule.<name>.active = true unless an
existing, valid, and matching global submodule.active pattern already
makes it active. This leads to what I feel is a more robust and
explicit configuration outcome from git submodule add.

Perhaps the ideal long-term solution would involve a more nuanced
helper function (which I'm more than happy to write, if I get the
green flag.), but for the immediate improvement to git add's behavior,
I believe this patch strikes a good balance, prioritizing explicit
activation and config clarity for newly added submodules.



Also to clarify :)
I tried to dig and clarify as much as I can
I maybe wrong in some places cause obviously I'm still relatively much
newer to the source code.

But I thought providing a huge para with what I thought would be
helpful for corrections.

Thank you for reading all the blab : )

- Jayatheerth

[PATCH v8 0/2] Avoid submodule overwritten and skip redundant active entries

[K Jayatheerth]

The first patch i.e. prevent overwriting .gitmodules entry on path reuse
is exactly the same 

The second patch however i.e. 
skip redundant active entries when pattern covers path
has changed logic 
with the helper function being code maintainance because 
of the duplicated logic. I've tried to wrap it as much as 
possible to it's own core need without having to change it
unless the core way of submodules addition and active status
itself changes.

The CI was tested in which 
the only compiler error I found was 
osx-gcc
Which I suppose is already addressed,
apart from this all of the other tests 
ran successfully which includes the 
t7413 (prvious 9) and the new one.


K Jayatheerth (2):
  submodule: prevent overwriting .gitmodules entry on path reuse
  submodule: skip redundant active entries when pattern covers path

 builtin/submodule--helper.c    | 60 +++++++++++++++++++++++++++-------
 t/t7400-submodule-basic.sh     | 23 +++++++++++++
 t/t7413-submodule-is-active.sh | 15 +++++++++
 3 files changed, 87 insertions(+), 11 deletions(-)

-- 
2.49.GIT

[PATCH v8 1/2] submodule: prevent overwriting .gitmodules entry on path reuse

[K Jayatheerth]

Adding a submodule at a path that previously hosted another submodule
(e.g., 'child') reuses the submodule name derived from the path. If the
original submodule was only moved (e.g., to 'child_old') and not renamed,
this silently overwrites its configuration in .gitmodules.

This behavior loses user configuration and causes confusion when the
original submodule is expected to remain intact. It assumes that the
path-derived name is always safe to reuse, even though the name might
still be in use elsewhere in the repository.

Teach `module_add()` to check if the computed submodule name already
exists in the repository's submodule config, and if so, refuse the
operation unless the user explicitly renames or uses force to auto increment.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 builtin/submodule--helper.c | 28 ++++++++++++++++++++++++++++
 t/t7400-submodule-basic.sh  | 23 +++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 53da2116dd..9f6df833f0 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -3444,6 +3444,10 @@ static int module_add(int argc, const char **argv, const char *prefix,
 	struct add_data add_data = ADD_DATA_INIT;
 	const char *ref_storage_format = NULL;
 	char *to_free = NULL;
+	const struct submodule *existing;
+	struct strbuf buf = STRBUF_INIT;
+	int i;
+	char *sm_name_to_free = NULL;
 	struct option options[] = {
 		OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
 			   N_("branch of repository to add as submodule")),
@@ -3546,6 +3550,29 @@ static int module_add(int argc, const char **argv, const char *prefix,
 	if(!add_data.sm_name)
 		add_data.sm_name = add_data.sm_path;
 
+	existing = submodule_from_name(the_repository,
+					null_oid(the_hash_algo),
+					add_data.sm_name);
+
+	if (existing && strcmp(existing->path, add_data.sm_path)) {
+		if (!force) {
+			die(_("submodule name '%s' already used for path '%s'"),
+			add_data.sm_name, existing->path);
+		}
+
+		/* --force: build <name><n> until unique */
+		for (i = 1; ; i++) {
+			strbuf_reset(&buf);
+			strbuf_addf(&buf, "%s%d", add_data.sm_name, i);
+			if (!submodule_from_name(the_repository,
+						null_oid(the_hash_algo),
+						buf.buf)) {
+				break;
+			}
+		}
+
+		add_data.sm_name = sm_name_to_free = strbuf_detach(&buf, NULL);
+	}
 	if (check_submodule_name(add_data.sm_name))
 		die(_("'%s' is not a valid submodule name"), add_data.sm_name);
 
@@ -3561,6 +3588,7 @@ static int module_add(int argc, const char **argv, const char *prefix,
 
 	ret = 0;
 cleanup:
+	free(sm_name_to_free);
 	free(add_data.sm_path);
 	free(to_free);
 	strbuf_release(&sb);
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index d6a501d453..f5514decab 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1482,4 +1482,27 @@ test_expect_success '`submodule init` and `init.templateDir`' '
 	)
 '
 
+test_expect_success 'submodule add fails when name is reused' '
+	git init test-submodule &&
+	(
+		cd test-submodule &&
+		git commit --allow-empty -m init &&
+
+		git init ../child-origin &&
+		git -C ../child-origin commit --allow-empty -m init &&
+
+		git submodule add ../child-origin child &&
+		git commit -m "Add submodule child" &&
+
+		git mv child child_old &&
+		git commit -m "Move child to child_old" &&
+
+		# Now adding a *new* repo at the old name must fail
+		git init ../child2-origin &&
+		git -C ../child2-origin commit --allow-empty -m init &&
+		test_must_fail git submodule add ../child2-origin child
+	)
+'
+
+
 test_done
-- 
2.49.GIT

Re: [PATCH v8 1/2] submodule: prevent overwriting .gitmodules entry on path reuse

[D. Ben Knoble]

On Sat, Jun 7, 2025 at 11:28 PM K Jayatheerth
<jayatheerthkulkarni2005@gmail.com> wrote:
>
> Adding a submodule at a path that previously hosted another submodule
> (e.g., 'child') reuses the submodule name derived from the path. If the
> original submodule was only moved (e.g., to 'child_old') and not renamed,
> this silently overwrites its configuration in .gitmodules.
>
> This behavior loses user configuration and causes confusion when the
> original submodule is expected to remain intact. It assumes that the
> path-derived name is always safe to reuse, even though the name might
> still be in use elsewhere in the repository.
>
> Teach `module_add()` to check if the computed submodule name already
> exists in the repository's submodule config, and if so, refuse the
> operation unless the user explicitly renames or uses force to auto increment.

I had to read the patch to figure out what "auto increment"
meant—perhaps some accompanying docs in `git help submodule`?

>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
>  builtin/submodule--helper.c | 28 ++++++++++++++++++++++++++++
>  t/t7400-submodule-basic.sh  | 23 +++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 53da2116dd..9f6df833f0 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -3444,6 +3444,10 @@ static int module_add(int argc, const char **argv, const char *prefix,
>         struct add_data add_data = ADD_DATA_INIT;
>         const char *ref_storage_format = NULL;
>         char *to_free = NULL;
> +       const struct submodule *existing;
> +       struct strbuf buf = STRBUF_INIT;
> +       int i;
> +       char *sm_name_to_free = NULL;
>         struct option options[] = {
>                 OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
>                            N_("branch of repository to add as submodule")),
> @@ -3546,6 +3550,29 @@ static int module_add(int argc, const char **argv, const char *prefix,
>         if(!add_data.sm_name)
>                 add_data.sm_name = add_data.sm_path;
>
> +       existing = submodule_from_name(the_repository,
> +                                       null_oid(the_hash_algo),
> +                                       add_data.sm_name);
> +
> +       if (existing && strcmp(existing->path, add_data.sm_path)) {
> +               if (!force) {
> +                       die(_("submodule name '%s' already used for path '%s'"),
> +                       add_data.sm_name, existing->path);
> +               }
> +
> +               /* --force: build <name><n> until unique */
> +               for (i = 1; ; i++) {
> +                       strbuf_reset(&buf);
> +                       strbuf_addf(&buf, "%s%d", add_data.sm_name, i);
> +                       if (!submodule_from_name(the_repository,
> +                                               null_oid(the_hash_algo),
> +                                               buf.buf)) {
> +                               break;
> +                       }
> +               }

This isn't typically what I'd expect --force to do, personally, though
in this case it allows me to proceed with an operation that wasn't
allowed otherwise.

Still, I wonder if a user might be confused by "I said 'child' and got
'child2'?"

> +
> +               add_data.sm_name = sm_name_to_free = strbuf_detach(&buf, NULL);
> +       }
>         if (check_submodule_name(add_data.sm_name))
>                 die(_("'%s' is not a valid submodule name"), add_data.sm_name);
>
> @@ -3561,6 +3588,7 @@ static int module_add(int argc, const char **argv, const char *prefix,
>
>         ret = 0;
>  cleanup:
> +       free(sm_name_to_free);
>         free(add_data.sm_path);
>         free(to_free);
>         strbuf_release(&sb);
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index d6a501d453..f5514decab 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -1482,4 +1482,27 @@ test_expect_success '`submodule init` and `init.templateDir`' '
>         )
>  '
>
> +test_expect_success 'submodule add fails when name is reused' '
> +       git init test-submodule &&
> +       (
> +               cd test-submodule &&
> +               git commit --allow-empty -m init &&
> +
> +               git init ../child-origin &&
> +               git -C ../child-origin commit --allow-empty -m init &&
> +
> +               git submodule add ../child-origin child &&
> +               git commit -m "Add submodule child" &&
> +
> +               git mv child child_old &&
> +               git commit -m "Move child to child_old" &&
> +
> +               # Now adding a *new* repo at the old name must fail
> +               git init ../child2-origin &&
> +               git -C ../child2-origin commit --allow-empty -m init &&
> +               test_must_fail git submodule add ../child2-origin child

This makes sense, though I was hoping (when I'd only skimmed the
message and not seen "refuse") that this would be permitted by some
clever trick. Oh well.


> +       )
> +'
> +
> +
>  test_done
> --
> 2.49.GIT
>
>


--
D. Ben Knoble

Re: [PATCH v8 1/2] submodule: prevent overwriting .gitmodules entry on path reuse

[JAYATHEERTH K]

On Wed, Jul 9, 2025 at 8:20 AM D. Ben Knoble <ben.knoble@gmail.com> wrote:
>
> On Sat, Jun 7, 2025 at 11:28 PM K Jayatheerth
> <jayatheerthkulkarni2005@gmail.com> wrote:
> >
> > Adding a submodule at a path that previously hosted another submodule
> > (e.g., 'child') reuses the submodule name derived from the path. If the
> > original submodule was only moved (e.g., to 'child_old') and not renamed,
> > this silently overwrites its configuration in .gitmodules.
> >
> > This behavior loses user configuration and causes confusion when the
> > original submodule is expected to remain intact. It assumes that the
> > path-derived name is always safe to reuse, even though the name might
> > still be in use elsewhere in the repository.
> >
> > Teach `module_add()` to check if the computed submodule name already
> > exists in the repository's submodule config, and if so, refuse the
> > operation unless the user explicitly renames or uses force to auto increment.
>
> I had to read the patch to figure out what "auto increment"
> meant—perhaps some accompanying docs in `git help submodule`?
>
> >
> > Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> > ---
> >  builtin/submodule--helper.c | 28 ++++++++++++++++++++++++++++
> >  t/t7400-submodule-basic.sh  | 23 +++++++++++++++++++++++
> >  2 files changed, 51 insertions(+)
> >
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index 53da2116dd..9f6df833f0 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -3444,6 +3444,10 @@ static int module_add(int argc, const char **argv, const char *prefix,
> >         struct add_data add_data = ADD_DATA_INIT;
> >         const char *ref_storage_format = NULL;
> >         char *to_free = NULL;
> > +       const struct submodule *existing;
> > +       struct strbuf buf = STRBUF_INIT;
> > +       int i;
> > +       char *sm_name_to_free = NULL;
> >         struct option options[] = {
> >                 OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
> >                            N_("branch of repository to add as submodule")),
> > @@ -3546,6 +3550,29 @@ static int module_add(int argc, const char **argv, const char *prefix,
> >         if(!add_data.sm_name)
> >                 add_data.sm_name = add_data.sm_path;
> >
> > +       existing = submodule_from_name(the_repository,
> > +                                       null_oid(the_hash_algo),
> > +                                       add_data.sm_name);
> > +
> > +       if (existing && strcmp(existing->path, add_data.sm_path)) {
> > +               if (!force) {
> > +                       die(_("submodule name '%s' already used for path '%s'"),
> > +                       add_data.sm_name, existing->path);
> > +               }
> > +
> > +               /* --force: build <name><n> until unique */
> > +               for (i = 1; ; i++) {
> > +                       strbuf_reset(&buf);
> > +                       strbuf_addf(&buf, "%s%d", add_data.sm_name, i);
> > +                       if (!submodule_from_name(the_repository,
> > +                                               null_oid(the_hash_algo),
> > +                                               buf.buf)) {
> > +                               break;
> > +                       }
> > +               }
>
> This isn't typically what I'd expect --force to do, personally, though
> in this case it allows me to proceed with an operation that wasn't
> allowed otherwise.
>
> Still, I wonder if a user might be confused by "I said 'child' and got
> 'child2'?"
>


Ok so while fixing the previous versions of my submissions
I got stumped at this, I found child<incremented val> to be intuitive
at that time
but I can see why it may not be intuitive too, I mean I could just
remove the previous child and add
the current data as the new child because that feels intuitive for force.
If that is something which is in the interest I could send the new
patches as soon as possible.

Thank You

- Jayatheerth

Re: [PATCH v8 1/2] submodule: prevent overwriting .gitmodules entry on path reuse

[Ben Knoble]

> Le 20 juil. 2025 à 08:25, JAYATHEERTH K <jayatheerthkulkarni2005@gmail.com> a écrit :
> 
> On Wed, Jul 9, 2025 at 8:20 AM D. Ben Knoble <ben.knoble@gmail.com> wrote:
>> 
>>> On Sat, Jun 7, 2025 at 11:28 PM K Jayatheerth
>>> <jayatheerthkulkarni2005@gmail.com> wrote:
>>> 
>>> Adding a submodule at a path that previously hosted another submodule
>>> (e.g., 'child') reuses the submodule name derived from the path. If the
>>> original submodule was only moved (e.g., to 'child_old') and not renamed,
>>> this silently overwrites its configuration in .gitmodules.
>>> 
>>> This behavior loses user configuration and causes confusion when the
>>> original submodule is expected to remain intact. It assumes that the
>>> path-derived name is always safe to reuse, even though the name might
>>> still be in use elsewhere in the repository.
>>> 
>>> Teach `module_add()` to check if the computed submodule name already
>>> exists in the repository's submodule config, and if so, refuse the
>>> operation unless the user explicitly renames or uses force to auto increment.
>> 
>> I had to read the patch to figure out what "auto increment"
>> meant—perhaps some accompanying docs in `git help submodule`?
>> 
>>> 
>>> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
>>> ---
>>> builtin/submodule--helper.c | 28 ++++++++++++++++++++++++++++
>>> t/t7400-submodule-basic.sh  | 23 +++++++++++++++++++++++
>>> 2 files changed, 51 insertions(+)
>>> 
>>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>>> index 53da2116dd..9f6df833f0 100644
>>> --- a/builtin/submodule--helper.c
>>> +++ b/builtin/submodule--helper.c
>>> @@ -3444,6 +3444,10 @@ static int module_add(int argc, const char **argv, const char *prefix,
>>>        struct add_data add_data = ADD_DATA_INIT;
>>>        const char *ref_storage_format = NULL;
>>>        char *to_free = NULL;
>>> +       const struct submodule *existing;
>>> +       struct strbuf buf = STRBUF_INIT;
>>> +       int i;
>>> +       char *sm_name_to_free = NULL;
>>>        struct option options[] = {
>>>                OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
>>>                           N_("branch of repository to add as submodule")),
>>> @@ -3546,6 +3550,29 @@ static int module_add(int argc, const char **argv, const char *prefix,
>>>        if(!add_data.sm_name)
>>>                add_data.sm_name = add_data.sm_path;
>>> 
>>> +       existing = submodule_from_name(the_repository,
>>> +                                       null_oid(the_hash_algo),
>>> +                                       add_data.sm_name);
>>> +
>>> +       if (existing && strcmp(existing->path, add_data.sm_path)) {
>>> +               if (!force) {
>>> +                       die(_("submodule name '%s' already used for path '%s'"),
>>> +                       add_data.sm_name, existing->path);
>>> +               }
>>> +
>>> +               /* --force: build <name><n> until unique */
>>> +               for (i = 1; ; i++) {
>>> +                       strbuf_reset(&buf);
>>> +                       strbuf_addf(&buf, "%s%d", add_data.sm_name, i);
>>> +                       if (!submodule_from_name(the_repository,
>>> +                                               null_oid(the_hash_algo),
>>> +                                               buf.buf)) {
>>> +                               break;
>>> +                       }
>>> +               }
>> 
>> This isn't typically what I'd expect --force to do, personally, though
>> in this case it allows me to proceed with an operation that wasn't
>> allowed otherwise.
>> 
>> Still, I wonder if a user might be confused by "I said 'child' and got
>> 'child2'?"
>> 
> 
> 
> Ok so while fixing the previous versions of my submissions
> I got stumped at this, I found child<incremented val> to be intuitive
> at that time
> but I can see why it may not be intuitive too, I mean I could just
> remove the previous child and add
> the current data as the new child because that feels intuitive for force.
> If that is something which is in the interest I could send the new
> patches as soon as possible.

That sounds more like a typical forceful operation to me.

[PATCH v8 2/2] submodule: skip redundant active entries when pattern covers path

[K Jayatheerth]

configure_added_submodule always writes an explicit submodule.<name>.active
entry, even when the new path is already matched by submodule.active
patterns. This leads to unnecessary and cluttered configuration.

change the logic to centralize wildmatch-based pattern lookup,
in configure_added_submodule. Wrap the active-entry write in a conditional
that only fires when that helper reports no existing pattern covers the
submodule’s path.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 builtin/submodule--helper.c    | 32 +++++++++++++++++++++-----------
 t/t7413-submodule-is-active.sh | 15 +++++++++++++++
 2 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9f6df833f0..514abe480e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -32,6 +32,8 @@
 #include "advice.h"
 #include "branch.h"
 #include "list-objects-filter-options.h"
+#include "wildmatch.h"
+#include "strbuf.h"
 
 #define OPT_QUIET (1 << 0)
 #define OPT_CACHED (1 << 1)
@@ -3328,6 +3330,9 @@ static void configure_added_submodule(struct add_data *add_data)
 	char *key;
 	struct child_process add_submod = CHILD_PROCESS_INIT;
 	struct child_process add_gitmodules = CHILD_PROCESS_INIT;
+	const struct string_list *values;
+	size_t i;
+	int matched = 0;
 
 	key = xstrfmt("submodule.%s.url", add_data->sm_name);
 	git_config_set_gently(key, add_data->realrepo);
@@ -3370,20 +3375,25 @@ static void configure_added_submodule(struct add_data *add_data)
 	 * is_submodule_active(), since that function needs to find
 	 * out the value of "submodule.active" again anyway.
 	 */
-	if (!git_config_get("submodule.active")) {
-		/*
-		 * If the submodule being added isn't already covered by the
-		 * current configured pathspec, set the submodule's active flag
-		 */
-		if (!is_submodule_active(the_repository, add_data->sm_path)) {
-			key = xstrfmt("submodule.%s.active", add_data->sm_name);
-			git_config_set_gently(key, "true");
-			free(key);
-		}
-	} else {
+	if (git_config_get("submodule.active") || /* key absent */
+	    git_config_get_string_multi("submodule.active", &values)) {
+		/* submodule.active is missing -> force-enable */
 		key = xstrfmt("submodule.%s.active", add_data->sm_name);
 		git_config_set_gently(key, "true");
 		free(key);
+	} else {
+		for (i = 0; i < values->nr; i++) {
+			const char *pat = values->items[i].string;
+			if (!wildmatch(pat, add_data->sm_path, 0)) { /* match found */
+				matched = 1;
+				break;
+			}
+		}
+		if (!matched) { /* no pattern matched -> force-enable */
+ 			key = xstrfmt("submodule.%s.active", add_data->sm_name);
+ 			git_config_set_gently(key, "true");
+ 			free(key);
+ 		}
 	}
 }
 
diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh
index 9509dc18fd..a42060cac9 100755
--- a/t/t7413-submodule-is-active.sh
+++ b/t/t7413-submodule-is-active.sh
@@ -124,4 +124,19 @@ test_expect_success 'is-active, submodule.active and submodule add' '
 	git -C super2 config --get submodule.mod.active
 '
 
+test_expect_success 'submodule add skips redundant active entry' '
+	git init repo &&
+	(
+		cd repo &&
+		git config submodule.active "lib/*" &&
+		git commit --allow-empty -m init &&
+
+		git init ../lib-origin &&
+		git -C ../lib-origin commit --allow-empty -m init &&
+
+		git submodule add ../lib-origin lib/foo &&
+		! git config --get submodule.lib/foo.active
+	)
+'
+
 test_done
-- 
2.49.GIT

Re: [PATCH v8 2/2] submodule: skip redundant active entries when pattern covers path

[D. Ben Knoble]

On Sat, Jun 7, 2025 at 11:28 PM K Jayatheerth
<jayatheerthkulkarni2005@gmail.com> wrote:
>
> configure_added_submodule always writes an explicit submodule.<name>.active
> entry, even when the new path is already matched by submodule.active
> patterns. This leads to unnecessary and cluttered configuration.
>
> change the logic to centralize wildmatch-based pattern lookup,
> in configure_added_submodule. Wrap the active-entry write in a conditional
> that only fires when that helper reports no existing pattern covers the
> submodule’s path.

I use submodules, but am not too familiar with their internals, so I
find it hard to follow the details here. Perhaps some examples showing
when the configuration becomes cluttered and what about it is
cluttered would help?

When I check a repo of mine that has ~50 submodules:

    git config get submodule.active
    .
    git config get --show-names --regexp 'submodule.*active'
    submodule.active .

So I'm not seeing this in practice, though I didn't try with the test
case in the patch.

[Nit: s/change/Change]

>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
>  builtin/submodule--helper.c    | 32 +++++++++++++++++++++-----------
>  t/t7413-submodule-is-active.sh | 15 +++++++++++++++
>  2 files changed, 36 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 9f6df833f0..514abe480e 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -32,6 +32,8 @@
>  #include "advice.h"
>  #include "branch.h"
>  #include "list-objects-filter-options.h"
> +#include "wildmatch.h"
> +#include "strbuf.h"
>
>  #define OPT_QUIET (1 << 0)
>  #define OPT_CACHED (1 << 1)
> @@ -3328,6 +3330,9 @@ static void configure_added_submodule(struct add_data *add_data)
>         char *key;
>         struct child_process add_submod = CHILD_PROCESS_INIT;
>         struct child_process add_gitmodules = CHILD_PROCESS_INIT;
> +       const struct string_list *values;
> +       size_t i;
> +       int matched = 0;
>
>         key = xstrfmt("submodule.%s.url", add_data->sm_name);
>         git_config_set_gently(key, add_data->realrepo);
> @@ -3370,20 +3375,25 @@ static void configure_added_submodule(struct add_data *add_data)
>          * is_submodule_active(), since that function needs to find
>          * out the value of "submodule.active" again anyway.
>          */
> -       if (!git_config_get("submodule.active")) {
> -               /*
> -                * If the submodule being added isn't already covered by the
> -                * current configured pathspec, set the submodule's active flag
> -                */

Do we want to lose this comment? The replacement below ("… ->
force-enable") is a bit terse for me and rather loses some
information.

> -               if (!is_submodule_active(the_repository, add_data->sm_path)) {
> -                       key = xstrfmt("submodule.%s.active", add_data->sm_name);
> -                       git_config_set_gently(key, "true");
> -                       free(key);
> -               }
> -       } else {
> +       if (git_config_get("submodule.active") || /* key absent */
> +           git_config_get_string_multi("submodule.active", &values)) {
> +               /* submodule.active is missing -> force-enable */
>                 key = xstrfmt("submodule.%s.active", add_data->sm_name);
>                 git_config_set_gently(key, "true");
>                 free(key);
> +       } else {
> +               for (i = 0; i < values->nr; i++) {
> +                       const char *pat = values->items[i].string;
> +                       if (!wildmatch(pat, add_data->sm_path, 0)) { /* match found */
> +                               matched = 1;
> +                               break;
> +                       }
> +               }
> +               if (!matched) { /* no pattern matched -> force-enable */
> +                       key = xstrfmt("submodule.%s.active", add_data->sm_name);
> +                       git_config_set_gently(key, "true");
> +                       free(key);
> +               }
>         }
>  }
>
> diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh
> index 9509dc18fd..a42060cac9 100755
> --- a/t/t7413-submodule-is-active.sh
> +++ b/t/t7413-submodule-is-active.sh
> @@ -124,4 +124,19 @@ test_expect_success 'is-active, submodule.active and submodule add' '
>         git -C super2 config --get submodule.mod.active
>  '
>
> +test_expect_success 'submodule add skips redundant active entry' '
> +       git init repo &&
> +       (
> +               cd repo &&
> +               git config submodule.active "lib/*" &&
> +               git commit --allow-empty -m init &&
> +
> +               git init ../lib-origin &&
> +               git -C ../lib-origin commit --allow-empty -m init &&
> +
> +               git submodule add ../lib-origin lib/foo &&
> +               ! git config --get submodule.lib/foo.active

(Not my area of expertise) Should this be test_must_fail?

> +       )
> +'
> +
>  test_done
> --
> 2.49.GIT
>
>


-- 
D. Ben Knoble

Re: [PATCH v8 2/2] submodule: skip redundant active entries when pattern covers path

[Junio C Hamano]

"D. Ben Knoble" <ben.knoble@gmail.com> writes:

>> +test_expect_success 'submodule add skips redundant active entry' '
>> +       git init repo &&
>> +       (
>> +               cd repo &&
>> +               git config submodule.active "lib/*" &&
>> +               git commit --allow-empty -m init &&
>> +
>> +               git init ../lib-origin &&
>> +               git -C ../lib-origin commit --allow-empty -m init &&
>> +
>> +               git submodule add ../lib-origin lib/foo &&
>> +               ! git config --get submodule.lib/foo.active
>
> (Not my area of expertise) Should this be test_must_fail?

That is certainly better.  "! git config ..." would succeed even
when that command segfaults and dumps core, which we may want to
notice.

Re: [PATCH v8 2/2] submodule: skip redundant active entries when pattern covers path

[Junio C Hamano]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

> +		if (!matched) { /* no pattern matched -> force-enable */
> + 			key = xstrfmt("submodule.%s.active", add_data->sm_name);
> + 			git_config_set_gently(key, "true");
> + 			free(key);
> + 		}

Somehow these lines begin with SP and then HT.  If you are going to
send an updated version, please make sure to fix the whitespace
issues around here.

In the meantime, I'll tweak the version I have in 'seen'.

Thanks.

Re: [PATCH v8 2/2] submodule: skip redundant active entries when pattern covers path

[JAYATHEERTH K]

On Thu, Jul 17, 2025 at 10:28 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
>
> > +             if (!matched) { /* no pattern matched -> force-enable */
> > +                     key = xstrfmt("submodule.%s.active", add_data->sm_name);
> > +                     git_config_set_gently(key, "true");
> > +                     free(key);
> > +             }
>
> Somehow these lines begin with SP and then HT.  If you are going to
> send an updated version, please make sure to fix the whitespace
> issues around here.
>
> In the meantime, I'll tweak the version I have in 'seen'.
>
> Thanks.


Damn, I missed the previous emails,
Actually my fault it is, I made some tweaks to my email
But anyways
I will go through and send an updated version

Thank you

- Jayatheerth

Re: [PATCH v8 0/2] Avoid submodule overwritten and skip redundant active entries

[Junio C Hamano]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

> The first patch i.e. prevent overwriting .gitmodules entry on path reuse
> is exactly the same 
>
> The second patch however i.e. 
> skip redundant active entries when pattern covers path
> has changed logic 
> with the helper function being code maintainance because 
> of the duplicated logic. I've tried to wrap it as much as 
> possible to it's own core need without having to change it
> unless the core way of submodules addition and active status
> itself changes.
>
> The CI was tested in which 
> the only compiler error I found was 
> osx-gcc
> Which I suppose is already addressed,
> apart from this all of the other tests 
> ran successfully which includes the 
> t7413 (prvious 9) and the new one.
>
>
> K Jayatheerth (2):
>   submodule: prevent overwriting .gitmodules entry on path reuse
>   submodule: skip redundant active entries when pattern covers path

Haven't seen any comment from others on this topic for quite a
while.  How does this one look to those who do use submodules (I am
not one of them)?

Thanks.