[PATCH 0/2] rebase -i: respect commit.cleanup when picking fixups

[PATCH 0/2] rebase -i: respect commit.cleanup when picking fixups

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

This series fixes a recent bug report[2] where "git rebase
--autosquash" left comments in the commit message when squashing fixup
commits. The first patch fixes the bug and the second removes some
code that becomes unused after the changes in the first patch. Thanks
to Simon for reporting this.

[1] <CA+itcS3DxbgpFy2aPRvHQvTAYE=dU0kfeDdidVwWLU=rBAWR4w@mail.gmail.com>

Base-Commit: 92c87bdc406e5bcf1b516a861d609b03d99c23b7
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-commit-msg-cleanup%2Fv1
View-Changes-At: https://github.com/phillipwood/git/compare/92c87bdc4...9c7f4e39c
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-commit-msg-cleanup/v1


Phillip Wood (2):
  rebase -i: respect commit.cleanup when picking fixups
  sequencer: remove VERBATIM_MSG flag

 sequencer.c                  | 12 ------------
 t/t3415-rebase-autosquash.sh | 28 ++++++++++++++++++++++------
 2 files changed, 22 insertions(+), 18 deletions(-)

-- 
2.51.0.595.g259a2096d9c

[PATCH 1/2] rebase -i: respect commit.cleanup when picking fixups

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If the user uses a prepare-commit-msg hook to add comments to the
commit message template and sets commit.cleanup to remove them when the
commit is created then the comments will not be removed when rebase
commits the final command in a chain of "fixup" commands[1].  This
happens because f7d42ceec52 (rebase -i: do leave commit message intact
in fixup! chains, 2021-01-28) started passing the VERBATIM_MSG flag
when committing the final command in a chain of "fixup" commands. That
change was added in response to a bug report[2] where the commit
message was being cleaned up when it should not be. The cause of that
bug was that before f7d42ceec52 the sequencer passed CLEANUP_MSG
when committing the final fixup. That commit should have simply
removed the CLEANUP_MSG flag, not changed it to VERBATIM_MSG. Using
VERBATIM_MSG ignores the user's commit.cleanup config when committing
the final fixup which means it behaves differently to an ordinary
"pick" command which respects commit.cleanup.

Fix this by not setting an explicit cleanup flag when committing the
final fixup which matches the way "pick" commands behave. The test
added in f7d42ceec52 is replaced with one that checks that "fixup"
and "pick" commands do not clean up the message when commit.cleanup
is not set and do clean up the message when it is set.

[1] https://lore.kernel.org/git/CA+itcS3DxbgpFy2aPRvHQvTAYE=dU0kfeDdidVwWLU=rBAWR4w@mail.gmail.com
[2] https://lore.kernel.org/git/CANVGpwZGbzYLMeMze64e_OU9p3bjyEgzC5thmNBr6LttBt+YGw@mail.gmail.com

Reported-by: Simon Cheng <cyqsimon@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                  |  1 -
 t/t3415-rebase-autosquash.sh | 28 ++++++++++++++++++++++------
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 9ae40a91b24..47c57c2ea3c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2436,7 +2436,6 @@ 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 |= VERBATIM_MSG;
 			msg_file = rebase_path_fixup_msg();
 		} else {
 			const char *dest = git_path_squash_msg(r);
diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 5d093e3a7ab..5033411a431 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -486,12 +486,28 @@ 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_expect_success 'pick and fixup respect commit.cleanup' '
+	git reset --hard base &&
+	test_commit --no-tag "fixup! second commit" file1 fixup &&
+	test_commit something &&
+	write_script .git/hooks/prepare-commit-msg <<-\EOF &&
+	printf "\n# Prepared\n" >> "$1"
+	EOF
+	git rebase -i --autosquash HEAD~3 &&
+	test_commit_message HEAD~1 <<-\EOF &&
+	second commit
+
+	# Prepared
+	EOF
+	test_commit_message HEAD <<-\EOF &&
+	something
+
+	# Prepared
+	EOF
+	git reset --hard something &&
+	git -c commit.cleanup=strip rebase -i --autosquash HEAD~3 &&
+	test_commit_message HEAD~1 -m "second commit" &&
+	test_commit_message HEAD -m "something"
 '
 
 test_done
-- 
2.51.0.595.g259a2096d9c

Re: [PATCH 1/2] rebase -i: respect commit.cleanup when picking fixups

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> ... The cause of that
> bug was that before f7d42ceec52 the sequencer passed CLEANUP_MSG
> when committing the final fixup. That commit should have simply
> removed the CLEANUP_MSG flag, not changed it to VERBATIM_MSG. Using
> VERBATIM_MSG ignores the user's commit.cleanup config when committing
> the final fixup which means it behaves differently to an ordinary
> "pick" command which respects commit.cleanup.

So cleanly analysed and clearly described. 

> diff --git a/sequencer.c b/sequencer.c
> index 9ae40a91b24..47c57c2ea3c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2436,7 +2436,6 @@ 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 |= VERBATIM_MSG;
>  			msg_file = rebase_path_fixup_msg();
>  		} else {
>  			const char *dest = git_path_squash_msg(r);

And this is the location the earlier commit replaced the code that
sets CLEANUP_MSG, which matches the fix described in the proposed
log message.  Makes sense.

The commit at the source of the bug is what introduced VERBATIM_MSG,
and I think it can be removed (this was the only place it was set).

And you removed it in a separate step ;-).  Well done.

Thanks.

[PATCH 2/2] sequencer: remove VERBATIM_MSG flag

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As the last commit deleted the only user of VERBATIM_MSG remove
it. This reverts remaining parts of commit f7d42ceec52 (rebase -i:
do leave commit message intact in fixup! chains, 2021-01-28) that
were not deleted by the last commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 47c57c2ea3c..6d29a938aa5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1087,7 +1087,6 @@ 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)
 {
@@ -1125,9 +1124,6 @@ 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) &&
@@ -1166,8 +1162,6 @@ 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) &&
@@ -1540,9 +1534,6 @@ 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;
 
@@ -1618,8 +1609,6 @@ 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;
-- 
2.51.0.595.g259a2096d9c