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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 19, 2016, 00:56 UTC
Message-ID: <xmqq60opnolz.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kZHLVpxbJ_C-dM2LDA64-_TJNyY+52fTWkOvLvvAq2XDg@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 20 quoted lines

> 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?

Show 8 quoted lines

> 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.

Show 18 quoted lines

> 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.

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.

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 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?

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?

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.