Merge branch 'pw/rebase-fixup-fixes' into seen

Two bugs in how 'git rebase' handles skipped 'fixup' and 'squash'
commands have been fixed.  One bug caused an incorrect commit count to
be shown in the template message when multiple commands were skipped,
and another prevented the editor from opening when the final command
in a chain containing 'fixup -c' was skipped.

* pw/rebase-fixup-fixes:
  rebase: remember fixup -c after skipping fixup/squash
  rebase -i: fix counting of fixups after rebase --skip

sequencer.c

@@ -1946,6 +1946,13 @@ static int seen_squash(struct replay_ctx *ctx)
 		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)
 {
 	strbuf_setlen(buf1, strlen(comment_line_str) + 1);
@@ -2156,9 +2163,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,
@@ -3319,7 +3331,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++;
 			}
 		}
@@ -5414,6 +5432,9 @@ static int commit_staged_changes(struct repository *r,
 				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(),
 					  0) < 0) {
@@ -5442,8 +5463,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))) {
 				/*

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,6 +173,32 @@ 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
@@ -178,12 +206,12 @@ test_expect_success '--skip after failed fixup cleans commit message' '
 	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' '

t/t3437-rebase-fixup-options.sh

@@ -186,6 +186,53 @@ test_expect_success 'multiple fixup -c opens editor once' '
 	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' '
 	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout --detach A3 &&