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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

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.

Hope that helps,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/176139/focus=176183