Re: [PATCH v4 2/3] implement fetching of moved submodules

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 18, 2017, 00:03 UTC
Message-ID: <xmqqfuahmif9.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kZsQoU8wJk+i5aJOxFtsD=EWu_ycEPLM1KhTaOCWD7Y2w@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 12 quoted lines

>> +                       /* make sure name does not collide with existing one */
>> +                       submodule = submodule_from_name(commit_oid, name);
>> +                       if (submodule) {
>> +                               warning("Submodule in commit %s at path: "
>> +                                       "'%s' collides with a submodule named "
>> +                                       "the same. Skipping it.",
>> +                                       oid_to_hex(commit_oid), name);
>> +                               name = NULL;
>> +                       }
>
> This is the ugly part of using one string list and storing names or
> path in it. I wonder if we could omit this warning if we had 2 string lists?

We are keying off of 'name', because that is what will give a module its identity. If we have a gitlink whose path is not in .gitmodules in the same tree, then we are seeing an unregistered submodule. If we were to "git add" it, then we'd use its path as the default name, but if we already have a submodule with that name (the most likely explanation for its existence is because it started its life there and then later moved), and the submodule is bound to a different path, then that is a different submodule. Skipping and warning both are sensible thing to do.

I do not know what you see as ugly here, and more importantly, I am not sure how having two lists would help.