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

From: Jeff King <peff@peff.net>
Date: Oct 2, 2007, 15:58 UTC
Message-ID: <20071002155800.GA6828@coredump.intra.peff.net>
In-Reply-To: <200710020941.05288.andyparkins@gmail.com>

On Tue, Oct 02, 2007 at 09:41:03AM +0100, Andy Parkins wrote:

Show 5 quoted lines

> 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 wonder if you could check the patch mechanically (i.e., write a script to confirm that '5' got replaced by tokens equal to '5'), though that might require some tricky parsing of C.

If you a post an updated version, I will try to read through it carefully, as two eyes are better than one (er, four eyes, I guess).

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

When in doubt, I would suggest a separate patch, as it makes the review easier.

Show 5 quoted lines

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

Yes, you should definitely listen to Junio on such issues, and not me. :)

-Peff