On Sat, Jun 7, 2025 at 11:28 PM K Jayatheerth
<jayatheerthkulkarni2005@xxxxxxxxx> wrote:
>
> Adding a submodule at a path that previously hosted another submodule
> (e.g., 'child') reuses the submodule name derived from the path. If the
> original submodule was only moved (e.g., to 'child_old') and not renamed,
> this silently overwrites its configuration in .gitmodules.
>
> This behavior loses user configuration and causes confusion when the
> original submodule is expected to remain intact. It assumes that the
> path-derived name is always safe to reuse, even though the name might
> still be in use elsewhere in the repository.
>
> Teach `module_add()` to check if the computed submodule name already
> exists in the repository's submodule config, and if so, refuse the
> operation unless the user explicitly renames or uses force to auto increment.
I had to read the patch to figure out what "auto increment"
meant—perhaps some accompanying docs in `git help submodule`?
>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@xxxxxxxxx>
> ---
> builtin/submodule--helper.c | 28 ++++++++++++++++++++++++++++
> t/t7400-submodule-basic.sh | 23 +++++++++++++++++++++++
> 2 files changed, 51 insertions(+)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 53da2116dd..9f6df833f0 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -3444,6 +3444,10 @@ static int module_add(int argc, const char **argv, const char *prefix,
> struct add_data add_data = ADD_DATA_INIT;
> const char *ref_storage_format = NULL;
> char *to_free = NULL;
> + const struct submodule *existing;
> + struct strbuf buf = STRBUF_INIT;
> + int i;
> + char *sm_name_to_free = NULL;
> struct option options[] = {
> OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
> N_("branch of repository to add as submodule")),
> @@ -3546,6 +3550,29 @@ static int module_add(int argc, const char **argv, const char *prefix,
> if(!add_data.sm_name)
> add_data.sm_name = add_data.sm_path;
>
> + existing = submodule_from_name(the_repository,
> + null_oid(the_hash_algo),
> + add_data.sm_name);
> +
> + if (existing && strcmp(existing->path, add_data.sm_path)) {
> + if (!force) {
> + die(_("submodule name '%s' already used for path '%s'"),
> + add_data.sm_name, existing->path);
> + }
> +
> + /* --force: build <name><n> until unique */
> + for (i = 1; ; i++) {
> + strbuf_reset(&buf);
> + strbuf_addf(&buf, "%s%d", add_data.sm_name, i);
> + if (!submodule_from_name(the_repository,
> + null_oid(the_hash_algo),
> + buf.buf)) {
> + break;
> + }
> + }
This isn't typically what I'd expect --force to do, personally, though
in this case it allows me to proceed with an operation that wasn't
allowed otherwise.
Still, I wonder if a user might be confused by "I said 'child' and got
'child2'?"
> +
> + add_data.sm_name = sm_name_to_free = strbuf_detach(&buf, NULL);
> + }
> if (check_submodule_name(add_data.sm_name))
> die(_("'%s' is not a valid submodule name"), add_data.sm_name);
>
> @@ -3561,6 +3588,7 @@ static int module_add(int argc, const char **argv, const char *prefix,
>
> ret = 0;
> cleanup:
> + free(sm_name_to_free);
> free(add_data.sm_path);
> free(to_free);
> strbuf_release(&sb);
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index d6a501d453..f5514decab 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -1482,4 +1482,27 @@ test_expect_success '`submodule init` and `init.templateDir`' '
> )
> '
>
> +test_expect_success 'submodule add fails when name is reused' '
> + git init test-submodule &&
> + (
> + cd test-submodule &&
> + git commit --allow-empty -m init &&
> +
> + git init ../child-origin &&
> + git -C ../child-origin commit --allow-empty -m init &&
> +
> + git submodule add ../child-origin child &&
> + git commit -m "Add submodule child" &&
> +
> + git mv child child_old &&
> + git commit -m "Move child to child_old" &&
> +
> + # Now adding a *new* repo at the old name must fail
> + git init ../child2-origin &&
> + git -C ../child2-origin commit --allow-empty -m init &&
> + test_must_fail git submodule add ../child2-origin child
This makes sense, though I was hoping (when I'd only skimmed the
message and not seen "refuse") that this would be permitted by some
clever trick. Oh well.
> + )
> +'
> +
> +
> test_done
> --
> 2.49.GIT
>
>
--
D. Ben Knoble
