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

From: Andy Parkins <andyparkins@gmail.com>
Date: Oct 3, 2007, 07:37 UTC
Message-ID: <200710030837.25689.andyparkins@gmail.com>
In-Reply-To: <20071002204848.GA8284@coredump.intra.peff.net>

On Tuesday 2007 October 02, Jeff King wrote:

> 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:

I've got a patch replacing every appropriate memcmp() with prefixcmp(), but it goes on top of this one, so wanted to get this through review to save constantly spamming the list with the same patch slightly modified because of changes in a different patch.

Show 7 quoted lines

>   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);

I've not been following the strbuf() changes, so have missed the appearance of these handy new functions. They would appear to be an improvement for cases just like this.

> > 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?

I completely agree with the length and loudness concerns, but my worry was polluting the namespace while maintaining some sort of rationality between PATH_REFS_HEADS and STRLEN_PATH_REFS_HEADS. My reasoning was that

 "refs/heads" -> PATH_REFS_HEADS

is only three extra characters, and

 strlen("refs/heads/") -> STRLEN_PATH_REFS_HEADS

is only one extra character.

However I have no strong feelings about changing them.

Andy

-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com