On Sat, Oct 22, 2016 at 12:33 AM, Junio C Hamano <[email protected]> wrote:
> Johannes Sixt <[email protected]> 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)
>
> 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.
>
> 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.
>
> 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
