[PATCH 0/2] rebase: a couple of fixup fixes

[PATCH 0/2] rebase: a couple of fixup fixes

[Phillip Wood]

These patches fix a couple of small bugs in the way skipped "fixup"
and "squash" commands are handled. A skipped command can lead to
an incorrect commit count in the template message which is fixed in
patch 1. It can also mean we fail to open the editor after a "fixup
-c" command which is fixed in patch 2

base-commit: d35c5399e3e54ac277bb391fc2f6be3e816d312b
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-fixup-fixes-part-1%2Fv1
View-Changes-At: https://github.com/phillipwood/git/compare/d35c5399e...7c8075ff2
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-fixup-fixes-part-1/v1


Phillip Wood (2):
  rebase -i: fix counting of fixups after rebase --skip
  rebase: remember fixup -c after skipping fixup/squash

 sequencer.c                     | 31 ++++++++++++++++++----
 t/t3418-rebase-continue.sh      | 36 ++++++++++++++++++++++---
 t/t3437-rebase-fixup-options.sh | 47 +++++++++++++++++++++++++++++++++
 3 files changed, 105 insertions(+), 9 deletions(-)

-- 
2.54.0.200.gfd8d68259e3

[PATCH 1/2] rebase -i: fix counting of fixups after rebase --skip

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When the sequencer processes a chain of "fixup" and "squash" commands
it keeps a list of the commands that have been executed. If there are
conflicts, then the list is saved when the rebase stops for the user to
resolve them. When the rebase resumes, the list is loaded and is used
to initialize the count of how many "fixup" and "squash" commands have
been processed; if a command has been skipped with "git rebase --skip",
then the last command needs to be popped off the end of the list.

To count the number of commands, commit_staged_changes() uses the
number of newlines in the file plus one. This is due to the slightly
unusual way the list is constructed - instead of appending a newline
when a command is added, a newline is inserted before the command
if the current count is greater than zero. Therefore, when we pop a
skipped command off the list, we should also remove the newline that
precedes it. Otherwise, when a new command is added, a blank line
will be left before it, which will contribute to the fixup count the
next time the file is read. Unfortunately, the preceding newline is
not removed, leading to an incorrect count. Fix this by removing the
newline that appears before the skipped command.

In addition to fixing the code that removes a skipped command from the
list, the code that reads the list is fixed to skip blank lines. We
have had reports of users starting a rebase with one version of
git and continuing it with another. Often this happens because the
version of git bundled with an IDE or TUI differs from the one used
at the command line. By fixing both the reading and writing ends of
the problem we ensure the count is correct when an older version of
git reads the fixup file written by a newer version and vice versa.

Triggering the incorrect count requires the user to skip two "fixup" or
"squash" commands before the final command in the chain. An existing
test is extended to prevent future regressions. The consequence of
miscounting is not serious: we just print the wrong count in the
header of the commit message template.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                | 11 ++++++++++-
 t/t3418-rebase-continue.sh | 36 ++++++++++++++++++++++++++++++++----
 2 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 1355a99a092..af3d2c72616 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3281,7 +3281,13 @@ static int read_populate_opts(struct replay_opts *opts)
 			const char *p = ctx->current_fixups.buf;
 			ctx->current_fixup_count = 1;
 			while ((p = strchr(p, '\n'))) {
-				ctx->current_fixup_count++;
+				/*
+				 * Older versions of git accidentally
+				 * inserted blank lines when a fixup
+				 * was skipped.
+				 */
+				if (p[1] != '\n')
+					ctx->current_fixup_count++;
 				p++;
 			}
 		}
@@ -5353,6 +5359,9 @@ static int commit_staged_changes(struct repository *r,
 			if (!len)
 				BUG("Incorrect current_fixups:\n%s", p);
 			while (len && p[len - 1] != '\n')
+				len--;
+			/* Remove trailing newline */
+			if (len)
 				len--;
 			strbuf_setlen(&ctx->current_fixups, len);
 			if (write_message(p, len, rebase_path_current_fixups(),
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index f9b8999db50..3c248e97364 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -134,6 +134,7 @@ test_expect_success '--skip after failed fixup cleans commit message' '
 	EOF
 
 	: skip and continue &&
+	test_config commit.status false &&
 	echo "cp \"\$1\" .git/copy.txt" | write_script copy-editor.sh &&
 	(test_set_editor "$PWD/copy-editor.sh" && git rebase --skip) &&
 
@@ -145,7 +146,8 @@ test_expect_success '--skip after failed fixup cleans commit message' '
 
 	: now, let us ensure that "squash" is handled correctly &&
 	git reset --hard wants-fixup-3 &&
-	test_must_fail env FAKE_LINES="1 squash 2 squash 1 squash 3 squash 1" \
+	test_must_fail env \
+		FAKE_LINES="1 squash 2 squash 1 squash 3 squash 1 squash 4 squash 1" \
 		git rebase -i HEAD~4 &&
 
 	: the second squash failed, but there are two more in the chain &&
@@ -171,19 +173,45 @@ test_expect_success '--skip after failed fixup cleans commit message' '
 	fixup 2
 	EOF
 
+	(test_set_editor "$PWD/copy-editor.sh" &&
+	 test_must_fail git rebase --skip) &&
+	: not the final squash, no need to edit the commit message &&
+	test_path_is_missing .git/copy.txt &&
+
+	: The first, third and fifth squashes succeeded, therefore: &&
+	cat >expect <<-\EOF &&
+	# This is a combination of 4 commits.
+	# This is the 1st commit message:
+
+	wants-fixup
+
+	# This is the commit message #2:
+
+	fixup 1
+
+	# This is the commit message #3:
+
+	fixup 2
+
+	# This is the commit message #4:
+
+	fixup 3
+	EOF
+	test_commit_message HEAD expect &&
+
 	(test_set_editor "$PWD/copy-editor.sh" && git rebase --skip) &&
 	test_commit_message HEAD <<-\EOF &&
 	wants-fixup
 
 	fixup 1
 
 	fixup 2
+
+	fixup 3
 	EOF
 
 	: Final squash failed, but there was still a squash &&
-	head -n1 .git/copy.txt >first-line &&
-	test_grep "# This is a combination of 3 commits" first-line &&
-	test_grep "# This is the commit message #3:" .git/copy.txt
+	test_cmp expect .git/copy.txt
 '
 
 test_expect_success 'setup rerere database' '
-- 
2.54.0.200.gfd8d68259e3

Re: [PATCH 1/2] rebase -i: fix counting of fixups after rebase --skip

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> @@ -3281,7 +3281,13 @@ static int read_populate_opts(struct replay_opts *opts)
>  			const char *p = ctx->current_fixups.buf;
>  			ctx->current_fixup_count = 1;
>  			while ((p = strchr(p, '\n'))) {
> -				ctx->current_fixup_count++;
> +				/*
> +				 * Older versions of git accidentally
> +				 * inserted blank lines when a fixup
> +				 * was skipped.
> +				 */
> +				if (p[1] != '\n')
> +					ctx->current_fixup_count++;
>  				p++;
>  			}
>  		}

If we hit the LF at the very end (e.g. "fixup A\n" at the end of the
file), strchr() would have moved p to the newline, and p[1] will be
'\0', no?  And because p[1] != '\n' and wouldn't current_fixup_count
be incremented again?  It might be safer to check p[1] != '\n' &&
p[1] != '\0' to avoid counting a trailing newline as an extra
command when reading legacy files.

> @@ -5353,6 +5359,9 @@ static int commit_staged_changes(struct repository *r,
>  			if (!len)
>  				BUG("Incorrect current_fixups:\n%s", p);
>  			while (len && p[len - 1] != '\n')
> +				len--;
> +			/* Remove trailing newline */
> +			if (len)
>  				len--;

So we removed all the non newline from the end, and the loop would
break if !len or p[len - 1] == '\n'.  And in the latter case, we
also drop that '\n'.  Which sounds right.

Thanks.

[PATCH 2/2] rebase: remember fixup -c after skipping fixup/squash

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When the final command in a chain of "fixup" and "squash" commands
is skipped, we should prompt the user to edit the commit message
if the chain contains a "fixup -c" command that was not skipped.
Unfortunately, commit_staged_changes() only looks for completed "squash"
commands and so does not prompt the user to edit the message. Fix
this by recording whether a fixup command has the "-c" flag set and
then checking whether we have seen either a "fixup -c" or a "squash"
command. Add regression tests for skipping a command in the middle
of the chain (which currently works but has no test coverage), and
for skipping the final command (which is fixed by this patch).

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                     | 20 +++++++++++---
 t/t3437-rebase-fixup-options.sh | 47 +++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index af3d2c72616..25ef076216c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1924,6 +1924,13 @@ static int seen_squash(struct replay_ctx *ctx)
 {
 	return starts_with(ctx->current_fixups.buf, "squash") ||
 		strstr(ctx->current_fixups.buf, "\nsquash");
+}
+
+/* Does the current fixup chain contain a "fixup -c" command? */
+static int seen_fixup_edit_msg(struct replay_ctx *ctx)
+{
+	return starts_with(ctx->current_fixups.buf, "fixup -c") ||
+		strstr(ctx->current_fixups.buf, "\nfixup -c");
 }
 
 static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)
@@ -2148,9 +2155,14 @@ static int update_squash_messages(struct repository *r,
 	strbuf_release(&buf);
 
 	if (!res) {
-		strbuf_addf(&ctx->current_fixups, "%s%s %s",
+		const char *fixup_flag = "";
+
+		if (is_fixup_flag(command, flag) && (flag & TODO_EDIT_FIXUP_MSG))
+			fixup_flag = " -c";
+
+		strbuf_addf(&ctx->current_fixups, "%s%s%s %s",
 			    ctx->current_fixups.len ? "\n" : "",
-			    command_to_string(command),
+			    command_to_string(command), fixup_flag,
 			    oid_to_hex(&commit->object.oid));
 		res = write_message(ctx->current_fixups.buf,
 				    ctx->current_fixups.len,
@@ -5391,8 +5403,8 @@ static int commit_staged_changes(struct repository *r,
 				 * message, no need to bother the user with
 				 * opening the commit message in the editor.
 				 */
-				if (!starts_with(p, "squash ") &&
-				    !strstr(p, "\nsquash "))
+				if (!seen_squash(ctx) &&
+				    !seen_fixup_edit_msg(ctx))
 					flags = (flags & ~EDIT_MSG) | CLEANUP_MSG;
 			} else if (is_fixup(peek_command(todo_list, 0))) {
 				/*
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 5d306a47692..a4b2a631654 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -184,6 +184,53 @@ test_expect_success 'multiple fixup -c opens editor once' '
 	get_author HEAD >actual-author &&
 	test_cmp expected-author actual-author &&
 	test_commit_message HEAD expected-message
+'
+
+test_expect_success 'fixup -c is remembered after skipping final fixup' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	cat >todo <<-\EOF &&
+	pick B
+	fixup -c A1
+	fixup A3
+	EOF
+	(
+		set_fake_editor &&
+		set_replace_editor todo &&
+		test_must_fail git rebase -i A A &&
+		git show && cat .git/rebase-merge/message-squash &&
+		FAKE_COMMIT_AMEND=edited git rebase --skip
+	) &&
+	test_commit_message HEAD <<-\EOF
+	new subject
+
+	new
+	body
+
+	edited
+	EOF
+'
+test_expect_success 'fixup -c is remembered after skipping later fixup' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	cat >todo <<-\EOF &&
+	pick B
+	fixup -c A1
+	fixup A3
+	fixup A2
+	EOF
+	(
+		set_fake_editor &&
+		set_replace_editor todo &&
+		test_must_fail git rebase -i A A &&
+		FAKE_COMMIT_AMEND=edited git rebase --skip
+	) &&
+	test_commit_message HEAD <<-\EOF
+	new subject
+
+	new
+	body
+
+	edited
+	EOF
 '
 
 test_expect_success 'sequence squash, fixup & fixup -c gives combined message' '
-- 
2.54.0.200.gfd8d68259e3

Re: [PATCH 2/2] rebase: remember fixup -c after skipping fixup/squash

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>  	return starts_with(ctx->current_fixups.buf, "squash") ||
>  		strstr(ctx->current_fixups.buf, "\nsquash");
> +}
> +
> +/* Does the current fixup chain contain a "fixup -c" command? */
> +static int seen_fixup_edit_msg(struct replay_ctx *ctx)
> +{
> +	return starts_with(ctx->current_fixups.buf, "fixup -c") ||
> +		strstr(ctx->current_fixups.buf, "\nfixup -c");
>  }

It is a bit annoying that "git diff" decided to consider the "}" at
the end of the otherwise unmodified function to be the one that was
added X-<.  But thanks to it, we can see this mirrors the previous
function to check if we have "squash" anywhere.  I wonder what
diff-algorithm was used to produce this result, but it is an
unrelated tangent.

It is a bit surprising that we do not carefully parse each line to
identify a 'squash' or a 'fixup -c', which would make it unnecessary
to guess whether the current line is what we are looking for or if
the desired string immediately follows a newline later on.  Still,
this patch inherits that pattern from the original code, so it is
not a fault of this change.

>  static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)
> @@ -2148,9 +2155,14 @@ static int update_squash_messages(struct repository *r,
>  	strbuf_release(&buf);
>  
>  	if (!res) {
> -		strbuf_addf(&ctx->current_fixups, "%s%s %s",
> +		const char *fixup_flag = "";
> +
> +		if (is_fixup_flag(command, flag) && (flag & TODO_EDIT_FIXUP_MSG))
> +			fixup_flag = " -c";
> +
> +		strbuf_addf(&ctx->current_fixups, "%s%s%s %s",
>  			    ctx->current_fixups.len ? "\n" : "",
> -			    command_to_string(command),
> +			    command_to_string(command), fixup_flag,
>  			    oid_to_hex(&commit->object.oid));
>  		res = write_message(ctx->current_fixups.buf,
>  				    ctx->current_fixups.len,
> @@ -5391,8 +5403,8 @@ static int commit_staged_changes(struct repository *r,
>  				 * message, no need to bother the user with
>  				 * opening the commit message in the editor.
>  				 */
> -				if (!starts_with(p, "squash ") &&
> -				    !strstr(p, "\nsquash "))
> +				if (!seen_squash(ctx) &&
> +				    !seen_fixup_edit_msg(ctx))
>  					flags = (flags & ~EDIT_MSG) | CLEANUP_MSG;

If 'fixup -c' is anywhere in the chain, we would need to offer the
user a chance to edit (similar to having 'squash').

It is a bit surprising that the 'squash' detection, for which we
already had a helper function, was open-coded here.  I also notice
that the helpers (including the new 'fixup -c' one) do not insist on
having a space immediately after the verb 'squash'.  Should we add
one above?

Other than these minor nits, this looks good.

It is a bit disappointing that, with so many users who crucially
depend on the proper operation of 'rebase -i', we have received no
review comments on these two patches so far.  Perhaps summer is a
truly quiet and slow season ;-)

I will wait for a few more days and then mark the topic for 'next'.

Thanks.