On Wed, Jul 9, 2025 at 8:20 AM D. Ben Knoble <ben.knoble@xxxxxxxxx> wrote:
>
> 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'?"
>
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
