[PATCH 0/3] rebase -i: avoid stale "# This is a combination of" in commit messages

[PATCH 0/3] rebase -i: avoid stale "# This is a combination of" in commit messages

[Johannes Schindelin]

Eric Sunshine pointed out that I had such a commit message in
https://public-inbox.org/git/CAPig+cRrS0_nYJJY=O6cboV630sNQHPV5QGrQdD8MW-sYzNFGQ@mail.gmail.com/
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.


Johannes Schindelin (3):
  rebase -i: demonstrate bug with fixup!/squash! commit messages
  sequencer: leave a tell-tale when a fixup/squash failed
  rebase --skip: clean up commit message after a failed fixup/squash

 sequencer.c                | 57 ++++++++++++++++++++++++++++++++------
 t/t3418-rebase-continue.sh | 21 ++++++++++++++
 2 files changed, 69 insertions(+), 9 deletions(-)


base-commit: fe0a9eaf31dd0c349ae4308498c33a5c3794b293
Published-As: https://github.com/dscho/git/releases/tag/clean-msg-after-fixup-continue-v1
Fetch-It-Via: git fetch https://github.com/dscho/git clean-msg-after-fixup-continue-v1
-- 
2.17.0.windows.1.15.gaa56ade3205

[PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Johannes Schindelin]

When multiple fixup/squash commands are processed and the last one
causes merge conflicts and is skipped, we leave the "This is a
combination of ..." comments in the commit message.

Noticed by Eric Sunshine.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3418-rebase-continue.sh | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 9214d0bb511..b177baee322 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -88,6 +88,27 @@ test_expect_success 'rebase passes merge strategy options correctly' '
 	git rebase --continue
 '
 
+test_expect_failure '--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 &&
+	test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 &&
+	test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 &&
+	test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
+		git rebase -i HEAD~4 &&
+
+	: now there is a conflict, and comments in the commit message &&
+	git show HEAD >out &&
+	grep "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_expect_success 'setup rerere database' '
 	rm -fr .git/rebase-* &&
 	git reset --hard commit-new-file-F3-on-topic-branch &&
-- 
2.17.0.windows.1.15.gaa56ade3205

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Stefan Beller]

On Fri, Apr 20, 2018 at 5:17 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> When multiple fixup/squash commands are processed and the last one
> causes merge conflicts and is skipped, we leave the "This is a
> combination of ..." comments in the commit message.
>
> Noticed by Eric Sunshine.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t3418-rebase-continue.sh | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 9214d0bb511..b177baee322 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -88,6 +88,27 @@ test_expect_success 'rebase passes merge strategy options correctly' '
>         git rebase --continue
>  '
>
> +test_expect_failure '--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 &&
> +       test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 &&
> +       test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 &&
> +       test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
> +               git rebase -i HEAD~4 &&
> +
> +       : now there is a conflict, and comments in the commit message &&
> +       git show HEAD >out &&
> +       grep "This is a combination of" out &&

test_i18n_grep ?

> +
> +       : 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

same

Thanks,
Stefan

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Johannes Schindelin]

Hi Stefan,

On Fri, 20 Apr 2018, Stefan Beller wrote:

> On Fri, Apr 20, 2018 at 5:17 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > When multiple fixup/squash commands are processed and the last one
> > causes merge conflicts and is skipped, we leave the "This is a
> > combination of ..." comments in the commit message.
> >
> > Noticed by Eric Sunshine.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  t/t3418-rebase-continue.sh | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> > index 9214d0bb511..b177baee322 100755
> > --- a/t/t3418-rebase-continue.sh
> > +++ b/t/t3418-rebase-continue.sh
> > @@ -88,6 +88,27 @@ test_expect_success 'rebase passes merge strategy options correctly' '
> >         git rebase --continue
> >  '
> >
> > +test_expect_failure '--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 &&
> > +       test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 &&
> > +       test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 &&
> > +       test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
> > +               git rebase -i HEAD~4 &&
> > +
> > +       : now there is a conflict, and comments in the commit message &&
> > +       git show HEAD >out &&
> > +       grep "This is a combination of" out &&
> 
> test_i18n_grep ?

Funny thing is: I tested this with GETTEXT_POISON=1, and it succeeded. So
I dug deeper why: I never understood that this is a *build* option. I
always thought that would be a test-time option... Oh well ;-)

> > +
> > +       : 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
> 
> same

Will work on a fix. A brief test shows, however, that it is not quite as
easy as s/grep/test_i18ngrep/, something more seems to be broken.

Will keep you posted,
Dscho

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Johannes Schindelin]

Hi Stefan,

On Fri, 20 Apr 2018, Johannes Schindelin wrote:

> A brief test shows, however, that it is not quite as easy as
> s/grep/test_i18ngrep/, something more seems to be broken.

It seems that this week is my Rabbit Hole Week.

Turns out that we have a really, really long-standing bug in our rebase -i
where we construct the commit messages for fixup/squash chains.

Background: when having multiple fixup!/squash! commits for the same
original commit, the intermediate commits have messages starting with the
message

	# This is a combination of <N> commits.

and then every fixup/squash command increments that <N> and adds a header

	# This is the commit message #<N>:

before writing the respective commit message.

The problem arises from the fact that we deduce <N> from parsing the first
number in ASCII encoding on the first line.

That breaks e.g. when compiling with GETTEXT_POISON, and it is probably
not true in general, either.

So I introduced a patch that handles the absence of an ASCII-encoded
number gracefully, and now the test passes with and without
GETTEXT_POISON.

Thanks for the review that let me find and fix this bug!
Dscho

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Stefan Beller]

Hi Johannes,

> Funny thing is: I tested this with GETTEXT_POISON=1, and it succeeded.

I actually wanted to review the code leading to this commit, and to find
where to start reviewing I had 'git grep "This is a combination of"' which
lead me to the translation files.

s/grep/test_i18ngrep/ doesn't work as the syntax is slightly off,
s/ ! grep/ test_i18n_grep !/ would work, just pointing out the obvious.

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Johannes Schindelin]

Hi Stefan,

On Fri, 20 Apr 2018, Stefan Beller wrote:

> > Funny thing is: I tested this with GETTEXT_POISON=1, and it succeeded.
> 
> I actually wanted to review the code leading to this commit, and to find
> where to start reviewing I had 'git grep "This is a combination of"' which
> lead me to the translation files.

Heh...

> s/grep/test_i18ngrep/ doesn't work as the syntax is slightly off,
> s/ ! grep/ test_i18n_grep !/ would work, just pointing out the obvious.

Yes, I actually knew that, but my usage of a s/// as a shorthand for the
idea to replace `grep` with `test_i18ngrep` was indeed misleading. Sorry
about that, and thank you for helping me getting this right.

Ciao,
Dscho

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Eric Sunshine]

On Fri, Apr 20, 2018 at 8:17 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> When multiple fixup/squash commands are processed and the last one
> causes merge conflicts and is skipped, we leave the "This is a
> combination of ..." comments in the commit message.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> @@ -88,6 +88,27 @@ test_expect_success 'rebase passes merge strategy options correctly' '
> +test_expect_failure '--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 &&
> +       test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 &&
> +       test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 &&
> +       test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
> +               git rebase -i HEAD~4 &&
> +
> +       : now there is a conflict, and comments in the commit message &&
> +       git show HEAD >out &&
> +       grep "This is a combination of" out &&
> +
> +       : skip and continue &&
> +       git rebase --skip &&

I see that this test script doesn't utilize it, but do you want a

    test_when_finished "reset_rebase" &&

before starting the rebase to clean up in case something before "git
rebase --skip" fails?

> +       : now the comments in the commit message should have been cleaned up &&
> +       git show HEAD >out &&
> +       ! grep "This is a combination of" out
> +'

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Eric Sunshine]

On Fri, Apr 20, 2018 at 3:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Apr 20, 2018 at 8:17 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>> +       test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
>> +               git rebase -i HEAD~4 &&
>> +
>> +       : now there is a conflict, and comments in the commit message &&
>> +       git show HEAD >out &&
>> +       grep "This is a combination of" out &&
>> +
>> +       : skip and continue &&
>> +       git rebase --skip &&
>
> I see that this test script doesn't utilize it, but do you want a
>
>     test_when_finished "reset_rebase" &&
>
> before starting the rebase to clean up in case something before "git
> rebase --skip" fails?

Stated less ambiguously:

    ... in case something fails between "git rebase -i ..."
    and "git rebase --skip"?

Re: [PATCH 1/3] rebase -i: demonstrate bug with fixup!/squash! commit messages

[Johannes Schindelin]

Hi Eric,

On Fri, 20 Apr 2018, Eric Sunshine wrote:

> On Fri, Apr 20, 2018 at 3:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Fri, Apr 20, 2018 at 8:17 AM, Johannes Schindelin
> > <johannes.schindelin@gmx.de> wrote:
> >> +       test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
> >> +               git rebase -i HEAD~4 &&
> >> +
> >> +       : now there is a conflict, and comments in the commit message &&
> >> +       git show HEAD >out &&
> >> +       grep "This is a combination of" out &&
> >> +
> >> +       : skip and continue &&
> >> +       git rebase --skip &&
> >
> > I see that this test script doesn't utilize it, but do you want a
> >
> >     test_when_finished "reset_rebase" &&
> >
> > before starting the rebase to clean up in case something before "git
> > rebase --skip" fails?

No, because then one of the next test cases fails:

	not ok 15 - rebase -i --continue remembers --rerere-autoupdate

;-)

I'll use

	test_when_finished "test_might_fail git rebase --abort"

instead, okay? ;-)

Ciao,
Dscho

[PATCH 2/3] sequencer: leave a tell-tale when a fixup/squash failed

[Johannes Schindelin]

In the upcoming patch to clean up fixup/squash commit messages even when
skipping a final fixup/squash that failed with merge conflicts, we will
need to have some indicator what happened.

As we need to remove the message-fixup and message-squash files upon
failure, we cannot use those. So let's just write an explicit amend-type
file, containing either `fixup` or `squash`. The absence of that file
indicates that the we were not in the middle of a fixup or squash when
merge conflicts were happening.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 667f35ebdff..a9c3bc26f84 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -106,6 +106,13 @@ static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
  * command is processed, this file is deleted.
  */
 static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
+/*
+ * If there was a merge conflict in a fixup/squash series, we need to
+ * record the type so that a `git rebase --skip` can clean up the commit
+ * message as appropriate. This file will contain that type (`fixup` or
+ * `squash`), and not exist otherwise.
+ */
+static GIT_PATH_FUNC(rebase_path_amend_type, "rebase-merge/amend-type")
 /*
  * When we stop at a given patch via the "edit" command, this file contains
  * the abbreviated commit name of the corresponding patch.
@@ -2392,10 +2399,20 @@ static int error_with_patch(struct commit *commit,
 static int error_failed_squash(struct commit *commit,
 	struct replay_opts *opts, int subject_len, const char *subject)
 {
+	const char *amend_type = "squash";
+
+	if (file_exists(rebase_path_fixup_msg())) {
+		unlink(rebase_path_fixup_msg());
+		amend_type = "fixup";
+	}
+	if (write_message(amend_type, strlen(amend_type),
+		       rebase_path_amend_type(), 0))
+		return error(_("could not write '%s'"),
+			     rebase_path_amend_type());
+
 	if (rename(rebase_path_squash_msg(), rebase_path_message()))
 		return error(_("could not rename '%s' to '%s'"),
 			rebase_path_squash_msg(), rebase_path_message());
-	unlink(rebase_path_fixup_msg());
 	unlink(git_path_merge_msg());
 	if (copy_file(git_path_merge_msg(), rebase_path_message(), 0666))
 		return error(_("could not copy '%s' to '%s'"),
@@ -2572,6 +2589,7 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
 			unlink(rebase_path_author_script());
 			unlink(rebase_path_stopped_sha());
 			unlink(rebase_path_amend());
+			unlink(rebase_path_amend_type());
 			delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
 		}
 		if (item->command <= TODO_SQUASH) {
@@ -2799,6 +2817,7 @@ static int commit_staged_changes(struct replay_opts *opts)
 	if (run_git_commit(rebase_path_message(), opts, flags))
 		return error(_("could not commit staged changes."));
 	unlink(rebase_path_amend());
+	unlink(rebase_path_amend_type());
 	return 0;
 }
 
-- 
2.17.0.windows.1.15.gaa56ade3205

Re: [PATCH 2/3] sequencer: leave a tell-tale when a fixup/squash failed

[Eric Sunshine]

On Fri, Apr 20, 2018 at 8:18 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> In the upcoming patch to clean up fixup/squash commit messages even when
> skipping a final fixup/squash that failed with merge conflicts, we will
> need to have some indicator what happened.
>
> As we need to remove the message-fixup and message-squash files upon
> failure, we cannot use those. So let's just write an explicit amend-type
> file, containing either `fixup` or `squash`. The absence of that file
> indicates that the we were not in the middle of a fixup or squash when

s/the we/we/

> merge conflicts were happening.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

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

[Johannes Schindelin]

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.

This commit fixes that behavior.

The diff is best viewed with --color-moved.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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)) {
+			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;
+			}
+		}
 
 		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))
 		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 &&
-- 
2.17.0.windows.1.15.gaa56ade3205

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

[Phillip Wood]

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@gmx.de>
> ---
>   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 &&
> 

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

[Johannes Schindelin]

Hi Phillip,

On Sat, 21 Apr 2018, Phillip Wood wrote:

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

It was on my mind, but since I got caught twice by this bug within a week,
I figured it was about time.

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

This is indeed the intent of that file.

> > +			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.

As this patch series is purely about the bug fix where interrupted
fixup/squash series can lead to incorrect commit messages, I would say
that if this is a bug, it should be fixed in a separate patch series.

The name of that option is actually a little bit unfortunate: it bypasses
the pre-commit/commit-msg hooks. I am not sure why they are bypassed in
the commit_staged_changes() function.

*clickety-click*

It would appear that I simply copied this from

https://github.com/git/git/blob/v2.17.0/git-rebase--interactive.sh#L794-L808

So where does that come from? Let's use `git log -L
794,808:git-rebase--interactive.sh v2.17.0` to find out.

*clickety-click*

From that log, it looks as if this was added in 2147f844ed1 (rebase -i: handle
fixup of root commit correctly, 2012-07-24). But that is incorrect: the
--no-verify invocation was only split into two by said commit, and moved
into the conditional. So we need to look a little further, with a larger
line range (I extended it to 810 for the purpose of this analysis).

*clickety-click*

So it goes all the way back to c5b09feb786 (Avoid update hook during
git-rebase --interactive, 2007-12-19). From that commit message, you can
see that the rationale for the --no-verify flag was as following: the `git
commit` might fail when continuing with staged changes, due to a check in
a hook, and since there is inadequate error checking in the
git-rebase--interactive.sh script, it would continue and squash the
changes into the next commit.

From that description, it would appear that the proper fix would have been
to 1) introduce proper error checking, and 2) offer a mode to bypass the
hooks for *all* of the interactive rebase.

It is funny that 1) was addressed apparently 4 minutes later, in
dbedf9729bd (Catch and handle git-commit failures in git-rebase
--interactive, 2007-12-19).

As to 2): it seems to have been implemented in c44276563f9 (rebase
--no-verify, 2008-10-06), but by that time it was probably safely
forgotten that the --no-verify option was only introduced as a work-around
for the absence of the `git rebase -i --no-verify` mode.

So I guess it would be time to undo c5b09feb786...

But as I said, I'd rather really not taint this here patch series by an
unrelated bug fix. It has gone to the length of four iterations already
even without such distractions.

> > +			}
> > +		}
> >   
> >     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.

True. One problem that I only caught late in my work on v4 is that
run_git_commit() did not handle the absence of defmsg *and* EDIT_MSG well:
in that case, it always opened the editor. I addressed that in the new
3/4. (The old 3/4 is no longer needed: it wrote the amend-type file whose
purpose is now folded into current-fixups, introduced in the new 2/4.)

> Also I think we only want to re-commit if the skipped squash/fixup was
> preceded by another squash/fixup.

I thought so, too, at first. But I think that is still not quite the
correct thing: we really only want to re-commit if we are in the middle of
the final fixup/squash of the fixup/squash chain. And only if that final
fixup/squash was not the only one in that chain.

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

As I mentioned in another reply to you, I redid the whole shebang so that
we always write a `current-fixups` file as soon as we encounter a fixup or
squash. This file will build up the fixup/squash chain.

One thing that I had missed when I wrote that reply is that we sometimes
do *not* skip a failed fixup/squash, but instead resolve the merge
conflicts, stage the changes and call `git rebase --continue`. In this
case, `git rebase` will commit the changes, *opening the commit message in
an editor*. Therefore, the fixup/squash chain is broken at that point, and
we have to treat the current fixup/squash as if it were the final in the
current chain.

I imagine that it will take a while to review v4 in depth ;-)

Ciao,
Dscho