On 20/04/18 13:18, Johannes Schindelin wrote:
During a series of fixup/squash commands, the interactive rebase builds
up a commit message with comments. This will be presented to the user in
the editor if at least one of those commands was a `squash`.
However, if the last of these fixup/squash commands fails with merge
conflicts, and if the user then decides to skip it (or resolve it to a
clean worktree and then continue the rebase), the current code fails to
clean up the commit message.
Thanks for taking the time to track this down and fix it.
This commit fixes that behavior.
The diff is best viewed with --color-moved.
Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
---
sequencer.c | 36 ++++++++++++++++++++++++++++--------
t/t3418-rebase-continue.sh | 2 +-
2 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index a9c3bc26f84..f067b7b24c5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2781,17 +2781,12 @@ static int continue_single_pick(void)
static int commit_staged_changes(struct replay_opts *opts)
{
- unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
+ unsigned int flags = ALLOW_EMPTY | EDIT_MSG, is_fixup = 0, is_clean;
if (has_unstaged_changes(1))
return error(_("cannot rebase: You have unstaged changes."));
- if (!has_uncommitted_changes(0)) {
- const char *cherry_pick_head = git_path_cherry_pick_head();
- if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
- return error(_("could not remove CHERRY_PICK_HEAD"));
- return 0;
- }
+ is_clean = !has_uncommitted_changes(0);
if (file_exists(rebase_path_amend())) {
struct strbuf rev = STRBUF_INIT;
@@ -2804,16 +2799,41 @@ static int commit_staged_changes(struct replay_opts *opts)
if (get_oid_hex(rev.buf, &to_amend))
return error(_("invalid contents: '%s'"),
rebase_path_amend());
- if (oidcmp(&head, &to_amend))
+ if (!is_clean && oidcmp(&head, &to_amend))
return error(_("\nYou have uncommitted changes in your "
"working tree. Please, commit them\n"
"first and then run 'git rebase "
"--continue' again."));
+ if (is_clean && !oidcmp(&head, &to_amend)) {
Looking at pick_commits() it only writes to rebase_path_amend() if there
are conflicts, not if the command has been rescheduled so this is safe.
+ strbuf_reset(&rev);
+ /*
+ * Clean tree, but we may need to finalize a
+ * fixup/squash chain. A failed fixup/squash leaves the
+ * file amend-type in rebase-merge/; It is okay if that
+ * file is missing, in which case there is no such
+ * chain to finalize.
+ */
+ read_oneliner(&rev, rebase_path_amend_type(), 0);
+ if (!strcmp("squash", rev.buf))
+ is_fixup = TODO_SQUASH;
+ else if (!strcmp("fixup", rev.buf)) {
+ is_fixup = TODO_FIXUP;
+ flags = (flags & ~EDIT_MSG) | CLEANUP_MSG;
I was going to say this should probably be (flags & ~(EDIT_MSG |
VERIFY_MSG)) but for some reason VERIFY_MSG isn't set here - I wonder if
it should be as I think it's set elsewhere when we edit the message.
+ }
+ }
strbuf_release(&rev);
flags |= AMEND_MSG;
}
+ if (is_clean && !is_fixup) {
+ const char *cherry_pick_head = git_path_cherry_pick_head();
+
+ if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
+ return error(_("could not remove CHERRY_PICK_HEAD"));
+ return 0;
+ }
+
if (run_git_commit(rebase_path_message(), opts, flags))
If a squash command has been skipped, then rebase_path_message() still
contains the message of the skipped commit. If it passed NULL instead
then the user would get to edit the previous version of the squash
message without the skipped commit message in it.
Also I think we only want to re-commit if the skipped squash/fixup was
preceded by another squash/fixup. If the user skips the first
squash/fixup in a chain then HEAD has the commit message from the
original pick so does not need amending. The first patch could perhaps
avoid writing rebase_path_amend_type() in that case by reading the
squash message and checking the message count is greater than two.
Best Wishes
Phillip
return error(_("could not commit staged changes."));
unlink(rebase_path_amend());
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index b177baee322..4880bff82ff 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -88,7 +88,7 @@ test_expect_success 'rebase passes merge strategy options correctly' '
git rebase --continue
'
-test_expect_failure '--continue after failed fixup cleans commit message' '
+test_expect_success '--continue after failed fixup cleans commit message' '
git checkout -b with-conflicting-fixup &&
test_commit wants-fixup &&
test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 &&
