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@xxxxxxxxxxxxxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html