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

>
> On Sat, Jun 7, 2025 at 11:28 PM K Jayatheerth
> <jayatheerthkulkarni2005@gmail.com> 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@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;
> > +                       }
> > +               }
>
> 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'?"
>