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

From: Johan Herland <johan@herland.net>
Date: May 13, 2013, 06:53 UTC
Message-ID: <CALKQrgcry9bwmonaeWA4M7a3k36S_Q3ZQLmv7Ui5r+tdzdMr_A@mail.gmail.com>
In-Reply-To: <7vtxm7scn5.fsf@alter.siamese.dyndns.org>

On Mon, May 13, 2013 at 6:54 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 30 quoted lines

> 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