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

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

[K Jayatheerth]

Changes:

1. Readded the comment so the patch now shows no added lines at the comments it's 
just the previous one (As clean as I could do it).            
/*
 * If the submodule being added isn't already covered by the
 * current configured pathspec, set the submodule's active flag
*/

2. Instead of ! git config I now use test_must_fail

3. For --force used the same logic the incremental logic.

4. I also updated the docs for --force.

5. Removed the white spaces from the lines.

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

 Documentation/git-submodule.adoc |  7 +++++
 builtin/submodule--helper.c      | 52 ++++++++++++++++++++++++++++----
 t/t7400-submodule-basic.sh       | 22 ++++++++++++++
 t/t7413-submodule-is-active.sh   | 15 +++++++++
 4 files changed, 90 insertions(+), 6 deletions(-)

-- 
2.50.GIT

[PATCH 1/2] submodule: prevent overwriting .gitmodules 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 the submodule or uses the --force option,
which will automatically generate a unique name by
appending a number (e.g., child1).

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 Documentation/git-submodule.adoc |  7 +++++++
 builtin/submodule--helper.c      | 27 +++++++++++++++++++++++++++
 t/t7400-submodule-basic.sh       | 22 ++++++++++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/Documentation/git-submodule.adoc b/Documentation/git-submodule.adoc
index 87d8e0f0c5..503c84a200 100644
--- a/Documentation/git-submodule.adoc
+++ b/Documentation/git-submodule.adoc
@@ -307,6 +307,13 @@ OPTIONS
 --force::
 	This option is only valid for add, deinit and update commands.
 	When running add, allow adding an otherwise ignored submodule path.
+	This option is also used to bypass a check that the submodule's name
+	is not already in use. By default, 'git submodule add' will fail if
+	the proposed name (which is derived from the path) is already registered
+	for another submodule in the repository. Using '--force' allows the command
+	to proceed by automatically generating a unique name by appending a number
+	to the conflicting name (e.g., if a submodule named 'child' exists, it will
+	try 'child1', and so on).
 	When running deinit the submodule working trees will be removed even
 	if they contain local changes.
 	When running update (only effective with the checkout procedure),
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d8a6fa47e5..b4f5d6e26a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -3423,6 +3423,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")),
@@ -3525,6 +3529,28 @@ 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);
 
@@ -3540,6 +3566,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..6812df3081 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1482,4 +1482,26 @@ 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.50.GIT

[PATCH 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    | 25 +++++++++++++++++++------
 t/t7413-submodule-is-active.sh | 15 +++++++++++++++
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b4f5d6e26a..1fb49a2c4c 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)
@@ -3308,6 +3310,9 @@ static void configure_added_submodule(struct add_data *add_data)
 	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);
 	free(key);
@@ -3349,20 +3354,28 @@ 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 (git_config_get("submodule.active") || /* key absent */
+	    git_config_get_string_multi("submodule.active", &values)) {
 		/*
 		 * 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 {
+		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);
 	}
 }
 
diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh
index 9509dc18fd..6fd3b870de 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 &&
+		test_must_fail git config --get submodule.lib/foo.active
+	)
+'
+
 test_done
-- 
2.50.GIT

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

[Junio C Hamano]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

> 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 the submodule or uses the --force option,
> which will automatically generate a unique name by
> appending a number (e.g., child1).
>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---

Very well described.

> +	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);

I'll locally fix this funny indentation; not a reason to require an
update.

> +		}
> +		/* --force: build <name><n> until unique */
> +		for (i = 1; ; i++) {

I think you can narrow the scope of "i" to this loop alone.  I'll
locally do so (and if anything breaks, which I doubt); not a reason
to require an update.

> + ...
> +		# 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

Shouldn't we also check what this failed command tell the end-user?  E.g.

	test_must_fail git submodule add ../child2-origin child	2>err &&
	test_grep "alreayd used for" err

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

[Junio C Hamano]

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:

> @@ -3308,6 +3310,9 @@ static void configure_added_submodule(struct add_data *add_data)
>  	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);
>  	free(key);

The blank line should be between the end of block of decls
(i.e. "int matched = 0") and the first statement (i.e. "key =
xstrfmt(...)"), not there.  You probably do not need "i" in such a
wide scope; just use

	for (size_t i = 0; i < values->nr; i++)

in the only loop that uses it.

> @@ -3349,20 +3354,28 @@ 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 (git_config_get("submodule.active") || /* key absent */
> +	    git_config_get_string_multi("submodule.active", &values)) {

Hmph, do we need two calls here, or would a single call to
get_string_multi() sufficient to learn what we want here?  When
there is no such key, the function may fail (or succeed and leave
values->nr == 0), and either way, we can tell that there is no such
key, right?

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

[K Jayatheerth]

Changes:
1. Test now has an explicit error message in patch 1 
2. Nice catch on the double function calls those were not needed 
   a single function call to git_config_get_string_multi did get job done.
3. While I am at it also fixed some indentation issues (Only hoping i didn't create any unknown ones)
and localized the loop variables.

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

 Documentation/git-submodule.adoc |  7 +++++
 builtin/submodule--helper.c      | 54 ++++++++++++++++++++++++++------
 t/t7400-submodule-basic.sh       | 22 +++++++++++++
 t/t7413-submodule-is-active.sh   | 15 +++++++++
 4 files changed, 88 insertions(+), 10 deletions(-)

-- 
2.50.GIT

[PATCH 1/2] submodule: prevent overwriting .gitmodules 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 the submodule or uses the --force option,
which will automatically generate a unique name by
appending a number (e.g., child1).

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 Documentation/git-submodule.adoc |  7 +++++++
 builtin/submodule--helper.c      | 26 ++++++++++++++++++++++++++
 t/t7400-submodule-basic.sh       | 22 ++++++++++++++++++++++
 3 files changed, 55 insertions(+)

diff --git a/Documentation/git-submodule.adoc b/Documentation/git-submodule.adoc
index 87d8e0f0c5..503c84a200 100644
--- a/Documentation/git-submodule.adoc
+++ b/Documentation/git-submodule.adoc
@@ -307,6 +307,13 @@ OPTIONS
 --force::
 	This option is only valid for add, deinit and update commands.
 	When running add, allow adding an otherwise ignored submodule path.
+	This option is also used to bypass a check that the submodule's name
+	is not already in use. By default, 'git submodule add' will fail if
+	the proposed name (which is derived from the path) is already registered
+	for another submodule in the repository. Using '--force' allows the command
+	to proceed by automatically generating a unique name by appending a number
+	to the conflicting name (e.g., if a submodule named 'child' exists, it will
+	try 'child1', and so on).
 	When running deinit the submodule working trees will be removed even
 	if they contain local changes.
 	When running update (only effective with the checkout procedure),
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d8a6fa47e5..9406e732c4 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -3423,6 +3423,9 @@ 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;
+	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")),
@@ -3525,6 +3528,28 @@ 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 (int 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);
 
@@ -3540,6 +3565,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..0743ccdfe2 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1482,4 +1482,26 @@ 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" &&
+
+		git init ../child2-origin &&
+		git -C ../child2-origin commit --allow-empty -m init &&
+		test_must_fail git submodule add ../child2-origin child 2>err &&
+		test_grep "submodule name '\''child'\'' already used for path '\''child_old'\''" err
+	)
+'
+
 test_done
-- 
2.50.GIT

[PATCH 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    | 28 ++++++++++++++++++----------
 t/t7413-submodule-is-active.sh | 15 +++++++++++++++
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9406e732c4..d4c4d9b0e1 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)
@@ -3307,7 +3309,8 @@ 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;
+	int matched = 0;
 	key = xstrfmt("submodule.%s.url", add_data->sm_name);
 	git_config_set_gently(key, add_data->realrepo);
 	free(key);
@@ -3349,17 +3352,22 @@ 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 (!git_config_get_string_multi("submodule.active", &values)) {
+		/* The key exists and we have its values. Check for a match. */
+		for (size_t i = 0; i < values->nr; i++) {
+			const char *pat = values->items[i].string;
+			if (!wildmatch(pat, add_data->sm_path, 0)) {
+				matched = 1;
+				break;
+			}
+		}
+	}
+
+	if (!matched) {
 		/*
-		 * If the submodule being added isn't already covered by the
-		 * current configured pathspec, set the submodule's active flag
+		 * No pattern matched (or no 'submodule.active' patterns
+		 * were configured at all), so explicitly activate.
 		 */
-		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 {
 		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..6fd3b870de 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 &&
+		test_must_fail git config --get submodule.lib/foo.active
+	)
+'
+
 test_done
-- 
2.50.GIT