{"thread":{"id":"66030","subject":"[PATCH 0/2] rebase: a couple of fixup fixes","startedAt":"2026-07-17T16:06:55Z","lastAt":"2026-07-26T21:32:42Z","messageCount":10,"participants":["Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"548541","messageId":"cover.1784304378.git.phillip.wood@dunelm.org.uk","threadId":"66030","inReplyTo":null,"subject":"[PATCH 0/2] rebase: a couple of fixup fixes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-17T16:06:35Z","receivedAt":"2026-07-17T16:06:55Z","isPatch":true,"body":"These patches fix a couple of small bugs in the way skipped \"fixup\"\nand \"squash\" commands are handled. A skipped command can lead to\nan incorrect commit count in the template message which is fixed in\npatch 1. It can also mean we fail to open the editor after a \"fixup\n-c\" command which is fixed in patch 2\n\nbase-commit: d35c5399e3e54ac277bb391fc2f6be3e816d312b\nPublished-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-fixup-fixes-part-1%2Fv1\nView-Changes-At: https://github.com/phillipwood/git/compare/d35c5399e...7c8075ff2\nFetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-fixup-fixes-part-1/v1\n\n\nPhillip Wood (2):\n  rebase -i: fix counting of fixups after rebase --skip\n  rebase: remember fixup -c after skipping fixup/squash\n\n sequencer.c                     | 31 ++++++++++++++++++----\n t/t3418-rebase-continue.sh      | 36 ++++++++++++++++++++++---\n t/t3437-rebase-fixup-options.sh | 47 +++++++++++++++++++++++++++++++++\n 3 files changed, 105 insertions(+), 9 deletions(-)\n\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"548542","messageId":"c37a518486a8fa9832a6dbbe6048cda70af87d73.1784304378.git.phillip.wood@dunelm.org.uk","threadId":"66030","inReplyTo":"cover.1784304378.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 1/2] rebase -i: fix counting of fixups after rebase --skip","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-17T16:06:36Z","receivedAt":"2026-07-17T16:06:56Z","isPatch":true,"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nWhen the sequencer processes a chain of \"fixup\" and \"squash\" commands\nit keeps a list of the commands that have been executed. If there are\nconflicts, then the list is saved when the rebase stops for the user to\nresolve them. When the rebase resumes, the list is loaded and is used\nto initialize the count of how many \"fixup\" and \"squash\" commands have\nbeen processed; if a command has been skipped with \"git rebase --skip\",\nthen the last command needs to be popped off the end of the list.\n\nTo count the number of commands, commit_staged_changes() uses the\nnumber of newlines in the file plus one. This is due to the slightly\nunusual way the list is constructed - instead of appending a newline\nwhen a command is added, a newline is inserted before the command\nif the current count is greater than zero. Therefore, when we pop a\nskipped command off the list, we should also remove the newline that\nprecedes it. Otherwise, when a new command is added, a blank line\nwill be left before it, which will contribute to the fixup count the\nnext time the file is read. Unfortunately, the preceding newline is\nnot removed, leading to an incorrect count. Fix this by removing the\nnewline that appears before the skipped command.\n\nIn addition to fixing the code that removes a skipped command from the\nlist, the code that reads the list is fixed to skip blank lines. We\nhave had reports of users starting a rebase with one version of\ngit and continuing it with another. Often this happens because the\nversion of git bundled with an IDE or TUI differs from the one used\nat the command line. By fixing both the reading and writing ends of\nthe problem we ensure the count is correct when an older version of\ngit reads the fixup file written by a newer version and vice versa.\n\nTriggering the incorrect count requires the user to skip two \"fixup\" or\n\"squash\" commands before the final command in the chain. An existing\ntest is extended to prevent future regressions. The consequence of\nmiscounting is not serious: we just print the wrong count in the\nheader of the commit message template.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c                | 11 ++++++++++-\n t/t3418-rebase-continue.sh | 36 ++++++++++++++++++++++++++++++++----\n 2 files changed, 42 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 1355a99a092..af3d2c72616 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3281,7 +3281,13 @@ static int read_populate_opts(struct replay_opts *opts)\n \t\t\tconst char *p = ctx->current_fixups.buf;\n \t\t\tctx->current_fixup_count = 1;\n \t\t\twhile ((p = strchr(p, '\\n'))) {\n-\t\t\t\tctx->current_fixup_count++;\n+\t\t\t\t/*\n+\t\t\t\t * Older versions of git accidentally\n+\t\t\t\t * inserted blank lines when a fixup\n+\t\t\t\t * was skipped.\n+\t\t\t\t */\n+\t\t\t\tif (p[1] != '\\n')\n+\t\t\t\t\tctx->current_fixup_count++;\n \t\t\t\tp++;\n \t\t\t}\n \t\t}\n@@ -5353,6 +5359,9 @@ static int commit_staged_changes(struct repository *r,\n \t\t\tif (!len)\n \t\t\t\tBUG(\"Incorrect current_fixups:\\n%s\", p);\n \t\t\twhile (len && p[len - 1] != '\\n')\n+\t\t\t\tlen--;\n+\t\t\t/* Remove trailing newline */\n+\t\t\tif (len)\n \t\t\t\tlen--;\n \t\t\tstrbuf_setlen(&ctx->current_fixups, len);\n \t\t\tif (write_message(p, len, rebase_path_current_fixups(),\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex f9b8999db50..3c248e97364 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -134,6 +134,7 @@ test_expect_success '--skip after failed fixup cleans commit message' '\n \tEOF\n \n \t: skip and continue &&\n+\ttest_config commit.status false &&\n \techo \"cp \\\"\\$1\\\" .git/copy.txt\" | write_script copy-editor.sh &&\n \t(test_set_editor \"$PWD/copy-editor.sh\" && git rebase --skip) &&\n \n@@ -145,7 +146,8 @@ test_expect_success '--skip after failed fixup cleans commit message' '\n \n \t: now, let us ensure that \"squash\" is handled correctly &&\n \tgit reset --hard wants-fixup-3 &&\n-\ttest_must_fail env FAKE_LINES=\"1 squash 2 squash 1 squash 3 squash 1\" \\\n+\ttest_must_fail env \\\n+\t\tFAKE_LINES=\"1 squash 2 squash 1 squash 3 squash 1 squash 4 squash 1\" \\\n \t\tgit rebase -i HEAD~4 &&\n \n \t: the second squash failed, but there are two more in the chain &&\n@@ -171,19 +173,45 @@ test_expect_success '--skip after failed fixup cleans commit message' '\n \tfixup 2\n \tEOF\n \n+\t(test_set_editor \"$PWD/copy-editor.sh\" &&\n+\t test_must_fail git rebase --skip) &&\n+\t: not the final squash, no need to edit the commit message &&\n+\ttest_path_is_missing .git/copy.txt &&\n+\n+\t: The first, third and fifth squashes succeeded, therefore: &&\n+\tcat >expect <<-\\EOF &&\n+\t# This is a combination of 4 commits.\n+\t# This is the 1st commit message:\n+\n+\twants-fixup\n+\n+\t# This is the commit message #2:\n+\n+\tfixup 1\n+\n+\t# This is the commit message #3:\n+\n+\tfixup 2\n+\n+\t# This is the commit message #4:\n+\n+\tfixup 3\n+\tEOF\n+\ttest_commit_message HEAD expect &&\n+\n \t(test_set_editor \"$PWD/copy-editor.sh\" && git rebase --skip) &&\n \ttest_commit_message HEAD <<-\\EOF &&\n \twants-fixup\n \n \tfixup 1\n \n \tfixup 2\n+\n+\tfixup 3\n \tEOF\n \n \t: Final squash failed, but there was still a squash &&\n-\thead -n1 .git/copy.txt >first-line &&\n-\ttest_grep \"# This is a combination of 3 commits\" first-line &&\n-\ttest_grep \"# This is the commit message #3:\" .git/copy.txt\n+\ttest_cmp expect .git/copy.txt\n '\n \n test_expect_success 'setup rerere database' '\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"548543","messageId":"7c8075ff2675976821a1ee979f86c7c46a35bd15.1784304378.git.phillip.wood@dunelm.org.uk","threadId":"66030","inReplyTo":"cover.1784304378.git.phillip.wood@dunelm.org.uk","subject":"[PATCH 2/2] rebase: remember fixup -c after skipping fixup/squash","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-17T16:06:37Z","receivedAt":"2026-07-17T16:06:57Z","isPatch":true,"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nWhen the final command in a chain of \"fixup\" and \"squash\" commands\nis skipped, we should prompt the user to edit the commit message\nif the chain contains a \"fixup -c\" command that was not skipped.\nUnfortunately, commit_staged_changes() only looks for completed \"squash\"\ncommands and so does not prompt the user to edit the message. Fix\nthis by recording whether a fixup command has the \"-c\" flag set and\nthen checking whether we have seen either a \"fixup -c\" or a \"squash\"\ncommand. Add regression tests for skipping a command in the middle\nof the chain (which currently works but has no test coverage), and\nfor skipping the final command (which is fixed by this patch).\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c                     | 20 +++++++++++---\n t/t3437-rebase-fixup-options.sh | 47 +++++++++++++++++++++++++++++++++\n 2 files changed, 63 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex af3d2c72616..25ef076216c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1924,6 +1924,13 @@ static int seen_squash(struct replay_ctx *ctx)\n {\n \treturn starts_with(ctx->current_fixups.buf, \"squash\") ||\n \t\tstrstr(ctx->current_fixups.buf, \"\\nsquash\");\n+}\n+\n+/* Does the current fixup chain contain a \"fixup -c\" command? */\n+static int seen_fixup_edit_msg(struct replay_ctx *ctx)\n+{\n+\treturn starts_with(ctx->current_fixups.buf, \"fixup -c\") ||\n+\t\tstrstr(ctx->current_fixups.buf, \"\\nfixup -c\");\n }\n \n static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)\n@@ -2148,9 +2155,14 @@ static int update_squash_messages(struct repository *r,\n \tstrbuf_release(&buf);\n \n \tif (!res) {\n-\t\tstrbuf_addf(&ctx->current_fixups, \"%s%s %s\",\n+\t\tconst char *fixup_flag = \"\";\n+\n+\t\tif (is_fixup_flag(command, flag) && (flag & TODO_EDIT_FIXUP_MSG))\n+\t\t\tfixup_flag = \" -c\";\n+\n+\t\tstrbuf_addf(&ctx->current_fixups, \"%s%s%s %s\",\n \t\t\t    ctx->current_fixups.len ? \"\\n\" : \"\",\n-\t\t\t    command_to_string(command),\n+\t\t\t    command_to_string(command), fixup_flag,\n \t\t\t    oid_to_hex(&commit->object.oid));\n \t\tres = write_message(ctx->current_fixups.buf,\n \t\t\t\t    ctx->current_fixups.len,\n@@ -5391,8 +5403,8 @@ static int commit_staged_changes(struct repository *r,\n \t\t\t\t * message, no need to bother the user with\n \t\t\t\t * opening the commit message in the editor.\n \t\t\t\t */\n-\t\t\t\tif (!starts_with(p, \"squash \") &&\n-\t\t\t\t    !strstr(p, \"\\nsquash \"))\n+\t\t\t\tif (!seen_squash(ctx) &&\n+\t\t\t\t    !seen_fixup_edit_msg(ctx))\n \t\t\t\t\tflags = (flags & ~EDIT_MSG) | CLEANUP_MSG;\n \t\t\t} else if (is_fixup(peek_command(todo_list, 0))) {\n \t\t\t\t/*\ndiff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh\nindex 5d306a47692..a4b2a631654 100755\n--- a/t/t3437-rebase-fixup-options.sh\n+++ b/t/t3437-rebase-fixup-options.sh\n@@ -184,6 +184,53 @@ test_expect_success 'multiple fixup -c opens editor once' '\n \tget_author HEAD >actual-author &&\n \ttest_cmp expected-author actual-author &&\n \ttest_commit_message HEAD expected-message\n+'\n+\n+test_expect_success 'fixup -c is remembered after skipping final fixup' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\tcat >todo <<-\\EOF &&\n+\tpick B\n+\tfixup -c A1\n+\tfixup A3\n+\tEOF\n+\t(\n+\t\tset_fake_editor &&\n+\t\tset_replace_editor todo &&\n+\t\ttest_must_fail git rebase -i A A &&\n+\t\tgit show && cat .git/rebase-merge/message-squash &&\n+\t\tFAKE_COMMIT_AMEND=edited git rebase --skip\n+\t) &&\n+\ttest_commit_message HEAD <<-\\EOF\n+\tnew subject\n+\n+\tnew\n+\tbody\n+\n+\tedited\n+\tEOF\n+'\n+test_expect_success 'fixup -c is remembered after skipping later fixup' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\tcat >todo <<-\\EOF &&\n+\tpick B\n+\tfixup -c A1\n+\tfixup A3\n+\tfixup A2\n+\tEOF\n+\t(\n+\t\tset_fake_editor &&\n+\t\tset_replace_editor todo &&\n+\t\ttest_must_fail git rebase -i A A &&\n+\t\tFAKE_COMMIT_AMEND=edited git rebase --skip\n+\t) &&\n+\ttest_commit_message HEAD <<-\\EOF\n+\tnew subject\n+\n+\tnew\n+\tbody\n+\n+\tedited\n+\tEOF\n '\n \n test_expect_success 'sequence squash, fixup & fixup -c gives combined message' '\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"548918","messageId":"xmqqbjbw5cgx.fsf@gitster.g","threadId":"66030","inReplyTo":"c37a518486a8fa9832a6dbbe6048cda70af87d73.1784304378.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 1/2] rebase -i: fix counting of fixups after rebase --skip","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-24T21:01:18Z","receivedAt":"2026-07-24T21:01:20Z","isPatch":true,"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> @@ -3281,7 +3281,13 @@ static int read_populate_opts(struct replay_opts *opts)\n>  \t\t\tconst char *p = ctx->current_fixups.buf;\n>  \t\t\tctx->current_fixup_count = 1;\n>  \t\t\twhile ((p = strchr(p, '\\n'))) {\n> -\t\t\t\tctx->current_fixup_count++;\n> +\t\t\t\t/*\n> +\t\t\t\t * Older versions of git accidentally\n> +\t\t\t\t * inserted blank lines when a fixup\n> +\t\t\t\t * was skipped.\n> +\t\t\t\t */\n> +\t\t\t\tif (p[1] != '\\n')\n> +\t\t\t\t\tctx->current_fixup_count++;\n>  \t\t\t\tp++;\n>  \t\t\t}\n>  \t\t}\n\nIf we hit the LF at the very end (e.g. \"fixup A\\n\" at the end of the\nfile), strchr() would have moved p to the newline, and p[1] will be\n'\\0', no?  And because p[1] != '\\n' and wouldn't current_fixup_count\nbe incremented again?  It might be safer to check p[1] != '\\n' &&\np[1] != '\\0' to avoid counting a trailing newline as an extra\ncommand when reading legacy files.\n\n> @@ -5353,6 +5359,9 @@ static int commit_staged_changes(struct repository *r,\n>  \t\t\tif (!len)\n>  \t\t\t\tBUG(\"Incorrect current_fixups:\\n%s\", p);\n>  \t\t\twhile (len && p[len - 1] != '\\n')\n> +\t\t\t\tlen--;\n> +\t\t\t/* Remove trailing newline */\n> +\t\t\tif (len)\n>  \t\t\t\tlen--;\n\nSo we removed all the non newline from the end, and the loop would\nbreak if !len or p[len - 1] == '\\n'.  And in the latter case, we\nalso drop that '\\n'.  Which sounds right.\n\nThanks.\n\n"},{"id":"548925","messageId":"xmqqtspo3x31.fsf@gitster.g","threadId":"66030","inReplyTo":"7c8075ff2675976821a1ee979f86c7c46a35bd15.1784304378.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH 2/2] rebase: remember fixup -c after skipping fixup/squash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-24T21:18:58Z","receivedAt":"2026-07-24T21:19:00Z","isPatch":true,"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n>  \treturn starts_with(ctx->current_fixups.buf, \"squash\") ||\n>  \t\tstrstr(ctx->current_fixups.buf, \"\\nsquash\");\n> +}\n> +\n> +/* Does the current fixup chain contain a \"fixup -c\" command? */\n> +static int seen_fixup_edit_msg(struct replay_ctx *ctx)\n> +{\n> +\treturn starts_with(ctx->current_fixups.buf, \"fixup -c\") ||\n> +\t\tstrstr(ctx->current_fixups.buf, \"\\nfixup -c\");\n>  }\n\nIt is a bit annoying that \"git diff\" decided to consider the \"}\" at\nthe end of the otherwise unmodified function to be the one that was\nadded X-<.  But thanks to it, we can see this mirrors the previous\nfunction to check if we have \"squash\" anywhere.  I wonder what\ndiff-algorithm was used to produce this result, but it is an\nunrelated tangent.\n\nIt is a bit surprising that we do not carefully parse each line to\nidentify a 'squash' or a 'fixup -c', which would make it unnecessary\nto guess whether the current line is what we are looking for or if\nthe desired string immediately follows a newline later on.  Still,\nthis patch inherits that pattern from the original code, so it is\nnot a fault of this change.\n\n>  static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)\n> @@ -2148,9 +2155,14 @@ static int update_squash_messages(struct repository *r,\n>  \tstrbuf_release(&buf);\n>  \n>  \tif (!res) {\n> -\t\tstrbuf_addf(&ctx->current_fixups, \"%s%s %s\",\n> +\t\tconst char *fixup_flag = \"\";\n> +\n> +\t\tif (is_fixup_flag(command, flag) && (flag & TODO_EDIT_FIXUP_MSG))\n> +\t\t\tfixup_flag = \" -c\";\n> +\n> +\t\tstrbuf_addf(&ctx->current_fixups, \"%s%s%s %s\",\n>  \t\t\t    ctx->current_fixups.len ? \"\\n\" : \"\",\n> -\t\t\t    command_to_string(command),\n> +\t\t\t    command_to_string(command), fixup_flag,\n>  \t\t\t    oid_to_hex(&commit->object.oid));\n>  \t\tres = write_message(ctx->current_fixups.buf,\n>  \t\t\t\t    ctx->current_fixups.len,\n> @@ -5391,8 +5403,8 @@ static int commit_staged_changes(struct repository *r,\n>  \t\t\t\t * message, no need to bother the user with\n>  \t\t\t\t * opening the commit message in the editor.\n>  \t\t\t\t */\n> -\t\t\t\tif (!starts_with(p, \"squash \") &&\n> -\t\t\t\t    !strstr(p, \"\\nsquash \"))\n> +\t\t\t\tif (!seen_squash(ctx) &&\n> +\t\t\t\t    !seen_fixup_edit_msg(ctx))\n>  \t\t\t\t\tflags = (flags & ~EDIT_MSG) | CLEANUP_MSG;\n\nIf 'fixup -c' is anywhere in the chain, we would need to offer the\nuser a chance to edit (similar to having 'squash').\n\nIt is a bit surprising that the 'squash' detection, for which we\nalready had a helper function, was open-coded here.  I also notice\nthat the helpers (including the new 'fixup -c' one) do not insist on\nhaving a space immediately after the verb 'squash'.  Should we add\none above?\n\nOther than these minor nits, this looks good.\n\nIt is a bit disappointing that, with so many users who crucially\ndepend on the proper operation of 'rebase -i', we have received no\nreview comments on these two patches so far.  Perhaps summer is a\ntruly quiet and slow season ;-)\n\nI will wait for a few more days and then mark the topic for 'next'.\n\nThanks.\n"},{"id":"549017","messageId":"cover.1785080337.git.phillip.wood@dunelm.org.uk","threadId":"66030","inReplyTo":"cover.1784304378.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 0/2] rebase: a couple of fixup fixes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-26T15:38:58Z","receivedAt":"2026-07-26T15:39:22Z","isPatch":true,"body":"These patches fix a couple of small bugs in the way skipped \"fixup\"\nand \"squash\" commands are handled. A skipped command can lead to\nan incorrect commit count in the template message which is fixed in\npatch 1. It can also mean we fail to open the editor after a \"fixup\n-c\" command which is fixed in patch 2\n\nThanks for the comments on V1. The only change here is to make sure\na character non-NUL when we're checking if it isn't a LF in patch 1\nas suggested by Junio.\n\nbase-commit: 9a0c4701dcd5725c4184599322b52933ff5005ca\nPublished-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-fixup-fixes-part-1%2Fv2\nView-Changes-At: https://github.com/phillipwood/git/compare/9a0c4701d...3089979e2\nFetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-fixup-fixes-part-1/v2\n\n\nPhillip Wood (2):\n  rebase -i: fix counting of fixups after rebase --skip\n  rebase: remember fixup -c after skipping fixup/squash\n\n sequencer.c                     | 31 ++++++++++++++++++----\n t/t3418-rebase-continue.sh      | 36 ++++++++++++++++++++++---\n t/t3437-rebase-fixup-options.sh | 47 +++++++++++++++++++++++++++++++++\n 3 files changed, 105 insertions(+), 9 deletions(-)\n\nRange-diff against v1:\n1:  c37a518486a ! 1:  f95668512a8 rebase -i: fix counting of fixups after rebase --skip\n    @@ sequencer.c: static int read_populate_opts(struct replay_opts *opts)\n     +\t\t\t\t * inserted blank lines when a fixup\n     +\t\t\t\t * was skipped.\n     +\t\t\t\t */\n    -+\t\t\t\tif (p[1] != '\\n')\n    ++\t\t\t\tif (p[1] && p[1] != '\\n')\n     +\t\t\t\t\tctx->current_fixup_count++;\n      \t\t\t\tp++;\n      \t\t\t}\n2:  7c8075ff267 = 2:  3089979e2da rebase: remember fixup -c after skipping fixup/squash\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"549018","messageId":"f95668512a8ec6f7e81fcb761e883877df87deee.1785080337.git.phillip.wood@dunelm.org.uk","threadId":"66030","inReplyTo":"cover.1785080337.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 1/2] rebase -i: fix counting of fixups after rebase --skip","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-26T15:38:59Z","receivedAt":"2026-07-26T15:39:23Z","isPatch":true,"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nWhen the sequencer processes a chain of \"fixup\" and \"squash\" commands\nit keeps a list of the commands that have been executed. If there are\nconflicts, then the list is saved when the rebase stops for the user to\nresolve them. When the rebase resumes, the list is loaded and is used\nto initialize the count of how many \"fixup\" and \"squash\" commands have\nbeen processed; if a command has been skipped with \"git rebase --skip\",\nthen the last command needs to be popped off the end of the list.\n\nTo count the number of commands, commit_staged_changes() uses the\nnumber of newlines in the file plus one. This is due to the slightly\nunusual way the list is constructed - instead of appending a newline\nwhen a command is added, a newline is inserted before the command\nif the current count is greater than zero. Therefore, when we pop a\nskipped command off the list, we should also remove the newline that\nprecedes it. Otherwise, when a new command is added, a blank line\nwill be left before it, which will contribute to the fixup count the\nnext time the file is read. Unfortunately, the preceding newline is\nnot removed, leading to an incorrect count. Fix this by removing the\nnewline that appears before the skipped command.\n\nIn addition to fixing the code that removes a skipped command from the\nlist, the code that reads the list is fixed to skip blank lines. We\nhave had reports of users starting a rebase with one version of\ngit and continuing it with another. Often this happens because the\nversion of git bundled with an IDE or TUI differs from the one used\nat the command line. By fixing both the reading and writing ends of\nthe problem we ensure the count is correct when an older version of\ngit reads the fixup file written by a newer version and vice versa.\n\nTriggering the incorrect count requires the user to skip two \"fixup\" or\n\"squash\" commands before the final command in the chain. An existing\ntest is extended to prevent future regressions. The consequence of\nmiscounting is not serious: we just print the wrong count in the\nheader of the commit message template.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c                | 11 ++++++++++-\n t/t3418-rebase-continue.sh | 36 ++++++++++++++++++++++++++++++++----\n 2 files changed, 42 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 1355a99a092..4640ee9b7f5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3281,7 +3281,13 @@ static int read_populate_opts(struct replay_opts *opts)\n \t\t\tconst char *p = ctx->current_fixups.buf;\n \t\t\tctx->current_fixup_count = 1;\n \t\t\twhile ((p = strchr(p, '\\n'))) {\n-\t\t\t\tctx->current_fixup_count++;\n+\t\t\t\t/*\n+\t\t\t\t * Older versions of git accidentally\n+\t\t\t\t * inserted blank lines when a fixup\n+\t\t\t\t * was skipped.\n+\t\t\t\t */\n+\t\t\t\tif (p[1] && p[1] != '\\n')\n+\t\t\t\t\tctx->current_fixup_count++;\n \t\t\t\tp++;\n \t\t\t}\n \t\t}\n@@ -5353,6 +5359,9 @@ static int commit_staged_changes(struct repository *r,\n \t\t\tif (!len)\n \t\t\t\tBUG(\"Incorrect current_fixups:\\n%s\", p);\n \t\t\twhile (len && p[len - 1] != '\\n')\n+\t\t\t\tlen--;\n+\t\t\t/* Remove trailing newline */\n+\t\t\tif (len)\n \t\t\t\tlen--;\n \t\t\tstrbuf_setlen(&ctx->current_fixups, len);\n \t\t\tif (write_message(p, len, rebase_path_current_fixups(),\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex 03e0714864c..cb5c3a1cb5b 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -134,6 +134,7 @@ test_expect_success '--skip after failed fixup cleans commit message' '\n \tEOF\n \n \t: skip and continue &&\n+\ttest_config commit.status false &&\n \techo \"cp \\\"\\$1\\\" .git/copy.txt\" | write_script copy-editor.sh &&\n \t(test_set_editor \"$PWD/copy-editor.sh\" && git rebase --skip) &&\n \n@@ -145,7 +146,8 @@ test_expect_success '--skip after failed fixup cleans commit message' '\n \n \t: now, let us ensure that \"squash\" is handled correctly &&\n \tgit reset --hard wants-fixup-3 &&\n-\ttest_must_fail env FAKE_LINES=\"1 squash 2 squash 1 squash 3 squash 1\" \\\n+\ttest_must_fail env \\\n+\t\tFAKE_LINES=\"1 squash 2 squash 1 squash 3 squash 1 squash 4 squash 1\" \\\n \t\tgit rebase -i HEAD~4 &&\n \n \t: the second squash failed, but there are two more in the chain &&\n@@ -171,19 +173,45 @@ test_expect_success '--skip after failed fixup cleans commit message' '\n \tfixup 2\n \tEOF\n \n+\t(test_set_editor \"$PWD/copy-editor.sh\" &&\n+\t test_must_fail git rebase --skip) &&\n+\t: not the final squash, no need to edit the commit message &&\n+\ttest_path_is_missing .git/copy.txt &&\n+\n+\t: The first, third and fifth squashes succeeded, therefore: &&\n+\tcat >expect <<-\\EOF &&\n+\t# This is a combination of 4 commits.\n+\t# This is the 1st commit message:\n+\n+\twants-fixup\n+\n+\t# This is the commit message #2:\n+\n+\tfixup 1\n+\n+\t# This is the commit message #3:\n+\n+\tfixup 2\n+\n+\t# This is the commit message #4:\n+\n+\tfixup 3\n+\tEOF\n+\ttest_commit_message HEAD expect &&\n+\n \t(test_set_editor \"$PWD/copy-editor.sh\" && git rebase --skip) &&\n \ttest_commit_message HEAD <<-\\EOF &&\n \twants-fixup\n \n \tfixup 1\n \n \tfixup 2\n+\n+\tfixup 3\n \tEOF\n \n \t: Final squash failed, but there was still a squash &&\n-\thead -n1 .git/copy.txt >first-line &&\n-\ttest_grep \"# This is a combination of 3 commits\" first-line &&\n-\ttest_grep \"# This is the commit message #3:\" .git/copy.txt\n+\ttest_cmp expect .git/copy.txt\n '\n \n test_expect_success 'setup rerere database' '\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"549019","messageId":"3089979e2daf5bc8532008539e37695091dd10b2.1785080337.git.phillip.wood@dunelm.org.uk","threadId":"66030","inReplyTo":"cover.1785080337.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 2/2] rebase: remember fixup -c after skipping fixup/squash","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-26T15:39:00Z","receivedAt":"2026-07-26T15:39:24Z","isPatch":true,"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nWhen the final command in a chain of \"fixup\" and \"squash\" commands\nis skipped, we should prompt the user to edit the commit message\nif the chain contains a \"fixup -c\" command that was not skipped.\nUnfortunately, commit_staged_changes() only looks for completed \"squash\"\ncommands and so does not prompt the user to edit the message. Fix\nthis by recording whether a fixup command has the \"-c\" flag set and\nthen checking whether we have seen either a \"fixup -c\" or a \"squash\"\ncommand. Add regression tests for skipping a command in the middle\nof the chain (which currently works but has no test coverage), and\nfor skipping the final command (which is fixed by this patch).\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c                     | 20 +++++++++++---\n t/t3437-rebase-fixup-options.sh | 47 +++++++++++++++++++++++++++++++++\n 2 files changed, 63 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 4640ee9b7f5..1a0a283b42c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1924,6 +1924,13 @@ static int seen_squash(struct replay_ctx *ctx)\n {\n \treturn starts_with(ctx->current_fixups.buf, \"squash\") ||\n \t\tstrstr(ctx->current_fixups.buf, \"\\nsquash\");\n+}\n+\n+/* Does the current fixup chain contain a \"fixup -c\" command? */\n+static int seen_fixup_edit_msg(struct replay_ctx *ctx)\n+{\n+\treturn starts_with(ctx->current_fixups.buf, \"fixup -c\") ||\n+\t\tstrstr(ctx->current_fixups.buf, \"\\nfixup -c\");\n }\n \n static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)\n@@ -2148,9 +2155,14 @@ static int update_squash_messages(struct repository *r,\n \tstrbuf_release(&buf);\n \n \tif (!res) {\n-\t\tstrbuf_addf(&ctx->current_fixups, \"%s%s %s\",\n+\t\tconst char *fixup_flag = \"\";\n+\n+\t\tif (is_fixup_flag(command, flag) && (flag & TODO_EDIT_FIXUP_MSG))\n+\t\t\tfixup_flag = \" -c\";\n+\n+\t\tstrbuf_addf(&ctx->current_fixups, \"%s%s%s %s\",\n \t\t\t    ctx->current_fixups.len ? \"\\n\" : \"\",\n-\t\t\t    command_to_string(command),\n+\t\t\t    command_to_string(command), fixup_flag,\n \t\t\t    oid_to_hex(&commit->object.oid));\n \t\tres = write_message(ctx->current_fixups.buf,\n \t\t\t\t    ctx->current_fixups.len,\n@@ -5391,8 +5403,8 @@ static int commit_staged_changes(struct repository *r,\n \t\t\t\t * message, no need to bother the user with\n \t\t\t\t * opening the commit message in the editor.\n \t\t\t\t */\n-\t\t\t\tif (!starts_with(p, \"squash \") &&\n-\t\t\t\t    !strstr(p, \"\\nsquash \"))\n+\t\t\t\tif (!seen_squash(ctx) &&\n+\t\t\t\t    !seen_fixup_edit_msg(ctx))\n \t\t\t\t\tflags = (flags & ~EDIT_MSG) | CLEANUP_MSG;\n \t\t\t} else if (is_fixup(peek_command(todo_list, 0))) {\n \t\t\t\t/*\ndiff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh\nindex 5d306a47692..a4b2a631654 100755\n--- a/t/t3437-rebase-fixup-options.sh\n+++ b/t/t3437-rebase-fixup-options.sh\n@@ -184,6 +184,53 @@ test_expect_success 'multiple fixup -c opens editor once' '\n \tget_author HEAD >actual-author &&\n \ttest_cmp expected-author actual-author &&\n \ttest_commit_message HEAD expected-message\n+'\n+\n+test_expect_success 'fixup -c is remembered after skipping final fixup' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\tcat >todo <<-\\EOF &&\n+\tpick B\n+\tfixup -c A1\n+\tfixup A3\n+\tEOF\n+\t(\n+\t\tset_fake_editor &&\n+\t\tset_replace_editor todo &&\n+\t\ttest_must_fail git rebase -i A A &&\n+\t\tgit show && cat .git/rebase-merge/message-squash &&\n+\t\tFAKE_COMMIT_AMEND=edited git rebase --skip\n+\t) &&\n+\ttest_commit_message HEAD <<-\\EOF\n+\tnew subject\n+\n+\tnew\n+\tbody\n+\n+\tedited\n+\tEOF\n+'\n+test_expect_success 'fixup -c is remembered after skipping later fixup' '\n+\ttest_when_finished \"test_might_fail git rebase --abort\" &&\n+\tcat >todo <<-\\EOF &&\n+\tpick B\n+\tfixup -c A1\n+\tfixup A3\n+\tfixup A2\n+\tEOF\n+\t(\n+\t\tset_fake_editor &&\n+\t\tset_replace_editor todo &&\n+\t\ttest_must_fail git rebase -i A A &&\n+\t\tFAKE_COMMIT_AMEND=edited git rebase --skip\n+\t) &&\n+\ttest_commit_message HEAD <<-\\EOF\n+\tnew subject\n+\n+\tnew\n+\tbody\n+\n+\tedited\n+\tEOF\n '\n \n test_expect_success 'sequence squash, fixup & fixup -c gives combined message' '\n-- \n2.54.0.200.gfd8d68259e3\n\n"},{"id":"549020","messageId":"c9631a42-ea7b-45bb-a153-0372784b8f24@gmail.com","threadId":"66030","inReplyTo":"xmqqtspo3x31.fsf@gitster.g","subject":"Re: [PATCH 2/2] rebase: remember fixup -c after skipping fixup/squash","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-26T15:41:37Z","receivedAt":"2026-07-26T15:41:41Z","isPatch":true,"body":"Hi Junio\n\nOn 24/07/2026 22:18, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>>   \treturn starts_with(ctx->current_fixups.buf, \"squash\") ||\n>>   \t\tstrstr(ctx->current_fixups.buf, \"\\nsquash\");\n>> +}\n>> +\n>> +/* Does the current fixup chain contain a \"fixup -c\" command? */\n>> +static int seen_fixup_edit_msg(struct replay_ctx *ctx)\n>> +{\n>> +\treturn starts_with(ctx->current_fixups.buf, \"fixup -c\") ||\n>> +\t\tstrstr(ctx->current_fixups.buf, \"\\nfixup -c\");\n>>   }\n> \n> It is a bit annoying that \"git diff\" decided to consider the \"}\" at\n> the end of the otherwise unmodified function to be the one that was\n> added X-<.  But thanks to it, we can see this mirrors the previous\n> function to check if we have \"squash\" anywhere.  I wonder what\n> diff-algorithm was used to produce this result, but it is an\n> unrelated tangent.\n\nPatience diff without the diff slider. When I was reviewing some of \nEzekiel's patches I noticed that the diff slider was munging some diffs \ngenerated by patience in a way I didn't like so I tried turning it off \nto see what happened. It seems I haven't rebuilt my local git in a while ...\n\n>> @@ -5391,8 +5403,8 @@ static int commit_staged_changes(struct repository *r,\n>>   \t\t\t\t * message, no need to bother the user with\n>>   \t\t\t\t * opening the commit message in the editor.\n>>   \t\t\t\t */\n>> -\t\t\t\tif (!starts_with(p, \"squash \") &&\n>> -\t\t\t\t    !strstr(p, \"\\nsquash \"))\n>> +\t\t\t\tif (!seen_squash(ctx) &&\n>> +\t\t\t\t    !seen_fixup_edit_msg(ctx))\n>>   \t\t\t\t\tflags = (flags & ~EDIT_MSG) | CLEANUP_MSG;\n> \n> If 'fixup -c' is anywhere in the chain, we would need to offer the\n> user a chance to edit (similar to having 'squash').\n> \n> It is a bit surprising that the 'squash' detection, for which we\n> already had a helper function, was open-coded here.  I also notice\n> that the helpers (including the new 'fixup -c' one) do not insist on\n> having a space immediately after the verb 'squash'.  Should we add\n> one above?\n\nI'm not sure the space thing makes much difference as this isn't the \ntodo file that the user edits. We're reading a file that we've written \nand the lines can only start with \"fixup\" or \"squash\"\n\n> Other than these minor nits, this looks good.\n> \n> It is a bit disappointing that, with so many users who crucially\n> depend on the proper operation of 'rebase -i', we have received no\n> review comments on these two patches so far.  Perhaps summer is a\n> truly quiet and slow season ;-)\n\nOswald mentioned in another thread that he'd read these and they seemed \nto make sense. In general I find it hard to attract reviewers for \nrebase/sequencer patches - it is one of those features that everyone \nuses but not many people on the list seem to be familiar with the code.\n\n> I will wait for a few more days and then mark the topic for 'next'.\n\nThanks for your review, I've sent a re-roll fixing the newline detection \nin the previous patch.\n\nThanks\n\nPhillip\n"},{"id":"549047","messageId":"xmqqse55tp1k.fsf@gitster.g","threadId":"66030","inReplyTo":"c9631a42-ea7b-45bb-a153-0372784b8f24@gmail.com","subject":"Re: [PATCH 2/2] rebase: remember fixup -c after skipping fixup/squash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-26T21:32:39Z","receivedAt":"2026-07-26T21:32:42Z","isPatch":true,"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> I'm not sure the space thing makes much difference as this isn't the \n> todo file that the user edits. We're reading a file that we've written \n> and the lines can only start with \"fixup\" or \"squash\"\n\nAs long as we are internally consistent, I would be happy either way.\nAll code paths that read what we ourselves wrote consistently parse\nwithout a space because of the update in this hunk, so the omission\nof the space check is perfectly OK.\n\n> Oswald mentioned in another thread that he'd read these and they\n> seemed to make sense. In general I find it hard to attract\n> reviewers for rebase/sequencer patches - it is one of those\n> features that everyone uses but not many people on the list seem\n> to be familiar with the code.\n\nI wonder why that is, though.  I would not say it is the most\ncleanly designed and implemented piece of code, but I do not think\nit is so bad as to be impossible to read.\n\n>> I will wait for a few more days and then mark the topic for 'next'.\n>\n> Thanks for your review, I've sent a re-roll fixing the newline detection \n> in the previous patch.\n\nThanks.\n"}]}