{"thread":{"id":"35819","subject":"[PATCH v5 00/14] Add interpret-trailers builtin","startedAt":"2014-02-06T20:19:49Z","lastAt":"2014-02-23T10:44:38Z","messageCount":48,"participants":["Christian Couder","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":5,"patchTotal":14},"messages":[{"id":"234398","messageId":"20140206194123.325.99451.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":null,"subject":"[PATCH v5 00/14] Add interpret-trailers builtin","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-06T20:19:49Z","receivedAt":"2014-02-06T20:19:49Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This patch series implements a new command:\n\n git interpret-trailers\n\nand an infrastructure to process trailers that can be reused,\nfor example in \"commit.c\".\n\n1) Rationale:\n\nThis command should help with RFC 822 style headers, called\n\"trailers\", that are found at the end of commit messages.\n\n(Note that these headers do not follow and are not intended to\nfollow many rules that are in RFC 822. For example they do not\nfollow the line breaking rules, the encoding rules and probably\nmany other rules.)\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 first in a\nnew command 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=] [([(=|:)])...]\n\nThe following features are implemented:\n\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 - if \"--infile\" is not used, a commit message is read from stdin\n - the \"trailer..key\" options in the config are interpreted\n - the \"trailer..where\" options are interpreted\n - the \"trailer..ifExist\" options are interpreted\n - the \"trailer..ifMissing\" options are interpreted\n - the \"trailer..command\" config works\n - $ARG can be used in commands\n - ditto for GIT_{AUTHOR,COMMITTER}_{NAME,EMAIL} env variables\n - there are some tests\n - there is some documentation\n\nThe following features are planned but not yet implemented:\n - add more tests related to commands\n - add examples in documentation\n - integration with \"git commit\"\n\nPossible improvements:\n - support GIT_COMMIT_PROTO env variable in commands\n\n3) Changes since version 4, thanks to Eric:\n\n* many small functions became 'static inline' instead of just 'static' \n* alnum_len() has an \"int len\" parameter instead of \"size_t len\"\n* some redundant comments were removed\n* some \"const\" have been added\n* some \"switch\" replaced some \"if ... else if ...\"\n* some string related functions have been removed from strbuf.{c,h}\n* some memory leaks have been fixed (but see below)\n* the refactoring patch 11/17 was squashed into a previous patch\n* the documentation patch was improved\n\n\n4) TODO from previous review:\n\n- check for memory leaks\n- add test for empty token\n\n\nChristian Couder (14):\n Add data structures and basic functions for commit trailers\n trailer: process trailers from file and arguments\n trailer: read and process config information\n trailer: process command line trailer arguments\n trailer: parse trailers from input file\n trailer: put all the processing together and print\n trailer: add interpret-trailers command\n trailer: add tests for \"git interpret-trailers\"\n trailer: if no input file is passed, read from stdin\n trailer: execute command from 'trailer..command'\n trailer: add tests for trailer command\n trailer: set author and committer env variables\n trailer: add tests for commands using env variables\n Documentation: add documentation for 'git interpret-trailers'\n\n .gitignore | 1 +\n Documentation/git-interpret-trailers.txt | 132 +++++++\n Makefile | 2 +\n builtin.h | 1 +\n builtin/interpret-trailers.c | 36 ++\n git.c | 1 +\n t/t7513-interpret-trailers.sh | 262 ++++++++++++\n trailer.c | 658 +++++++++++++++++++++++++++++++\n trailer.h | 6 +\n 9 files changed, 1099 insertions(+)\n create mode 100644 Documentation/git-interpret-trailers.txt\n create mode 100644 builtin/interpret-trailers.c\n create mode 100755 t/t7513-interpret-trailers.sh\n create mode 100644 trailer.c\n create mode 100644 trailer.h\n\n-- \n1.8.5.2.206.g98f5689.dirty\n"},{"id":"234408","messageId":"20140206202004.325.80557.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"20140206194123.325.99451.chriscool@tuxfamily.org","subject":"[PATCH v5 01/14] Add data structures and basic functions for commit trailers","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-06T20:19:50Z","receivedAt":"2014-02-06T20:19:50Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"We will use a doubly linked list to store all information\nabout trailers and their configuration.\n\nThis way we can easily remove or add trailers to or from\ntrailer lists while traversing the lists in either direction.\n\nSigned-off-by: Christian Couder \n---\n Makefile | 1 +\n trailer.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 49 insertions(+)\n create mode 100644 trailer.c\n\ndiff --git a/Makefile b/Makefile\nindex b4af1e2..ec90feb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -871,6 +871,7 @@ LIB_OBJS += submodule.o\n LIB_OBJS += symlinks.o\n LIB_OBJS += tag.o\n LIB_OBJS += trace.o\n+LIB_OBJS += trailer.o\n LIB_OBJS += transport.o\n LIB_OBJS += transport-helper.o\n LIB_OBJS += tree-diff.o\ndiff --git a/trailer.c b/trailer.c\nnew file mode 100644\nindex 0000000..f129b5a\n--- /dev/null\n+++ b/trailer.c\n@@ -0,0 +1,48 @@\n+#include \"cache.h\"\n+/*\n+ * Copyright (c) 2013 Christian Couder \n+ */\n+\n+enum action_where { WHERE_AFTER, WHERE_BEFORE };\n+enum action_if_exist { EXIST_ADD_IF_DIFFERENT, EXIST_ADD_IF_DIFFERENT_NEIGHBOR,\n+\t\t EXIST_ADD, EXIST_OVERWRITE, EXIST_DO_NOTHING };\n+enum action_if_missing { MISSING_ADD, MISSING_DO_NOTHING };\n+\n+struct conf_info {\n+\tchar *name;\n+\tchar *key;\n+\tchar *command;\n+\tenum action_where where;\n+\tenum action_if_exist if_exist;\n+\tenum action_if_missing if_missing;\n+};\n+\n+struct trailer_item {\n+\tstruct trailer_item *previous;\n+\tstruct trailer_item *next;\n+\tconst char *token;\n+\tconst char *value;\n+\tstruct conf_info *conf;\n+};\n+\n+static inline int same_token(struct trailer_item *a, struct trailer_item *b, int alnum_len)\n+{\n+\treturn !strncasecmp(a->token, b->token, alnum_len);\n+}\n+\n+static inline int same_value(struct trailer_item *a, struct trailer_item *b)\n+{\n+\treturn !strcasecmp(a->value, b->value);\n+}\n+\n+static inline int same_trailer(struct trailer_item *a, struct trailer_item *b, int alnum_len)\n+{\n+\treturn same_token(a, b, alnum_len) && same_value(a, b);\n+}\n+\n+/* Get the length of buf from its beginning until its last alphanumeric character */\n+static inline size_t alnum_len(const char *buf, int len)\n+{\n+\twhile (--len >= 0 && !isalnum(buf[len]));\n+\treturn (size_t) len + 1;\n+}\n-- \n1.8.5.2.206.g98f5689.dirty\n"},{"id":"234410","messageId":"20140206202004.325.96525.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"20140206194123.325.99451.chriscool@tuxfamily.org","subject":"[PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-06T20:19:51Z","receivedAt":"2014-02-06T20:19:51Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This patch implements the logic that process trailers\nfrom file and arguments.\n\nAt the beginning trailers from file are in their own\ninfile_tok doubly linked list, and trailers from\narguments are in their own arg_tok doubly linked list.\n\nThe lists are traversed and when an arg_tok should be\n\"applied\", it is removed from its list and inserted\ninto the infile_tok list.\n\nSigned-off-by: Christian Couder \n---\n trailer.c | 187 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 187 insertions(+)\n\ndiff --git a/trailer.c b/trailer.c\nindex f129b5a..ba0cfe0 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -46,3 +46,190 @@ static inline size_t alnum_len(const char *buf, int len)\n \twhile (--len >= 0 && !isalnum(buf[len]));\n \treturn (size_t) len + 1;\n }\n+\n+static void add_arg_to_infile(struct trailer_item *infile_tok,\n+\t\t\t struct trailer_item *arg_tok)\n+{\n+\tif (arg_tok->conf->where == WHERE_AFTER) {\n+\t\targ_tok->next = infile_tok->next;\n+\t\tinfile_tok->next = arg_tok;\n+\t\targ_tok->previous = infile_tok;\n+\t\tif (arg_tok->next)\n+\t\t\targ_tok->next->previous = arg_tok;\n+\t} else {\n+\t\targ_tok->previous = infile_tok->previous;\n+\t\tinfile_tok->previous = arg_tok;\n+\t\targ_tok->next = infile_tok;\n+\t\tif (arg_tok->previous)\n+\t\t\targ_tok->previous->next = arg_tok;\n+\t}\n+}\n+\n+static int check_if_different(struct trailer_item *infile_tok,\n+\t\t\t struct trailer_item *arg_tok,\n+\t\t\t int alnum_len, int check_all)\n+{\n+\tenum action_where where = arg_tok->conf->where;\n+\tdo {\n+\t\tif (!infile_tok)\n+\t\t\treturn 1;\n+\t\tif (same_trailer(infile_tok, arg_tok, alnum_len))\n+\t\t\treturn 0;\n+\t\t/*\n+\t\t * if we want to add a trailer after another one,\n+\t\t * we have to check those before this one\n+\t\t */\n+\t\tinfile_tok = (where == WHERE_AFTER) ? infile_tok->previous : infile_tok->next;\n+\t} while (check_all);\n+\treturn 1;\n+}\n+\n+static void apply_arg_if_exist(struct trailer_item *infile_tok,\n+\t\t\t struct trailer_item *arg_tok,\n+\t\t\t int alnum_len)\n+{\n+\tswitch (arg_tok->conf->if_exist) {\n+\tcase EXIST_DO_NOTHING:\n+\t\tfree(arg_tok);\n+\t\tbreak;\n+\tcase EXIST_OVERWRITE:\n+\t\tfree((char *)infile_tok->value);\n+\t\tinfile_tok->value = xstrdup(arg_tok->value);\n+\t\tfree(arg_tok);\n+\t\tbreak;\n+\tcase EXIST_ADD:\n+\t\tadd_arg_to_infile(infile_tok, arg_tok);\n+\t\tbreak;\n+\tcase EXIST_ADD_IF_DIFFERENT:\n+\t\tif (check_if_different(infile_tok, arg_tok, alnum_len, 1))\n+\t\t\tadd_arg_to_infile(infile_tok, arg_tok);\n+\t\telse\n+\t\t\tfree(arg_tok);\n+\t\tbreak;\n+\tcase EXIST_ADD_IF_DIFFERENT_NEIGHBOR:\n+\t\tif (check_if_different(infile_tok, arg_tok, alnum_len, 0))\n+\t\t\tadd_arg_to_infile(infile_tok, arg_tok);\n+\t\telse\n+\t\t\tfree(arg_tok);\n+\t\tbreak;\n+\t}\n+}\n+\n+static void remove_from_list(struct trailer_item *item,\n+\t\t\t struct trailer_item **first)\n+{\n+\tif (item->next)\n+\t\titem->next->previous = item->previous;\n+\tif (item->previous)\n+\t\titem->previous->next = item->next;\n+\telse\n+\t\t*first = item->next;\n+}\n+\n+static struct trailer_item *remove_first(struct trailer_item **first)\n+{\n+\tstruct trailer_item *item = *first;\n+\t*first = item->next;\n+\tif (item->next) {\n+\t\titem->next->previous = NULL;\n+\t\titem->next = NULL;\n+\t}\n+\treturn item;\n+}\n+\n+static void process_infile_tok(struct trailer_item *infile_tok,\n+\t\t\t struct trailer_item **arg_tok_first,\n+\t\t\t enum action_where where)\n+{\n+\tstruct trailer_item *arg_tok;\n+\tstruct trailer_item *next_arg;\n+\n+\tint tok_alnum_len = alnum_len(infile_tok->token, strlen(infile_tok->token));\n+\tfor (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {\n+\t\tnext_arg = arg_tok->next;\n+\t\tif (same_token(infile_tok, arg_tok, tok_alnum_len) &&\n+\t\t arg_tok->conf->where == where) {\n+\t\t\tremove_from_list(arg_tok, arg_tok_first);\n+\t\t\tapply_arg_if_exist(infile_tok, arg_tok, tok_alnum_len);\n+\t\t\t/*\n+\t\t\t * If arg has been added to infile,\n+\t\t\t * then we need to process it too now.\n+\t\t\t */\n+\t\t\tif ((where == WHERE_AFTER ? infile_tok->next : infile_tok->previous) == arg_tok)\n+\t\t\t\tinfile_tok = arg_tok;\n+\t\t}\n+\t}\n+}\n+\n+static void update_last(struct trailer_item **last)\n+{\n+\tif (*last)\n+\t\twhile((*last)->next != NULL)\n+\t\t\t*last = (*last)->next;\n+}\n+\n+static void update_first(struct trailer_item **first)\n+{\n+\tif (*first)\n+\t\twhile((*first)->previous != NULL)\n+\t\t\t*first = (*first)->previous;\n+}\n+\n+static void apply_arg_if_missing(struct trailer_item **infile_tok_first,\n+\t\t\t\t struct trailer_item **infile_tok_last,\n+\t\t\t\t struct trailer_item *arg_tok)\n+{\n+\tstruct trailer_item **infile_tok;\n+\tenum action_where where;\n+\n+\tswitch (arg_tok->conf->if_missing) {\n+\tcase MISSING_DO_NOTHING:\n+\t\tfree(arg_tok);\n+\t\tbreak;\n+\tcase MISSING_ADD:\n+\t\twhere = arg_tok->conf->where;\n+\t\tinfile_tok = (where == WHERE_AFTER) ? infile_tok_last : infile_tok_first;\n+\t\tif (*infile_tok) {\n+\t\t\tadd_arg_to_infile(*infile_tok, arg_tok);\n+\t\t\t*infile_tok = arg_tok;\n+\t\t} else {\n+\t\t\t*infile_tok_first = arg_tok;\n+\t\t\t*infile_tok_last = arg_tok;\n+\t\t}\n+\t\tbreak;\n+\t}\n+}\n+\n+static void process_trailers_lists(struct trailer_item **infile_tok_first,\n+\t\t\t\t struct trailer_item **infile_tok_last,\n+\t\t\t\t struct trailer_item **arg_tok_first)\n+{\n+\tstruct trailer_item *infile_tok;\n+\tstruct trailer_item *arg_tok;\n+\n+\tif (!*arg_tok_first)\n+\t\treturn;\n+\n+\t/* Process infile from end to start */\n+\tfor (infile_tok = *infile_tok_last; infile_tok; infile_tok = infile_tok->previous) {\n+\t\tprocess_infile_tok(infile_tok, arg_tok_first, WHERE_AFTER);\n+\t}\n+\n+\tupdate_last(infile_tok_last);\n+\n+\tif (!*arg_tok_first)\n+\t\treturn;\n+\n+\t/* Process infile from start to end */\n+\tfor (infile_tok = *infile_tok_first; infile_tok; infile_tok = infile_tok->next) {\n+\t\tprocess_infile_tok(infile_tok, arg_tok_first, WHERE_BEFORE);\n+\t}\n+\n+\tupdate_first(infile_tok_first);\n+\n+\t/* Process args left */\n+\twhile (*arg_tok_first) {\n+\t\targ_tok = remove_first(arg_tok_first);\n+\t\tapply_arg_if_missing(infile_tok_first, infile_tok_last, arg_tok);\n+\t}\n+}\n-- \n1.8.5.2.206.g98f5689.dirty\n"},{"id":"234400","messageId":"20140206202004.325.65208.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"20140206194123.325.99451.chriscool@tuxfamily.org","subject":"[PATCH v5 03/14] trailer: read and process config information","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-06T20:19:52Z","receivedAt":"2014-02-06T20:19:52Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This patch implements reading the configuration\nto get trailer information, and then processing\nit and storing it in a doubly linked list.\n\nThe config information is stored in the list\nwhose first item is pointed to by:\n\nstatic struct trailer_item *first_conf_item;\n\nSigned-off-by: Christian Couder \n---\n trailer.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 134 insertions(+)\n\ndiff --git a/trailer.c b/trailer.c\nindex ba0cfe0..f844f46 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -25,6 +25,8 @@ struct trailer_item {\n \tstruct conf_info *conf;\n };\n \n+static struct trailer_item *first_conf_item;\n+\n static inline int same_token(struct trailer_item *a, struct trailer_item *b, int alnum_len)\n {\n \treturn !strncasecmp(a->token, b->token, alnum_len);\n@@ -233,3 +235,135 @@ static void process_trailers_lists(struct trailer_item **infile_tok_first,\n \t\tapply_arg_if_missing(infile_tok_first, infile_tok_last, arg_tok);\n \t}\n }\n+\n+static int set_where(struct conf_info *item, const char *value)\n+{\n+\tif (!strcasecmp(\"after\", value))\n+\t\titem->where = WHERE_AFTER;\n+\telse if (!strcasecmp(\"before\", value))\n+\t\titem->where = WHERE_BEFORE;\n+\telse\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n+static int set_if_exist(struct conf_info *item, const char *value)\n+{\n+\tif (!strcasecmp(\"addIfDifferent\", value))\n+\t\titem->if_exist = EXIST_ADD_IF_DIFFERENT;\n+\telse if (!strcasecmp(\"addIfDifferentNeighbor\", value))\n+\t\titem->if_exist = EXIST_ADD_IF_DIFFERENT_NEIGHBOR;\n+\telse if (!strcasecmp(\"add\", value))\n+\t\titem->if_exist = EXIST_ADD;\n+\telse if (!strcasecmp(\"overwrite\", value))\n+\t\titem->if_exist = EXIST_OVERWRITE;\n+\telse if (!strcasecmp(\"doNothing\", value))\n+\t\titem->if_exist = EXIST_DO_NOTHING;\n+\telse\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n+static int set_if_missing(struct conf_info *item, const char *value)\n+{\n+\tif (!strcasecmp(\"doNothing\", value))\n+\t\titem->if_missing = MISSING_DO_NOTHING;\n+\telse if (!strcasecmp(\"add\", value))\n+\t\titem->if_missing = MISSING_ADD;\n+\telse\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n+enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_WHERE,\n+\t\t\t TRAILER_IF_EXIST, TRAILER_IF_MISSING };\n+\n+static int set_name_and_type(const char *conf_key, const char *suffix,\n+\t\t\t enum trailer_info_type type,\n+\t\t\t char **pname, enum trailer_info_type *ptype)\n+{\n+\tint ret = ends_with(conf_key, suffix);\n+\tif (ret) {\n+\t\t*pname = xstrndup(conf_key, strlen(conf_key) - strlen(suffix));\n+\t\t*ptype = type;\n+\t}\n+\treturn ret;\n+}\n+\n+static struct trailer_item *get_conf_item(const char *name)\n+{\n+\tstruct trailer_item *item;\n+\tstruct trailer_item *previous;\n+\n+\t/* Look up item with same name */\n+\tfor (previous = NULL, item = first_conf_item;\n+\t item;\n+\t previous = item, item = item->next) {\n+\t\tif (!strcasecmp(item->conf->name, name))\n+\t\t\treturn item;\n+\t}\n+\n+\t/* Item does not already exists, create it */\n+\titem = xcalloc(sizeof(struct trailer_item), 1);\n+\titem->conf = xcalloc(sizeof(struct conf_info), 1);\n+\titem->conf->name = xstrdup(name);\n+\n+\tif (!previous)\n+\t\tfirst_conf_item = item;\n+\telse {\n+\t\tprevious->next = item;\n+\t\titem->previous = previous;\n+\t}\n+\n+\treturn item;\n+}\n+\n+static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n+{\n+\tif (starts_with(conf_key, \"trailer.\")) {\n+\t\tconst char *orig_conf_key = conf_key;\n+\t\tstruct trailer_item *item;\n+\t\tstruct conf_info *conf;\n+\t\tchar *name;\n+\t\tenum trailer_info_type type;\n+\n+\t\tconf_key += 8;\n+\t\tif (!set_name_and_type(conf_key, \".key\", TRAILER_KEY, &name, &type) &&\n+\t\t !set_name_and_type(conf_key, \".command\", TRAILER_COMMAND, &name, &type) &&\n+\t\t !set_name_and_type(conf_key, \".where\", TRAILER_WHERE, &name, &type) &&\n+\t\t !set_name_and_type(conf_key, \".ifexist\", TRAILER_IF_EXIST, &name, &type) &&\n+\t\t !set_name_and_type(conf_key, \".ifmissing\", TRAILER_IF_MISSING, &name, &type))\n+\t\t\treturn 0;\n+\n+\t\titem = get_conf_item(name);\n+\t\tconf = item->conf;\n+\n+\t\tswitch (type) {\n+\t\tcase TRAILER_KEY:\n+\t\t\tif (conf->key)\n+\t\t\t\twarning(_(\"more than one %s\"), orig_conf_key);\n+\t\t\tconf->key = xstrdup(value);\n+\t\t\tbreak;\n+\t\tcase TRAILER_COMMAND:\n+\t\t\tif (conf->command)\n+\t\t\t\twarning(_(\"more than one %s\"), orig_conf_key);\n+\t\t\tconf->command = xstrdup(value);\n+\t\t\tbreak;\n+\t\tcase TRAILER_WHERE:\n+\t\t\tif (set_where(conf, value))\n+\t\t\t\twarning(_(\"unknown value '%s' for key '%s'\"), value, orig_conf_key);\n+\t\t\tbreak;\n+\t\tcase TRAILER_IF_EXIST:\n+\t\t\tif (set_if_exist(conf, value))\n+\t\t\t\twarning(_(\"unknown value '%s' for key '%s'\"), value, orig_conf_key);\n+\t\t\tbreak;\n+\t\tcase TRAILER_IF_MISSING:\n+\t\t\tif (set_if_missing(conf, value))\n+\t\t\t\twarning(_(\"unknown value '%s' for key '%s'\"), value, orig_conf_key);\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tdie(\"internal bug in trailer.c\");\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n-- \n1.8.5.2.206.g98f5689.dirty\n"},{"id":"234412","messageId":"20140206202004.325.93876.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"20140206194123.325.99451.chriscool@tuxfamily.org","subject":"[PATCH v5 04/14] trailer: process command line trailer arguments","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-06T20:19:53Z","receivedAt":"2014-02-06T20:19:53Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This patch parses the trailer command line arguments\nand put the result into an arg_tok doubly linked\nlist.\n\nSigned-off-by: Christian Couder \n---\n trailer.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 84 insertions(+)\n\ndiff --git a/trailer.c b/trailer.c\nindex f844f46..3fde21d 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -367,3 +367,87 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n \t}\n \treturn 0;\n }\n+\n+static void parse_trailer(struct strbuf *tok, struct strbuf *val, const char *trailer)\n+{\n+\tconst char *end = strchr(trailer, '=');\n+\tif (!end)\n+\t\tend = strchr(trailer, ':');\n+\tif (end) {\n+\t\tstrbuf_add(tok, trailer, end - trailer);\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, trailer);\n+\t\tstrbuf_trim(tok);\n+\t}\n+}\n+\n+static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,\n+\t\t\t\t\t const char* tok, const char* val)\n+{\n+\tstruct trailer_item *new = xcalloc(sizeof(*new), 1);\n+\tnew->value = val;\n+\n+\tif (conf_item) {\n+\t\tnew->conf = conf_item->conf;\n+\t\tnew->token = xstrdup(conf_item->conf->key);\n+\t} else {\n+\t\tnew->conf = xcalloc(sizeof(struct conf_info), 1);\n+\t\tnew->token = tok;\n+\t}\n+\n+\treturn new;\n+}\n+\n+static struct trailer_item *create_trailer_item(const char *string)\n+{\n+\tstruct strbuf tok = STRBUF_INIT;\n+\tstruct strbuf val = STRBUF_INIT;\n+\tstruct trailer_item *item;\n+\tint tok_alnum_len;\n+\n+\tparse_trailer(&tok, &val, string);\n+\n+\ttok_alnum_len = alnum_len(tok.buf, tok.len);\n+\n+\t/* Lookup if the token matches something in the config */\n+\tfor (item = first_conf_item; item; item = item->next) {\n+\t\tif (!strncasecmp(tok.buf, item->conf->key, tok_alnum_len) ||\n+\t\t !strncasecmp(tok.buf, item->conf->name, tok_alnum_len)) {\n+\t\t\tstrbuf_release(&tok);\n+\t\t\treturn new_trailer_item(item, NULL, strbuf_detach(&val, NULL));\n+\t\t}\n+\t}\n+\n+\treturn new_trailer_item(NULL, strbuf_detach(&tok, NULL), strbuf_detach(&val, NULL));;\n+}\n+\n+static void add_trailer_item(struct trailer_item **first,\n+\t\t\t struct trailer_item **last,\n+\t\t\t struct trailer_item *new)\n+{\n+\tif (!*last) {\n+\t\t*first = new;\n+\t\t*last = new;\n+\t} else {\n+\t\t(*last)->next = new;\n+\t\tnew->previous = *last;\n+\t\t*last = new;\n+\t}\n+}\n+\n+static struct trailer_item *process_command_line_args(int argc, const char **argv)\n+{\n+\tint i;\n+\tstruct trailer_item *arg_tok_first = NULL;\n+\tstruct trailer_item *arg_tok_last = NULL;\n+\n+\tfor (i = 0; i < argc; i++) {\n+\t\tstruct trailer_item *new = create_trailer_item(argv[i]);\n+\t\tadd_trailer_item(&arg_tok_first, &arg_tok_last, new);\n+\t}\n+\n+\treturn arg_tok_first;\n+}\n-- \n1.8.5.2.206.g98f5689.dirty\n"},{"id":"234409","messageId":"20140206202004.325.52337.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"20140206194123.325.99451.chriscool@tuxfamily.org","subject":"[PATCH v5 05/14] trailer: parse trailers from input file","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-06T20:19:54Z","receivedAt":"2014-02-06T20:19:54Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This patch reads trailers from an input file, parses\nthem and puts the result into a doubly linked list.\n\nSigned-off-by: Christian Couder \n---\n trailer.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 71 insertions(+)\n\ndiff --git a/trailer.c b/trailer.c\nindex 3fde21d..f59efd1 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -49,6 +49,13 @@ static inline size_t alnum_len(const char *buf, int len)\n \treturn (size_t) len + 1;\n }\n \n+static inline int contains_only_spaces(const char *str)\n+{\n+\tconst char *s;\n+\tfor (s = str; *s && isspace(*s); s++);\n+\treturn !*s;\n+}\n+\n static void add_arg_to_infile(struct trailer_item *infile_tok,\n \t\t\t struct trailer_item *arg_tok)\n {\n@@ -451,3 +458,67 @@ static struct trailer_item *process_command_line_args(int argc, const char **arg\n \n \treturn arg_tok_first;\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 (contains_only_spaces(lines[start]->buf)) {\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 process_input_file(const char *infile,\n+\t\t\t struct trailer_item **infile_tok_first,\n+\t\t\t struct trailer_item **infile_tok_last)\n+{\n+\tstruct strbuf **lines = read_input_file(infile);\n+\tint start = find_trailer_start(lines);\n+\tint i;\n+\n+\t/* Print 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/* Parse trailer lines */\n+\tfor (i = start; lines[i]; i++) {\n+\t\tstruct trailer_item *new = create_trailer_item(lines[i]->buf);\n+\t\tadd_trailer_item(infile_tok_first, infile_tok_last, new);\n+\t}\n+\n+\tstrbuf_list_free(lines);\n+}\n-- \n1.8.5.2.206.g98f5689.dirty\n"},{"id":"234406","messageId":"20140206202004.325.31354.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"20140206194123.325.99451.chriscool@tuxfamily.org","subject":"[PATCH v5 06/14] trailer: put all the processing together and print","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-06T20:19:55Z","receivedAt":"2014-02-06T20:19:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This patch adds the process_trailers() function that\ncalls all the previously added processing functions\nand then prints the results on the standard output.\n\nSigned-off-by: Christian Couder \n---\n trailer.c | 40 ++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 40 insertions(+)\n\ndiff --git a/trailer.c b/trailer.c\nindex f59efd1..186316f 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -56,6 +56,26 @@ static inline int contains_only_spaces(const char *str)\n \treturn !*s;\n }\n \n+static void print_tok_val(const char *tok, const char *val)\n+{\n+\tchar c = tok[strlen(tok) - 1];\n+\tif (isalnum(c))\n+\t\tprintf(\"%s: %s\\n\", tok, val);\n+\telse if (isspace(c) || c == '#')\n+\t\tprintf(\"%s%s\\n\", tok, val);\n+\telse\n+\t\tprintf(\"%s %s\\n\", tok, val);\n+}\n+\n+static void print_all(struct trailer_item *first, int trim_empty)\n+{\n+\tstruct trailer_item *item;\n+\tfor (item = first; item; item = item->next) {\n+\t\tif (!trim_empty || strlen(item->value) > 0)\n+\t\t\tprint_tok_val(item->token, item->value);\n+\t}\n+}\n+\n static void add_arg_to_infile(struct trailer_item *infile_tok,\n \t\t\t struct trailer_item *arg_tok)\n {\n@@ -522,3 +542,23 @@ static void process_input_file(const char *infile,\n \n \tstrbuf_list_free(lines);\n }\n+\n+void process_trailers(const char *infile, int trim_empty, int argc, const char **argv)\n+{\n+\tstruct trailer_item *infile_tok_first = NULL;\n+\tstruct trailer_item *infile_tok_last = NULL;\n+\tstruct trailer_item *arg_tok_first;\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, &infile_tok_first, &infile_tok_last);\n+\t}\n+\n+\targ_tok_first = process_command_line_args(argc, argv);\n+\n+\tprocess_trailers_lists(&infile_tok_first, &infile_tok_last, &arg_tok_first);\n+\n+\tprint_all(infile_tok_first, trim_empty);\n+}\n-- \n1.8.5.2.206.g98f5689.dirty\n"},{"id":"234403","messageId":"20140206202004.325.48784.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"20140206194123.325.99451.chriscool@tuxfamily.org","subject":"[PATCH v5 07/14] trailer: add interpret-trailers command","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-06T20:19:56Z","receivedAt":"2014-02-06T20:19:56Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This patch adds the \"git interpret-trailers\" command.\nThis command uses the previously added process_trailers()\nfunction in trailer.c.\n\nSigned-off-by: Christian Couder \n---\n .gitignore | 1 +\n Makefile | 1 +\n builtin.h | 1 +\n builtin/interpret-trailers.c | 36 ++++++++++++++++++++++++++++++++++++\n git.c | 1 +\n trailer.h | 6 ++++++\n 6 files changed, 46 insertions(+)\n create mode 100644 builtin/interpret-trailers.c\n create mode 100644 trailer.h\n\ndiff --git a/.gitignore b/.gitignore\nindex b5f9def..c870ada 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -74,6 +74,7 @@\n /git-index-pack\n /git-init\n /git-init-db\n+/git-interpret-trailers\n /git-instaweb\n /git-log\n /git-ls-files\ndiff --git a/Makefile b/Makefile\nindex ec90feb..a91465e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -935,6 +935,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 d4afbfe..30f4c30 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..04b0ae2\n--- /dev/null\n+++ b/builtin/interpret-trailers.c\n@@ -0,0 +1,36 @@\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+#include \"trailer.h\"\n+\n+static const char * const git_interpret_trailers_usage[] = {\n+\tN_(\"git interpret-trailers [--trim-empty] [--infile=] [([(=|:)])...]\"),\n+\tNULL\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+\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+\tprocess_trailers(infile, trim_empty, argc, argv);\n+\n+\treturn 0;\n+}\ndiff --git a/git.c b/git.c\nindex 3799514..1420b58 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/trailer.h b/trailer.h\nnew file mode 100644\nindex 0000000..9db4459\n--- /dev/null\n+++ b/trailer.h\n@@ -0,0 +1,6 @@\n+#ifndef TRAILER_H\n+#define TRAILER_H\n+\n+void process_trailers(const char *infile, int trim_empty, int argc, const char **argv);\n+\n+#endif /* TRAILER_H */\n-- \n1.8.5.2.206.g98f5689.dirty\n"},{"id":"234401","messageId":"20140206202004.325.20315.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"20140206194123.325.99451.chriscool@tuxfamily.org","subject":"[PATCH v5 08/14] trailer: add tests for \"git interpret-trailers\"","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-06T20:19:57Z","receivedAt":"2014-02-06T20:19:57Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n t/t7513-interpret-trailers.sh | 208 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 208 insertions(+)\n create mode 100755 t/t7513-interpret-trailers.sh\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nnew file mode 100755\nindex 0000000..8be333c\n--- /dev/null\n+++ b/t/t7513-interpret-trailers.sh\n@@ -0,0 +1,208 @@\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+# We want one trailing space at the end of each line.\n+# Let's use sed to make sure that these spaces are not removed\n+# by any automatic tool.\n+sed -e 's/ Z$/ /' >complex_message_trailers <<-\\EOF\n+Fixes: Z\n+Acked-by: Z\n+Reviewed-by: Z\n+Signed-off-by: Z\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.key \"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.key \"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.key \"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 \"Fixes: \\nAcked-by= \\nAcked-by= Peff\\nReviewed-by: \\nSigned-off-by: \\nBug #42\\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 \"Acked-by= Peff\\nBug #42\\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_expect_success 'using \"where = before\"' '\n+\tgit config trailer.bug.where \"before\" &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Bug #42\\nFixes: \\nAcked-by= \\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 'using \"where = before\" for a token in the middle of infile' '\n+\tgit config trailer.review.key \"Reviewed-by:\" &&\n+\tgit config trailer.review.where \"before\" &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Bug #42\\nFixes: \\nAcked-by= \\nAcked-by= Peff\\nReviewed-by: Johan\\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n+\tgit interpret-trailers --infile complex_message \"ack: Peff\" \"bug: 42\" \"review: Johan\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'using \"where = before\" and --trim-empty' '\n+\tcat complex_message_body >expected &&\n+\tprintf \"Bug #46\\nBug #42\\nAcked-by= Peff\\nReviewed-by: Johan\\n\" >>expected &&\n+\tgit interpret-trailers --infile complex_message --trim-empty \"ack: Peff\" \"bug: 42\" \"review: Johan\" \"Bug: 46\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'the default is \"ifExist = addIfDifferent\"' '\n+\tcat complex_message_body >expected &&\n+\tprintf \"Bug #42\\nFixes: \\nAcked-by= \\nAcked-by= Peff\\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n+\tgit interpret-trailers --infile complex_message \"ack: Peff\" \"review:\" \"bug: 42\" \"ack: Peff\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'using \"ifExist = addIfDifferent\"' '\n+\tgit config trailer.review.ifExist \"addIfDifferent\" &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Bug #42\\nFixes: \\nAcked-by= \\nAcked-by= Peff\\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n+\tgit interpret-trailers --infile complex_message \"ack: Peff\" \"review:\" \"bug: 42\" \"ack: Peff\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'using \"ifExist = addIfDifferentNeighbor\"' '\n+\tgit config trailer.ack.ifExist \"addIfDifferentNeighbor\" &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Bug #42\\nFixes: \\nAcked-by= \\nAcked-by= Peff\\nAcked-by= Junio\\nAcked-by= Peff\\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n+\tgit interpret-trailers --infile complex_message \"ack: Peff\" \"review:\" \"ack: Junio\" \"bug: 42\" \"ack: Peff\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'using \"ifExist = addIfDifferentNeighbor\" and --trim-empty' '\n+\tgit config trailer.ack.ifExist \"addIfDifferentNeighbor\" &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Bug #42\\nAcked-by= Peff\\nAcked-by= Junio\\nAcked-by= Peff\\n\" >>expected &&\n+\tgit interpret-trailers --infile complex_message --trim-empty \"ack: Peff\" \"Acked-by= Peff\" \"review:\" \"ack: Junio\" \"bug: 42\" \"ack: Peff\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'using \"ifExist = add\"' '\n+\tgit config trailer.ack.ifExist \"add\" &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Bug #42\\nFixes: \\nAcked-by= \\nAcked-by= Peff\\nAcked-by= Peff\\nAcked-by= Junio\\nAcked-by= Peff\\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n+\tgit interpret-trailers --infile complex_message \"ack: Peff\" \"Acked-by= Peff\" \"review:\" \"ack: Junio\" \"bug: 42\" \"ack: Peff\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'using \"ifExist = overwrite\"' '\n+\tgit config trailer.fix.key \"Fixes:\" &&\n+\tgit config trailer.fix.ifExist \"overwrite\" &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Bug #42\\nFixes: 22\\nAcked-by= \\nAcked-by= Junio\\nAcked-by= Peff\\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n+\tgit interpret-trailers --infile complex_message \"review:\" \"fix=53\" \"ack: Junio\" \"fix=22\" \"bug: 42\" \"ack: Peff\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'using \"ifExist = doNothing\"' '\n+\tgit config trailer.fix.ifExist \"doNothing\" &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Bug #42\\nFixes: \\nAcked-by= \\nAcked-by= Junio\\nAcked-by= Peff\\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n+\tgit interpret-trailers --infile complex_message \"review:\" \"fix=53\" \"ack: Junio\" \"fix=22\" \"bug: 42\" \"ack: Peff\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'the default is \"ifMissing = add\"' '\n+\tgit config trailer.cc.key \"Cc: \" &&\n+\tgit config trailer.Cc.where \"before\" &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Bug #42\\nCc: Linus\\nFixes: \\nAcked-by= \\nAcked-by= Junio\\nAcked-by= Peff\\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n+\tgit interpret-trailers --infile complex_message \"review:\" \"fix=53\" \"cc=Linus\" \"ack: Junio\" \"fix=22\" \"bug: 42\" \"ack: Peff\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'using \"ifMissing = add\"' '\n+\tgit config trailer.Cc.ifMissing \"add\" &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Cc: Linus\\nBug #42\\nFixes: \\nAcked-by= \\nAcked-by= Junio\\nAcked-by= Peff\\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n+\tgit interpret-trailers --infile complex_message \"review:\" \"fix=53\" \"ack: Junio\" \"fix=22\" \"bug: 42\" \"cc=Linus\" \"ack: Peff\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'using \"ifMissing = doNothing\"' '\n+\tgit config trailer.Cc.ifMissing \"doNothing\" &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Bug #42\\nFixes: \\nAcked-by= \\nAcked-by= Junio\\nAcked-by= Peff\\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n+\tgit interpret-trailers --infile complex_message \"review:\" \"fix=53\" \"cc=Linus\" \"ack: Junio\" \"fix=22\" \"bug: 42\" \"ack: Peff\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_done\n-- \n1.8.5.2.206.g98f5689.dirty\n"},{"id":"234407","messageId":"20140206202004.325.76199.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"20140206194123.325.99451.chriscool@tuxfamily.org","subject":"[PATCH v5 09/14] trailer: if no input file is passed, read from stdin","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-06T20:19:58Z","receivedAt":"2014-02-06T20:19:58Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"It is simpler and more natural if the \"git interpret-trailers\"\nis made a filter as its output already goes to sdtout.\n\nSigned-off-by: Christian Couder \n---\n builtin/interpret-trailers.c | 2 +-\n t/t7513-interpret-trailers.sh | 7 +++++++\n trailer.c | 15 +++++++++------\n 3 files changed, 17 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex 04b0ae2..ae8e561 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -23,7 +23,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\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_FILENAME(0, \"infile\", &infile, N_(\"use message from file, instead of stdin\")),\n \t\tOPT_END()\n \t};\n \ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 8be333c..f5ef81f 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -205,4 +205,11 @@ test_expect_success 'using \"ifMissing = doNothing\"' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'with input from stdin' '\n+\tcat complex_message_body >expected &&\n+\tprintf \"Bug #42\\nFixes: \\nAcked-by= \\nAcked-by= Junio\\nAcked-by= Peff\\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n+\tgit interpret-trailers \"review:\" \"fix=53\" \"cc=Linus\" \"ack: Junio\" \"fix=22\" \"bug: 42\" \"ack: Peff\" < complex_message >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\ndiff --git a/trailer.c b/trailer.c\nindex 186316f..108e104 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -483,8 +483,13 @@ 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+\tif (infile) {\n+\t\tif (strbuf_read_file(&sb, infile, 0) < 0)\n+\t\t\tdie_errno(_(\"could not read input file '%s'\"), infile);\n+\t} else {\n+\t\tif (strbuf_read(&sb, fileno(stdin), 0) < 0)\n+\t\t\tdie_errno(_(\"could not read from stdin\"));\n+\t}\n \n \treturn strbuf_split(&sb, '\\n');\n }\n@@ -551,10 +556,8 @@ void process_trailers(const char *infile, int trim_empty, int argc, const char *\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, &infile_tok_first, &infile_tok_last);\n-\t}\n+\t/* Print the non trailer part of infile (or stdin if infile is NULL) */\n+\tprocess_input_file(infile, &infile_tok_first, &infile_tok_last);\n \n \targ_tok_first = process_command_line_args(argc, argv);\n \n-- \n1.8.5.2.206.g98f5689.dirty\n"},{"id":"234411","messageId":"20140206202004.325.54178.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"20140206194123.325.99451.chriscool@tuxfamily.org","subject":"[PATCH v5 10/14] trailer: execute command from 'trailer..command'","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-06T20:19:59Z","receivedAt":"2014-02-06T20:19:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n trailer.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 63 insertions(+)\n\ndiff --git a/trailer.c b/trailer.c\nindex 108e104..98187fc 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"run-command.h\"\n /*\n * Copyright (c) 2013 Christian Couder \n */\n@@ -12,11 +13,14 @@ struct conf_info {\n \tchar *name;\n \tchar *key;\n \tchar *command;\n+\tunsigned command_uses_arg : 1;\n \tenum action_where where;\n \tenum action_if_exist if_exist;\n \tenum action_if_missing if_missing;\n };\n \n+#define TRAILER_ARG_STRING \"$ARG\"\n+\n struct trailer_item {\n \tstruct trailer_item *previous;\n \tstruct trailer_item *next;\n@@ -56,6 +60,13 @@ static inline int contains_only_spaces(const char *str)\n \treturn !*s;\n }\n \n+static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *b)\n+{\n+\tconst char *ptr = strstr(sb->buf, a);\n+\tif (ptr)\n+\t\tstrbuf_splice(sb, ptr - sb->buf, strlen(a), b, strlen(b));\n+}\n+\n static void print_tok_val(const char *tok, const char *val)\n {\n \tchar c = tok[strlen(tok) - 1];\n@@ -375,6 +386,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n \t\t\tif (conf->command)\n \t\t\t\twarning(_(\"more than one %s\"), orig_conf_key);\n \t\t\tconf->command = xstrdup(value);\n+\t\t\tconf->command_uses_arg = !!strstr(conf->command, TRAILER_ARG_STRING);\n \t\t\tbreak;\n \t\tcase TRAILER_WHERE:\n \t\t\tif (set_where(conf, value))\n@@ -411,6 +423,45 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val, const char *tr\n \t}\n }\n \n+static int read_from_command(struct child_process *cp, struct strbuf *buf)\n+{\n+\tif (run_command(cp))\n+\t\treturn error(\"running trailer command '%s' failed\", cp->argv[0]);\n+\tif (strbuf_read(buf, cp->out, 1024) < 1)\n+\t\treturn error(\"reading from trailer command '%s' failed\", cp->argv[0]);\n+\tstrbuf_trim(buf);\n+\treturn 0;\n+}\n+\n+static const char *apply_command(const char *command, const char *arg)\n+{\n+\tstruct strbuf cmd = STRBUF_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct child_process cp;\n+\tconst char *argv[] = {NULL, NULL};\n+\tconst char *result = \"\";\n+\n+\tstrbuf_addstr(&cmd, command);\n+\tif (arg)\n+\t\tstrbuf_replace(&cmd, TRAILER_ARG_STRING, arg);\n+\n+\targv[0] = cmd.buf;\n+\tmemset(&cp, 0, sizeof(cp));\n+\tcp.argv = argv;\n+\tcp.env = local_repo_env;\n+\tcp.no_stdin = 1;\n+\tcp.out = -1;\n+\tcp.use_shell = 1;\n+\n+\tif (read_from_command(&cp, &buf))\n+\t\tstrbuf_release(&buf);\n+\telse\n+\t\tresult = strbuf_detach(&buf, NULL);\n+\n+\tstrbuf_release(&cmd);\n+\treturn result;\n+}\n+\n static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,\n \t\t\t\t\t const char* tok, const char* val)\n {\n@@ -420,6 +471,8 @@ static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,\n \tif (conf_item) {\n \t\tnew->conf = conf_item->conf;\n \t\tnew->token = xstrdup(conf_item->conf->key);\n+\t\tif (conf_item->conf->command_uses_arg || !val)\n+\t\t\tnew->value = apply_command(conf_item->conf->command, val);\n \t} else {\n \t\tnew->conf = xcalloc(sizeof(struct conf_info), 1);\n \t\tnew->token = tok;\n@@ -470,12 +523,22 @@ static struct trailer_item *process_command_line_args(int argc, const char **arg\n \tint i;\n \tstruct trailer_item *arg_tok_first = NULL;\n \tstruct trailer_item *arg_tok_last = NULL;\n+\tstruct trailer_item *item;\n \n \tfor (i = 0; i < argc; i++) {\n \t\tstruct trailer_item *new = create_trailer_item(argv[i]);\n \t\tadd_trailer_item(&arg_tok_first, &arg_tok_last, new);\n \t}\n \n+\t/* Add conf commands that don't use $ARG */\n+\tfor (item = first_conf_item; item; item = item->next) {\n+\t\tif (item->conf->command && !item->conf->command_uses_arg)\n+\t\t{\n+\t\t\tstruct trailer_item *new = new_trailer_item(item, NULL, NULL);\n+\t\t\tadd_trailer_item(&arg_tok_first, &arg_tok_last, new);\n+\t\t}\n+\t}\n+\n \treturn arg_tok_first;\n }\n \n-- \n1.8.5.2.206.g98f5689.dirty\n"},{"id":"234399","messageId":"20140206202004.325.6993.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"20140206194123.325.99451.chriscool@tuxfamily.org","subject":"[PATCH v5 11/14] trailer: add tests for trailer command","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-06T20:20:00Z","receivedAt":"2014-02-06T20:20:00Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n t/t7513-interpret-trailers.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex f5ef81f..2d50b7a 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -212,4 +212,31 @@ test_expect_success 'with input from stdin' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'with simple command' '\n+\tgit config trailer.sign.key \"Signed-off-by: \" &&\n+\tgit config trailer.sign.where \"after\" &&\n+\tgit config trailer.sign.ifExist \"addIfDifferentNeighbor\" &&\n+\tgit config trailer.sign.command \"echo \\\"A U Thor \\\"\" &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Fixes: \\nAcked-by= \\nReviewed-by: \\nSigned-off-by: \\nSigned-off-by: A U Thor \\n\" >>expected &&\n+\tgit interpret-trailers \"review:\" \"fix=22\" < complex_message >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'setup a commit' '\n+\techo \"Content of the first commit.\" > a.txt &&\n+\tgit add a.txt &&\n+\tgit commit -m \"Add file a.txt\"\n+'\n+\n+test_expect_success 'with command using $ARG' '\n+\tgit config trailer.fix.ifExist \"overwrite\" &&\n+\tgit config trailer.fix.command \"git log -1 --oneline --format=\\\"%h (%s)\\\" --abbrev-commit --abbrev=14 \\$ARG\" &&\n+\tFIXED=$(git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 HEAD) &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Fixes: $FIXED\\nAcked-by= \\nReviewed-by: \\nSigned-off-by: \\nSigned-off-by: A U Thor \\n\" >>expected &&\n+\tgit interpret-trailers \"review:\" \"fix=HEAD\" < complex_message >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.8.5.2.206.g98f5689.dirty\n"},{"id":"234405","messageId":"20140206202004.325.20148.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"20140206194123.325.99451.chriscool@tuxfamily.org","subject":"[PATCH v5 12/14] trailer: set author and committer env variables","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-06T20:20:01Z","receivedAt":"2014-02-06T20:20:01Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n trailer.c | 30 +++++++++++++++++++++++++++++-\n 1 file changed, 29 insertions(+), 1 deletion(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 98187fc..b5de616 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"run-command.h\"\n+#include \"argv-array.h\"\n /*\n * Copyright (c) 2013 Christian Couder \n */\n@@ -433,14 +434,40 @@ static int read_from_command(struct child_process *cp, struct strbuf *buf)\n \treturn 0;\n }\n \n+static void setup_ac_env(struct argv_array *env, const char *ac_name, const char *ac_mail, const char *(*read)(int))\n+{\n+\tif (!getenv(ac_name) || !getenv(ac_mail)) {\n+\t\tstruct ident_split ident;\n+\t\tconst char *namebuf, *mailbuf;\n+\t\tint namelen, maillen;\n+\t\tconst char *ac_info = read(IDENT_NO_DATE);\n+\n+\t\tif (split_ident_line(&ident, ac_info, strlen(ac_info)))\n+\t\t\treturn;\n+\n+\t\tnamelen = ident.name_end - ident.name_begin;\n+\t\tnamebuf = ident.name_begin;\n+\n+\t\tmaillen = ident.mail_end - ident.mail_begin;\n+\t\tmailbuf = ident.mail_begin;\n+\n+\t\targv_array_pushf(env, \"%s=%.*s\", ac_name, namelen, namebuf);\n+\t\targv_array_pushf(env, \"%s=%.*s\", ac_mail, maillen, mailbuf);\n+\t}\n+}\n+\n static const char *apply_command(const char *command, const char *arg)\n {\n+\tstruct argv_array env = ARGV_ARRAY_INIT;\n \tstruct strbuf cmd = STRBUF_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct child_process cp;\n \tconst char *argv[] = {NULL, NULL};\n \tconst char *result = \"\";\n \n+\tsetup_ac_env(&env, \"GIT_AUTHOR_NAME\", \"GIT_AUTHOR_EMAIL\", git_author_info);\n+\tsetup_ac_env(&env, \"GIT_COMMITTER_NAME\", \"GIT_COMMITTER_EMAIL\", git_committer_info);\n+\n \tstrbuf_addstr(&cmd, command);\n \tif (arg)\n \t\tstrbuf_replace(&cmd, TRAILER_ARG_STRING, arg);\n@@ -448,7 +475,7 @@ static const char *apply_command(const char *command, const char *arg)\n \targv[0] = cmd.buf;\n \tmemset(&cp, 0, sizeof(cp));\n \tcp.argv = argv;\n-\tcp.env = local_repo_env;\n+\tcp.env = env.argv;\n \tcp.no_stdin = 1;\n \tcp.out = -1;\n \tcp.use_shell = 1;\n@@ -459,6 +486,7 @@ static const char *apply_command(const char *command, const char *arg)\n \t\tresult = strbuf_detach(&buf, NULL);\n \n \tstrbuf_release(&cmd);\n+\targv_array_clear(&env);\n \treturn result;\n }\n \n-- \n1.8.5.2.206.g98f5689.dirty\n"},{"id":"234402","messageId":"20140206202004.325.2983.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"20140206194123.325.99451.chriscool@tuxfamily.org","subject":"[PATCH v5 13/14] trailer: add tests for commands using env variables","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-06T20:20:02Z","receivedAt":"2014-02-06T20:20:02Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n t/t7513-interpret-trailers.sh | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 2d50b7a..00894a8 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -223,6 +223,26 @@ test_expect_success 'with simple command' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'with command using commiter information' '\n+\tgit config trailer.sign.ifExist \"addIfDifferent\" &&\n+\tgit config trailer.sign.command \"echo \\\"\\$GIT_COMMITTER_NAME <\\$GIT_COMMITTER_EMAIL>\\\"\" &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Fixes: \\nAcked-by= \\nReviewed-by: \\nSigned-off-by: \\nSigned-off-by: C O Mitter \\n\" >>expected &&\n+\tgit interpret-trailers \"review:\" \"fix=22\" < complex_message >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'with command using author information' '\n+\tgit config trailer.sign.key \"Signed-off-by: \" &&\n+\tgit config trailer.sign.where \"after\" &&\n+\tgit config trailer.sign.ifExist \"addIfDifferentNeighbor\" &&\n+\tgit config trailer.sign.command \"echo \\\"\\$GIT_AUTHOR_NAME <\\$GIT_AUTHOR_EMAIL>\\\"\" &&\n+\tcat complex_message_body >expected &&\n+\tprintf \"Fixes: \\nAcked-by= \\nReviewed-by: \\nSigned-off-by: \\nSigned-off-by: A U Thor \\n\" >>expected &&\n+\tgit interpret-trailers \"review:\" \"fix=22\" < complex_message >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'setup a commit' '\n \techo \"Content of the first commit.\" > a.txt &&\n \tgit add a.txt &&\n-- \n1.8.5.2.206.g98f5689.dirty\n"},{"id":"234404","messageId":"20140206202004.325.93939.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"20140206194123.325.99451.chriscool@tuxfamily.org","subject":"[PATCH v5 14/14] Documentation: add documentation for 'git interpret-trailers'","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-06T20:20:03Z","receivedAt":"2014-02-06T20:20:03Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Signed-off-by: Christian Couder \n---\n Documentation/git-interpret-trailers.txt | 132 +++++++++++++++++++++++++++++++\n 1 file changed, 132 insertions(+)\n create mode 100644 Documentation/git-interpret-trailers.txt\n\ndiff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt\nnew file mode 100644\nindex 0000000..0617941\n--- /dev/null\n+++ b/Documentation/git-interpret-trailers.txt\n@@ -0,0 +1,132 @@\n+git-interpret-trailers(1)\n+=========================\n+\n+NAME\n+----\n+git-interpret-trailers - help add stuctured information into commit messages\n+\n+SYNOPSIS\n+--------\n+[verse]\n+'git interpret-trailers' [--trim-empty] [--infile=] [([(=|:)])...]\n+\n+DESCRIPTION\n+-----------\n+Help add RFC 822-like headers, called 'trailers', at the end of the\n+otherwise free-form part of a commit message.\n+\n+Unless `--infile=` is used, this command is a filter. It reads\n+the standard input for a commit message and applies the `token`\n+arguments, if any, to this message. The resulting message is emited on\n+the standard output.\n+\n+Some configuration variables control the way the `token` arguments are\n+applied to the message and the way any existing trailer in the message\n+is changed. They also make it possible to automatically add some\n+trailers.\n+\n+By default, a 'token=value' or 'token:value' argument will be added\n+only if no trailer with the same (token, value) pair is already in the\n+message. The 'token' and 'value' parts will be trimmed to remove\n+starting and trailing whitespace, and the resulting trimmed 'token'\n+and 'value' will appear in the message like this:\n+\n+------------------------------------------------\n+token: value\n+------------------------------------------------\n+\n+By default, if there are already trailers with the same 'token' the\n+new trailer will appear just after the last trailer with the same\n+'token'. Otherwise it will appear at the end of the message.\n+\n+Note that 'trailers' do not follow and are not intended to follow many\n+rules that are in RFC 822. For example they do not follow the line\n+breaking rules, the encoding rules and probably many other rules.\n+\n+OPTIONS\n+-------\n+--trim-empty::\n+\tIf the 'value' part of any trailer contains only whitespace,\n+\tthe whole trailer will be removed from the resulting message.\n+\n+----infile=::\n+\tRead the commit message from `file` instead of the standard\n+\tinput.\n+\n+CONFIGURATION VARIABLES\n+-----------------------\n+\n+trailer..key::\n+\tThis 'key' will be used instead of 'token' in the\n+\ttrailer. After some alphanumeric characters, it can contain\n+\tsome non alphanumeric characters like ':', '=' or '#' that will\n+\tbe used instead of ':' to separate the token from the value in\n+\tthe trailer, though the default ':' is more standard.\n+\n+trailer..where::\n+\tThis can be either `after`, which is the default, or\n+\t`before`. If it is `before`, then a trailer with the specified\n+\ttoken, will appear before, instead of after, other trailers\n+\twith the same token, or otherwise at the beginning, instead of\n+\tat the end, of all the trailers.\n+\n+trailer..ifexist::\n+\tThis option makes it possible to choose what action will be\n+\tperformed when there is already at least one trailer with the\n+\tsame token in the message.\n++\n+The valid values for this option are: `addIfDifferent` (this is the\n+default), `addIfDifferentNeighbor`, `add`, `overwrite` or `doNothing`.\n++\n+With `addIfDifferent`, a new trailer will be added only if no trailer\n+with the same (token, value) pair is already in the message.\n++\n+With `addIfDifferentNeighbor`, a new trailer will be added only if no\n+trailer with the same (token, value) pair is above or below the line\n+where the new trailer will be added.\n++\n+With `add`, a new trailer will be added, even if some trailers with\n+the same (token, value) pair are already in the message.\n++\n+With `overwrite`, the new trailer will overwrite an existing trailer\n+with the same token.\n++\n+With `doNothing`, nothing will be done, that is no new trailer will be\n+added if there is already one with the same token in the message.\n+\n+trailer..ifmissing::\n+\tThis option makes it possible to choose what action will be\n+\tperformed when there is not yet any trailer with the same\n+\ttoken in the message.\n++\n+The valid values for this option are: `add` (this is the default) and\n+`doNothing`.\n++\n+With `add`, a new trailer will be added.\n++\n+With `doNothing`, nothing will be done.\n+\n+trailer..command::\n+\tThis option can be used to specify a shell command that will\n+\tbe used to automatically add or modify a trailer with the\n+\tspecified 'token'.\n++\n+When this option is specified, it is like if a special 'token=value'\n+argument is added at the end of the command line, where 'value' will\n+be given by the standard output of the specified command.\n++\n+If the command contains the `$ARG` string, this string will be\n+replaced with the 'value' part of an existing trailer with the same\n+token, if any, before the command is launched.\n++\n+The following environment variables are set when the command is run:\n+GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, GIT_COMMITTER_NAME,\n+GIT_COMMITTER_EMAIL.\n+\n+SEE ALSO\n+--------\n+linkgit:git-commit[1]\n+\n+GIT\n+---\n+Part of the linkgit:git[1] suite\n-- \n1.8.5.2.206.g98f5689.dirty\n"},{"id":"234420","messageId":"xmqqzjm3x0v3.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"20140206202004.325.80557.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 01/14] Add data structures and basic functions for commit trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-06T23:44:32Z","receivedAt":"2014-02-06T23:44:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> We will use a doubly linked list to store all information\n> about trailers and their configuration.\n>\n> This way we can easily remove or add trailers to or from\n> trailer lists while traversing the lists in either direction.\n>\n> Signed-off-by: Christian Couder \n> ---\n> Makefile | 1 +\n> trailer.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 49 insertions(+)\n> create mode 100644 trailer.c\n>\n> diff --git a/Makefile b/Makefile\n> index b4af1e2..ec90feb 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -871,6 +871,7 @@ LIB_OBJS += submodule.o\n> LIB_OBJS += symlinks.o\n> LIB_OBJS += tag.o\n> LIB_OBJS += trace.o\n> +LIB_OBJS += trailer.o\n> LIB_OBJS += transport.o\n> LIB_OBJS += transport-helper.o\n> LIB_OBJS += tree-diff.o\n> diff --git a/trailer.c b/trailer.c\n> new file mode 100644\n> index 0000000..f129b5a\n> --- /dev/null\n> +++ b/trailer.c\n> @@ -0,0 +1,48 @@\n> +#include \"cache.h\"\n> +/*\n> + * Copyright (c) 2013 Christian Couder \n> + */\n> +\n> +enum action_where { WHERE_AFTER, WHERE_BEFORE };\n> +enum action_if_exist { EXIST_ADD_IF_DIFFERENT, EXIST_ADD_IF_DIFFERENT_NEIGHBOR,\n> +\t\t EXIST_ADD, EXIST_OVERWRITE, EXIST_DO_NOTHING };\n> +enum action_if_missing { MISSING_ADD, MISSING_DO_NOTHING };\n\nAll these names and \"conf_info\" below are not named to be specific\nto this little tool. Can I assume that these will never be exposed\nto the rest of the system? If so, they are fine.\n\n> +struct conf_info {\n> +\tchar *name;\n> +\tchar *key;\n> +\tchar *command;\n> +\tenum action_where where;\n> +\tenum action_if_exist if_exist;\n> +\tenum action_if_missing if_missing;\n\nIt still feels somewhat strange. It is true that an item can be\neither \"exist\" or \"missing\" and it is understandable that it tempts\nyou to split that into two, but EXIST_OVERWRITE will not trigger\neither WHERE_AFTER or WHERE_BEFORE action.\n\n\n\n> +};\n> +\n> +struct trailer_item {\n> +\tstruct trailer_item *previous;\n> +\tstruct trailer_item *next;\n> +\tconst char *token;\n> +\tconst char *value;\n> +\tstruct conf_info *conf;\n> +};\n> +\n> +static inline int same_token(struct trailer_item *a, struct trailer_item *b, int alnum_len)\n> +{\n> +\treturn !strncasecmp(a->token, b->token, alnum_len);\n> +}\n> +\n> +static inline int same_value(struct trailer_item *a, struct trailer_item *b)\n> +{\n> +\treturn !strcasecmp(a->value, b->value);\n> +}\n> +\n> +static inline int same_trailer(struct trailer_item *a, struct trailer_item *b, int alnum_len)\n> +{\n> +\treturn same_token(a, b, alnum_len) && same_value(a, b);\n> +}\n\nAll these \"inlines\" look premature optimization that can be\ndelegated to any decent compiler, don't they?\n\n> +/* Get the length of buf from its beginning until its last alphanumeric character */\n> +static inline size_t alnum_len(const char *buf, int len)\n> +{\n> +\twhile (--len >= 0 && !isalnum(buf[len]));\n\nStyle:\n\n\twhile (--len >= 0 && !isalnum(buf[len]))\n \t;\n\nYou may add a comment on the empty statement to make it stand out\neven more, i.e.\n\n\t\t; /* nothing */\n\n> +\treturn (size_t) len + 1;\n\nThis is somewhat unfortunate. if the caller wants to receive\nsize_t, perhaps it should be passing in size_t (or ssize_t) to the\nfunction? Hard to guess without an actual caller, though.\n\n> +}\n"},{"id":"234421","messageId":"xmqqvbwrx0sh.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"20140206202004.325.80557.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 01/14] Add data structures and basic functions for commit trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-06T23:46:05Z","receivedAt":"2014-02-06T23:46:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> +\tenum action_if_exist if_exist;\n> +\tenum action_if_missing if_missing;\n\nProbably \"if_exists\" is more gramatically correct.\n\n\tif (x->if_exists) {\n \t... do this ...\n\t}\n\nwould read well, but not \"x->if_exist\". \n"},{"id":"234422","messageId":"xmqqr47fx001.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"20140206202004.325.96525.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-07T00:03:10Z","receivedAt":"2014-02-07T00:03:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> This patch implements the logic that process trailers\n> from file and arguments.\n>\n> At the beginning trailers from file are in their own\n> infile_tok doubly linked list, and trailers from\n> arguments are in their own arg_tok doubly linked list.\n>\n> The lists are traversed and when an arg_tok should be\n> \"applied\", it is removed from its list and inserted\n> into the infile_tok list.\n>\n> Signed-off-by: Christian Couder \n> ---\n> trailer.c | 187 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 187 insertions(+)\n>\n> diff --git a/trailer.c b/trailer.c\n> index f129b5a..ba0cfe0 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -46,3 +46,190 @@ static inline size_t alnum_len(const char *buf, int len)\n> \twhile (--len >= 0 && !isalnum(buf[len]));\n> \treturn (size_t) len + 1;\n> }\n> +\n> +static void add_arg_to_infile(struct trailer_item *infile_tok,\n> +\t\t\t struct trailer_item *arg_tok)\n> +{\n> +\tif (arg_tok->conf->where == WHERE_AFTER) {\n> +\t\targ_tok->next = infile_tok->next;\n> +\t\tinfile_tok->next = arg_tok;\n> +\t\targ_tok->previous = infile_tok;\n> +\t\tif (arg_tok->next)\n> +\t\t\targ_tok->next->previous = arg_tok;\n> +\t} else {\n> +\t\targ_tok->previous = infile_tok->previous;\n> +\t\tinfile_tok->previous = arg_tok;\n> +\t\targ_tok->next = infile_tok;\n> +\t\tif (arg_tok->previous)\n> +\t\t\targ_tok->previous->next = arg_tok;\n> +\t}\n> +}\n> +\n> +static int check_if_different(struct trailer_item *infile_tok,\n> +\t\t\t struct trailer_item *arg_tok,\n> +\t\t\t int alnum_len, int check_all)\n> +{\n> +\tenum action_where where = arg_tok->conf->where;\n> +\tdo {\n> +\t\tif (!infile_tok)\n> +\t\t\treturn 1;\n> +\t\tif (same_trailer(infile_tok, arg_tok, alnum_len))\n> +\t\t\treturn 0;\n> +\t\t/*\n> +\t\t * if we want to add a trailer after another one,\n> +\t\t * we have to check those before this one\n> +\t\t */\n> +\t\tinfile_tok = (where == WHERE_AFTER) ? infile_tok->previous : infile_tok->next;\n> +\t} while (check_all);\n> +\treturn 1;\n> +}\n> +\n> +static void apply_arg_if_exist(struct trailer_item *infile_tok,\n> +\t\t\t struct trailer_item *arg_tok,\n> +\t\t\t int alnum_len)\n> +{\n> +\tswitch (arg_tok->conf->if_exist) {\n> +\tcase EXIST_DO_NOTHING:\n> +\t\tfree(arg_tok);\n> +\t\tbreak;\n> +\tcase EXIST_OVERWRITE:\n> +\t\tfree((char *)infile_tok->value);\n> +\t\tinfile_tok->value = xstrdup(arg_tok->value);\n> +\t\tfree(arg_tok);\n> +\t\tbreak;\n> +\tcase EXIST_ADD:\n> +\t\tadd_arg_to_infile(infile_tok, arg_tok);\n> +\t\tbreak;\n> +\tcase EXIST_ADD_IF_DIFFERENT:\n> +\t\tif (check_if_different(infile_tok, arg_tok, alnum_len, 1))\n> +\t\t\tadd_arg_to_infile(infile_tok, arg_tok);\n> +\t\telse\n> +\t\t\tfree(arg_tok);\n> +\t\tbreak;\n> +\tcase EXIST_ADD_IF_DIFFERENT_NEIGHBOR:\n> +\t\tif (check_if_different(infile_tok, arg_tok, alnum_len, 0))\n> +\t\t\tadd_arg_to_infile(infile_tok, arg_tok);\n> +\t\telse\n> +\t\t\tfree(arg_tok);\n> +\t\tbreak;\n\nMakes me wonder if people want a rule to say \"if the same key\nalready exists, regardless of the value\".\n\n> +\t}\n> +}\n> +\n> +static void remove_from_list(struct trailer_item *item,\n> +\t\t\t struct trailer_item **first)\n> +{\n> +\tif (item->next)\n> +\t\titem->next->previous = item->previous;\n> +\tif (item->previous)\n> +\t\titem->previous->next = item->next;\n> +\telse\n> +\t\t*first = item->next;\n> +}\n\nWill callers free the item that now is not on the list?\n\n> +static struct trailer_item *remove_first(struct trailer_item **first)\n> +{\n> +\tstruct trailer_item *item = *first;\n> +\t*first = item->next;\n> +\tif (item->next) {\n> +\t\titem->next->previous = NULL;\n> +\t\titem->next = NULL;\n> +\t}\n> +\treturn item;\n> +}\n> +\n> +static void process_infile_tok(struct trailer_item *infile_tok,\n> +\t\t\t struct trailer_item **arg_tok_first,\n> +\t\t\t enum action_where where)\n> +{\n> +\tstruct trailer_item *arg_tok;\n> +\tstruct trailer_item *next_arg;\n> +\n> +\tint tok_alnum_len = alnum_len(infile_tok->token, strlen(infile_tok->token));\n> +\tfor (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {\n> +\t\tnext_arg = arg_tok->next;\n> +\t\tif (same_token(infile_tok, arg_tok, tok_alnum_len) &&\n> +\t\t arg_tok->conf->where == where) {\n> +\t\t\tremove_from_list(arg_tok, arg_tok_first);\n> +\t\t\tapply_arg_if_exist(infile_tok, arg_tok, tok_alnum_len);\n> +\t\t\t/*\n> +\t\t\t * If arg has been added to infile,\n> +\t\t\t * then we need to process it too now.\n> +\t\t\t */\n> +\t\t\tif ((where == WHERE_AFTER ? infile_tok->next : infile_tok->previous) == arg_tok)\n> +\t\t\t\tinfile_tok = arg_tok;\n> +\t\t}\n> +\t}\n> +}\n> +\n> +static void update_last(struct trailer_item **last)\n> +{\n> +\tif (*last)\n> +\t\twhile((*last)->next != NULL)\n> +\t\t\t*last = (*last)->next;\n> +}\n> +\n> +static void update_first(struct trailer_item **first)\n> +{\n> +\tif (*first)\n> +\t\twhile((*first)->previous != NULL)\n> +\t\t\t*first = (*first)->previous;\n> +}\n> +\n> +static void apply_arg_if_missing(struct trailer_item **infile_tok_first,\n> +\t\t\t\t struct trailer_item **infile_tok_last,\n> +\t\t\t\t struct trailer_item *arg_tok)\n> +{\n\nMakes me wonder if it would make the code simpler to keep an anchor\nitem \"struct trailer_item\" that is off heap, and pass that single\nanchor item around, using its next/prev fields as the first and the\nlast. Wouldn't it let you remove the special cases for the first\nand last item?\n\n> +\tstruct trailer_item **infile_tok;\n> +\tenum action_where where;\n> +\n> +\tswitch (arg_tok->conf->if_missing) {\n> +\tcase MISSING_DO_NOTHING:\n> +\t\tfree(arg_tok);\n> +\t\tbreak;\n> +\tcase MISSING_ADD:\n> +\t\twhere = arg_tok->conf->where;\n> +\t\tinfile_tok = (where == WHERE_AFTER) ? infile_tok_last : infile_tok_first;\n> +\t\tif (*infile_tok) {\n> +\t\t\tadd_arg_to_infile(*infile_tok, arg_tok);\n> +\t\t\t*infile_tok = arg_tok;\n> +\t\t} else {\n> +\t\t\t*infile_tok_first = arg_tok;\n> +\t\t\t*infile_tok_last = arg_tok;\n> +\t\t}\n> +\t\tbreak;\n\nThis piece makes me wonder if \"after\" is a good name. prepend and\nappend, perhaps?\n"},{"id":"234423","messageId":"xmqqmwi3wzrb.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"20140206202004.325.93876.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 04/14] trailer: process command line trailer arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-07T00:08:24Z","receivedAt":"2014-02-07T00:08:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> This patch parses the trailer command line arguments\n> and put the result into an arg_tok doubly linked\n> list.\n\nNo the patch doesn't parse anything ;-).\n\n\"Parse the command line arguments and ...\".\n\n> +static void parse_trailer(struct strbuf *tok, struct strbuf *val, const char *trailer)\n> +{\n> +\tconst char *end = strchr(trailer, '=');\n> +\tif (!end)\n> +\t\tend = strchr(trailer, ':');\n\nHow would you explain the behaviour of the above code for this\ninput?\n\n\tfrotz: nitfol=xyzzy\n\nPerhaps strcspn()?\n"},{"id":"234424","messageId":"xmqqiosrwzoh.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"20140206202004.325.48784.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 07/14] trailer: add interpret-trailers command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-07T00:10:06Z","receivedAt":"2014-02-07T00:10:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> diff --git a/git.c b/git.c\n> index 3799514..1420b58 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 },\n\nDoes this even need to have a git repository? What is the RUN_SETUP\nfor?\n"},{"id":"234425","messageId":"xmqqeh3fwzlf.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"20140206202004.325.20315.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 08/14] trailer: add tests for \"git interpret-trailers\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-07T00:11:56Z","receivedAt":"2014-02-07T00:11:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\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> +# We want one trailing space at the end of each line.\n> +# Let's use sed to make sure that these spaces are not removed\n> +# by any automatic tool.\n> +sed -e 's/ Z$/ /' >complex_message_trailers <<-\\EOF\n> +Fixes: Z\n> +Acked-by: Z\n> +Reviewed-by: Z\n> +Signed-off-by: Z\n> +EOF\n\nPlease put things like these inside the first \"setup\" test.\n"},{"id":"234426","messageId":"xmqqa9e3wzju.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"20140206202004.325.76199.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 09/14] trailer: if no input file is passed, read from stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-07T00:12:53Z","receivedAt":"2014-02-07T00:12:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> It is simpler and more natural if the \"git interpret-trailers\"\n> is made a filter as its output already goes to sdtout.\n\nsdtout?\n\nWhy isn't this a pure filter without any \"infile\" parameter in the\nfirst place?\n\n> Signed-off-by: Christian Couder \n> ---\n> builtin/interpret-trailers.c | 2 +-\n> t/t7513-interpret-trailers.sh | 7 +++++++\n> trailer.c | 15 +++++++++------\n> 3 files changed, 17 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n> index 04b0ae2..ae8e561 100644\n> --- a/builtin/interpret-trailers.c\n> +++ b/builtin/interpret-trailers.c\n> @@ -23,7 +23,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\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_FILENAME(0, \"infile\", &infile, N_(\"use message from file, instead of stdin\")),\n> \t\tOPT_END()\n> \t};\n> \n> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\n> index 8be333c..f5ef81f 100755\n> --- a/t/t7513-interpret-trailers.sh\n> +++ b/t/t7513-interpret-trailers.sh\n> @@ -205,4 +205,11 @@ test_expect_success 'using \"ifMissing = doNothing\"' '\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success 'with input from stdin' '\n> +\tcat complex_message_body >expected &&\n> +\tprintf \"Bug #42\\nFixes: \\nAcked-by= \\nAcked-by= Junio\\nAcked-by= Peff\\nReviewed-by: \\nSigned-off-by: \\n\" >>expected &&\n> +\tgit interpret-trailers \"review:\" \"fix=53\" \"cc=Linus\" \"ack: Junio\" \"fix=22\" \"bug: 42\" \"ack: Peff\" < complex_message >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> test_done\n> diff --git a/trailer.c b/trailer.c\n> index 186316f..108e104 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -483,8 +483,13 @@ 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> +\tif (infile) {\n> +\t\tif (strbuf_read_file(&sb, infile, 0) < 0)\n> +\t\t\tdie_errno(_(\"could not read input file '%s'\"), infile);\n> +\t} else {\n> +\t\tif (strbuf_read(&sb, fileno(stdin), 0) < 0)\n> +\t\t\tdie_errno(_(\"could not read from stdin\"));\n> +\t}\n> \n> \treturn strbuf_split(&sb, '\\n');\n> }\n> @@ -551,10 +556,8 @@ void process_trailers(const char *infile, int trim_empty, int argc, const char *\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, &infile_tok_first, &infile_tok_last);\n> -\t}\n> +\t/* Print the non trailer part of infile (or stdin if infile is NULL) */\n> +\tprocess_input_file(infile, &infile_tok_first, &infile_tok_last);\n> \n> \targ_tok_first = process_command_line_args(argc, argv);\n"},{"id":"234427","messageId":"xmqq4n4bwzb1.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"20140206202004.325.54178.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 10/14] trailer: execute command from 'trailer..command'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-07T00:18:10Z","receivedAt":"2014-02-07T00:18:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> +#define TRAILER_ARG_STRING \"$ARG\"\n\nNo need to support users who may want to use a string that happens\nto match this substring literally as part of the command line?\n\n> struct trailer_item {\n> \tstruct trailer_item *previous;\n> \tstruct trailer_item *next;\n> @@ -56,6 +60,13 @@ static inline int contains_only_spaces(const char *str)\n> \treturn !*s;\n> }\n> \n> +static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *b)\n\nSame comment about \"inline\" for an earlier patch applies.\n"},{"id":"234428","messageId":"xmqqzjm3vkm3.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"20140206202004.325.20148.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 12/14] trailer: set author and committer env variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-07T00:20:52Z","receivedAt":"2014-02-07T00:20:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Signed-off-by: Christian Couder \n> ---\n> trailer.c | 30 +++++++++++++++++++++++++++++-\n> 1 file changed, 29 insertions(+), 1 deletion(-)\n>\n> diff --git a/trailer.c b/trailer.c\n> index 98187fc..b5de616 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -1,5 +1,6 @@\n> #include \"cache.h\"\n> #include \"run-command.h\"\n> +#include \"argv-array.h\"\n> /*\n> * Copyright (c) 2013 Christian Couder \n> */\n> @@ -433,14 +434,40 @@ static int read_from_command(struct child_process *cp, struct strbuf *buf)\n> \treturn 0;\n> }\n> \n> +static void setup_ac_env(struct argv_array *env, const char *ac_name, const char *ac_mail, const char *(*read)(int))\n> +{\n> +\tif (!getenv(ac_name) || !getenv(ac_mail)) {\n> +\t\tstruct ident_split ident;\n> +\t\tconst char *namebuf, *mailbuf;\n> +\t\tint namelen, maillen;\n> +\t\tconst char *ac_info = read(IDENT_NO_DATE);\n\nThis is far too confusing. Name it read_fn() or something.\n\n> +\t\tif (split_ident_line(&ident, ac_info, strlen(ac_info)))\n> +\t\t\treturn;\n> +\n> +\t\tnamelen = ident.name_end - ident.name_begin;\n> +\t\tnamebuf = ident.name_begin;\n> +\n> +\t\tmaillen = ident.mail_end - ident.mail_begin;\n> +\t\tmailbuf = ident.mail_begin;\n> +\n> +\t\targv_array_pushf(env, \"%s=%.*s\", ac_name, namelen, namebuf);\n> +\t\targv_array_pushf(env, \"%s=%.*s\", ac_mail, maillen, mailbuf);\n> +\t}\n> +}\n> +\n> static const char *apply_command(const char *command, const char *arg)\n> {\n> +\tstruct argv_array env = ARGV_ARRAY_INIT;\n> \tstruct strbuf cmd = STRBUF_INIT;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tstruct child_process cp;\n> \tconst char *argv[] = {NULL, NULL};\n> \tconst char *result = \"\";\n> \n> +\tsetup_ac_env(&env, \"GIT_AUTHOR_NAME\", \"GIT_AUTHOR_EMAIL\", git_author_info);\n> +\tsetup_ac_env(&env, \"GIT_COMMITTER_NAME\", \"GIT_COMMITTER_EMAIL\", git_committer_info);\n> +\n> \tstrbuf_addstr(&cmd, command);\n> \tif (arg)\n> \t\tstrbuf_replace(&cmd, TRAILER_ARG_STRING, arg);\n> @@ -448,7 +475,7 @@ static const char *apply_command(const char *command, const char *arg)\n> \targv[0] = cmd.buf;\n> \tmemset(&cp, 0, sizeof(cp));\n> \tcp.argv = argv;\n> -\tcp.env = local_repo_env;\n> +\tcp.env = env.argv;\n> \tcp.no_stdin = 1;\n> \tcp.out = -1;\n> \tcp.use_shell = 1;\n> @@ -459,6 +486,7 @@ static const char *apply_command(const char *command, const char *arg)\n> \t\tresult = strbuf_detach(&buf, NULL);\n> \n> \tstrbuf_release(&cmd);\n> +\targv_array_clear(&env);\n> \treturn result;\n> }\n"},{"id":"234429","messageId":"xmqqvbwrvkcv.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"20140206202004.325.20148.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 12/14] trailer: set author and committer env variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-07T00:26:24Z","receivedAt":"2014-02-07T00:26:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Signed-off-by: Christian Couder \n> ---\n> trailer.c | 30 +++++++++++++++++++++++++++++-\n> 1 file changed, 29 insertions(+), 1 deletion(-)\n>\n> diff --git a/trailer.c b/trailer.c\n> index 98187fc..b5de616 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -1,5 +1,6 @@\n> #include \"cache.h\"\n> #include \"run-command.h\"\n> +#include \"argv-array.h\"\n> /*\n> * Copyright (c) 2013 Christian Couder \n> */\n> @@ -433,14 +434,40 @@ static int read_from_command(struct child_process *cp, struct strbuf *buf)\n> \treturn 0;\n> }\n> \n> +static void setup_ac_env(struct argv_array *env, const char *ac_name, const char *ac_mail, const char *(*read)(int))\n> +{\n> +\tif (!getenv(ac_name) || !getenv(ac_mail)) {\n\nWhoever is using this tool while replaying an existing commit likely\nwants to export these two variables _into_ this command from the\noutside, and this function will not interfere with it.\n\nBut for other uses, do we need to export these variables? After\nall, this matters _only_ when the command we spawn wants to know\nthe idents to be used, but they can ask git-var themselves to cover\nboth cases, whether the environment that called this tool already\nhad the ident environment variables or it didn't.\n\nSo I am not sure what value this step is adding to the series.\n\n> +\t\tstruct ident_split ident;\n> +\t\tconst char *namebuf, *mailbuf;\n> +\t\tint namelen, maillen;\n> +\t\tconst char *ac_info = read(IDENT_NO_DATE);\n> +\n> +\t\tif (split_ident_line(&ident, ac_info, strlen(ac_info)))\n> +\t\t\treturn;\n> +\n> +\t\tnamelen = ident.name_end - ident.name_begin;\n> +\t\tnamebuf = ident.name_begin;\n> +\n> +\t\tmaillen = ident.mail_end - ident.mail_begin;\n> +\t\tmailbuf = ident.mail_begin;\n> +\n> +\t\targv_array_pushf(env, \"%s=%.*s\", ac_name, namelen, namebuf);\n> +\t\targv_array_pushf(env, \"%s=%.*s\", ac_mail, maillen, mailbuf);\n> +\t}\n> +}\n> +\n> static const char *apply_command(const char *command, const char *arg)\n> {\n> +\tstruct argv_array env = ARGV_ARRAY_INIT;\n> \tstruct strbuf cmd = STRBUF_INIT;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tstruct child_process cp;\n> \tconst char *argv[] = {NULL, NULL};\n> \tconst char *result = \"\";\n> \n> +\tsetup_ac_env(&env, \"GIT_AUTHOR_NAME\", \"GIT_AUTHOR_EMAIL\", git_author_info);\n> +\tsetup_ac_env(&env, \"GIT_COMMITTER_NAME\", \"GIT_COMMITTER_EMAIL\", git_committer_info);\n> +\n\n\n\n> \tstrbuf_addstr(&cmd, command);\n> \tif (arg)\n> \t\tstrbuf_replace(&cmd, TRAILER_ARG_STRING, arg);\n> @@ -448,7 +475,7 @@ static const char *apply_command(const char *command, const char *arg)\n> \targv[0] = cmd.buf;\n> \tmemset(&cp, 0, sizeof(cp));\n> \tcp.argv = argv;\n> -\tcp.env = local_repo_env;\n> +\tcp.env = env.argv;\n> \tcp.no_stdin = 1;\n> \tcp.out = -1;\n> \tcp.use_shell = 1;\n> @@ -459,6 +486,7 @@ static const char *apply_command(const char *command, const char *arg)\n> \t\tresult = strbuf_detach(&buf, NULL);\n> \n> \tstrbuf_release(&cmd);\n> +\targv_array_clear(&env);\n> \treturn result;\n> }\n"},{"id":"234434","messageId":"CAP8UFD10aGq+cUcaiMHRKsLLKvF1eYUuM+G85MRHqhwBXZVv-g@mail.gmail.com","threadId":"35819","inReplyTo":"xmqqiosrwzoh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 07/14] trailer: add interpret-trailers command","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-02-07T08:34:33Z","receivedAt":"2014-02-07T08:34:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Feb 7, 2014 at 1:10 AM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> diff --git a/git.c b/git.c\n>> index 3799514..1420b58 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>\n> Does this even need to have a git repository? What is the RUN_SETUP\n> for?\n\nIt needs to read git config files, but it could work without reading them too.\nI will have another look at it.\n\nThanks,\nChristian.\n"},{"id":"234459","messageId":"xmqqk3d6vlow.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"CAP8UFD10aGq+cUcaiMHRKsLLKvF1eYUuM+G85MRHqhwBXZVv-g@mail.gmail.com","subject":"Re: [PATCH v5 07/14] trailer: add interpret-trailers command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-07T18:09:51Z","receivedAt":"2014-02-07T18:09:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Fri, Feb 7, 2014 at 1:10 AM, Junio C Hamano wrote:\n>> Christian Couder writes:\n>>\n>>> diff --git a/git.c b/git.c\n>>> index 3799514..1420b58 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>>\n>> Does this even need to have a git repository? What is the RUN_SETUP\n>> for?\n>\n> It needs to read git config files, but it could work without reading them too.\n> I will have another look at it.\n\nOf course. At this point in the series while reviewing 7/14 there\nwas no config [*1*] and that was why I was scratching my head.\n\n\n[Footnote]\n\n*1* Flipping the series structure to a top-down fashion, having an\nalmost no-op command that fails all the new tests in the beginning\nand then building the internal incrementally, might be a worthwhile\nchange, but it is *not* worth the effort to add the command without\nRUN_SETUP at 7/14 and then change the same line to have RUN_SETUP\nwhen you start to need it could be an option; I am *not* suggesting\nthat.\n"},{"id":"234460","messageId":"xmqqfvnuvlc1.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"20140206202004.325.54178.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 10/14] trailer: execute command from 'trailer..command'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-07T18:17:34Z","receivedAt":"2014-02-07T18:17:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Signed-off-by: Christian Couder \n\n\"execute command from ...\" is fine, but I wish there were more\ndetails before S-o-b line. Is is not worth explaining what happens\nto the output, and what the facility is used for in general?\n\nIs it to give a string used for the value part? \"Key: Value\"\nstring? Is the command allowed to say \"Put an entry with this\nstring before the existing one, after the existing one, or replace\nthe existing one\"? Can it say \"Upon inspection of the existing\nentry, it is no longer necessary to have it in the footer---remove\nit\"?\n"},{"id":"234463","messageId":"xmqq7g96veyr.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"xmqqk3d6vlow.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 07/14] trailer: add interpret-trailers command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-07T20:35:08Z","receivedAt":"2014-02-07T20:35:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> ... RUN_SETUP at 7/14 and then change the same line to have RUN_SETUP\n> when you start to need it could be an option; I am *not* suggesting\n> that.\n\nSorry, typo. s/could be an option;/;/\n"},{"id":"234543","messageId":"20140209.144809.1747063529382396154.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"xmqqzjm3x0v3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 01/14] Add data structures and basic functions for commit trailers","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-09T13:48:09Z","receivedAt":"2014-02-09T13:48:09Z","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>> +enum action_where { WHERE_AFTER, WHERE_BEFORE };\n>> +enum action_if_exist { EXIST_ADD_IF_DIFFERENT, EXIST_ADD_IF_DIFFERENT_NEIGHBOR,\n>> +\t\t EXIST_ADD, EXIST_OVERWRITE, EXIST_DO_NOTHING };\n>> +enum action_if_missing { MISSING_ADD, MISSING_DO_NOTHING };\n> \n> All these names and \"conf_info\" below are not named to be specific\n> to this little tool. Can I assume that these will never be exposed\n> to the rest of the system? If so, they are fine.\n\nYeah, I don't plan them to be exposed to other files.\n \n>> +struct conf_info {\n>> +\tchar *name;\n>> +\tchar *key;\n>> +\tchar *command;\n>> +\tenum action_where where;\n>> +\tenum action_if_exist if_exist;\n>> +\tenum action_if_missing if_missing;\n> \n> It still feels somewhat strange. It is true that an item can be\n> either \"exist\" or \"missing\" and it is understandable that it tempts\n> you to split that into two, but EXIST_OVERWRITE will not trigger\n> either WHERE_AFTER or WHERE_BEFORE action.\n\nYeah, it's true that WHERE_AFTER/WHERE_BEFORE does not make sense for\nEXIST_OVERWRITE, EXIST_DO_NOTHING and MISSING_DO_NOTHING, but it's a\nfact of life that sometimes some options do not make sense with\nothers.\n\n>> +static inline int same_token(struct trailer_item *a, struct trailer_item *b, int alnum_len)\n>> +{\n>> +\treturn !strncasecmp(a->token, b->token, alnum_len);\n>> +}\n>> +\n>> +static inline int same_value(struct trailer_item *a, struct trailer_item *b)\n>> +{\n>> +\treturn !strcasecmp(a->value, b->value);\n>> +}\n>> +\n>> +static inline int same_trailer(struct trailer_item *a, struct trailer_item *b, int alnum_len)\n>> +{\n>> +\treturn same_token(a, b, alnum_len) && same_value(a, b);\n>> +}\n> \n> All these \"inlines\" look premature optimization that can be\n> delegated to any decent compiler, don't they?\n\nYeah, but as Eric suggested to add them like in header files and you\ndid not reply to him, I thought you agreed with him.\nI will remove them.\n\n>> +/* Get the length of buf from its beginning until its last alphanumeric character */\n>> +static inline size_t alnum_len(const char *buf, int len)\n>> +{\n>> +\twhile (--len >= 0 && !isalnum(buf[len]));\n> \n> Style:\n> \n> \twhile (--len >= 0 && !isalnum(buf[len]))\n> \t;\n> \n> You may add a comment on the empty statement to make it stand out\n> even more, i.e.\n> \n> \t\t; /* nothing */\n\nOk, I will do that.\n\n>> +\treturn (size_t) len + 1;\n> \n> This is somewhat unfortunate. if the caller wants to receive\n> size_t, perhaps it should be passing in size_t (or ssize_t) to the\n> function? Hard to guess without an actual caller, though.\n\nOk, I will make it return an int.\n\nThanks,\nChristian.\n"},{"id":"234544","messageId":"20140209.145103.574296288999775548.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"xmqqvbwrx0sh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 01/14] Add data structures and basic functions for commit trailers","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-09T13:51:03Z","receivedAt":"2014-02-09T13:51:03Z","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>> +\tenum action_if_exist if_exist;\n>> +\tenum action_if_missing if_missing;\n> \n> Probably \"if_exists\" is more gramatically correct.\n> \n> \tif (x->if_exists) {\n> \t... do this ...\n> \t}\n> \n> would read well, but not \"x->if_exist\". \n\nOk, I will use \"if_exists\" instead of \"if_exist\" and also:\n\nenum action_if_exists { EXISTS_ADD_IF_DIFFERENT, EXISTS_ADD_IF_DIFFERENT_NEIGHBOR,\n\t\t\tEXISTS_ADD, EXISTS_OVERWRITE, EXISTS_DO_NOTHING };\n\ninstead of:\n\nenum action_if_exist { EXIST_ADD_IF_DIFFERENT, EXIST_ADD_IF_DIFFERENT_NEIGHBOR,\n\t\t\tEXIST_ADD, EXIST_OVERWRITE, EXIST_DO_NOTHING };\n\nto be consistent.\n\nThanks,\nChristian.\n"},{"id":"234545","messageId":"20140209.145205.1882309246743951569.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"xmqqr47fx001.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-09T13:52:05Z","receivedAt":"2014-02-09T13:52:05Z","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>> +static void apply_arg_if_exist(struct trailer_item *infile_tok,\n>> +\t\t\t struct trailer_item *arg_tok,\n>> +\t\t\t int alnum_len)\n>> +{\n>> +\tswitch (arg_tok->conf->if_exist) {\n>> +\tcase EXIST_DO_NOTHING:\n>> +\t\tfree(arg_tok);\n>> +\t\tbreak;\n>> +\tcase EXIST_OVERWRITE:\n>> +\t\tfree((char *)infile_tok->value);\n>> +\t\tinfile_tok->value = xstrdup(arg_tok->value);\n>> +\t\tfree(arg_tok);\n>> +\t\tbreak;\n>> +\tcase EXIST_ADD:\n>> +\t\tadd_arg_to_infile(infile_tok, arg_tok);\n>> +\t\tbreak;\n>> +\tcase EXIST_ADD_IF_DIFFERENT:\n>> +\t\tif (check_if_different(infile_tok, arg_tok, alnum_len, 1))\n>> +\t\t\tadd_arg_to_infile(infile_tok, arg_tok);\n>> +\t\telse\n>> +\t\t\tfree(arg_tok);\n>> +\t\tbreak;\n>> +\tcase EXIST_ADD_IF_DIFFERENT_NEIGHBOR:\n>> +\t\tif (check_if_different(infile_tok, arg_tok, alnum_len, 0))\n>> +\t\t\tadd_arg_to_infile(infile_tok, arg_tok);\n>> +\t\telse\n>> +\t\t\tfree(arg_tok);\n>> +\t\tbreak;\n> \n> Makes me wonder if people want a rule to say \"if the same key\n> already exists, regardless of the value\".\n\nThis is what \"if_exists\" and \"if_missing\" are all about.\n\nEither:\n\n\tthe same key already exists regardless of the value\n\nand, in this case, what happens depends on what has been specified using\nthe \"if_exists\" configuration variable.\n\nOr:\n\n\tthe same key DOES NOT already exists regardless of the value\n\nand in this case, what happens depends on what has been specified\nusing the \"if_missing\" configuration variable.\n\n>> +static void remove_from_list(struct trailer_item *item,\n>> +\t\t\t struct trailer_item **first)\n>> +{\n>> +\tif (item->next)\n>> +\t\titem->next->previous = item->previous;\n>> +\tif (item->previous)\n>> +\t\titem->previous->next = item->next;\n>> +\telse\n>> +\t\t*first = item->next;\n>> +}\n> \n> Will callers free the item that now is not on the list?\n\nYes, or the item will be inserted into another list.\n\n>> +static struct trailer_item *remove_first(struct trailer_item **first)\n>> +{\n>> +\tstruct trailer_item *item = *first;\n>> +\t*first = item->next;\n>> +\tif (item->next) {\n>> +\t\titem->next->previous = NULL;\n>> +\t\titem->next = NULL;\n>> +\t}\n>> +\treturn item;\n>> +}\n>> +\n>> +static void process_infile_tok(struct trailer_item *infile_tok,\n>> +\t\t\t struct trailer_item **arg_tok_first,\n>> +\t\t\t enum action_where where)\n>> +{\n>> +\tstruct trailer_item *arg_tok;\n>> +\tstruct trailer_item *next_arg;\n>> +\n>> +\tint tok_alnum_len = alnum_len(infile_tok->token, strlen(infile_tok->token));\n>> +\tfor (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {\n>> +\t\tnext_arg = arg_tok->next;\n>> +\t\tif (same_token(infile_tok, arg_tok, tok_alnum_len) &&\n>> +\t\t arg_tok->conf->where == where) {\n>> +\t\t\tremove_from_list(arg_tok, arg_tok_first);\n>> +\t\t\tapply_arg_if_exist(infile_tok, arg_tok, tok_alnum_len);\n>> +\t\t\t/*\n>> +\t\t\t * If arg has been added to infile,\n>> +\t\t\t * then we need to process it too now.\n>> +\t\t\t */\n>> +\t\t\tif ((where == WHERE_AFTER ? infile_tok->next : infile_tok->previous) == arg_tok)\n>> +\t\t\t\tinfile_tok = arg_tok;\n>> +\t\t}\n>> +\t}\n>> +}\n>> +\n>> +static void update_last(struct trailer_item **last)\n>> +{\n>> +\tif (*last)\n>> +\t\twhile((*last)->next != NULL)\n>> +\t\t\t*last = (*last)->next;\n>> +}\n>> +\n>> +static void update_first(struct trailer_item **first)\n>> +{\n>> +\tif (*first)\n>> +\t\twhile((*first)->previous != NULL)\n>> +\t\t\t*first = (*first)->previous;\n>> +}\n>> +\n>> +static void apply_arg_if_missing(struct trailer_item **infile_tok_first,\n>> +\t\t\t\t struct trailer_item **infile_tok_last,\n>> +\t\t\t\t struct trailer_item *arg_tok)\n>> +{\n> \n> Makes me wonder if it would make the code simpler to keep an anchor\n> item \"struct trailer_item\" that is off heap, and pass that single\n> anchor item around, using its next/prev fields as the first and the\n> last. Wouldn't it let you remove the special cases for the first\n> and last item?\n\nYeah, that could work. On the other hand the other fields of this\nspecial item would not be used for anything.\nI will have a look at it.\n\n>> +\tstruct trailer_item **infile_tok;\n>> +\tenum action_where where;\n>> +\n>> +\tswitch (arg_tok->conf->if_missing) {\n>> +\tcase MISSING_DO_NOTHING:\n>> +\t\tfree(arg_tok);\n>> +\t\tbreak;\n>> +\tcase MISSING_ADD:\n>> +\t\twhere = arg_tok->conf->where;\n>> +\t\tinfile_tok = (where == WHERE_AFTER) ? infile_tok_last : infile_tok_first;\n>> +\t\tif (*infile_tok) {\n>> +\t\t\tadd_arg_to_infile(*infile_tok, arg_tok);\n>> +\t\t\t*infile_tok = arg_tok;\n>> +\t\t} else {\n>> +\t\t\t*infile_tok_first = arg_tok;\n>> +\t\t\t*infile_tok_last = arg_tok;\n>> +\t\t}\n>> +\t\tbreak;\n> \n> This piece makes me wonder if \"after\" is a good name. prepend and\n> append, perhaps?\n\nThe problem is that \"prepend\" and \"append\" could be confusing when\nrelated to EXISTS_DO_NOTHING, MISSING_DO_NOTHING and EXISTS_OVERWRITE.\n\nAlso WHERE_MIDDLE and WHERE_SORTED could perhaps be added later in the\nsame enum as WHERE_AFTER and WHERE_BEFORE, and in this case, we would\nneed to find names for those that are like \"prepend\" and \"append\", but\nthat are also difficult to confuse with the EXISTS_XXX and MISSING_XXX\nnames.\n\nThanks,\nChristian.\n"},{"id":"234547","messageId":"20140209.150655.1495985026469658708.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"xmqqmwi3wzrb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 04/14] trailer: process command line trailer arguments","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-09T14:06:55Z","receivedAt":"2014-02-09T14:06:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano \nSubject: Re: [PATCH v5 04/14] trailer: process command line trailer arguments\nDate: Thu, 06 Feb 2014 16:08:24 -0800\n\n> Christian Couder writes:\n> \n>> This patch parses the trailer command line arguments\n>> and put the result into an arg_tok doubly linked\n>> list.\n> \n> No the patch doesn't parse anything ;-).\n> \n> \"Parse the command line arguments and ...\".\n> \n>> +static void parse_trailer(struct strbuf *tok, struct strbuf *val, const char *trailer)\n>> +{\n>> +\tconst char *end = strchr(trailer, '=');\n>> +\tif (!end)\n>> +\t\tend = strchr(trailer, ':');\n> \n> How would you explain the behaviour of the above code for this\n> input?\n> \n> \tfrotz: nitfol=xyzzy\n> \n> Perhaps strcspn()?\n\nOk to use strcspn().\n\nThanks,\nChristian.\n"},{"id":"234571","messageId":"CAPig+cQePC+ozQL9RqGhU8kJL=JOauuJHSSTJZXLP5m7Qhc-Pw@mail.gmail.com","threadId":"35819","inReplyTo":"20140206202004.325.93939.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 14/14] Documentation: add documentation for 'git interpret-trailers'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-02-10T07:17:50Z","receivedAt":"2014-02-10T07:17:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 6, 2014 at 3:20 PM, Christian Couder\n wrote:\n> Signed-off-by: Christian Couder \n> ---\n> diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt\n> new file mode 100644\n> index 0000000..0617941\n> --- /dev/null\n> +++ b/Documentation/git-interpret-trailers.txt\n> @@ -0,0 +1,132 @@\n> +OPTIONS\n> +-------\n> +--trim-empty::\n> + If the 'value' part of any trailer contains only whitespace,\n> + the whole trailer will be removed from the resulting message.\n> +\n> +----infile=::\n> + Read the commit message from `file` instead of the standard\n> + input.\n\ns/----/--/\n\n> +\n> +CONFIGURATION VARIABLES\n> +-----------------------\n"},{"id":"234572","messageId":"CAPig+cQpCSMeZg=vj1Hawu_RuDG3j6uWqj2_wSfhCw8-jsdrcQ@mail.gmail.com","threadId":"35819","inReplyTo":"20140209.144809.1747063529382396154.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 01/14] Add data structures and basic functions for commit trailers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-02-10T07:27:33Z","receivedAt":"2014-02-10T07:27:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 9, 2014 at 8:48 AM, Christian Couder\n wrote:\n> From: Junio C Hamano \n>>\n>> Christian Couder writes:\n>>\n>>> +static inline int same_trailer(struct trailer_item *a, struct trailer_item *b, int alnum_len)\n>>> +{\n>>> + return same_token(a, b, alnum_len) && same_value(a, b);\n>>> +}\n>>\n>> All these \"inlines\" look premature optimization that can be\n>> delegated to any decent compiler, don't they?\n>\n> Yeah, but as Eric suggested to add them like in header files and you\n> did not reply to him, I thought you agreed with him.\n> I will remove them.\n\nIf these functions are used only by trailer.c, then it would make\nsense to move them from trailer.h to trailer.c so that they don't get\ndefined unnecessarily by each .c file which includes trailer.h.\n\n>>> +/* Get the length of buf from its beginning until its last alphanumeric character */\n>>> +static inline size_t alnum_len(const char *buf, int len)\n>>> +{\n>>> + while (--len >= 0 && !isalnum(buf[len]));\n>>> + return (size_t) len + 1;\n>>\n>> This is somewhat unfortunate. if the caller wants to receive\n>> size_t, perhaps it should be passing in size_t (or ssize_t) to the\n>> function? Hard to guess without an actual caller, though.\n>\n> Ok, I will make it return an int.\n\nWhy not adjust the loop condition slightly instead so that you can\ncontinue to accept and return size_t?\n"},{"id":"234585","messageId":"xmqq61omu96d.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"20140209.145205.1882309246743951569.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-10T18:14:34Z","receivedAt":"2014-02-10T18:14:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> This is what \"if_exists\" and \"if_missing\" are all about.\n>\n> Either:\n>\n> \tthe same key already exists regardless of the value\n>\n> and, in this case, what happens depends on what has been specified using\n> the \"if_exists\" configuration variable.\n>\n> Or:\n>\n> \tthe same key DOES NOT already exists regardless of the value\n>\n> and in this case, what happens depends on what has been specified\n> using the \"if_missing\" configuration variable.\n\nHmm, how should things like signed-off-by be handled? You may want\nto allow many entries with the same key with distinct values, but\nduplicated values may want to be handled differently (currently we\nonly avoid to place a duplicate consecutively, but keys\nwith different semantics (e.g. \"fixes-bug: #bugid\") may want to say\n\"unless the same key with the same value exists, append it at the\nend\".\n"},{"id":"234596","messageId":"20140210.205936.813126606054805390.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"xmqq61omu96d.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-10T19:59:36Z","receivedAt":"2014-02-10T19:59:36Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano \nSubject: Re: [PATCH v5 02/14] trailer: process trailers from file and arguments\nDate: Mon, 10 Feb 2014 10:14:34 -0800\n\n> Christian Couder writes:\n> \n>> This is what \"if_exists\" and \"if_missing\" are all about.\n>>\n>> Either:\n>>\n>> \tthe same key already exists regardless of the value\n>>\n>> and, in this case, what happens depends on what has been specified using\n>> the \"if_exists\" configuration variable.\n>>\n>> Or:\n>>\n>> \tthe same key DOES NOT already exists regardless of the value\n>>\n>> and in this case, what happens depends on what has been specified\n>> using the \"if_missing\" configuration variable.\n> \n> Hmm, how should things like signed-off-by be handled? You may want\n> to allow many entries with the same key with distinct values, but\n> duplicated values may want to be handled differently (currently we\n> only avoid to place a duplicate consecutively, but keys\n> with different semantics (e.g. \"fixes-bug: #bugid\") may want to say\n> \"unless the same key with the same value exists, append it at the\n> end\".\n\nMany entries with the same key but distinct values can be configured\nusing:\n\nif_exists = add_if_different\nif_missing = add\n\nMany entries with the same key but values that can be the same can be\nconfigured using:\n\nif_exists = add\nif_missing = add\n\nThe place where the trailers are added, if they should be added, can\nbe selected using either \"where = after\", the default, or \"where =\nbefore\".\n\n\"where = after\" means just after trailers with the same key, or, if\nthere are no trailers with the same key, after all the existing\ntrailers.\n\n\"where = before\" means just before trailers with the same key, or, if\nthere are no trailers with the same key, before all the existing\ntrailers.\n\nI think that this is enough to handle most of the usual cases, but we\ncan add other more complex options later, as I said in the last\nmessage of this thread:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/237278/\n\nThanks,\nChristian.\n"},{"id":"234597","messageId":"xmqq38jqsnc2.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"20140210.205936.813126606054805390.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-10T20:51:41Z","receivedAt":"2014-02-10T20:51:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Many entries with the same key but distinct values can be configured\n> using:\n>\n> if_exists = add_if_different\n> if_missing = add\n>\n> Many entries with the same key but values that can be the same can be\n> configured using:\n>\n> if_exists = add\n> if_missing = add\n\nWhile the above certainly can express the combinations, I am still\npuzzled about the value of having \"under this condition\" (i.e.\nif-exists/if-missing) and \"do this\" (i.e. add/add-if-different) as\ntwo separate concepts.\n\nIf you do not have an existing entry with the same key, no new entry\ncan be the same as an existing one---therefore, the former \"allow\nonly distinct values for the same key\" can just say\n\n trailer.\"Fixes\".action = add_if_different\n\nor something, without any if_exists/if_missing. In a similar way,\nthe latter \"duplicated values allowed for the same key\" can say\n\n trailer.\"Sob\".action = add\n\nYou can throw into the mix other actions like \"add_if_missing\", you\ncan also express \"only one value allowed for the same key---the\nfirst one wins\", \"replace\" to mean \"replace if there is one with the\nsame key\", and \"replace_or_add\" to mean \"same as 'replace', but add\nif there is no existing entries with the same key\". Will we lose\nexpressiveness by simplifying the semantics, getting rid of this\n\"under this condition\" part and instead making \"do this\" part more\nintelligent?\n\nEven if we assume, for the sake of discussion, that it *is* a good\nidea to separate \"under this condition\" part and \"do this\" part, I\ndo not think the above is the only or the best way to express\n\"distinct values allowed for the same key\". How do we argue that\nthis from your example\n\n if_exists = add_if_different\n if_missing = add\n\nis a better design than this, for example?\n\n if_key_value_exists = ignore ; exact matching exists\n if_key_exists = add ; otherwise\n if_missing = add\n\nThe latter splits remaining conditional logic from \"do this\" part\n(no more \"add_if_different\" that causes \"add\" action to see if there\nis the same key and act differently) and moves it to \"under this\ncondition\" part.\n\nI am trying to illustrate that the line to split the \"under this\ncondition\" and \"do this\" looks arbitrary and fuzzy here, and because\nof this arbitrary-ness and fuzziness, it is not very obvious to me\nwhy it is a good idea to have \"under this condition\" as a separate\nconcept from \"do this\" settings.\n\nWhat is the advantage of having such an extra axis? Does it make it\neasier to manage? Does it allow us to express more?\n\nI can see that the location where a new entry (or a duplicated one)\nis added (i.e. do we prepend? do we append?) can be orthogonal to\nany of the above; no need to bring up 'where' in the discussion.\n"},{"id":"234614","messageId":"CAP8UFD1Nq-LkE=FW5dnBZKKd7-ORJPo1BFs3sY+MLGxuXEWuTw@mail.gmail.com","threadId":"35819","inReplyTo":"xmqq38jqsnc2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-02-11T15:02:40Z","receivedAt":"2014-02-11T15:02:40Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Feb 10, 2014 at 9:51 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> Many entries with the same key but distinct values can be configured\n>> using:\n>>\n>> if_exists = add_if_different\n>> if_missing = add\n>>\n>> Many entries with the same key but values that can be the same can be\n>> configured using:\n>>\n>> if_exists = add\n>> if_missing = add\n>\n> While the above certainly can express the combinations, I am still\n> puzzled about the value of having \"under this condition\" (i.e.\n> if-exists/if-missing) and \"do this\" (i.e. add/add-if-different) as\n> two separate concepts.\n\nIt is because there are many possible conditions under which the user\nmight want to do different things, and the syntax of our configuration\nfiles is not a programming language, so it is not well suited to\nnicely express what the user might want under different conditions.\n\nThat's what I tried to show in my last message in the thread I sent\nthe link to in my previous message.\n\n(http://thread.gmane.org/gmane.comp.version-control.git/237278/)\n\nSo we have to choose a few conditions to avoid possible combinatorial\nexplosion in the number of possible values.\n\n> If you do not have an existing entry with the same key, no new entry\n> can be the same as an existing one---therefore, the former \"allow\n> only distinct values for the same key\" can just say\n>\n> trailer.\"Fixes\".action = add_if_different\n>\n> or something, without any if_exists/if_missing. In a similar way,\n> the latter \"duplicated values allowed for the same key\" can say\n>\n> trailer.\"Sob\".action = add\n>\n> You can throw into the mix other actions like \"add_if_missing\", you\n> can also express \"only one value allowed for the same key---the\n> first one wins\", \"replace\" to mean \"replace if there is one with the\n> same key\", and \"replace_or_add\" to mean \"same as 'replace', but add\n> if there is no existing entries with the same key\". Will we lose\n> expressiveness by simplifying the semantics, getting rid of this\n> \"under this condition\" part and instead making \"do this\" part more\n> intelligent?\n\nI don't think we will lose expressiveness, but I think there might be\na combinatorial explosion in the number of choices.\n\nRight now there are 2 choices for \"if_missing\" and 5 choices for\n\"if_exists\". This means that if we use only one \"action\" config\nvariable and want to have the same expressiveness, we need 10 choices.\n\nIt doesn't seem a big difference now, but, if we add more choices,\nthen the difference will increase a lot.\n\n> Even if we assume, for the sake of discussion, that it *is* a good\n> idea to separate \"under this condition\" part and \"do this\" part, I\n> do not think the above is the only or the best way to express\n> \"distinct values allowed for the same key\". How do we argue that\n> this from your example\n>\n> if_exists = add_if_different\n> if_missing = add\n>\n> is a better design than this, for example?\n>\n> if_key_value_exists = ignore ; exact matching exists\n> if_key_exists = add ; otherwise\n> if_missing = add\n\nThe question is what happens if we want to say \"if the same pair exists but not near where we would add.\n\nWith the solution I implemented, that is:\n\nif_exists = add_if_different_neighbor\n\nWith the solution you suggest, should we have:\n\n if_key_value_exists_not_neighbor = add ; exact matching \nexists but not near where we would add\n if_key_value_exists = ignore ; exact matching exists\n if_key_exists = add ; otherwise\n if_missing = add\n\nor:\n\n if_key_value_exists = ignore_if_neighbor ; exact matching exists\n if_key_exists = add ; otherwise\n if_missing = add\n\nAnd what happens if we want to say \"if key exists and value matches\n\", how do we express that then?\nOr what happens when we want to say \"if key exists and succeeds\"?\n\nWith what I suggest, we can still say:\n\nif_exists = add_if_values_matches \n\nor\n\nif_exists = add_if_succeeds \n\nWith the solution you suggest, should it be:\n\n if_key_value_exists = ignore\n if_key_exists = add_if_values_matches \n\nand\n\n if_key_value_exists = ignore\n if_key_exists = add_if_succeeds \n\n?\n\nDoesn't it look like redondant between the 2 lines?\n\nAnd then people might ask if they can also use something like this:\n\n if_key_value_exists = add_if_succeeds \n if_key_exists = add_if_succeeds \n if_key_missing = add_if_succeeds \n\nand it will look like we are trying too much to do what should be done\nin only one script.\n\n> The latter splits remaining conditional logic from \"do this\" part\n> (no more \"add_if_different\" that causes \"add\" action to see if there\n> is the same key and act differently) and moves it to \"under this\n> condition\" part.\n>\n> I am trying to illustrate that the line to split the \"under this\n> condition\" and \"do this\" looks arbitrary and fuzzy here, and because\n> of this arbitrary-ness and fuzziness, it is not very obvious to me\n> why it is a good idea to have \"under this condition\" as a separate\n> concept from \"do this\" settings.\n>\n> What is the advantage of having such an extra axis? Does it make it\n> easier to manage? Does it allow us to express more?\n\nIt will be complex anyway, but at least, with what I implemented,\nthere are only 2 cases in the \"under this condition part\" and they are\ncompletely disjoint, which means that they are hopefully not very\ndifficult to understand.\n\nI agree that it may look arbitrary and fuzzy to split things this way,\nbut I think it's a good compromise between avoiding combinatorial\nexplosion in the number of choices, keeping it easy to understand and\nallowing many features now and more in the future.\n\nThanks,\nChristian.\n"},{"id":"234619","messageId":"xmqqa9dxr09k.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"CAP8UFD1Nq-LkE=FW5dnBZKKd7-ORJPo1BFs3sY+MLGxuXEWuTw@mail.gmail.com","subject":"Re: [PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-11T18:07:35Z","receivedAt":"2014-02-11T18:07:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>> Even if we assume, for the sake of discussion, that it *is* a good\n>> idea to separate \"under this condition\" part and \"do this\" part, I\n>> do not think the above is the only or the best way to express\n>> \"distinct values allowed for the same key\". How do we argue that\n>> this from your example\n>>\n>> if_exists = add_if_different\n>> if_missing = add\n>>\n>> is a better design than this, for example?\n>>\n>> if_key_value_exists = ignore ; exact matching exists\n>> if_key_exists = add ; otherwise\n>> if_missing = add\n>\n> The question is what happens if we want to say \"if the same value> pair exists but not near where we would add.\n>\n> With the solution I implemented, that is:\n> ...\n> With the solution you suggest, should we have:\n> ...\n> if_key_value_exists_not_neighbor = add ; exact matching \n> ...\n> or:\n> if_key_value_exists = ignore_if_neighbor ; exact matching exists\n> ...\n>\n> And what happens if we want to say \"if key exists and value matches\n> \", how do we express that then?\n> Or what happens when we want to say \"if key exists and succeeds\"?\n> ...\n> With what I suggest, we can still say:\n> ...\n> And then people might ask if they can also use something like this:\n> ...\n> and it will look like we are trying too much to do what should be done\n> in only one script.\n\nI think the above illustrates my point very clearly.\n\nThese numerous questions you have to ask are indications why\nchoosing \"this condition goes to the left hand side of the equal\nsign (e.g. exists)\" and \"this condition goes to the right hand side\n(e.g. do-this-if_neighbor)\" is not working well. The user has to\nremember which conditions go to the variable name and which\nconditions go to the action part.\n\nAnd the made-up alternative you listed above is not a solution I\nsuggest to begin with---it is a strawman \"if we assume such a\nsplitting were a good idea\" in the first place ;-).\n\nWhat I was wondering if it is an better alternative was the below.\n\n>> The latter splits remaining conditional logic from \"do this\" part\n>> (no more \"add_if_different\" that causes \"add\" action to see if there\n>> is the same key and act differently) and moves it to \"under this\n>> condition\" part.\n>> ...\n>> I am trying to illustrate that the line to split the \"under this\n>> condition\" and \"do this\" looks arbitrary and fuzzy here, and because\n>> of this arbitrary-ness and fuzziness, it is not very obvious to me\n>> why it is a good idea to have \"under this condition\" as a separate\n>> concept from \"do this\" settings.\n\nThat is, not splitting the logic into two parts like you did,\ni.e. \"if_X = do_Y_if_Z\", which invites \"why is it not if_true =\ndo_Y_if_X_and_Z, if_X_and_Z = do_Y, if_Z = do_Y_if_X\"?\n\nOne possible way to avoid the confusion is to say \"do_Y_if_X_and_Z\"\nwithout making the rule split into conditions and actions---I am\nNOT saying that is _better_, there may be other solutions to avoid\nthis two-part logic in a cleaner way.\n"},{"id":"234814","messageId":"20140214.224133.484636406629780362.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"xmqqa9dxr09k.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-14T21:41:33Z","receivedAt":"2014-02-14T21:41:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano \n>\n> These numerous questions you have to ask are indications why\n> choosing \"this condition goes to the left hand side of the equal\n> sign (e.g. exists)\" and \"this condition goes to the right hand side\n> (e.g. do-this-if_neighbor)\" is not working well. The user has to\n> remember which conditions go to the variable name and which\n> conditions go to the action part.\n\nIf we limit it to \"if_exists\" and \"if_missing\", the user can remember\nthat without things becoming too complex.\n\n> That is, not splitting the logic into two parts like you did,\n> i.e. \"if_X = do_Y_if_Z\", which invites \"why is it not if_true =\n> do_Y_if_X_and_Z, if_X_and_Z = do_Y, if_Z = do_Y_if_X\"?\n\nIt perhaps invite it, but there are reasons why splitting the logic\ntoo much is not a good idea and why limiting the split to \"if_exists\"\nand \"if_missing\" is a good tradeoff.\n\n> One possible way to avoid the confusion is to say \"do_Y_if_X_and_Z\"\n> without making the rule split into conditions and actions---I am\n> NOT saying that is _better_, there may be other solutions to avoid\n> this two-part logic in a cleaner way.\n\nThis has been discussed first last November and I don't think that a\nsolution better than what I implemented has been suggested.\n\nThe problem is we not only want to say:\n\n action = do_Y_if_X_and_Z\n\nbut we also want to say:\n\n action = do_Y_if_X_and_Z AND do_U_if_V\n\nFor example some people might want:\n\n if_exists = overwrite\n if_missing = add\n\nwhile others might want:\n\n if_exists = overwrite\n if_missing = do_nothing\n\nand I don't see how we can say that with just:\n\n action = do_Y_if_X_and_Z\n\nThanks,\nChristian.\n"},{"id":"234815","messageId":"xmqqfvnlgyel.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"20140214.224133.484636406629780362.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-14T21:46:58Z","receivedAt":"2014-02-14T21:46:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> For example some people might want:\n>\n> if_exists = overwrite\n> if_missing = add\n>\n> while others might want:\n>\n> if_exists = overwrite\n> if_missing = do_nothing\n>\n> and I don't see how we can say that with just:\n>\n> action = do_Y_if_X_and_Z\n\nYes, but then we go back to my original question: why exists and\nmissing are so special, and why there aren't two kinds of exists\n(i.e. \"there exists an entry with the same \" vs \"there\nexists an entry with the same \"). I would have understood your\n\"this is not too hard to understand for users\" if you had three\n(i.e. \"missing\", in addition to these two flavours of \"exists\"), but\nwith only two, I do not see how it is useful in a hypothetical\nsituation like above.\n\nFor example, how would you express something like this only with\n\"if-exists\" vs \"if-missing\"?\n\n\tif_exists_exactly = ignore\n if_exists_with_different_value = append\n if_missng = prepend_to_the_beginning\n"},{"id":"234824","messageId":"20140215.002950.97819205213642217.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"xmqqfvnlgyel.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-14T23:29:50Z","receivedAt":"2014-02-14T23:29:50Z","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>> For example some people might want:\n>>\n>> if_exists = overwrite\n>> if_missing = add\n>>\n>> while others might want:\n>>\n>> if_exists = overwrite\n>> if_missing = do_nothing\n>>\n>> and I don't see how we can say that with just:\n>>\n>> action = do_Y_if_X_and_Z\n> \n> Yes, but then we go back to my original question: why exists and\n> missing are so special,\n\nBecause they are completely disjoint, easy to understand, and they can\navoid a lot of combinatorial explosion we would have if we used only\none \"action\" variable, while still providing lot of expressiveness.\n\nThey are just a good tradeoff for the special problem we have.\n\n> and why there aren't two kinds of exists\n> (i.e. \"there exists an entry with the same \" vs \"there\n> exists an entry with the same \").\n\nBecause it doesn't improve expressiveness much, doesn't remove much\ncombinatorial explosion and make it significantly more difficult to\nunderstand, compared to only \"if_exists\" and \"if_missing\".\n\n> I would have understood your\n> \"this is not too hard to understand for users\" if you had three\n> (i.e. \"missing\", in addition to these two flavours of \"exists\"), but\n> with only two, I do not see how it is useful in a hypothetical\n> situation like above.\n\nYou mean that you do not see why:\n\n if_exists = overwrite\n if_missing = do_nothing\n\nis simple and expressive?\n\n> For example, how would you express something like this only with\n> \"if-exists\" vs \"if-missing\"?\n> \n> \tif_exists_exactly = ignore\n> if_exists_with_different_value = append\n> if_missng = prepend_to_the_beginning\n\nFirst, previously in the discussion you said that you didn't want us\nto talk about the \"where = (after | before)\" part, because you could\nsee that it was orthogonal to the other stuff, but now it looks like\nyou want again to put that on the table.\n\nThen yes, it is not possible to express the above with what I\nimplemented. But it could be possible with only \"if-exists\" vs\n\"if-missing\" like this:\n\n\tif_exists = append_if_different\n\tif_missing = prepend\n\nAnd yes I think it is much better than:\n\n\tif_exists_exactly = ignore\n\tif_exists_with_different_value = append\n\tif_missng = prepend_to_the_beginning\n\nbecause we can still easily express things like:\n\n\tif_exists = append_if_different_neighbor\n\tif_missing = prepend\n\nwhile it would be more difficult to understand with\n\"if_exists_exactly\", \"if_exists_with_different_value\" and\n\"if_missing\".\n\nThanks,\nChristian.\n"},{"id":"234828","messageId":"CAPc5daUsKaKfDD4AN10Az4HGAdLWnDkSYmLEOCMBrEkQtuRtAQ@mail.gmail.com","threadId":"35819","inReplyTo":"20140215.002950.97819205213642217.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-14T23:39:31Z","receivedAt":"2014-02-14T23:39:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">> For example, how would you express something like this only with\n>> \"if-exists\" vs \"if-missing\"?\n>>\n>> if_exists_exactly = ignore\n>> if_exists_with_different_value = append\n>> if_missng = prepend_to_the_beginning\n>\n> First, previously in the discussion you said that you didn't want us\n> to talk about the \"where = (after | before)\" part, because you could\n> see that it was orthogonal to the other stuff, but now it looks like\n> you want again to put that on the table.\n\nOh, then replace both \"append\" and \"prepend\" with \"append\" (it was a mistake).\nCan you express that without having two kinds of if-exists?\n\n> But it could be possible with only \"if-exists\" vs\n> \"if-missing\" like this:\n>\n> if_exists = append_if_different\n> if_missing = prepend\n> ...\n> because we can still easily express things like:\n>\n> if_exists = append_if_different_neighbor\n\nThe proliferation of these random \"if_X\" on the action part _is_ exactly\nwhat I find the proposal confusing.\n"},{"id":"234829","messageId":"xmqqtxc1fdsk.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"20140214.224133.484636406629780362.chriscool@tuxfamily.org","subject":"Re: [PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-14T23:57:31Z","receivedAt":"2014-02-14T23:57:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> but we also want to say:\n>\n> action = do_Y_if_X_and_Z AND do_U_if_V\n>\n> For example some people might want:\n>\n> if_exists = overwrite\n> if_missing = add\n>\n> while others might want:\n>\n> if_exists = overwrite\n> if_missing = do_nothing\n>\n> and I don't see how we can say that with just:\n>\n> action = do_Y_if_X_and_Z\n\nThat is a very relevant illustration that makes me realize why I\nfound your \"if-exists/if-missing = do-Y-if-Z\" somewhat distasteful.\n\nYour\n\n if_exists = add_if_different\n\nsays \"if the same key is there, add it if the value is different\",\nbut it also implicitly says \"donothing if the value is the same\".\n\nThat is, you are saying with the above\n\n if_exists = add_if_different AND ignore_if_same\n\nSo you already have to support more than one actions depending on\nthe condition, unless you want to limit the actions for all the\ncases other than X to be only \"ignore\" when you invent your next\n\"do_Y_if_X\", X being \"different\" in this case, but you support (and\nneed to support) \"different-neighbour\" and other random collections\nof conditions, I think. Which is essentially the same as saying\nthat you need this:\n\n> action = do_Y_if_X_and_Z AND do_U_if_V\n\nAgain, unless all the U's are limited to \"ignore\", that is.\n"},{"id":"234830","messageId":"xmqqppmpfcx1.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"xmqqtxc1fdsk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-15T00:16:26Z","receivedAt":"2014-02-15T00:16:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> That is, you are saying with the above\n>\n> if_exists = add_if_different AND ignore_if_same\n>\n> So you already have to support more than one actions depending on\n> the condition, ...\n> of conditions, I think. Which is essentially the same as saying\n> that you need this:\n>\n>> action = do_Y_if_X_and_Z AND do_U_if_V\n>\n> Again, unless all the U's are limited to \"ignore\", that is.\n\nOh by the way, don't get me wrong. I am not saying that the last\none is necessarily better or worse. I am only saying that the\nsemantics proposed seems to be hard to explain and we need to do\nfind a way to do better.\n\nIf you have these existing ones:\n\n\tSob: A\n Sob: B\n Sob: C\n Sob: D\n\nand you have \"Sob: B\" at hand, \"Sob.if-missing\" would not fire\n(because if-exists/if-missing is only about keys) ans\n\"Sob.if-exists\" will. What happens is now up to the action part\n(i.e. what follows \"if_exists =\", e.g. \"add_if_different\").\n\nThe conditional part of \"add_if_different\" action is explainable as\na conditon on the value (as opposed to condition on keys, which is\nthe left-hand-side). But what does a condition with \"neighbour\" in\nits name really mean? It is not a condition about the value, but\nalso is a condition about the order of the existing records.\n\nWhat is the right mental model the end-user needs to form when\nunderstanding these? Conditions on keys go on the left, and any\nother random conditions can come as a modifier after action\ne.g. add_if_same_value_is_not_at_the_end?\n"},{"id":"235117","messageId":"xmqq8ut55n6x.fsf@gitster.dls.corp.google.com","threadId":"35819","inReplyTo":"xmqqppmpfcx1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-21T00:22:46Z","receivedAt":"2014-02-21T00:22:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n> What is the right mental model the end-user needs to form when\n> understanding these? Conditions on keys go on the left, and any\n> other random conditions can come as a modifier after action\n> e.g. add_if_same_value_is_not_at_the_end?\n\nHaving said all that, it appears that nobody seems to be able to\ncome up with a saner arrangement that would not paint us into a\ntough corner that we would not be able to later escape from without\nbeing backward incompatible---I certainly didn't.\n\nSo... let's take this from your earlier message:\n\n>> If we limit it to \"if_exists\" and \"if_missing\", the user can remember\n>> that without things becoming too complex.\n\nand go with the semantics the posted patches (I believe I have the\nlatest from you on 'pu') attempt to implement, at least for now.\n\nIOW, when re-rolling, let's not try changing the arrangement to use\nif-exists/if-missing (configuration variable names) for keys'\nexistence and include chosen set of conditions on values as\nmodifiers to the action (i.e. X in \"do_Y_in_X\").\n"},{"id":"235202","messageId":"20140223.114438.2222295831383883854.chriscool@tuxfamily.org","threadId":"35819","inReplyTo":"xmqq8ut55n6x.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 02/14] trailer: process trailers from file and arguments","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-02-23T10:44:38Z","receivedAt":"2014-02-23T10:44:38Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano \n>\n> Junio C Hamano writes:\n>\n> Having said all that, it appears that nobody seems to be able to\n> come up with a saner arrangement that would not paint us into a\n> tough corner that we would not be able to later escape from without\n> being backward incompatible---I certainly didn't.\n> \n> So... let's take this from your earlier message:\n> \n>>> If we limit it to \"if_exists\" and \"if_missing\", the user can remember\n>>> that without things becoming too complex.\n> \n> and go with the semantics the posted patches (I believe I have the\n> latest from you on 'pu') attempt to implement, at least for now.\n> \n> IOW, when re-rolling, let's not try changing the arrangement to use\n> if-exists/if-missing (configuration variable names) for keys'\n> existence and include chosen set of conditions on values as\n> modifiers to the action (i.e. X in \"do_Y_in_X\").\n\nOk, will re-roll soon.\n\nThanks,\nChristian.\n"}]}