Re: [PATCH] Change "refs/" references to symbolic constants

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 2, 2007, 19:47 UTC
Message-ID: <7vsl4tjo28.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20071002191104.GA7901@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

> On Tue, Oct 02, 2007 at 07:16:43PM +0100, Andy Parkins wrote:
>
>> Changed repeated use of the same constants for the ref paths to be
>> symbolic constants.  I've defined them in refs.h
>
> I've manually inspected the patch. Comments are below.
>
>> -		if (prefixcmp(head, "refs/heads/"))
>> -			die("HEAD not found below refs/heads!");
>> -		head += 11;
>> +		if (prefixcmp(head, PATH_REFS_HEADS))
>> +			die("HEAD not found below " PATH_REFS_HEADS "!");
>> +		head += STRLEN_PATH_REFS_HEADS;
>
> This slightly changes the message (extra "/"), but I don't think that is
> a big deal...

        die("HEAD not found below %.*%s!",
             PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS-1)

Show 5 quoted lines

>> -		if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p)
>> +		if (snprintf(ref, sizeof(ref), PATH_REFS_TAGS"%s", *p)
>
> I find the 'PATH_REFS_TAGS "%s"' (with a space) you used earlier a
> little easier to read.

Even though we all know that PATH_REFS_* do not have any '%' in them, it is somewhat unnerving to see such an opaque string in the format specifier part of _any_printf() function. It just makes you think twice, disrupting the flow of thoughts.

This applies to die() and friends as well; see my above rewrite.

To me, the valid reasons for this kind of rewrite are if:

 - it makes typo harder to make and easier to spot
   (e.g. "refs/head/");

 - it makes miscount harder to make and easier to spot (e.g.
   what is this magic constant 11? Is it strlen("refs/heads/")?);

 - it makes reviewing the resulting code, and more importantly,
   future patches on the resulting code, easier.

 - it makes it easier for us to later revamp the strings
   wholesale (e.g. "refs/heads/" => "refs/branches/").

 - it saves us repeated instances of the same string constant;
   using C literal string as values for PATH_REFS_HEADS would
   not help and you would need (const char []) strings instead,
   but the compiler may be clever enough to do so.

Unquestionably, this series helps on the first two counts.

It however actively hurts on the third count. These long constants in CAPITAL_LETTERS_WITH_UNDERSCORE shout too loudly to the eye, overwhelming the surrounding code. I wonder if we can do anything about this point to resurrect the first two benefits, which I like very much.

The forth is a myth we shouldn't care about. If we later would want to change refs/heads to refs/branches, we would want to rename PATH_REFS_HEADS to PATH_REFS_BRANCHES at the same time as well, so the kind of rewrite this patch does does not buy us anything there. More importantly, such a change would need to be made in a backward compatible way (e.g. "if we have heads then keep using them but in new repositories we favor branches"), so it won't be straight token replacement anyway.

And the fifth do not apply to us. This matters only if we were an embedded application on memory starved machine and string constants are far smaller matter compared to the amount of other data we use in-core.