Re: [PATCH] for-each-ref: remove multiple xstrdup() in get_short_ref()

From: Jeff King <peff@peff.net>
Date: Apr 7, 2009, 21:41 UTC
Message-ID: <20090407214144.GA17962@coredump.intra.peff.net>
In-Reply-To: <20090407074435.GB7327@coredump.intra.peff.net>

On Tue, Apr 07, 2009 at 03:44:35AM -0400, Jeff King wrote:

Show 6 quoted lines

> +		if (1 != sscanf(ref, scanf_fmts[i], short_name))
> [...]
> +		if (j == i) {
> +			ref += strlen(ref) - strlen(short_name);
> +			break;
> +		}

Actually, I am not sure this is correct, either. It is making the assumption that the short_name is always a suffix of the ref. But one of the rev_parse_rules is:

     "refs/remotes/%.*s/HEAD"

for which this is not true. _But_ as it happens, this rule doesn't actually work in reverse because of the way scanf works with %s. The code:

    sscanf("refs/remotes/origin/HEAD", "refs/remotes/%s/HEAD", buf);

will put "origin/HEAD" in buf, not "origin"; %s eats until it sees whitespace (which should not be occuring in a ref, fortunately).

So it actually _is_ correct to assume with the current code that the short_name is always a suffix, but I am not sure if that is what we actually want. We will always see "$remote/HEAD" instead of "$remote".

Part of me actually thinks the "incorrect" behavior we are doing now is actually more explicit and readable. But if that is the case, we should perhaps simply be excluding that final rule explicitly, and then our "suffix" assumption will hold.

-Peff