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

From: Johan Herland <johan@herland.net>
Date: May 13, 2013, 06:31 UTC
Message-ID: <CALKQrgf0m8r-Ofb+Ss1OpEF67dPS73b8nB+usVxH=Y=h3441Wg@mail.gmail.com>
In-Reply-To: <7vmwrzsck1.fsf@alter.siamese.dyndns.org>

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

Show 24 quoted lines

> 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