From 2f5ff2c339d90422dd4010b5d980d33d1959fdd9 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Sun, 26 Jul 2026 16:38:59 +0100 Subject: [PATCH 1/2] rebase -i: fix counting of fixups after rebase --skip 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 Signed-off-by: Junio C Hamano --- 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 1355a99a09..4640ee9b7f 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] && p[1] != '\n') + ctx->current_fixup_count++; p++; } } @@ -5354,6 +5360,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) { diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index f9b8999db5..3c248e9736 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,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' ' From 4a3a8ee96c7c749b99c91345db190a2c3720abeb Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Sun, 26 Jul 2026 16:39:00 +0100 Subject: [PATCH 2/2] rebase: remember fixup -c after skipping fixup/squash 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 Signed-off-by: Junio C Hamano --- 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 4640ee9b7f..1a0a283b42 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1926,6 +1926,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); @@ -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 5d306a4769..a4b2a63165 100755 --- a/t/t3437-rebase-fixup-options.sh +++ b/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 &&