Re: [PATCH] rebase -i: fix numbering in squash message

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 15, 2018, 18:05 UTC
Message-ID: <xmqqbma3ijyq.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180815094125.12530-1-phillip.wood@talktalk.net>

Phillip Wood <phillip.wood@talktalk.net> writes:

Show 34 quoted lines

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Commit e12a7ef597 ("rebase -i: Handle "combination of <n> commits" with
> GETTEXT_POISON", 2018-04-27) changed the way that individual commit
> messages are labelled when squashing commits together. In doing so a
> regression was introduced where the numbering of the messages is off by
> one. This commit fixes that and adds a test for the numbering.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  sequencer.c                | 4 ++--
>  t/t3418-rebase-continue.sh | 4 +++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 2eb5ec7227..77d3c2346f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1387,13 +1387,13 @@ static int update_squash_messages(enum todo_command command,
>  		unlink(rebase_path_fixup_msg());
>  		strbuf_addf(&buf, "\n%c ", comment_line_char);
>  		strbuf_addf(&buf, _("This is the commit message #%d:"),
> -			    ++opts->current_fixup_count);
> +			    ++opts->current_fixup_count + 1);
>  		strbuf_addstr(&buf, "\n\n");
>  		strbuf_addstr(&buf, body);
>  	} else if (command == TODO_FIXUP) {
>  		strbuf_addf(&buf, "\n%c ", comment_line_char);
>  		strbuf_addf(&buf, _("The commit message #%d will be skipped:"),
> -			    ++opts->current_fixup_count);
> +			    ++opts->current_fixup_count + 1);
>  		strbuf_addstr(&buf, "\n\n");
>  		strbuf_add_commented_lines(&buf, body, strlen(body));
>  	} else

Good spotting. When viewed in a wider context (e.g. "git show -W" after applying this patch), the way opts->current_fixup_count is used is somewhat incoherent and adding 1 to pre-increment would make it even more painful to read. Given that there already is

		strbuf_addf(&header, _("This is a combination of %d commits."),
			    opts->current_fixup_count + 2);

before this part, the code should make it clear these three places refer to the same number for it to be readable.

I wonder if it makes it easier to read, understand and maintain if there were a local variable that gets opts->current_fixup_count+2 at the beginning of the function, make these three places refer to that variable, and move the increment of opts->current_fixup_count down in the function, after the "if we are squashing, do this, if we are fixing up, do that, otherwise, we do not know what we are doing" cascade. And use the more common post-increment, as we no longer depend on the returned value while at it.

IOW, something like this (untested), on top of yours.

 sequencer.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 77d3c2346f..f82c283a89 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1331,8 +1331,9 @@ static int update_squash_messages(enum todo_command command,
 	struct strbuf buf = STRBUF_INIT;
 	int res;
 	const char *message, *body;
+	int fixup_count = opts->current_fixup_count + 2;
 
-	if (opts->current_fixup_count > 0) {
+	if (fixup_count > 2) {
 		struct strbuf header = STRBUF_INIT;
 		char *eol;
 
@@ -1345,7 +1346,7 @@ static int update_squash_messages(enum todo_command command,
 
 		strbuf_addf(&header, "%c ", comment_line_char);
 		strbuf_addf(&header, _("This is a combination of %d commits."),
-			    opts->current_fixup_count + 2);
+			    fixup_count);
 		strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);
 		strbuf_release(&header);
 	} else {
@@ -1387,18 +1388,19 @@ static int update_squash_messages(enum todo_command command,
 		unlink(rebase_path_fixup_msg());
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
 		strbuf_addf(&buf, _("This is the commit message #%d:"),
-			    ++opts->current_fixup_count + 1);
+			    fixup_count);
 		strbuf_addstr(&buf, "\n\n");
 		strbuf_addstr(&buf, body);
 	} else if (command == TODO_FIXUP) {
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
 		strbuf_addf(&buf, _("The commit message #%d will be skipped:"),
-			    ++opts->current_fixup_count + 1);
+			    fixup_count);
 		strbuf_addstr(&buf, "\n\n");
 		strbuf_add_commented_lines(&buf, body, strlen(body));
 	} else
 		return error(_("unknown command: %d"), command);
 	unuse_commit_buffer(commit, message);
+	opts->current_fixup_count++;
 
 	res = write_message(buf.buf, buf.len, rebase_path_squash_msg(), 0);
 	strbuf_release(&buf);