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

From: Andy Parkins <andyparkins@gmail.com>
Date: Oct 2, 2007, 08:41 UTC
Message-ID: <200710020941.05288.andyparkins@gmail.com>
In-Reply-To: <20071002011659.GA7938@coredump.intra.peff.net>

On Tuesday 2007 October 02, Jeff King wrote:

Show 9 quoted lines

> -		    patlen != namelen - 5 &&
> -		    prefixcmp(name, "refs/heads/") &&
> -		    prefixcmp(name, "refs/tags/")) {
> +		    patlen != namelen - STRLEN_PATH_REFS_HEADS &&
> +		    prefixcmp(name, PATH_REFS_HEADS) &&
> +		    prefixcmp(name, PATH_REFS_HEADS)) {
>
> This is totally bogus. You meant STRLEN_PATH_REFS, and the second path
> should be PATH_REFS_TAGS. With those changes, t5516 passes.

Excellent! Well done. I spent a couple of hours last night going through every changed line and have spotted the TAGS mistake but didn't spot the STRLEN being wrong. Amazing how easy it is to become blind to these things. There were a couple of errors in "/" placement too, but I don't think they were causing any trouble, just doubled up "/" characters.

> I haven't combed through your patch in detail, so there might be similar
> problems lurking. I did notice one or two spots where you call
> strlen(PATH_REFS_*), which should of course also be changed to
> STRLEN_PATH_REFS_*.

I think I caught a few of them in my review. I think I had originally left them like that on purpose. My reasoning was that a patch like that should leave the resultant code identical. So I did replacements like:

- strlen("refs/heads/");
+ strlen(PATH_REFS_HEADS);

However, I think it was just pedantry, so I've been correcting them too.

I noticed a couple of places where memcmp() has been used where prefixcmp() would work fine. I'm tempted to change them too - what do you think? Perhaps a separate patch?

> And as a final comment, your patch doesn't apply to next at all because
> of the reorganization of the fetching API (e.g., fetch-pack.c doesn't
> exist at all anymore). You should probably prepare a parallel patch for
> next.

I'm happy to do prepare a patch against any revision, I was really waiting for feedback from Junio as to how he'd like to manage it. Last time I submitted this patch he (quite correctly) asked that I delay until after the next point release; of course I promptly found other things to do and never resubmitted :-)

Andy

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