Re: [PATCHv2 04/10] remote: Reject remote names containing '/'

[Johan Herland <johan@herland.net>]

On Mon, May 13, 2013 at 6:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>
>> Although we definitely support and encourage use of multi-level branch
>> names, we have never conciously tried to give support for multi-level
>> remote names. Currently, they are allowed, but there is no evidence that
>> they are commonly used.
>>
>> Now, they do provide a source of problems when trying to expand the
>> "$nick/$name" shorthand notation (where $nick matches a remote name)
>> into a full refname. Consider the shorthand "foo/bar/baz": Does this
>> parse as $nick = foo, $name = bar/baz, or $nick = foo/bar, $name = baz?
>>
>> Since we need to be unambiguous about these things, we hereby declare
>> that a remote name shall never contain a '/' character, and that the
>> only correct way to parse "foo/bar/baz" is $nick = foo, $name = bar/baz.
>
> I know I am guilty of hinting to go in this direction in the earlier
> discussion, but I have to wonder how much more refactoring is needed
> to see if there is only one unique possibility among many.
>
> For a string with N slashes, you have only N possible ways to split
> it into $nick and $name, and if you see a ref "bar/baz" copied from
> remote "foo" but no ref "baz" copied from remote "foo/bar" (or you
> may not even have a remote "foo/bar" in the first place), the user
> is already very unambiguous. The declaration is merely being lazy.
>
> I am not saying we must implement such a back-track to disambiguate
> the user input better.  I am wondering how much more effort on top
> of this series is needed if we want to get there (provided that the
> disambiguation is a good thing to do in the first place).

I feel the problem with multi-level remote names runs a little deeper
than merely disambiguation when resolving remote-tracking refs: If you
have two remotes "foo" and "foo/bar", and they have branches "bar/baz"
and "baz", respectively, then they will (in the default current
configuration) end up clobbering eachother due to the overlapping
remote-tracking branch (refs/remotes/foo/bar/baz). Although the remote
ref namespace coincidentally resolves this by mapping the two to
"refs/peers/foo/heads/bar/baz" and "refs/peers/foo/bar/heads/baz"
respectively, you can easily create a different (although probably
even more unlikely) case where the remote ref namespace causes the
same kind of overlap: One remote "foo" with branch "heads/bar", and
another remote "foo/heads" with branch "bar" will both end up
clobbering eachother at "refs/peers/foo/heads/heads/bar"...

The disambiguation can probably be resolved, although the resulting
code will obviously be somewhat more cumbersome and ugly (and IMHO the
current code is plenty of that already...). Combine this with the
problems of clobbering of the same remote-tracking ref (describe
above), and the fact that AFAIK a multi-level remote name has never
been observed "in the wild" (none of the people I asked at the Git
Merge conference had ever observed multi-level remote names, nor did
they directly oppose banning them), I'm not at all sure it's worth
bothering about this at all. Simply disallowing multi-level remote
names seems like the simpler and naturally ambiguity-resistant
approach.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net