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

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to
More majordomo info at

Reply via email to