{"thread":{"id":"31427","subject":"[PATCH] cherry-pick: Append -x line on separate paragraph","startedAt":"2012-09-04T17:16:46Z","lastAt":"2012-09-10T16:27:54Z","messageCount":6,"participants":["Robin Stocker","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"198313","messageId":"1244407605.1231.1346779006295.JavaMail.root@bazinga.schuettel.ch","threadId":"31427","inReplyTo":"739367481.1229.1346778500787.JavaMail.root@bazinga.schuettel.ch","subject":"[PATCH] cherry-pick: Append -x line on separate paragraph","fromName":"Robin Stocker","fromEmail":"robin@nibor.org","sentAt":"2012-09-04T17:16:46Z","receivedAt":"2012-09-04T17:16:46Z","isPatch":true,"sender":{"key":"robin@nibor.org","avatar":"https://gravatar.com/avatar/c14673277e79914545e24f8ca819310592d6cb15b0f7832e176523b955de2c5d?d=mp&s=160"},"body":"Before this, git cherry-pick -x resulted in messages like this:\n\n    Message of cherry-picked commit\n    (cherry picked from commit 871e293c9acbeaacce59dcd98fab6028f552f5be)\n\nWhich is not the recommended way to write commit messages. When the\ncommit message ends with a Signed-off-by, it's less bad. But having it\non a line apart from the others also doesn't hurt there.\n\nNow, care is taken that the line is appended as a separate paragraph:\n\n    Message of cherry-picked commit\n\n    (cherry picked from commit 871e293c9acbeaacce59dcd98fab6028f552f5be)\n\nBecause some Git implementations (JGit) don't always terminate the\ncommit message with a newline, this change also inserts a second newline\nif necessary.\n\nSigned-off-by: Robin Stocker <robin@nibor.org>\n---\n sequencer.c                    |    4 ++++\n t/t3511-cherry-pick-message.sh |   26 ++++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 0 deletions(-)\n create mode 100755 t/t3511-cherry-pick-message.sh\n\ndiff --git a/sequencer.c b/sequencer.c\nindex bf078f2..41852e6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -484,6 +484,10 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t}\n \n \t\tif (opts->record_origin) {\n+\t\t\t/* Some implementations don't terminate message with final \\n, so add it */\n+\t\t\tif (msg.message[strlen(msg.message)-1] != '\\n')\n+\t\t\t\tstrbuf_addch(&msgbuf, '\\n');\n+\t\t\tstrbuf_addch(&msgbuf, '\\n');\n \t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n \t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\ndiff --git a/t/t3511-cherry-pick-message.sh b/t/t3511-cherry-pick-message.sh\nnew file mode 100755\nindex 0000000..6aba8b2\n--- /dev/null\n+++ b/t/t3511-cherry-pick-message.sh\n@@ -0,0 +1,26 @@\n+#!/bin/sh\n+\n+test_description='Test cherry-pick commit message with -x'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tgit config advice.detachedhead false &&\n+\ttest_commit initial foo a &&\n+\ttest_commit base foo b &&\n+\tgit checkout initial\n+'\n+\n+test_expect_success 'cherry-pick -x appends message on separate line' '\n+\tgit cherry-pick -x base &&\n+\tcat >expect <<-\\EOF &&\n+\tbase\n+\n+\t(cherry picked from commit 462a97d89aa55720c97984a3ce09665989193981)\n+\n+\tEOF\n+\tgit log --format=%B -n 1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.7.7.6\n"},{"id":"198320","messageId":"7vsjaxaasv.fsf@alter.siamese.dyndns.org","threadId":"31427","inReplyTo":"1244407605.1231.1346779006295.JavaMail.root@bazinga.schuettel.ch","subject":"Re: [PATCH] cherry-pick: Append -x line on separate paragraph","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-04T18:43:28Z","receivedAt":"2012-09-04T18:43:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robin Stocker <robin@nibor.org> writes:\n\n>  \t\tif (opts->record_origin) {\n> +\t\t\t/* Some implementations don't terminate message with final \\n, so add it */\n> +\t\t\tif (msg.message[strlen(msg.message)-1] != '\\n')\n> +\t\t\t\tstrbuf_addch(&msgbuf, '\\n');\n\nI can agree that this is a good change.\n\n> +\t\t\tstrbuf_addch(&msgbuf, '\\n');\n\nBut this is somewhat dubious.  Even if what we are adding is merely\nan extra LF, that changes the mechanically generated output format\nand can break existing hooks that read from these generated commit\nlog template.\n\nIs there a reason better than \"having an empty line there look\nbetter to _me_\" to justify this change?\n\n>  \t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n>  \t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n>  \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n\nHaving said that, I've seen proposals to update this message to\nformat more like the other trailers, so that we would see this:\n\n\tThe title of the original commit\n\n\tThe log message taken from the original\n        commit comes here.\n\n\tSigned-off-by: First person who signed off the original\n        Signed-off-by: Another person who signed off the original\n        Cherry-picked-from: a9bbc121ea850e49d52ba3cb5a6b7f8077d195d2\n        \nin the editor, to allow you to add your own Sign-off at the end to\nmake it look like this:\n\n\tThe title of the original commit\n\n\tThe log message taken from the original\n        commit comes here.\n\n\tSigned-off-by: First person who signed off the original\n        Signed-off-by: Another person who signed off the original\n        Cherry-picked-from: a9bbc121ea850e49d52ba3cb5a6b7f8077d195d2\n\tSigned-off-by: Me who did the cherry-pick\n\nI think that might be a worthwhile thing to do perhaps as an\noptional behaviour (e.g. perhaps triggered with a new option\n\"--trailer\", or with the same \"-x\" but only when \"cherry-pick.origin\n= trailer\" configuration is set, or something).  At that point, the\noutput will look vastly different to existing hooks and those who\ncare how this field looks like are forced to be updated, but as long\nas it is an opt-in feature, it may be worth it.\n"},{"id":"198403","messageId":"1410595949.1269.1346877367487.JavaMail.root@bazinga.schuettel.ch","threadId":"31427","inReplyTo":"7vsjaxaasv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] cherry-pick: Append -x line on separate paragraph","fromName":"Robin Stocker","fromEmail":"robin@nibor.org","sentAt":"2012-09-05T20:36:07Z","receivedAt":"2012-09-05T20:36:07Z","isPatch":true,"sender":{"key":"robin@nibor.org","avatar":"https://gravatar.com/avatar/c14673277e79914545e24f8ca819310592d6cb15b0f7832e176523b955de2c5d?d=mp&s=160"},"body":"Junio C Hamano writes:\n> Robin Stocker <robin@nibor.org> writes:\n> \n> >  \t\tif (opts->record_origin) {\n> > + /* Some implementations don't terminate message with final \\n, so\n> > add it */\n> > + if (msg.message[strlen(msg.message)-1] != '\\n')\n> > + strbuf_addch(&msgbuf, '\\n');\n> \n> I can agree that this is a good change.\n> \n> > + strbuf_addch(&msgbuf, '\\n');\n> \n> But this is somewhat dubious. Even if what we are adding is merely\n> an extra LF, that changes the mechanically generated output format\n> and can break existing hooks that read from these generated commit\n> log template.\n\nHm, for a script to break because of an extra LF it would have to be\nvery badly written. If it looks for \"\\n(cherry picked ...\", it would\nstill work. But I see the point.\n\n> Is there a reason better than \"having an empty line there look\n> better to _me_\" to justify this change?\n\nYes:\n\n* If the original commit message consisted just of a summary line,\n  the commit message after -x would then not have a blank second\n  line, which is bad style, e.g.:\n\nThe title of the original commit\n(cherry picked ...)\n\n* If the original message did not have any trailers, the appended\n  text would stick to the last paragraph, even though it is a\n  separate thing.\n\nThese don't apply to the git project itself, as its commit message\nalways have at least a Signed-off-by. But there are projects where\nthis is not the case and the above reasons apply.\n\nMaybe the solution is to detect if the original commit message\nends with a trailer and in that case keep the existing behavior\nof not inserting a blank line?\n\n> >  \t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n> >  \t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n> >  \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n> \n> Having said that, I've seen proposals to update this message to\n> format more like the other trailers, so that we would see this:\n> \n> The title of the original commit\n> \n> The log message taken from the original\n> commit comes here.\n> \n> Signed-off-by: First person who signed off the original\n> Signed-off-by: Another person who signed off the original\n> Cherry-picked-from: a9bbc121ea850e49d52ba3cb5a6b7f8077d195d2\n> \n> in the editor, to allow you to add your own Sign-off at the end to\n> make it look like this:\n> \n> The title of the original commit\n> \n> The log message taken from the original\n> commit comes here.\n> \n> Signed-off-by: First person who signed off the original\n> Signed-off-by: Another person who signed off the original\n> Cherry-picked-from: a9bbc121ea850e49d52ba3cb5a6b7f8077d195d2\n> Signed-off-by: Me who did the cherry-pick\n> \n> I think that might be a worthwhile thing to do perhaps as an\n> optional behaviour (e.g. perhaps triggered with a new option\n> \"--trailer\", or with the same \"-x\" but only when \"cherry-pick.origin\n> = trailer\" configuration is set, or something). At that point, the\n> output will look vastly different to existing hooks and those who\n> care how this field looks like are forced to be updated, but as long\n> as it is an opt-in feature, it may be worth it.\n\nOh, I like that proposal. I'd lean towards a new --trailer option I\nthink.\n\nIt would have the same problem of having to append it on a separate\nparagraph if the original commit message does not already have a\ntrailer though.\n\nBut I still think that adding the \"(cherry picked ...\" on a separate\nparagraph would be a good thing until \"Cherry-picked-from\" can be\nused.\n\nRegards,\n  Robin Stocker\n"},{"id":"198415","messageId":"7vmx136cdc.fsf@alter.siamese.dyndns.org","threadId":"31427","inReplyTo":"1410595949.1269.1346877367487.JavaMail.root@bazinga.schuettel.ch","subject":"Re: [PATCH] cherry-pick: Append -x line on separate paragraph","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-06T03:47:43Z","receivedAt":"2012-09-06T03:47:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robin Stocker <robin@nibor.org> writes:\n\n> Junio C Hamano writes:\n>> Robin Stocker <robin@nibor.org> writes:\n>> \n>> >  \t\tif (opts->record_origin) {\n>> > + /* Some implementations don't terminate message with final \\n, so\n>> > add it */\n>> > + if (msg.message[strlen(msg.message)-1] != '\\n')\n>> > + strbuf_addch(&msgbuf, '\\n');\n>> \n>> I can agree that this is a good change.\n>> \n>> > + strbuf_addch(&msgbuf, '\\n');\n>> \n>> But this is somewhat dubious. Even if what we are adding is merely\n>> an extra LF, that changes the mechanically generated output format\n>> and can break existing hooks that read from these generated commit\n>> log template.\n>\n> Hm, for a script to break because of an extra LF it would have to be\n> very badly written. If it looks for \"\\n(cherry picked ...\", it would\n> still work. But I see the point.\n\nIf you approach this change from the \"information left by -x is\nsomehow useful\" point of view, it wouldn't make any difference.\nScripts can match \"(cherry picked from ...\" and glean useful\ninformation out of it, with or without an empty line around it.\n\nBut if you look at it from the other perspective [*1*], it makes a\nbig difference.  A script that wants to excise these lines used to\nbe able to just delete such a line.  With the proposed change, it\nalso has to be aware of an empty line next to it.\n\n>> Is there a reason better than \"having an empty line there look\n>> better to _me_\" to justify this change?\n>\n> Yes:\n\nThen please have them in the proposed commit log message to justify\nthe change.  I think the following analysis I quoted from your\nmessage summarizes the motivation well.\n\n> * If the original commit message consisted just of a summary line,\n>   the commit message after -x would then not have a blank second\n>   line, which is bad style, e.g.:\n>\n> The title of the original commit\n> (cherry picked ...)\n\nThis is very true.  So we at least want an empty line added when the\noriginal is one-liner.\n\n> * If the original message did not have any trailers, the appended\n>   text would stick to the last paragraph, even though it is a\n>   separate thing.\n\nThe other side of this argument is if there are trailers, we would\nnot want an extra blank line.  We need to look at the last paragraph\nof the log message and if it does not end with a trailer, we want an\nadditional empty line.\n\n> Maybe the solution is to detect if the original commit message\n> ends with a trailer and in that case keep the existing behavior\n> of not inserting a blank line?\n\nYeah, that sounds like a good change from \"this makes the result\nlook better\" point of view.\n\n> Oh, I like that proposal. I'd lean towards a new --trailer option I\n> think.\n>\n> It would have the same problem of having to append it on a separate\n> paragraph if the original commit message does not already have a\n> trailer though.\n\nYes.  The logic would be the same.  First terminate the incomplete\nlast line, if any, and then look at the last paragraph of the commit\nlog message (one liner is a natural degenerate case of this; its\nsingle-line title is the last paragraph) and if and only if it does\nnot end with a trailer, add a blank line before adding the marker.\n\nThe only difference between the two would be how the \"cherry-picked\nfrom\" line is formatted.\n\n\n[Footnote]\n\n*1* Originally, we added \"(cherry picked from ...\" by default, and\nhad a switch to disable it; later we made it off by default and made\nit optional (and discouraged) with \"-x\" and this was for a reason.\nUnless the original commit object is also available to the reader of\nthe history, the line is a useless noise, and many people are found\nto cherry-pick from their private branches; by definition, the line\nis useless in the resulting commit of such a cherry-pick.\n"},{"id":"198566","messageId":"1498324622.1357.1347113459304.JavaMail.root@bazinga.schuettel.ch","threadId":"31427","inReplyTo":"7vmx136cdc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] cherry-pick: Append -x line on separate paragraph","fromName":"Robin Stocker","fromEmail":"robin@nibor.org","sentAt":"2012-09-08T14:10:59Z","receivedAt":"2012-09-08T14:10:59Z","isPatch":true,"sender":{"key":"robin@nibor.org","avatar":"https://gravatar.com/avatar/c14673277e79914545e24f8ca819310592d6cb15b0f7832e176523b955de2c5d?d=mp&s=160"},"body":"Junio C Hamano writes:\n> Robin Stocker <robin@nibor.org> writes:\n> \n> > Junio C Hamano writes:\n> >> Robin Stocker <robin@nibor.org> writes:\n> >>\n> >> >  \t\tif (opts->record_origin) {\n> >> > + /* Some implementations don't terminate message with final \\n,\n> >> > so\n> >> > add it */\n> >> > + if (msg.message[strlen(msg.message)-1] != '\\n')\n> >> > + strbuf_addch(&msgbuf, '\\n');\n> >>\n> >> I can agree that this is a good change.\n> >>\n> >> > + strbuf_addch(&msgbuf, '\\n');\n> >>\n> >> But this is somewhat dubious. Even if what we are adding is merely\n> >> an extra LF, that changes the mechanically generated output format\n> >> and can break existing hooks that read from these generated commit\n> >> log template.\n> >\n> > Hm, for a script to break because of an extra LF it would have to be\n> > very badly written. If it looks for \"\\n(cherry picked ...\", it would\n> > still work. But I see the point.\n> \n> If you approach this change from the \"information left by -x is\n> somehow useful\" point of view, it wouldn't make any difference.\n> Scripts can match \"(cherry picked from ...\" and glean useful\n> information out of it, with or without an empty line around it.\n> \n> But if you look at it from the other perspective [*1*], it makes a\n> big difference. A script that wants to excise these lines used to\n> be able to just delete such a line. With the proposed change, it\n> also has to be aware of an empty line next to it.\n\nOk, didn't think that a script would want to remove such a line. It\nmakes sense when considering that it used to always add the line.\nThanks for explaining.\n\n> >> Is there a reason better than \"having an empty line there look\n> >> better to _me_\" to justify this change?\n> >\n> > Yes:\n> \n> Then please have them in the proposed commit log message to justify\n> the change. I think the following analysis I quoted from your\n> message summarizes the motivation well.\n> \n> > * If the original commit message consisted just of a summary line,\n> >   the commit message after -x would then not have a blank second\n> >   line, which is bad style, e.g.:\n> >\n> > The title of the original commit\n> > (cherry picked ...)\n> \n> This is very true. So we at least want an empty line added when the\n> original is one-liner.\n> \n> > * If the original message did not have any trailers, the appended\n> >   text would stick to the last paragraph, even though it is a\n> >   separate thing.\n> \n> The other side of this argument is if there are trailers, we would\n> not want an extra blank line. We need to look at the last paragraph\n> of the log message and if it does not end with a trailer, we want an\n> additional empty line.\n> \n> > Maybe the solution is to detect if the original commit message\n> > ends with a trailer and in that case keep the existing behavior\n> > of not inserting a blank line?\n> \n> Yeah, that sounds like a good change from \"this makes the result\n> look better\" point of view.\n\nHow do you think we could best detect a tailer? Check if all\nlines of the last paragraph start with /[\\w-]+: /?\n\n> > Oh, I like that proposal. I'd lean towards a new --trailer option I\n> > think.\n> >\n> > It would have the same problem of having to append it on a separate\n> > paragraph if the original commit message does not already have a\n> > trailer though.\n> \n> Yes. The logic would be the same. First terminate the incomplete\n> last line, if any, and then look at the last paragraph of the commit\n> log message (one liner is a natural degenerate case of this; its\n> single-line title is the last paragraph) and if and only if it does\n> not end with a trailer, add a blank line before adding the marker.\n> \n> The only difference between the two would be how the \"cherry-picked\n> from\" line is formatted.\n\nRight.\n\nI'm going to work on this and submit a new version of the patch. The\n\"Cherry-picked-from\" change could then be made later on top of that.\n\n> [Footnote]\n> \n> *1* Originally, we added \"(cherry picked from ...\" by default, and\n> had a switch to disable it; later we made it off by default and made\n> it optional (and discouraged) with \"-x\" and this was for a reason.\n> Unless the original commit object is also available to the reader of\n> the history, the line is a useless noise, and many people are found\n> to cherry-pick from their private branches; by definition, the line\n> is useless in the resulting commit of such a cherry-pick.\n"},{"id":"198689","messageId":"20120910162754.GD9435@sigill.intra.peff.net","threadId":"31427","inReplyTo":"1498324622.1357.1347113459304.JavaMail.root@bazinga.schuettel.ch","subject":"Re: [PATCH] cherry-pick: Append -x line on separate paragraph","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-10T16:27:54Z","receivedAt":"2012-09-10T16:27:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 08, 2012 at 04:10:59PM +0200, Robin Stocker wrote:\n\n> > > Maybe the solution is to detect if the original commit message\n> > > ends with a trailer and in that case keep the existing behavior\n> > > of not inserting a blank line?\n> > \n> > Yeah, that sounds like a good change from \"this makes the result\n> > look better\" point of view.\n> \n> How do you think we could best detect a tailer? Check if all\n> lines of the last paragraph start with /[\\w-]+: /?\n\nThere is ends_rfc2822_footer() in builtin/commit.c, which is currently\nused for adding Signed-off-by lines. It might make sense to factor that\nout, and have a new:\n\n  add_pseudoheader(struct strbuf *commit_message,\n                   const char *header)\n\nthat would implement the same set of spacing rules for signed-off-by,\ncherry-picked-from, and anything else we end up adding later.\n\nI think pseudo-headers like these might also want duplicate suppression\nof the final line, which could be part of that function. Note that you\nwould not want to suppress a duplicate line from the middle of the\ntrailer, since you might want to sign-off twice (e.g., you sign-off the\noriginal, and then also the cherry-pick). But you would not want two\nduplicate lines at the end saying \"Signed-off-by: ...\", and I believe\n\"git commit\" already suppresses those duplicates.\n\n> I'm going to work on this and submit a new version of the patch. The\n> \"Cherry-picked-from\" change could then be made later on top of that.\n\nYay. This has come up more than once over the years, so I am glad to see\nsomebody working on it.\n\n-Peff\n"}]}