Re: [PATCH 02/17] revert: Inline add_message_to_msg function

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