{"thread":{"id":"35270","subject":"[RFC/PATCH] Add interpret-trailers builtin","startedAt":"2013-11-03T21:17:30Z","lastAt":"2013-11-07T14:57:38Z","messageCount":13,"participants":["Christian Couder","Johan Herland","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"230070","messageId":"20131103211731.15716.95056.chriscool@tuxfamily.org","threadId":"35270","inReplyTo":null,"subject":"[RFC/PATCH] Add interpret-trailers builtin","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-11-03T21:17:30Z","receivedAt":"2013-11-03T21:17:30Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This RFC patch shows the work in progress to implement a new\ncommand:\n\n\tgit interpret-trailers\n\n1) Rational:\n\nThis command should help with RFC 822 style headers, called\n\"trailers\", that are found at the end of commit messages.\n\nFor a long time, these trailers have become a de facto standard\nway to add helpful information into commit messages.\n\nUntil now git commit has only supported the well known\n\"Signed-off-by: \" trailer, that is used by many projects like\nthe Linux kernel and Git.\n\nIt is better to implement features for these trailers in a new\ncommand rather than in builtin/commit.c, because this way the\nprepare-commit-msg and commit-msg hooks can reuse this command.\n\n2) Current state:\n\nCurrently the usage string of this command is:\n\ngit interpret-trailers [--trim-empty] [--infile=file] [...]\n\nThe following features are implemented:\n\t- the result is printed on stdout\n\t- the [...] arguments are interpreted\n\t- a commit message passed using the \"--infile=file\" option is interpreted\n\t- the \"trailer..value\" options in the config are interpreted\n\nThe following features are planned but not yet implemented:\n\t- some documentation\n\t- more tests\n\t- the \"trailer..if_exist\" config option\n\t- the \"trailer..if_missing\" config option\n\t- the \"trailer..command\" config option\n\n3) Notes:\n\n* \"trailer\" seems better than \"commitTrailer\" as the config key because\nit looks like all the config keys are lower case and \"committrailer\" is not\nvery readable.\n\n* \"trailer..value\" looks better than \"trailer..trailer\", so\nI chose the former.\n\n* Rather than only one \"trailer..style\" config option, it seems\nbetter to me to have both \"trailer..if_exist\" and\n\"trailer..if_missing\".\n\n* I might send a patch series instead of just one big patch when there will\nbe fewer big changes in the code.\n\nSigned-off-by: Christian Couder \n---\n .gitignore | 1 +\n Makefile | 1 +\n builtin.h | 1 +\n builtin/interpret-trailers.c | 284 ++++++++++++++++++++++++++++++++++++++++++\n git.c | 1 +\n strbuf.c | 7 ++\n strbuf.h | 1 +\n t/t7513-interpret-trailers.sh | 101 +++++++++++++++\n 8 files changed, 397 insertions(+)\n create mode 100644 builtin/interpret-trailers.c\n create mode 100755 t/t7513-interpret-trailers.sh\n\ndiff --git a/.gitignore b/.gitignore\nindex 66199ed..e6cf15b 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -73,6 +73,7 @@\n /git-index-pack\n /git-init\n /git-init-db\n+/git-interpret-trailers\n /git-instaweb\n /git-log\n /git-lost-found\ndiff --git a/Makefile b/Makefile\nindex af847f8..96441f1 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -937,6 +937,7 @@ BUILTIN_OBJS += builtin/hash-object.o\n BUILTIN_OBJS += builtin/help.o\n BUILTIN_OBJS += builtin/index-pack.o\n BUILTIN_OBJS += builtin/init-db.o\n+BUILTIN_OBJS += builtin/interpret-trailers.o\n BUILTIN_OBJS += builtin/log.o\n BUILTIN_OBJS += builtin/ls-files.o\n BUILTIN_OBJS += builtin/ls-remote.o\ndiff --git a/builtin.h b/builtin.h\nindex b56cf07..88c2999 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -71,6 +71,7 @@ extern int cmd_hash_object(int argc, const char **argv, const char *prefix);\n extern int cmd_help(int argc, const char **argv, const char *prefix);\n extern int cmd_index_pack(int argc, const char **argv, const char *prefix);\n extern int cmd_init_db(int argc, const char **argv, const char *prefix);\n+extern int cmd_interpret_trailers(int argc, const char **argv, const char *prefix);\n extern int cmd_log(int argc, const char **argv, const char *prefix);\n extern int cmd_log_reflog(int argc, const char **argv, const char *prefix);\n extern int cmd_ls_files(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nnew file mode 100644\nindex 0000000..2bcd480\n--- /dev/null\n+++ b/builtin/interpret-trailers.c\n@@ -0,0 +1,284 @@\n+/*\n+ * Builtin \"git interpret-trailers\"\n+ *\n+ * Copyright (c) 2013 Christian Couder \n+ *\n+ */\n+\n+#include \"cache.h\"\n+#include \"builtin.h\"\n+#include \"parse-options.h\"\n+#include \"strbuf.h\"\n+\n+static const char * const git_interpret_trailers_usage[] = {\n+\tN_(\"git interpret-trailers [--trim-empty] [--infile=file] [...]\"),\n+\tNULL\n+};\n+\n+static void parse_arg(struct strbuf *tok, struct strbuf *val, const char *arg)\n+{\n+\tchar *end = strchr(arg, '=');\n+\tif (!end)\n+\t\tend = strchr(arg, ':');\n+\tif (end) {\n+\t\tstrbuf_add(tok, arg, end - arg);\n+\t\tstrbuf_trim(tok);\n+\t\tstrbuf_addstr(val, end + 1);\n+\t\tstrbuf_trim(val);\n+\t} else {\n+\t\tstrbuf_addstr(tok, arg);\n+\t\tstrbuf_trim(tok);\n+\t}\n+}\n+\n+static struct string_list trailer_list;\n+\n+enum style_if_exist { DONT_REPEAT, OVERWRITE, REPEAT };\n+enum style_if_missing { DONT_APPEND, APPEND };\n+\n+struct trailer_info {\n+\tchar *value;\n+\tchar *command;\n+\tenum style_if_exist style_exist;\n+\tenum style_if_missing style_missing;\n+};\n+\n+static int git_trailer_config(const char *key, const char *value, void *cb)\n+{\n+\tif (!prefixcmp(key, \"trailer.\")) {\n+\t\tconst char *orig_key = key;\n+\t\tchar *name;\n+\t\tstruct string_list_item *item;\n+\t\tstruct trailer_info *info;\n+\t\tenum { VALUE, COMMAND, IF_EXIST, IF_MISSING } type;\n+\n+\t\tkey += 8;\n+\t\tif (!suffixcmp(key, \".value\")) {\n+\t\t\tname = xstrndup(key, strlen(key) - 6);\n+\t\t\ttype = VALUE;\n+\t\t} else if (!suffixcmp(key, \".command\")) {\n+\t\t\tname = xstrndup(key, strlen(key) - 8);\n+\t\t\ttype = COMMAND;\n+\t\t} else if (!suffixcmp(key, \".if_exist\")) {\n+\t\t\tname = xstrndup(key, strlen(key) - 9);\n+\t\t\ttype = IF_EXIST;\n+\t\t} else if (!suffixcmp(key, \".if_missing\")) {\n+\t\t\tname = xstrndup(key, strlen(key) - 11);\n+\t\t\ttype = IF_MISSING;\n+\t\t} else\n+\t\t\treturn 0;\n+\n+\t\titem = string_list_insert(&trailer_list, name);\n+\n+\t\tif (!item->util)\n+\t\t\titem->util = xcalloc(sizeof(struct trailer_info), 1);\n+\t\tinfo = item->util;\n+\t\tif (type == VALUE) {\n+\t\t\tif (info->value)\n+\t\t\t\twarning(_(\"more than one %s\"), orig_key);\n+\t\t\tinfo->value = xstrdup(value);\n+\t\t} else if (type == IF_EXIST) {\n+\t\t\tif (!strcasecmp(\"dont_repeat\", value)) {\n+\t\t\t\tinfo->style_exist = DONT_REPEAT;\n+\t\t\t} else if (!strcasecmp(\"overwrite\", value)) {\n+\t\t\t\tinfo->style_exist = OVERWRITE;\n+\t\t\t} else if (!strcasecmp(\"repeat\", value)) {\n+\t\t\t\tinfo->style_exist = REPEAT;\n+\t\t\t} else\n+\t\t\t\twarning(_(\"unknow value '%s' for key '%s'\"), value, orig_key);\n+\t\t} else if (type == IF_MISSING) {\n+\t\t\tif (!strcasecmp(\"dont_append\", value)) {\n+\t\t\t\tinfo->style_missing = DONT_APPEND;\n+\t\t\t} else if (!strcasecmp(\"append\", value)) {\n+\t\t\t\tinfo->style_missing = APPEND;\n+\t\t\t} else\n+\t\t\t\twarning(_(\"unknow value '%s' for key '%s'\"), value, orig_key);\n+\t\t} else {\n+\t\t\tif (info->command)\n+\t\t\t\twarning(_(\"more than one %s\"), orig_key);\n+\t\t\tinfo->command = xstrdup(value);\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n+static size_t alnum_len(const char *buf, size_t len) {\n+\twhile (--len >= 0 && !isalnum(buf[len]));\n+\treturn len + 1;\n+}\n+\n+static void apply_config(struct strbuf *tok, struct strbuf *val, struct trailer_info *info)\n+{\n+\tif (info->value) {\n+\t\tstrbuf_reset(tok);\n+\t\tstrbuf_addstr(tok, info->value);\n+\t}\n+\tif (info->command) {\n+\t}\n+}\n+\n+static void apply_config_list(struct strbuf *tok, struct strbuf *val)\n+{\n+\tint j, tok_alnum_len = alnum_len(tok->buf, tok->len);\n+\n+\tfor (j = 0; j < trailer_list.nr; j++) {\n+\t\tstruct string_list_item *item = trailer_list.items + j;\n+\t\tstruct trailer_info *info = item->util;\n+\t\tif (!strncasecmp(tok->buf, item->string, tok_alnum_len) ||\n+\t\t !strncasecmp(tok->buf, info->value, tok_alnum_len)) {\n+\t\t\tapply_config(tok, val, info);\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+}\n+\n+static struct strbuf **read_input_file(const char *infile)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (strbuf_read_file(&sb, infile, 0) < 0)\n+\t\tdie_errno(_(\"could not read input file '%s'\"), infile);\n+\n+\treturn strbuf_split(&sb, '\\n');\n+}\n+\n+/*\n+ * Return the the (0 based) index of the first trailer line\n+ * or the line count if there are no trailers.\n+ */\n+static int find_trailer_start(struct strbuf **lines)\n+{\n+\tint count, start, empty = 1;\n+\n+\t/* Get the line count */\n+\tfor (count = 0; lines[count]; count++);\n+\n+\t/*\n+\t * Get the start of the trailers by looking starting from the end\n+\t * for a line with only spaces before lines with one ':'.\n+\t */\n+\tfor (start = count - 1; start >= 0; start--) {\n+\t\tif (strbuf_isspace(lines[start])) {\n+\t\t\tif (empty)\n+\t\t\t\tcontinue;\n+\t\t\treturn start + 1;\n+\t\t}\n+\t\tif (strchr(lines[start]->buf, ':')) {\n+\t\t\tif (empty)\n+\t\t\t\tempty = 0;\n+\t\t\tcontinue;\n+\t\t}\n+\t\treturn count;\n+\t}\n+\n+\treturn empty ? count : start + 1;\n+}\n+\n+static void print_tok_val(const char *tok_buf, size_t tok_len,\n+\t\t\t const char *val_buf, size_t val_len)\n+{\n+\tchar c = tok_buf[tok_len - 1];\n+\tif (isalnum(c))\n+\t\tprintf(\"%s: %s\\n\", tok_buf, val_buf);\n+\telse if (isspace(c) || c == '#')\n+\t\tprintf(\"%s%s\\n\", tok_buf, val_buf);\n+\telse\n+\t\tprintf(\"%s %s\\n\", tok_buf, val_buf);\n+}\n+\n+static void process_input_file(const char *infile,\n+\t\t\t struct string_list *tok_list,\n+\t\t\t struct string_list *val_list)\n+{\n+\tstruct strbuf **lines = read_input_file(infile);\n+\tint start = find_trailer_start(lines);\n+\tint i;\n+\n+\t/* Output non trailer lines as is */\n+\tfor (i = 0; lines[i] && i < start; i++) {\n+\t\tprintf(\"%s\", lines[i]->buf);\n+\t}\n+\n+\t/* Process trailer lines */\n+\tfor (i = start; lines[i]; i++) {\n+\t\tstruct strbuf tok = STRBUF_INIT;\n+\t\tstruct strbuf val = STRBUF_INIT;\n+\t\tparse_arg(&tok, &val, lines[i]->buf);\n+\t\tstring_list_append(tok_list, strbuf_detach(&tok, NULL));\n+\t\tstring_list_append(val_list, strbuf_detach(&val, NULL));\n+\t}\n+}\n+\n+int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *infile = NULL;\n+\tint trim_empty = 0;\n+\tint i;\n+\tstruct string_list tok_list = STRING_LIST_INIT_NODUP;\n+\tstruct string_list val_list = STRING_LIST_INIT_NODUP;\n+\tchar *applied_arg;\n+\n+\tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"trim-empty\", &trim_empty, N_(\"trim empty trailers\")),\n+\t\tOPT_FILENAME(0, \"infile\", &infile, N_(\"use message from file\")),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\t git_interpret_trailers_usage, 0);\n+\n+\tgit_config(git_trailer_config, NULL);\n+\n+\t/* Print the non trailer part of infile */\n+\tif (infile) {\n+\t\tprocess_input_file(infile, &tok_list, &val_list);\n+\t\tapplied_arg = xcalloc(tok_list.nr, 1);\n+\t}\n+\n+\tfor (i = 0; i < argc; i++) {\n+\t\tstruct strbuf tok = STRBUF_INIT;\n+\t\tstruct strbuf val = STRBUF_INIT;\n+\t\tint j, seen = 0;\n+\n+\t\tparse_arg(&tok, &val, argv[i]);\n+\n+\t\tapply_config_list(&tok, &val);\n+\n+\t\t/* Apply the trailer arguments to the trailers in infile */\n+\t\tfor (j = 0; j < tok_list.nr; j++) {\n+\t\t\tstruct string_list_item *tok_item = tok_list.items + j;\n+\t\t\tstruct string_list_item *val_item = val_list.items + j;\n+\t\t\tint tok_alnum_len = alnum_len(tok.buf, tok.len);\n+\t\t\tif (!strncasecmp(tok.buf, tok_item->string, tok_alnum_len)) {\n+\t\t\t\ttok_item->string = xstrdup(tok.buf);\n+\t\t\t\tval_item->string = xstrdup(val.buf);\n+\t\t\t\tseen = 1;\n+\t\t\t\tapplied_arg[j] = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\t/* Print the trailer arguments that are not in infile */\n+\t\tif (!seen && (!trim_empty || val.len > 0))\n+\t\t\tprint_tok_val(tok.buf, tok.len, val.buf, val.len);\n+\n+\t\tstrbuf_release(&tok);\n+\t\tstrbuf_release(&val);\n+\t}\n+\n+\t/* Print the trailer part of infile */\n+\tfor (i = 0; i < tok_list.nr; i++) {\n+\t\tstruct strbuf tok = STRBUF_INIT;\n+\t\tstruct strbuf val = STRBUF_INIT;\n+\t\tstrbuf_addstr(&tok, tok_list.items[i].string);\n+\t\tstrbuf_addstr(&val, val_list.items[i].string);\n+\n+\t\tif (!applied_arg[i])\n+\t\t\tapply_config_list(&tok, &val);\n+\n+\t\tif (!trim_empty || val.len > 0)\n+\t\t\tprint_tok_val(tok.buf, tok.len, val.buf, val.len);\n+\t}\n+\n+\treturn 0;\n+}\ndiff --git a/git.c b/git.c\nindex cb5208d..9f2eb77 100644\n--- a/git.c\n+++ b/git.c\n@@ -383,6 +383,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"index-pack\", cmd_index_pack, RUN_SETUP_GENTLY },\n \t\t{ \"init\", cmd_init_db },\n \t\t{ \"init-db\", cmd_init_db },\n+\t\t{ \"interpret-trailers\", cmd_interpret_trailers, RUN_SETUP },\n \t\t{ \"log\", cmd_log, RUN_SETUP },\n \t\t{ \"ls-files\", cmd_ls_files, RUN_SETUP },\n \t\t{ \"ls-remote\", cmd_ls_remote, RUN_SETUP_GENTLY },\ndiff --git a/strbuf.c b/strbuf.c\nindex 1170d01..f9080fa 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -106,6 +106,13 @@ void strbuf_ltrim(struct strbuf *sb)\n \tsb->buf[sb->len] = '\\0';\n }\n \n+int strbuf_isspace(struct strbuf *sb)\n+{\n+\tchar *b;\n+\tfor (b = sb->buf; *b && isspace(*b); b++);\n+\treturn !*b;\n+}\n+\n struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n \t\t\t\t int terminator, int max)\n {\ndiff --git a/strbuf.h b/strbuf.h\nindex 73e80ce..02bff3a 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -42,6 +42,7 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len) {\n extern void strbuf_trim(struct strbuf *);\n extern void strbuf_rtrim(struct strbuf *);\n extern void strbuf_ltrim(struct strbuf *);\n+extern int strbuf_isspace(struct strbuf *);\n extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);\n \n /*\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nnew file mode 100755\nindex 0000000..5d2f967\n--- /dev/null\n+++ b/t/t7513-interpret-trailers.sh\n@@ -0,0 +1,101 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2013 Christian Couder\n+#\n+\n+test_description='git interpret-trailers'\n+\n+. ./test-lib.sh\n+\n+cat >basic_message <<'EOF'\n+subject\n+\n+body\n+EOF\n+\n+cat >complex_message_body <<'EOF'\n+my subject\n+\n+my body which is long\n+and contains some special\n+chars like : = ? !\n+\n+EOF\n+\n+# Do not remove trailing spaces below!\n+cat >complex_message_trailers <<'EOF'\n+Fixes: \n+Acked-by: \n+Reviewed-by: \n+Signed-off-by: \n+EOF\n+\n+test_expect_success 'without config' '\n+\tprintf \"ack: Peff\\nReviewed-by: \\nAcked-by: Johan\\n\" >expected &&\n+\tgit interpret-trailers \"ack = Peff\" \"Reviewed-by\" \"Acked-by: Johan\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success '--trim-empty without config' '\n+\tprintf \"ack: Peff\\nAcked-by: Johan\\n\" >expected &&\n+\tgit interpret-trailers --trim-empty \"ack = Peff\" \"Reviewed-by\" \"Acked-by: Johan\" \"sob:\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'with config setup' '\n+\tgit config trailer.ack.value \"Acked-by: \" &&\n+\tprintf \"Acked-by: Peff\\n\" >expected &&\n+\tgit interpret-trailers --trim-empty \"ack = Peff\" >actual &&\n+\ttest_cmp expected actual &&\n+\tgit interpret-trailers --trim-empty \"Acked-by = Peff\" >actual &&\n+\ttest_cmp expected actual &&\n+\tgit interpret-trailers --trim-empty \"Acked-by :Peff\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'with config setup and = sign' '\n+\tgit config trailer.ack.value \"Acked-by= \" &&\n+\tprintf \"Acked-by= Peff\\n\" >expected &&\n+\tgit interpret-trailers --trim-empty \"ack = Peff\" >actual &&\n+\ttest_cmp expected actual &&\n+\tgit interpret-trailers --trim-empty \"Acked-by= Peff\" >actual &&\n+\ttest_cmp expected actual &&\n+\tgit interpret-trailers --trim-empty \"Acked-by : Peff\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'with config setup and # sign' '\n+\tgit config trailer.bug.value \"Bug #\" &&\n+\tprintf \"Bug #42\\n\" >expected &&\n+\tgit interpret-trailers --trim-empty \"bug = 42\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'with commit basic message' '\n+\tgit interpret-trailers --infile basic_message >actual &&\n+\ttest_cmp basic_message actual\n+'\n+\n+test_expect_success 'with commit complex message' '\n+\tcat complex_message_body complex_message_trailers >complex_message &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Fixes: \\nAcked-by= \\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n+\tgit interpret-trailers --infile complex_message >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'with commit complex message and args' '\n+\tcat complex_message_body >expected &&\n+\tprintf \"Bug #42\\nFixes: \\nAcked-by= Peff\\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n+\tgit interpret-trailers --infile complex_message \"ack: Peff\" \"bug: 42\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'with commit complex message, args and --trim-empty' '\n+\tcat complex_message_body >expected &&\n+\tprintf \"Bug #42\\nAcked-by= Peff\\n\" >>expected &&\n+\tgit interpret-trailers --trim-empty --infile complex_message \"ack: Peff\" \"bug: 42\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_done\n-- \n1.8.4.1.576.g36ba827.dirty\n"},{"id":"230072","messageId":"CALKQrgdJ6d2SVWNQGa6d-eLYPAL-C21=tCyJczCDExLQRfq=jA@mail.gmail.com","threadId":"35270","inReplyTo":"20131103211731.15716.95056.chriscool@tuxfamily.org","subject":"Re: [RFC/PATCH] Add interpret-trailers builtin","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-11-04T01:01:05Z","receivedAt":"2013-11-04T01:01:05Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sun, Nov 3, 2013 at 10:17 PM, Christian Couder\n wrote:\n> This RFC patch shows the work in progress to implement a new\n> command:\n\nFirst of all: Thanks for working on this! This looks like a really\ngood start. Plenty of comments below (mostly either to learn myself,\nor to check what alternatives you've considered), but overall, I'm\nhappy about where this is going.\n\n> git interpret-trailers\n\nPesonally, I'd rather name it \"git process-footers\", since I think\n\"process\" better captures the two-way functionality of this program\n(both parsing/interpreting _and_ generating/writiing), and I believe\nI'm not alone in preferring \"footer\" over \"trailer\". That said, this\nis certainly bikeshedding territory, and I am not the one writing the\ncode, so feel free to ignore.\n\n> 1) Rational:\n\ns/Rational/Rationale/\n\n>\n> This command should help with RFC 822 style headers, called\n> \"trailers\", that are found at the end of commit messages.\n\nAs has been asked earlier in this discussion, we should probably\nspecify explicitly what we _mean_ with \"RFC822-style\"\nheaders/footers/trailers, and exactly how closely we follow the actual\nRFC... E.g. do we make use of the linebreaking rules? encoding\nhandling? etc... We may want to take a more relaxed approach (after\nall, we're not including a complete RFC822/RFC2822 implementation),\nbut we should at least state so, and possibly how/why we do so.\n\n> For a long time, these trailers have become a de facto standard\n> way to add helpful information into commit messages.\n>\n> Until now git commit has only supported the well known\n> \"Signed-off-by: \" trailer, that is used by many projects like\n> the Linux kernel and Git.\n>\n> It is better to implement features for these trailers in a new\n> command rather than in builtin/commit.c, because this way the\n> prepare-commit-msg and commit-msg hooks can reuse this command.\n>\n> 2) Current state:\n>\n> Currently the usage string of this command is:\n>\n> git interpret-trailers [--trim-empty] [--infile=file] [...]\n\n--trim-empty will remove empty footers given on the command-line? Or\nin the infile? or both? I think I'm fine with both, but I wonder if\nthere is a use case for treating the two types of empty footers\nseparately...\n\n> The following features are implemented:\n> - the result is printed on stdout\n> - the [...] arguments are interpreted\n> - a commit message passed using the \"--infile=file\" option is interpreted\n\nIf the output is written to stdout, then why is not the input taken\nfrom stdin? Or vice versa: why not --outfile?\n\n> - the \"trailer..value\" options in the config are interpreted\n\nThis is the default value of the footer if not further specified on\nthe command-line?\n\n> The following features are planned but not yet implemented:\n> - some documentation\n> - more tests\n> - the \"trailer..if_exist\" config option\n> - the \"trailer..if_missing\" config option\n\nNot sure what these two options will do, or what kind of values they take...\n\n> - the \"trailer..command\" config option\n\nI guess the value of .command is executed to generate the default\ncontent of the footer?\n\n> 3) Notes:\n>\n> * \"trailer\" seems better than \"commitTrailer\" as the config key because\n> it looks like all the config keys are lower case and \"committrailer\" is not\n> very readable.\n\nAs stated above, I prefer \"footer\" over \"trailer\"...\n\n> * \"trailer..value\" looks better than \"trailer..trailer\", so\n> I chose the former.\n>\n> * Rather than only one \"trailer..style\" config option, it seems\n> better to me to have both \"trailer..if_exist\" and\n> \"trailer..if_missing\".\n>\n> * I might send a patch series instead of just one big patch when there will\n> be fewer big changes in the code.\n\nMaybe at least split in two:\n - One patch to deal with the administrivia of adding a new command.\n - One (or more) patch(es) to introduce the logic/functionality of the\nnew command.\n\n> Signed-off-by: Christian Couder \n> ---\n> .gitignore | 1 +\n> Makefile | 1 +\n> builtin.h | 1 +\n> builtin/interpret-trailers.c | 284 ++++++++++++++++++++++++++++++++++++++++++\n> git.c | 1 +\n> strbuf.c | 7 ++\n> strbuf.h | 1 +\n> t/t7513-interpret-trailers.sh | 101 +++++++++++++++\n> 8 files changed, 397 insertions(+)\n> create mode 100644 builtin/interpret-trailers.c\n> create mode 100755 t/t7513-interpret-trailers.sh\n>\n> diff --git a/.gitignore b/.gitignore\n> index 66199ed..e6cf15b 100644\n> --- a/.gitignore\n> +++ b/.gitignore\n> @@ -73,6 +73,7 @@\n> /git-index-pack\n> /git-init\n> /git-init-db\n> +/git-interpret-trailers\n> /git-instaweb\n> /git-log\n> /git-lost-found\n> diff --git a/Makefile b/Makefile\n> index af847f8..96441f1 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -937,6 +937,7 @@ BUILTIN_OBJS += builtin/hash-object.o\n> BUILTIN_OBJS += builtin/help.o\n> BUILTIN_OBJS += builtin/index-pack.o\n> BUILTIN_OBJS += builtin/init-db.o\n> +BUILTIN_OBJS += builtin/interpret-trailers.o\n> BUILTIN_OBJS += builtin/log.o\n> BUILTIN_OBJS += builtin/ls-files.o\n> BUILTIN_OBJS += builtin/ls-remote.o\n> diff --git a/builtin.h b/builtin.h\n> index b56cf07..88c2999 100644\n> --- a/builtin.h\n> +++ b/builtin.h\n> @@ -71,6 +71,7 @@ extern int cmd_hash_object(int argc, const char **argv, const char *prefix);\n> extern int cmd_help(int argc, const char **argv, const char *prefix);\n> extern int cmd_index_pack(int argc, const char **argv, const char *prefix);\n> extern int cmd_init_db(int argc, const char **argv, const char *prefix);\n> +extern int cmd_interpret_trailers(int argc, const char **argv, const char *prefix);\n> extern int cmd_log(int argc, const char **argv, const char *prefix);\n> extern int cmd_log_reflog(int argc, const char **argv, const char *prefix);\n> extern int cmd_ls_files(int argc, const char **argv, const char *prefix);\n> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n> new file mode 100644\n> index 0000000..2bcd480\n> --- /dev/null\n> +++ b/builtin/interpret-trailers.c\n> @@ -0,0 +1,284 @@\n> +/*\n> + * Builtin \"git interpret-trailers\"\n> + *\n> + * Copyright (c) 2013 Christian Couder \n> + *\n> + */\n> +\n> +#include \"cache.h\"\n> +#include \"builtin.h\"\n> +#include \"parse-options.h\"\n> +#include \"strbuf.h\"\n> +\n> +static const char * const git_interpret_trailers_usage[] = {\n> + N_(\"git interpret-trailers [--trim-empty] [--infile=file] [...]\"),\n> + NULL\n> +};\n> +\n> +static void parse_arg(struct strbuf *tok, struct strbuf *val, const char *arg)\n\nWhen discussing RFC822-style headers (or in our case,\nfooter/trailers), one often talks about a \"field\" which consists of a\n\"field name\" and a \"field value\" (or \"field body\"). I propose that we\nuse the same naming convention in our code.\n\nHere, I assume 'tok' is the field name, and 'val' is the field value.\nMaybe rename to \"name\" and \"value\", respectively?\n\n> +{\n> + char *end = strchr(arg, '=');\n> + if (!end)\n> + end = strchr(arg, ':');\n\nSo both '=' (preferred) and ':' are accepted as field/value\nseparators. That's ok for the command-line, I believe.\n\n> + if (end) {\n> + strbuf_add(tok, arg, end - arg);\n> + strbuf_trim(tok);\n> + strbuf_addstr(val, end + 1);\n> + strbuf_trim(val);\n> + } else {\n> + strbuf_addstr(tok, arg);\n> + strbuf_trim(tok);\n> + }\n> +}\n> +\n> +static struct string_list trailer_list;\n> +\n> +enum style_if_exist { DONT_REPEAT, OVERWRITE, REPEAT };\n\n>From the enum values, I assume that these declare the desired behavior\nwhen there are two (or more?) or the same footer (i.e. same \"field\nname\"). However, it's not (yet) obvious to me in which order they are\nprocessed. There are several opportunities for multiple instances of\nthe same \"field name\":\n\n - Two (or more) occurences in the infile\n - Two (or more) occurences on the command line\n - One occurence in the infile, and one of the command line\n\nIt is not obvious to me in which order these are processed, or which\nof those cases constitute a duplicate/repetition, hence it's not clear\nto me what the result will be when e.g. using OVERWRITE. Also not sure\nwhat DONT_REPEAT means, and if/how it's different from OVERWRITE.\nInstead, I suggest we should specify clearly:\n\n 1. In which order are the footers processed\n 2. What constitutes a repetition/duplicate (IMHO all of the above three cases)\n 3. Better enum names that are easily understood given the rules in #1\nand #2, I suggest:\n\nenum handle_duplicates { KEEP_FIRST, KEEP_LAST, KEEP_ALL };\n\n> +enum style_if_missing { DONT_APPEND, APPEND };\n\nenum handle_missing { SKIP, INCLUDE }; ?\n\n> +struct trailer_info {\n> + char *value;\n\nIs this the field _name_ or field _value_?\n\n> + char *command;\n\nI assume this is only used when trailer..command is used?\n\n> + enum style_if_exist style_exist;\n> + enum style_if_missing style_missing;\n> +};\n> +\n> +static int git_trailer_config(const char *key, const char *value, void *cb)\n> +{\n> + if (!prefixcmp(key, \"trailer.\")) {\n> + const char *orig_key = key;\n> + char *name;\n> + struct string_list_item *item;\n> + struct trailer_info *info;\n> + enum { VALUE, COMMAND, IF_EXIST, IF_MISSING } type;\n> +\n> + key += 8;\n> + if (!suffixcmp(key, \".value\")) {\n> + name = xstrndup(key, strlen(key) - 6);\n> + type = VALUE;\n> + } else if (!suffixcmp(key, \".command\")) {\n> + name = xstrndup(key, strlen(key) - 8);\n> + type = COMMAND;\n> + } else if (!suffixcmp(key, \".if_exist\")) {\n> + name = xstrndup(key, strlen(key) - 9);\n> + type = IF_EXIST;\n> + } else if (!suffixcmp(key, \".if_missing\")) {\n> + name = xstrndup(key, strlen(key) - 11);\n> + type = IF_MISSING;\n> + } else\n> + return 0;\n> +\n> + item = string_list_insert(&trailer_list, name);\n> +\n> + if (!item->util)\n> + item->util = xcalloc(sizeof(struct trailer_info), 1);\n> + info = item->util;\n> + if (type == VALUE) {\n> + if (info->value)\n> + warning(_(\"more than one %s\"), orig_key);\n> + info->value = xstrdup(value);\n> + } else if (type == IF_EXIST) {\n\nConsider splitting the parsing of values -> enums into helper\nfunctions, to make this function shorter and its logic more easily\nfollowed.\n\n> + if (!strcasecmp(\"dont_repeat\", value)) {\n> + info->style_exist = DONT_REPEAT;\n> + } else if (!strcasecmp(\"overwrite\", value)) {\n> + info->style_exist = OVERWRITE;\n> + } else if (!strcasecmp(\"repeat\", value)) {\n> + info->style_exist = REPEAT;\n> + } else\n> + warning(_(\"unknow value '%s' for key '%s'\"), value, orig_key);\n\ns/unknow/unknown/\n\n> + } else if (type == IF_MISSING) {\n> + if (!strcasecmp(\"dont_append\", value)) {\n> + info->style_missing = DONT_APPEND;\n> + } else if (!strcasecmp(\"append\", value)) {\n> + info->style_missing = APPEND;\n> + } else\n> + warning(_(\"unknow value '%s' for key '%s'\"), value, orig_key);\n> + } else {\n> + if (info->command)\n> + warning(_(\"more than one %s\"), orig_key);\n> + info->command = xstrdup(value);\n> + }\n> + }\n> + return 0;\n> +}\n> +\n> +static size_t alnum_len(const char *buf, size_t len) {\n> + while (--len >= 0 && !isalnum(buf[len]));\n> + return len + 1;\n> +}\n> +\n> +static void apply_config(struct strbuf *tok, struct strbuf *val, struct trailer_info *info)\n> +{\n> + if (info->value) {\n> + strbuf_reset(tok);\n> + strbuf_addstr(tok, info->value);\n> + }\n> + if (info->command) {\n> + }\n\nI guess this if-block might be filled by a later commit, but is there\nany reason to have the empty block in this commit?\n\n> +}\n> +\n> +static void apply_config_list(struct strbuf *tok, struct strbuf *val)\n> +{\n> + int j, tok_alnum_len = alnum_len(tok->buf, tok->len);\n\nSo tok_alnum_len is the length of the initial run of alphanumeric\ncharacters in tok (i.e. does not include '-'?)\n\n> +\n> + for (j = 0; j < trailer_list.nr; j++) {\n\nWe're iterating through the footers we found in infile?\n\n> + struct string_list_item *item = trailer_list.items + j;\n> + struct trailer_info *info = item->util;\n> + if (!strncasecmp(tok->buf, item->string, tok_alnum_len) ||\n> + !strncasecmp(tok->buf, info->value, tok_alnum_len)) {\n\nWhat is held in item->string? and what is held in info->value, and why\nare _both_ being compared to (the initial run of alphanumerics in)\ntok?\n\n> + apply_config(tok, val, info);\n> + break;\n> + }\n> + }\n> +}\n> +\n> +static struct strbuf **read_input_file(const char *infile)\n> +{\n> + struct strbuf sb = STRBUF_INIT;\n> +\n> + if (strbuf_read_file(&sb, infile, 0) < 0)\n> + die_errno(_(\"could not read input file '%s'\"), infile);\n> +\n> + return strbuf_split(&sb, '\\n');\n> +}\n> +\n> +/*\n> + * Return the the (0 based) index of the first trailer line\n> + * or the line count if there are no trailers.\n> + */\n> +static int find_trailer_start(struct strbuf **lines)\n> +{\n> + int count, start, empty = 1;\n> +\n> + /* Get the line count */\n> + for (count = 0; lines[count]; count++);\n> +\n> + /*\n> + * Get the start of the trailers by looking starting from the end\n> + * for a line with only spaces before lines with one ':'.\n> + */\n\nThis means that if the footers end up in multiple paragraphs, like this:\n\n Tested-by: foo\n Reviewed-by: bar\n\n Signed-off-by: xyzzy\n Signed-off-by: zyxxy\n\nthen only the latter of these two sections will be considered? What\nabout instead looking for the last line (i.e. first line from bottom)\nthat neither (a) contains ':' or (b) consists solely of zero or more\nwhitespace (blank lines)? Maybe also want to tighten rule (a) to only\nmatch lines that consist of a single word (i.e. no inter-word spaces)\nbefore the ':' (i.e. something like \"^\\s*[a-zA-Z0-9-]+\\s*:\" in\nregexp-speak).\n\nThat's all I had time for now. Hopefully, I'll have some more time to\ncontinue the review later.\n\n\nHope this helps,\n\n...Johan\n\n> + for (start = count - 1; start >= 0; start--) {\n> + if (strbuf_isspace(lines[start])) {\n> + if (empty)\n> + continue;\n> + return start + 1;\n> + }\n> + if (strchr(lines[start]->buf, ':')) {\n> + if (empty)\n> + empty = 0;\n> + continue;\n> + }\n> + return count;\n> + }\n> +\n> + return empty ? count : start + 1;\n> +}\n> +\n> +static void print_tok_val(const char *tok_buf, size_t tok_len,\n> + const char *val_buf, size_t val_len)\n> +{\n> + char c = tok_buf[tok_len - 1];\n> + if (isalnum(c))\n> + printf(\"%s: %s\\n\", tok_buf, val_buf);\n> + else if (isspace(c) || c == '#')\n> + printf(\"%s%s\\n\", tok_buf, val_buf);\n> + else\n> + printf(\"%s %s\\n\", tok_buf, val_buf);\n> +}\n> +\n> +static void process_input_file(const char *infile,\n> + struct string_list *tok_list,\n> + struct string_list *val_list)\n> +{\n> + struct strbuf **lines = read_input_file(infile);\n> + int start = find_trailer_start(lines);\n> + int i;\n> +\n> + /* Output non trailer lines as is */\n> + for (i = 0; lines[i] && i < start; i++) {\n> + printf(\"%s\", lines[i]->buf);\n> + }\n> +\n> + /* Process trailer lines */\n> + for (i = start; lines[i]; i++) {\n> + struct strbuf tok = STRBUF_INIT;\n> + struct strbuf val = STRBUF_INIT;\n> + parse_arg(&tok, &val, lines[i]->buf);\n> + string_list_append(tok_list, strbuf_detach(&tok, NULL));\n> + string_list_append(val_list, strbuf_detach(&val, NULL));\n> + }\n> +}\n> +\n> +int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n> +{\n> + const char *infile = NULL;\n> + int trim_empty = 0;\n> + int i;\n> + struct string_list tok_list = STRING_LIST_INIT_NODUP;\n> + struct string_list val_list = STRING_LIST_INIT_NODUP;\n> + char *applied_arg;\n> +\n> + struct option options[] = {\n> + OPT_BOOL(0, \"trim-empty\", &trim_empty, N_(\"trim empty trailers\")),\n> + OPT_FILENAME(0, \"infile\", &infile, N_(\"use message from file\")),\n> + OPT_END()\n> + };\n> +\n> + argc = parse_options(argc, argv, prefix, options,\n> + git_interpret_trailers_usage, 0);\n> +\n> + git_config(git_trailer_config, NULL);\n> +\n> + /* Print the non trailer part of infile */\n> + if (infile) {\n> + process_input_file(infile, &tok_list, &val_list);\n> + applied_arg = xcalloc(tok_list.nr, 1);\n> + }\n> +\n> + for (i = 0; i < argc; i++) {\n> + struct strbuf tok = STRBUF_INIT;\n> + struct strbuf val = STRBUF_INIT;\n> + int j, seen = 0;\n> +\n> + parse_arg(&tok, &val, argv[i]);\n> +\n> + apply_config_list(&tok, &val);\n> +\n> + /* Apply the trailer arguments to the trailers in infile */\n> + for (j = 0; j < tok_list.nr; j++) {\n> + struct string_list_item *tok_item = tok_list.items + j;\n> + struct string_list_item *val_item = val_list.items + j;\n> + int tok_alnum_len = alnum_len(tok.buf, tok.len);\n> + if (!strncasecmp(tok.buf, tok_item->string, tok_alnum_len)) {\n> + tok_item->string = xstrdup(tok.buf);\n> + val_item->string = xstrdup(val.buf);\n> + seen = 1;\n> + applied_arg[j] = 1;\n> + break;\n> + }\n> + }\n> +\n> + /* Print the trailer arguments that are not in infile */\n> + if (!seen && (!trim_empty || val.len > 0))\n> + print_tok_val(tok.buf, tok.len, val.buf, val.len);\n> +\n> + strbuf_release(&tok);\n> + strbuf_release(&val);\n> + }\n> +\n> + /* Print the trailer part of infile */\n> + for (i = 0; i < tok_list.nr; i++) {\n> + struct strbuf tok = STRBUF_INIT;\n> + struct strbuf val = STRBUF_INIT;\n> + strbuf_addstr(&tok, tok_list.items[i].string);\n> + strbuf_addstr(&val, val_list.items[i].string);\n> +\n> + if (!applied_arg[i])\n> + apply_config_list(&tok, &val);\n> +\n> + if (!trim_empty || val.len > 0)\n> + print_tok_val(tok.buf, tok.len, val.buf, val.len);\n> + }\n> +\n> + return 0;\n> +}\n> diff --git a/git.c b/git.c\n> index cb5208d..9f2eb77 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -383,6 +383,7 @@ static void handle_internal_command(int argc, const char **argv)\n> { \"index-pack\", cmd_index_pack, RUN_SETUP_GENTLY },\n> { \"init\", cmd_init_db },\n> { \"init-db\", cmd_init_db },\n> + { \"interpret-trailers\", cmd_interpret_trailers, RUN_SETUP },\n> { \"log\", cmd_log, RUN_SETUP },\n> { \"ls-files\", cmd_ls_files, RUN_SETUP },\n> { \"ls-remote\", cmd_ls_remote, RUN_SETUP_GENTLY },\n> diff --git a/strbuf.c b/strbuf.c\n> index 1170d01..f9080fa 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -106,6 +106,13 @@ void strbuf_ltrim(struct strbuf *sb)\n> sb->buf[sb->len] = '\\0';\n> }\n>\n> +int strbuf_isspace(struct strbuf *sb)\n> +{\n> + char *b;\n> + for (b = sb->buf; *b && isspace(*b); b++);\n> + return !*b;\n> +}\n> +\n> struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n> int terminator, int max)\n> {\n> diff --git a/strbuf.h b/strbuf.h\n> index 73e80ce..02bff3a 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -42,6 +42,7 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len) {\n> extern void strbuf_trim(struct strbuf *);\n> extern void strbuf_rtrim(struct strbuf *);\n> extern void strbuf_ltrim(struct strbuf *);\n> +extern int strbuf_isspace(struct strbuf *);\n> extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);\n>\n> /*\n> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\n> new file mode 100755\n> index 0000000..5d2f967\n> --- /dev/null\n> +++ b/t/t7513-interpret-trailers.sh\n> @@ -0,0 +1,101 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2013 Christian Couder\n> +#\n> +\n> +test_description='git interpret-trailers'\n> +\n> +. ./test-lib.sh\n> +\n> +cat >basic_message <<'EOF'\n> +subject\n> +\n> +body\n> +EOF\n> +\n> +cat >complex_message_body <<'EOF'\n> +my subject\n> +\n> +my body which is long\n> +and contains some special\n> +chars like : = ? !\n> +\n> +EOF\n> +\n> +# Do not remove trailing spaces below!\n> +cat >complex_message_trailers <<'EOF'\n> +Fixes:\n> +Acked-by:\n> +Reviewed-by:\n> +Signed-off-by:\n> +EOF\n> +\n> +test_expect_success 'without config' '\n> + printf \"ack: Peff\\nReviewed-by: \\nAcked-by: Johan\\n\" >expected &&\n> + git interpret-trailers \"ack = Peff\" \"Reviewed-by\" \"Acked-by: Johan\" >actual &&\n> + test_cmp expected actual\n> +'\n> +\n> +test_expect_success '--trim-empty without config' '\n> + printf \"ack: Peff\\nAcked-by: Johan\\n\" >expected &&\n> + git interpret-trailers --trim-empty \"ack = Peff\" \"Reviewed-by\" \"Acked-by: Johan\" \"sob:\" >actual &&\n> + test_cmp expected actual\n> +'\n> +\n> +test_expect_success 'with config setup' '\n> + git config trailer.ack.value \"Acked-by: \" &&\n> + printf \"Acked-by: Peff\\n\" >expected &&\n> + git interpret-trailers --trim-empty \"ack = Peff\" >actual &&\n> + test_cmp expected actual &&\n> + git interpret-trailers --trim-empty \"Acked-by = Peff\" >actual &&\n> + test_cmp expected actual &&\n> + git interpret-trailers --trim-empty \"Acked-by :Peff\" >actual &&\n> + test_cmp expected actual\n> +'\n> +\n> +test_expect_success 'with config setup and = sign' '\n> + git config trailer.ack.value \"Acked-by= \" &&\n> + printf \"Acked-by= Peff\\n\" >expected &&\n> + git interpret-trailers --trim-empty \"ack = Peff\" >actual &&\n> + test_cmp expected actual &&\n> + git interpret-trailers --trim-empty \"Acked-by= Peff\" >actual &&\n> + test_cmp expected actual &&\n> + git interpret-trailers --trim-empty \"Acked-by : Peff\" >actual &&\n> + test_cmp expected actual\n> +'\n> +\n> +test_expect_success 'with config setup and # sign' '\n> + git config trailer.bug.value \"Bug #\" &&\n> + printf \"Bug #42\\n\" >expected &&\n> + git interpret-trailers --trim-empty \"bug = 42\" >actual &&\n> + test_cmp expected actual\n> +'\n> +\n> +test_expect_success 'with commit basic message' '\n> + git interpret-trailers --infile basic_message >actual &&\n> + test_cmp basic_message actual\n> +'\n> +\n> +test_expect_success 'with commit complex message' '\n> + cat complex_message_body complex_message_trailers >complex_message &&\n> + cat complex_message_body >expected &&\n> + printf \"Fixes: \\nAcked-by= \\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n> + git interpret-trailers --infile complex_message >actual &&\n> + test_cmp expected actual\n> +'\n> +\n> +test_expect_success 'with commit complex message and args' '\n> + cat complex_message_body >expected &&\n> + printf \"Bug #42\\nFixes: \\nAcked-by= Peff\\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n> + git interpret-trailers --infile complex_message \"ack: Peff\" \"bug: 42\" >actual &&\n> + test_cmp expected actual\n> +'\n> +\n> +test_expect_success 'with commit complex message, args and --trim-empty' '\n> + cat complex_message_body >expected &&\n> + printf \"Bug #42\\nAcked-by= Peff\\n\" >>expected &&\n> + git interpret-trailers --trim-empty --infile complex_message \"ack: Peff\" \"bug: 42\" >actual &&\n> + test_cmp expected actual\n> +'\n> +\n> +test_done\n> --\n> 1.8.4.1.576.g36ba827.dirty\n>\n\n\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"230084","messageId":"xmqqli14yoqi.fsf@gitster.dls.corp.google.com","threadId":"35270","inReplyTo":"20131103211731.15716.95056.chriscool@tuxfamily.org","subject":"Re: [RFC/PATCH] Add interpret-trailers builtin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-04T19:02:29Z","receivedAt":"2013-11-04T19:02:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> This RFC patch shows the work in progress to implement a new\n> command:\n>\n> \tgit interpret-trailers\n>\n> 1) Rational:\n>\n> This command should help with RFC 822 style headers, called\n> \"trailers\", that are found at the end of commit messages.\n>\n> For a long time, these trailers have become a de facto standard\n> way to add helpful information into commit messages.\n>\n> Until now git commit has only supported the well known\n> \"Signed-off-by: \" trailer, that is used by many projects like\n> the Linux kernel and Git.\n>\n> It is better to implement features for these trailers in a new\n> command rather than in builtin/commit.c, because this way the\n> prepare-commit-msg and commit-msg hooks can reuse this command.\n>\n> 2) Current state:\n>\n> Currently the usage string of this command is:\n>\n> git interpret-trailers [--trim-empty] [--infile=file] [...]\n>\n> The following features are implemented:\n> \t- the result is printed on stdout\n> \t- the [...] arguments are interpreted\n> \t- a commit message passed using the \"--infile=file\" option is interpreted\n> \t- the \"trailer..value\" options in the config are interpreted\n>\n> The following features are planned but not yet implemented:\n> \t- some documentation\n> \t- more tests\n> \t- the \"trailer..if_exist\" config option\n> \t- the \"trailer..if_missing\" config option\n> \t- the \"trailer..command\" config option\n>\n> 3) Notes:\n>\n> * \"trailer\" seems better than \"commitTrailer\" as the config key because\n> it looks like all the config keys are lower case and \"committrailer\" is not\n> very readable.\n\nAnd closes the door for other things from later acquiring trailers?\n\n> * \"trailer..value\" looks better than \"trailer..trailer\", so\n> I chose the former.\n\nIf that is a literal value, then I think \".value\" is indeed good.\n\n> * Rather than only one \"trailer..style\" config option, it seems\n> better to me to have both \"trailer..if_exist\" and\n> \"trailer..if_missing\".\n\nAs there is no \".if_exist\", \".if_missing\", etc. here, I cannot guess\nwhat these \"seemingly independent and orthogonal, but some\ncombinations are impossible\" configuration variables are meant to be\nused, let alone agreeing to the above claim that they are better\nthan a single \".style\". I think you took the \".style\" from my\nthinking-aloud message the other day, but that aloud-thinking lead\nto \".style\" by taking various use cases people wanted to have\nfooters into account, including:\n\n - just appending, allowing duplication of the field name (e.g. more\n than one \"cc:\" can name different recipients);\n\n - appending, allowing duplication of the field pair\n (e.g. a patch that flowed from person A to B and possibly\n somewhere else then finally back to A may have \"Signed-off-by:\n A\", chain of other's Sob, and another \"Signed-off-by: A\");\n\n - appending, but without consecutive repeats (e.g. the same\n \"Signed-off-by:\" example; person A does not pass a patch to\n himself, adding two same pair next to each other);\n\n - adding only if there is no same (e.g. \"Change-Id:\");\n\n - adding only if there is no pair (e.g. \"Closes: #bugId\");\n\n - adding one if there is none, replacing one if there already is.\n\nI do not think it is easier for users to express these (and other)\nsemantics as combinations of \"seemingly independent and orthogonal,\nbut some combinations are impossible\" configuration variables.\n\n> * I might send a patch series instead of just one big patch when there will\n> be fewer big changes in the code.\n>\n> Signed-off-by: Christian Couder \n> ---\n> .gitignore | 1 +\n> Makefile | 1 +\n> builtin.h | 1 +\n> builtin/interpret-trailers.c | 284 ++++++++++++++++++++++++++++++++++++++++++\n> git.c | 1 +\n> strbuf.c | 7 ++\n> strbuf.h | 1 +\n\nI think you're better off having trailers.c at the top level that is\ncalled from builtin/interpret-trailers.c (aside from names), so that\nwe can later hook the former up with builtin/commit.c codepath.\n"},{"id":"230085","messageId":"xmqqfvrcyoaj.fsf@gitster.dls.corp.google.com","threadId":"35270","inReplyTo":"CALKQrgdJ6d2SVWNQGa6d-eLYPAL-C21=tCyJczCDExLQRfq=jA@mail.gmail.com","subject":"Re: [RFC/PATCH] Add interpret-trailers builtin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-04T19:12:04Z","receivedAt":"2013-11-04T19:12:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\nThanks for looking this over. I am mostly in agreement and will\nelide the parts I do not have much to add.\n\n>> This command should help with RFC 822 style headers, called\n>> \"trailers\", that are found at the end of commit messages.\n>\n> As has been asked earlier in this discussion, we should probably\n> specify explicitly what we _mean_ with \"RFC822-style\"\n> headers/footers/trailers, and exactly how closely we follow the actual\n> RFC... E.g. do we make use of the linebreaking rules? encoding\n> handling? etc... We may want to take a more relaxed approach (after\n> all, we're not including a complete RFC822/RFC2822 implementation),\n> but we should at least state so, and possibly how/why we do so.\n\nIndeed; especially I do not think we would want to do the 2822\ncontination lines, or 2047 MIME quoting, ever.\n\n>> For a long time, these trailers have become a de facto standard\n>> way to add helpful information into commit messages.\n\n\"helpful\" is not the key aspect of these footers. They are added at\nthe end to introduce some structure into a free-form part of the\ncommit objects.\n\n>> The following features are implemented:\n>> - the result is printed on stdout\n>> - the [...] arguments are interpreted\n>> - a commit message passed using the \"--infile=file\" option is interpreted\n>\n> If the output is written to stdout, then why is not the input taken\n> from stdin? Or vice versa: why not --outfile?\n\nI'd say just make it a filter without --in/outfile ;-)\n\n>> +{\n>> + char *end = strchr(arg, '=');\n>> + if (!end)\n>> + end = strchr(arg, ':');\n>\n> So both '=' (preferred) and ':' are accepted as field/value\n> separators. That's ok for the command-line, I believe.\n\nWhy?\n\nSometimes you have to be loose from the beginning _if_ some existing\nuses and established conventions make it easier for the users, but\nif you do not have to start from being loose, it is almost always a\nmistake to do so. The above code just closed the door to use \":\"\nfor some other useful purposes we may later discover, and will make\nus regret for doing so.\n\n> From the enum values, I assume that these declare the desired behavior\n> when there are two (or more?) or the same footer (i.e. same \"field\n> name\"). However, it's not (yet) obvious to me in which order they are\n> processed. There are several opportunities for multiple instances of\n> the same \"field name\":\n>\n> - Two (or more) occurences in the infile\n> - Two (or more) occurences on the command line\n> - One occurence in the infile, and one of the command line\n\nAlso, there is a distinction between fields with the same field name\nappearing twice and fields with the same field name and the same\nfield value appearing twice. Some headers do want to have them, some\ndo not. \n"},{"id":"230108","messageId":"CALKQrgcweo+B8JKH85-4-SOHsAi6SAxstnXhRWCst_qEyXYmhA@mail.gmail.com","threadId":"35270","inReplyTo":"xmqqfvrcyoaj.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH] Add interpret-trailers builtin","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-11-05T02:45:53Z","receivedAt":"2013-11-05T02:45:53Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, Nov 4, 2013 at 8:12 PM, Junio C Hamano wrote:\n> Johan Herland writes:\n>>> +{\n>>> + char *end = strchr(arg, '=');\n>>> + if (!end)\n>>> + end = strchr(arg, ':');\n>>\n>> So both '=' (preferred) and ':' are accepted as field/value\n>> separators. That's ok for the command-line, I believe.\n>\n> Why?\n>\n> Sometimes you have to be loose from the beginning _if_ some existing\n> uses and established conventions make it easier for the users, but\n> if you do not have to start from being loose, it is almost always a\n> mistake to do so. The above code just closed the door to use \":\"\n> for some other useful purposes we may later discover, and will make\n> us regret for doing so.\n\nAlthough I agree with the principle, I think there are (at least) two\nestablished conventions that will be commonly used from the start, and\nthat we should support:\n\n - Using short forms with '=', e.g. \"ack=Peff\". There is already a\nconvention on how we specify + pairs on the command\nline, e.g. \"git -c foo=bar ...\"\n\n - Copy-pasting footers from existing commit messages. These will have\nthe same format as the expected output of this command, and not\naccepting the same format in its input seems silly, IMHO.\n\nThat said, I think this applies only to the formatting on the _command\nline_. When it comes to how the resulting footers are formatted in the\ncommit message, I would argue it only makes sense to use ':', and I\nthink the testcase named 'with config setup and = sign' in the above\npatch is ugly and unnecessary.\n\n>> From the enum values, I assume that these declare the desired behavior\n>> when there are two (or more?) or the same footer (i.e. same \"field\n>> name\"). However, it's not (yet) obvious to me in which order they are\n>> processed. There are several opportunities for multiple instances of\n>> the same \"field name\":\n>>\n>> - Two (or more) occurences in the infile\n>> - Two (or more) occurences on the command line\n>> - One occurence in the infile, and one of the command line\n>\n> Also, there is a distinction between fields with the same field name\n> appearing twice and fields with the same field name and the same\n> field value appearing twice. Some headers do want to have them, some\n> do not.\n\nTrue. This complexity is partly why I initially wanted to leave this\nwhole thing up to hooks, but I guess now we have to deal with it...\nThat said, I believe we don't need to cater to every possibility under\nthe sun, just the most common ones. If a minority of users are not\nsatisfied, they can simply configure this to leave all duplicates in\nplace, and then write their own commit-msg hook to do whatever weird\nconsolidation/cleanup they prefer.\n\n...Johan\n\nPS: Since this program will be run by \"git commit\", and might also be\ninvoked by hooks, we need to keep the following in mind:\n\n - Design for idempotence. A given set of headers might be filtered\nthrough this program multiple times, and we should make sure that\nmultiple runs over the same data does not itself cause problems.\n\n - Optional/flexible configuration. When a hook runs this program, it\nmay want to impose its own set of rules that does not entirely (or at\nall) coincide with what is set in the config. We should therefore\nconsider providing a way for the caller to specify a separate source\nof trailer/footer config to apply on a given run.\n\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"230110","messageId":"20131105.064219.1992046444757435360.chriscool@tuxfamily.org","threadId":"35270","inReplyTo":"CALKQrgcweo+B8JKH85-4-SOHsAi6SAxstnXhRWCst_qEyXYmhA@mail.gmail.com","subject":"Re: [RFC/PATCH] Add interpret-trailers builtin","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-11-05T05:42:19Z","receivedAt":"2013-11-05T05:42:19Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Johan Herland \n> On Mon, Nov 4, 2013 at 8:12 PM, Junio C Hamano wrote:\n>> Johan Herland writes:\n>>>> +{\n>>>> + char *end = strchr(arg, '=');\n>>>> + if (!end)\n>>>> + end = strchr(arg, ':');\n>>>\n>>> So both '=' (preferred) and ':' are accepted as field/value\n>>> separators. That's ok for the command-line, I believe.\n>>\n>> Why?\n>>\n>> Sometimes you have to be loose from the beginning _if_ some existing\n>> uses and established conventions make it easier for the users,\n\nThe users are already used to appending \"Acked-by: Joe \".\nSo I think it makes it easier for the user to accept what they are already\nused to provide.\n\n>> but\n>> if you do not have to start from being loose, it is almost always a\n>> mistake to do so. The above code just closed the door to use \":\"\n>> for some other useful purposes we may later discover, and will make\n>> us regret for doing so.\n> \n> Although I agree with the principle, I think there are (at least) two\n> established conventions that will be commonly used from the start, and\n> that we should support:\n> \n> - Using short forms with '=', e.g. \"ack=Peff\". There is already a\n> convention on how we specify + pairs on the command\n> line, e.g. \"git -c foo=bar ...\"\n> \n> - Copy-pasting footers from existing commit messages. These will have\n> the same format as the expected output of this command, and not\n> accepting the same format in its input seems silly, IMHO.\n\nI agree. Also I think it will avoid some mistakes by the users.\nBecause it would require an effort for them to remember that if they\nwant to see \"Acked-by: Joe \" they have to put\n\"Acked-by= Joe \" on the command line.\n\n> That said, I think this applies only to the formatting on the _command\n> line_.\n\nBut wouldn't it be nice if the same parsing function could be used for\nboth the command line and the commit message template?\n\n> When it comes to how the resulting footers are formatted in the\n> commit message, I would argue it only makes sense to use ':', and I\n> think the testcase named 'with config setup and = sign' in the above\n> patch is ugly and unnecessary.\n\nI wanted to support configurations like this:\n\n[trailer \"ack\"]\n value = \"Acked-by= \"\n[trailer \"bug\"]\n value = \"Bug #\" \n\nbecause Peff said that GitHub uses '#' and while at it I suppose some\npeople might prefer '=' over ':'.\n\nThanks,\nChristian.\n"},{"id":"230121","messageId":"xmqqli12acoh.fsf@gitster.dls.corp.google.com","threadId":"35270","inReplyTo":"CALKQrgcweo+B8JKH85-4-SOHsAi6SAxstnXhRWCst_qEyXYmhA@mail.gmail.com","subject":"Re: [RFC/PATCH] Add interpret-trailers builtin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-05T19:09:02Z","receivedAt":"2013-11-05T19:09:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland writes:\n\n> On Mon, Nov 4, 2013 at 8:12 PM, Junio C Hamano wrote:\n>> Johan Herland writes:\n>>>> +{\n>>>> + char *end = strchr(arg, '=');\n>>>> + if (!end)\n>>>> + end = strchr(arg, ':');\n>>>\n>>> So both '=' (preferred) and ':' are accepted as field/value\n>>> separators. That's ok for the command-line, I believe.\n>>\n>> Why?\n>>\n>> Sometimes you have to be loose from the beginning _if_ some existing\n>> uses and established conventions make it easier for the users, but\n>> if you do not have to start from being loose, it is almost always a\n>> mistake to do so. The above code just closed the door to use \":\"\n>> for some other useful purposes we may later discover, and will make\n>> us regret for doing so.\n>\n> Although I agree with the principle, I think there are (at least) two\n> established conventions that will be commonly used from the start, and\n> that we should support:\n>\n> - Using short forms with '=', e.g. \"ack=Peff\". There is already a\n> convention on how we specify + pairs on the command\n> line, e.g. \"git -c foo=bar ...\"\n\nI do not have much problem with this.\n\n> - Copy-pasting footers from existing commit messages. These will have\n> the same format as the expected output of this command, and not\n> accepting the same format in its input seems silly, IMHO.\n\nI am not sure about this, but syntactically, it is very similar to\n\n --message \"CC: Johan Herland \"\n\nso probably it is OK, but then we do not even have to limit it to\ncolon, no? E.g. appending an arbitrary footer, with its literal\nvalue, may be done with something like:\n\n --footer \"CC: Johan Herland \"\n --footer \"Closes #12345\"\n\n>> Also, there is a distinction between fields with the same field name\n>> appearing twice and fields with the same field name and the same\n>> field value appearing twice. Some headers do want to have them, some\n>> do not.\n>\n> True. This complexity is partly why I initially wanted to leave this\n> whole thing up to hooks, but I guess now we have to deal with it...\n\nIf we are adding anything, it has to be able to express what we\nalready do for Signed-off-by: line, so we cannot start from\nsomewhere any simpler than this, I am afraid.\n"},{"id":"230135","messageId":"20131106.074355.225932577498673677.chriscool@tuxfamily.org","threadId":"35270","inReplyTo":"xmqqli14yoqi.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH] Add interpret-trailers builtin","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-11-06T06:43:55Z","receivedAt":"2013-11-06T06:43:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano \n> Christian Couder writes:\n>>\n>> * \"trailer\" seems better than \"commitTrailer\" as the config key because\n>> it looks like all the config keys are lower case and \"committrailer\" is not\n>> very readable.\n> \n> And closes the door for other things from later acquiring trailers?\n\nI don't think it really closes the door, as they can have a name like\n\"stufftrailer\" then. Or better they could call it something else than\n\"trailer\" everywhere from the beginning to avoid mistakes in the first\nplace.\n\nOr if they are really trailers, for example maybe tag trailers, then\nin many cases they might want to share the same configuration as\ncommit trailers. In this case, it would be a mistake to use\n\"commitTrailer\" when most people would like them to also apply to\ntags.\n\nIf/when tag trailers appear, then we can decide that \"trailer\" is\ncommon for all trailers, \"commitTrailer\" is for commits only and\n\"tagTrailer\" for tags only.\n\nAnd anyway commit trailers have existed since the beginning or nearly\nthe beginning of Git, and we haven't seen yet any other kind of\ntrailer, so it's reasonnable to think that we can safely take the name\nand not worry, be happy.\n\n>> * \"trailer..value\" looks better than \"trailer..trailer\", so\n>> I chose the former.\n> \n> If that is a literal value, then I think \".value\" is indeed good.\n\nThat was what I thought first too.\n\nBut, after thinking about what Johan said, I think that it might be\nconfusing for some people, so now I wonder if I should call it \"key\".\n\n>> * Rather than only one \"trailer..style\" config option, it seems\n>> better to me to have both \"trailer..if_exist\" and\n>> \"trailer..if_missing\".\n> \n> As there is no \".if_exist\", \".if_missing\", etc. here, I cannot guess\n> what these \"seemingly independent and orthogonal, but some\n> combinations are impossible\" configuration variables are meant to be\n> used, let alone agreeing to the above claim that they are better\n> than a single \".style\".\n\nYeah, I should have explained more. So I will do it now.\n\nIn the code I used the following enums:\n\nenum style_if_exist { DONT_REPEAT, OVERWRITE, REPEAT };\nenum style_if_missing { DONT_APPEND, APPEND };\n\nThe style_if_exist enum is meant to decide what is done when we have\nto deal with 2 or more trailers, from the infile or the command line,\nthat have the same key but different not empty values.\n\nFor example, me might have the 3 following trailers:\n\n\"Acked-by: joe \"\n\"Acked-by: bob \"\n\"Acked-by: joe \"\n\n- The DONT_REPEAT style, which should be the default in my opinion,\nwould mean that we don't repeat the same values. So we would get:\n\n\"Acked-by: joe \"\n\"Acked-by: bob \"\n\n- The OVERWRITE style would mean that we keep only one, (for example\nthe last one). So we would get:\n\n\"Acked-by: joe \"\n\n- The REPEAT style would mean that we keep everything. So we would\nget:\n\n\"Acked-by: joe \"\n\"Acked-by: bob \"\n\"Acked-by: joe \"\n\nThe style_if_missing enum is meant to decide what is done when we have\nno trailer with the specified key. Then DONT_APPEND means that we do\nnothing, which should be the default, and APPEND means that we append\na trailer with the specified key.\n\nOf course in the latter case, a command should probably be specified\nto tell which value should be used with the key.\n\nFor example:\n\n[trailer \"signoff\"]\n\t key = \"Signed-off-by:\"\n\t if_missing = append\n\t command = echo \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"'\n\nwould append a s-o-b line only if there is no s-o-b already. \n\nNote that to always append a specific trailer one could use:\n\n[trailer \"signoff\"]\n\t key = \"Signed-off-by:\"\n\t if_missing = append\n\t if_exist = repeat\n\t command = echo \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"'\n\n> I think you took the \".style\" from my\n> thinking-aloud message the other day, but that aloud-thinking lead\n> to \".style\" by taking various use cases people wanted to have\n> footers into account, including:\n\nOk, I will try to guess below, how the use cases could be configured.\n\n> - just appending, allowing duplication of the field name (e.g. more\n> than one \"cc:\" can name different recipients);\n\nThat would be the default (if_exist = dont_repeat, if_missing = dont_append).\n\n> - appending, allowing duplication of the field pair\n> (e.g. a patch that flowed from person A to B and possibly\n> somewhere else then finally back to A may have \"Signed-off-by:\n> A\", chain of other's Sob, and another \"Signed-off-by: A\");\n\nThat would be: if_exist = repeat, if_missing = dont_append\n\n> - appending, but without consecutive repeats (e.g. the same\n> \"Signed-off-by:\" example; person A does not pass a patch to\n> himself, adding two same pair next to each other);\n\nI could add a DONT_REPEAT_PREVIOUS into the style_if_exist enum, for\nthis case.\n\nThen that would be: if_exist = dont_repeat_previous, if_missing = dont_append\n\n> - adding only if there is no same (e.g. \"Change-Id:\");\n\nI could add a DONT_APPEND into the style_if_exist enum, for this case.\n\nThen that would be: if_exist = dont_append, if_missing = append\n\n> - adding only if there is no pair (e.g. \"Closes: #bugId\");\n\nThat would be: if_exist = dont_repeat, if_missing = append\n\n> - adding one if there is none, replacing one if there already is.\n\nThat would be: if_exist = overwrite, if_missing = append\n\n> I do not think it is easier for users to express these (and other)\n> semantics as combinations of \"seemingly independent and orthogonal,\n> but some combinations are impossible\" configuration variables.\n\nWith what I suggest, I don't think there would be some impossible\ncombinations.\n\n>> * I might send a patch series instead of just one big patch when there will\n>> be fewer big changes in the code.\n>>\n>> Signed-off-by: Christian Couder \n>> ---\n>> .gitignore | 1 +\n>> Makefile | 1 +\n>> builtin.h | 1 +\n>> builtin/interpret-trailers.c | 284 ++++++++++++++++++++++++++++++++++++++++++\n>> git.c | 1 +\n>> strbuf.c | 7 ++\n>> strbuf.h | 1 +\n> \n> I think you're better off having trailers.c at the top level that is\n> called from builtin/interpret-trailers.c (aside from names), so that\n> we can later hook the former up with builtin/commit.c codepath.\n\nOk, I will do that.\n\nThanks,\nChristian.\n"},{"id":"230136","messageId":"CAP8UFD0STna++2StV1RcT2bB83Lh_hFQU94A0y4ziovs61Z==A@mail.gmail.com","threadId":"35270","inReplyTo":"20131106.074355.225932577498673677.chriscool@tuxfamily.org","subject":"Re: [RFC/PATCH] Add interpret-trailers builtin","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2013-11-06T09:23:24Z","receivedAt":"2013-11-06T09:23:24Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Nov 6, 2013 at 7:43 AM, Christian Couder\n wrote:\n> Of course in the latter case, a command should probably be specified\n> to tell which value should be used with the key.\n>\n> For example:\n>\n> [trailer \"signoff\"]\n> key = \"Signed-off-by:\"\n> if_missing = append\n> command = echo \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"'\n>\n> would append a s-o-b line only if there is no s-o-b already.\n\nSorry, I realize that I was wrong above.\nAs the default for \"if_exist\" is \"dont_repeat\", the above would append\na s-o-b if there is no s-o-b or if there is one but with a different\nvalue.\n\nTo append a s-o-b only if there is no s-o-b already, one would need to use:\n\n[trailer \"signoff\"]\n key = \"Signed-off-by:\"\n if_exist = dont_append\n if_missing = append\n command = echo \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"'\n\nThanks,\nChristian.\n"},{"id":"230145","messageId":"xmqq4n7pa1pn.fsf@gitster.dls.corp.google.com","threadId":"35270","inReplyTo":"CAP8UFD0STna++2StV1RcT2bB83Lh_hFQU94A0y4ziovs61Z==A@mail.gmail.com","subject":"Re: [RFC/PATCH] Add interpret-trailers builtin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-06T17:18:12Z","receivedAt":"2013-11-06T17:18:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> To append a s-o-b only if there is no s-o-b already, one would need to use:\n>\n> [trailer \"signoff\"]\n> key = \"Signed-off-by:\"\n> if_exist = dont_append\n> if_missing = append\n> command = echo \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"'\n\nBut that is insufficient to emulate what we do, no? I.e. append\nunless the last one is from the same person we are about to add.\n"},{"id":"230153","messageId":"20131106.211659.1008913569403460043.chriscool@tuxfamily.org","threadId":"35270","inReplyTo":"xmqq4n7pa1pn.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH] Add interpret-trailers builtin","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2013-11-06T20:16:59Z","receivedAt":"2013-11-06T20:16:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano \n\n> Christian Couder writes:\n> \n>> To append a s-o-b only if there is no s-o-b already, one would need to use:\n>>\n>> [trailer \"signoff\"]\n>> key = \"Signed-off-by:\"\n>> if_exist = dont_append\n>> if_missing = append\n>> command = echo \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"'\n> \n> But that is insufficient to emulate what we do, no? I.e. append\n> unless the last one is from the same person we are about to add.\n\nYeah, but, with DONT_REPEAT_PREVIOUS, it would be possible using:\n\n[trailer \"signoff\"]\n key = \"Signed-off-by:\"\n if_exist = dont_repeat_previous\n if_missing = append\n command = echo \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"'\n\nCheers,\nChristian.\n"},{"id":"230155","messageId":"xmqq7gcl8dor.fsf@gitster.dls.corp.google.com","threadId":"35270","inReplyTo":"20131106.211659.1008913569403460043.chriscool@tuxfamily.org","subject":"Re: [RFC/PATCH] Add interpret-trailers builtin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-06T20:42:28Z","receivedAt":"2013-11-06T20:42:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> From: Junio C Hamano \n>\n>> Christian Couder writes:\n>> \n>>> To append a s-o-b only if there is no s-o-b already, one would need to use:\n>>>\n>>> [trailer \"signoff\"]\n>>> key = \"Signed-off-by:\"\n>>> if_exist = dont_append\n>>> if_missing = append\n>>> command = echo \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"'\n>> \n>> But that is insufficient to emulate what we do, no? I.e. append\n>> unless the last one is from the same person we are about to add.\n>\n> Yeah, but, with DONT_REPEAT_PREVIOUS, it would be possible using:\n>\n> [trailer \"signoff\"]\n> key = \"Signed-off-by:\"\n> if_exist = dont_repeat_previous\n> if_missing = append\n> command = echo \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"'\n\nAnything is possible, but \"possible\" does not justify \"it is better\nway than other possible ways\".\n\nWhat are the plausible values for if_missing? If if_missing needs\n\"prepend\", for example, in addition to \"append\", does it mean\nif_exist also needs corresponding \"prepend\" variant for the value\n\"dont_repeat_previous\" you would give to if_exist?\n\nHaving two that are seemingly independent configuration does not\nseem to be helping in reducing complexity (by keeping settings that\ncan be independently set orthogonal, by saying \"if the other rule\ndecides to add, do we append, prepend, insert at the middle?\", for\nexample).\n\nHow would one differentiate between \"there is a field with that key\"\nand \"there is a field with that combination\" with a\nsingle if_exist? Add another variable if_exist_exact?\n"},{"id":"230196","messageId":"CAP8UFD09PbOL=EPCQifsakZo5s_vhzQRfcp9LhOjq2gAbUcKpw@mail.gmail.com","threadId":"35270","inReplyTo":"xmqq7gcl8dor.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH] Add interpret-trailers builtin","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2013-11-07T14:57:38Z","receivedAt":"2013-11-07T14:57:38Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Nov 6, 2013 at 9:42 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>> From: Junio C Hamano \n>>\n>>> But that is insufficient to emulate what we do, no? I.e. append\n>>> unless the last one is from the same person we are about to add.\n>>\n>> Yeah, but, with DONT_REPEAT_PREVIOUS, it would be possible using:\n>>\n>> [trailer \"signoff\"]\n>> key = \"Signed-off-by:\"\n>> if_exist = dont_repeat_previous\n>> if_missing = append\n>> command = echo \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"'\n>\n> Anything is possible, but \"possible\" does not justify \"it is better\n> way than other possible ways\".\n>\n> What are the plausible values for if_missing? If if_missing needs\n> \"prepend\", for example, in addition to \"append\", does it mean\n> if_exist also needs corresponding \"prepend\" variant for the value\n> \"dont_repeat_previous\" you would give to if_exist?\n\nYeah, we could add \"prepend\" to the possible values for if_missing and\nalso other values that prepend instead of append to if_exist.\nBut I am not sure they would be as useful.\n\nIf we think they are useful, then maybe we could instead add another\nconfiguration to say if we want to append or prepend or put in the\nmiddle, and change the others to make them clearer.\nFor example we could have:\n\nwhere = (after | middle | before)\nif_exist = (add_if_different | add_if_neighbor_different | add |\noverwrite | do_nothing)\nif_missing = (do_nothing | add)\n\n(The default being the first choice.)\n\n> Having two that are seemingly independent configuration does not\n> seem to be helping in reducing complexity (by keeping settings that\n> can be independently set orthogonal, by saying \"if the other rule\n> decides to add, do we append, prepend, insert at the middle?\", for\n> example).\n\nI am not sure I understand what you mean.\nIn my opinion, if we want to use just one configuration, instead of 2\nor 3, it will not reduce complexity.\n\nMaybe we could parse something like:\n\nstyle = if_exist:add_if_different:after, if_missing:add:before\n\nor like:\n\nstyle = (append | prepend | insert | append_unless |\nprepend_unless | insert_unless )\n\nwith being a regexp where $KEY is the key and $VALUE is the\nvalue, so you can say: append_unless ^$KEY[:=]$VALUE.*\n\nor like:\n\nstyle = (append_if | prepend_if | insert_if )\n\nwith being a shell command that should exit with code 0.\n\n(The command would be passed the existing trailers in stdin.\nSo you could say things like: \"append_if true\" or \"append_if tail -1 |\ngrep -v '$KEY: $VALUE'\")\n\nBut if we want to keep things simple, I think what I suggest first\nabove is probably best.\n\nAnd by the way, later we might add \"add_if_match \" or\n\"add_if_true \" to \"if_exist\" and \"if_missing\", so we could still\nbe as powerful as the other styles.\n\n> How would one differentiate between \"there is a field with that key\"\n> and \"there is a field with that combination\" with a\n> single if_exist? Add another variable if_exist_exact?\n\n\"if_exist = do_nothing\" would mean: \"do nothing if there is a field\nwith that key\"\n\"if_exist = overwrite\" would mean: \"overwrite the existing value of a\nfield with that key\"\n\"if_exist = add\" would mean: \"add if there is one or more fields with\nthat key (whatever the value(s), so the value(s) could be the same)\"\n\"if_exist = add_if_different\" would mean: \"add only if there is no\nfield with the same pair\"\n\"if_exist = add_if_different_neighbor\" would mean: \"add only if there\nis no field with the same pair where we are going to add\"\n\n\"if_missing = do_nothing\" would mean: \"do nothing if there is no field\nwith that key\"\n\"if_missing= add\" would mean: \"add if there is no field with that key\"\n\n\"where = after\" would mean: \"if we decide to add, we will put the\ntrailer after the other trailers\"\n\"where = middle\" would mean: \"if we decide to add, we will put the\ntrailer in the middle of the other trailers\"\n\"where = before\" would mean: \"if we decide to add, we will put the\ntrailer before the other trailers\"\n\nIn my opinion, that should be enough for most use cases.\n\nIt is true that some people might want something more complex (like\nadding only if there is no value for the same key that match a given\nregexp) or something stranger like adding only if there is already a\nfield with the same pair.\n\nBut we can take care of these special cases later if they actually happen.\nThen we will hopefully have more experience.\nAnd we could add things like \"add_if_no_value_match \" or\n\"add_if \" to if_exist and if_missing .\n\nThanks,\nChristian.\n"}]}