{"thread":{"id":"50836","subject":"[PATCH] sequencer: fix cleanup with --signoff and -x","startedAt":"2019-03-29T11:11:08Z","lastAt":"2019-03-29T15:51:58Z","messageCount":2,"participants":["Phillip Wood","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"372685","messageId":"20190329110842.30604-1-phillip.wood123@gmail.com","threadId":"50836","inReplyTo":null,"subject":"[PATCH] sequencer: fix cleanup with --signoff and -x","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-03-29T11:08:42Z","receivedAt":"2019-03-29T11:11:08Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nBefore commit 356ee4659b (\"sequencer: try to commit without forking 'git\ncommit'\", 2017-11-24) when --signoff or -x were given on the command\nline the commit message was cleaned up with --cleanup=space or\ncommit.cleanup if it was set. Unfortunately this behavior was lost when\nI implemented committing without forking. Fix this and add some tests to\ncatch future regressions.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n\nThis clashes with dl/merge-cleanup-scissors-fix, I've sent some\nrebased patches to Denton and he's going to send a re-roll based on\nthose.\n\n sequencer.c              | 24 +++++++++++++++++-------\n sequencer.h              |  1 +\n t/t3511-cherry-pick-x.sh | 20 ++++++++++++++++++++\n 3 files changed, 38 insertions(+), 7 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0db410d590..15457bbe71 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -172,17 +172,22 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n \t\tif (status)\n \t\t\treturn status;\n \n-\t\tif (!strcmp(s, \"verbatim\"))\n+\t\tif (!strcmp(s, \"verbatim\")) {\n \t\t\topts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;\n-\t\telse if (!strcmp(s, \"whitespace\"))\n+\t\t\topts->explicit_cleanup = 1;\n+\t\t} else if (!strcmp(s, \"whitespace\")) {\n \t\t\topts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;\n-\t\telse if (!strcmp(s, \"strip\"))\n+\t\t\topts->explicit_cleanup = 1;\n+\t\t} else if (!strcmp(s, \"strip\")) {\n \t\t\topts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;\n-\t\telse if (!strcmp(s, \"scissors\"))\n+\t\t\topts->explicit_cleanup = 1;\n+\t\t} else if (!strcmp(s, \"scissors\")) {\n \t\t\topts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;\n-\t\telse\n+\t\t\topts->explicit_cleanup = 1;\n+\t\t} else {\n \t\t\twarning(_(\"invalid commit message cleanup mode '%s'\"),\n \t\t\t\t  s);\n+\t\t}\n \n \t\tfree((char *)s);\n \t\treturn status;\n@@ -1382,8 +1387,13 @@ static int try_to_commit(struct repository *r,\n \t\tmsg = &commit_msg;\n \t}\n \n-\tcleanup = (flags & CLEANUP_MSG) ? COMMIT_MSG_CLEANUP_ALL :\n-\t\t\t\t\t  opts->default_msg_cleanup;\n+\tif (flags & CLEANUP_MSG)\n+\t\tcleanup = COMMIT_MSG_CLEANUP_ALL;\n+\telse if ((opts->signoff || opts->record_origin) &&\n+\t\t !opts->explicit_cleanup)\n+\t\tcleanup = COMMIT_MSG_CLEANUP_SPACE;\n+\telse\n+\t\tcleanup = opts->default_msg_cleanup;\n \n \tif (cleanup != COMMIT_MSG_CLEANUP_NONE)\n \t\tstrbuf_stripspace(msg, cleanup == COMMIT_MSG_CLEANUP_ALL);\ndiff --git a/sequencer.h b/sequencer.h\nindex 4d505b3590..82bc7a48d5 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -47,6 +47,7 @@ struct replay_opts {\n \n \tchar *gpg_sign;\n \tenum commit_msg_cleanup_mode default_msg_cleanup;\n+\tint explicit_cleanup;\n \n \t/* Merge strategy */\n \tchar *strategy;\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex 9888bf34b9..84a587daf3 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -298,4 +298,24 @@ test_expect_success 'cherry-pick preserves commit message' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -x cleans commit message' '\n+\tpristine_detach initial &&\n+\tgit cherry-pick -x mesg-unclean &&\n+\tgit log -1 --pretty=format:%B >actual &&\n+\tprintf \"%s\\n(cherry picked from commit %s)\\n\" \\\n+\t\t\"$mesg_unclean\" $(git rev-parse mesg-unclean) |\n+\t\t\tgit stripspace >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'cherry-pick -x respects commit.cleanup' '\n+\tpristine_detach initial &&\n+\tgit -c commit.cleanup=strip cherry-pick -x mesg-unclean &&\n+\tgit log -1 --pretty=format:%B >actual &&\n+\tprintf \"%s\\n(cherry picked from commit %s)\\n\" \\\n+\t\t\"$mesg_unclean\" $(git rev-parse mesg-unclean) |\n+\t\t\tgit stripspace -s >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.21.0\n\n"},{"id":"372710","messageId":"nycvar.QRO.7.76.6.1903291651340.41@tvgsbejvaqbjf.bet","threadId":"50836","inReplyTo":"20190329110842.30604-1-phillip.wood123@gmail.com","subject":"Re: [PATCH] sequencer: fix cleanup with --signoff and -x","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-29T15:51:50Z","receivedAt":"2019-03-29T15:51:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Fri, 29 Mar 2019, Phillip Wood wrote:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> Before commit 356ee4659b (\"sequencer: try to commit without forking 'git\n> commit'\", 2017-11-24) when --signoff or -x were given on the command\n> line the commit message was cleaned up with --cleanup=space or\n> commit.cleanup if it was set. Unfortunately this behavior was lost when\n> I implemented committing without forking. Fix this and add some tests to\n> catch future regressions.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n\nLooks good to me!\n\nThank you,\nDscho\n\n>\n> This clashes with dl/merge-cleanup-scissors-fix, I've sent some\n> rebased patches to Denton and he's going to send a re-roll based on\n> those.\n>\n>  sequencer.c              | 24 +++++++++++++++++-------\n>  sequencer.h              |  1 +\n>  t/t3511-cherry-pick-x.sh | 20 ++++++++++++++++++++\n>  3 files changed, 38 insertions(+), 7 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 0db410d590..15457bbe71 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -172,17 +172,22 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n>  \t\tif (status)\n>  \t\t\treturn status;\n>\n> -\t\tif (!strcmp(s, \"verbatim\"))\n> +\t\tif (!strcmp(s, \"verbatim\")) {\n>  \t\t\topts->default_msg_cleanup = COMMIT_MSG_CLEANUP_NONE;\n> -\t\telse if (!strcmp(s, \"whitespace\"))\n> +\t\t\topts->explicit_cleanup = 1;\n> +\t\t} else if (!strcmp(s, \"whitespace\")) {\n>  \t\t\topts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;\n> -\t\telse if (!strcmp(s, \"strip\"))\n> +\t\t\topts->explicit_cleanup = 1;\n> +\t\t} else if (!strcmp(s, \"strip\")) {\n>  \t\t\topts->default_msg_cleanup = COMMIT_MSG_CLEANUP_ALL;\n> -\t\telse if (!strcmp(s, \"scissors\"))\n> +\t\t\topts->explicit_cleanup = 1;\n> +\t\t} else if (!strcmp(s, \"scissors\")) {\n>  \t\t\topts->default_msg_cleanup = COMMIT_MSG_CLEANUP_SPACE;\n> -\t\telse\n> +\t\t\topts->explicit_cleanup = 1;\n> +\t\t} else {\n>  \t\t\twarning(_(\"invalid commit message cleanup mode '%s'\"),\n>  \t\t\t\t  s);\n> +\t\t}\n>\n>  \t\tfree((char *)s);\n>  \t\treturn status;\n> @@ -1382,8 +1387,13 @@ static int try_to_commit(struct repository *r,\n>  \t\tmsg = &commit_msg;\n>  \t}\n>\n> -\tcleanup = (flags & CLEANUP_MSG) ? COMMIT_MSG_CLEANUP_ALL :\n> -\t\t\t\t\t  opts->default_msg_cleanup;\n> +\tif (flags & CLEANUP_MSG)\n> +\t\tcleanup = COMMIT_MSG_CLEANUP_ALL;\n> +\telse if ((opts->signoff || opts->record_origin) &&\n> +\t\t !opts->explicit_cleanup)\n> +\t\tcleanup = COMMIT_MSG_CLEANUP_SPACE;\n> +\telse\n> +\t\tcleanup = opts->default_msg_cleanup;\n>\n>  \tif (cleanup != COMMIT_MSG_CLEANUP_NONE)\n>  \t\tstrbuf_stripspace(msg, cleanup == COMMIT_MSG_CLEANUP_ALL);\n> diff --git a/sequencer.h b/sequencer.h\n> index 4d505b3590..82bc7a48d5 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -47,6 +47,7 @@ struct replay_opts {\n>\n>  \tchar *gpg_sign;\n>  \tenum commit_msg_cleanup_mode default_msg_cleanup;\n> +\tint explicit_cleanup;\n>\n>  \t/* Merge strategy */\n>  \tchar *strategy;\n> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\n> index 9888bf34b9..84a587daf3 100755\n> --- a/t/t3511-cherry-pick-x.sh\n> +++ b/t/t3511-cherry-pick-x.sh\n> @@ -298,4 +298,24 @@ test_expect_success 'cherry-pick preserves commit message' '\n>  \ttest_cmp expect actual\n>  '\n>\n> +test_expect_success 'cherry-pick -x cleans commit message' '\n> +\tpristine_detach initial &&\n> +\tgit cherry-pick -x mesg-unclean &&\n> +\tgit log -1 --pretty=format:%B >actual &&\n> +\tprintf \"%s\\n(cherry picked from commit %s)\\n\" \\\n> +\t\t\"$mesg_unclean\" $(git rev-parse mesg-unclean) |\n> +\t\t\tgit stripspace >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'cherry-pick -x respects commit.cleanup' '\n> +\tpristine_detach initial &&\n> +\tgit -c commit.cleanup=strip cherry-pick -x mesg-unclean &&\n> +\tgit log -1 --pretty=format:%B >actual &&\n> +\tprintf \"%s\\n(cherry picked from commit %s)\\n\" \\\n> +\t\t\"$mesg_unclean\" $(git rev-parse mesg-unclean) |\n> +\t\t\tgit stripspace -s >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n> --\n> 2.21.0\n>\n>\n"}]}