Re: [PATCH 3/3] rebase --skip: clean up commit message after a failedfixup/squash

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 &&





[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux