From: Jeff King <peff@peff.net>
Date: Tue, 02 Oct 2007 01:16:59 GMT
Subject: Re: [PATCH 1/2] Change "refs/" references to symbolic constants
Message-ID: <20071002011659.GA7938@coredump.intra.peff.net>
In-Reply-To: <200710012141.44459.andyparkins@gmail.com>

On Mon, Oct 01, 2007 at 09:41:43PM +0100, Andy Parkins wrote:

> Please hold off on applying this.  I'm getting this when running the tests:
> 
> *** t5516-fetch-push.sh ***
> *   ok 1: setup
> *   ok 2: fetch without wildcard
> *   ok 3: fetch with wildcard
> *   ok 4: push without wildcard
> *   ok 5: push with wildcard
> *   ok 6: push with matching heads
> *   ok 7: push with no ambiguity (1)
> *   ok 8: push with no ambiguity (2)
> *   ok 9: push with weak ambiguity (1)
> *   ok 10: push with weak ambiguity (2)
> *   ok 11: push with ambiguity (1)
> * FAIL 12: push with ambiguity (2)
> 
> I'm having trouble seeing where the fault is at the moment though.

>From your patch:

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

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_*.

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.

-Peff