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

[Junio C Hamano <gitster@pobox.com>]

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.

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.