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

From: Jeff King <peff@peff.net>
Date: Oct 2, 2007, 01:16 UTC
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:

Show 17 quoted lines

> 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