Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"

[Stefan Beller <sbeller@google.com>]

On Sun, Oct 23, 2016 at 3:14 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 22.10.2016 um 22:46 schrieb Stefan Beller:
>>
>> I have looked into it again, and by now I think the bug is a feature,
>> actually.
>>
>> Consider this:
>>
>>     git clone . super
>>     git -C super submodule add ../submodule
>>     # we thought the previous line is buggy
>>     git clone super super-clone
>
>
> At this point, we *should* have this if there were no bugs (at least that is
> my assumption):
>
>   /tmp
>   !
>   + submodule     <- submodule's remote repo
>   !
>   + foo           <- we are here (.), super's remote repo
>     !
>     + super       <- remote.origin.url=/tmp/foo/.
>       !
>       + submodule <- remote.origin.url=/tmp/foo/./../submodule
>                      submodule.submodule.url=../submodule
>
> When I test this, 'git submodule add' fails:

Yes this setup currently fails because the  /tmp/foo/./../submodule
is computed to be /tmp/foo/submodule.

In the test suite the directory "foo" is usually called
"trash\ directory.tXXXX-description" and the remote of
the submodule is created inside of it, such that:

/tmp
   !
   + foo            <- we are here (.), super's remote repo
     !
     + submodule    <- submodule's remote repo
     !
     + super        <- remote.origin.url=/tmp/foo/.
       !
       + submodule  <- remote.origin.url=/tmp/foo/./../submodule
                       submodule.submodule.url=../submodule
                       current result=/tmp/foo/submodule

works.

>
> foo@master> git -C super submodule add ../submodule
> fatal: repository '/tmp/foo/submodule' does not exist
> fatal: clone of '/tmp/foo/submodule' into submodule path
> '/tmp/foo/super/submodule' failed
>
>> Now in the super-clone the ../submodule is the correct
>> relative url, because the url where we cloned from doesn't
>> end in /.
>
>
> I do not understand why this would be relevant. The question is not how the
> submodule's remote URL ends, but how the submodule's remote URL is
> constructed from the super-project's URL and the relative path specified for
> 'git submodule add'.

I was not precise here. If you have the working setup as above, you can clone
super to super-clone and it keeps working, because of the current "bug".

The difference between "super" that is cloned from . and "super-clone" that
is cloned from "super" is only the remote url, and currently
/tmp/foo/super
/tmp/foo/.

+ relative path ../submodule resolve to the same submodule remote.

>
> Whether ../submodule or ./submodule is the correct relative URL depends on
> where the origin of the submodule is located relative to the origin of the
> super-project. In the above example, it is ../submodule. However, the error
> message tells us that git looked in /tmp/foo/submodule, which looks like the
> /. bug!

Correct.

At the time I was trying to fix the urls in the test suite with the
bug fix and I then
realized that this bugfix introduces a lot of pain to our test suite,
because we do
these secondary clones quite a few times. The pattern usually is:
* setup super (cloned from .)
* clone super to super-clone
* trash super-clone for testing purposes and delete it.

>
> I do not understand where you see a feature here. What am I missing?

The ease of use in our own testing suite is the feature I was talking about.

When we would fix the bug, we would not just need to fix
s|../submodule|./submodule| when setting up super, but we would need to
fix it every time again in reverse when creating these short lived
"super-clones"
that get tested on and deleted.

So maybe the correct fix for the testing suite is a double clone, i.e. instead
of


    # prepare some stuff
    git clone . super

we rather do:

    # mkdir upstream && prepare stuff in upstream
    git clone upstream super

However that way we would end up not exercising the
code path to be fixed with the actual bug fix i.e. we'd never clone from /.
because it is silly conceptually. We could add a new test for that though
that only tests that cloning from . constructs the correct URL.
However that is already tested in t0060 resolving the relative URLs
via the git submodule helper.

Thanks for bearing this discussion,
I feel like I am missing the obvious point here,

Stefan