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

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

[K Jayatheerth]

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(-)

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

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

[JAYATHEERTH K]

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

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

[Junio C Hamano]

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?

> 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(-)
>
> 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

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

[JAYATHEERTH K]

On Mon, May 12, 2025 at 6:02 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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

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

[K Jayatheerth]

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(-)

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

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

[Junio C Hamano]

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.

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?

> @@ -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.

> +		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"

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

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

[K Jayatheerth]

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(-)

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

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

[Junio C Hamano]

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?

> @@ -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.

>  	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

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

[JAYATHEERTH K]

On Thu, May 15, 2025 at 4:18 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

> > @@ -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

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

[K Jayatheerth]

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(-)

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

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

[JAYATHEERTH K]

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

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

[K Jayatheerth]

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(-)

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

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

[JAYATHEERTH K]

No separate change
just changed the commit message to the format

Added _order_ to the last line.

Thank you,

-Jayatheerth

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

[Junio C Hamano]

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.

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?

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

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

[JAYATHEERTH K]

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.

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

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

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

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

[Junio C Hamano]

JAYATHEERTH K <jayatheerthkulkarni2005@gmail.com> writes:

> 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?

> 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"?

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

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

[K Jayatheerth]

This series of patch covers mainly two areas

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

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

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

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

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

-- 
2.49.GIT

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

[K Jayatheerth]

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

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

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

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 53da2116dd..9f6df833f0 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -3444,6 +3444,10 @@ static int module_add(int argc, const char **argv, const char *prefix,
 	struct add_data add_data = ADD_DATA_INIT;
 	const char *ref_storage_format = NULL;
 	char *to_free = NULL;
+	const struct submodule *existing;
+	struct strbuf buf = STRBUF_INIT;
+	int i;
+	char *sm_name_to_free = NULL;
 	struct option options[] = {
 		OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
 			   N_("branch of repository to add as submodule")),
@@ -3546,6 +3550,29 @@ static int module_add(int argc, const char **argv, const char *prefix,
 	if(!add_data.sm_name)
 		add_data.sm_name = add_data.sm_path;
 
+	existing = submodule_from_name(the_repository,
+					null_oid(the_hash_algo),
+					add_data.sm_name);
+
+	if (existing && strcmp(existing->path, add_data.sm_path)) {
+		if (!force) {
+			die(_("submodule name '%s' already used for path '%s'"),
+			add_data.sm_name, existing->path);
+		}
+
+		/* --force: build <name><n> until unique */
+		for (i = 1; ; i++) {
+			strbuf_reset(&buf);
+			strbuf_addf(&buf, "%s%d", add_data.sm_name, i);
+			if (!submodule_from_name(the_repository,
+						null_oid(the_hash_algo),
+						buf.buf)) {
+				break;
+			}
+		}
+
+		add_data.sm_name = sm_name_to_free = strbuf_detach(&buf, NULL);
+	}
 	if (check_submodule_name(add_data.sm_name))
 		die(_("'%s' is not a valid submodule name"), add_data.sm_name);
 
@@ -3561,6 +3588,7 @@ static int module_add(int argc, const char **argv, const char *prefix,
 
 	ret = 0;
 cleanup:
+	free(sm_name_to_free);
 	free(add_data.sm_path);
 	free(to_free);
 	strbuf_release(&sb);
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index d6a501d453..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

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

[K Jayatheerth]

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

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

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

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

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

[JAYATHEERTH K]

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

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

[K Jayatheerth]

This series of patch covers mainly two areas

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

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

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

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

[PATCH v7 1/2] The seventeenth batch

[K Jayatheerth]

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

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

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

[K Jayatheerth]

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

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

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

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

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

Re: [PATCH v7 1/2] The seventeenth batch

[JAYATHEERTH K]

Oh my god

I'm so clumsy today

Sorry bout that
will send in a new thread.

-Jayatheerth