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

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