threads / patch / 31427

patchcherry-pick: Append -x line on separate paragraph

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

## tl;dr

6 messages between Sep 4, 2012 and Sep 10, 2012. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Robin Stocker· Sep 4, 2012, 17:16 UTC · lore
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
Show changes to 2 files +30 −0

sequencer.c, 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
Junio C Hamano· Sep 4, 2012, 18:43 UTC · re: Robin Stocker · lore

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

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.

Robin Stocker· Sep 5, 2012, 20:36 UTC · re: Junio C Hamano · lore

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

Junio C Hamano writes:
Show 16 quoted lines
> 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?

Show 36 quoted lines
> >  			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
Junio C Hamano· Sep 6, 2012, 03:47 UTC · re: Robin Stocker · lore

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

Robin Stocker <robin@nibor.org> writes:
Show 21 quoted lines
> 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.

Show 6 quoted lines
> * 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.

Show 6 quoted lines
> 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.

Robin Stocker· Sep 8, 2012, 14:10 UTC · re: Junio C Hamano · lore

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

Junio C Hamano writes:
Show 34 quoted lines
> 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.

Show 34 quoted lines
> >> 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-]+: /?

Show 15 quoted lines
> > 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.

Show 9 quoted lines
> [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.
Jeff King· Sep 10, 2012, 16:27 UTC · re: Robin Stocker · lore

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

On Sat, Sep 08, 2012 at 04:10:59PM +0200, Robin Stocker wrote:
Show 9 quoted lines
> > > 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

← back to recent threads