Hi Jonathan,
Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>
>> The add_message_to_msg function is poorly implemented, has an unclear
>> API, and only one callsite. Replace the callsite with a cleaner
>> implementation. Additionally, fix a bug introduced in 9509af6 (Make
>> git-revert & git-cherry-pick a builtin, 2007-03-01) -- a NULL pointer
>> was being incremented when "\n\n" was not found in "message".
>
> Rather than being an optimization, the main impact of this change is
> to avoid a NULL pointer dereference in some cases, right?
>
> If so, the subject line should say so. Is it possible to reproduce
> this? Could we add a test to avoid regressing in the future?
>
> Less importantly:
>
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
> [...]
>> @@ -462,11 +449,16 @@ static int do_pick_commit(void)
>> }
>> strbuf_addstr(&msgbuf, ".\n");
>> } else {
>> + const char *p = strstr(msg.message, "\n\n");
>> +
>> base = parent;
>> base_label = msg.parent_label;
>> next = commit;
>> next_label = msg.label;
>> - add_message_to_msg(&msgbuf, msg.message);
>> +
>> + p = p ? p + 2 : sha1_to_hex(commit->object.sha1);
>> + strbuf_addstr(&msgbuf, p);
>
> I think this would be clearer like so:
>
> const char *p;
> ...
> p = strstr(...);
> if (p)
> p += 2;
> else
> p = sha1_to_hex...
> strbuf_addstr(&msgbuf, p);
>
> i.e., putting all the code that manipulates p together. Besides,
> pre-C99 compilers don't like "p" to be initialized to a non-constant.
> :)
>
> At [1], I also see a suggestion of a comment that could clarify the
> code a little.
Fixed all issues. The commit message now reads
revert: Inline add_message_to_msg function
The add_message_to_msg function is poorly implemented, has an unclear
API, and only one callsite. Replace the callsite with a cleaner
implementation. Additionally, fix a bug introduced in 9509af6 (Make
git-revert & git-cherry-pick a builtin, 2007-03-01) -- a NULL pointer
was being dereferenced when "\n\n" was not found in "message".
Thanks.
-- Ram
--
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