threads / bug / 38620

[BUG] git mangles up commit messages on rebase

Subject: [BUG] git mangles up commit messages on rebase

## tl;dr

14 messages between Feb 21, 2015 and Mar 6, 2015.

replies: 13people: 3as markdown or json

Christoph Anton Mitterer· Feb 21, 2015, 17:48 UTC · lore
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.

Michael J Gruber· Feb 23, 2015, 13:23 UTC · re: Christoph Anton Mitterer · lore

[PATCH] sequencer: preserve commit messages

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
Junio C Hamano· Feb 23, 2015, 18:54 UTC · re: Michael J Gruber · lore

Re: [PATCH] sequencer: preserve commit messages

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 7 quoted lines
> 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?

Show 23 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");
Show 54 quoted lines
>  
>  	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
Michael J Gruber· Feb 24, 2015, 15:29 UTC · re: Junio C Hamano · lore

Re: [PATCH] sequencer: preserve commit messages

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
Junio C Hamano· Feb 24, 2015, 18:29 UTC · re: Michael J Gruber · lore

Re: [PATCH] sequencer: preserve commit messages

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 10 quoted lines
>> 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?

Michael J Gruber· Feb 25, 2015, 09:50 UTC · re: Junio C Hamano · lore

Re: [PATCH] sequencer: preserve commit messages

Junio C Hamano venit, vidit, dixit 24.02.2015 19:29:
Show 17 quoted lines
> 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
Junio C Hamano· Feb 25, 2015, 18:22 UTC · re: Michael J Gruber · lore

Re: [PATCH] sequencer: preserve commit messages

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 20 quoted lines
> 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?

Michael J Gruber· Feb 26, 2015, 11:05 UTC · re: Junio C Hamano · lore

Re: [PATCH] sequencer: preserve commit messages

Junio C Hamano venit, vidit, dixit 25.02.2015 19:22:
Show 27 quoted lines
> 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.
Show 5 quoted lines
> 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
Junio C Hamano· Feb 26, 2015, 19:49 UTC · re: Michael J Gruber · lore

Re: [PATCH] sequencer: preserve commit messages

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?
Michael J Gruber· Feb 27, 2015, 15:31 UTC · re: Junio C Hamano · lore

Re: [PATCH] sequencer: preserve commit messages

Junio C Hamano venit, vidit, dixit 26.02.2015 20:49:
Show 48 quoted lines
> 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
Junio C Hamano· Feb 27, 2015, 18:31 UTC · re: Michael J Gruber · lore

Re: [PATCH] sequencer: preserve commit messages

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 8 quoted lines
> 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).

Michael J Gruber· Mar 6, 2015, 13:55 UTC · re: Junio C Hamano · lore

[PATCHv2] sequencer: preserve commit messages

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
Junio C Hamano· Mar 6, 2015, 18:59 UTC · re: Michael J Gruber · lore

Re: [PATCHv2] sequencer: preserve commit messages

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 12 quoted lines
> 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.
Show 78 quoted lines
>  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
Christoph Anton Mitterer· Feb 25, 2015, 13:44 UTC · re: Junio C Hamano · lore

Re: [PATCH] sequencer: preserve commit messages

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.

← back to recent threads