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

On Mon, May 13, 2013 at 6:56 AM, Junio C Hamano [off-list ref] wrote:

Johan Herland [off-list ref] writes:

quoted

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, [off-list ref]
www.herland.net