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

On Wed, Jul 9, 2025 at 8:20 AM D. Ben Knoble [off-list ref] wrote:

On Sat, Jun 7, 2025 at 11:28 PM K Jayatheerth
[off-list ref] wrote:

quoted

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

quoted

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

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 53da2116dd..9f6df833f0 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c

@@ -3444,6 +3444,10 @@ static int module_add(int argc, const char **argv, const char *prefix,
        struct add_data add_data = ADD_DATA_INIT;
        const char *ref_storage_format = NULL;
        char *to_free = NULL;
+       const struct submodule *existing;
+       struct strbuf buf = STRBUF_INIT;
+       int i;
+       char *sm_name_to_free = NULL;
        struct option options[] = {
                OPT_STRING('b', "branch", &add_data.branch, N_("branch"),
                           N_("branch of repository to add as submodule")),

@@ -3546,6 +3550,29 @@ static int module_add(int argc, const char **argv, const char *prefix,
        if(!add_data.sm_name)
                add_data.sm_name = add_data.sm_path;

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

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

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

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

Thank You

- Jayatheerth