{"thread":{"id":"25762","subject":"[PATCH] cherry-pick -x: add newline before pick note","startedAt":"2010-11-16T15:11:17Z","lastAt":"2011-03-08T22:34:48Z","messageCount":13,"participants":["Michael J Gruber","Jeff King","Jonathan Nieder","Junio C Hamano","Jay Soffian","Oswald Buddenhagen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"155998","messageId":"d0318dcd2b52f2e818888003e3dd81c7b713fec6.1289920242.git.git@drmicha.warpmail.net","threadId":"25762","inReplyTo":null,"subject":"[PATCH] cherry-pick -x: add newline before pick note","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2010-11-16T15:11:17Z","receivedAt":"2010-11-16T15:11:17Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Currently, cherry-pick -x sticks the pick note immediately after the\nexisting commit message. This\n\n* is bad for commits with 1 line subject (it makes a 2 line subject)\n* is different from git-svn, e.g., which leaves an empty line before.\n\nMake cherry-pick always insert an empty line before the pick note.\n\nReported-by: Martin Svensson \nSigned-off-by: Michael J Gruber \n---\n builtin/revert.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 57b51e4..9251257 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -485,7 +485,7 @@ static int do_pick_commit(void)\n \t\tset_author_ident_env(msg.message);\n \t\tadd_message_to_msg(&msgbuf, msg.message);\n \t\tif (no_replay) {\n-\t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n+\t\t\tstrbuf_addstr(&msgbuf, \"\\n(cherry picked from commit \");\n \t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n \t\t}\n-- \n1.7.3.2.193.g78bbb\n"},{"id":"156015","messageId":"20101116193018.GA31036@sigill.intra.peff.net","threadId":"25762","inReplyTo":"d0318dcd2b52f2e818888003e3dd81c7b713fec6.1289920242.git.git@drmicha.warpmail.net","subject":"Re: [PATCH] cherry-pick -x: add newline before pick note","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-11-16T19:30:18Z","receivedAt":"2010-11-16T19:30:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 16, 2010 at 04:11:17PM +0100, Michael J Gruber wrote:\n\n> Currently, cherry-pick -x sticks the pick note immediately after the\n> existing commit message. This\n> \n> * is bad for commits with 1 line subject (it makes a 2 line subject)\n> * is different from git-svn, e.g., which leaves an empty line before.\n> \n> Make cherry-pick always insert an empty line before the pick note.\n\nHmm. Should this respect pseudo-header blocks at the end? E.g., if I\nhave:\n\n message subject\n\n Message body.\n\n Signed-off-by: Jeff King \n\nshouldn't it result in:\n\n message subject\n\n Message body.\n\n (cherry picked from commit ...)\n\n Signed-off-by: Jeff King \n\n?\n\nEven better, I wonder if it should actually be:\n\n message subject\n\n Message body.\n\n Signed-off-by: Jeff King \n Cherry-picked-from: ...\n\nAnd then you could actually sign off the cherry-pick separately, too, if\nyou wanted, by adding a line _below_ the cherry-picked-from. I have no\nidea if people are trying to grep for \"cherry picked from commit...\",\nwhich my proposal would break.\n\nNote that none of this is introduced by your patch. The current output\nfor this case is terribly ugly. But I thought I would mention it, as my\nthird version means we _do_ want the current behavior in some cases\n(i.e., when there is already a pseudo-header block).\n\n-Peff\n"},{"id":"156021","messageId":"20101116202556.GA27390@burratino","threadId":"25762","inReplyTo":"20101116193018.GA31036@sigill.intra.peff.net","subject":"[PATCH] commit -s: allow \"(cherry picked \" lines in sign-off section","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-16T20:25:56Z","receivedAt":"2010-11-16T20:25:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Using cherry-pick -x -s to backport a public commit results in\nan unsightly gap in the sign-off chain:\n\n\tReported-by: Jarek Poplawski \n\tTested-by: Jarek Poplawski \n\tSigned-off-by: Frederic Weisbecker \n\tCc: Jeff Mahoney \n\tCc: All since 2.6.32 \n\tSigned-off-by: Andrew Morton \n\tSigned-off-by: Linus Torvalds \n\t(cherry picked from commit 9d8117e72bf453dd9d85e0cd322ce4a0f8bccbc0)\n\n\tSigned-off-by: Back Porter \n\nThe cherry-pick is a step in the line of a patch like any other,\nso one might prefer to lose the extra newline.\n\n\t...\n\tSigned-off-by: Linus Torvalds \n\t(cherry picked from commit 9d8117e72bf453dd9d85e0cd322ce4a0f8bccbc0)\n\tSigned-off-by: Back Porter \n\nThis commit teaches \"git commit --signoff\", and thus cherry-pick -s,\nto do exactly that. It works by treating the \"(cherry picked\" line as\njust another line in the signoff chain, except as the first line (that\nlast exception is to avoid false positives).\n\nSigned-off-by: Jonathan Nieder \n---\nJeff King wrote:\n\n> Even better, I wonder if it should actually be:\n> \n> message subject\n> \n> Message body.\n> \n> Signed-off-by: Jeff King \n> Cherry-picked-from: ...\n\nHere's something like that. I use \"git cherry-pick -x -s\" to\nbackport patches from a public upstream. Now you can, too.\n\nIdeally inline notes like\n\n\t [akpm@linux-foundation.org: coding-style fixes]\n\nalso ought to be tolerated.\n\n builtin/commit.c | 20 +++++++\n t/t3510-cherry-pick-message.sh | 112 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 132 insertions(+), 0 deletions(-)\n create mode 100755 t/t3510-cherry-pick-message.sh\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 66fdd22..71dd52b 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -528,6 +528,8 @@ static int ends_rfc2822_footer(struct strbuf *sb)\n \t\ti++;\n \n \tfor (; i < len; i = k) {\n+\t\tstatic const char cherry_pick[] = \"(cherry picked from commit \";\n+\n \t\tfor (k = i; k < len && buf[k] != '\\n'; k++)\n \t\t\t; /* do nothing */\n \t\tk++;\n@@ -535,6 +537,20 @@ static int ends_rfc2822_footer(struct strbuf *sb)\n \t\tif ((buf[k] == ' ' || buf[k] == '\\t') && !first)\n \t\t\tcontinue;\n \n+\t\tif (!first && buf[k] == '(' && k + strlen(cherry_pick) < len) {\n+\t\t\t/* Might be a cherry-pick notice. */\n+\t\t\tconst char *p = buf + k;\n+\t\t\tif (!memcmp(p, cherry_pick, strlen(cherry_pick))) {\n+\t\t\t\tp = memchr(buf + k, '\\n', len - k);\n+\t\t\t\tif (!p)\n+\t\t\t\t\treturn 0;\n+\t\t\t\tif (p + 1 == buf + len)\n+\t\t\t\t\treturn 1;\n+\t\t\t\tk = p - buf;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\n \t\tfirst = 0;\n \n \t\tfor (j = 0; i + j < len; j++) {\n@@ -625,6 +641,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tfor (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\\n'; i--)\n \t\t\t; /* do nothing */\n \t\tif (prefixcmp(sb.buf + i, sob.buf)) {\n+\t\t\t/*\n+\t\t\t * Only insert an extra newline if the previous line\n+\t\t\t * is not part of a Signed-off-by:/Acked-by:/etc chain.\n+\t\t\t */\n \t\t\tif (!i || !ends_rfc2822_footer(&sb))\n \t\t\t\tstrbuf_addch(&sb, '\\n');\n \t\t\tstrbuf_addbuf(&sb, &sob);\ndiff --git a/t/t3510-cherry-pick-message.sh b/t/t3510-cherry-pick-message.sh\nnew file mode 100755\nindex 0000000..83e50a3\n--- /dev/null\n+++ b/t/t3510-cherry-pick-message.sh\n@@ -0,0 +1,112 @@\n+#!/bin/sh\n+\n+test_description='tests for the log messages cherry-pick produces\n+\n+ ----\n+ + cherry-pick of branch\n+ + signoff\n+ + basic\n+ ++ mainline\n+ +++ initial\n+'\n+. ./test-lib.sh\n+\n+prepare_commit () {\n+\tgit checkout initial &&\n+\ttest_commit \"$1\" &&\n+\ttest_tick &&\n+\tgit commit --amend --allow-empty-message -F \"$1.message\" &&\n+\tgit tag -d \"$1\" &&\n+\tgit tag \"$1\"\n+}\n+\n+test_cmp_message () {\n+\texpect=$1 &&\n+\tshift &&\n+\tgit log -1 --pretty=format:%B \"$@\" >actual &&\n+\ttest_cmp \"$expect\" actual\n+}\n+\n+cat >basic.message <<\\EOF\n+ A branch\n+\n+Here comes the lovely description of a change to pick up.\n+Contributions come from many people:\n+EOF\n+\n+{\n+\tcat basic.message\n+\tcat <<-\\EOF\n+\n+\tSigned-off-by: Foo \n+\tSigned-off-by: Bar \n+\tTested-by: Baz \n+\tEOF\n+} >signoff.message\n+\n+test_expect_success 'setup' '\n+\ttest_commit initial &&\n+\tprepare_commit basic &&\n+\tprepare_commit signoff\n+'\n+\n+test_expect_success 'cherry-pick preserves message' '\n+\tcat basic.message >expect &&\n+\tgit checkout initial &&\n+\tgit cherry-pick basic &&\n+\ttest_cmp_message expect\n+'\n+\n+test_expect_success 'cherry-pick -s adds signoff' '\n+\t{\n+\t\tcat basic.message &&\n+\t\techo &&\n+\t\techo \"Signed-off-by: C O Mitter \"\n+\t} >expect &&\n+\tgit checkout initial &&\n+\tgit cherry-pick -s basic &&\n+\ttest_cmp_message expect HEAD\n+'\n+\n+test_expect_success 'cherry-pick -s integrates into existing signoff chain' '\n+\t{\n+\t\tcat signoff.message &&\n+\t\techo \"Signed-off-by: C O Mitter \"\n+\t} >expect &&\n+\tgit checkout initial &&\n+\tgit cherry-pick -s signoff &&\n+\ttest_cmp_message expect HEAD\n+'\n+\n+test_expect_success 'cherry-pick -x adds old commit id' '\n+\t{\n+\t\tcat basic.message &&\n+\t\techo \"(cherry picked from commit $(git rev-parse basic^0))\"\n+\t} >expect &&\n+\tgit checkout initial &&\n+\tgit cherry-pick -x basic &&\n+\ttest_cmp_message expect HEAD\n+'\n+\n+test_expect_success 'cherry-pick -x integrates into signoff chain' '\n+\t{\n+\t\tcat signoff.message &&\n+\t\techo \"(cherry picked from commit $(git rev-parse signoff^0))\"\n+\t} >expect &&\n+\tgit checkout initial &&\n+\tgit cherry-pick -x signoff &&\n+\ttest_cmp_message expect HEAD\n+'\n+\n+test_expect_success 'cherry-pick -x -s' '\n+\t{\n+\t\tcat signoff.message &&\n+\t\techo \"(cherry picked from commit $(git rev-parse signoff^0))\"\n+\t\techo \"Signed-off-by: C O Mitter \"\n+\t} >expect &&\n+\tgit checkout initial &&\n+\tgit cherry-pick -x -s signoff &&\n+\ttest_cmp_message expect HEAD\n+'\n+\n+test_done\n-- \n1.7.2.3.551.g13682.dirty\n"},{"id":"156022","messageId":"20101116204027.GB27390@burratino","threadId":"25762","inReplyTo":"20101116202556.GA27390@burratino","subject":"Re: [PATCH] commit -s: allow \"(cherry picked \" lines in sign-off section","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-16T20:40:27Z","receivedAt":"2010-11-16T20:40:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> \t(cherry picked from commit 9d8117e72bf453dd9d85e0cd322ce4a0f8bccbc0)\n> \n> \tSigned-off-by: Back Porter \n> \n> The cherry-pick is a step in the line of a patch like any other,\n> so one might prefer to lose the extra newline.\n\nSigh. s/line/life/\n\n[...]\n> Signed-off-by: Jonathan Nieder \n\nLet's kick off the reviews.\n\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -528,6 +528,8 @@ static int ends_rfc2822_footer(struct strbuf *sb)\n> \t\ti++;\n> \n> \tfor (; i < len; i = k) {\n> +\t\tstatic const char cherry_pick[] = \"(cherry picked from commit \";\n> +\n\nBetter to share this string with builtin/revert.c, no?\n\nWhat would happen when \"(cherry picked ...\" gets translated?\nShould only the current language's version be tolerated in\nthe commit footer, or is there something more generic to\nmatch for that could take care of wording changes automatically?\n\n> @@ -535,6 +537,20 @@ static int ends_rfc2822_footer(struct strbuf *sb)\n> \t\tif ((buf[k] == ' ' || buf[k] == '\\t') && !first)\n> \t\t\tcontinue;\n> \n> +\t\tif (!first && buf[k] == '(' && k + strlen(cherry_pick) < len) {\n> +\t\t\t/* Might be a cherry-pick notice. */\n> +\t\t\tconst char *p = buf + k;\n> +\t\t\tif (!memcmp(p, cherry_pick, strlen(cherry_pick))) {\n> +\t\t\t\tp = memchr(buf + k, '\\n', len - k);\n\nMaybe simpler:\n\n\tp = memchr(...\n\tif (!p)\n\t\treturn 0;\n\ti = p - buf;\n\nto reuse the termination condition in the sign-off parser.\n\nPresumably the main loop could use memchr() instead of open-coding\nit as well.\n"},{"id":"156025","messageId":"7vlj4shoej.fsf@alter.siamese.dyndns.org","threadId":"25762","inReplyTo":"20101116204027.GB27390@burratino","subject":"Re: [PATCH] commit -s: allow \"(cherry picked \" lines in sign-off section","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-11-16T22:52:36Z","receivedAt":"2010-11-16T22:52:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Jonathan Nieder wrote:\n>\n>> \t(cherry picked from commit 9d8117e72bf453dd9d85e0cd322ce4a0f8bccbc0)\n>> \n>> \tSigned-off-by: Back Porter \n>> \n>> The cherry-pick is a step in the line of a patch like any other,\n>> so one might prefer to lose the extra newline.\n>\n> Sigh. s/line/life/\n>\n> [...]\n>> Signed-off-by: Jonathan Nieder \n>\n> Let's kick off the reviews.\n>\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -528,6 +528,8 @@ static int ends_rfc2822_footer(struct strbuf *sb)\n>> \t\ti++;\n>> \n>> \tfor (; i < len; i = k) {\n>> +\t\tstatic const char cherry_pick[] = \"(cherry picked from commit \";\n>> +\n>\n> Better to share this string with builtin/revert.c, no?\n>\n> What would happen when \"(cherry picked ...\" gets translated?\n> Should only the current language's version be tolerated in\n> the commit footer, or is there something more generic to\n> match for that could take care of wording changes automatically?\n\nWith this patch you are declaring that \"(cherry picked from...\" is a magic\nmarker just like \"Signed-off-by: \" never to be translated, no?\n\nI am not sure I agree with the reasoning of this patch, by the way. A\ncherry-pick is an event that breaks the life of the patch, so it may even\nbe a sensible thing to do to express \"the above sign-off chain shows who\nwere involved in the original commit; I am cherry-picking it out of\ncontext, and these people do not have much to do with the result\" with a\nblank line on both sides of the \"cherry picked\" line, like this:\n\n A concise summary of the change\n\n\tA detailed description of the change, why it is needed, what\n was broken and why applying this is the best course of action.\n\n\tSigned-off-by: Andrew Morton \n\tSigned-off-by: Linus Torvalds \n\n\t(cherry picked from commit 9d8117e72bf453dd9d85e0cd322ce4a0f8bccbc0)\n\n\tSigned-off-by: Back Porter \n"},{"id":"156026","messageId":"20101116233649.GA30700@burratino","threadId":"25762","inReplyTo":"7vlj4shoej.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] commit -s: allow \"(cherry picked \" lines in sign-off section","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-16T23:36:49Z","receivedAt":"2010-11-16T23:36:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> I am not sure I agree with the reasoning of this patch, by the way. A\n> cherry-pick is an event that breaks the life of the patch, so it may even\n> be a sensible thing to do to express \"the above sign-off chain shows who\n> were involved in the original commit; I am cherry-picking it out of\n> context, and these people do not have much to do with the result\" with a\n> blank line on both sides of the \"cherry picked\" line, like this:\n> \n>\tA concise summary of the change\n> \n> \tA detailed description of the change, why it is needed, what\n>\twas broken and why applying this is the best course of action.\n> \n> \tSigned-off-by: Andrew Morton \n> \tSigned-off-by: Linus Torvalds \n> \n> \t(cherry picked from commit 9d8117e72bf453dd9d85e0cd322ce4a0f8bccbc0)\n> \n> \tSigned-off-by: Back Porter \n\nHow is the cherry-pick event different from the send-by-mail-and-apply\nevent?\n\nIn both cases, the result has a distinct commit id and distinct\nsignoff and it is unlikely that the previous patch handler was testing\nwith the same tree as the next one. (And each patch handler should add\nrelevant comments if the new situation warrants that.)\n"},{"id":"156030","messageId":"AANLkTinAWSNKK3VMY_jAy-8-M-80d_EvW299ZfVFPwpo@mail.gmail.com","threadId":"25762","inReplyTo":"20101116193018.GA31036@sigill.intra.peff.net","subject":"Re: [PATCH] cherry-pick -x: add newline before pick note","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-11-17T06:14:16Z","receivedAt":"2010-11-17T06:14:16Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Tue, Nov 16, 2010 at 2:30 PM, Jeff King wrote:\n> shouldn't it result in:\n>\n>  message subject\n>\n>  Message body.\n>\n>  (cherry picked from commit ...)\n>\n>  Signed-off-by: Jeff King \n\n+1.\n\n> Even better, I wonder if it should actually be:\n>\n>  message subject\n>\n>  Message body.\n>\n>  Signed-off-by: Jeff King \n>  Cherry-picked-from: ...\n\n+2.\n\n> And then you could actually sign off the cherry-pick separately, too, if\n> you wanted, by adding a line _below_ the cherry-picked-from. I have no\n> idea if people are trying to grep for \"cherry picked from commit...\",\n> which my proposal would break.\n\nI can fix my regex easily enough, but I'd also be happy to have this\nuse some other switch than -x.\n\nBTW, I notice that cherry-pick also misbehaves if the original commit\nmessage doesn't end in a newline. I'm not sure whether that's a\ncherry-pick bug for not checking that case, or whether it's a commit\nbug for not ensuring a newline terminates the commit message.\n\nj.\n"},{"id":"156032","messageId":"AANLkTinpEVuPVhDDfEcrvHa4T5BfL+X5w4y=cJkho4d+@mail.gmail.com","threadId":"25762","inReplyTo":"7vlj4shoej.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] commit -s: allow \"(cherry picked \" lines in sign-off section","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-11-17T06:23:44Z","receivedAt":"2010-11-17T06:23:44Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Tue, Nov 16, 2010 at 5:52 PM, Junio C Hamano wrote:\n> I am not sure I agree with the reasoning of this patch, by the way.  A\n> cherry-pick is an event that breaks the life of the patch, so it may even\n> be a sensible thing to do to express \"the above sign-off chain shows who\n> were involved in the original commit; I am cherry-picking it out of\n> context, and these people do not have much to do with the result\" with a\n> blank line on both sides of the \"cherry picked\" line, like this:\n>\n>        A concise summary of the change\n>\n>        A detailed description of the change, why it is needed, what\n>        was broken and why applying this is the best course of action.\n>\n>        Signed-off-by: Andrew Morton \n>        Signed-off-by: Linus Torvalds \n>\n>        (cherry picked from commit 9d8117e72bf453dd9d85e0cd322ce4a0f8bccbc0)\n>\n>        Signed-off-by: Back Porter \n\nOr perhaps prefix them with Original-, inspired by email headers, and\nwhich I think makes it even more clear that the sob lines don't apply\nto the new commit.\n\n        Original-Signed-off-by: Andrew Morton \n        Original-Signed-off-by: Linus Torvalds \n Cherry-picked-from: 9d8117e72bf453dd9d85e0cd322ce4a0f8bccbc0\n        Signed-off-by: Back Porter \n\nj.\n"},{"id":"156054","messageId":"7vd3q3hp8e.fsf@alter.siamese.dyndns.org","threadId":"25762","inReplyTo":"20101116233649.GA30700@burratino","subject":"Re: [PATCH] commit -s: allow \"(cherry picked \" lines in sign-off section","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-11-17T16:46:57Z","receivedAt":"2010-11-17T16:46:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> How is the cherry-pick event different from the send-by-mail-and-apply\n> event?\n>\n> In both cases, the result has a distinct commit id and distinct\n> signoff and it is unlikely that the previous patch handler was testing\n> with the same tree as the next one. (And each patch handler should add\n> relevant comments if the new situation warrants that.)\n\nFair enough. If that is the direction we would want to go, perhaps it\nsuggests that we might eventually want to use \"Cherry-picked-from: \" as\nthe marker for this information?\n\nAnd if we go that route, and if this information is being used, we have a\nrather serious backward compatibility problem. Older scripts will break,\nand this cannot be handwaved away with a configuration option or a command\nline switch (even if you personally choose to keep using the old format,\nyou may get a commit with the new style trailer from elsewhere).\n\nHmm.\n"},{"id":"162990","messageId":"loom.20110308T134920-72@post.gmane.org","threadId":"25762","inReplyTo":"d0318dcd2b52f2e818888003e3dd81c7b713fec6.1289920242.git.git@drmicha.warpmail.net","subject":"Re: [PATCH] cherry-pick -x: add newline before pick note","fromName":"Oswald Buddenhagen","fromEmail":"ossi@kde.org","sentAt":"2011-03-08T12:54:33Z","receivedAt":"2011-03-08T12:54:33Z","isPatch":true,"sender":{"key":"ossi@kde.org","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"Michael J Gruber drmicha.warpmail.net> writes:\n> Currently, cherry-pick -x sticks the pick note immediately after the\n> existing commit message. This\n> \n> * is bad for commits with 1 line subject (it makes a 2 line subject)\n> * is different from git-svn, e.g., which leaves an empty line before.\n> \n> Make cherry-pick always insert an empty line before the pick note.\n> \n> Reported-by: Martin Svensson netinsight.se>\n> Signed-off-by: Michael J Gruber drmicha.warpmail.net>\n> ---\n> builtin/revert.c | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n> \n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 57b51e4..9251257 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -485,7 +485,7 @@ static int do_pick_commit(void)\n> \t\tset_author_ident_env(msg.message);\n> \t\tadd_message_to_msg(&msgbuf, msg.message);\n> \t\tif (no_replay) {\n> -\t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n> +\t\t\tstrbuf_addstr(&msgbuf, \"\\n(cherry picked from commit \");\n> \t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n> \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n> \t\t}\n\nso while everybody is apparently thinking about totally over-engineering\nthings as much as possible, could we please have this patch applied so we\nhave a solution for the time being? i really hate to tell my coworkers that\nthey have to amend the cherry-picks just to make them comply with git's\nown guidelines for well-formed commit messages (and thus have them pass\nour pre-receive hook).\n\nregards\n"},{"id":"163032","messageId":"20110308220843.GA27156@elie","threadId":"25762","inReplyTo":"loom.20110308T134920-72@post.gmane.org","subject":"Re: [PATCH] cherry-pick -x: add newline before pick note","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-08T22:08:43Z","receivedAt":"2011-03-08T22:08:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(please do not cull the CC list)\nHi Oswald,\n\nOswald Buddenhagen wrote:\n\n> so while everybody is apparently thinking about totally over-engineering\n> things as much as possible, could we please have this patch applied so we\n> have a solution for the time being?\n\nI am not convinced that this patch makes a positive change in general.\nStarting from a message\n\n\tFoo the bar\n\n\tMake some excellent improvement to the frobnicator.\n\n\tSigned-off-by: A U Thor \n\na person passing on the patch might write\n\n\tFoo the bar\n[...]\n\tSigned-off-by: A U Thor \n\t[committer@example.com: avoid multiple return points]\n\tSigned-off-by: C O Mitter \n\nSimilarly, when cherry-picking from permanent history, it can make\nsense to write\n\n\tFoo the bar\n[...]\n\tSigned-off-by: A U Thor \n\t(cherry picked from commit 78a8b989a76c8798a9898c98a98c98a98ca)\n\tSigned-off-by: C O Mitter \n\nIn both cases, it's just another hop in the life of a patch and not\nsomething that seems to deserve emphasis with extra whitespace.\n\n> i really hate to tell my coworkers that\n> they have to amend the cherry-picks just to make them comply with git's\n> own guidelines for well-formed commit messages (and thus have them pass\n> our pre-receive hook).\n\nI assume you are referring to one-line commit messages becoming two-line?\nHere's something rough to start.\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex dc1b702..343e7e7 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -198,6 +198,28 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)\n \tstrbuf_addstr(msgbuf, p);\n }\n \n+static void add_blank_line_if_oneline(struct strbuf *msgbuf)\n+{\n+\tconst char *newline = memchr(msgbuf->buf, '\\n', msgbuf->len);\n+\n+\tif (!newline) {\t/* No newline at end of message. */\n+\t\tstrbuf_addch(msgbuf, '\\n');\n+\t\tnewline = msgbuf->buf + msgbuf->len - 1;\n+\t}\n+\n+\t/*\n+\t * If the change description consists of a single line,\n+\t * add a blank line separating the title from the new\n+\t * message body (the \"(cherry picked from\" line).\n+\t *\n+\t * NEEDSWORK: it would be better to reuse the append_signoff\n+\t * logic.\n+\t */\n+\tnewline = memchr(newline, '\\n', msgbuf-> buf + msgbuf->len - newline);\n+\tif (!newline)\n+\t\tstrbuf_addch(msgbuf, '\\n');\n+}\n+\n static void set_author_ident_env(const char *message)\n {\n \tconst char *p = message;\n@@ -499,6 +521,7 @@ static int do_pick_commit(void)\n \t\tnext_label = msg.label;\n \t\tset_author_ident_env(msg.message);\n \t\tadd_message_to_msg(&msgbuf, msg.message);\n+\t\tadd_blank_line_if_oneline(&msgbuf);\n \t\tif (no_replay) {\n \t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n \t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n-- \n"},{"id":"163036","messageId":"20110308221855.GA4030@ugly.local","threadId":"25762","inReplyTo":"20110308220843.GA27156@elie","subject":"Re: [PATCH] cherry-pick -x: add newline before pick note","fromName":"Oswald Buddenhagen","fromEmail":"ossi@kde.org","sentAt":"2011-03-08T22:18:55Z","receivedAt":"2011-03-08T22:18:55Z","isPatch":true,"sender":{"key":"ossi@kde.org","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Tue, Mar 08, 2011 at 04:08:43PM -0600, Jonathan Nieder wrote:\n> (please do not cull the CC list)\n>\ni posted via the gmane webform ...\n\n> Oswald Buddenhagen wrote:\n> > i really hate to tell my coworkers that they have to amend the\n> > cherry-picks just to make them comply with git's own guidelines for\n> > well-formed commit messages\n> \n> I assume you are referring to one-line commit messages becoming\n> two-line?\n>\nyes\n\n> Here's something rough to start.\n> \ni did a much simpler patch in that vein as well, but scrapped it again -\ntotally overengineered. the idea is to optimize the simple case -\ncherry-pick -x, push. everything else needs amends anyway.\n\nregards\n"},{"id":"163038","messageId":"20110308223448.GF26471@elie","threadId":"25762","inReplyTo":"20110308221855.GA4030@ugly.local","subject":"Re: [PATCH] cherry-pick -x: add newline before pick note","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-03-08T22:34:48Z","receivedAt":"2011-03-08T22:34:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi again.\n\nOswald Buddenhagen wrote:\n\n> i posted via the gmane webform ...\n\n(See .\nIt's obnoxious that there is not an easier way, I agree.)\n\n> i did a much simpler patch in that vein as well, but scrapped it again -\n> totally overengineered. the idea is to optimize the simple case -\n> cherry-pick -x, push. everything else needs amends anyway.\n\nIt's been a vague wish of mine for a while to fix \"cherry-pick -x -s\".\nI currently use it without amends and tolerate the blank line between\nthe \"cherry picked\" and the sign-off.\n\nMaybe you can convince people that wanting no extra blank line between\nan existing sign-off and the \"cherry picked from\" line is\noverengineering and worth regressing. I am not a fanatic about it ---\nI just thought it was worth mentioning that this proposed change would\nnot be seen as positive by everyone.\n\nPut another way, I don't find the two words \"totally overengineered\"\nvery convincing here. For what it's worth.\n\nCheers,\nJonathan\n"}]}