Jeff King <peff@xxxxxxxx> writes:
> 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)
>> - 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.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
