Re: [PATCHv3] submodule--helper: normalize funny urls

[Stefan Beller <sbeller@google.com>]

On Tue, Oct 18, 2016 at 5:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The underlying issue is two fold:
>>
>> * in t3600 we'd need
>> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
>> index d046d98..545d32f 100755
>> --- a/t/t3600-rm.sh
>> +++ b/t/t3600-rm.sh
>> @@ -616,7 +616,7 @@ test_expect_success 'setup subsubmodule' '
>>         git submodule update &&
>>         (cd submod &&
>>                 git update-index --add --cacheinfo 160000 $(git
>> rev-parse HEAD) subsubmod &&
>> -               git config -f .gitmodules submodule.sub.url ../. &&
>> +               git config -f .gitmodules submodule.sub.url ./. &&
>>                 git config -f .gitmodules submodule.sub.path subsubmod &&
>>                 git submodule init &&
>>                 git add .gitmodules &&
>>
>> because the sub-submodule URL is actually the same as the submodule
>> (because we'd test lazily)
>
> This fix sounds entirely sane.  The "../." you touched was depending
> on the buggy behaviour; it is exactly the case of "there were two
> wrongs that were covering each other; after one of them gets fixed,
> the other one's brokenness is exposed", right?
>
>> However in t7403, we have a construct like:
>>
>>     git clone . super
>>
>> which then results in
>>
>>     git -C super remote -v
>> ...../git/t/trash directory.t7403-submodule-sync/. (fetch)
>
> That sounds expected.  We do not have to add trailing "/.", but the
> system ought to work with or without it correctly and the same way.
>
>> However instead of fixing the levels of nesting, the fix is as easy as:
>> diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
>> index 0726799..525d32b 100755
>> --- a/t/t7403-submodule-sync.sh
>> +++ b/t/t7403-submodule-sync.sh
>> @@ -15,7 +15,9 @@ test_expect_success setup '
>>         git add file &&
>>         test_tick &&
>>         git commit -m upstream &&
>> -       git clone . super &&
>> +       # avoid cloning a repository with a url ending in /.
>> +       git clone . root &&
>> +       git clone root super &&
>>         git clone super submodule &&
>>         (
>>                 cd submodule &&
>>
>> Same goes for t740{6,7} as well as t7506.
>
> Isn't the issue the same as that "3600-rm" one?  I know you said
> twofold upfront, but I am not sure I agree.

I took a couple of hours trying to get the same fix applied to the t7* tests,
but that doesn't seem to be as easy. I'll try again.

>
> The "super" repository refers to its submodules with "../submodule"
> in the test code we have, even though the submodule referred to
> lives inside $TRASH, and by fixing the "trailing /. and trailing
> /root are treated the same way" bug, its reference created in the
> test ends up referring to one level above, perhaps in t/submodule,
> instead of the intended place t/trash/submodule.  By using "root",
> you are making their wrong references point at the right place.

Correct.

>
> Admittedly, the updated test above tests something different from
> what it originally wanted to test, which feels somewhat distasteful
> but I do not think it is wrong.

I think it is. I was just showing how to quick fix the issue, and how wide the
impact was. More like assessing the situation of what is broken with
that patch, rather than proposing a way to go for fixing.

>
>> I think this change to the test suite is not warranted, because
>> we want to have the current behavior as-is ...
>
> I am not sure.  Certainly we would want to make sure that the normal
> case (i.e. no funny trailing junk) to work correctly, but we do want
> to protect the fix from future breakage as well, no?

Exactly. So not intermediate "root" that we clone from, but adapting the
relative URLs. Maybe half the broken tests can switch to 'root' and the others
go with the current behavior of cloning . to super.

>
> Perhaps we can do a preliminary step to update tests to primarily
> check the cases that do not involve URL with trailing "/." by either
> doing that double clone, or more preferrably, clone from "$(pwd)"
> and adjust the incorrect submodule reference that have been relying
> on the buggy behaviour.  With that "root" trick, the test would pass
> with or without the fix under discussion, right?

I assume so, (not tested).

>
> Then do the fix under discussion and introduce a test that clones
> from "." refers to submodules with relative path and makes sure that
> trailing "/." is interpreted correctly.