{"thread":{"id":"44188","subject":"[RFC/PATCH 2/2] sequencer: allow origin line below commit title","startedAt":"2016-09-29T19:22:04Z","lastAt":"2016-10-06T19:24:58Z","messageCount":20,"participants":["Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"302922","messageId":"14d37973223a60e8ea7a2267683dabda2cc492f2.1475176070.git.jonathantanmy@google.com","threadId":"44188","inReplyTo":"cover.1475176070.git.jonathantanmy@google.com","subject":"[RFC/PATCH 2/2] sequencer: allow origin line below commit title","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-29T19:21:38Z","receivedAt":"2016-09-29T19:22:04Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When git cherry-pick -x is invoked, a \"(cherry picked from commit ...)\"\nline is appended to the footer of a commit message that Git interprets\nto contain a footer; otherwise it is appended at the end as a new\nparagraph, preceded by a blank line. This behavior may appear\ninconsistent, especially to users who differ from Git in their\ninterpretation of what constitutes a footer.\n\nProvide the user, through a configuration option and command-line flag,\nthe option of placing the \"cherry picked\" line below the commit title\ninstead of the current behavior.  This allows the \"cherry picked\" line\nto be placed in a consistent manner, independent of the nature of the\nfooter of the existing commit message.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n Documentation/config.txt          |  4 +++\n Documentation/git-cherry-pick.txt | 15 +++++++++-\n builtin/revert.c                  | 38 ++++++++++++++++++++++++-\n sequencer.c                       | 39 ++++++++++++++++++++------\n sequencer.h                       |  7 +++++\n t/t3511-cherry-pick-x.sh          | 59 +++++++++++++++++++++++++++++++++++++++\n 6 files changed, 152 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0bcb679..fb1990f 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -945,6 +945,10 @@ browser.<tool>.path::\n \tbrowse HTML help (see `-w` option in linkgit:git-help[1]) or a\n \tworking repository in gitweb (see linkgit:git-instaweb[1]).\n \n+cherrypick.originLineLocation::\n+\tDefault for the `--origin-line-location` option in git-cherry-pick.\n+\tDefaults to `bottom`.\n+\n clean.requireForce::\n \tA boolean to make git-clean do nothing unless given -f,\n \t-i or -n.   Defaults to true.\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex d35d771..5a8359f 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -58,7 +58,7 @@ OPTIONS\n \tmessage prior to committing.\n \n -x::\n-\tWhen recording the commit, append a line that says\n+\tWhen recording the commit, add a line that says\n \t\"(cherry picked from commit ...)\" to the original commit\n \tmessage in order to indicate which commit this change was\n \tcherry-picked from.  This is done only for cherry\n@@ -71,6 +71,14 @@ OPTIONS\n \tdevelopment branch), adding this information can be\n \tuseful.\n \n+--origin-line-location::\n+\tWhere to put the \"(cherry picked from commit ...)\" line when requested\n+\twith the `-x` option.  May be `top`, meaning at the top of the commit\n+\tmessage body, immediately below the commit title (see the DISCUSSION\n+\tsection of linkgit:git-commit[1]), or `bottom`, meaning at the end of\n+\tthe commit message.  The default is controlled by the\n+\t`cherrypick.originLineLocation` configuration variable.\n+\n -r::\n \tIt used to be that the command defaulted to do `-x`\n \tdescribed above, and `-r` was to disable it.  Now the\n@@ -224,6 +232,11 @@ the working tree.\n spending extra time to avoid mistakes based on incorrectly matching\n context lines.\n \n+CONFIGURATION\n+-------------\n+cherrypick.originLineLocation::\n+\tDefault for the `--origin-line-location` option.  Defaults to `bottom`.\n+\n SEE ALSO\n --------\n linkgit:git-revert[1]\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 4e69380..a5459a0 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -71,11 +71,25 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...)\n \t\tdie(_(\"%s: %s cannot be used with %s\"), me, this_opt, base_opt);\n }\n \n+static int set_origin_line(enum origin_line *line, const char *str)\n+{\n+\tif (!strcmp(str, \"bottom\")) {\n+\t\t*line = ORIGIN_LINE_BOTTOM;\n+\t\treturn 1;\n+\t}\n+\tif (!strcmp(str, \"top\")) {\n+\t\t*line = ORIGIN_LINE_TOP;\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n {\n \tconst char * const * usage_str = revert_or_cherry_pick_usage(opts);\n \tconst char *me = action_name(opts);\n \tint cmd = 0;\n+\tconst char *origin_str = NULL;\n \tstruct option base_options[] = {\n \t\tOPT_CMDMODE(0, \"quit\", &cmd, N_(\"end revert or cherry-pick sequence\"), 'q'),\n \t\tOPT_CMDMODE(0, \"continue\", &cmd, N_(\"resume revert or cherry-pick sequence\"), 'c'),\n@@ -98,6 +112,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \tif (opts->action == REPLAY_PICK) {\n \t\tstruct option cp_extra[] = {\n \t\t\tOPT_BOOL('x', NULL, &opts->record_origin, N_(\"append commit name\")),\n+\t\t\tOPT_STRING(0, \"origin-line-location\", &origin_str, N_(\"origin-line-location\"), N_(\"location of appended commit name\")),\n \t\t\tOPT_BOOL(0, \"ff\", &opts->allow_ff, N_(\"allow fast-forward\")),\n \t\t\tOPT_BOOL(0, \"allow-empty\", &opts->allow_empty, N_(\"preserve initially empty commits\")),\n \t\t\tOPT_BOOL(0, \"allow-empty-message\", &opts->allow_empty_message, N_(\"allow commits with empty messages\")),\n@@ -125,6 +140,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \telse\n \t\topts->subcommand = REPLAY_NONE;\n \n+\t/* Set the origin line location */\n+\tif (origin_str)\n+\t\tif (!set_origin_line(&opts->origin_line, origin_str))\n+\t\t\tdie(_(\"%s: --origin-line-location must be top or bottom\"),\n+\t\t\t    me);\n+\n \t/* Check for incompatible command line arguments */\n \tif (opts->subcommand != REPLAY_NONE) {\n \t\tchar *this_operation;\n@@ -176,6 +197,21 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\tusage_with_options(usage_str, options);\n }\n \n+static int git_cherry_pick_config(const char *var, const char *value,\n+\t\t\t\t  void *opts_)\n+{\n+\tstruct replay_opts *opts = opts_;\n+\n+\tif (!strcmp(var, \"cherrypick.originlinelocation\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\tset_origin_line(&opts->origin_line, value);\n+\t\treturn 0;\n+\t}\n+\n+\treturn git_default_config(var, value, opts_);\n+}\n+\n int cmd_revert(int argc, const char **argv, const char *prefix)\n {\n \tstruct replay_opts opts;\n@@ -200,7 +236,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.action = REPLAY_PICK;\n-\tgit_config(git_default_config, NULL);\n+\tgit_config(git_cherry_pick_config, &opts);\n \tparse_args(argc, argv, &opts);\n \tres = sequencer_pick_revisions(&opts);\n \tif (res < 0)\ndiff --git a/sequencer.c b/sequencer.c\nindex b29c9ca..ef9e5bb 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -450,23 +450,45 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)\n static void append_message(struct strbuf *msgbuf,\n \t\t\t   const struct commit_message *msg,\n \t\t\t   int record_origin,\n+\t\t\t   enum origin_line origin_line,\n \t\t\t   const struct commit *commit)\n {\n \t/*\n-\t * Append the commit log message to msgbuf; it starts\n+\t * The commit log message starts\n \t * after the tree, parent, author, committer\n \t * information followed by \"\\n\\n\".\n \t */\n \tconst char *p = strstr(msg->message, \"\\n\\n\");\n-\tif (p)\n-\t\tstrbuf_addstr(msgbuf, skip_blank_lines(p + 2));\n+\tp = skip_blank_lines(p + 2);\n+\tif (!record_origin) {\n+\t\tstrbuf_addstr(msgbuf, p);\n+\t\treturn;\n+\t}\n \n-\tif (record_origin) {\n+\tswitch (origin_line) {\n+\tcase ORIGIN_LINE_TOP:\n+\t\t/* First, add only the subject. */\n+\t\tp = format_subject(msgbuf, p, \"\\n\");\n+\t\tstrbuf_addstr(msgbuf, \"\\n\\n\");\n+\t\tbreak;\n+\tcase ORIGIN_LINE_BOTTOM:\n+\t\tstrbuf_addstr(msgbuf, p);\n \t\tif (!has_conforming_footer(msgbuf, NULL, 0))\n \t\t\tstrbuf_addch(msgbuf, '\\n');\n-\t\tstrbuf_addstr(msgbuf, cherry_picked_prefix);\n-\t\tstrbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));\n-\t\tstrbuf_addstr(msgbuf, \")\\n\");\n+\t\tbreak;\n+\t}\n+\n+\tstrbuf_addstr(msgbuf, cherry_picked_prefix);\n+\tstrbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));\n+\tstrbuf_addstr(msgbuf, \")\\n\");\n+\n+\tif (origin_line == ORIGIN_LINE_TOP) {\n+\t\t/* Add the rest of the commit message. */\n+\t\tp = skip_blank_lines(p);\n+\t\tif (*p) {\n+\t\t\tstrbuf_addch(msgbuf, '\\n');\n+\t\t\tstrbuf_addstr(msgbuf, p);\n+\t\t}\n \t}\n }\n \n@@ -566,7 +588,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\tnext = commit;\n \t\tnext_label = msg.label;\n \n-\t\tappend_message(&msgbuf, &msg, opts->record_origin, commit);\n+\t\tappend_message(&msgbuf, &msg, opts->record_origin,\n+\t\t\t       opts->origin_line, commit);\n \t}\n \n \tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || opts->action == REPLAY_REVERT) {\ndiff --git a/sequencer.h b/sequencer.h\nindex 5ed5cb1..7cf381c 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -20,6 +20,11 @@ enum replay_subcommand {\n \tREPLAY_ROLLBACK\n };\n \n+enum origin_line {\n+\tORIGIN_LINE_BOTTOM,\n+\tORIGIN_LINE_TOP\n+};\n+\n struct replay_opts {\n \tenum replay_action action;\n \tenum replay_subcommand subcommand;\n@@ -46,6 +51,8 @@ struct replay_opts {\n \n \t/* Only used by REPLAY_NONE */\n \tstruct rev_info *revs;\n+\n+\tenum origin_line origin_line;\n };\n \n int sequencer_pick_revisions(struct replay_opts *opts);\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex 9cce5ae..57e3861 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -244,4 +244,63 @@ test_expect_success 'cherry-pick preserves commit message' '\n \ttest_cmp expect actual\n '\n \n+mesg_one_para=\"This is a commit message\n+in one paragraph\"\n+\n+test_expect_success 'cherry-pick -x (top location) one-paragraph commit message' '\n+\tpristine_detach initial &&\n+\ttest_config cherrypick.originLineLocation top &&\n+\ttest_commit \"$mesg_one_para\" foo b mesg-one-para &&\n+\tgit reset --hard initial &&\n+\tsha1=$(git rev-parse mesg-one-para^0) &&\n+\tgit cherry-pick -x mesg-one-para &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_one_para\n+\n+\t\t(cherry picked from commit $sha1)\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+space=\" \"\n+mesg_multi_para=\"$mesg_one_para\n+$space\n+\n+$mesg_one_para\"\n+\n+test_expect_success 'cherry-pick -x (top location) multi-paragraph commit message' '\n+\tpristine_detach initial &&\n+\ttest_config cherrypick.originLineLocation top &&\n+\ttest_commit \"$mesg_multi_para\" foo b mesg-multi-para &&\n+\tgit reset --hard initial &&\n+\tsha1=$(git rev-parse mesg-multi-para^0) &&\n+\tgit cherry-pick -x mesg-multi-para &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_one_para\n+\n+\t\t(cherry picked from commit $sha1)\n+\n+\t\t$mesg_one_para\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'cherry-pick -x location argument overrides config' '\n+\ttest_config cherrypick.originLineLocation top &&\n+\tgit reset --hard initial &&\n+\tsha1=$(git rev-parse mesg-multi-para^0) &&\n+\tgit cherry-pick -x --origin-line-location=bottom mesg-multi-para &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_one_para\n+\n+\t\t$mesg_one_para\n+\n+\t\t(cherry picked from commit $sha1)\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"302923","messageId":"cover.1475176070.git.jonathantanmy@google.com","threadId":"44188","inReplyTo":null,"subject":"[RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-29T19:21:36Z","receivedAt":"2016-09-29T19:22:19Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is somewhat of a follow-up to my previous e-mail with subject\n\"[PATCH] sequencer: support folding in rfc2822 footer\" [1], in which I\nproposed relaxing the definition of a commit message footer to allow\nmultiple-line field bodies (as described in RFC2822), but its strictness\nwas deemed deliberate.\n\nBelow is a patch set that allows placing the \"cherry picked from\" line\nwithout taking into account the definition of a commit message footer.\nFor example, \"git cherry-pick -x\" (with the appropriate configuration\nvariable or argument) would, to this commit message:\n\n  commit title\n\n  This is an explanatory paragraph.\n\n  Footer: foo\n\nplace the \"(cherry picked from ...)\" line below \"commit title\".\n\nWould this be better?\n\n[1] <1472846322-5592-1-git-send-email-jonathantanmy@google.com>\n\nJonathan Tan (2):\n  sequencer: refactor message and origin appending\n  sequencer: allow origin line below commit title\n\n Documentation/config.txt          |  4 +++\n Documentation/git-cherry-pick.txt | 15 ++++++++-\n builtin/revert.c                  | 38 ++++++++++++++++++++-\n sequencer.c                       | 69 +++++++++++++++++++++++++++++----------\n sequencer.h                       |  7 ++++\n t/t3511-cherry-pick-x.sh          | 59 +++++++++++++++++++++++++++++++++\n 6 files changed, 172 insertions(+), 20 deletions(-)\n\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"302924","messageId":"44fe5d92cdcd1b952fe44c9e0b37a1dbf55b38b8.1475176070.git.jonathantanmy@google.com","threadId":"44188","inReplyTo":"cover.1475176070.git.jonathantanmy@google.com","subject":"[RFC/PATCH 1/2] sequencer: refactor message and origin appending","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-29T19:21:37Z","receivedAt":"2016-09-29T19:22:21Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Move the appending of the commit message and origin information into its\nown function, in preparation for a subsequent patch.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n sequencer.c | 46 ++++++++++++++++++++++++++++------------------\n 1 file changed, 28 insertions(+), 18 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3804fa9..b29c9ca 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -443,6 +443,33 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)\n \t\treturn 1;\n }\n \n+/*\n+ * Appends the commit log message, including the cherry picked notification if\n+ * record_origin is nonzero.\n+ */\n+static void append_message(struct strbuf *msgbuf,\n+\t\t\t   const struct commit_message *msg,\n+\t\t\t   int record_origin,\n+\t\t\t   const struct commit *commit)\n+{\n+\t/*\n+\t * Append the commit log message to msgbuf; it starts\n+\t * after the tree, parent, author, committer\n+\t * information followed by \"\\n\\n\".\n+\t */\n+\tconst char *p = strstr(msg->message, \"\\n\\n\");\n+\tif (p)\n+\t\tstrbuf_addstr(msgbuf, skip_blank_lines(p + 2));\n+\n+\tif (record_origin) {\n+\t\tif (!has_conforming_footer(msgbuf, NULL, 0))\n+\t\t\tstrbuf_addch(msgbuf, '\\n');\n+\t\tstrbuf_addstr(msgbuf, cherry_picked_prefix);\n+\t\tstrbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));\n+\t\tstrbuf_addstr(msgbuf, \")\\n\");\n+\t}\n+}\n+\n static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n {\n \tunsigned char head[20];\n@@ -534,29 +561,12 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t}\n \t\tstrbuf_addstr(&msgbuf, \".\\n\");\n \t} else {\n-\t\tconst char *p;\n-\n \t\tbase = parent;\n \t\tbase_label = msg.parent_label;\n \t\tnext = commit;\n \t\tnext_label = msg.label;\n \n-\t\t/*\n-\t\t * Append the commit log message to msgbuf; it starts\n-\t\t * after the tree, parent, author, committer\n-\t\t * information followed by \"\\n\\n\".\n-\t\t */\n-\t\tp = strstr(msg.message, \"\\n\\n\");\n-\t\tif (p)\n-\t\t\tstrbuf_addstr(&msgbuf, skip_blank_lines(p + 2));\n-\n-\t\tif (opts->record_origin) {\n-\t\t\tif (!has_conforming_footer(&msgbuf, NULL, 0))\n-\t\t\t\tstrbuf_addch(&msgbuf, '\\n');\n-\t\t\tstrbuf_addstr(&msgbuf, cherry_picked_prefix);\n-\t\t\tstrbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));\n-\t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n-\t\t}\n+\t\tappend_message(&msgbuf, &msg, opts->record_origin, commit);\n \t}\n \n \tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || opts->action == REPLAY_REVERT) {\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"302958","messageId":"xmqqtwcycqul.fsf@gitster.mtv.corp.google.com","threadId":"44188","inReplyTo":"cover.1475176070.git.jonathantanmy@google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-29T21:56:34Z","receivedAt":"2016-09-29T21:56:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> This is somewhat of a follow-up to my previous e-mail with subject\n> \"[PATCH] sequencer: support folding in rfc2822 footer\" [1], in which I\n> proposed relaxing the definition of a commit message footer to allow\n> multiple-line field bodies (as described in RFC2822), but its strictness\n> was deemed deliberate.\n\nIt does not necessarily mean we can never change it when we did\nsomething deliberately, though.  With a good enough justification,\nand with a transitition plan if the backward incompatibility is\nsevere enough to warrant one, we can change things.\n\nI vaguely recall that there were some discussion on the definition\nof \"what's a trailer line\" with folks from the kernel land, perhaps\nwhile discussing the interpret-trailers topic.  IIRC, when somebody\npasses an improved version along, the resulting message's trailer\nblock may look like this:\n\n    Signed-off-by: Original Author <original@author.xz>\n    [fixed typo in the variable names]\n    Signed-off-by: Somebhody Else <somebody@else.xz>\n\nand an obvious \"wish\" of theirs was to treat not just RFC2822-like\n\"a line that begins with token followed by a colon\" but also these\nshort comments as part of the trailer block.  Your original wish in\n[*1*] is to also treat \"a line that begin with a whitespace that\nfollows a line that begins with token followed by a colon\" as part\nof the trailer block and I personally think that is a reasonable\nthing to wish for, too.\n\nI recall that I was somewhat surprised and dissapointed to see no\nchange to interpret-trailers when you tried [*1*], which was really\nabout improving the definition of what the trailer block is, by the\nway.\n\nIn any case, if we want to improve what the trailer block is, we\nwould certainly need to make sure what is inserted by \"cherry-pick -x\"\nis also considered as part of the trailer block, so it may be necessary\nto change it to \"Cherry-picked-from: ...\" while doing so.  I dunno.\n\n> Below is a patch set that allows placing the \"cherry picked from\" line\n> without taking into account the definition of a commit message footer.\n> For example, \"git cherry-pick -x\" (with the appropriate configuration\n> variable or argument) would, to this commit message:\n>\n>   commit title\n>\n>   This is an explanatory paragraph.\n>\n>   Footer: foo\n>\n> place the \"(cherry picked from ...)\" line below \"commit title\".\n>\n> Would this be better?\n\nIt is not immediately obvious what such a change buys us.  Wouldn't\nthe current code place that line below \"Footer: foo\"?  I cannot\nthink of any reason why anybody would want to place \"cherry-picked\nfrom\" immediately below the title and before the first line of the\nbody.\n\n\n[Footnotes]\n\n*1* http://public-inbox.org/git/1472846322-5592-1-git-send-email-jonathantanmy@google.com/\n"},{"id":"302999","messageId":"11e41a94-df8c-494a-584b-e2bc8da2de3a@google.com","threadId":"44188","inReplyTo":"xmqqtwcycqul.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-30T18:22:05Z","receivedAt":"2016-09-30T18:22:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 09/29/2016 02:56 PM, Junio C Hamano wrote:\n> Jonathan Tan <jonathantanmy@google.com> writes:\n>\n>> This is somewhat of a follow-up to my previous e-mail with subject\n>> \"[PATCH] sequencer: support folding in rfc2822 footer\" [1], in which I\n>> proposed relaxing the definition of a commit message footer to allow\n>> multiple-line field bodies (as described in RFC2822), but its strictness\n>> was deemed deliberate.\n>\n> It does not necessarily mean we can never change it when we did\n> something deliberately, though.  With a good enough justification,\n> and with a transitition plan if the backward incompatibility is\n> severe enough to warrant one, we can change things.\n>\n> I vaguely recall that there were some discussion on the definition\n> of \"what's a trailer line\" with folks from the kernel land, perhaps\n> while discussing the interpret-trailers topic.  IIRC, when somebody\n> passes an improved version along, the resulting message's trailer\n> block may look like this:\n>\n>     Signed-off-by: Original Author <original@author.xz>\n>     [fixed typo in the variable names]\n>     Signed-off-by: Somebhody Else <somebody@else.xz>\n>\n> and an obvious \"wish\" of theirs was to treat not just RFC2822-like\n> \"a line that begins with token followed by a colon\" but also these\n> short comments as part of the trailer block.  Your original wish in\n> [*1*] is to also treat \"a line that begin with a whitespace that\n> follows a line that begins with token followed by a colon\" as part\n> of the trailer block and I personally think that is a reasonable\n> thing to wish for, too.\n\nIf we allowed arbitrary lines in the trailer block, this would solve my \noriginal problem, yes.\n\n> I recall that I was somewhat surprised and dissapointed to see no\n> change to interpret-trailers when you tried [*1*], which was really\n> about improving the definition of what the trailer block is, by the\n> way.\n\nSorry, I had missed that.\n\nLooking at that, it seems that sequencer.c started interpreting the last \nparagraph of the commit message as a footer and adding an exception for \n\"cherry picked from\" in commit b971e04 (\"sequencer.c: always separate \n\"(cherry picked from\" from commit body\", 2013-02-12). So the \ninterpretations of sequencer.c and interpret-trailers were already \ndivergent, but I should have probably at least discussed that.\n\n>> Below is a patch set that allows placing the \"cherry picked from\" line\n>> without taking into account the definition of a commit message footer.\n>> For example, \"git cherry-pick -x\" (with the appropriate configuration\n>> variable or argument) would, to this commit message:\n>>\n>>   commit title\n>>\n>>   This is an explanatory paragraph.\n>>\n>>   Footer: foo\n>>\n>> place the \"(cherry picked from ...)\" line below \"commit title\".\n>>\n>> Would this be better?\n>\n> It is not immediately obvious what such a change buys us.  Wouldn't\n> the current code place that line below \"Footer: foo\"?  I cannot\n> think of any reason why anybody would want to place \"cherry-picked\n> from\" immediately below the title and before the first line of the\n> body.\n\nYes, the current code would place it below \"Footer: foo\" without a blank \nline before it, but if it thinks that the so-called footer is not \nactually a footer, it would insert a blank line before that line.\n\nAs for a reason:\n\n1) I do not have a specific reason for placing it in that exact \nposition, but I would like to be able to place the \"cherry picked from\" \nline without affecting the last paragraph (specifically, without making \nthe \"cherry picked from\" line the only line in the last paragraph). It \nseems to me that placing it below the title was the most straightforward \nway to do that - this way, Git can have its own idea of what a footer \nconstitutes, and the user can treat it as completely separate from the \n\"cherry picked from\" line mechanism.\n\n1a) (Avoiding the footer might also be a good way of more clearly \ndefining what the footer is. For example, currently, \"cherry picked \nfrom\" is treated as a special case in sequencer.c but not in trailer.c, \nas far as I can tell. If we consistently avoided the footer, we wouldn't \nneed such a special case anywhere.)\n\n2) The Linux kernel's repository has some \"commit ... upstream.\" lines \nin this position (below the commit title) - for example, in commit \ndacc0987fd2e.\n\n[1] \nhttps://kernel.googlesource.com/pub/scm/linux/kernel/git/stable/linux-stable/+/dacc0987fd2e25a8b4b8c19778862ba12ce76d0a\n"},{"id":"303005","messageId":"xmqq8tu99o75.fsf@gitster.mtv.corp.google.com","threadId":"44188","inReplyTo":"11e41a94-df8c-494a-584b-e2bc8da2de3a@google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-30T19:34:22Z","receivedAt":"2016-09-30T19:34:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n>> I vaguely recall that there were some discussion on the definition\n>> of \"what's a trailer line\" with folks from the kernel land, perhaps\n>> while discussing the interpret-trailers topic.  IIRC, when somebody\n>> passes an improved version along, the resulting message's trailer\n>> block may look like this:\n>>\n>>     Signed-off-by: Original Author <original@author.xz>\n>>     [fixed typo in the variable names]\n>>     Signed-off-by: Somebhody Else <somebody@else.xz>\n>>\n>> and an obvious \"wish\" of theirs was to treat not just RFC2822-like\n>> \"a line that begins with token followed by a colon\" but also these\n>> short comments as part of the trailer block.  Your original wish in\n>> [*1*] is to also treat \"a line that begin with a whitespace that\n>> follows a line that begins with token followed by a colon\" as part\n>> of the trailer block and I personally think that is a reasonable\n>> thing to wish for, too.\n>\n> If we allowed arbitrary lines in the trailer block, this would solve\n> my original problem, yes.\n\nOK.\n\n> Looking at that, it seems that sequencer.c started interpreting the\n> last paragraph of the commit message as a footer and adding an\n> exception for \"cherry picked from\" in commit b971e04 (\"sequencer.c:\n> always separate \"(cherry picked from\" from commit body\",\n> 2013-02-12). So the interpretations of sequencer.c and\n> interpret-trailers were already divergent, but I should have probably\n> at least discussed that.\n\nIt is not too late to discuss it.  I still think it is a good longer\nterm plan to try to unify the definition of what a trailer block is\nand the implementation of the code that determines the boundary\nbetween the log message proper and the trailer block and that allows\nus to manipulate the trailer block, that currently is scattered\nacross multiple places into one.  Historically, \"commit -s\" had one\n(because it needed to decide if it needs to see if the last sign-off\nis already the one it is adding, and to decide if a blank line is\nneeded before the sing-off being added), \"am -s\" had another, and\n\"cherry-pick\" probably had one, too.  \"interpret-trailers\" was, at\nleast originally, envisioned as an effort to develop a unified\nmachinery that can be called from these codepaths, and to aid the\ndevelopment and encourage its use, it also had its own end-user\nfacing command.  Your interest in the \"trailer\" topic may be a good\ntrigger for us to further that original vision.\n\n> As for a reason:\n>\n> 1) I do not have a specific reason for placing it in that exact\n> position, but I would like to be able to place the \"cherry picked\n> from\" line without affecting the last paragraph (specifically, without\n> making the \"cherry picked from\" line the only line in the last\n> paragraph).\n> ...\n> 1a) (Avoiding the footer might also be a good way of more clearly\n> defining what the footer is. For example, currently, \"cherry picked\n> from\" is treated as a special case in sequencer.c but not in\n> trailer.c, as far as I can tell. If we consistently avoided the\n> footer, we wouldn't need such a special case anywhere.)\n\nThat is one of the numerous shortcomings of the \"interpret-trailers\"\nthat is still not finished, I would say.\n\n> 2) The Linux kernel's repository has some \"commit ... upstream.\" lines\n> in this position (below the commit title) - for example, in commit\n> dacc0987fd2e.\n\n\"A group of people seem to prefer it there\" does not lead to\n\"therefore let's move it there for everybody\".  It does open a\npossibility that we may want to add a new option to put it there,\nbut does not justify changing what existing \"-x\" option does.\n"},{"id":"303016","messageId":"42332581-e47a-0fc8-ed5c-44e7e1c19341@google.com","threadId":"44188","inReplyTo":"xmqq8tu99o75.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-30T20:23:28Z","receivedAt":"2016-09-30T20:23:37Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 09/30/2016 12:34 PM, Junio C Hamano wrote:\n>> 2) The Linux kernel's repository has some \"commit ... upstream.\" lines\n>> in this position (below the commit title) - for example, in commit\n>> dacc0987fd2e.\n>\n> \"A group of people seem to prefer it there\" does not lead to\n> \"therefore let's move it there for everybody\".  It does open a\n> possibility that we may want to add a new option to put it there,\n> but does not justify changing what existing \"-x\" option does.\n\nTo clarify, my patch adds the new option you described (to place it \nbelow the title instead of at the bottom of the commit message). The \ndefault is still the current behavior.\n"},{"id":"303017","messageId":"xmqqtwcx8669.fsf@gitster.mtv.corp.google.com","threadId":"44188","inReplyTo":"xmqq8tu99o75.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-30T20:49:02Z","receivedAt":"2016-09-30T20:49:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jonathan Tan <jonathantanmy@google.com> writes:\n>\n>>> I vaguely recall that there were some discussion on the definition\n>>> of \"what's a trailer line\" with folks from the kernel land, perhaps\n>>> while discussing the interpret-trailers topic.  IIRC, when somebody\n>>> passes an improved version along, the resulting message's trailer\n>>> block may look like this:\n>>>\n>>>     Signed-off-by: Original Author <original@author.xz>\n>>>     [fixed typo in the variable names]\n>>>     Signed-off-by: Somebhody Else <somebody@else.xz>\n>>>\n>>> and an obvious \"wish\" of theirs was to treat not just RFC2822-like\n>>> \"a line that begins with token followed by a colon\" but also these\n>>> short comments as part of the trailer block.  Your original wish in\n>>> [*1*] is to also treat \"a line that begin with a whitespace that\n>>> follows a line that begins with token followed by a colon\" as part\n>>> of the trailer block and I personally think that is a reasonable\n>>> thing to wish for, too.\n>>\n>> If we allowed arbitrary lines in the trailer block, this would solve\n>> my original problem, yes.\n\nHere is an experiment I ran during my lunch break.  The script\n(attached) is meant to run in the kernel repository and\nfor each log messages of each non-merge commit:\n\n * find its last paragraph, where the definition of paragraph is\n   simply \"a blank/empty line\";\n\n * inspect if there is at least one RFC2822-header-looking line, or\n   a line that begins with \"(cherry picked from\";\n\n * dump the ones that do not pass the above criteria.\n\nMy cursory look of the output did not spot a legitimate trailer\nblock that we should have identified.  The output lines shown were\nones that are not signed off at all (e.g. af8c34ce6ae32add that says\n\"Linux 4.7-rc2\"), ones that has three-dash line \"---\" in them\n(e.g. 133d558216d9), ones that has diffstat that should have been\nafter \"---\" (e.g. 259307074bfcf1f).\n\nThe story is the same if you run it in git.git; the \"do we have at\nleast one rfc2822-header-looking line or '(cherry picked from' line\nin the last paragraph? if so, then that is an existing trailer\nblock\" seems to be a good heuristics to cover many cases like\nthese:\n\n    d0196c8d5d3057c5c21a82f3d0113ca8e501033b\n    Signed-off-by: Arnd Bergmann <arnd@arndb.de>\n    [tomi.valkeinen@ti.com: resolved conflicts]\n    Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>\n\n    59f0aa9480cfef9173a648cec4537addc5f3ad94\n    Link 1: https://bugzilla.kernel.org/show_bug.cgi?id=9916\n            http://bugzilla.kernel.org/show_bug.cgi?id=10100\n            https://lkml.org/lkml/2008/2/25/282\n    Link 2: https://bugzilla.kernel.org/show_bug.cgi?id=9399\n            https://bugzilla.kernel.org/show_bug.cgi?id=12461\n            https://bugzilla.kernel.org/show_bug.cgi?id=11880\n    Link 3: https://bugzilla.kernel.org/show_bug.cgi?id=11884\n            https://bugzilla.kernel.org/show_bug.cgi?id=14081\n            https://bugzilla.kernel.org/show_bug.cgi?id=14086\n            https://bugzilla.kernel.org/show_bug.cgi?id=14446\n    Link 4: https://bugzilla.kernel.org/show_bug.cgi?id=112911\n    Signed-off-by: Lv Zheng <lv.zheng@intel.com>\n    Tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>\n    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>\n\n-- >8 --\n#!/bin/sh\n\ngit log --no-merges |\nperl -e '\n\tsub flush {\n\t\tmy ($commit, @lines) = @_;\n\t\tmy $seen_good = 0;\n\t\tfor (@lines) {\n\t\t\tif (/^[-A-Za-z0-9]+: / ||\n\t\t\t    /^\\(cherry picked from/) {\n\t\t\t\t$seen_good = 1;\n\t\t\t\tlast;\n\t\t\t}\n\t\t}\n\t\tif (!$seen_good) {\n\t\t\tprint \"\\n$commit\\n\";\n\t\t\tfor (@lines) {\n\t\t\t\tprint;\n\t\t\t}\n\t\t}\n\t}\n\n\tmy (@lines, $this);\n\twhile (<>) {\n\t\tif (/^commit (.*)$/) {\n\t\t\tmy $next = $1;\n\t\t\tflush($this, @lines);\n\t\t\t@lines = ();\n\t\t\t$this = $next;\n\t\t}\n\t\tif (s/^    //) {\n\t\t\tif (/^\\s*$/) {\n\t\t\t\t@lines = ();\n\t\t\t} else {\n\t\t\t\tpush @lines, $_;\n\t\t\t}\n\t\t}\n\t}\n\tif (@lines && $this) {\n\t\tflush($this, @lines);\n\t}\n'\n"},{"id":"303091","messageId":"xmqqa8el8nja.fsf@gitster.mtv.corp.google.com","threadId":"44188","inReplyTo":"42332581-e47a-0fc8-ed5c-44e7e1c19341@google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-03T15:23:05Z","receivedAt":"2016-10-03T15:23:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> On 09/30/2016 12:34 PM, Junio C Hamano wrote:\n>>> 2) The Linux kernel's repository has some \"commit ... upstream.\" lines\n>>> in this position (below the commit title) - for example, in commit\n>>> dacc0987fd2e.\n>>\n>> \"A group of people seem to prefer it there\" does not lead to\n>> \"therefore let's move it there for everybody\".  It does open a\n>> possibility that we may want to add a new option to put it there,\n>> but does not justify changing what existing \"-x\" option does.\n>\n> To clarify, my patch adds the new option you described (to place it\n> below the title instead of at the bottom of the commit message). The\n> default is still the current behavior.\n\nAh, sorry, I missed that.  No objection from me on this point then.\n\nThanks.\n"},{"id":"303102","messageId":"e03fdabd-6690-5244-5f79-1715b0364845@google.com","threadId":"44188","inReplyTo":"xmqqtwcx8669.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-03T17:44:54Z","receivedAt":"2016-10-03T17:45:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 09/30/2016 01:49 PM, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Jonathan Tan <jonathantanmy@google.com> writes:\n>>\n>>>> I vaguely recall that there were some discussion on the definition\n>>>> of \"what's a trailer line\" with folks from the kernel land, perhaps\n>>>> while discussing the interpret-trailers topic.  IIRC, when somebody\n>>>> passes an improved version along, the resulting message's trailer\n>>>> block may look like this:\n>>>>\n>>>>     Signed-off-by: Original Author <original@author.xz>\n>>>>     [fixed typo in the variable names]\n>>>>     Signed-off-by: Somebhody Else <somebody@else.xz>\n>>>>\n>>>> and an obvious \"wish\" of theirs was to treat not just RFC2822-like\n>>>> \"a line that begins with token followed by a colon\" but also these\n>>>> short comments as part of the trailer block.  Your original wish in\n>>>> [*1*] is to also treat \"a line that begin with a whitespace that\n>>>> follows a line that begins with token followed by a colon\" as part\n>>>> of the trailer block and I personally think that is a reasonable\n>>>> thing to wish for, too.\n>>>\n>>> If we allowed arbitrary lines in the trailer block, this would solve\n>>> my original problem, yes.\n>\n> Here is an experiment I ran during my lunch break.  The script\n> (attached) is meant to run in the kernel repository and\n> for each log messages of each non-merge commit:\n>\n>  * find its last paragraph, where the definition of paragraph is\n>    simply \"a blank/empty line\";\n>\n>  * inspect if there is at least one RFC2822-header-looking line, or\n>    a line that begins with \"(cherry picked from\";\n>\n>  * dump the ones that do not pass the above criteria.\n>\n> My cursory look of the output did not spot a legitimate trailer\n> block that we should have identified.  The output lines shown were\n> ones that are not signed off at all (e.g. af8c34ce6ae32add that says\n> \"Linux 4.7-rc2\"), ones that has three-dash line \"---\" in them\n> (e.g. 133d558216d9), ones that has diffstat that should have been\n> after \"---\" (e.g. 259307074bfcf1f).\n>\n> The story is the same if you run it in git.git; the \"do we have at\n> least one rfc2822-header-looking line or '(cherry picked from' line\n> in the last paragraph? if so, then that is an existing trailer\n> block\" seems to be a good heuristics to cover many cases like\n> these:\n>\n>     d0196c8d5d3057c5c21a82f3d0113ca8e501033b\n>     Signed-off-by: Arnd Bergmann <arnd@arndb.de>\n>     [tomi.valkeinen@ti.com: resolved conflicts]\n>     Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>\n>\n>     59f0aa9480cfef9173a648cec4537addc5f3ad94\n>     Link 1: https://bugzilla.kernel.org/show_bug.cgi?id=9916\n>             http://bugzilla.kernel.org/show_bug.cgi?id=10100\n>             https://lkml.org/lkml/2008/2/25/282\n>     Link 2: https://bugzilla.kernel.org/show_bug.cgi?id=9399\n>             https://bugzilla.kernel.org/show_bug.cgi?id=12461\n>             https://bugzilla.kernel.org/show_bug.cgi?id=11880\n>     Link 3: https://bugzilla.kernel.org/show_bug.cgi?id=11884\n>             https://bugzilla.kernel.org/show_bug.cgi?id=14081\n>             https://bugzilla.kernel.org/show_bug.cgi?id=14086\n>             https://bugzilla.kernel.org/show_bug.cgi?id=14446\n>     Link 4: https://bugzilla.kernel.org/show_bug.cgi?id=112911\n>     Signed-off-by: Lv Zheng <lv.zheng@intel.com>\n>     Tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>\n>     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>\n\nThat sounds reasonable to me. Would a patch set to implement this new \ntrailer block heuristic (in both sequencer and trailer) be reasonable? \nAnd if yes, should trailer know about the \"(cherry picked from\" prefix? \n(I can see it both ways - knowing about the \"(cherry picked from\" prefix \nwould make it consistent with sequencer, but it seems like a detail that \nit shouldn't know about. Writing \"Cherry-picked-from:\" instead probably \nwouldn't solve our problem because, for backwards compatibility, we \nwould still need to support reading the old format.)\n"},{"id":"303107","messageId":"xmqqbmz16y42.fsf@gitster.mtv.corp.google.com","threadId":"44188","inReplyTo":"e03fdabd-6690-5244-5f79-1715b0364845@google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-03T19:17:33Z","receivedAt":"2016-10-03T19:17:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> That sounds reasonable to me. Would a patch set to implement this new\n> trailer block heuristic (in both sequencer and trailer) be reasonable?\n> And if yes, should trailer know about the \"(cherry picked from\"\n> prefix? (I can see it both ways - knowing about the \"(cherry picked\n> from\" prefix would make it consistent with sequencer, but it seems\n> like a detail that it shouldn't know about. Writing\n> \"Cherry-picked-from:\" instead probably wouldn't solve our problem\n> because, for backwards compatibility, we would still need to support\n> reading the old format.)\n\nIf we were to go that route, I'd suggest keeping the historical\npractice supported, exactly because you would need to be prepared to\ncherry-pick an old commit.\n\nIt may be necessary for the code to analize the lines in a block\nidentified as \"likely to be a trailing block\" more carefully,\nthough.  The example 59f0aa94 in the message you are responding to\nhas \"Link 1:\" that consists of 3 physical lines.  An instruction to\ninterpret-trailers to add a new one _after_ \"Link-$n:\" may have to\ntreat these as a single logical line and do the addition after them,\ni.e. before \"Link 2:\" line, for example.\n\nI also saw\n\n\tSigned-off-by: Some body <some@body.xz> (some comment\n        that extends to the next line without being indented)\n\tSined-off-by: Some body Else <some.body@else.xz>\n\nwhere the only clue that the second line is logically a part of the\nfirst one was the balancing of parentheses (or [brakets]).  To\naccomodate real-world use cases, you may have to take into account a\nlot more than the strict rfc-822 style \"line folding\".\n\n\n"},{"id":"303143","messageId":"d3df0636-1975-1d08-2f34-384984c72e5d@google.com","threadId":"44188","inReplyTo":"xmqqbmz16y42.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-03T21:28:14Z","receivedAt":"2016-10-03T21:28:21Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 10/03/2016 12:17 PM, Junio C Hamano wrote:\n> It may be necessary for the code to analize the lines in a block\n> identified as \"likely to be a trailing block\" more carefully,\n> though.  The example 59f0aa94 in the message you are responding to\n> has \"Link 1:\" that consists of 3 physical lines.  An instruction to\n> interpret-trailers to add a new one _after_ \"Link-$n:\" may have to\n> treat these as a single logical line and do the addition after them,\n> i.e. before \"Link 2:\" line, for example.\n>\n> I also saw\n>\n> \tSigned-off-by: Some body <some@body.xz> (some comment\n>         that extends to the next line without being indented)\n> \tSined-off-by: Some body Else <some.body@else.xz>\n>\n> where the only clue that the second line is logically a part of the\n> first one was the balancing of parentheses (or [brakets]).  To\n> accomodate real-world use cases, you may have to take into account a\n> lot more than the strict rfc-822 style \"line folding\".\n\nIf we define a logical trailer line as the set of physical lines until \nthe next trailer line...it is true that this has some precedence in that \nsuch lines can be written (although this might not be intentional):\n\n   $ git interpret-trailers --trailer \"a=foo\n   bar\" </dev/null\n\n   a: foo\n   bar\n\nThis solves the \"Line 1:\" case above, but raises other issues:\n\n1. Checking for existence (trailer.ifexists) is now conceptually more \ndifficult. For example, handling of inner whitespace in between lines \nmight be confusing (currently, there is only one line, and whitespace \nhandling is clearly described).\n2. Replacement (trailer.ifexists=replace) might replace more than expected.\n3. There is a corner case - the first line might not be a trailer line. \nEven if interpret-trailers knew about \"(cherry picked from\", a user \nmight enter something else there. (We could just declare such blocks as \nnon-trailers, but if we are already loosening the definition of a \ntrailer, this might be something that we should allow.)\n\nMy initial thought was to think of a trailer as a block of trailer lines \npossibly interspersed with other lines. This leads to interpret-trailers \nplacing the trailer in the wrong place if trailer.where=after, as you \ndescribe, but this might not be a big problem if trailer.where=after is \nnot widely used (and it is not the default). (The other options behave \nas expected.)\n\nThere are other options like checking for indentation or checking for \nbalanced parentheses/brackets, but I think that these would lead to \nsurprising behavior for the user (this would mean that whitespace or \ncertain characters could turn a valid trailer into an invalid one or \nvice versa, or change the behavior of trailer.ifexists, especially \n\"replace\").\n"},{"id":"303151","messageId":"xmqq8tu55bel.fsf@gitster.mtv.corp.google.com","threadId":"44188","inReplyTo":"d3df0636-1975-1d08-2f34-384984c72e5d@google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-03T22:13:22Z","receivedAt":"2016-10-03T22:13:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> There are other options like checking for indentation or checking for\n> balanced parentheses/brackets, but I think that these would lead to\n> surprising behavior for the user (this would mean that whitespace or\n> certain characters could turn a valid trailer into an invalid one or\n> vice versa, or change the behavior of trailer.ifexists, especially\n> \"replace\").\n\nYes, that is exactly why I said that it may be necessary for the\ncode to analize the lines in a block identified as \"likely to be a\ntrailing block\" more carefully.  We can afford to be loose as long\nas the only allowed operation is to append one at the end, but once\nwe start removing/replacing an existing entry, etc., the definition\nof what an entry is becomes very much relevant.\n\n"},{"id":"303166","messageId":"84f28caa-2e4b-1231-1a76-3b7e765c0b61@google.com","threadId":"44188","inReplyTo":"xmqq8tu55bel.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-04T00:08:02Z","receivedAt":"2016-10-04T00:08:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 10/03/2016 03:13 PM, Junio C Hamano wrote:\n> Jonathan Tan <jonathantanmy@google.com> writes:\n>\n>> There are other options like checking for indentation or checking for\n>> balanced parentheses/brackets, but I think that these would lead to\n>> surprising behavior for the user (this would mean that whitespace or\n>> certain characters could turn a valid trailer into an invalid one or\n>> vice versa, or change the behavior of trailer.ifexists, especially\n>> \"replace\").\n>\n> Yes, that is exactly why I said that it may be necessary for the\n> code to analize the lines in a block identified as \"likely to be a\n> trailing block\" more carefully.  We can afford to be loose as long\n> as the only allowed operation is to append one at the end, but once\n> we start removing/replacing an existing entry, etc., the definition\n> of what an entry is becomes very much relevant.\n\nI agree, and I was trying to discuss the possible alternatives for the \ndefinition of what an entry is in my previous e-mail.\n\nIf you think that the alternatives are still too loose, I'm not sure if \nwe can make it any tighter. As far as I know, we're dealing with \ntrailers like the following:\n\n   Signed-off-by: A <author@example.com>\n   [This has nothing to do with the above line]\n   Signed-off-by: B <buthor@example.com>\n\nand:\n\n   Link 1: a link\n     a continuation of the above\n\nand:\n\n   Signed-off-by: Some body <some@body.xz> (comment\n   on two lines)\n\nAs I stated in the quoted paragraph, one possibility is to use \nindentation and/or balanced parentheses/brackets to determine if a \ntrailer line continues onto the next line, and this would handle all the \nabove cases, but I still think that these would lead to surprising \nbehavior. Hence my suggestion to just simply define it as a single \nphysical line. But if you think that the pros (of the more complicated \napproach) outweigh the cons, I'm OK with that.\n\nOne alternative is to postpone this decision by changing sequencer only \n(and not trailer) to tolerate other lines in the trailer. This would \nmake them even more divergent (sequencer supports arbitrary lines while \ntrailer doesn't), but they were divergent already (sequencer supports \n\"(cherry picked by\" but trailer doesn't).\n"},{"id":"303261","messageId":"xmqqwphouivf.fsf@gitster.mtv.corp.google.com","threadId":"44188","inReplyTo":"84f28caa-2e4b-1231-1a76-3b7e765c0b61@google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-04T17:25:08Z","receivedAt":"2016-10-04T17:25:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> One alternative is to postpone this decision by changing sequencer\n> only (and not trailer) to tolerate other lines in the trailer. This\n> would make them even more divergent (sequencer supports arbitrary\n> lines while trailer doesn't), but they were divergent already\n> (sequencer supports \"(cherry picked by\" but trailer doesn't).\n\nGiven that we internally do not use the \"trailers\" for anything\nreal, anything you decide to do here would be an improvement ;-)\nBefore, users couldn't even get any of the examples (below, from\nyour message) recognized as trailer blocks.\n\n>   Signed-off-by: A <author@example.com>\n>   [This has nothing to do with the above line]\n>   Signed-off-by: B <buthor@example.com>\n>\n> and:\n>\n>   Link 1: a link\n>     a continuation of the above\n>\n> and:\n>\n>   Signed-off-by: Some body <some@body.xz> (comment\n>   on two lines)\n\nSo I would say it is perfectly OK if your update works only for\ncases we can clearly define the semantics for.  For example, we can\neven start with something simple like:\n\n * A RFC822-header like line, together with any number of whitespace\n   indented lines that immediately follow it, will be taken as a\n   single logical trailer element (with embedded LF in it if it uses\n   the \"line folding\").  For the purpose of \"replace\", the entire\n   single logical trailer element is replaced.\n\n * A line that begins with \"(cherry picked from\" and \"[\" becomes a\n   single logical trailer element.  No continuation of anything\n   fancy.\n\n * A line with any other shape is a garbage line in a trailer\n   block.  It is kept in its place, but because it does not even\n   have <token> part, it will not participate in locating with\n   \"trailer.where\", \"trailer.ifexists\", etc.\n\nA block of lines that appear as the last paragraph in a commit\nmessage is a trailer block if and only if certain number or\npercentage of lines are non-garbage lines according to the above\ndefinition.\n\nThe operations done by the codepaths in the core part of the system\nare much simpler subset of what \"interpret-trailers\" wants to\nsupport, namely, \n\n - append \"(cherry picked from X)\" at the end.\n\n - append \"S-o-b:\" at the end.\n\n - append \"S-o-b:\" unless the same line appears as the last line in\n   the existing trailer block.\n\nand these are quite compatible with a simplified definition of what\na logical line is illustrated in the above example, I would think.\n\nI wonder if we can share a new helper function to do the detection\n(and classification) of a trailer block and parsing the logical\nlines out of a commit log message.  The function signature could be\nas simple as taking a single <const char *> (or a strbuf) that holds\na commit log message, and splitting it out into something like:\n\n    struct {\n\tconst char *whole;\n\tconst char *end_of_message_proper;\n\tstruct {\n\t\tconst char *token;\n\t\tconst char *contents;\n\t} *trailer;\n\tint alloc_trailers, nr_trailers;\n    };\n\nwhere \n\n - whole points at the first byte of the input, i.e. the beginning\n   of the commit message buffer.\n\n - end-of-message-proper points at the first byte of the trailer\n   block into the buffer at \"whole\".\n\n - token is a canonical header name for easy comparison for\n   interpret-trailers (you can use NULL for garbage lines, and made\n   up token like \"[bracket]\" and \"(cherrypick)\" that would not clash\n   with real tokens like \"Signed-off-by\").\n\n - contents is the bytes on the logical line, including the header\n   part\n\nE.g. an element in trailer[] array may say\n\n    {\n\t.token = \"Signed-off-by\",\n        .contents = \"Signed-Off-By: Some Body <some@body.xz>\\n\",\n    }\n\nWith something like that, you can manipulate the \"insert at ...\",\n\"replace\", etc. in the trailer[] array and then produce an updated\ncommit message fairly easily (i.e. copy out the bytes beginning at\n\"whole\" up to \"end_of_message_proper\", then iterate over trailer[]\narray and show their contents field).  The codepaths in the core\npart only need to know \n\n - how to check the last item in trailer[] array to see if it ends\n   with the same sign-off as they are trying to add.\n\n - how to append one new element to the trailer[] array.\n\n - reproduce an updated commit log message after the above.\n\nHmm?\n"},{"id":"303280","messageId":"xmqq37kcufya.fsf@gitster.mtv.corp.google.com","threadId":"44188","inReplyTo":"xmqqwphouivf.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-04T18:28:13Z","receivedAt":"2016-10-04T18:28:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> A block of lines that appear as the last paragraph in a commit\n> message is a trailer block if and only if certain number or\n> percentage of lines are non-garbage lines according to the above\n> definition.\n> ...\n> I wonder if we can share a new helper function to do the detection\n> (and classification) of a trailer block and parsing the logical\n> lines out of a commit log message.  The function signature could be\n> as simple as taking a single <const char *> (or a strbuf) that holds\n> a commit log message, and splitting it out into something like:\n>\n>     struct {\n> \tconst char *whole;\n> \tconst char *end_of_message_proper;\n> \tstruct {\n> \t\tconst char *token;\n> \t\tconst char *contents;\n> \t} *trailer;\n> \tint alloc_trailers, nr_trailers;\n>     };\n>\n> where \n> ...\n\nAn addendum.  We may also want to be prepared to accept an input\nthat has some garbage lines _after_ the trailer block, if we can\nclearly identify them as such.  For example, we could change the\ndefinition of \"the last paragraph\" as \"the block of lines that\ndo not have any empty (or blank) line, that appear either at the end\nof the input, or immediately before three-dash lines\", to allow\n\n    commit title\n    \n    explanation of the change\n\n    Signed-off-by: Some Body <some@body.xz>\n    [some other things]\n    Acked-by: Some Other Person <some@other.xz>\n\n    ---\n     additional comment\n\nwhich (unfortunately) is a rather common pattern for people who plan\nto send the commit over e-mail.\n\nIf we add a new field \"const char *beginning_of_tail_garbage\" next\nto \"end_of_message_proper\" that points at the blank line before the\nthree-dash line in the above example, the parser should be able to\nbreak such an input into a parsed form, allow the trailer[] array to\nbe manipulated and reproduce a commit log message.\n\n"},{"id":"303430","messageId":"a7304731-fa05-92c2-6139-ecd1e6adcefd@google.com","threadId":"44188","inReplyTo":"xmqqwphouivf.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-05T19:38:46Z","receivedAt":"2016-10-05T19:38:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 10/04/2016 10:25 AM, Junio C Hamano wrote:\n> So I would say it is perfectly OK if your update works only for\n> cases we can clearly define the semantics for.  For example, we can\n> even start with something simple like:\n>\n>  * A RFC822-header like line, together with any number of whitespace\n>    indented lines that immediately follow it, will be taken as a\n>    single logical trailer element (with embedded LF in it if it uses\n>    the \"line folding\").  For the purpose of \"replace\", the entire\n>    single logical trailer element is replaced.\n>\n>  * A line that begins with \"(cherry picked from\" and \"[\" becomes a\n>    single logical trailer element.  No continuation of anything\n>    fancy.\n>\n>  * A line with any other shape is a garbage line in a trailer\n>    block.  It is kept in its place, but because it does not even\n>    have <token> part, it will not participate in locating with\n>    \"trailer.where\", \"trailer.ifexists\", etc.\n\nSounds reasonable to me. Would the \"[\" be a bit of overspecification, \nthough, since Git doesn't produce it? Also, identifying it as a garbage \nline probably wouldn't change any behavior - in the Linux kernel \nexamples, it is used to show what happened in between sign-offs, so \nthere will always be one \"Signed-off-by:\" at the top.  (But I do not \nfeel strongly about this.)\n\n> A block of lines that appear as the last paragraph in a commit\n> message is a trailer block if and only if certain number or\n> percentage of lines are non-garbage lines according to the above\n> definition.\n\nI think the number should be 1 - that seems like the easiest to explain. \nBut I'm OK with other suggestions.\n\n> I wonder if we can share a new helper function to do the detection\n> (and classification) of a trailer block and parsing the logical\n> lines out of a commit log message.  The function signature could be\n> as simple as taking a single <const char *> (or a strbuf) that holds\n> a commit log message, and splitting it out into something like:\n>\n>     struct {\n> \tconst char *whole;\n> \tconst char *end_of_message_proper;\n> \tstruct {\n> \t\tconst char *token;\n> \t\tconst char *contents;\n> \t} *trailer;\n> \tint alloc_trailers, nr_trailers;\n>     };\n>\n> where\n>\n>  - whole points at the first byte of the input, i.e. the beginning\n>    of the commit message buffer.\n>\n>  - end-of-message-proper points at the first byte of the trailer\n>    block into the buffer at \"whole\".\n>\n>  - token is a canonical header name for easy comparison for\n>    interpret-trailers (you can use NULL for garbage lines, and made\n>    up token like \"[bracket]\" and \"(cherrypick)\" that would not clash\n>    with real tokens like \"Signed-off-by\").\n>\n>  - contents is the bytes on the logical line, including the header\n>    part\n>\n> E.g. an element in trailer[] array may say\n>\n>     {\n> \t.token = \"Signed-off-by\",\n>         .contents = \"Signed-Off-By: Some Body <some@body.xz>\\n\",\n>     }\n\nI get the impression from the rest of your e-mail that no strings are \nmeant to be copied - is that true? (That sounds like a good idea to me.) \nIn which case this might be better:\n\n   struct {\n     const char *first_trailer; /* = end_of_message_proper */\n     struct {\n       const char *start;\n       const char *value;\n       const char *end;\n     } *trailers;\n     int trailers_nr, trailers_alloc;\n   };\n\nstart = value for \"[\", \"(cherry picked from\" and garbage lines. We also \nneed end because there is no \\0 there (we didn't copy any strings).\n\nThe existing code (in trailer.c) uses a linked list to store trailers, \nbut an array (as written in your e-mail) is probably better for us since \nclients would want to access the last element (as also written in your \ne-mail).\n\n> With something like that, you can manipulate the \"insert at ...\",\n> \"replace\", etc. in the trailer[] array and then produce an updated\n> commit message fairly easily (i.e. copy out the bytes beginning at\n> \"whole\" up to \"end_of_message_proper\", then iterate over trailer[]\n> array and show their contents field).  The codepaths in the core\n> part only need to know\n>\n>  - how to check the last item in trailer[] array to see if it ends\n>    with the same sign-off as they are trying to add.\n>\n>  - how to append one new element to the trailer[] array.\n>\n>  - reproduce an updated commit log message after the above.\n\nI don't think we need trailer block struct -> commit message conversion \n- when adding a new trailer or replacing an existing trailer, the client \ncode can just remember the index and then modify its behavior \naccordingly when iterating through all trailers. But this conversion can \nbe easily added if/when we need it.\n\n > Hmm?\n\nOverall, this seems like a good idea - I'll go ahead and do this if \nthere are no other objections.\n\nIt just occurred to me that there could be some corner cases when the \ntrailer separator is configured to not include \":\" - I'll make sure to \ninclude tests that check those corner cases.\n"},{"id":"303431","messageId":"b0e43079-498a-469b-c4b6-f4fc2b8e2aa4@google.com","threadId":"44188","inReplyTo":"xmqq37kcufya.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-10-05T19:44:26Z","receivedAt":"2016-10-05T19:44:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 10/04/2016 11:28 AM, Junio C Hamano wrote:\n> An addendum.  We may also want to be prepared to accept an input\n> that has some garbage lines _after_ the trailer block, if we can\n> clearly identify them as such.  For example, we could change the\n> definition of \"the last paragraph\" as \"the block of lines that\n> do not have any empty (or blank) line, that appear either at the end\n> of the input, or immediately before three-dash lines\", to allow\n>\n>     commit title\n>\n>     explanation of the change\n>\n>     Signed-off-by: Some Body <some@body.xz>\n>     [some other things]\n>     Acked-by: Some Other Person <some@other.xz>\n>\n>     ---\n>      additional comment\n>\n> which (unfortunately) is a rather common pattern for people who plan\n> to send the commit over e-mail.\n>\n> If we add a new field \"const char *beginning_of_tail_garbage\" next\n> to \"end_of_message_proper\" that points at the blank line before the\n> three-dash line in the above example, the parser should be able to\n> break such an input into a parsed form, allow the trailer[] array to\n> be manipulated and reproduce a commit log message.\n\nHow important is this feature? It doesn't seem too difficult to add, \nalthough it does break compatibility (in particular, \"--signoff\" must \nnow be documented as \"after the last trailer\" instead of \"at the end of \nthe commit message\").\n"},{"id":"303434","messageId":"xmqqint6o7sl.fsf@gitster.mtv.corp.google.com","threadId":"44188","inReplyTo":"a7304731-fa05-92c2-6139-ecd1e6adcefd@google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-05T20:33:14Z","receivedAt":"2016-10-05T20:33:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Sounds reasonable to me. Would the \"[\" be a bit of overspecification,\n> though, since Git doesn't produce it? Also, identifying it as a\n> garbage line probably wouldn't change any behavior - in the Linux\n> kernel examples, it is used to show what happened in between\n> sign-offs, so there will always be one \"Signed-off-by:\" at the top.\n\nGood thinking.  As \"interpret trailers\" cannot locate such a line to\nmanipulate (as it lacks <token>), we can simply treat it as a\ngarbage line.\n\n>>     struct {\n>> \tconst char *whole;\n>> \tconst char *end_of_message_proper;\n>> \tstruct {\n>> \t\tconst char *token;\n>> \t\tconst char *contents;\n>> \t} *trailer;\n>> \tint alloc_trailers, nr_trailers;\n>>     };\n>>\n>> where\n>>\n>>  - whole points at the first byte of the input, i.e. the beginning\n>>    of the commit message buffer.\n>>\n>>  - end-of-message-proper points at the first byte of the trailer\n>>    block into the buffer at \"whole\".\n>>\n>>  - token is a canonical header name for easy comparison for\n>>    interpret-trailers (you can use NULL for garbage lines, and made\n>>    up token like \"[bracket]\" and \"(cherrypick)\" that would not clash\n>>    with real tokens like \"Signed-off-by\").\n>>\n>>  - contents is the bytes on the logical line, including the header\n>>    part\n>>\n>> E.g. an element in trailer[] array may say\n>>\n>>     {\n>> \t.token = \"Signed-off-by\",\n>>         .contents = \"Signed-Off-By: Some Body <some@body.xz>\\n\",\n>>     }\n>\n> I get the impression from the rest of your e-mail that no strings are\n> meant to be copied - is that true? (That sounds like a good idea to\n> me.)\n\nI was envisioning that \"whole\", \"end-of-message\" can point into the\ninput buffer, while trailer[].contents may have to be copied, if\nonly to make it to a NUL-terminated string.  But I am fine with\n<ptr,len> or <begin,end> pair to avoid copying .contents if that is\ndesired.  You'd need to worry about differentiating .contents that\nneed to be freed after \"interpret trailers\" inserted a new entry or\nreplaced the contents, though.\n"},{"id":"303520","messageId":"xmqq7f9ljn5o.fsf@gitster.mtv.corp.google.com","threadId":"44188","inReplyTo":"b0e43079-498a-469b-c4b6-f4fc2b8e2aa4@google.com","subject":"Re: [RFC/PATCH 0/2] place cherry pick line below commit title","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-06T19:24:51Z","receivedAt":"2016-10-06T19:24:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> How important is this feature? It doesn't seem too difficult to add,\n> although it does break compatibility (in particular, \"--signoff\" must\n> now be documented as \"after the last trailer\" instead of \"at the end\n> of the commit message\").\n\nThe sign-off has always been meant to be appended at the end of the\ntrailer block, so there is no \"compatibility\" issue.  It's just how\nyou phrase the explanation of the behaviour, I would think.\n"}]}