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

[Ben Knoble <ben.knoble@gmail.com>]

> Le 20 juil. 2025 à 08:25, JAYATHEERTH K <jayatheerthkulkarni2005@gmail.com> a écrit :
> 
> On Wed, Jul 9, 2025 at 8:20 AM D. Ben Knoble <ben.knoble@gmail.com> wrote:
>> 
>>> 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'?"
>> 
> 
> 
> 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.

That sounds more like a typical forceful operation to me.