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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html