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

[Jeff King <peff@peff.net>]

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

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

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

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