Eric Sunshine pointed out that I had such a commit message in https://public-inbox.org/git/CAPig+cRrS0_nYJJY=O6cboV630sNQHPV5QGrQdD8MW-sYzNFGQ@xxxxxxxxxxxxxx/ and I went on a hunt to figure out how the heck this happened. Turns out that if there is a fixup/squash chain where the *last* command fails with merge conflicts, and we either --skip ahead or resolve the conflict to a clean tree and then --continue, our code does not do a final cleanup. Contrary to my initial gut feeling, this bug was not introduced by my rewrite in C of the core parts of rebase -i, but it looks to me as if that bug was with us for a very long time (at least the --skip part). The developer (read: user of rebase -i) in me says that we would want to fast-track this, but the author of rebase -i in me says that we should be cautious and cook this in `next` for a while. Fixes since v1: - Using test_i18ngrep instead of grep, because "This is a combination of <N> commits" is marked for translation. - Added a patch to actually fix `rebase -i` when building with GETTEXT_POISON, because we used to assume that numbers are encoded as ASCII so that we can increment it when writing the next commit message in the fixup/squash chain. This also seems to be a long-standing bug that has been with us since the the beginning of the localization of rebase -i's commit messages. - The test case now starts with test_when_finished "test_might_fail git rebase --abort" to be allow for failing more gently. - Fixed grammar of 2/3 (now 3/4): thanks, Eric! - Fixed the description of the new test case (it purported to test --continue, but it really tests --skip). Johannes Schindelin (4): rebase -i: demonstrate bugs with fixup!/squash! commit messages rebase -i: Handle "combination of <n> commits" with GETTEXT_POISON sequencer: leave a tell-tale when a fixup/squash failed rebase --skip: clean up commit message after a failed fixup/squash sequencer.c | 93 ++++++++++++++++++++++++++++---------- t/t3418-rebase-continue.sh | 22 +++++++++ 2 files changed, 92 insertions(+), 23 deletions(-) base-commit: fe0a9eaf31dd0c349ae4308498c33a5c3794b293 Published-As: https://github.com/dscho/git/releases/tag/clean-msg-after-fixup-continue-v2 Fetch-It-Via: git fetch https://github.com/dscho/git clean-msg-after-fixup-continue-v2 Interdiff vs v1: diff --git a/sequencer.c b/sequencer.c index f067b7b24c5..881503a6463 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1350,19 +1350,18 @@ static int update_squash_messages(enum todo_command command, eol = strchrnul(buf.buf, '\n'); if (buf.buf[0] != comment_line_char || (p += strcspn(p, "0123456789\n")) == eol) - return error(_("unexpected 1st line of squash message:" - "\n\n\t%.*s"), - (int)(eol - buf.buf), buf.buf); - count = strtol(p, NULL, 10); - - if (count < 1) - return error(_("invalid 1st line of squash message:\n" - "\n\t%.*s"), - (int)(eol - buf.buf), buf.buf); + count = -1; + else + count = strtol(p, NULL, 10); strbuf_addf(&header, "%c ", comment_line_char); - strbuf_addf(&header, - _("This is a combination of %d commits."), ++count); + if (count < 1) + strbuf_addf(&header, _("This is a combination of " + "several commits.")); + else + strbuf_addf(&header, + _("This is a combination of %d commits."), + ++count); strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len); strbuf_release(&header); } else { @@ -1405,13 +1404,22 @@ static int update_squash_messages(enum todo_command command, if (command == TODO_SQUASH) { unlink(rebase_path_fixup_msg()); strbuf_addf(&buf, "\n%c ", comment_line_char); - strbuf_addf(&buf, _("This is the commit message #%d:"), count); + if (count < 2) + strbuf_addf(&buf, _("This is the next commit " + "message:")); + else + strbuf_addf(&buf, _("This is the commit message #%d:"), + 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:"), - count); + if (count < 2) + strbuf_addf(&buf, _("The next commit message will be " + "skipped:")); + else + strbuf_addf(&buf, _("The commit message #%d will be " + "skipped:"), count); 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 4880bff82ff..693f92409ec 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -88,7 +88,8 @@ test_expect_success 'rebase passes merge strategy options correctly' ' git rebase --continue ' -test_expect_success '--continue after failed fixup cleans commit message' ' +test_expect_success '--skip after failed fixup cleans commit message' ' + test_when_finished "test_might_fail git rebase --abort" && git checkout -b with-conflicting-fixup && test_commit wants-fixup && test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 && @@ -99,14 +100,14 @@ test_expect_success '--continue after failed fixup cleans commit message' ' : now there is a conflict, and comments in the commit message && git show HEAD >out && - grep "This is a combination of" out && + test_i18ngrep "This is a combination of" out && : skip and continue && git rebase --skip && : now the comments in the commit message should have been cleaned up && git show HEAD >out && - ! grep "This is a combination of" out + test_i18ngrep ! "This is a combination of" out ' test_expect_success 'setup rerere database' ' -- 2.17.0.windows.1.15.gaa56ade3205
