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

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

[Phillip Wood]

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
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index e9fd029160..ee871dd1bb 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -128,13 +128,15 @@ test_expect_success '--skip after failed fixup cleans commit message' '
 	: The first squash was skipped, therefore: &&
 	git show HEAD >out &&
 	test_i18ngrep "# This is a combination of 2 commits" out &&
+	test_i18ngrep "# This is the commit message #2:" out &&
 
 	(test_set_editor "$PWD/copy-editor.sh" && git rebase --skip) &&
 	git show HEAD >out &&
 	test_i18ngrep ! "# This is a combination" out &&
 
 	: Final squash failed, but there was still a squash &&
-	test_i18ngrep "# This is a combination of 2 commits" .git/copy.txt
+	test_i18ngrep "# This is a combination of 2 commits" .git/copy.txt &&
+	test_i18ngrep "# This is the commit message #2:" .git/copy.txt
 '
 
 test_expect_success 'setup rerere database' '
-- 
2.18.0

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

[Junio C Hamano]

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

> 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);

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

[Phillip Wood]

Hi Junio

On 15/08/2018 19:05, Junio C Hamano wrote:
> 
> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
>> 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.

I think you'd need to change commit_staged_changes() as well as it
relies on the current_fixup_count counting the number of fixups, not the
number of fixups plus two. Having said that using 'current_fixup_count +
2' to create the labels and incrementing the count at the end of
update_squash_messages() would probably be clearer than my version. I'm
about to go away so it'll probably be the second week of September
before I can re-roll this, will that be too late for getting it into 2.19?

Best Wishes

Phillip

> 
>  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);
> 
> 

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

[Junio C Hamano]

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

>> 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.
>
> I think you'd need to change commit_staged_changes() as well as it
> relies on the current_fixup_count counting the number of fixups, not the
> number of fixups plus two.

I suspect you misread what I wrote (see below for the patch).  

The fixup_count is a new local variable in update_squash_messages()
that holds N+2; in other words, I am not suggesting to change what
opts->current_fixup_count means.

> Having said that using 'current_fixup_count +
> 2' to create the labels and incrementing the count at the end of
> update_squash_messages() would probably be clearer than my version. I'm
> about to go away so it'll probably be the second week of September
> before I can re-roll this, will that be too late for getting it into 2.19?

I actually do not mind to go with what you originally sent, unless a
cleaned up version is vastly more readable.  After all, a clean-up
can be done separately and safely.

Thanks.

>>  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);
>> 
>> 

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

[Johannes Schindelin]

Hi,

On Wed, 15 Aug 2018, Junio C Hamano wrote:

> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
> >> 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.
> >
> > I think you'd need to change commit_staged_changes() as well as it
> > relies on the current_fixup_count counting the number of fixups, not the
> > number of fixups plus two.
> 
> I suspect you misread what I wrote (see below for the patch).  

I had the same reaction as Phillip: is your patch good enough, or does it
only touch one part, but not other that may need the same "touch-upping".

> The fixup_count is a new local variable in update_squash_messages()
> that holds N+2; in other words, I am not suggesting to change what
> opts->current_fixup_count means.

Sure, and the better cleanup could possibly be to change the meaning of
opts->current_fixup_count altogether.

> > Having said that using 'current_fixup_count +
> > 2' to create the labels and incrementing the count at the end of
> > update_squash_messages() would probably be clearer than my version. I'm
> > about to go away so it'll probably be the second week of September
> > before I can re-roll this, will that be too late for getting it into 2.19?
> 
> I actually do not mind to go with what you originally sent, unless a
> cleaned up version is vastly more readable.  After all, a clean-up
> can be done separately and safely.

At this point, I think Phillip's version would be safer, as it would make
it easier to do a more complete cleanup without the pressure of having to
fix a bug in one big hurry.

So: ACK on Phillip's patch from me.

Ciao,
Dscho