[BUG] git mangles up commit messages on rebase

[BUG] git mangles up commit messages on rebase

[Christoph Anton Mitterer]

[-- Attachment #1: Type: text/plain, Size: 796 bytes --]

Hey.

When I do a simple interactive rebase, e.g. something like this:
edit 78f3ba8 editing or just rewriting this commit
pick b621076 foo
pick e06c28e this one had a "verbatim" commit message
pick c0a447f bar

and one of the commit messages from the children I edit/rewrite had a
commit message that was edited with --cleanup=verbatim (e.g. double
newlines, etc.).

Then these get lost once I --continue and it appears that the messages
are recreated but with the default of --cleanup=default .

IMO that's quite annoying, cause when one intentionally chose e.g.
-cleanup=verbatim and made commit messages with that, then this is
probably what one wanted and it should be dumped just because of
changing another commit.

Could that possibly be solved? :)

Cheers,
Chris.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5313 bytes --]

[PATCH] sequencer: preserve commit messages

[Michael J Gruber]

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

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
-- 
2.3.0.296.g32c87e1

Re: [PATCH] sequencer: preserve commit messages

[Junio C Hamano]

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?

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

Re: [PATCH] sequencer: preserve commit messages

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 23.02.2015 19:54:
> 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?

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

Re: [PATCH] sequencer: preserve commit messages

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

>> 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.
> ...
> But git cherry-pick without conflict should no re-cleanup the commit
> message either, should it?

Hmm, but if it does not, wouldn't that countermand the wish of the
user who explicitly asked to clean crufty messages up by setting
their own commit.cleanup configuration?

Re: [PATCH] sequencer: preserve commit messages

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 24.02.2015 19:29:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>>> 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.
>> ...
>> But git cherry-pick without conflict should no re-cleanup the commit
>> message either, should it?
> 
> Hmm, but if it does not, wouldn't that countermand the wish of the
> user who explicitly asked to clean crufty messages up by setting
> their own commit.cleanup configuration?
> 

Note that "verbatim" is not the default - we cleanup commits even
without being asked to. And this makes sense for "git commit", of course.

I myself certainly expected "git cherry-pick" to transfer a commit as
verbatim as possible. "git rebase" preserves the commit message (at
least more than cherry-pick). What's the difference between them?
Technically the difference between commit-tree and commit, sure, but for
the user?

Michael

Re: [PATCH] sequencer: preserve commit messages

[Christoph Anton Mitterer]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]

On Tue, 2015-02-24 at 10:29 -0800, Junio C Hamano wrote: 
> Hmm, but if it does not, wouldn't that countermand the wish of the
> user who explicitly asked to clean crufty messages up by setting
> their own commit.cleanup configuration?

IMHO it's just wrong behaviour if the commit messages of people who
intentionally chose "verbatim" to get multiple newline, etc. are mangled
up, just to allow such people, that also intentionally chose some
non-default cleanup mode, but changed their mind later, allow easy clean
up.

Cheers,
Chris.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5313 bytes --]

Re: [PATCH] sequencer: preserve commit messages

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Junio C Hamano venit, vidit, dixit 24.02.2015 19:29:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>> 
>>>> 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.
>>> ...
>>> But git cherry-pick without conflict should no re-cleanup the commit
>>> message either, should it?
>> 
>> Hmm, but if it does not, wouldn't that countermand the wish of the
>> user who explicitly asked to clean crufty messages up by setting
>> their own commit.cleanup configuration?
>
> Note that "verbatim" is not the default - we cleanup commits even
> without being asked to. And this makes sense for "git commit", of course.

I am fine with the result of the updated code if the user does not
have anything in the config and uses the "default".  Not touching in
"cherry-pick" would be more desirable than cleaning.  We are in
agreement for that obvious case.

But your response is sidestepping my question, isn't it?

What does your change do to the user who wants that the clean-up to
always happen and expresses that desire by setting
commit.cleanup=strip in the config?  Doesn't the internal invocation
of "commit --cleanup=verbatim" that is unconditional override it?

Re: [PATCH] sequencer: preserve commit messages

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 25.02.2015 19:22:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Junio C Hamano venit, vidit, dixit 24.02.2015 19:29:
>>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>>>
>>>>> 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.
>>>> ...
>>>> But git cherry-pick without conflict should no re-cleanup the commit
>>>> message either, should it?
>>>
>>> Hmm, but if it does not, wouldn't that countermand the wish of the
>>> user who explicitly asked to clean crufty messages up by setting
>>> their own commit.cleanup configuration?
>>
>> Note that "verbatim" is not the default - we cleanup commits even
>> without being asked to. And this makes sense for "git commit", of course.
> 
> I am fine with the result of the updated code if the user does not
> have anything in the config and uses the "default".  Not touching in
> "cherry-pick" would be more desirable than cleaning.  We are in
> agreement for that obvious case.

I didn't know we were. It's clear now.

> But your response is sidestepping my question, isn't it?

I simply misunderstood it.

> What does your change do to the user who wants that the clean-up to
> always happen and expresses that desire by setting
> commit.cleanup=strip in the config?  Doesn't the internal invocation
> of "commit --cleanup=verbatim" that is unconditional override it?
> 

Yes, it obviously overrides it.

I have to re-check which cleanups rebase does. I hope none.

But I would think that to clean up a commit message according to the
current config settings, a user should have to "commit --amend" or
"rebase -i with reword" explicitly.

I still think of rebase and cherry-picks as means to transplant a commit
as unchanged as possible.

Now, if there are conflicts and the user has to resolve them, they will
use "git commit" themselves with their current config in effect. That is
to be effected, and the user can use "git commit --cleanup=..." however
they want.

That leaves the case of "git cherry-pick --edit". I could easily catch
that and not overrride config in this case. But the user cannot
influence that other than by using "git -c commit.cleanup=...
cherry-pick --edit".

Hmm. With "--edit", current config being in effect should be expected,
right? So how about:

In case of no conflict: force cleanup=verbatim unless --edit is used?

Michael

Re: [PATCH] sequencer: preserve commit messages

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Hmm. With "--edit", current config being in effect should be expected,
> right? So how about:
>
> In case of no conflict: force cleanup=verbatim unless --edit is used?

Perhaps something like that.

Stepping back a bit and imagine a world where the sole purpose of
cherry-pick were to recreate the original commit as faithfully as
possible.  The commit log message would not be cleaned up in such
a case by default, and the users need cherrypick.cleanup setting
if they do not like that default.

The implementation of cherry-pick that does not spawn the editor
in that world would look like this:

    - read the cleanup mode from cherrypick.cleanup config; if there
      is none, read the cleanup mode from commit.cleanup config; if
      neither is defined, then use 'verbatim' as the default;

    - invoke "commit --cleanup=" + that mode from the command line
      to force the mode chosen by the above.

Thanks to the falling back to commit.cleanup, the above logic would
be usable even before we invent cherrypick.cleanup configuration,
i.e. in today's world.  If there is no commit.cleanup defined by the
user, the above logic would still use 'verbatim' as the default for
'cherry-pick', while using the 'default' for 'commit'.

When cherry-pick invokes the editor, then the first part would be
different.  So my conclusion would be something like:

    #if IN_THE_FUTURE
        if (config_exists(cherrypick.cleanup))
            mode = config_value(cherrypick.cleanup);
        else
    #endif
        if (config_exists(commit.cleanup))
            mode = config_value(commit.cleanup);
        else
            mode = editing ? 'verbatim' : 'default';

        invoke "commit --cleanup=" + mode;

perhaps?

Re: [PATCH] sequencer: preserve commit messages

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 26.02.2015 20:49:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Hmm. With "--edit", current config being in effect should be expected,
>> right? So how about:
>>
>> In case of no conflict: force cleanup=verbatim unless --edit is used?
> 
> Perhaps something like that.
> 
> Stepping back a bit and imagine a world where the sole purpose of
> cherry-pick were to recreate the original commit as faithfully as
> possible.  The commit log message would not be cleaned up in such
> a case by default, and the users need cherrypick.cleanup setting
> if they do not like that default.
> 
> The implementation of cherry-pick that does not spawn the editor
> in that world would look like this:
> 
>     - read the cleanup mode from cherrypick.cleanup config; if there
>       is none, read the cleanup mode from commit.cleanup config; if
>       neither is defined, then use 'verbatim' as the default;
> 
>     - invoke "commit --cleanup=" + that mode from the command line
>       to force the mode chosen by the above.
> 
> Thanks to the falling back to commit.cleanup, the above logic would
> be usable even before we invent cherrypick.cleanup configuration,
> i.e. in today's world.  If there is no commit.cleanup defined by the
> user, the above logic would still use 'verbatim' as the default for
> 'cherry-pick', while using the 'default' for 'commit'.
> 
> When cherry-pick invokes the editor, then the first part would be
> different.  So my conclusion would be something like:
> 
>     #if IN_THE_FUTURE
>         if (config_exists(cherrypick.cleanup))
>             mode = config_value(cherrypick.cleanup);
>         else
>     #endif
>         if (config_exists(commit.cleanup))
>             mode = config_value(commit.cleanup);
>         else
>             mode = editing ? 'verbatim' : 'default';
> 
>         invoke "commit --cleanup=" + mode;
> 
> perhaps?
> 

Without any config being set the result is certainly what I'm after.

What I'm still wondering about is the case without --edit but with
commit.cleanup: It seems to me that "git commit" being involved in a
conflict-less cherry-pick is solely an implemention detail (and it could
be done differently). Applying commit.* in this situation is a total
surpise to the normal user, isn't it? I mean, again, what's the
difference to rebase from a user perspective?

Michael

Re: [PATCH] sequencer: preserve commit messages

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Without any config being set the result is certainly what I'm after.
>
> What I'm still wondering about is the case without --edit but with
> commit.cleanup: It seems to me that "git commit" being involved in a
> conflict-less cherry-pick is solely an implemention detail (and it could
> be done differently). Applying commit.* in this situation is a total
> surpise to the normal user, isn't it? I mean, again, what's the
> difference to rebase from a user perspective?

OK, a revised logic with the above input from you may look like
this:

     #if IN_THE_FUTURE
         if (config_exists(cherrypick.cleanup))
             mode = config_value(cherrypick.cleanup);
         else
     #endif
         if (editing && config_exists(commit.cleanup))
             mode = config_value(commit.cleanup);
         else
             mode = 'verbatim';
 
         invoke "commit --cleanup=" + mode;

This is a change in behavoiur (I just checked with v1.6.0 codebase
and we seem to run a clean-up without "--edit"); what is our plan to
help those who have been relying on the auto clean-up behaviour?

Also a tangent.

I recently run "cherry-pick -s" on two commits, and I am not sure if
"with --edit, and only with -edit, do the usual clean-up" is a
sensible thing to do, or "-s" or any other option should trigger the
usual clean-up if it implies that the user understands and asks the
log message to be different from the original (I am leaning towards
the latter).

[PATCHv2] sequencer: preserve commit messages

[Michael J Gruber]

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 default, unless the user has set config for "commit"
or the message is amended with -s or -x.

Reported-by: Christoph Anton Mitterer <calestyo@scientia.net>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 sequencer.c              |  5 +++++
 t/t3511-cherry-pick-x.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 77a1266..8cf575c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -373,6 +373,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 {
 	struct argv_array array;
 	int rc;
+	const char *value;
 
 	argv_array_init(&array);
 	argv_array_push(&array, "commit");
@@ -385,6 +386,10 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	if (!opts->edit) {
 		argv_array_push(&array, "-F");
 		argv_array_push(&array, defmsg);
+		if (!opts->signoff &&
+		    !opts->record_origin &&
+		    git_config_get_value("commit.cleanup", &value))
+			argv_array_push(&array, "--cleanup=verbatim");
 	}
 
 	if (allow_empty)
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
-- 
2.3.1.303.g5174db1

Re: [PATCHv2] sequencer: preserve commit messages

[Junio C Hamano]

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 default, unless the user has set config for "commit"
> or the message is amended with -s or -x.
>
> Reported-by: Christoph Anton Mitterer <calestyo@scientia.net>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---

Looks very sensible.

Thank you very much for your efforts to tie the loose ends on many
topics that were discussed already without leading to their
completion.

Will replace and queue.

>  sequencer.c              |  5 +++++
>  t/t3511-cherry-pick-x.sh | 28 ++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index 77a1266..8cf575c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -373,6 +373,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>  {
>  	struct argv_array array;
>  	int rc;
> +	const char *value;
>  
>  	argv_array_init(&array);
>  	argv_array_push(&array, "commit");
> @@ -385,6 +386,10 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>  	if (!opts->edit) {
>  		argv_array_push(&array, "-F");
>  		argv_array_push(&array, defmsg);
> +		if (!opts->signoff &&
> +		    !opts->record_origin &&
> +		    git_config_get_value("commit.cleanup", &value))
> +			argv_array_push(&array, "--cleanup=verbatim");
>  	}
>  
>  	if (allow_empty)
> 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