Re: [PATCH] sequencer: preserve commit messages
- From
Michael J Gruber <git@drmicha.warpmail.net>
- Date
- Feb 24, 2015, 15:29 UTC
- Message-ID
- <54EC98BD.7060100@drmicha.warpmail.net>
- In-Reply-To
- <xmqqsidwtq4i.fsf@gitster.dls.corp.google.com>
Junio C Hamano venit, vidit, dixit 23.02.2015 19:54:
Show 15 quoted lines
> Michael J Gruber <git@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?
Show 79 quoted lines
>> Reported-by: Christoph Anton Mitterer <calestyo@scientia.net> >> Signed-off-by: Michael J Gruber <git@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 <cuthor@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