Re: [PATCH 02/13] revert: Factor out add_message_to_msg function
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 21, 2011, 19:01 UTC
- Message-ID
- <7vmxhb80vp.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20110621155836.GG15461@elie>
Jonathan Nieder <jrnieder@gmail.com> writes:
Show 9 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. > > The above does not answer the question I would have, namely what > exactly is wrong with add_message_to_msg. Is it too slow? Not > robust? Is the generated assembly too long? Is it hard for a reader > to figure out the intent? Does it blend in poorly with its > surroundings?
I do not know if I _suggested_ this, but ...
Show 13 quoted lines
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 1f27c63..d6d2356 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++;This is a loop that manually implements strstr("\n\n"); rewriting of which is a Good Thing.
Show 5 quoted lines
> - if (!*p) > - strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1)); > - > - p += 2; > - strbuf_addstr(msgbuf, p);
I am not sure if this corresponds to what the new code does. What happens if *p was NUL (i.e. we did not find any "\n\n" in the "message" at all)? We go two bytes beyond that NUL and run addstr from that unknown space? The new code does not seem to do that ;-)
I suspect that this was a bug from the original C-rewrite of git-revert that was done by 9509af6 (Make git-revert & git-cherry-pick a builtin, 2007-03-01), and the new code fixes the bug, but then it would be nice to note that in the commit log message.
Show 13 quoted lines
> @@ -471,11 +458,17 @@ 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);
> +
> + /* Add msg.message to msgbuf */This is an incorrect comment that confuses the reader. Think:
- What did you skip by looking for the first "\n\n"? - What do you instead when no such "\n\n" is found?
Perhaps what it is doing is: "Add commit log message, if exists, or the commit object name if it doesn't"
Show 6 quoted lines
> + p = p ? p + 2 : sha1_to_hex(commit->object.sha1);
> + strbuf_addstr(&msgbuf, p);
> +
> if (no_replay) {
> strbuf_addstr(&msgbuf, "(cherry picked from commit ");
> strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));