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 6:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>
>> The refname_expand() function no longer uses mkpath()/mksnpath() to
>> perform the pattern expansion. Instead, it uses strbuf_expand(), which
>> removes the need for using fixed-length buffers from the code.
>
> It is a brilliant idea to use strbuf_expand() for this. I like it.
>
> I notice that you later introduce %1 (that is 'one', not 'el'), but
> unless you are planning to introduce %2 and %3 that semantically
> fall into a similar category as %1, I would rather see a different
> letter used that is mnemonic to what the placeholder _means_.
>
> The choice of the letter is arbitrary and may not look like it
> matters that much, because it is not exposed to the end user.  But
> by switching from the sprintf() semantics that shows things given to
> it in the order they were given, without knowing what they mean, and
> introducing a strbuf_expand() machinery tailored for refnames (and
> refnames only), the new code assigns meanings to each part of the
> refname, and we can afford to be more descriptive.
>
> The choice of '%*' is justifiable, "it is the closest to the '*' we
> traditionally used to replace only one thing", but '%1' does not
> look the best placeholder to use, at least to me.

Obviously, I named it '%1' since it expands into the _first_ component
of the (slash-separated) shorthand. There is no further parsing or
verification that it actually corresponds to a remote (and as far as I
currently understand, we do not want to do such verification), so I
thought it better not to make such assumptions in the placeholder
name. That said, I could go with '%r' for "remote", although we have
plenty of other concepts in Git that use 'r' as the initial letter. I
could maybe use '%remote' instead?

Also, about the '%*': When used alone, it means "the entire
shorthand", but when preceded with a '%1' it subtly changes meaning
into 'the remainder of the shorthand after extracting the first
component'. I believe the two interpretations are compatible and
unambiguous, but if we want to be very explicit about what's
happening, we could use something like '%all' and '%the_rest' for the
two cases, respectively?


...Johan

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