{"thread":{"id":"41953","subject":"[PATCH 0/4] git-am: use trailers to add extra signatures","startedAt":"2016-04-07T15:23:03Z","lastAt":"2016-04-10T14:56:00Z","messageCount":23,"participants":["Michael S. Tsirkin","Christian Couder","Junio C Hamano","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"282839","messageId":"1460042563-32741-1-git-send-email-mst@redhat.com","threadId":"41953","inReplyTo":null,"subject":"[PATCH 0/4] git-am: use trailers to add extra signatures","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2016-04-07T15:23:03Z","receivedAt":"2016-04-07T15:23:03Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"I'm using git am to apply patches, and I like the ability\nto add arbitrary trailers instead of the standard Signed-off-by\none.\n\nTo this end, I have extended git am to call git interpret-trailers\ninternally. This way I can add arbitrary signatures.\n\nFor example, I have:\n[trailer \"t\"]\n key = Tested-by\n command = \"echo \\\"Michael S. Tsirkin \\\"\"\n[trailer \"r\"]\n key = Reviewed-by\n command = \"echo \\\"Michael S. Tsirkin \\\"\"\n[trailer \"a\"]\n key = Acked-by\n command = \"echo \\\"Michael S. Tsirkin \\\"\"\n[trailer \"s\"]\n key = Signed-off-by\n command = \"echo \\\"Michael S. Tsirkin \\\"\"\n\nAnd now:\n\tgit am -t t -t r -t s\nadds all of:\n\tTested-by: Michael S. Tsirkin \n\tReviewed-by: Michael S. Tsirkin \n\tSigned-off-by: Michael S. Tsirkin \n\nThis was originally suggested by Junio (a long time ago).\n\nDocumentation and tests are still TBD.\n\nMichael S. Tsirkin (4):\n builtin/interpret-trailers.c: allow -t\n builtin/interpret-trailers: suppress blank line\n builtin/am: read mailinfo from file\n builtin/am: passthrough -t and --trailer flags\n\n trailer.h | 2 +-\n builtin/am.c | 57 +++++++++++++++++++++++++++++++++++++++++++-\n builtin/interpret-trailers.c | 11 ++++++---\n trailer.c | 10 +++++---\n 4 files changed, 72 insertions(+), 8 deletions(-)\n\n-- \nMST\n"},{"id":"282840","messageId":"1460042563-32741-2-git-send-email-mst@redhat.com","threadId":"41953","inReplyTo":"1460042563-32741-1-git-send-email-mst@redhat.com","subject":"[PATCH 1/4] builtin/interpret-trailers.c: allow -t","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2016-04-07T15:23:07Z","receivedAt":"2016-04-07T15:23:07Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"Allow -t as a short-cut for --trailer.\n\nSigned-off-by: Michael S. Tsirkin \n---\n builtin/interpret-trailers.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex b99ae4b..18cf640 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -25,7 +25,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"in-place\", &in_place, N_(\"edit files in place\")),\n \t\tOPT_BOOL(0, \"trim-empty\", &trim_empty, N_(\"trim empty trailers\")),\n-\t\tOPT_STRING_LIST(0, \"trailer\", &trailers, N_(\"trailer\"),\n+\t\tOPT_STRING_LIST('t', \"trailer\", &trailers, N_(\"trailer\"),\n \t\t\t\tN_(\"trailer(s) to add\")),\n \t\tOPT_END()\n \t};\n-- \nMST\n"},{"id":"282841","messageId":"1460042563-32741-3-git-send-email-mst@redhat.com","threadId":"41953","inReplyTo":"1460042563-32741-1-git-send-email-mst@redhat.com","subject":"[PATCH 2/4] builtin/interpret-trailers: suppress blank line","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2016-04-07T15:23:10Z","receivedAt":"2016-04-07T15:23:10Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"it's sometimes useful to be able to pass output message of\ngit-mailinfo through git-interpret-trailers,\nbut that creates problems since that does not\ninclude the subject and an empty line after that,\nmaking interpret-trailers add an empty line.\n\nAdd a flag to bypass adding the blank line.\n\nSigned-off-by: Michael S. Tsirkin \n---\n trailer.h | 2 +-\n builtin/interpret-trailers.c | 9 +++++++--\n trailer.c | 10 +++++++---\n 3 files changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/trailer.h b/trailer.h\nindex 36b40b8..afcf680 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -2,6 +2,6 @@\n #define TRAILER_H\n \n void process_trailers(const char *file, int in_place, int trim_empty,\n-\t\t struct string_list *trailers);\n+\t\t int suppress_blank_line, struct string_list *trailers);\n \n #endif /* TRAILER_H */\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex 18cf640..4a92788 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -18,11 +18,14 @@ static const char * const git_interpret_trailers_usage[] = {\n \n int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n {\n+\tint suppress_blank_line = 0;\n \tint in_place = 0;\n \tint trim_empty = 0;\n \tstruct string_list trailers = STRING_LIST_INIT_DUP;\n \n \tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"suppress-blank-line\", &suppress_blank_line,\n+\t\t\t N_(\"suppress prefixing tailer(s) with a blank line \")),\n \t\tOPT_BOOL(0, \"in-place\", &in_place, N_(\"edit files in place\")),\n \t\tOPT_BOOL(0, \"trim-empty\", &trim_empty, N_(\"trim empty trailers\")),\n \t\tOPT_STRING_LIST('t', \"trailer\", &trailers, N_(\"trailer\"),\n@@ -36,11 +39,13 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n \tif (argc) {\n \t\tint i;\n \t\tfor (i = 0; i < argc; i++)\n-\t\t\tprocess_trailers(argv[i], in_place, trim_empty, &trailers);\n+\t\t\tprocess_trailers(argv[i], in_place, trim_empty,\n+\t\t\t\t\t suppress_blank_line, &trailers);\n \t} else {\n \t\tif (in_place)\n \t\t\tdie(_(\"no input file given for in-place editing\"));\n-\t\tprocess_trailers(NULL, in_place, trim_empty, &trailers);\n+\t\tprocess_trailers(NULL, in_place, trim_empty,\n+\t\t\t\t suppress_blank_line, &trailers);\n \t}\n \n \tstring_list_clear(&trailers, 0);\ndiff --git a/trailer.c b/trailer.c\nindex 8e48a5c..8e5be91 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -805,6 +805,7 @@ static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end\n \n static int process_input_file(FILE *outfile,\n \t\t\t struct strbuf **lines,\n+\t\t\t int suppress_blank_line,\n \t\t\t struct trailer_item **in_tok_first,\n \t\t\t struct trailer_item **in_tok_last)\n {\n@@ -822,7 +823,8 @@ static int process_input_file(FILE *outfile,\n \t/* Print lines before the trailers as is */\n \tprint_lines(outfile, lines, 0, trailer_start);\n \n-\tif (!has_blank_line_before(lines, trailer_start - 1))\n+\tif (!suppress_blank_line &&\n+\t !has_blank_line_before(lines, trailer_start - 1))\n \t\tfprintf(outfile, \"\\n\");\n \n \t/* Parse trailer lines */\n@@ -875,7 +877,8 @@ static FILE *create_in_place_tempfile(const char *file)\n \treturn outfile;\n }\n \n-void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)\n+void process_trailers(const char *file, int in_place, int trim_empty,\n+\t\t int suppress_blank_line, struct string_list *trailers)\n {\n \tstruct trailer_item *in_tok_first = NULL;\n \tstruct trailer_item *in_tok_last = NULL;\n@@ -894,7 +897,8 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n \t\toutfile = create_in_place_tempfile(file);\n \n \t/* Print the lines before the trailers */\n-\ttrailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);\n+\ttrailer_end = process_input_file(outfile, lines, suppress_blank_line,\n+\t\t\t\t\t &in_tok_first, &in_tok_last);\n \n \targ_tok_first = process_command_line_args(trailers);\n \n-- \nMST\n"},{"id":"282843","messageId":"1460042563-32741-4-git-send-email-mst@redhat.com","threadId":"41953","inReplyTo":"1460042563-32741-1-git-send-email-mst@redhat.com","subject":"[PATCH 3/4] builtin/am: read mailinfo from file","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2016-04-07T15:23:13Z","receivedAt":"2016-04-07T15:23:13Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"Slightly slower, but will allow easy additional processing on it.\n\nSigned-off-by: Michael S. Tsirkin \n---\n builtin/am.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex d003939..4180b04 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1246,6 +1246,7 @@ static int parse_mail(struct am_state *state, const char *mail)\n \tFILE *fp;\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct strbuf msg = STRBUF_INIT;\n+\tstruct strbuf log_msg = STRBUF_INIT;\n \tstruct strbuf author_name = STRBUF_INIT;\n \tstruct strbuf author_date = STRBUF_INIT;\n \tstruct strbuf author_email = STRBUF_INIT;\n@@ -1330,7 +1331,12 @@ static int parse_mail(struct am_state *state, const char *mail)\n \t}\n \n \tstrbuf_addstr(&msg, \"\\n\\n\");\n-\tstrbuf_addbuf(&msg, &mi.log_message);\n+\n+\tif (strbuf_read_file(&log_msg, am_path(state, \"msg\"), 0) < 0) {\n+\t\tdie_errno(_(\"could not read '%s'\"), am_path(state, \"msg\"));\n+\t}\n+\n+\tstrbuf_addbuf(&msg, &log_msg);\n \tstrbuf_stripspace(&msg, 0);\n \n \tif (state->signoff)\n@@ -1349,6 +1355,7 @@ static int parse_mail(struct am_state *state, const char *mail)\n \tstate->msg = strbuf_detach(&msg, &state->msg_len);\n \n finish:\n+\tstrbuf_release(&log_msg);\n \tstrbuf_release(&msg);\n \tstrbuf_release(&author_date);\n \tstrbuf_release(&author_email);\n-- \nMST\n"},{"id":"282842","messageId":"1460042563-32741-5-git-send-email-mst@redhat.com","threadId":"41953","inReplyTo":"1460042563-32741-1-git-send-email-mst@redhat.com","subject":"[PATCH 4/4] builtin/am: passthrough -t and --trailer flags","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2016-04-07T15:23:16Z","receivedAt":"2016-04-07T15:23:16Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"Pass -t and --trailer flags to git-reinterpret-trailers.\n\nSigned-off-by: Michael S. Tsirkin \n---\n builtin/am.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 48 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 4180b04..480c4c2 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -122,6 +122,7 @@ struct am_state {\n \tint message_id;\n \tint scissors; /* enum scissors_type */\n \tstruct argv_array git_apply_opts;\n+\tstruct argv_array git_interpret_trailers_opts;\n \tconst char *resolvemsg;\n \tint committer_date_is_author_date;\n \tint ignore_date;\n@@ -157,6 +158,8 @@ static void am_state_init(struct am_state *state, const char *dir)\n \n \tif (!git_config_get_bool(\"commit.gpgsign\", &gpgsign))\n \t\tstate->sign_commit = gpgsign ? \"\" : NULL;\n+\n+\targv_array_init(&state->git_interpret_trailers_opts);\n }\n \n /**\n@@ -170,6 +173,7 @@ static void am_state_release(struct am_state *state)\n \tfree(state->author_date);\n \tfree(state->msg);\n \targv_array_clear(&state->git_apply_opts);\n+\targv_array_clear(&state->git_interpret_trailers_opts);\n }\n \n /**\n@@ -472,6 +476,11 @@ static void am_load(struct am_state *state)\n \tif (sq_dequote_to_argv_array(sb.buf, &state->git_apply_opts) < 0)\n \t\tdie(_(\"could not parse %s\"), am_path(state, \"apply-opt\"));\n \n+\tread_state_file(&sb, state, \"interpret-trailers-opt\", 1);\n+\targv_array_clear(&state->git_interpret_trailers_opts);\n+\tif (sq_dequote_to_argv_array(sb.buf, &state->git_interpret_trailers_opts) < 0)\n+\t\tdie(_(\"could not parse %s\"), am_path(state, \"interpret-trailers-opt\"));\n+\n \tstate->rebasing = !!file_exists(am_path(state, \"rebasing\"));\n \n \tstrbuf_release(&sb);\n@@ -988,6 +997,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \tunsigned char curr_head[GIT_SHA1_RAWSZ];\n \tconst char *str;\n \tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf tsb = STRBUF_INIT;\n \n \tif (!patch_format)\n \t\tpatch_format = detect_patch_format(paths);\n@@ -1048,6 +1058,9 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \tsq_quote_argv(&sb, state->git_apply_opts.argv, 0);\n \twrite_state_text(state, \"apply-opt\", sb.buf);\n \n+\tsq_quote_argv(&tsb, state->git_interpret_trailers_opts.argv, 0);\n+\twrite_state_text(state, \"interpret-trailers-opt\", tsb.buf);\n+\n \tif (state->rebasing)\n \t\twrite_state_text(state, \"rebasing\", \"\");\n \telse\n@@ -1072,6 +1085,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \twrite_state_count(state, \"next\", state->cur);\n \twrite_state_count(state, \"last\", state->last);\n \n+\tstrbuf_release(&tsb);\n \tstrbuf_release(&sb);\n }\n \n@@ -1233,6 +1247,34 @@ static void am_append_signoff(struct am_state *state)\n }\n \n /**\n+ * Processes the supplied message file in-place with git-interpret-trailers.\n+ * Returns 0 on success, -1 otherwise.\n+ */\n+static int run_interpret_trailers(const struct am_state *state, const char *msg)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tif (!state->git_interpret_trailers_opts.argc)\n+\t\treturn 0;\n+\n+\tcp.git_cmd = 1;\n+\n+\targv_array_push(&cp.args, \"interpret-trailers\");\n+\n+\targv_array_push(&cp.args, \"--in-place\");\n+\targv_array_push(&cp.args, \"--suppress-blank-line\");\n+\n+\targv_array_pushv(&cp.args, state->git_interpret_trailers_opts.argv);\n+\n+\targv_array_push(&cp.args, msg);\n+\n+\tif (run_command(&cp))\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n+/**\n * Parses `mail` using git-mailinfo, extracting its patch and authorship info.\n * state->msg will be set to the patch message. state->author_name,\n * state->author_email and state->author_date will be set to the patch author's\n@@ -1301,6 +1343,9 @@ static int parse_mail(struct am_state *state, const char *mail)\n \tfclose(mi.input);\n \tfclose(mi.output);\n \n+\tif (run_interpret_trailers(state, am_path(state, \"msg\")) < 0)\n+\t\tdie(\"could not interpret trailers\");\n+\n \t/* Extract message and author information */\n \tfp = xfopen(am_path(state, \"info\"), \"r\");\n \twhile (!strbuf_getline_lf(&sb, fp)) {\n@@ -2299,6 +2344,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\tOPT_PASSTHRU_ARGV('p', NULL, &state.git_apply_opts, N_(\"num\"),\n \t\t\tN_(\"pass it through git-apply\"),\n \t\t\t0),\n+\t\tOPT_PASSTHRU_ARGV('t', \"trailer\", &state.git_interpret_trailers_opts, N_(\"trailer\"),\n+\t\t\tN_(\"pass it through git-interpret-trailers\"),\n+\t\t\t0),\n \t\tOPT_CALLBACK(0, \"patch-format\", &patch_format, N_(\"format\"),\n \t\t\tN_(\"format the patch(es) are in\"),\n \t\t\tparse_opt_patchformat),\n-- \nMST\n"},{"id":"282847","messageId":"CAP8UFD3+pTwD3qNv7A9Bcyprf523RDt9Hh05Pfsdj04aTCjPDw@mail.gmail.com","threadId":"41953","inReplyTo":"1460042563-32741-5-git-send-email-mst@redhat.com","subject":"Re: [PATCH 4/4] builtin/am: passthrough -t and --trailer flags","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-04-07T16:39:45Z","receivedAt":"2016-04-07T16:39:45Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Apr 7, 2016 at 11:23 AM, Michael S. Tsirkin wrote:\n> Pass -t and --trailer flags to git-reinterpret-trailers.\n\ns/git-reinterpret-trailers/git-interpret-trailers/\n\nThanks,\nChristian.\n"},{"id":"282849","messageId":"xmqqr3eh1hq6.fsf@gitster.mtv.corp.google.com","threadId":"41953","inReplyTo":"1460042563-32741-2-git-send-email-mst@redhat.com","subject":"Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-07T16:55:29Z","receivedAt":"2016-04-07T16:55:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Michael S. Tsirkin\" writes:\n\n> Allow -t as a short-cut for --trailer.\n>\n> Signed-off-by: Michael S. Tsirkin \n> ---\n\nAs I do not think interpret-trailers is meant to be end-user facing,\nI am not sure I should be interested in this step.\n\nI am in principle OK with the later step that teaches a single\nletter option to end-user facing \"git am\" that would be turned into\n\"--trailer\" when it calls out to \"interpret-trailers\" (I haven't\nchecked if 't' is a sensible choice for that single letter option,\nthough).\n\n> builtin/interpret-trailers.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n> index b99ae4b..18cf640 100644\n> --- a/builtin/interpret-trailers.c\n> +++ b/builtin/interpret-trailers.c\n> @@ -25,7 +25,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n> \tstruct option options[] = {\n> \t\tOPT_BOOL(0, \"in-place\", &in_place, N_(\"edit files in place\")),\n> \t\tOPT_BOOL(0, \"trim-empty\", &trim_empty, N_(\"trim empty trailers\")),\n> -\t\tOPT_STRING_LIST(0, \"trailer\", &trailers, N_(\"trailer\"),\n> +\t\tOPT_STRING_LIST('t', \"trailer\", &trailers, N_(\"trailer\"),\n> \t\t\t\tN_(\"trailer(s) to add\")),\n> \t\tOPT_END()\n> \t};\n"},{"id":"282850","messageId":"xmqqmvp51hhm.fsf@gitster.mtv.corp.google.com","threadId":"41953","inReplyTo":"1460042563-32741-3-git-send-email-mst@redhat.com","subject":"Re: [PATCH 2/4] builtin/interpret-trailers: suppress blank line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-07T17:00:37Z","receivedAt":"2016-04-07T17:00:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Michael S. Tsirkin\" writes:\n\n> it's sometimes useful to be able to pass output message of\n> git-mailinfo through git-interpret-trailers,\n> but that creates problems since that does not\n> include the subject and an empty line after that,\n> making interpret-trailers add an empty line.\n>\n> Add a flag to bypass adding the blank line.\n\nI think I understand what you are trying to do, but using output\nthat comes from 'mailinfo' alone as the input to anything (including\ninterpret-trailers) does not make much sense.\n\nIf you use the mailinfo output in the way it is expected to be used,\ni.e. take the subject from the \"info\" that goes to its standard\noutput and append the \"msg\" with a blank between them, and feed the\nresult to interpret-trailers, do you still need this step in your\nseries?\n\n>\n> Signed-off-by: Michael S. Tsirkin \n> ---\n> trailer.h | 2 +-\n> builtin/interpret-trailers.c | 9 +++++++--\n> trailer.c | 10 +++++++---\n> 3 files changed, 15 insertions(+), 6 deletions(-)\n>\n> diff --git a/trailer.h b/trailer.h\n> index 36b40b8..afcf680 100644\n> --- a/trailer.h\n> +++ b/trailer.h\n> @@ -2,6 +2,6 @@\n> #define TRAILER_H\n> \n> void process_trailers(const char *file, int in_place, int trim_empty,\n> -\t\t struct string_list *trailers);\n> +\t\t int suppress_blank_line, struct string_list *trailers);\n> \n> #endif /* TRAILER_H */\n> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n> index 18cf640..4a92788 100644\n> --- a/builtin/interpret-trailers.c\n> +++ b/builtin/interpret-trailers.c\n> @@ -18,11 +18,14 @@ static const char * const git_interpret_trailers_usage[] = {\n> \n> int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n> {\n> +\tint suppress_blank_line = 0;\n> \tint in_place = 0;\n> \tint trim_empty = 0;\n> \tstruct string_list trailers = STRING_LIST_INIT_DUP;\n> \n> \tstruct option options[] = {\n> +\t\tOPT_BOOL(0, \"suppress-blank-line\", &suppress_blank_line,\n> +\t\t\t N_(\"suppress prefixing tailer(s) with a blank line \")),\n> \t\tOPT_BOOL(0, \"in-place\", &in_place, N_(\"edit files in place\")),\n> \t\tOPT_BOOL(0, \"trim-empty\", &trim_empty, N_(\"trim empty trailers\")),\n> \t\tOPT_STRING_LIST('t', \"trailer\", &trailers, N_(\"trailer\"),\n> @@ -36,11 +39,13 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n> \tif (argc) {\n> \t\tint i;\n> \t\tfor (i = 0; i < argc; i++)\n> -\t\t\tprocess_trailers(argv[i], in_place, trim_empty, &trailers);\n> +\t\t\tprocess_trailers(argv[i], in_place, trim_empty,\n> +\t\t\t\t\t suppress_blank_line, &trailers);\n> \t} else {\n> \t\tif (in_place)\n> \t\t\tdie(_(\"no input file given for in-place editing\"));\n> -\t\tprocess_trailers(NULL, in_place, trim_empty, &trailers);\n> +\t\tprocess_trailers(NULL, in_place, trim_empty,\n> +\t\t\t\t suppress_blank_line, &trailers);\n> \t}\n> \n> \tstring_list_clear(&trailers, 0);\n> diff --git a/trailer.c b/trailer.c\n> index 8e48a5c..8e5be91 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -805,6 +805,7 @@ static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end\n> \n> static int process_input_file(FILE *outfile,\n> \t\t\t struct strbuf **lines,\n> +\t\t\t int suppress_blank_line,\n> \t\t\t struct trailer_item **in_tok_first,\n> \t\t\t struct trailer_item **in_tok_last)\n> {\n> @@ -822,7 +823,8 @@ static int process_input_file(FILE *outfile,\n> \t/* Print lines before the trailers as is */\n> \tprint_lines(outfile, lines, 0, trailer_start);\n> \n> -\tif (!has_blank_line_before(lines, trailer_start - 1))\n> +\tif (!suppress_blank_line &&\n> +\t !has_blank_line_before(lines, trailer_start - 1))\n> \t\tfprintf(outfile, \"\\n\");\n> \n> \t/* Parse trailer lines */\n> @@ -875,7 +877,8 @@ static FILE *create_in_place_tempfile(const char *file)\n> \treturn outfile;\n> }\n> \n> -void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)\n> +void process_trailers(const char *file, int in_place, int trim_empty,\n> +\t\t int suppress_blank_line, struct string_list *trailers)\n> {\n> \tstruct trailer_item *in_tok_first = NULL;\n> \tstruct trailer_item *in_tok_last = NULL;\n> @@ -894,7 +897,8 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n> \t\toutfile = create_in_place_tempfile(file);\n> \n> \t/* Print the lines before the trailers */\n> -\ttrailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);\n> +\ttrailer_end = process_input_file(outfile, lines, suppress_blank_line,\n> +\t\t\t\t\t &in_tok_first, &in_tok_last);\n> \n> \targ_tok_first = process_command_line_args(trailers);\n"},{"id":"282851","messageId":"xmqqinzt1h4a.fsf@gitster.mtv.corp.google.com","threadId":"41953","inReplyTo":"1460042563-32741-4-git-send-email-mst@redhat.com","subject":"Re: [PATCH 3/4] builtin/am: read mailinfo from file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-07T17:08:37Z","receivedAt":"2016-04-07T17:08:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Michael S. Tsirkin\" writes:\n\n> Slightly slower, but will allow easy additional processing on it.\n>\n> Signed-off-by: Michael S. Tsirkin \n> ---\n\nI haven't read 4/4 yet, but can guess from what this patch does that\nthe next step would let others futz with the contents of the message\nthat is on disk (i.e. what mailinfo() wrote out, which is identical\nto what we have in mi.log_message at this point of the codeflow)\nbefore you do the new strbuf_read_file().\n\nIt probably is better to do this as part of 4/4; it is easier to\nunderstand why this is a good and necessary thing to do. An obvious\nimprovement is to omit this extra \"read back from the filesystem\"\nwhen we won't be making any interpret-trailer calls (i.e. no -t\noption from the command line), but if we stop at this step 3/4, then\nwe'd end up wasting cycles without having any benefit.\n\n> builtin/am.c | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/am.c b/builtin/am.c\n> index d003939..4180b04 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1246,6 +1246,7 @@ static int parse_mail(struct am_state *state, const char *mail)\n> \tFILE *fp;\n> \tstruct strbuf sb = STRBUF_INIT;\n> \tstruct strbuf msg = STRBUF_INIT;\n> +\tstruct strbuf log_msg = STRBUF_INIT;\n> \tstruct strbuf author_name = STRBUF_INIT;\n> \tstruct strbuf author_date = STRBUF_INIT;\n> \tstruct strbuf author_email = STRBUF_INIT;\n> @@ -1330,7 +1331,12 @@ static int parse_mail(struct am_state *state, const char *mail)\n> \t}\n> \n> \tstrbuf_addstr(&msg, \"\\n\\n\");\n> -\tstrbuf_addbuf(&msg, &mi.log_message);\n> +\n> +\tif (strbuf_read_file(&log_msg, am_path(state, \"msg\"), 0) < 0) {\n> +\t\tdie_errno(_(\"could not read '%s'\"), am_path(state, \"msg\"));\n> +\t}\n\nI do not think these {} serve any purpose; drop them?\n\n> +\n> +\tstrbuf_addbuf(&msg, &log_msg);\n> \tstrbuf_stripspace(&msg, 0);\n> \n> \tif (state->signoff)\n> @@ -1349,6 +1355,7 @@ static int parse_mail(struct am_state *state, const char *mail)\n> \tstate->msg = strbuf_detach(&msg, &state->msg_len);\n> \n> finish:\n> +\tstrbuf_release(&log_msg);\n> \tstrbuf_release(&msg);\n> \tstrbuf_release(&author_date);\n> \tstrbuf_release(&author_email);\n"},{"id":"282852","messageId":"20160407201323-mutt-send-email-mst@redhat.com","threadId":"41953","inReplyTo":"xmqqinzt1h4a.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/4] builtin/am: read mailinfo from file","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2016-04-07T17:15:51Z","receivedAt":"2016-04-07T17:15:51Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Thu, Apr 07, 2016 at 10:08:37AM -0700, Junio C Hamano wrote:\n> \"Michael S. Tsirkin\" writes:\n> \n> > Slightly slower, but will allow easy additional processing on it.\n> >\n> > Signed-off-by: Michael S. Tsirkin \n> > ---\n> \n> I haven't read 4/4 yet, but can guess from what this patch does that\n> the next step would let others futz with the contents of the message\n> that is on disk (i.e. what mailinfo() wrote out, which is identical\n> to what we have in mi.log_message at this point of the codeflow)\n> before you do the new strbuf_read_file().\n> \n> It probably is better to do this as part of 4/4; it is easier to\n> understand why this is a good and necessary thing to do. An obvious\n> improvement is to omit this extra \"read back from the filesystem\"\n> when we won't be making any interpret-trailer calls (i.e. no -t\n> option from the command line), but if we stop at this step 3/4, then\n> we'd end up wasting cycles without having any benefit.\n\nHmm - splitting it out was easy for development since I could verify all\ntests pass. But if you do want the optimization, then sure, I'll have\nto squash it in.\n\n> > builtin/am.c | 9 ++++++++-\n> > 1 file changed, 8 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/am.c b/builtin/am.c\n> > index d003939..4180b04 100644\n> > --- a/builtin/am.c\n> > +++ b/builtin/am.c\n> > @@ -1246,6 +1246,7 @@ static int parse_mail(struct am_state *state, const char *mail)\n> > \tFILE *fp;\n> > \tstruct strbuf sb = STRBUF_INIT;\n> > \tstruct strbuf msg = STRBUF_INIT;\n> > +\tstruct strbuf log_msg = STRBUF_INIT;\n> > \tstruct strbuf author_name = STRBUF_INIT;\n> > \tstruct strbuf author_date = STRBUF_INIT;\n> > \tstruct strbuf author_email = STRBUF_INIT;\n> > @@ -1330,7 +1331,12 @@ static int parse_mail(struct am_state *state, const char *mail)\n> > \t}\n> > \n> > \tstrbuf_addstr(&msg, \"\\n\\n\");\n> > -\tstrbuf_addbuf(&msg, &mi.log_message);\n> > +\n> > +\tif (strbuf_read_file(&log_msg, am_path(state, \"msg\"), 0) < 0) {\n> > +\t\tdie_errno(_(\"could not read '%s'\"), am_path(state, \"msg\"));\n> > +\t}\n> \n> I do not think these {} serve any purpose; drop them?\n> \n> > +\n> > +\tstrbuf_addbuf(&msg, &log_msg);\n> > \tstrbuf_stripspace(&msg, 0);\n> > \n> > \tif (state->signoff)\n> > @@ -1349,6 +1355,7 @@ static int parse_mail(struct am_state *state, const char *mail)\n> > \tstate->msg = strbuf_detach(&msg, &state->msg_len);\n> > \n> > finish:\n> > +\tstrbuf_release(&log_msg);\n> > \tstrbuf_release(&msg);\n> > \tstrbuf_release(&author_date);\n> > \tstrbuf_release(&author_email);\n"},{"id":"282853","messageId":"vpqtwjduymh.fsf@anie.imag.fr","threadId":"41953","inReplyTo":"xmqqr3eh1hq6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-04-07T17:17:42Z","receivedAt":"2016-04-07T17:17:42Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> \"Michael S. Tsirkin\" writes:\n>\n>> Allow -t as a short-cut for --trailer.\n>>\n>> Signed-off-by: Michael S. Tsirkin \n>> ---\n>\n> As I do not think interpret-trailers is meant to be end-user facing,\n> I am not sure I should be interested in this step.\n>\n> I am in principle OK with the later step that teaches a single\n> letter option to end-user facing \"git am\" that would be turned into\n> \"--trailer\" when it calls out to \"interpret-trailers\" (I haven't\n> checked if 't' is a sensible choice for that single letter option,\n> though).\n\nIf 'am' has -t == --trailer, I think it makes sense to have the same\nshortcut in interpret-trailers for consistency.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"282854","messageId":"xmqqegah1gis.fsf@gitster.mtv.corp.google.com","threadId":"41953","inReplyTo":"xmqqmvp51hhm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/4] builtin/interpret-trailers: suppress blank line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-07T17:21:31Z","receivedAt":"2016-04-07T17:21:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Michael S. Tsirkin\" writes:\n>\n>> it's sometimes useful to be able to pass output message of\n>> git-mailinfo through git-interpret-trailers,\n>> but that creates problems since that does not\n>> include the subject and an empty line after that,\n>> making interpret-trailers add an empty line.\n>>\n>> Add a flag to bypass adding the blank line.\n>\n> I think I understand what you are trying to do, but using output\n> that comes from 'mailinfo' alone as the input to anything (including\n> interpret-trailers) does not make much sense.\n>\n> If you use the mailinfo output in the way it is expected to be used,\n> i.e. take the subject from the \"info\" that goes to its standard\n> output and append the \"msg\" with a blank between them, and feed the\n> result to interpret-trailers, do you still need this step in your\n> series?\n\nOK, after reading 3/4 and guessing that you hand \"msg\" to\ninterpret-trailers to let it munge its contents, it makes sense to\nallow us to tell interpret-trailers that you are feeding only the\nbody of the message without the title and the blank line before it.\n\n\"suppress blank line\" is a terrible title for that new feature,\nthough. Perhaps \"--body-only\"?\n\nThe difference in behaviour in two modes (i.e. with title and\nwithout title) is that when the input does not have any blank line\nin it, the normal mode considers that there is no body (i.e. only\ntitle exists in the input) hence there is no existing trailer lines,\nand new trailer lines need to be added after adding blank, while the\nnew \"body only\" mode considers that there is only one paragraph in\nthe body, hence it may be the existing trailer block without any\nmessage (in which case that is the block new trailer lines are to be\nadded to or existing ones to be removed from), or there is no\ntrailer block but one paragraph of the message (in which case you\nwould do the \"add blank and append new trailer lines\" thing).\n\nI wrote the above down, hoping that it would give you a hint to\nbetter explain what this patch aims to do, so please feel free to\nfurther rephrase (or steal outright from) it when you reroll the\nseries.\n\nThanks.\n"},{"id":"282855","messageId":"20160407201853-mutt-send-email-mst@redhat.com","threadId":"41953","inReplyTo":"xmqqmvp51hhm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/4] builtin/interpret-trailers: suppress blank line","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2016-04-07T17:21:49Z","receivedAt":"2016-04-07T17:21:49Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Thu, Apr 07, 2016 at 10:00:37AM -0700, Junio C Hamano wrote:\n> \"Michael S. Tsirkin\" writes:\n> \n> > it's sometimes useful to be able to pass output message of\n> > git-mailinfo through git-interpret-trailers,\n> > but that creates problems since that does not\n> > include the subject and an empty line after that,\n> > making interpret-trailers add an empty line.\n> >\n> > Add a flag to bypass adding the blank line.\n> \n> I think I understand what you are trying to do, but using output\n> that comes from 'mailinfo' alone as the input to anything (including\n> interpret-trailers) does not make much sense.\n> \n> If you use the mailinfo output in the way it is expected to be used,\n> i.e. take the subject from the \"info\" that goes to its standard\n> output and append the \"msg\" with a blank between them, and feed the\n> result to interpret-trailers, do you still need this step in your\n> series?\n\nNo - but then I will need to re-run mailinfo to parse the result,\nwill I not?\n\nAnd unfortunately it appears that interpret-trailers can't\nhandle arbitrary mail - it wants data from commit.\n\n> >\n> > Signed-off-by: Michael S. Tsirkin \n> > ---\n> > trailer.h | 2 +-\n> > builtin/interpret-trailers.c | 9 +++++++--\n> > trailer.c | 10 +++++++---\n> > 3 files changed, 15 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/trailer.h b/trailer.h\n> > index 36b40b8..afcf680 100644\n> > --- a/trailer.h\n> > +++ b/trailer.h\n> > @@ -2,6 +2,6 @@\n> > #define TRAILER_H\n> > \n> > void process_trailers(const char *file, int in_place, int trim_empty,\n> > -\t\t struct string_list *trailers);\n> > +\t\t int suppress_blank_line, struct string_list *trailers);\n> > \n> > #endif /* TRAILER_H */\n> > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n> > index 18cf640..4a92788 100644\n> > --- a/builtin/interpret-trailers.c\n> > +++ b/builtin/interpret-trailers.c\n> > @@ -18,11 +18,14 @@ static const char * const git_interpret_trailers_usage[] = {\n> > \n> > int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n> > {\n> > +\tint suppress_blank_line = 0;\n> > \tint in_place = 0;\n> > \tint trim_empty = 0;\n> > \tstruct string_list trailers = STRING_LIST_INIT_DUP;\n> > \n> > \tstruct option options[] = {\n> > +\t\tOPT_BOOL(0, \"suppress-blank-line\", &suppress_blank_line,\n> > +\t\t\t N_(\"suppress prefixing tailer(s) with a blank line \")),\n> > \t\tOPT_BOOL(0, \"in-place\", &in_place, N_(\"edit files in place\")),\n> > \t\tOPT_BOOL(0, \"trim-empty\", &trim_empty, N_(\"trim empty trailers\")),\n> > \t\tOPT_STRING_LIST('t', \"trailer\", &trailers, N_(\"trailer\"),\n> > @@ -36,11 +39,13 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n> > \tif (argc) {\n> > \t\tint i;\n> > \t\tfor (i = 0; i < argc; i++)\n> > -\t\t\tprocess_trailers(argv[i], in_place, trim_empty, &trailers);\n> > +\t\t\tprocess_trailers(argv[i], in_place, trim_empty,\n> > +\t\t\t\t\t suppress_blank_line, &trailers);\n> > \t} else {\n> > \t\tif (in_place)\n> > \t\t\tdie(_(\"no input file given for in-place editing\"));\n> > -\t\tprocess_trailers(NULL, in_place, trim_empty, &trailers);\n> > +\t\tprocess_trailers(NULL, in_place, trim_empty,\n> > +\t\t\t\t suppress_blank_line, &trailers);\n> > \t}\n> > \n> > \tstring_list_clear(&trailers, 0);\n> > diff --git a/trailer.c b/trailer.c\n> > index 8e48a5c..8e5be91 100644\n> > --- a/trailer.c\n> > +++ b/trailer.c\n> > @@ -805,6 +805,7 @@ static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end\n> > \n> > static int process_input_file(FILE *outfile,\n> > \t\t\t struct strbuf **lines,\n> > +\t\t\t int suppress_blank_line,\n> > \t\t\t struct trailer_item **in_tok_first,\n> > \t\t\t struct trailer_item **in_tok_last)\n> > {\n> > @@ -822,7 +823,8 @@ static int process_input_file(FILE *outfile,\n> > \t/* Print lines before the trailers as is */\n> > \tprint_lines(outfile, lines, 0, trailer_start);\n> > \n> > -\tif (!has_blank_line_before(lines, trailer_start - 1))\n> > +\tif (!suppress_blank_line &&\n> > +\t !has_blank_line_before(lines, trailer_start - 1))\n> > \t\tfprintf(outfile, \"\\n\");\n> > \n> > \t/* Parse trailer lines */\n> > @@ -875,7 +877,8 @@ static FILE *create_in_place_tempfile(const char *file)\n> > \treturn outfile;\n> > }\n> > \n> > -void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)\n> > +void process_trailers(const char *file, int in_place, int trim_empty,\n> > +\t\t int suppress_blank_line, struct string_list *trailers)\n> > {\n> > \tstruct trailer_item *in_tok_first = NULL;\n> > \tstruct trailer_item *in_tok_last = NULL;\n> > @@ -894,7 +897,8 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str\n> > \t\toutfile = create_in_place_tempfile(file);\n> > \n> > \t/* Print the lines before the trailers */\n> > -\ttrailer_end = process_input_file(outfile, lines, &in_tok_first, &in_tok_last);\n> > +\ttrailer_end = process_input_file(outfile, lines, suppress_blank_line,\n> > +\t\t\t\t\t &in_tok_first, &in_tok_last);\n> > \n> > \targ_tok_first = process_command_line_args(trailers);\n"},{"id":"282856","messageId":"xmqqa8l51gae.fsf@gitster.mtv.corp.google.com","threadId":"41953","inReplyTo":"vpqtwjduymh.fsf@anie.imag.fr","subject":"Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-07T17:26:33Z","receivedAt":"2016-04-07T17:26:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n>> I am in principle OK with the later step that teaches a single\n>> letter option to end-user facing \"git am\" that would be turned into\n>> \"--trailer\" when it calls out to \"interpret-trailers\" (I haven't\n>> checked if 't' is a sensible choice for that single letter option,\n>> though).\n>\n> If 'am' has -t == --trailer, I think it makes sense to have the same\n> shortcut in interpret-trailers for consistency.\n\nIt is the other way around. \"git am\" may be OK with \"-t\" (or it may\nnot--I do not know yet), but other commands that are currently\nunaware of \"interpret-trailers\" (cherry-pick, revert, etc.) may have\nbetter uses for a short-and-sweet 't'.\n\nIn the ideal future, \"interpret-trailers\" should not have to exist\nin the end-users' vocabulary, as all the front-line end-user facing\nprograms would be aware of it. But we are not there.\n\nLetting it reserve a short-and-sweet 't' that allows it to dictate\nthat its callers must have the same 't' is tail wagging the dog that\nI want to avoid.\n"},{"id":"282857","messageId":"20160407202631-mutt-send-email-mst@redhat.com","threadId":"41953","inReplyTo":"xmqqr3eh1hq6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2016-04-07T17:28:32Z","receivedAt":"2016-04-07T17:28:32Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Thu, Apr 07, 2016 at 09:55:29AM -0700, Junio C Hamano wrote:\n> \"Michael S. Tsirkin\" writes:\n> \n> > Allow -t as a short-cut for --trailer.\n> >\n> > Signed-off-by: Michael S. Tsirkin \n> > ---\n> \n> As I do not think interpret-trailers is meant to be end-user facing,\n> I am not sure I should be interested in this step.\n> \n> I am in principle OK with the later step that teaches a single\n> letter option to end-user facing \"git am\" that would be turned into\n> \"--trailer\" when it calls out to \"interpret-trailers\" (I haven't\n> checked if 't' is a sensible choice for that single letter option,\n> though).\n\nDoes OPT_PASSTHRU_ARGV handle this transformation for me?\n\n> > builtin/interpret-trailers.c | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n> > index b99ae4b..18cf640 100644\n> > --- a/builtin/interpret-trailers.c\n> > +++ b/builtin/interpret-trailers.c\n> > @@ -25,7 +25,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n> > \tstruct option options[] = {\n> > \t\tOPT_BOOL(0, \"in-place\", &in_place, N_(\"edit files in place\")),\n> > \t\tOPT_BOOL(0, \"trim-empty\", &trim_empty, N_(\"trim empty trailers\")),\n> > -\t\tOPT_STRING_LIST(0, \"trailer\", &trailers, N_(\"trailer\"),\n> > +\t\tOPT_STRING_LIST('t', \"trailer\", &trailers, N_(\"trailer\"),\n> > \t\t\t\tN_(\"trailer(s) to add\")),\n> > \t\tOPT_END()\n> > \t};\n"},{"id":"282858","messageId":"xmqq60vt1g4l.fsf@gitster.mtv.corp.google.com","threadId":"41953","inReplyTo":"20160407202631-mutt-send-email-mst@redhat.com","subject":"Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-07T17:30:02Z","receivedAt":"2016-04-07T17:30:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Michael S. Tsirkin\" writes:\n\n> On Thu, Apr 07, 2016 at 09:55:29AM -0700, Junio C Hamano wrote:\n>> \"Michael S. Tsirkin\" writes:\n>> \n>> > Allow -t as a short-cut for --trailer.\n>> >\n>> > Signed-off-by: Michael S. Tsirkin \n>> > ---\n>> \n>> As I do not think interpret-trailers is meant to be end-user facing,\n>> I am not sure I should be interested in this step.\n>> \n>> I am in principle OK with the later step that teaches a single\n>> letter option to end-user facing \"git am\" that would be turned into\n>> \"--trailer\" when it calls out to \"interpret-trailers\" (I haven't\n>> checked if 't' is a sensible choice for that single letter option,\n>> though).\n>\n> Does OPT_PASSTHRU_ARGV handle this transformation for me?\n\nAs I wrote in my response to Matthieu, PASSTHRU_ARGV is one thing I\nspecifically do not want to see used in this codepath.\n"},{"id":"282859","messageId":"20160407202938-mutt-send-email-mst@redhat.com","threadId":"41953","inReplyTo":"xmqqa8l51gae.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2016-04-07T17:30:34Z","receivedAt":"2016-04-07T17:30:34Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Thu, Apr 07, 2016 at 10:26:33AM -0700, Junio C Hamano wrote:\n> Matthieu Moy writes:\n> \n> >> I am in principle OK with the later step that teaches a single\n> >> letter option to end-user facing \"git am\" that would be turned into\n> >> \"--trailer\" when it calls out to \"interpret-trailers\" (I haven't\n> >> checked if 't' is a sensible choice for that single letter option,\n> >> though).\n> >\n> > If 'am' has -t == --trailer, I think it makes sense to have the same\n> > shortcut in interpret-trailers for consistency.\n> \n> It is the other way around. \"git am\" may be OK with \"-t\" (or it may\n> not--I do not know yet), but other commands that are currently\n> unaware of \"interpret-trailers\" (cherry-pick, revert, etc.) may have\n> better uses for a short-and-sweet 't'.\n> \n> In the ideal future, \"interpret-trailers\" should not have to exist\n> in the end-users' vocabulary, as all the front-line end-user facing\n> programs would be aware of it. But we are not there.\n> \n> Letting it reserve a short-and-sweet 't' that allows it to dictate\n> that its callers must have the same 't' is tail wagging the dog that\n> I want to avoid.\n\nIt's mostly a short-cut I took by copying calls to applypatch.\nAre there examples of other commands doing such transformations\non the fly?\n\n-- \nMST\n"},{"id":"282860","messageId":"xmqq1t6h1fwk.fsf@gitster.mtv.corp.google.com","threadId":"41953","inReplyTo":"20160407201853-mutt-send-email-mst@redhat.com","subject":"Re: [PATCH 2/4] builtin/interpret-trailers: suppress blank line","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-07T17:34:51Z","receivedAt":"2016-04-07T17:34:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Michael S. Tsirkin\" writes:\n\n> No - but then I will need to re-run mailinfo to parse the result,\n> will I not?\n\nBy the way, I suspect (if Christian did his implementation right\nwhen he did interpret-trailers) all these points may become moot.\n\nI haven't re-reviewed what is in interpret-trailers, but the vision\nhas been that its internal workings should be callable directly into\ninstead of running it via run_commands() interface passing the data\nvia on-disk file. In the codepath you touch in 3/4 and 4/4, you\nalready have not just mi.log_message but msg that has the whole\npayload to create a commit object out of already, so shouldn't it be\njust the matter of passing to some API function\nthat was prepared to implement interpret-trailers?\n"},{"id":"282861","messageId":"vpqoa9ltj8g.fsf@anie.imag.fr","threadId":"41953","inReplyTo":"1460042563-32741-3-git-send-email-mst@redhat.com","subject":"Re: [PATCH 2/4] builtin/interpret-trailers: suppress blank line","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-04-07T17:35:27Z","receivedAt":"2016-04-07T17:35:27Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"\"Michael S. Tsirkin\" writes:\n\n> it's sometimes useful to be able to pass output message of\n> git-mailinfo through git-interpret-trailers,\n> but that creates problems since that does not\n> include the subject and an empty line after that,\n> making interpret-trailers add an empty line.\n\nNit: we usually wrap our text around 72 columns in the commit message.\nYours is wrapped weirdly.\n\n> Add a flag to bypass adding the blank line.\n>\n> Signed-off-by: Michael S. Tsirkin \n> ---\n> trailer.h | 2 +-\n> builtin/interpret-trailers.c | 9 +++++++--\n> trailer.c | 10 +++++++---\n> 3 files changed, 15 insertions(+), 6 deletions(-)\n\nYou'd definitely need some tests and documentation if you introduce a\nnew option.\n\nNo time for a real review, sorry.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"282862","messageId":"vpqinzttj7c.fsf@anie.imag.fr","threadId":"41953","inReplyTo":"1460042563-32741-4-git-send-email-mst@redhat.com","subject":"Re: [PATCH 3/4] builtin/am: read mailinfo from file","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-04-07T17:36:07Z","receivedAt":"2016-04-07T17:36:07Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"\"Michael S. Tsirkin\" writes:\n\n> -\tstrbuf_addbuf(&msg, &mi.log_message);\n> +\n> +\tif (strbuf_read_file(&log_msg, am_path(state, \"msg\"), 0) < 0) {\n> +\t\tdie_errno(_(\"could not read '%s'\"), am_path(state, \"msg\"));\n> +\t}\n\nStyle: we omit the braces where not needed.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"282863","messageId":"20160407205144-mutt-send-email-mst@redhat.com","threadId":"41953","inReplyTo":"xmqq60vt1g4l.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2016-04-07T17:52:53Z","receivedAt":"2016-04-07T17:52:53Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Thu, Apr 07, 2016 at 10:30:02AM -0700, Junio C Hamano wrote:\n> \"Michael S. Tsirkin\" writes:\n> \n> > On Thu, Apr 07, 2016 at 09:55:29AM -0700, Junio C Hamano wrote:\n> >> \"Michael S. Tsirkin\" writes:\n> >> \n> >> > Allow -t as a short-cut for --trailer.\n> >> >\n> >> > Signed-off-by: Michael S. Tsirkin \n> >> > ---\n> >> \n> >> As I do not think interpret-trailers is meant to be end-user facing,\n> >> I am not sure I should be interested in this step.\n> >> \n> >> I am in principle OK with the later step that teaches a single\n> >> letter option to end-user facing \"git am\" that would be turned into\n> >> \"--trailer\" when it calls out to \"interpret-trailers\" (I haven't\n> >> checked if 't' is a sensible choice for that single letter option,\n> >> though).\n> >\n> > Does OPT_PASSTHRU_ARGV handle this transformation for me?\n> \n> As I wrote in my response to Matthieu, PASSTHRU_ARGV is one thing I\n> specifically do not want to see used in this codepath.\n\nIt sounds like a general kind of thing, does it not?\nAren't there other cases where a short option needs to be\nconverted to a long one?\n\n-- \nMST\n"},{"id":"282864","messageId":"xmqqwpo9z4j1.fsf@gitster.mtv.corp.google.com","threadId":"41953","inReplyTo":"20160407205144-mutt-send-email-mst@redhat.com","subject":"Re: [PATCH 1/4] builtin/interpret-trailers.c: allow -t","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-07T17:56:34Z","receivedAt":"2016-04-07T17:56:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Michael S. Tsirkin\" writes:\n\n> Aren't there other cases where a short option needs to be\n> converted to a long one?\n\nAs I already said, making internal call to libified part (perhaps in\ntrailer.c) would make this part of conversation a moot point, but in\ngeneral you can find\n\n\targv_push(&child.args, \"cmd2\");\n\tif (... some condition that involves variables parsed out ...\n ... by parse_options() of implementation of cmd1 ...)\n\t\targv_push(&child.args, \"--option-for-cmd2\");\n\t...\n run_command(&child);\n\nin implementation of cmd1 that calls out to cmd2.\n"},{"id":"283098","messageId":"20160410175217-mutt-send-email-mst@redhat.com","threadId":"41953","inReplyTo":"xmqq1t6h1fwk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/4] builtin/interpret-trailers: suppress blank line","fromName":"Michael S. Tsirkin","fromEmail":"mst@redhat.com","sentAt":"2016-04-10T14:56:00Z","receivedAt":"2016-04-10T14:56:00Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"On Thu, Apr 07, 2016 at 10:34:51AM -0700, Junio C Hamano wrote:\n> \"Michael S. Tsirkin\" writes:\n> \n> > No - but then I will need to re-run mailinfo to parse the result,\n> > will I not?\n> \n> By the way, I suspect (if Christian did his implementation right\n> when he did interpret-trailers) all these points may become moot.\n> \n> I haven't re-reviewed what is in interpret-trailers, but the vision\n> has been that its internal workings should be callable directly into\n> instead of running it via run_commands() interface passing the data\n> via on-disk file. In the codepath you touch in 3/4 and 4/4, you\n> already have not just mi.log_message but msg that has the whole\n> payload to create a commit object out of already, so shouldn't it be\n> just the matter of passing to some API function\n> that was prepared to implement interpret-trailers?\n\nThat's certainly possible, though it will need a rework\nof the internal API: we currently have:\n\nvoid process_trailers(const char *file, int in_place, int trim_empty,\n int suppress_blank_line, struct string_list *trailers)\n{\n struct trailer_item *in_tok_first = NULL;\n struct trailer_item *in_tok_last = NULL;\n struct trailer_item *arg_tok_first;\n struct strbuf **lines;\n int trailer_end;\n FILE *outfile = stdout;\n\n /* Default config must be setup first */\n git_config(git_trailer_default_config, NULL);\n git_config(git_trailer_config, NULL);\n\n lines = read_input_file(file);\n\nSo process_trailers can be changed to get struct strbuf ** instead.\n\nBut it seems that the output would have to go into a temporary file\nanyway, unless trailer.c is completely rewritten, since it\ncurrently does all output by writing it into a file.\nIs that an issue?\n\n-- \nMST\n"}]}