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

[Andy Parkins <andyparkins@gmail.com>]

On Tuesday 2007 October 02, Jeff King wrote:

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