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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 12, 2011, 16:53 UTC
Message-ID: <20110712165302.GA13578@elie>
In-Reply-To: <1310396048-24925-3-git-send-email-artagnon@gmail.com>

Hi,

Ramkumar Ramachandra wrote:

Show 5 quoted lines

> 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

[...]

Show 14 quoted lines

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