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

[Michael J Gruber <git@drmicha.warpmail.net>]

Jeff King venit, vidit, dixit 09.11.2012 17:48:
> 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.

No problem. This is not urgent, and it takes some effort to look at code
amongst all those black-and-white discussions. [It even takes effort to
refrain from responding when you words are being twisted around...]

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

We could just leave '*p' as it is (after all, that was the user input),
or use 'ref+strlen("refs/replace/")'.

I wasn't aware of the volatile nature of the return value. Thanks for
spotting!

Michael