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

From: Junio C Hamano <gitster@pobox.com>
Date: May 13, 2013, 04:56 UTC
Message-ID: <7vmwrzsck1.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1368289280-30337-4-git-send-email-johan@herland.net>

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.