Show 88 quoted lines
> 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.Ben Knoble <ben.knoble@gmail.com>