Re: [PATCH v2] rebase -i: do leave commit message intact in fixup! chains
flat view
From: Martin Ågren <hidden>
Date: 2021-01-12 21:41:58
Subsystem:
the rest · Maintainer:
Linus Torvalds
Johannes Schindelin via GitGitGadget [off-list ref] wrote:
We actually need to actively suppress that clean-up lest a configured `commit.cleanup` may interfere with what we want to do: leave the commit message unchanged.
Changes since v1:> * The fix now works even if commit.cleanup = commit
Indeed. FWIW, this patch looks good to me. There is the lone remaining user of `CLEANUP_MSG` where we're handling the skipping of the final fixup in a chain and which is part of a larger block of code to handle various cases like that around `git rebase --skip`. I wrote some tests on top of your patch to see what happens and try to understand what's going on. The tests are below. I can't say I grok all that's going on in the implementation. By the time we're finalizing the commit message, it seems to me that we've lost track of which of the lines that look like comments are indeed comment lines added by us and which actually originate in the commit messages we're trying to rebase and which just happen to begin with a comment character. Maybe these tests could be simplified a bit, or de-boilerplated to some extent, but I think they do correctly demonstrate a similar bug that remains after your fix. What do you think? Are these test failures useful at all? Would it be an easy fix? In any case, I don't think the presence of these bugs need to hold up the fix you've posted here. The bugs look like they're related, but they are in different parts of the code (to the best of my understanding). FWIW, this diff is Signed-off-by me if you want to run with it. Maybe include some part of it that you agree with in your commit? (Yes, this diff mentions "master" several times. This file already mentions that branch name. You have a patch in seen to address that, but there would be a semantic conflict.) Martin
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 88040bc435..f80318998f 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh@@ -448,4 +448,63 @@ test_expect_success 'fixup does not clean up commit message' ' test "$oneline" = "$(git show -s --format=%s)" ' +test_expect_failure 'fixup does not clean message (conflict in only fixup)' ' + test_when_finished "git checkout master" && + oneline="#ladder" && + echo version-1 >file && + git add file && + git commit -m version-1 && + git checkout -B fixup-topic HEAD^ && + # Now make two commits (oneline+fixup) that conflict + # in the fixup when we rebase them onto master. + git commit --allow-empty -m "$oneline" && + echo conflict >file && + git add file && + git commit --fixup HEAD && + test_must_fail git -c commit.cleanup=strip rebase -ki \ + --autosquash --onto=master HEAD~2 && + git rebase --skip && + test "$oneline" = "$(git show -s --format=%s)" +' + +test_expect_failure 'fixup does not clean message (conflict in non-last fixup)' ' + test_when_finished "git checkout master" && + oneline="#not-a-comment" && + echo version-2 >file && + git add file && + git commit -m version-2 && + git checkout -B fixup-topic HEAD^ && + # Now make three commits (oneline+fixup+fixup) that conflict + # in the first fixup when we rebase them onto master. + git commit --allow-empty -m "$oneline" && + echo conflict >file && + git add file && + git commit --fixup HEAD && + git commit --fixup HEAD --allow-empty && + test_must_fail git -c commit.cleanup=strip rebase -ki \ + --autosquash --onto=master HEAD~3 && + git rebase --skip && + test "$oneline" = "$(git show -s --format=%s)" +' + +test_expect_failure 'fixup does not clean message (conflict in last fixup)' ' + test_when_finished "git checkout master" && + oneline="#hash" && + echo version-3 >file && + git add file && + git commit -m version-3 && + git checkout -B fixup-topic HEAD^ && + # Now make three commits (oneline+fixup+fixup) that conflict + # in the second fixup when we rebase them onto master. + git commit --allow-empty -m "$oneline" && + git commit --fixup HEAD --allow-empty && + echo conflict >file && + git add file && + git commit --fixup HEAD && + test_must_fail git -c commit.cleanup=strip rebase -ki \ + --autosquash --onto=master HEAD~3 && + git rebase --skip && + test "$oneline" = "$(git show -s --format=%s)" +' + test_done