From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Wed, 13 Jul 2011 06:00:40 GMT
Subject: Re: [PATCH 02/17] revert: Inline add_message_to_msg function
Message-ID: <CALkWK0kHutr41aH7OrmW3REV+YBUeHSryA9n62yxyF0qRd1FEw@mail.gmail.com>
In-Reply-To: <20110712165302.GA13578@elie>

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