[PATCH] cherry-pick: Append -x line on separate paragraph

[PATCH] cherry-pick: Append -x line on separate paragraph

[Robin Stocker]

Before this, git cherry-pick -x resulted in messages like this:

    Message of cherry-picked commit
    (cherry picked from commit 871e293c9acbeaacce59dcd98fab6028f552f5be)

Which is not the recommended way to write commit messages. When the
commit message ends with a Signed-off-by, it's less bad. But having it
on a line apart from the others also doesn't hurt there.

Now, care is taken that the line is appended as a separate paragraph:

    Message of cherry-picked commit

    (cherry picked from commit 871e293c9acbeaacce59dcd98fab6028f552f5be)

Because some Git implementations (JGit) don't always terminate the
commit message with a newline, this change also inserts a second newline
if necessary.

Signed-off-by: Robin Stocker <robin@nibor.org>
---
 sequencer.c                    |    4 ++++
 t/t3511-cherry-pick-message.sh |   26 ++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)
 create mode 100755 t/t3511-cherry-pick-message.sh

diff --git a/sequencer.c b/sequencer.c
index bf078f2..41852e6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -484,6 +484,10 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 
 		if (opts->record_origin) {
+			/* Some implementations don't terminate message with final \n, so add it */
+			if (msg.message[strlen(msg.message)-1] != '\n')
+				strbuf_addch(&msgbuf, '\n');
+			strbuf_addch(&msgbuf, '\n');
 			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
diff --git a/t/t3511-cherry-pick-message.sh b/t/t3511-cherry-pick-message.sh
new file mode 100755
index 0000000..6aba8b2
--- /dev/null
+++ b/t/t3511-cherry-pick-message.sh
@@ -0,0 +1,26 @@
+#!/bin/sh
+
+test_description='Test cherry-pick commit message with -x'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	git config advice.detachedhead false &&
+	test_commit initial foo a &&
+	test_commit base foo b &&
+	git checkout initial
+'
+
+test_expect_success 'cherry-pick -x appends message on separate line' '
+	git cherry-pick -x base &&
+	cat >expect <<-\EOF &&
+	base
+
+	(cherry picked from commit 462a97d89aa55720c97984a3ce09665989193981)
+
+	EOF
+	git log --format=%B -n 1 >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.7.7.6

Re: [PATCH] cherry-pick: Append -x line on separate paragraph

[Junio C Hamano]

Robin Stocker <robin@nibor.org> writes:

>  		if (opts->record_origin) {
> +			/* Some implementations don't terminate message with final \n, so add it */
> +			if (msg.message[strlen(msg.message)-1] != '\n')
> +				strbuf_addch(&msgbuf, '\n');

I can agree that this is a good change.

> +			strbuf_addch(&msgbuf, '\n');

But this is somewhat dubious.  Even if what we are adding is merely
an extra LF, that changes the mechanically generated output format
and can break existing hooks that read from these generated commit
log template.

Is there a reason better than "having an empty line there look
better to _me_" to justify this change?

>  			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
>  			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
>  			strbuf_addstr(&msgbuf, ")\n");

Having said that, I've seen proposals to update this message to
format more like the other trailers, so that we would see this:

	The title of the original commit

	The log message taken from the original
        commit comes here.

	Signed-off-by: First person who signed off the original
        Signed-off-by: Another person who signed off the original
        Cherry-picked-from: a9bbc121ea850e49d52ba3cb5a6b7f8077d195d2
        
in the editor, to allow you to add your own Sign-off at the end to
make it look like this:

	The title of the original commit

	The log message taken from the original
        commit comes here.

	Signed-off-by: First person who signed off the original
        Signed-off-by: Another person who signed off the original
        Cherry-picked-from: a9bbc121ea850e49d52ba3cb5a6b7f8077d195d2
	Signed-off-by: Me who did the cherry-pick

I think that might be a worthwhile thing to do perhaps as an
optional behaviour (e.g. perhaps triggered with a new option
"--trailer", or with the same "-x" but only when "cherry-pick.origin
= trailer" configuration is set, or something).  At that point, the
output will look vastly different to existing hooks and those who
care how this field looks like are forced to be updated, but as long
as it is an opt-in feature, it may be worth it.

Re: [PATCH] cherry-pick: Append -x line on separate paragraph

[Robin Stocker]

Junio C Hamano writes:
> Robin Stocker <robin@nibor.org> writes:
> 
> >  		if (opts->record_origin) {
> > + /* Some implementations don't terminate message with final \n, so
> > add it */
> > + if (msg.message[strlen(msg.message)-1] != '\n')
> > + strbuf_addch(&msgbuf, '\n');
> 
> I can agree that this is a good change.
> 
> > + strbuf_addch(&msgbuf, '\n');
> 
> But this is somewhat dubious. Even if what we are adding is merely
> an extra LF, that changes the mechanically generated output format
> and can break existing hooks that read from these generated commit
> log template.

Hm, for a script to break because of an extra LF it would have to be
very badly written. If it looks for "\n(cherry picked ...", it would
still work. But I see the point.

> Is there a reason better than "having an empty line there look
> better to _me_" to justify this change?

Yes:

* If the original commit message consisted just of a summary line,
  the commit message after -x would then not have a blank second
  line, which is bad style, e.g.:

The title of the original commit
(cherry picked ...)

* If the original message did not have any trailers, the appended
  text would stick to the last paragraph, even though it is a
  separate thing.

These don't apply to the git project itself, as its commit message
always have at least a Signed-off-by. But there are projects where
this is not the case and the above reasons apply.

Maybe the solution is to detect if the original commit message
ends with a trailer and in that case keep the existing behavior
of not inserting a blank line?

> >  			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
> >  			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
> >  			strbuf_addstr(&msgbuf, ")\n");
> 
> Having said that, I've seen proposals to update this message to
> format more like the other trailers, so that we would see this:
> 
> The title of the original commit
> 
> The log message taken from the original
> commit comes here.
> 
> Signed-off-by: First person who signed off the original
> Signed-off-by: Another person who signed off the original
> Cherry-picked-from: a9bbc121ea850e49d52ba3cb5a6b7f8077d195d2
> 
> in the editor, to allow you to add your own Sign-off at the end to
> make it look like this:
> 
> The title of the original commit
> 
> The log message taken from the original
> commit comes here.
> 
> Signed-off-by: First person who signed off the original
> Signed-off-by: Another person who signed off the original
> Cherry-picked-from: a9bbc121ea850e49d52ba3cb5a6b7f8077d195d2
> Signed-off-by: Me who did the cherry-pick
> 
> I think that might be a worthwhile thing to do perhaps as an
> optional behaviour (e.g. perhaps triggered with a new option
> "--trailer", or with the same "-x" but only when "cherry-pick.origin
> = trailer" configuration is set, or something). At that point, the
> output will look vastly different to existing hooks and those who
> care how this field looks like are forced to be updated, but as long
> as it is an opt-in feature, it may be worth it.

Oh, I like that proposal. I'd lean towards a new --trailer option I
think.

It would have the same problem of having to append it on a separate
paragraph if the original commit message does not already have a
trailer though.

But I still think that adding the "(cherry picked ..." on a separate
paragraph would be a good thing until "Cherry-picked-from" can be
used.

Regards,
  Robin Stocker

Re: [PATCH] cherry-pick: Append -x line on separate paragraph

[Junio C Hamano]

Robin Stocker <robin@nibor.org> writes:

> Junio C Hamano writes:
>> Robin Stocker <robin@nibor.org> writes:
>> 
>> >  		if (opts->record_origin) {
>> > + /* Some implementations don't terminate message with final \n, so
>> > add it */
>> > + if (msg.message[strlen(msg.message)-1] != '\n')
>> > + strbuf_addch(&msgbuf, '\n');
>> 
>> I can agree that this is a good change.
>> 
>> > + strbuf_addch(&msgbuf, '\n');
>> 
>> But this is somewhat dubious. Even if what we are adding is merely
>> an extra LF, that changes the mechanically generated output format
>> and can break existing hooks that read from these generated commit
>> log template.
>
> Hm, for a script to break because of an extra LF it would have to be
> very badly written. If it looks for "\n(cherry picked ...", it would
> still work. But I see the point.

If you approach this change from the "information left by -x is
somehow useful" point of view, it wouldn't make any difference.
Scripts can match "(cherry picked from ..." and glean useful
information out of it, with or without an empty line around it.

But if you look at it from the other perspective [*1*], it makes a
big difference.  A script that wants to excise these lines used to
be able to just delete such a line.  With the proposed change, it
also has to be aware of an empty line next to it.

>> Is there a reason better than "having an empty line there look
>> better to _me_" to justify this change?
>
> Yes:

Then please have them in the proposed commit log message to justify
the change.  I think the following analysis I quoted from your
message summarizes the motivation well.

> * If the original commit message consisted just of a summary line,
>   the commit message after -x would then not have a blank second
>   line, which is bad style, e.g.:
>
> The title of the original commit
> (cherry picked ...)

This is very true.  So we at least want an empty line added when the
original is one-liner.

> * If the original message did not have any trailers, the appended
>   text would stick to the last paragraph, even though it is a
>   separate thing.

The other side of this argument is if there are trailers, we would
not want an extra blank line.  We need to look at the last paragraph
of the log message and if it does not end with a trailer, we want an
additional empty line.

> Maybe the solution is to detect if the original commit message
> ends with a trailer and in that case keep the existing behavior
> of not inserting a blank line?

Yeah, that sounds like a good change from "this makes the result
look better" point of view.

> Oh, I like that proposal. I'd lean towards a new --trailer option I
> think.
>
> It would have the same problem of having to append it on a separate
> paragraph if the original commit message does not already have a
> trailer though.

Yes.  The logic would be the same.  First terminate the incomplete
last line, if any, and then look at the last paragraph of the commit
log message (one liner is a natural degenerate case of this; its
single-line title is the last paragraph) and if and only if it does
not end with a trailer, add a blank line before adding the marker.

The only difference between the two would be how the "cherry-picked
from" line is formatted.


[Footnote]

*1* Originally, we added "(cherry picked from ..." by default, and
had a switch to disable it; later we made it off by default and made
it optional (and discouraged) with "-x" and this was for a reason.
Unless the original commit object is also available to the reader of
the history, the line is a useless noise, and many people are found
to cherry-pick from their private branches; by definition, the line
is useless in the resulting commit of such a cherry-pick.

Re: [PATCH] cherry-pick: Append -x line on separate paragraph

[Robin Stocker]

Junio C Hamano writes:
> Robin Stocker <robin@nibor.org> writes:
> 
> > Junio C Hamano writes:
> >> Robin Stocker <robin@nibor.org> writes:
> >>
> >> >  		if (opts->record_origin) {
> >> > + /* Some implementations don't terminate message with final \n,
> >> > so
> >> > add it */
> >> > + if (msg.message[strlen(msg.message)-1] != '\n')
> >> > + strbuf_addch(&msgbuf, '\n');
> >>
> >> I can agree that this is a good change.
> >>
> >> > + strbuf_addch(&msgbuf, '\n');
> >>
> >> But this is somewhat dubious. Even if what we are adding is merely
> >> an extra LF, that changes the mechanically generated output format
> >> and can break existing hooks that read from these generated commit
> >> log template.
> >
> > Hm, for a script to break because of an extra LF it would have to be
> > very badly written. If it looks for "\n(cherry picked ...", it would
> > still work. But I see the point.
> 
> If you approach this change from the "information left by -x is
> somehow useful" point of view, it wouldn't make any difference.
> Scripts can match "(cherry picked from ..." and glean useful
> information out of it, with or without an empty line around it.
> 
> But if you look at it from the other perspective [*1*], it makes a
> big difference. A script that wants to excise these lines used to
> be able to just delete such a line. With the proposed change, it
> also has to be aware of an empty line next to it.

Ok, didn't think that a script would want to remove such a line. It
makes sense when considering that it used to always add the line.
Thanks for explaining.

> >> Is there a reason better than "having an empty line there look
> >> better to _me_" to justify this change?
> >
> > Yes:
> 
> Then please have them in the proposed commit log message to justify
> the change. I think the following analysis I quoted from your
> message summarizes the motivation well.
> 
> > * If the original commit message consisted just of a summary line,
> >   the commit message after -x would then not have a blank second
> >   line, which is bad style, e.g.:
> >
> > The title of the original commit
> > (cherry picked ...)
> 
> This is very true. So we at least want an empty line added when the
> original is one-liner.
> 
> > * If the original message did not have any trailers, the appended
> >   text would stick to the last paragraph, even though it is a
> >   separate thing.
> 
> The other side of this argument is if there are trailers, we would
> not want an extra blank line. We need to look at the last paragraph
> of the log message and if it does not end with a trailer, we want an
> additional empty line.
> 
> > Maybe the solution is to detect if the original commit message
> > ends with a trailer and in that case keep the existing behavior
> > of not inserting a blank line?
> 
> Yeah, that sounds like a good change from "this makes the result
> look better" point of view.

How do you think we could best detect a tailer? Check if all
lines of the last paragraph start with /[\w-]+: /?

> > Oh, I like that proposal. I'd lean towards a new --trailer option I
> > think.
> >
> > It would have the same problem of having to append it on a separate
> > paragraph if the original commit message does not already have a
> > trailer though.
> 
> Yes. The logic would be the same. First terminate the incomplete
> last line, if any, and then look at the last paragraph of the commit
> log message (one liner is a natural degenerate case of this; its
> single-line title is the last paragraph) and if and only if it does
> not end with a trailer, add a blank line before adding the marker.
> 
> The only difference between the two would be how the "cherry-picked
> from" line is formatted.

Right.

I'm going to work on this and submit a new version of the patch. The
"Cherry-picked-from" change could then be made later on top of that.

> [Footnote]
> 
> *1* Originally, we added "(cherry picked from ..." by default, and
> had a switch to disable it; later we made it off by default and made
> it optional (and discouraged) with "-x" and this was for a reason.
> Unless the original commit object is also available to the reader of
> the history, the line is a useless noise, and many people are found
> to cherry-pick from their private branches; by definition, the line
> is useless in the resulting commit of such a cherry-pick.

Re: [PATCH] cherry-pick: Append -x line on separate paragraph

[Jeff King]

On Sat, Sep 08, 2012 at 04:10:59PM +0200, Robin Stocker wrote:

> > > Maybe the solution is to detect if the original commit message
> > > ends with a trailer and in that case keep the existing behavior
> > > of not inserting a blank line?
> > 
> > Yeah, that sounds like a good change from "this makes the result
> > look better" point of view.
> 
> How do you think we could best detect a tailer? Check if all
> lines of the last paragraph start with /[\w-]+: /?

There is ends_rfc2822_footer() in builtin/commit.c, which is currently
used for adding Signed-off-by lines. It might make sense to factor that
out, and have a new:

  add_pseudoheader(struct strbuf *commit_message,
                   const char *header)

that would implement the same set of spacing rules for signed-off-by,
cherry-picked-from, and anything else we end up adding later.

I think pseudo-headers like these might also want duplicate suppression
of the final line, which could be part of that function. Note that you
would not want to suppress a duplicate line from the middle of the
trailer, since you might want to sign-off twice (e.g., you sign-off the
original, and then also the cherry-pick). But you would not want two
duplicate lines at the end saying "Signed-off-by: ...", and I believe
"git commit" already suppresses those duplicates.

> I'm going to work on this and submit a new version of the patch. The
> "Cherry-picked-from" change could then be made later on top of that.

Yay. This has come up more than once over the years, so I am glad to see
somebody working on it.

-Peff