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

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> 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".
>
> That's basically the same as before, with "dereferenced" in place of
> "incremented".  An improvement, sure, but it still doesn't answer the
> basic questions like "how can I reproduce the bug?".

Thanks for poking -- I figured out what's going on.  The msg.message
doesn't just contain the commit message; it contains information like
tree, parent etc (all the information shown by cat-file).  A typical
msg.message looks like:

>>
tree cb35cb5a8736245fd669729ce40d790062f1d74b
parent dcab267d57895e9e8710f5a954e8b9bdfa459c71
author Ramkumar Ramachandra <artagnon@gmail.com> 1311091028 +0530
committer Ramkumar Ramachandra <artagnon@gmail.com> 1311091028 +0530

This is the subject of the message

This is the description of the message
<<

The strstr(msg.message, "\n\n") actually stops after the 0 in the
committer line.  It's trying to find out where the commit message
actually begins.  I even created a commit using commit-tree (not using
porcelain), but it's impossible to find a msg.message that doesn't
have a "\n\n" -- even one with an empty commit message looks like:

>>
tree cb35cb5a8736245fd669729ce40d790062f1d74b
parent dcab267d57895e9e8710f5a954e8b9bdfa459c71
author Ramkumar Ramachandra <artagnon@gmail.com> 1311091028 +0530
committer Ramkumar Ramachandra <artagnon@gmail.com> 1311091028 +0530

<<

The earlier logic was flawed:

	while (*p && (*p != '\n' || p[1] != '\n'))
		p++;

	if (!*p)
		strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));

Here, the !*p will never be satisfied even in the case of the empty
commit message because p reads "\n\n" at this point: the picked commit
will be filled up with an empty commit message as well.  This is also
shown by the tests in t3505-cherry-pick-emtpy.sh.  Perhaps this really
is the desired behavior: why should cherry-pick try to add something
to the commit message when we're picking a commit with an empty commit
message? Now I realize that the new logic is flawed too.  Since it's
totally impossible to reproduce this bug, it's not really a bug --
there's just some dead code in the older implementation, and we want
to remove it and make it clear.

New commit message and commit.

-- 8< --
Subject: [PATCH] revert: Simplify and inline add_message_to_msg

The add_message_to_msg function has some dead code, an unclear API,
only one callsite.  While it originally intended fill up an empty
commit message with the commit object name while picking, it really
doesn't do this -- a bug introduced in 9509af6 (Make git-revert &
git-cherry-pick a builtin, 2007-03-01).  Today, tests in
t3505-cherry-pick-empty.sh indicate that not filling up an empty
commit message is the desired behavior.  Re-implement and inline the
function accordingly.

Helped-by: Junio C Hamano <gitster@pobox.com>
Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   26 ++++++++++++--------------
 1 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 2df3f3b..2b5f261 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -185,19 +185,6 @@ static char *get_encoding(const char *message)
        return NULL;
 }

-static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
-{
-       const char *p = message;
-       while (*p && (*p != '\n' || p[1] != '\n'))
-               p++;
-
-       if (!*p)
-               strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));
-
-       p += 2;
-       strbuf_addstr(msgbuf, p);
-}
-
 static void write_cherry_pick_head(void)
 {
        int fd;
@@ -462,11 +449,22 @@ static int do_pick_commit(void)
                }
                strbuf_addstr(&msgbuf, ".\n");
        } else {
+               const char *p;
+
                base = parent;
                base_label = msg.parent_label;
                next = commit;
                next_label = msg.label;
-               add_message_to_msg(&msgbuf, msg.message);
+
+               /*
+                * Append the commit log message to msgbuf; it starts
+                * after the tree, parent, author, committer
+                * information followed by "\n\n".
+                */
+               p = strstr(msg.message, "\n\n");
+               p += 2;
+               strbuf_addstr(&msgbuf, p);
+
                if (no_replay) {
                        strbuf_addstr(&msgbuf, "(cherry picked from commit ");
                        strbuf_addstr(&msgbuf,
sha1_to_hex(commit->object.sha1));
-- 
1.7.4.rc1.7.g2cf08.dirty