Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames

[Johan Herland <johan@herland.net>]

On Mon, May 13, 2013 at 10:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Johan Herland <johan@herland.net> writes:
>>> Obviously, I named it '%1' since it expands into the _first_ component
>>> of the (slash-separated) shorthand.
>>
>> OK, I can buy something like
>>
>>       %*
>>         refs/%*
>>         refs/heads/%*
>>         ...
>>         refs/remotes/%*/HEAD
>>         refs/remotes/%1/%2
>>         refs/peers/%1/heads/%2
>>
>> that is, for a pattern that has %*, we feed the end-user string as a
>> whole, and for a pattern that has %1 thru %N, we find an appropriate
>> way to chop the end-user string into N pieces (e.g. nick/name would
>> be split into %1 = nick, %2 = name, while foo/bar/baz might have two
>> possibilities, <%1, %2> = <foo, bar/baz> or <foo/bar, baz>).  The
>> earlier ones on the above list can even be written with their %*
>> substituted with %1 if we go that route.
>
> Just to make sure.
>
> Please do not let "two possibilities" stop you.  As I said in the
> nearby thread, I do not necessarily insist that we must try all N
> possibilities.  "We find an appropriate way" could be just "we
> always chop at the first slash, and %1 is what comes before it, %2
> is what comes after it".
>
> That will close the possibility for us to use %1 thru %N (N is
> limited to 2), but it still is "We have %1 and we have %2, both fall
> into the same 'path components, numbered from left to right'
> category", and justifies the use of %1 ("one", not "el").
>
> So still no objection to %1 from me.

I think I like "refs/peers/%1/heads/%*" better than
"refs/peers/%1/heads/%2", since the latter sort of makes me wonder
whether the 3rd, 4th, etc. components would be discarded. That said,
having %* mean "the rest of the shorthand" means that we must adjust
the expansion of %* for every preceding %N, which prevents us from
simplifying the code by using strbuf_expand_dict_cb() with a static
dictionary [1].

I am not sure why we would want "refs/remotes/%1/%2" instead of
"refs/remote/%*". Maybe I've been staring at this for too long, but I
find the latter shorter and more descriptive and the "%1/%2" notation
needlessly cumbersome, especially if it's also supposed to match
"foo/bar/baz"

When it comes to multi-level remote names, I guess I could drop the
patch that disallows them, and still just have "%1" only map to the
first component (i.e. "foo/bar/baz" would always be interpreted as %1
= "foo", and never %1 = "foo/bar"). This would mean that the
"foo/bar/baz" shorthand notation would simply not work against
remote-tracking branch "baz" from remote "foo/bar", but we might say
that's ok, because (a) multi-level remote names are so rare, and (b)
the simple workaround of explicitly saying
"refs/peers/foo/bar/heads/baz" would always be available in any case.


...Johan


[1]: Maybe we could use '%N+' to mean "everything starting from
component #N"? Then we could construct the following dictionary the
shorthand "foo/bar/baz":

  "*" -> "foo/bar/baz"
  "1" -> "foo"
  "1+" -> "foo/bar/baz"
  "2" -> "bar"
  "2+" -> "bar/baz"
etc.

But I really think this is overkill. Avoiding having to write our own
expansion helper is not _that_ important.

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