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

From: Jeff King <peff@peff.net>
Date: Oct 2, 2007, 20:48 UTC
Message-ID: <20071002204848.GA8284@coredump.intra.peff.net>
In-Reply-To: <7vsl4tjo28.fsf@gitster.siamese.dyndns.org>

On Tue, Oct 02, 2007 at 12:47:59PM -0700, Junio C Hamano wrote:

Show 10 quoted lines

>  - 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 however actively hurts on the third count.  These long

Yes, I find some of the substitutions more readable, but some are a bit less readable. The parts of the patch I found the _most_ improved are the ones that get rid of a memcmp in favor of a prefixcmp (i.e., removing the count entirely).

Perhaps a better quest would be to eliminate all of those counts entirely with code that is obviously correct. I think it is much more readable to replace:

  url = xmalloc(strlen(repo->base) + 64);
  sprintf(url, "%s/objects/pack/pack-%s.idx", repo->base, hex);

with something like:

  strbuf_init(&url);
  strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);

which has the same number of lines, but no magic numbers at all. Or since most of the uses of things like PATH_OBJECTS are more or less the same, maybe something like:

  mkpath_object(&url, "pack/pack-%s.idx", hex);

i.e., rather than fiddling with string constants, wrap them functionally.

> constants in CAPITAL_LETTERS_WITH_UNDERSCORE shout too loudly to

Part of the problem is also that they're long. Perhaps REFS_HEADS, while being less unique in the C namespace, would look better?

-Peff