rebase -i: do leave commit message intact in fixup! chains

From: Johannes Schindelin <[email protected]>

In 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and 'squash' commands, 2017-01-02), this developer introduced a change of behavior by mistake: when encountering a fixup! commit (or multiple fixup! commits) without any squash! commit thrown in, the final git commit was invoked with --cleanup=strip. Prior to that commit, the commit command had been called without that --cleanup option.

Since we explicitly read the original commit message from a file in that case, there is really no sense in forcing that clean-up.

---
Fix bug in interactive rebases where fixup! cleans up the commit message

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-818%2Fdscho%2Fautosquash-without-scissors-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-818/dscho/autosquash-without-scissors-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/818

 sequencer.c                  | 5 ++---
 t/t3415-rebase-autosquash.sh | 8 ++++++++
 2 files changed, 10 insertions(+), 3 deletions(-)

index 8909a467700..749bddd7a1f 100644
@@ -2001,10 +2001,9 @@ static int do_pick_commit(struct repository *r,
 		flags |= AMEND_MSG;
 		if (!final_fixup)
 			msg_file = rebase_path_squash_msg();
-		else if (file_exists(rebase_path_fixup_msg())) {
-			flags |= CLEANUP_MSG;
+		else if (file_exists(rebase_path_fixup_msg()))
 			msg_file = rebase_path_fixup_msg();
-		} else {
+		else {
 			const char *dest = git_path_squash_msg(r);
 			unlink(dest);
 			if (copy_file(dest, rebase_path_squash_msg(), 0666))

index 7bab6000dc7..4c83c98b3fc 100755
@@ -440,4 +440,12 @@ test_expect_success 'fixup a fixup' '
 	test XZWY = $(git show | tr -cd W-Z)
 '
 
+test_expect_success 'fixup does not clean up commit message' '
+	oneline="#818" &&
+	git commit --allow-empty -m "$oneline" &&
+	git commit --fixup HEAD --allow-empty &&
+	git rebase -ki --autosquash HEAD~2 &&
+	test "$oneline" = "$(git show -s --format=%s)"
+'
+
 test_done

base-commit: ba2aa15129e59f248d8cdd30404bc78b5178f61d

gitgitgadget

Hi Dscho,

Hi Martin,

Thank you so much for this detailed feedback. I will take care of it next year, after taking a semi-vacation interrupted only by releasing -rc1 (check), -rc2 (TBD) and v2.30.0 final (TBD).

Ciao,
Dscho

From: Johannes Schindelin <[email protected]>

In 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and 'squash' commands, 2017-01-02), this developer introduced a change of behavior by mistake: when encountering a fixup! commit (or multiple fixup! commits) without any squash! commit thrown in, the final git commit was invoked with --cleanup=strip. Prior to that commit, the commit command had been called without that --cleanup option.

Since we explicitly read the original commit message from a file in that case, there is really no sense in forcing that clean-up.

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.

---
Fix bug in interactive rebases where fixup! cleans up the commit message

Original report here:
https://lore.kernel.org/git/CANVGpwZGbzYLMeMze64e_OU9p3bjyEgzC5thmNBr6LttBt%2BYGw%40mail.gmail.com/t

Changes since v1:

* The fix now works even if commit.cleanup = commit

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-818%2Fdscho%2Fautosquash-without-scissors-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-818/dscho/autosquash-without-scissors-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/818

Range-diff vs v1:

1: bfade3b146f ! 1: c760d6cd203 rebase -i: do leave commit message intact in fixup! chains
@@ Commit message
Since we explicitly read the original commit message from a file in that case, there is really no sense in forcing that clean-up.

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

+ Helped-by: Martin Ågren <[email protected]>

## sequencer.c ##
+@@ sequencer.c: N_("you have staged changes in your working tree\n"
+ #define CLEANUP_MSG (1<<3)
+ #define VERIFY_MSG (1<<4)
+ #define CREATE_ROOT_COMMIT (1<<5)
++#define VERBATIM_MSG (1<<6)
+
+ static int run_command_silent_on_success(struct child_process *cmd)
+ {
+@@ sequencer.c: static int run_git_commit(const char *defmsg,
+ strvec_pushl(&cmd.args, "-C", "HEAD", NULL);
+ if ((flags & CLEANUP_MSG))
+ strvec_push(&cmd.args, "--cleanup=strip");
++ if ((flags & VERBATIM_MSG))
++ strvec_push(&cmd.args, "--cleanup=verbatim");
+ if ((flags & EDIT_MSG))
+ strvec_push(&cmd.args, "-e");
+ else if (!(flags & CLEANUP_MSG) &&
+@@ sequencer.c: static int try_to_commit(struct repository *r, +
+ if (flags & CLEANUP_MSG)
+ cleanup = COMMIT_MSG_CLEANUP_ALL;
++ else if (flags & VERBATIM_MSG)
++ cleanup = COMMIT_MSG_CLEANUP_NONE;
+ else if ((opts->signoff || opts->record_origin) &&
+ !opts->explicit_cleanup)
+ cleanup = COMMIT_MSG_CLEANUP_SPACE;
@@ sequencer.c: static int do_pick_commit(struct repository *r,
- flags |= AMEND_MSG;
if (!final_fixup)
msg_file = rebase_path_squash_msg();
-- else if (file_exists(rebase_path_fixup_msg())) {
+ else if (file_exists(rebase_path_fixup_msg())) {
- flags |= CLEANUP_MSG;
-+ else if (file_exists(rebase_path_fixup_msg()))
++ flags |= VERBATIM_MSG;
msg_file = rebase_path_fixup_msg();
-- } else {
-+ else {
+ } else {
const char *dest = git_path_squash_msg(r);
- unlink(dest);
- if (copy_file(dest, rebase_path_squash_msg(), 0666))

## t/t3415-rebase-autosquash.sh ##
@@ t/t3415-rebase-autosquash.sh: test_expect_success 'fixup a fixup' '
@@ t/t3415-rebase-autosquash.sh: test_expect_success 'fixup a fixup' '
+ oneline="#818" &&
+ git commit --allow-empty -m "$oneline" &&
+ git commit --fixup HEAD --allow-empty &&
-+ git rebase -ki --autosquash HEAD~2 &&
++ git -c commit.cleanup=strip rebase -ki --autosquash HEAD~2 &&
+ test "$oneline" = "$(git show -s --format=%s)"
+'
+

 sequencer.c                  | 7 ++++++-
 t/t3415-rebase-autosquash.sh | 8 ++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

index 8909a467700..092e7b811f0 100644
@@ -943,6 +943,7 @@ N_("you have staged changes in your working tree\n"
 #define CLEANUP_MSG (1<<3)
 #define VERIFY_MSG  (1<<4)
 #define CREATE_ROOT_COMMIT (1<<5)
+#define VERBATIM_MSG (1<<6)
 
 static int run_command_silent_on_success(struct child_process *cmd)
 {

@@ -1012,6 +1013,8 @@ static int run_git_commit(const char *defmsg,
 		strvec_pushl(&cmd.args, "-C", "HEAD", NULL);
 	if ((flags & CLEANUP_MSG))
 		strvec_push(&cmd.args, "--cleanup=strip");
+	if ((flags & VERBATIM_MSG))
+		strvec_push(&cmd.args, "--cleanup=verbatim");
 	if ((flags & EDIT_MSG))
 		strvec_push(&cmd.args, "-e");
 	else if (!(flags & CLEANUP_MSG) &&

@@ -1454,6 +1457,8 @@ static int try_to_commit(struct repository *r,
 
 	if (flags & CLEANUP_MSG)
 		cleanup = COMMIT_MSG_CLEANUP_ALL;
+	else if (flags & VERBATIM_MSG)
+		cleanup = COMMIT_MSG_CLEANUP_NONE;
 	else if ((opts->signoff || opts->record_origin) &&
 		 !opts->explicit_cleanup)
 		cleanup = COMMIT_MSG_CLEANUP_SPACE;

@@ -2002,7 +2007,7 @@ static int do_pick_commit(struct repository *r,
 		if (!final_fixup)
 			msg_file = rebase_path_squash_msg();
 		else if (file_exists(rebase_path_fixup_msg())) {
-			flags |= CLEANUP_MSG;
+			flags |= VERBATIM_MSG;
 			msg_file = rebase_path_fixup_msg();
 		} else {
 			const char *dest = git_path_squash_msg(r);

index 7bab6000dc7..88040bc4352 100755
@@ -440,4 +440,12 @@ test_expect_success 'fixup a fixup' '
 	test XZWY = $(git show | tr -cd W-Z)
 '
 
+test_expect_success 'fixup does not clean up commit message' '
+	oneline="#818" &&
+	git commit --allow-empty -m "$oneline" &&
+	git commit --fixup HEAD --allow-empty &&
+	git -c commit.cleanup=strip rebase -ki --autosquash HEAD~2 &&
+	test "$oneline" = "$(git show -s --format=%s)"
+'
+
 test_done

base-commit: ba2aa15129e59f248d8cdd30404bc78b5178f61d

gitgitgadget

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

index 88040bc435..f80318998f 100755
@@ -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

Hi Martin,

Indeed, we lost that information. For what it's worth, that's totally in line with how --amend works:

	git commit --allow-empty -m '#hash'
	git commit --allow-empty --amend

This will interpret the #hash line as a comment.

And since that's the case with git commit, I would contend that this is a different issue than the one I am trying to address in this patch series, and therefore should not be handled here (not even adding the test_expect_failure cases you generously provided).

Ciao,
Dscho

"Johannes Schindelin via GitGitGadget" <[email protected]>

Good thinking.

	if ((flags & (CLEANUP_MSG|VERBATIM_MSG)) == (CLEANUP_MSG|VERBATIM_MSG))
		BUG("cleanup and verbatim asked at the same time");

Hi Junio,

I did something similar in spirit in the latest iteration.

Thanks,
Dscho

Looks good. We do not expect for the BUG() to trigger as long as programmers are careful, but it is better to be careful.

Another possibility would have been, as VERBATIM is what only we set programmatically when we do not want any clean-up to happen, make it override CLEANUP, but what is written in the updated patch is safer, I would think.

Will queue v3.

Thanks.

From: Johannes Schindelin <[email protected]>

In 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and 'squash' commands, 2017-01-02), this developer introduced a change of behavior by mistake: when encountering a fixup! commit (or multiple fixup! commits) without any squash! commit thrown in, the final git commit was invoked with --cleanup=strip. Prior to that commit, the commit command had been called without that --cleanup option.

Since we explicitly read the original commit message from a file in that case, there is really no sense in forcing that clean-up.

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.

---
Fix bug in interactive rebases where fixup! cleans up the commit message

Original report here:
https://lore.kernel.org/git/CANVGpwZGbzYLMeMze64e_OU9p3bjyEgzC5thmNBr6LttBt%2BYGw%40mail.gmail.com/t

Changes since v2:

* Added a safeguard so that CLEANUP_MSG and VERBATIM_MSG aren't used together by mistake.
* Changes since v1:
* The fix now works even if commit.cleanup = commit

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-818%2Fdscho%2Fautosquash-without-scissors-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-818/dscho/autosquash-without-scissors-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/818

Range-diff vs v2:

1: c760d6cd203 ! 1: 8acb9b4562e rebase -i: do leave commit message intact in fixup! chains
@@ sequencer.c: N_("you have staged changes in your working tree\n"

static int run_command_silent_on_success(struct child_process *cmd) {
+@@ sequencer.c: static int run_git_commit(const char *defmsg,
+ {
+ struct child_process cmd = CHILD_PROCESS_INIT;
+
++ if ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG))
++ BUG("CLEANUP_MSG and VERBATIM_MSG are mutually exclusive");
++
+ cmd.git_cmd = 1;
+
+ if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
@@ sequencer.c: static int run_git_commit(const char *defmsg,
strvec_pushl(&cmd.args, "-C", "HEAD", NULL);
if ((flags & CLEANUP_MSG))
@@ sequencer.c: static int run_git_commit(const char *defmsg,
if ((flags & EDIT_MSG))
strvec_push(&cmd.args, "-e");
else if (!(flags & CLEANUP_MSG) &&
+@@ sequencer.c: static int try_to_commit(struct repository *r,
+ enum commit_msg_cleanup_mode cleanup;
+ int res = 0;
+
++ if ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG))
++ BUG("CLEANUP_MSG and VERBATIM_MSG are mutually exclusive");
++
+ if (parse_head(r, &current_head))
+ return -1;
+
@@ sequencer.c: static int try_to_commit(struct repository *r,

if (flags & CLEANUP_MSG)

 sequencer.c                  | 13 ++++++++++++-
 t/t3415-rebase-autosquash.sh |  8 ++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

index 8909a467700..264d494a64c 100644
@@ -943,6 +943,7 @@ N_("you have staged changes in your working tree\n"
 #define CLEANUP_MSG (1<<3)
 #define VERIFY_MSG  (1<<4)
 #define CREATE_ROOT_COMMIT (1<<5)
+#define VERBATIM_MSG (1<<6)
 
 static int run_command_silent_on_success(struct child_process *cmd)
 {

@@ -979,6 +980,9 @@ static int run_git_commit(const char *defmsg,
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 
+	if ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG))
+		BUG("CLEANUP_MSG and VERBATIM_MSG are mutually exclusive");
+
 	cmd.git_cmd = 1;
 
 	if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {

@@ -1012,6 +1016,8 @@ static int run_git_commit(const char *defmsg,
 		strvec_pushl(&cmd.args, "-C", "HEAD", NULL);
 	if ((flags & CLEANUP_MSG))
 		strvec_push(&cmd.args, "--cleanup=strip");
+	if ((flags & VERBATIM_MSG))
+		strvec_push(&cmd.args, "--cleanup=verbatim");
 	if ((flags & EDIT_MSG))
 		strvec_push(&cmd.args, "-e");
 	else if (!(flags & CLEANUP_MSG) &&

@@ -1380,6 +1386,9 @@ static int try_to_commit(struct repository *r,
 	enum commit_msg_cleanup_mode cleanup;
 	int res = 0;
 
+	if ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG))
+		BUG("CLEANUP_MSG and VERBATIM_MSG are mutually exclusive");
+
 	if (parse_head(r, &current_head))
 		return -1;
 

@@ -1454,6 +1463,8 @@ static int try_to_commit(struct repository *r,
 
 	if (flags & CLEANUP_MSG)
 		cleanup = COMMIT_MSG_CLEANUP_ALL;
+	else if (flags & VERBATIM_MSG)
+		cleanup = COMMIT_MSG_CLEANUP_NONE;
 	else if ((opts->signoff || opts->record_origin) &&
 		 !opts->explicit_cleanup)
 		cleanup = COMMIT_MSG_CLEANUP_SPACE;

@@ -2002,7 +2013,7 @@ static int do_pick_commit(struct repository *r,
 		if (!final_fixup)
 			msg_file = rebase_path_squash_msg();
 		else if (file_exists(rebase_path_fixup_msg())) {
-			flags |= CLEANUP_MSG;
+			flags |= VERBATIM_MSG;
 			msg_file = rebase_path_fixup_msg();
 		} else {
 			const char *dest = git_path_squash_msg(r);

index 7bab6000dc7..88040bc4352 100755
@@ -440,4 +440,12 @@ test_expect_success 'fixup a fixup' '
 	test XZWY = $(git show | tr -cd W-Z)
 '
 
+test_expect_success 'fixup does not clean up commit message' '
+	oneline="#818" &&
+	git commit --allow-empty -m "$oneline" &&
+	git commit --fixup HEAD --allow-empty &&
+	git -c commit.cleanup=strip rebase -ki --autosquash HEAD~2 &&
+	test "$oneline" = "$(git show -s --format=%s)"
+'
+
 test_done

base-commit: ba2aa15129e59f248d8cdd30404bc78b5178f61d

gitgitgadget