Re: [PATCH 6/6] git submodule add: Fix naming clash handling

["Lars Hjemli" <hjemli@gmail.com>]

On Sat, Sep 13, 2008 at 4:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Petr Baudis <pasky@suse.cz> writes:
>
>> This patch fixes git submodule add behaviour when we add submodule
>> living at a same path as logical name of existing submodule. This
>> can happen e.g. in case the user git mv's the previous submodule away
>> and then git submodule add's another under the same name.
>
> In short, name "example" was used to name the submodule bound at path
> "init" in earlier tests, and the new one adds another submodule whose name
> and path are both "example" and makes sure they do not get mixed up (and
> the original code did mix them up).
>
>>  git-submodule.sh           |   15 ++++++++++++---
>>  t/t7400-submodule-basic.sh |   11 +++++++++++
>>  2 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 1c39b59..3e4d839 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -201,10 +201,19 @@ cmd_add()
>>       git add "$path" ||
>>       die "Failed to add submodule '$path'"
>>
>> -     git config -f .gitmodules submodule."$path".path "$path" &&
>> -     git config -f .gitmodules submodule."$path".url "$repo" &&
>> +     name="$path"
>> +     if git config -f .gitmodules submodule."$name".path; then
>> +             name="$path~"; i=1;
>> +             while git config -f .gitmodules submodule."$name".path; do
>> +                     name="$path~$i"
>> +                     i=$((i+1))
>> +             done
>> +     fi
>> +
>> +     git config -f .gitmodules submodule."$name".path "$path" &&
>> +     git config -f .gitmodules submodule."$name".url "$repo" &&
>>       git add .gitmodules ||
>> -     die "Failed to register submodule '$path'"
>> +     die "Failed to register submodule '$path' (name '$name')"
>>  }
>
> The logic of the fix seems to be correct, but shouldn't the test be like
> this instead?
>
>        if git config -f .gitmodules "submodule.$name.path" >/dev/null
>        then
>
> The same thing for "git config" used in the "while" loop.

Yeah, redirecting to /dev/null seems like a good idea.


> Also I am not sure if name="$path~" is a good idea for two reasons:
>
>  - name suffixed with tilde and number looks too much like revision
>   expression.
>
>  - A, A~, A~1, A~2... looks ugly; A, A-0, A-1, A-2,... (or start counting
>   from 1 or 2) I would understand.

I'd just exit with an informative error if submodule.$name already
exists in .git/config.


> By the way, I noticed that cmd_add does not seem to cd_to_toplevel, and
> accesses .gitmodules in the current working directory.  Isn't this a bug?

Not really, since `git submodule` must be executed from the toplevel
($SUBDIRECTORY_OK is undefined, so the cd_to_toplevel in cmd_sync and
cmd_summary seems to be noops).

--
larsh