Re: [PATCH v2 2/2] submodules: always use a relative path from gitdir to work tree

From: Jens Lehmann <jens.lehmann@web.de>
Date: Feb 15, 2012, 22:18 UTC
Message-ID: <4F3C2F3F.7000203@web.de>
In-Reply-To: <7vzkcl5f37.fsf@alter.siamese.dyndns.org>

Am 14.02.2012 21:34, schrieb Junio C Hamano:

Show 17 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>
>>> After adding a comment, using test instead of [], testing both $a and
>>> $b and assigning each variable on it's own line I get the following
>>> interdiff. Does that make more sense?
>>
>> My earlier request for comment was to say
>>
>> 	# $a is always longer than $b for such and such reasons
>>
>> to explain why testing $b without testing $a was sufficient.
> 
> Heh, after I follow the entire module_clone, $gitdir is defined in earlier
> parts of the function to be "$(rev-parse --git-dir)/modules/$path", so it
> is clear that it is longer than $path.

Unfortunately only by accident. The usage of $path is not correct here, $name should be used instead (I have a patch in the making to correct that, but as that hits the same code area as these fixes I'll post that later together with some tests moving submodules around inside a superproject). Then the result of "$(rev-parse --git-dir)/modules/$name" can be shorter than "$path" when a submodule is renamed into a higher directory level.

Show 7 quoted lines

> Unless "cd $there && pwd" does not
> result in a funny situation (such as $something/modules is a symbolic link
> to another place that is much closer to the root of the filesystem), that
> is.
> 
> And in such a case, the prefix part of $a and $b would be different from
> the very beginning hopefully.

Yes, they should differ somewhere in any sane setup I can imagine.

Show 32 quoted lines

>> It is obvious (at least to me) that the loop continues as long as $a and
>> $b begin with the same string before their first '/' and removes that
>> common segment from both of them, so I do not think the new comment is
>> absolutely necessary, but it would not hurt to have it, especially it is
>> short enough and to the point.
>>
>> Thanks.
>>
>>> diff --git a/git-submodule.sh b/git-submodule.sh
>>> index 3463d6d..ed76ce2 100755
>>> --- a/git-submodule.sh
>>> +++ b/git-submodule.sh
>>> @@ -172,9 +172,11 @@ module_clone()
>>>
>>>         a=$(cd "$gitdir" && pwd)
>>>         b=$(cd "$path" && pwd)
>>> -       while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
>>> +       # Remove all common leading directories
>>> +       while test -n "$a" && test -n "$b" && test "${a%%/*}" = "${b%%/*}"
>>>         do
>>> -               a=${a#*/} b=${b#*/};
>>> +               a=${a#*/}
>>> +               b=${b#*/}
>>>         done
>>>         rel=$(echo $a | sed -e 's|[^/]*|..|g')
> 
> Perhaps aseert that $a never becomes empty before this line (or set it
> explicitly to "." when $a is empty), as otherwise
> 
>>>         (clear_local_git_env; cd "$path" && git config core.worktree "$rel/$b")
> 
> this will refer to "/$b" from the root?

I think neither $a nor $b should be empty after that. But thinking deeper about that while loop I suspect the real problem here is doing "a=${a#*/}" or "b=${b#*/}" on a string that doesn't contain a slash anymore. We'll happily remove a leading directory on the other path while the one without slash will stay untouched, leading to a bogus result which is off by one directory level.

AFAICS that will only happen when one path is a prefix of the other, which is a pretty pathological case. So I'll whip up a new version asserting that beforehand and dropping the -n test in the while loop, ok?