Re: [PATCHv2] replace: parse revision argument for -d

From: Jeff King <peff@peff.net>
Date: Nov 9, 2012, 16:48 UTC
Message-ID: <20121109164808.GE19725@sigill.intra.peff.net>
In-Reply-To: <24b0f81315ddab8cc37133d5b3bec8aec90ed652.1351516888.git.git@drmicha.warpmail.net>

On Mon, Oct 29, 2012 at 02:23:27PM +0100, Michael J Gruber wrote:

Show 10 quoted lines

> 'git replace' parses the revision arguments when it creates replacements
> (so that a sha1 can be abbreviated, e.g.) but not when deleting
> replacements.
> 
> Make it parse the arguments to 'replace -d' in the same way.
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
> v2 has the simplified error check as per Jeff, and a reworded message.
> Comes with a free test case, too.

I noticed this today in my pile of "to look at" patches. Sorry for being slow.

Show 10 quoted lines

>  	for (p = argv; *p; p++) {
> -		if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p)
> -					>= sizeof(ref)) {
> -			error("replace ref name too long: %.*s...", 50, *p);
> +		q = *p;
> +		if (get_sha1(q, sha1)) {
> +			error("Failed to resolve '%s' as a valid ref.", q);
>  			had_error = 1;
>  			continue;
>  		}

Looks reasonable.

Show 5 quoted lines

> +		q = sha1_to_hex(sha1);
> +		snprintf(ref, sizeof(ref), "refs/replace/%s", q);
>  		if (read_ref(ref, sha1)) {
> -			error("replace ref '%s' not found.", *p);
> +			error("replace ref '%s' not found.", q);

I worry a little about assuming that "q", which points to a static internal buffer of sha1_to_hex, is still valid after calling read_ref. We'll end up in resolve_ref, which might need to do considerable work (e.g., loading the whole packed refs file). Just grepping for sha1_to_hex, I don't think it is a problem currently, but it might be worth copying the value (you could even point into the "ref" buffer to avoid dealing with an extra allocation).

Other than that, it looks good to me.

-Peff