threads / patch / 63433

patchsubmodule: prevent overwriting .gitmodules entry on path reuse

Subject: [PATCH] submodule: prevent overwriting .gitmodules entry on path reuse

## tl;dr

24 messages between May 10, 2025 and May 24, 2025. Diffs are folded; open one to read it.

replies: 23people: 2as markdown or json

K Jayatheerth· May 10, 2025, 05:45 UTC · lore

When a submodule is added at a path that previously hosted another submodule (e.g., 'child'), Git reuses the submodule name derived from the path and updates the corresponding entry in .gitmodules. This can silently overwrite existing configuration if the old submodule was only moved (e.g., to 'child_old') without renaming the submodule.

This patch improves the `module_add()` logic by checking whether the submodule name already exists in the config but maps to a different path. In such a case, Git now errors out unless `--force` is specified, thus preventing accidental overwrites. To proceed safely, the user can provide a new name via `--name` or use `--force`.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 builtin/submodule--helper.c | 45 ++++++++++++++++++++++++++++---------
 t/t7400-submodule-basic.sh  | 23 +++++++++++++++++++
 2 files changed, 57 insertions(+), 11 deletions(-)
Show changes to 2 files +57 −11

builtin/submodule--helper.c, t/t7400-submodule-basic.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 53da2116dd..0f98ef122b 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"
+
 
 #define OPT_QUIET (1 << 0)
 #define OPT_CACHED (1 << 1)
@@ -3323,6 +3325,23 @@ 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 +3389,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);
@@ -3443,6 +3452,7 @@ static int module_add(int argc, const char **argv, const char *prefix,
 	int force = 0, quiet = 0, progress = 0, dissociate = 0;
 	struct add_data add_data = ADD_DATA_INIT;
 	const char *ref_storage_format = NULL;
+	const struct submodule *existing;
 	char *to_free = NULL;
 	struct option options[] = {
 		OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
@@ -3546,6 +3556,19 @@ 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' "
+			"(use --name to choose another or --force to overwrite)"),
+			add_data.sm_name, existing->path);
+		}
+		add_data.sm_name = xstrfmt("%s.%s", add_data.sm_name, basename(add_data.sm_path));
+	}
+
 	if (check_submodule_name(add_data.sm_name))
 		die(_("'%s' is not a valid submodule name"), add_data.sm_name);
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index d6a501d453..5c3f471338 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 "initial commit" &&
+
+		git init ../child-origin &&
+		git -C ../child-origin commit --allow-empty -m "initial commit" &&
+
+		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" &&
+
+		# Create another submodule repo
+		git init ../child2-origin &&
+		git -C ../child2-origin commit --allow-empty -m "initial commit" &&
+
+		test_must_fail git submodule add ../child2-origin child
+	)
+'
+
 test_done
-- 
2.49.0.533.g80f4e02b4b
JAYATHEERTH K· May 10, 2025, 05:57 UTC · re: K Jayatheerth · lore

Re: [PATCH] submodule: prevent overwriting .gitmodules entry on path reuse

Show 32 quoted lines
> +++ b/builtin/submodule--helper.c
> @@ -32,6 +32,8 @@
>  #include "advice.h"
>  #include "branch.h"
>  #include "list-objects-filter-options.h"
> +#include "wildmatch.h"
> +
>
>  #define OPT_QUIET (1 << 0)
>  #define OPT_CACHED (1 << 1)
> @@ -3323,6 +3325,23 @@ 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;
> +}
> +
>

I added this change in the same patch because t7413 was throwing errors at these specific lines of code I don't really have full understanding of these files yet but after this t7413 perfectly passed all the tests

I tried these changed because there was a comment about this in the file
/*
* NEEDSWORK: In a multi-working-tree world this needs to be
* set in the per-worktree config.
*/
/*
* NEEDSWORK: In the longer run, we need to get rid of this
* pattern of querying "submodule.active" before calling
* is_submodule_active(), since that function needs to find
* out the value of "submodule.active" again anyway.
*/

I can separate it into different patches if there is a requirement. Or change based on the feedback.

-Jayatheerth
Junio C Hamano· May 12, 2025, 12:32 UTC · re: K Jayatheerth · lore

Re: [PATCH] submodule: prevent overwriting .gitmodules entry on path reuse

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
Show 8 quoted lines
> When a submodule is added at a path that previously hosted another submodule
> (e.g., 'child'), Git reuses the submodule name derived from the path and
> updates the corresponding entry in .gitmodules. This can silently overwrite
> existing configuration if the old submodule was only moved (e.g., to
> 'child_old') without renaming the submodule.
>
> This patch improves the `module_add()` logic by checking whether the
> submodule name already exists in the config but maps to a different path.

Quite sensible description of the problem and the proposed course of improvement.

I do not think `--force` that allows the same name to be reused a good idea at all, though. We shouldn't encourage its use to resolve such a case. If what used to be called `child` now sits elsewhere, perhaps because the tree structure was reorganized due to mass renaming, but if it still is being used in the project, there is no good reason to nuke the configuration recorded for that existing module.

The module name used in .git/config is purely local so the user should just give a new one a name that does not conflict, or even better yet, perhaps the tool should pick a unique and nonconflicting name automatically, no?

> In such a case, Git now errors out unless `--force` is specified, thus
> preventing accidental overwrites. To proceed safely, the user can provide
> a new name via `--name` or use `--force`.
Show 123 quoted lines
>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
>  builtin/submodule--helper.c | 45 ++++++++++++++++++++++++++++---------
>  t/t7400-submodule-basic.sh  | 23 +++++++++++++++++++
>  2 files changed, 57 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 53da2116dd..0f98ef122b 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"
> +
>  
>  #define OPT_QUIET (1 << 0)
>  #define OPT_CACHED (1 << 1)
> @@ -3323,6 +3325,23 @@ 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 +3389,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);
> @@ -3443,6 +3452,7 @@ static int module_add(int argc, const char **argv, const char *prefix,
>  	int force = 0, quiet = 0, progress = 0, dissociate = 0;
>  	struct add_data add_data = ADD_DATA_INIT;
>  	const char *ref_storage_format = NULL;
> +	const struct submodule *existing;
>  	char *to_free = NULL;
>  	struct option options[] = {
>  		OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
> @@ -3546,6 +3556,19 @@ 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' "
> +			"(use --name to choose another or --force to overwrite)"),
> +			add_data.sm_name, existing->path);
> +		}
> +		add_data.sm_name = xstrfmt("%s.%s", add_data.sm_name, basename(add_data.sm_path));
> +	}
> +
>  	if (check_submodule_name(add_data.sm_name))
>  		die(_("'%s' is not a valid submodule name"), add_data.sm_name);
>  
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index d6a501d453..5c3f471338 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 "initial commit" &&
> +
> +		git init ../child-origin &&
> +		git -C ../child-origin commit --allow-empty -m "initial commit" &&
> +
> +		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" &&
> +
> +		# Create another submodule repo
> +		git init ../child2-origin &&
> +		git -C ../child2-origin commit --allow-empty -m "initial commit" &&
> +
> +		test_must_fail git submodule add ../child2-origin child
> +	)
> +'
> +
>  test_done
JAYATHEERTH K· May 12, 2025, 13:26 UTC · re: Junio C Hamano · lore

Re: [PATCH] submodule: prevent overwriting .gitmodules entry on path reuse

On Mon, May 12, 2025 at 6:02 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 28 quoted lines
>
> K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
>
> > When a submodule is added at a path that previously hosted another submodule
> > (e.g., 'child'), Git reuses the submodule name derived from the path and
> > updates the corresponding entry in .gitmodules. This can silently overwrite
> > existing configuration if the old submodule was only moved (e.g., to
> > 'child_old') without renaming the submodule.
> >
> > This patch improves the `module_add()` logic by checking whether the
> > submodule name already exists in the config but maps to a different path.
>
> Quite sensible description of the problem and the proposed course of
> improvement.
>
> I do not think `--force` that allows the same name to be reused a
> good idea at all, though.  We shouldn't encourage its use to resolve
> such a case.  If what used to be called `child` now sits elsewhere,
> perhaps because the tree structure was reorganized due to mass
> renaming, but if it still is being used in the project, there is no
> good reason to nuke the configuration recorded for that existing
> module.
>
> The module name used in .git/config is purely local so the user
> should just give a new one a name that does not conflict, or even
> better yet, perhaps the tool should pick a unique and nonconflicting
> name automatically, no?
>

That makes sense I see the point in using --force to resolve name conflicts might not be ideal, especially when the previous submodule config may still be valid and in use.

As a potential improvement, I was thinking of mimicking how duplicate filenames are handled: if the default submodule name (derived from the path) already exists and maps to a different path, we could automatically append an incrementing number (foo, foo1, foo2, etc.) until a non-conflicting name is found.

-Jayatheerth
K Jayatheerth· May 13, 2025, 03:34 UTC · re: JAYATHEERTH K · lore

[PATCH v2] submodule: prevent overwriting .gitmodules entry on path reuse

When a submodule is added at a path that previously hosted another submodule (e.g., 'child'), Git reuses the submodule name derived from the path and updates the corresponding entry in .gitmodules. This can silently overwrite existing configuration if the old submodule was only moved (e.g., to 'child_old') without renaming the submodule.

This patch improves the `module_add()` logic by checking whether the submodule name already exists in the config but maps to a different path. In such a case, Git now errors out unless `--force` is specified, thus preventing accidental overwrites. To proceed safely, the user can provide a new name via `--name` or use `--force`.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 builtin/submodule--helper.c | 58 ++++++++++++++++++++++++++++++-------
 t/t7400-submodule-basic.sh  | 23 +++++++++++++++
 2 files changed, 70 insertions(+), 11 deletions(-)
Show changes to 2 files +70 −11

builtin/submodule--helper.c, t/t7400-submodule-basic.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 53da2116dd..e70aa584f1 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,23 @@ 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 +3389,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);
@@ -3443,6 +3452,7 @@ static int module_add(int argc, const char **argv, const char *prefix,
 	int force = 0, quiet = 0, progress = 0, dissociate = 0;
 	struct add_data add_data = ADD_DATA_INIT;
 	const char *ref_storage_format = NULL;
+	const struct submodule *existing;
 	char *to_free = NULL;
 	struct option options[] = {
 		OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
@@ -3546,6 +3556,32 @@ 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 */
+		struct strbuf buf = STRBUF_INIT;
+		strbuf_addstr(&buf, add_data.sm_name);
+
+		for (int i = 1; ; i++) {
+			strbuf_setlen(&buf, 0);
+			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 = strbuf_detach(&buf, NULL);
+	}
+
 	if (check_submodule_name(add_data.sm_name))
 		die(_("'%s' is not a valid submodule name"), add_data.sm_name);
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index d6a501d453..5c3f471338 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 "initial commit" &&
+
+		git init ../child-origin &&
+		git -C ../child-origin commit --allow-empty -m "initial commit" &&
+
+		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" &&
+
+		# Create another submodule repo
+		git init ../child2-origin &&
+		git -C ../child2-origin commit --allow-empty -m "initial commit" &&
+
+		test_must_fail git submodule add ../child2-origin child
+	)
+'
+
 test_done
-- 
2.49.GIT
Junio C Hamano· May 13, 2025, 21:44 UTC · re: K Jayatheerth · lore

Re: [PATCH v2] submodule: prevent overwriting .gitmodules entry on path reuse

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
Show 5 quoted lines
> When a submodule is added at a path that previously hosted another submodule
> (e.g., 'child'), Git reuses the submodule name derived from the path and
> updates the corresponding entry in .gitmodules. This can silently overwrite
> existing configuration if the old submodule was only moved (e.g., to
> 'child_old') without renaming the submodule.
OK.
> This patch improves the `module_add()` logic by checking whether the
> submodule name already exists in the config but maps to a different path.

We frown upon a patch that says "This patch does X"; just give an order to the codebase to "be like so". I.e. "Improve the module-add by doing X..." is how we phrase a proposed change.

> In such a case, Git now errors out unless `--force` is specified, thus
> preventing accidental overwrites. To proceed safely, the user can provide
> a new name via `--name` or use `--force`.
The above explains what happens in module_add() quite well.

What is puzzling about this change is that the new helper function and changes to configure_added_submodule() is not described at all in the proposed log message. How are they relevant and why do we need them?

Show 17 quoted lines
> @@ -3443,6 +3452,7 @@ static int module_add(int argc, const char **argv, const char *prefix,
>  	int force = 0, quiet = 0, progress = 0, dissociate = 0;
>  	struct add_data add_data = ADD_DATA_INIT;
>  	const char *ref_storage_format = NULL;
> +	const struct submodule *existing;
>  	char *to_free = NULL;
>  	struct option options[] = {
>  		OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
> @@ -3546,6 +3556,32 @@ 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 the name is in use, and the submodule with that name is at a different path, then we are in trouble, OK.

Show 6 quoted lines
> +		if (!force)
> +			die(_("submodule name '%s' already used for path '%s'"),
> +			add_data.sm_name, existing->path);
> +
> +		/* --force: build <name><n> until unique */
> +		struct strbuf buf = STRBUF_INIT;
"-Wdeclaration-after-statement"
Show 15 quoted lines
> +		strbuf_addstr(&buf, add_data.sm_name);
> +
> +		for (int i = 1; ; i++) {
> +			strbuf_setlen(&buf, 0);
> +			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 = strbuf_detach(&buf, NULL);
> +	}
> +

What is the memory ownership rule for add_data.sm_name? Earlier we saw in a pre-context of a hunk that this was assigned from add_data.sm_path, so in that codepath it is considered a borrowed piece of memory, but here the member has to be the one that owns the string detached from the strbuf, which eventually must be freed by somebody, or it would be a memory leak.

K Jayatheerth· May 14, 2025, 02:01 UTC · re: Junio C Hamano · lore

[PATCH v3] submodule: prevent overwriting .gitmodules entry on path reuse

When a submodule is added at a path that previously hosted another submodule (e.g., 'child'), Git reuses the submodule name derived from the path and updates the corresponding entry in .gitmodules. This can silently overwrite existing configuration if the old submodule was only moved (e.g., to 'child_old') without renaming the submodule.

Teach `module_add()` to look up the chosen submodule name in the repository existing submodule config. If that name is already in use and points at a *different* path, we now die with an error, prompting the user to supply `--name`. Increment the name to an appropriate unique name (Like file system) when --force is called upon.

Add helper `submodule_active_matches_path()` so we can re-implement the old “is this path already covered by submodule.active?” logic without re-reading the config twice.

Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 builtin/submodule--helper.c | 62 ++++++++++++++++++++++++++++++-------
 t/t7400-submodule-basic.sh  | 23 ++++++++++++++
 2 files changed, 74 insertions(+), 11 deletions(-)
Show changes to 2 files +74 −11

builtin/submodule--helper.c, t/t7400-submodule-basic.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 53da2116dd..ef9e733cfb 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,23 @@ 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 +3389,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);
@@ -3443,7 +3452,11 @@ static int module_add(int argc, const char **argv, const char *prefix,
 	int force = 0, quiet = 0, progress = 0, dissociate = 0;
 	struct add_data add_data = ADD_DATA_INIT;
 	const char *ref_storage_format = NULL;
+	const struct submodule *existing;
 	char *to_free = NULL;
+	struct strbuf buf = STRBUF_INIT;
+	int i;
+	int allocated_sm_name = 0;
 	struct option options[] = {
 		OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
 			   N_("branch of repository to add as submodule")),
@@ -3546,6 +3559,31 @@ 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 = strbuf_detach(&buf, NULL);
+		allocated_sm_name = 1;
+	}
+
 	if (check_submodule_name(add_data.sm_name))
 		die(_("'%s' is not a valid submodule name"), add_data.sm_name);
 
@@ -3561,6 +3599,8 @@ static int module_add(int argc, const char **argv, const char *prefix,
 
 	ret = 0;
 cleanup:
+	if (allocated_sm_name)
+		free((char *)add_data.sm_name);
 	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..5c3f471338 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 "initial commit" &&
+
+		git init ../child-origin &&
+		git -C ../child-origin commit --allow-empty -m "initial commit" &&
+
+		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" &&
+
+		# Create another submodule repo
+		git init ../child2-origin &&
+		git -C ../child2-origin commit --allow-empty -m "initial commit" &&
+
+		test_must_fail git submodule add ../child2-origin child
+	)
+'
+
 test_done
-- 
2.49.GIT
Junio C Hamano· May 14, 2025, 22:47 UTC · re: K Jayatheerth · lore

Re: [PATCH v3] submodule: prevent overwriting .gitmodules entry on path reuse

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
> Add helper `submodule_active_matches_path()` so we can
> re-implement the old “is this path already covered by
> submodule.active?” logic without re-reading the config twice.

Having duplicated code to implement what is supposed to be the same thing is a bug waiting to happen by them diverging from each other.

Isn't the fact that our configuration reading code reads things just once and the caches the result good enough for the purpose of this code path? Do we have a measurement that tells us that the extra complexity is worth the maintenance headache?

Show 9 quoted lines
> @@ -3443,7 +3452,11 @@ static int module_add(int argc, const char **argv, const char *prefix,
>  	int force = 0, quiet = 0, progress = 0, dissociate = 0;
>  	struct add_data add_data = ADD_DATA_INIT;
>  	const char *ref_storage_format = NULL;
> +	const struct submodule *existing;
>  	char *to_free = NULL;
> +	struct strbuf buf = STRBUF_INIT;
> +	int i;
> +	int allocated_sm_name = 0;

A separate flag is not wrong per-se, but the idiom used in this project more often is to have an extra pointer variable that points at an allocated piece of memory (or NULL), and free the piece of memory unconditionally.

"git grep -e to_free" to see the idiom in action. Even better yet, this codepath already uses the idiom.

By doing so
> +	if (allocated_sm_name)
> +		free((char *)add_data.sm_name);
becomes
	free(sm_name_to_free);

and we can keep the "add_data.sm_name is pointing at a borrowed piece of memory, and we will _never_ free anything through that pointer" memory ownership rule. We were borrowing from a separate variable sm_name_to_free, and we may free it when add_data is getting destroyed, or we may be borrowing from the .sm_path string, which we would free it when add_data is getting destroyed.

Show 35 quoted lines
>  	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..5c3f471338 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 "initial commit" &&
> +
> +		git init ../child-origin &&
> +		git -C ../child-origin commit --allow-empty -m "initial commit" &&
> +
> +		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" &&
> +
> +		# Create another submodule repo
> +		git init ../child2-origin &&
> +		git -C ../child2-origin commit --allow-empty -m "initial commit" &&
> +
> +		test_must_fail git submodule add ../child2-origin child
> +	)
> +'
> +
>  test_done
JAYATHEERTH K· May 16, 2025, 08:51 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] submodule: prevent overwriting .gitmodules entry on path reuse

On Thu, May 15, 2025 at 4:18 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
>
> K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
>
> > Add helper `submodule_active_matches_path()` so we can
> > re-implement the old “is this path already covered by
> > submodule.active?” logic without re-reading the config twice.
>
> Having duplicated code to implement what is supposed to be the same
> thing is a bug waiting to happen by them diverging from each other.
>
> Isn't the fact that our configuration reading code reads things just
> once and the caches the result good enough for the purpose of this
> code path?  Do we have a measurement that tells us that the extra
> complexity is worth the maintenance headache?
>

Well when I first sent this patch I didn't quite understand why test 4137 was failing then I read the tests and I didn't have a lot of idea of the code. But I think it' better to remove helper as you said I will send a new patch with a different approach as I've spent some time understanding this now.

Show 34 quoted lines
> > @@ -3443,7 +3452,11 @@ static int module_add(int argc, const char **argv, const char *prefix,
> >       int force = 0, quiet = 0, progress = 0, dissociate = 0;
> >       struct add_data add_data = ADD_DATA_INIT;
> >       const char *ref_storage_format = NULL;
> > +     const struct submodule *existing;
> >       char *to_free = NULL;
> > +     struct strbuf buf = STRBUF_INIT;
> > +     int i;
> > +     int allocated_sm_name = 0;
>
> A separate flag is not wrong per-se, but the idiom used in this
> project more often is to have an extra pointer variable that points
> at an allocated piece of memory (or NULL), and free the piece of
> memory unconditionally.
>
> "git grep -e to_free" to see the idiom in action.  Even better yet,
> this codepath already uses the idiom.
>
> By doing so
>
> > +     if (allocated_sm_name)
> > +             free((char *)add_data.sm_name);
>
> becomes
>
>         free(sm_name_to_free);
>
> and we can keep the "add_data.sm_name is pointing at a borrowed
> piece of memory, and we will _never_ free anything through that
> pointer" memory ownership rule.  We were borrowing from a separate
> variable sm_name_to_free, and we may free it when add_data is
> getting destroyed, or we may be borrowing from the .sm_path string,
> which we would free it when add_data is getting destroyed.
>
Interesting, this is good, I'm going to copy this : )
Thank you,
-Jayatheerth
K Jayatheerth· May 16, 2025, 17:49 UTC · re: JAYATHEERTH K · lore

[PATCH v4] submodule: prevent overwriting .gitmodules entry on path reuse

When adding a submodule, Git may write `submodule.<name>.active = true` to the repository configuration to explicitly mark the submodule as active. However, if the submodule's path is already matched by a pattern in the `submodule.active` setting, this explicit entry is redundant.

This change inlines the logic, using `wildmatch()` to check if the added submodule path is already covered by one of the patterns defined in `submodule.active`.

If the path matches any pattern, Git now avoids writing the `submodule.<name>.active` entry. This prevents unnecessary configuration bloat and aligns the behavior with user expectations when using pattern-based submodule activation.

Reported-by: Moritz <mlell08@gmail.com>
Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 builtin/submodule--helper.c | 59 ++++++++++++++++++++++++++++++-------
 t/t7400-submodule-basic.sh  | 23 +++++++++++++++
 2 files changed, 72 insertions(+), 10 deletions(-)
Show changes to 2 files +72 −10

builtin/submodule--helper.c, t/t7400-submodule-basic.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 53da2116dd..5a9c8bdc0c 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 "strbuf.h"
+#include "wildmatch.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)) {
+	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);
 	}
 }
 
@@ -3443,7 +3453,11 @@ static int module_add(int argc, const char **argv, const char *prefix,
 	int force = 0, quiet = 0, progress = 0, dissociate = 0;
 	struct add_data add_data = ADD_DATA_INIT;
 	const char *ref_storage_format = NULL;
+	const struct submodule *existing;
 	char *to_free = NULL;
+	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 +3560,30 @@ 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 +3599,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..5c3f471338 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 "initial commit" &&
+
+		git init ../child-origin &&
+		git -C ../child-origin commit --allow-empty -m "initial commit" &&
+
+		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" &&
+
+		# Create another submodule repo
+		git init ../child2-origin &&
+		git -C ../child2-origin commit --allow-empty -m "initial commit" &&
+
+		test_must_fail git submodule add ../child2-origin child
+	)
+'
+
 test_done
-- 
2.49.GIT
JAYATHEERTH K· May 16, 2025, 17:53 UTC · re: K Jayatheerth · lore

Re: [PATCH v4] submodule: prevent overwriting .gitmodules entry on path reuse

I think the logic is almost similar as the helper function in this patch,
But I tried to wrap it within the if else block

Reason why this is needed is I initially thought that this was just a path issue Turned out tests like t4137 and t7413 failed

These failed tests were because I didn't take submodule active into consideration

And at the end since this is the only place where the helper is needed I just wrapped it up for this specific thing

Maybe if someone might need it in future or someone builds an helper function then using that might make sense but for now I think this is the middle ground approach we could find.

-Jayatheerth
K Jayatheerth· May 18, 2025, 07:54 UTC · re: JAYATHEERTH K · lore

[PATCH v5] submodule: prevent overwriting .gitmodules entry on path reuse

When a submodule is added, Git writes submodule.<name>.active = true to the repository configuration to mark it as active. This happens even when the submodule path already matches a pattern in submodule.active. This results in redundant configuration entries that are unnecessary and clutter the config, especially when pattern-based activation is used.

Avoid writing the submodule.<name>.active entry if the path is already covered by a pattern in submodule.active.

Reported-by: Moritz <mlell08@gmail.com>
Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
---
 builtin/submodule--helper.c | 59 ++++++++++++++++++++++++++++++-------
 t/t7400-submodule-basic.sh  | 23 +++++++++++++++
 2 files changed, 72 insertions(+), 10 deletions(-)
Show changes to 2 files +72 −10

builtin/submodule--helper.c, t/t7400-submodule-basic.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 53da2116dd..5a9c8bdc0c 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 "strbuf.h"
+#include "wildmatch.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)) {
+	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);
 	}
 }
 
@@ -3443,7 +3453,11 @@ static int module_add(int argc, const char **argv, const char *prefix,
 	int force = 0, quiet = 0, progress = 0, dissociate = 0;
 	struct add_data add_data = ADD_DATA_INIT;
 	const char *ref_storage_format = NULL;
+	const struct submodule *existing;
 	char *to_free = NULL;
+	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 +3560,30 @@ 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 +3599,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..5c3f471338 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 "initial commit" &&
+
+		git init ../child-origin &&
+		git -C ../child-origin commit --allow-empty -m "initial commit" &&
+
+		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" &&
+
+		# Create another submodule repo
+		git init ../child2-origin &&
+		git -C ../child2-origin commit --allow-empty -m "initial commit" &&
+
+		test_must_fail git submodule add ../child2-origin child
+	)
+'
+
 test_done
-- 
2.49.GIT
Junio C Hamano· May 19, 2025, 15:41 UTC · re: K Jayatheerth · lore

Re: [PATCH v5] submodule: prevent overwriting .gitmodules entry on path reuse

K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
Show 8 quoted lines
> When a submodule is added, Git writes submodule.<name>.active = true
> to the repository configuration to mark it as active. This happens even
> when the submodule path already matches a pattern in submodule.active.
> This results in redundant configuration entries that are unnecessary
> and clutter the config, especially when pattern-based activation is used.
>
> Avoid writing the submodule.<name>.active entry if the path is already
> covered by a pattern in submodule.active.

This explains why the part of the change that deals the .active bit makes sense.

But now do we drop the other fix from the patch, namely "ouch, we are adding submodule at 'foo/' but the name 'foo' is taken by a different submodule that used to live there and moved elsewhere", or have you forgotten to describe that fix in the proposed log message?

Stepping back a bit, perhaps this patch addresses two independent issues, both of which can trigger with"submodule add"? If so, would it make sense to have it in two separate patches?

Show 22 quoted lines
> +test_expect_success 'submodule add fails when name is reused' '
> +	git init test-submodule &&
> +	(
> +		cd test-submodule &&
> +		git commit --allow-empty -m "initial commit" &&
> +
> +		git init ../child-origin &&
> +		git -C ../child-origin commit --allow-empty -m "initial commit" &&
> +
> +		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" &&
> +
> +		# Create another submodule repo
> +		git init ../child2-origin &&
> +		git -C ../child2-origin commit --allow-empty -m "initial commit" &&
> +
> +		test_must_fail git submodule add ../child2-origin child
> +	)
> +'

The test seems to be about "the other issue". Shouldn't we also have a test about "we no longer add redundant configuration entries"?

Thanks.
JAYATHEERTH K· May 20, 2025, 01:31 UTC · re: Junio C Hamano · lore

Re: [PATCH v5] submodule: prevent overwriting .gitmodules entry on path reuse

On Mon, May 19, 2025 at 9:11 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
>
> K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
>
> > When a submodule is added, Git writes submodule.<name>.active = true
> > to the repository configuration to mark it as active. This happens even
> > when the submodule path already matches a pattern in submodule.active.
> > This results in redundant configuration entries that are unnecessary
> > and clutter the config, especially when pattern-based activation is used.
> >
> > Avoid writing the submodule.<name>.active entry if the path is already
> > covered by a pattern in submodule.active.
>
> This explains why the part of the change that deals the .active bit
> makes sense.
>
Hmm, I look at it this way,
active and path problem are not different,
It's just that this has to follow t7413
submodule active too.
Therefore the submodule.<name>.active = true
logic exists
 > >Avoid writing the submodule.<name>.active entry if the path is already
 > >covered by a pattern in submodule.active.
I think this order makes sense to me, but I could change if you want me to.

The path is the core issue and the active is just like a neat wrapper of following all the test cases in my opinion.

Show 9 quoted lines
> But now do we drop the other fix from the patch, namely "ouch, we
> are adding submodule at 'foo/' but the name 'foo' is taken by a
> different submodule that used to live there and moved elsewhere", or
> have you forgotten to describe that fix in the proposed log message?
>
> Stepping back a bit, perhaps this patch addresses two independent
> issues, both of which can trigger with"submodule add"?  If so, would
> it make sense to have it in two separate patches?
>

This would be the case if I wrote a separate helper function I guess but the core issue still lies at the if else block And the active part is just written to mark the submodule.<name>.active = true So I think these are a part of the same problem.

Show 26 quoted lines
> > +test_expect_success 'submodule add fails when name is reused' '
> > +     git init test-submodule &&
> > +     (
> > +             cd test-submodule &&
> > +             git commit --allow-empty -m "initial commit" &&
> > +
> > +             git init ../child-origin &&
> > +             git -C ../child-origin commit --allow-empty -m "initial commit" &&
> > +
> > +             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" &&
> > +
> > +             # Create another submodule repo
> > +             git init ../child2-origin &&
> > +             git -C ../child2-origin commit --allow-empty -m "initial commit" &&
> > +
> > +             test_must_fail git submodule add ../child2-origin child
> > +     )
> > +'
>
> The test seems to be about "the other issue".  Shouldn't we also
> have a test about "we no longer add redundant configuration entries"?
>

Actually t7413 has a detailed coverage of the _active_ logic. I actually didn't consider submodule.<name>.active = true but looking what failed in 7413 made me realise we have to solve the core issue and then submodule.<name>.active = true it too. Therefore I didn't add extra test case too.

> Thanks.

I'm open to a review from your side on this I may have missed some logic here.

Thank you,
-Jayatheerth
Junio C Hamano· May 20, 2025, 15:28 UTC · re: JAYATHEERTH K · lore

Re: [PATCH v5] submodule: prevent overwriting .gitmodules entry on path reuse

JAYATHEERTH K <jayatheerthkulkarni2005@gmail.com> writes:
Show 26 quoted lines
> On Mon, May 19, 2025 at 9:11 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
>>
>> > When a submodule is added, Git writes submodule.<name>.active = true
>> > to the repository configuration to mark it as active. This happens even
>> > when the submodule path already matches a pattern in submodule.active.
>> > This results in redundant configuration entries that are unnecessary
>> > and clutter the config, especially when pattern-based activation is used.
>> >
>> > Avoid writing the submodule.<name>.active entry if the path is already
>> > covered by a pattern in submodule.active.
>>
>> This explains why the part of the change that deals the .active bit
>> makes sense.
>>
>
> Hmm, I look at it this way,
> active and path problem are not different,
> It's just that this has to follow t7413
> submodule active too.
> Therefore the submodule.<name>.active = true
> logic exists
>  > >Avoid writing the submodule.<name>.active entry if the path is already
>  > >covered by a pattern in submodule.active.
> I think this order makes sense to me, but I could change if you want me to.

Do you mean the problem "we add a submodule at 'foo/' but the name 'foo' is taken already" does *not* happen when submodule.active patterns are *not* used? If so, I understand that these two problems are linked together, but I got an impression that it is not what is happening.

And that is where this question ...
>> Stepping back a bit, perhaps this patch addresses two independent
>> issues, both of which can trigger with"submodule add"?  If so, would
>> it make sense to have it in two separate patches?

... comes from. The added test does not use submodule.active pattern, so I assumed that the pattern-based activation is a separate issue.

The solution to pattern-based activation may have to happen by updating the same code path and it may turn out to be that having a single helper function that deals with both is the cleanest solution, but still if they are two different problems, a single patch should not try to solve both of them at the same time. Solve one by introducing a helper function and make sure that the fix for that one problem will not get broken by adding tests, and then on top, solve the other, perhaps by modifying the same helper function, and make sure that the fix for the other problem will not get broken by adding further tests, prehaps?

Show 30 quoted lines
> This would be the case if I wrote a separate helper function I guess
> but the core issue still lies at the if else block
> And the active part is just written to mark the submodule.<name>.active = true
> So I think these are a part of the same problem.
>
>> > +test_expect_success 'submodule add fails when name is reused' '
>> > +     git init test-submodule &&
>> > +     (
>> > +             cd test-submodule &&
>> > +             git commit --allow-empty -m "initial commit" &&
>> > +
>> > +             git init ../child-origin &&
>> > +             git -C ../child-origin commit --allow-empty -m "initial commit" &&
>> > +
>> > +             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" &&
>> > +
>> > +             # Create another submodule repo
>> > +             git init ../child2-origin &&
>> > +             git -C ../child2-origin commit --allow-empty -m "initial commit" &&
>> > +
>> > +             test_must_fail git submodule add ../child2-origin child
>> > +     )
>> > +'
>>
>> The test seems to be about "the other issue".  Shouldn't we also
>> have a test about "we no longer add redundant configuration entries"?
Show 6 quoted lines
> Actually t7413 has a detailed coverage of the _active_ logic.
> I actually didn't consider submodule.<name>.active = true
> but looking what failed in 7413 made me realise we have to
> solve the core issue and then
> submodule.<name>.active = true it too.
> Therefore I didn't add extra test case too.

When we re-read what you wrote as a proposed log message, you said that adding a new submodule that would match the "submodule.active" pattern will result in redundant configuration entries that are unnecessary.

If you perceive it as a problem to be solved (and you do---otherwise you wouldn't have written that there), the existing tests do *not* consider it as a problem, so even if they have tests that does the pattern-based activation, they would not have any tests that make sure they do not create redundant configuration entries, would they? If we fixed that issue, then now we need to make sure that future changes will not break the fix and start adding redundant configuration entries, but because there is no test that does so (otherwise, you wouldn't have found a need to fix that "redundant configuration entries" problem in the first place), the patch that fixes the problem needs to add tests to do so, no?

What "redundant" entries do you exactly mean? If it is "we end up with the same submodule.<name>.active appearing twice", then the test should make sure the resulting .git/config file has only one (perhaps with "git config --get-all"), for example.

K Jayatheerth· May 24, 2025, 06:48 UTC · re: Junio C Hamano · lore

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

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
K Jayatheerth· May 24, 2025, 06:48 UTC · re: K Jayatheerth · lore

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

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(+)
Show changes to 2 files +51 −0

builtin/submodule--helper.c, t/t7400-submodule-basic.sh

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..07951f0c6d 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
K Jayatheerth· May 24, 2025, 06:48 UTC · re: K Jayatheerth · lore

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

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(-)
Show changes to 2 files +36 −11

builtin/submodule--helper.c, t/t7413-submodule-is-active.sh

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
JAYATHEERTH K· May 24, 2025, 06:54 UTC · re: K Jayatheerth · lore

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

Since I added the helper back I wrote the tests for the active part of the problem too and separated the patches for clarity.

I also ran the tests and it passed the test 9 in t7413 and added test 10.
Thank you,
-Jayatheerth
K Jayatheerth· May 24, 2025, 07:30 UTC · re: JAYATHEERTH K · lore

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

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.
V7 has added Tab spaces in the test rather than 4 spaces in V6
Junio C Hamano (1):
  The seventeenth batch
K Jayatheerth (1):
  submodule: prevent overwriting .gitmodules entry on path reuse
 Documentation/RelNotes/2.50.0.adoc | 19 +++++++++++++++++++
 builtin/submodule--helper.c        | 28 ++++++++++++++++++++++++++++
 t/t7400-submodule-basic.sh         | 23 +++++++++++++++++++++++
 3 files changed, 70 insertions(+)
-- 
2.49.GIT
K Jayatheerth· May 24, 2025, 07:30 UTC · re: K Jayatheerth · lore

[PATCH v7 1/2] The seventeenth batch

From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/RelNotes/2.50.0.adoc | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
Show changes to Documentation/RelNotes/2.50.0.adoc +19 −0
diff --git a/Documentation/RelNotes/2.50.0.adoc b/Documentation/RelNotes/2.50.0.adoc
index bf73de114e..f721ea350d 100644
--- a/Documentation/RelNotes/2.50.0.adoc
+++ b/Documentation/RelNotes/2.50.0.adoc
@@ -72,6 +72,10 @@ UI, Workflows & Features
  * The `send-email` documentation has been updated with OAuth2.0
    related examples.
 
+ * Two of the "scalar" subcommands that add a repository that hasn't
+   been under "scalar"'s control are taught an option not to enable the
+   scheduled maintenance on it.
+
 
 Performance, Internal Implementation, Development Support etc.
 --------------------------------------------------------------
@@ -157,6 +161,12 @@ Performance, Internal Implementation, Development Support etc.
 
  * Build performance fix.
 
+ * Teach "git send-email" to also consult `hostname -f` for mail
+   domain to compute the identity given to SMTP servers.
+
+ * The dependency on the_repository variable has been reduced from the
+   code paths in "git replay".
+
 
 Fixes since v2.49
 -----------------
@@ -306,6 +316,15 @@ Fixes since v2.49
  * Use-after-free fix in the sequencer.
    (merge 5dbaec628d pw/sequencer-reflog-use-after-free later to maint).
 
+ * win+Meson CI pipeline, unlike other pipelines for Windows,
+   used to build artifacts in develper mode, which has been changed to
+   build them in release mode for consistency.
+   (merge 184abdcf05 js/ci-build-win-in-release-mode later to maint).
+
+ * CI settings at GitLab has been updated to run MSVC based Meson job
+   automatically (as opposed to be done only upon manual request).
+   (merge 6389579b2f ps/ci-gitlab-enable-msvc-meson-job later to maint).
+
  * Other code cleanup, docfix, build fix, etc.
    (merge 227c4f33a0 ja/doc-block-delimiter-markup-fix later to maint).
    (merge 2bfd3b3685 ab/decorate-code-cleanup later to maint).
-- 
2.49.GIT
K Jayatheerth· May 24, 2025, 07:30 UTC · re: K Jayatheerth · lore

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

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(+)
Show changes to 2 files +51 −0

builtin/submodule--helper.c, t/t7400-submodule-basic.sh

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

← back to recent threads