Jonathan Nieder <jrnieder@xxxxxxxxx> writes:
>> 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 ...
> 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.
> - 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.
> @@ -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"
> + 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));
--
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