threads / patch / 49130

patchrebase -i: fix numbering in squash message

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

## tl;dr

5 messages between Aug 15, 2018 and Aug 16, 2018. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Phillip Wood· Aug 15, 2018, 09:41 UTC · lore
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(-)
Show changes to 2 files +5 −3

sequencer.c, t/t3418-rebase-continue.sh

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
Junio C Hamano· Aug 15, 2018, 18:05 UTC · re: Phillip Wood · lore

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

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(-)
Show changes to sequencer.c +6 −4
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);
Phillip Wood· Aug 15, 2018, 18:33 UTC · re: Junio C Hamano · lore

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

Hi Junio
On 15/08/2018 19:05, Junio C Hamano wrote:
Show 59 quoted lines
> 
> 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
Show 52 quoted lines
> 
>  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);
> 
>
Junio C Hamano· Aug 15, 2018, 19:17 UTC · re: Phillip Wood · lore

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

Phillip Wood <phillip.wood@talktalk.net> writes:
Show 14 quoted lines
>> 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.

Show 5 quoted lines
> 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.
Show 51 quoted lines
>>  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);
>> 
>>
Johannes Schindelin· Aug 16, 2018, 08:42 UTC · re: Junio C Hamano · lore

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

Hi,
On Wed, 15 Aug 2018, Junio C Hamano wrote:
Show 18 quoted lines
> 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.

Show 9 quoted lines
> > 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

← back to recent threads