Junio C Hamano venit, vidit, dixit 23.02.2015 19:54: > Michael J Gruber <g...@drmicha.warpmail.net> writes: > >> sequencer calls "commit" with default options, which implies >> "--cleanup=default" unless the user specified something else in their >> config. This leads to cherry-picked commits getting a cleaned up commit >> message, which is usually not an intended side-effect. >> >> Make the sequencer use "--cleanup=verbatim" so that it preserves commit >> messages independent of the defaults and user config for "commit". > > Hmm, wouldn't it introduce a grave regression for users who > explicitly ask to clean crufty messages up (by setting their own > commit.cleanup configuration) if you unconditionally force > "--cleanup=verbatim" here? >
That's what I meant by possible side-effects below. There are no side-effects which our tests would catch. I don't know sequencer.c well enough to know whether run_git_commit() would be run with a user-edited commit message at all. My (possibly wrong) understanding is that it is called only when a cherry-pick succeeds without any conflicts, so that it is called with a commit message from a pre-existing commit, i.e. a message after cleanup which is to be preserved as is. In case of a conflict, resolution is left to be done by the user. But I guess I'm overlooking --edit and --continue here. But git cherry-pick without conflict should no re-cleanup the commit message either, should it? >> Reported-by: Christoph Anton Mitterer <cales...@scientia.net> >> Signed-off-by: Michael J Gruber <g...@drmicha.warpmail.net> >> --- >> >> Notes: >> All tests run fine with this changed behavior. I don't know >> whether this may have any side-effects on other (untested) >> uses of the sequencer. >> >> sequencer.c | 1 + >> t/t3511-cherry-pick-x.sh | 28 ++++++++++++++++++++++++++++ >> 2 files changed, 29 insertions(+) >> >> diff --git a/sequencer.c b/sequencer.c >> index 77a1266..35fe9d9 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -377,6 +377,7 @@ static int run_git_commit(const char *defmsg, struct >> replay_opts *opts, >> argv_array_init(&array); >> argv_array_push(&array, "commit"); >> argv_array_push(&array, "-n"); >> + argv_array_push(&array, "--cleanup=verbatim"); > > > >> >> if (opts->gpg_sign) >> argv_array_pushf(&array, "-S%s", opts->gpg_sign); >> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh >> index f977279..b7dff09 100755 >> --- a/t/t3511-cherry-pick-x.sh >> +++ b/t/t3511-cherry-pick-x.sh >> @@ -36,6 +36,20 @@ mesg_with_cherry_footer="$mesg_with_footer_sob >> (cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709) >> Tested-by: C.U. Thor <cut...@example.com>" >> >> +mesg_unclean="$mesg_one_line >> + >> + >> +leading empty lines >> + >> + >> +consecutive empty lines >> + >> +# hash tag comment >> + >> +trailing empty lines >> + >> + >> +" >> >> test_expect_success setup ' >> git config advice.detachedhead false && >> @@ -53,6 +67,10 @@ test_expect_success setup ' >> test_commit "$mesg_with_footer_sob" foo b mesg-with-footer-sob && >> git reset --hard initial && >> test_commit "$mesg_with_cherry_footer" foo b mesg-with-cherry-footer && >> + git reset --hard initial && >> + test_config commit.cleanup verbatim && >> + test_commit "$mesg_unclean" foo b mesg-unclean && >> + test_unconfig commit.cleanup && >> pristine_detach initial && >> test_commit conflicting unrelated >> ' >> @@ -216,4 +234,14 @@ test_expect_success 'cherry-pick -x -s treats "(cherry >> picked from..." line as p >> test_cmp expect actual >> ' >> >> +test_expect_success 'cherry-pick preserves commit message' ' >> + pristine_detach initial && >> + printf "$mesg_unclean" >expect && >> + git log -1 --pretty=format:%B mesg-unclean >actual && >> + test_cmp expect actual && >> + git cherry-pick mesg-unclean && >> + git log -1 --pretty=format:%B >actual && >> + test_cmp expect actual >> +' >> + >> test_done -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
