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

From: Stefan Beller <sbeller@google.com>
Date: Oct 22, 2016, 20:46 UTC
Message-ID: <CAGZ79kaukGh2ynkOQcF=skzxTMYr8CFRyGJw6FEmNsTAcaG_VQ@mail.gmail.com>
In-Reply-To: <xmqqh984aldl.fsf@gitster.mtv.corp.google.com>

On Sat, Oct 22, 2016 at 12:33 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 23 quoted lines

> Johannes Sixt <j6t@kdbg.org> writes:
>
>>> The logic to construct the relative urls is not smart enough to
>>> detect that the ending /. is referring to the directory itself
>>> but rather treats it like any other relative path, i.e.
>>>
>>>     path/to/dir/. + ../relative/path/to/submodule
>>>
>>> would result in
>>>
>>>     path/to/dir/relative/path/to/submodule
>>>
>>> and not omit the "dir" as you may expect.
>>>
>>> As in a later patch we'll normalize the remote url before the
>>> computation of relative urls takes place, we need to first get our
>>> test suite in a shape with normalized urls only, which is why we should
>>> refrain from cloning from '.'
>>
>> But you are removing a valid use case from the tests. Aren't you
>> sweeping something under the rug with this patch?
>
> I share the same reaction.

Oh I see. I agree. I reverted the lines that replace . by "$(pwd)" and it still works, but it still works because the code path regarding local paths was not broken (both . as well as "$(pwd)" are working without the fix)

Show 21 quoted lines

>
> If the primary problem being solved is that the combination of a
> relative URL ../sub and the base URL for the superproject which is
> set to /path/to/dir/. (due to "clone .") were incorrectly resolved
> as /path/to/dir/sub (because the buggy relative path logic did not
> know that removing "/." at the end does not take you to one level
> up), and a topic that fixes the bug would make that relative URL
> ../sub to be resolved as /path/to/sub, of course.  Otherwise, the
> topic did not fix the bug.
>
> Now if a test that wanted to have a clone of the superproject by
> "clone .", which results in the base URL of /path/to/dir/., actually
> wants to refer in its .gitmodules to /path/to/dir/sub (which after
> all was where the submodule the test created with or without the
> bugfix), I would think the right adjustment for the test that used
> to rely on the buggy behaviour would be to stop using ../sub and
> instead use ./sub as the relative URL, no?  After all, the bug forced
> the original test writer to write ../sub but the submodule in this
> case actually is at ./sub relative to its superproject, and that is
> what the original test writer would have written if the bug weren't
> there in the first place, no?

True.

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

Now in the super-clone the ../submodule is the correct relative url, because the url where we cloned from doesn't end in /.

If we were to fix the bug with the /. ending, then we would need the following:

    git clone . super
    git -C super submodule add ./submodule
    # correct relative URL above!

    git clone super super-clone
    cd superclone && sed s|url =./|url = ../|
    # make sure we fix the relative URLs to account for a different remote.

And this doesn't seem right to me as it burdens the user of the super-clone.

Show 12 quoted lines

>
> Another thing I do not quite understand is why this step comes
> before the fix.  If the "clone ." is adjusted to avoid triggering
> the buggy behaviour, i.e. making the base URL to /path/to/dir
> (instead of /path/to/dir/.), wouldn't the relative URL ../sub that
> was written to work around the bug that hasn't been fixed yet in
> this step need to be adjusted anyway?  It would end up referring to
> /path/to/sub and not /path/to/dir/sub until the fix is put in place.
>
> Is the removal of remote.origin.url a wrong workaround for that
> breakage, I wonder...  I do not understand that change at all, and I
> do not think it was explained in the log message.

I think it is wrong, because it is sidestepping the actual issue. Continuing from above:

    git clone super-clone super-clone2
    # this works again, as the remote change doesn't matter.

    mkdir test && git -C test clone ../ .
    # submodule urls need to be "undone again to work:
    cd test && sed s|url =../|url = ./|

So I think keeping the /. around as it currently is, is a pretty useful bug.

Show 7 quoted lines

>
> If we really wanted to update the test before fixing the bug, I
> would understand if this step changed ../sub (or whatever relative
> URL that has extra ../ only because the base URL has extra /. at the
> end to compensate for the buggy resolution) to ./sub in the tests
> and marked them to expect failure, and then a later step that fixes
> the bug turns them to expect success without make any other change.

I'll think about this further, but currently I am inclined to declare it a nonbug and as it eases testing a lot. Also if someone wants to clone a repository with broken relative urls, they can work around that by e.g.

    git clone <url>/.

to fix one level of indentation, though it is not documented and seems to be brittle.

Thanks, Stefan