[PATCH v9 00/11] Add interpret-trailers builtin

[PATCH v9 00/11] Add interpret-trailers builtin

[Christian Couder]

This patch series implements a new command:

        git interpret-trailers

and an infrastructure to process trailers that can be reused,
for example in "commit.c".

1) Rationale:

This command should help with RFC 822 style headers, called
"trailers", that are found at the end of commit messages.

(Note that these headers do not follow and are not intended to
follow many rules that are in RFC 822. For example they do not
follow the line breaking rules, the encoding rules and probably
many other rules.)

For a long time, these trailers have become a de facto standard
way to add helpful information into commit messages.

Until now git commit has only supported the well known
"Signed-off-by: " trailer, that is used by many projects like
the Linux kernel and Git.

It is better to keep builtin/commit.c uncontaminated by any more
hard-wired logic, like what we have for the signed-off-by line.  Any
new things can and should be doable in hooks, and this filter would
help writing these hooks.

And that is why the design goal of the filter is to make it at least
as powerful as the built-in logic we have for signed-off-by lines;
that would allow us to later eject the hard-wired logic for
signed-off-by line from the main codepath, if/when we wanted to.

Alternatively, we could build a library-ish API around this filter
code and replace the hard-wired logic for signed-off-by line with a
call into that API, if/when we wanted to, but that requires (in
addition to the "at least as powerful as the built-in logic") that
the implementation of this stand-alone filter can be cleanly made
into a reusable library, so that is a bit higher bar to cross than
"everything can be doable with hooks" alternative.

2) Current state:

Currently the usage string of this command is:

git interpret-trailers [--trim-empty] [(<token>[(=|:)<value>])...]

The following features are implemented:

        - the result is printed on stdout
        - the [<token>[=<value>]>] arguments are interpreted
        - a commit message read from stdin is interpreted
        - the "trailer.<token>.key" options in the config are interpreted
        - the "trailer.<token>.where" options are interpreted
        - the "trailer.<token>.ifExist" options are interpreted
        - the "trailer.<token>.ifMissing" options are interpreted
        - the "trailer.<token>.command" config works
        - $ARG can be used in commands
        - there are some tests
        - there is some documentation

The following features are planned but not yet implemented:
        - add more tests related to commands
        - add examples in documentation
        - integration with "git commit"

Possible improvements:
        - support GIT_COMMIT_PROTO env variable in commands

3) Changes since version 8, thanks to Junio:

* improve the rationale section above
* improve commit message in patch 1/11
* remove patch (3/12) that moved some lowercase
  functions to wrapper.c
* use strcmp() instead of strcasecmp() to compare
  some values from the config file matching a fixed set

This means code changes only in patch 3/11 and commit
message changes only in patch 1/11.


Christian Couder (11):
  trailer: add data structures and basic functions
  trailer: process trailers from stdin and arguments
  trailer: read and process config information
  trailer: process command line trailer arguments
  trailer: parse trailers from stdin
  trailer: put all the processing together and print
  trailer: add interpret-trailers command
  trailer: add tests for "git interpret-trailers"
  trailer: execute command from 'trailer.<name>.command'
  trailer: add tests for commands in config file
  Documentation: add documentation for 'git interpret-trailers'

 .gitignore                               |   1 +
 Documentation/git-interpret-trailers.txt | 123 ++++++
 Makefile                                 |   2 +
 builtin.h                                |   1 +
 builtin/interpret-trailers.c             |  33 ++
 git.c                                    |   1 +
 t/t7513-interpret-trailers.sh            | 407 ++++++++++++++++++
 trailer.c                                | 684 +++++++++++++++++++++++++++++++
 trailer.h                                |   6 +
 9 files changed, 1258 insertions(+)
 create mode 100644 Documentation/git-interpret-trailers.txt
 create mode 100644 builtin/interpret-trailers.c
 create mode 100755 t/t7513-interpret-trailers.sh
 create mode 100644 trailer.c
 create mode 100644 trailer.h

-- 
1.9.0.164.g3aa33cd.dirty

[PATCH v9 01/11] trailer: add data structures and basic functions

[Christian Couder]

We will use a doubly linked list to store all information
about trailers and their configuration.

This way we can easily remove or add trailers to or from
trailer lists while traversing the lists in either direction.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Makefile  |  1 +
 trailer.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)
 create mode 100644 trailer.c

diff --git a/Makefile b/Makefile
index d4ce53a..ec67ae1 100644
--- a/Makefile
+++ b/Makefile
@@ -883,6 +883,7 @@ LIB_OBJS += submodule.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
 LIB_OBJS += trace.o
+LIB_OBJS += trailer.o
 LIB_OBJS += transport.o
 LIB_OBJS += transport-helper.o
 LIB_OBJS += tree-diff.o
diff --git a/trailer.c b/trailer.c
new file mode 100644
index 0000000..db93a63
--- /dev/null
+++ b/trailer.c
@@ -0,0 +1,49 @@
+#include "cache.h"
+/*
+ * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
+ */
+
+enum action_where { WHERE_AFTER, WHERE_BEFORE };
+enum action_if_exists { EXISTS_ADD_IF_DIFFERENT, EXISTS_ADD_IF_DIFFERENT_NEIGHBOR,
+			EXISTS_ADD, EXISTS_OVERWRITE, EXISTS_DO_NOTHING };
+enum action_if_missing { MISSING_ADD, MISSING_DO_NOTHING };
+
+struct conf_info {
+	char *name;
+	char *key;
+	char *command;
+	enum action_where where;
+	enum action_if_exists if_exists;
+	enum action_if_missing if_missing;
+};
+
+struct trailer_item {
+	struct trailer_item *previous;
+	struct trailer_item *next;
+	const char *token;
+	const char *value;
+	struct conf_info conf;
+};
+
+static int same_token(struct trailer_item *a, struct trailer_item *b, int alnum_len)
+{
+	return !strncasecmp(a->token, b->token, alnum_len);
+}
+
+static int same_value(struct trailer_item *a, struct trailer_item *b)
+{
+	return !strcasecmp(a->value, b->value);
+}
+
+static int same_trailer(struct trailer_item *a, struct trailer_item *b, int alnum_len)
+{
+	return same_token(a, b, alnum_len) && same_value(a, b);
+}
+
+/* Get the length of buf from its beginning until its last alphanumeric character */
+static size_t alnum_len(const char *buf, size_t len)
+{
+	while (len > 0 && !isalnum(buf[len - 1]))
+		len--;
+	return len;
+}
-- 
1.9.0.164.g3aa33cd.dirty

[PATCH v9 02/11] trailer: process trailers from stdin and arguments

[Christian Couder]

Implement the logic to process trailers from stdin and arguments.

At the beginning trailers from stdin are in their own in_tok
doubly linked list, and trailers from arguments are in their own
arg_tok doubly linked list.

The lists are traversed and when an arg_tok should be "applied",
it is removed from its list and inserted into the in_tok list.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 trailer.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 198 insertions(+)

diff --git a/trailer.c b/trailer.c
index db93a63..52108c2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -47,3 +47,201 @@ static size_t alnum_len(const char *buf, size_t len)
 		len--;
 	return len;
 }
+
+static void free_trailer_item(struct trailer_item *item)
+{
+	free(item->conf.name);
+	free(item->conf.key);
+	free(item->conf.command);
+	free((char *)item->token);
+	free((char *)item->value);
+	free(item);
+}
+
+static void add_arg_to_input_list(struct trailer_item *in_tok,
+				  struct trailer_item *arg_tok)
+{
+	if (arg_tok->conf.where == WHERE_AFTER) {
+		arg_tok->next = in_tok->next;
+		in_tok->next = arg_tok;
+		arg_tok->previous = in_tok;
+		if (arg_tok->next)
+			arg_tok->next->previous = arg_tok;
+	} else {
+		arg_tok->previous = in_tok->previous;
+		in_tok->previous = arg_tok;
+		arg_tok->next = in_tok;
+		if (arg_tok->previous)
+			arg_tok->previous->next = arg_tok;
+	}
+}
+
+static int check_if_different(struct trailer_item *in_tok,
+			      struct trailer_item *arg_tok,
+			      int alnum_len, int check_all)
+{
+	enum action_where where = arg_tok->conf.where;
+	do {
+		if (!in_tok)
+			return 1;
+		if (same_trailer(in_tok, arg_tok, alnum_len))
+			return 0;
+		/*
+		 * if we want to add a trailer after another one,
+		 * we have to check those before this one
+		 */
+		in_tok = (where == WHERE_AFTER) ? in_tok->previous : in_tok->next;
+	} while (check_all);
+	return 1;
+}
+
+static void apply_arg_if_exists(struct trailer_item *in_tok,
+				struct trailer_item *arg_tok,
+				int alnum_len)
+{
+	switch (arg_tok->conf.if_exists) {
+	case EXISTS_DO_NOTHING:
+		free_trailer_item(arg_tok);
+		break;
+	case EXISTS_OVERWRITE:
+		free((char *)in_tok->value);
+		in_tok->value = xstrdup(arg_tok->value);
+		free_trailer_item(arg_tok);
+		break;
+	case EXISTS_ADD:
+		add_arg_to_input_list(in_tok, arg_tok);
+		break;
+	case EXISTS_ADD_IF_DIFFERENT:
+		if (check_if_different(in_tok, arg_tok, alnum_len, 1))
+			add_arg_to_input_list(in_tok, arg_tok);
+		else
+			free_trailer_item(arg_tok);
+		break;
+	case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
+		if (check_if_different(in_tok, arg_tok, alnum_len, 0))
+			add_arg_to_input_list(in_tok, arg_tok);
+		else
+			free_trailer_item(arg_tok);
+		break;
+	}
+}
+
+static void remove_from_list(struct trailer_item *item,
+			     struct trailer_item **first)
+{
+	if (item->next)
+		item->next->previous = item->previous;
+	if (item->previous)
+		item->previous->next = item->next;
+	else
+		*first = item->next;
+}
+
+static struct trailer_item *remove_first(struct trailer_item **first)
+{
+	struct trailer_item *item = *first;
+	*first = item->next;
+	if (item->next) {
+		item->next->previous = NULL;
+		item->next = NULL;
+	}
+	return item;
+}
+
+static void process_input_token(struct trailer_item *in_tok,
+				struct trailer_item **arg_tok_first,
+				enum action_where where)
+{
+	struct trailer_item *arg_tok;
+	struct trailer_item *next_arg;
+
+	int after = where == WHERE_AFTER;
+	int tok_alnum_len = alnum_len(in_tok->token, strlen(in_tok->token));
+
+	for (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {
+		next_arg = arg_tok->next;
+		if (!same_token(in_tok, arg_tok, tok_alnum_len))
+			continue;
+		if (arg_tok->conf.where != where)
+			continue;
+		remove_from_list(arg_tok, arg_tok_first);
+		apply_arg_if_exists(in_tok, arg_tok, tok_alnum_len);
+		/*
+		 * If arg has been added to input,
+		 * then we need to process it too now.
+		 */
+		if ((after ? in_tok->next : in_tok->previous) == arg_tok)
+			in_tok = arg_tok;
+	}
+}
+
+static void update_last(struct trailer_item **last)
+{
+	if (*last)
+		while ((*last)->next != NULL)
+			*last = (*last)->next;
+}
+
+static void update_first(struct trailer_item **first)
+{
+	if (*first)
+		while ((*first)->previous != NULL)
+			*first = (*first)->previous;
+}
+
+static void apply_arg_if_missing(struct trailer_item **in_tok_first,
+				 struct trailer_item **in_tok_last,
+				 struct trailer_item *arg_tok)
+{
+	struct trailer_item **in_tok;
+	enum action_where where;
+
+	switch (arg_tok->conf.if_missing) {
+	case MISSING_DO_NOTHING:
+		free_trailer_item(arg_tok);
+		break;
+	case MISSING_ADD:
+		where = arg_tok->conf.where;
+		in_tok = (where == WHERE_AFTER) ? in_tok_last : in_tok_first;
+		if (*in_tok) {
+			add_arg_to_input_list(*in_tok, arg_tok);
+			*in_tok = arg_tok;
+		} else {
+			*in_tok_first = arg_tok;
+			*in_tok_last = arg_tok;
+		}
+		break;
+	}
+}
+
+static void process_trailers_lists(struct trailer_item **in_tok_first,
+				   struct trailer_item **in_tok_last,
+				   struct trailer_item **arg_tok_first)
+{
+	struct trailer_item *in_tok;
+	struct trailer_item *arg_tok;
+
+	if (!*arg_tok_first)
+		return;
+
+	/* Process input from end to start */
+	for (in_tok = *in_tok_last; in_tok; in_tok = in_tok->previous)
+		process_input_token(in_tok, arg_tok_first, WHERE_AFTER);
+
+	update_last(in_tok_last);
+
+	if (!*arg_tok_first)
+		return;
+
+	/* Process input from start to end */
+	for (in_tok = *in_tok_first; in_tok; in_tok = in_tok->next)
+		process_input_token(in_tok, arg_tok_first, WHERE_BEFORE);
+
+	update_first(in_tok_first);
+
+	/* Process args left */
+	while (*arg_tok_first) {
+		arg_tok = remove_first(arg_tok_first);
+		apply_arg_if_missing(in_tok_first, in_tok_last, arg_tok);
+	}
+}
-- 
1.9.0.164.g3aa33cd.dirty

[PATCH v9 03/11] trailer: read and process config information

[Christian Couder]

Read the configuration to get trailer information, and then process
it and storing it in a doubly linked list.

The config information is stored in the list whose first item is
pointed to by:

static struct trailer_item *first_conf_item;

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 trailer.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 146 insertions(+)

diff --git a/trailer.c b/trailer.c
index 52108c2..f7714d5 100644
--- a/trailer.c
+++ b/trailer.c
@@ -25,6 +25,8 @@ struct trailer_item {
 	struct conf_info conf;
 };
 
+static struct trailer_item *first_conf_item;
+
 static int same_token(struct trailer_item *a, struct trailer_item *b, int alnum_len)
 {
 	return !strncasecmp(a->token, b->token, alnum_len);
@@ -245,3 +247,147 @@ static void process_trailers_lists(struct trailer_item **in_tok_first,
 		apply_arg_if_missing(in_tok_first, in_tok_last, arg_tok);
 	}
 }
+
+static int set_where(struct conf_info *item, const char *value)
+{
+	if (!strcmp("after", value))
+		item->where = WHERE_AFTER;
+	else if (!strcmp("before", value))
+		item->where = WHERE_BEFORE;
+	else
+		return -1;
+	return 0;
+}
+
+static int set_if_exists(struct conf_info *item, const char *value)
+{
+	if (!strcmp("addIfDifferent", value))
+		item->if_exists = EXISTS_ADD_IF_DIFFERENT;
+	else if (!strcmp("addIfDifferentNeighbor", value))
+		item->if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
+	else if (!strcmp("add", value))
+		item->if_exists = EXISTS_ADD;
+	else if (!strcmp("overwrite", value))
+		item->if_exists = EXISTS_OVERWRITE;
+	else if (!strcmp("doNothing", value))
+		item->if_exists = EXISTS_DO_NOTHING;
+	else
+		return -1;
+	return 0;
+}
+
+static int set_if_missing(struct conf_info *item, const char *value)
+{
+	if (!strcmp("doNothing", value))
+		item->if_missing = MISSING_DO_NOTHING;
+	else if (!strcmp("add", value))
+		item->if_missing = MISSING_ADD;
+	else
+		return -1;
+	return 0;
+}
+
+static struct trailer_item *get_conf_item(const char *name)
+{
+	struct trailer_item *item;
+	struct trailer_item *previous;
+
+	/* Look up item with same name */
+	for (previous = NULL, item = first_conf_item;
+	     item;
+	     previous = item, item = item->next) {
+		if (!strcasecmp(item->conf.name, name))
+			return item;
+	}
+
+	/* Item does not already exists, create it */
+	item = xcalloc(sizeof(struct trailer_item), 1);
+	item->conf.name = xstrdup(name);
+
+	if (!previous)
+		first_conf_item = item;
+	else {
+		previous->next = item;
+		item->previous = previous;
+	}
+
+	return item;
+}
+
+enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_WHERE,
+			 TRAILER_IF_EXISTS, TRAILER_IF_MISSING };
+
+static struct {
+	const char *name;
+	enum trailer_info_type type;
+} trailer_config_items[] = {
+	{ "key", TRAILER_KEY },
+	{ "command", TRAILER_COMMAND },
+	{ "where", TRAILER_WHERE },
+	{ "ifexists", TRAILER_IF_EXISTS },
+	{ "ifmissing", TRAILER_IF_MISSING }
+};
+
+static int git_trailer_config(const char *conf_key, const char *value, void *cb)
+{
+	const char *trailer_item, *variable_name;
+	struct trailer_item *item;
+	struct conf_info *conf;
+	char *name = NULL;
+	enum trailer_info_type type;
+	int i;
+
+	trailer_item = skip_prefix(conf_key, "trailer.");
+	if (!trailer_item)
+		return 0;
+
+	variable_name = strrchr(trailer_item, '.');
+	if (!variable_name) {
+		warning(_("two level trailer config variable %s"), conf_key);
+		return 0;
+	}
+
+	variable_name++;
+	for (i = 0; i < ARRAY_SIZE(trailer_config_items); i++) {
+		if (strcmp(trailer_config_items[i].name, variable_name))
+			continue;
+		name = xstrndup(trailer_item,  variable_name - trailer_item -1);
+		type = trailer_config_items[i].type;
+		break;
+	}
+
+	if (!name)
+		return 0;
+
+	item = get_conf_item(name);
+	conf = &item->conf;
+	free(name);
+
+	switch (type) {
+	case TRAILER_KEY:
+		if (conf->key)
+			warning(_("more than one %s"), conf_key);
+		conf->key = xstrdup(value);
+		break;
+	case TRAILER_COMMAND:
+		if (conf->command)
+			warning(_("more than one %s"), conf_key);
+		conf->command = xstrdup(value);
+		break;
+	case TRAILER_WHERE:
+		if (set_where(conf, value))
+			warning(_("unknown value '%s' for key '%s'"), value, conf_key);
+		break;
+	case TRAILER_IF_EXISTS:
+		if (set_if_exists(conf, value))
+			warning(_("unknown value '%s' for key '%s'"), value, conf_key);
+		break;
+	case TRAILER_IF_MISSING:
+		if (set_if_missing(conf, value))
+			warning(_("unknown value '%s' for key '%s'"), value, conf_key);
+		break;
+	default:
+		die("internal bug in trailer.c");
+	}
+	return 0;
+}
-- 
1.9.0.164.g3aa33cd.dirty

[PATCH v9 04/11] trailer: process command line trailer arguments

[Christian Couder]

Parse the trailer command line arguments and put
the result into an arg_tok doubly linked list.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 trailer.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/trailer.c b/trailer.c
index f7714d5..743ff23 100644
--- a/trailer.c
+++ b/trailer.c
@@ -391,3 +391,106 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
 	}
 	return 0;
 }
+
+static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char *trailer)
+{
+	size_t len = strcspn(trailer, "=:");
+	if (len == 0)
+		return error(_("empty trailer token in trailer '%s'"), trailer);
+	if (len < strlen(trailer)) {
+		strbuf_add(tok, trailer, len);
+		strbuf_trim(tok);
+		strbuf_addstr(val, trailer + len + 1);
+		strbuf_trim(val);
+	} else {
+		strbuf_addstr(tok, trailer);
+		strbuf_trim(tok);
+	}
+	return 0;
+}
+
+
+static void duplicate_conf(struct conf_info *dst, struct conf_info *src)
+{
+	*dst = *src;
+	if (src->name)
+		dst->name = xstrdup(src->name);
+	if (src->key)
+		dst->key = xstrdup(src->key);
+	if (src->command)
+		dst->command = xstrdup(src->command);
+}
+
+static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,
+					     char *tok, char *val)
+{
+	struct trailer_item *new = xcalloc(sizeof(*new), 1);
+	new->value = val;
+
+	if (conf_item) {
+		duplicate_conf(&new->conf, &conf_item->conf);
+		new->token = xstrdup(conf_item->conf.key);
+		free(tok);
+	} else
+		new->token = tok;
+
+	return new;
+}
+
+static struct trailer_item *create_trailer_item(const char *string)
+{
+	struct strbuf tok = STRBUF_INIT;
+	struct strbuf val = STRBUF_INIT;
+	struct trailer_item *item;
+	int tok_alnum_len;
+
+	if (parse_trailer(&tok, &val, string))
+		return NULL;
+
+	tok_alnum_len = alnum_len(tok.buf, tok.len);
+
+	/* Lookup if the token matches something in the config */
+	for (item = first_conf_item; item; item = item->next) {
+		if (!strncasecmp(tok.buf, item->conf.key, tok_alnum_len) ||
+		    !strncasecmp(tok.buf, item->conf.name, tok_alnum_len)) {
+			strbuf_release(&tok);
+			return new_trailer_item(item,
+						NULL,
+						strbuf_detach(&val, NULL));
+		}
+	}
+
+	return new_trailer_item(NULL,
+				strbuf_detach(&tok, NULL),
+				strbuf_detach(&val, NULL));
+}
+
+static void add_trailer_item(struct trailer_item **first,
+			     struct trailer_item **last,
+			     struct trailer_item *new)
+{
+	if (!new)
+		return;
+	if (!*last) {
+		*first = new;
+		*last = new;
+	} else {
+		(*last)->next = new;
+		new->previous = *last;
+		*last = new;
+	}
+}
+
+static struct trailer_item *process_command_line_args(int argc, const char **argv)
+{
+	int i;
+	struct trailer_item *arg_tok_first = NULL;
+	struct trailer_item *arg_tok_last = NULL;
+
+	for (i = 0; i < argc; i++) {
+		struct trailer_item *new = create_trailer_item(argv[i]);
+		add_trailer_item(&arg_tok_first, &arg_tok_last, new);
+	}
+
+	return arg_tok_first;
+}
-- 
1.9.0.164.g3aa33cd.dirty

[PATCH v9 05/11] trailer: parse trailers from stdin

[Christian Couder]

Read trailers from stdin, parse them and put the result into a doubly linked
list.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 trailer.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/trailer.c b/trailer.c
index 743ff23..953ae51 100644
--- a/trailer.c
+++ b/trailer.c
@@ -50,6 +50,14 @@ static size_t alnum_len(const char *buf, size_t len)
 	return len;
 }
 
+static inline int contains_only_spaces(const char *str)
+{
+	const char *s = str;
+	while (*s && isspace(*s))
+		s++;
+	return !*s;
+}
+
 static void free_trailer_item(struct trailer_item *item)
 {
 	free(item->conf.name);
@@ -494,3 +502,71 @@ static struct trailer_item *process_command_line_args(int argc, const char **arg
 
 	return arg_tok_first;
 }
+
+static struct strbuf **read_stdin(void)
+{
+	struct strbuf **lines;
+	struct strbuf sb = STRBUF_INIT;
+
+	if (strbuf_read(&sb, fileno(stdin), 0) < 0)
+		die_errno(_("could not read from stdin"));
+
+	lines = strbuf_split(&sb, '\n');
+
+	strbuf_release(&sb);
+
+	return lines;
+}
+
+/*
+ * Return the the (0 based) index of the first trailer line
+ * or the line count if there are no trailers.
+ */
+static int find_trailer_start(struct strbuf **lines)
+{
+	int start, empty = 1, count = 0;
+
+	/* Get the line count */
+	while (lines[count])
+		count++;
+
+	/*
+	 * Get the start of the trailers by looking starting from the end
+	 * for a line with only spaces before lines with one ':'.
+	 */
+	for (start = count - 1; start >= 0; start--) {
+		if (contains_only_spaces(lines[start]->buf)) {
+			if (empty)
+				continue;
+			return start + 1;
+		}
+		if (strchr(lines[start]->buf, ':')) {
+			if (empty)
+				empty = 0;
+			continue;
+		}
+		return count;
+	}
+
+	return empty ? count : start + 1;
+}
+
+static void process_stdin(struct trailer_item **in_tok_first,
+			  struct trailer_item **in_tok_last)
+{
+	struct strbuf **lines = read_stdin();
+	int start = find_trailer_start(lines);
+	int i;
+
+	/* Print non trailer lines as is */
+	for (i = 0; lines[i] && i < start; i++)
+		printf("%s", lines[i]->buf);
+
+	/* Parse trailer lines */
+	for (i = start; lines[i]; i++) {
+		struct trailer_item *new = create_trailer_item(lines[i]->buf);
+		add_trailer_item(in_tok_first, in_tok_last, new);
+	}
+
+	strbuf_list_free(lines);
+}
-- 
1.9.0.164.g3aa33cd.dirty

[PATCH v9 06/11] trailer: put all the processing together and print

[Christian Couder]

This patch adds the process_trailers() function that
calls all the previously added processing functions
and then prints the results on the standard output.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 trailer.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 trailer.h |  6 ++++++
 2 files changed, 55 insertions(+)
 create mode 100644 trailer.h

diff --git a/trailer.c b/trailer.c
index 953ae51..5d2b3ad 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "trailer.h"
 /*
  * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
  */
@@ -68,6 +69,26 @@ static void free_trailer_item(struct trailer_item *item)
 	free(item);
 }
 
+static void print_tok_val(const char *tok, const char *val)
+{
+	char c = tok[strlen(tok) - 1];
+	if (isalnum(c))
+		printf("%s: %s\n", tok, val);
+	else if (isspace(c) || c == '#')
+		printf("%s%s\n", tok, val);
+	else
+		printf("%s %s\n", tok, val);
+}
+
+static void print_all(struct trailer_item *first, int trim_empty)
+{
+	struct trailer_item *item;
+	for (item = first; item; item = item->next) {
+		if (!trim_empty || strlen(item->value) > 0)
+			print_tok_val(item->token, item->value);
+	}
+}
+
 static void add_arg_to_input_list(struct trailer_item *in_tok,
 				  struct trailer_item *arg_tok)
 {
@@ -570,3 +591,31 @@ static void process_stdin(struct trailer_item **in_tok_first,
 
 	strbuf_list_free(lines);
 }
+
+static void free_all(struct trailer_item **first)
+{
+	while (*first) {
+		struct trailer_item *item = remove_first(first);
+		free_trailer_item(item);
+	}
+}
+
+void process_trailers(int trim_empty, int argc, const char **argv)
+{
+	struct trailer_item *in_tok_first = NULL;
+	struct trailer_item *in_tok_last = NULL;
+	struct trailer_item *arg_tok_first;
+
+	git_config(git_trailer_config, NULL);
+
+	/* Print the non trailer part of stdin */
+	process_stdin(&in_tok_first, &in_tok_last);
+
+	arg_tok_first = process_command_line_args(argc, argv);
+
+	process_trailers_lists(&in_tok_first, &in_tok_last, &arg_tok_first);
+
+	print_all(in_tok_first, trim_empty);
+
+	free_all(&in_tok_first);
+}
diff --git a/trailer.h b/trailer.h
new file mode 100644
index 0000000..9323b1e
--- /dev/null
+++ b/trailer.h
@@ -0,0 +1,6 @@
+#ifndef TRAILER_H
+#define TRAILER_H
+
+void process_trailers(int trim_empty, int argc, const char **argv);
+
+#endif /* TRAILER_H */
-- 
1.9.0.164.g3aa33cd.dirty

[PATCH v9 07/11] trailer: add interpret-trailers command

[Christian Couder]

This patch adds the "git interpret-trailers" command.
This command uses the previously added process_trailers()
function in trailer.c.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 .gitignore                   |  1 +
 Makefile                     |  1 +
 builtin.h                    |  1 +
 builtin/interpret-trailers.c | 33 +++++++++++++++++++++++++++++++++
 git.c                        |  1 +
 5 files changed, 37 insertions(+)
 create mode 100644 builtin/interpret-trailers.c

diff --git a/.gitignore b/.gitignore
index dc600f9..c2a0b19 100644
--- a/.gitignore
+++ b/.gitignore
@@ -74,6 +74,7 @@
 /git-index-pack
 /git-init
 /git-init-db
+/git-interpret-trailers
 /git-instaweb
 /git-log
 /git-ls-files
diff --git a/Makefile b/Makefile
index ec67ae1..94f7e95 100644
--- a/Makefile
+++ b/Makefile
@@ -947,6 +947,7 @@ BUILTIN_OBJS += builtin/hash-object.o
 BUILTIN_OBJS += builtin/help.o
 BUILTIN_OBJS += builtin/index-pack.o
 BUILTIN_OBJS += builtin/init-db.o
+BUILTIN_OBJS += builtin/interpret-trailers.o
 BUILTIN_OBJS += builtin/log.o
 BUILTIN_OBJS += builtin/ls-files.o
 BUILTIN_OBJS += builtin/ls-remote.o
diff --git a/builtin.h b/builtin.h
index c47c110..8ca0065 100644
--- a/builtin.h
+++ b/builtin.h
@@ -73,6 +73,7 @@ extern int cmd_hash_object(int argc, const char **argv, const char *prefix);
 extern int cmd_help(int argc, const char **argv, const char *prefix);
 extern int cmd_index_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_init_db(int argc, const char **argv, const char *prefix);
+extern int cmd_interpret_trailers(int argc, const char **argv, const char *prefix);
 extern int cmd_log(int argc, const char **argv, const char *prefix);
 extern int cmd_log_reflog(int argc, const char **argv, const char *prefix);
 extern int cmd_ls_files(int argc, const char **argv, const char *prefix);
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
new file mode 100644
index 0000000..0c8ca72
--- /dev/null
+++ b/builtin/interpret-trailers.c
@@ -0,0 +1,33 @@
+/*
+ * Builtin "git interpret-trailers"
+ *
+ * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
+ *
+ */
+
+#include "cache.h"
+#include "builtin.h"
+#include "parse-options.h"
+#include "trailer.h"
+
+static const char * const git_interpret_trailers_usage[] = {
+	N_("git interpret-trailers [--trim-empty] [(<token>[(=|:)<value>])...]"),
+	NULL
+};
+
+int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
+{
+	int trim_empty = 0;
+
+	struct option options[] = {
+		OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty trailers")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			     git_interpret_trailers_usage, 0);
+
+	process_trailers(trim_empty, argc, argv);
+
+	return 0;
+}
diff --git a/git.c b/git.c
index 7cf2953..63a03eb 100644
--- a/git.c
+++ b/git.c
@@ -380,6 +380,7 @@ static struct cmd_struct commands[] = {
 	{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY },
 	{ "init", cmd_init_db },
 	{ "init-db", cmd_init_db },
+	{ "interpret-trailers", cmd_interpret_trailers, RUN_SETUP },
 	{ "log", cmd_log, RUN_SETUP },
 	{ "ls-files", cmd_ls_files, RUN_SETUP },
 	{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
-- 
1.9.0.164.g3aa33cd.dirty

[PATCH v9 08/11] trailer: add tests for "git interpret-trailers"

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t7513-interpret-trailers.sh | 336 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 336 insertions(+)
 create mode 100755 t/t7513-interpret-trailers.sh

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
new file mode 100755
index 0000000..417a4f3
--- /dev/null
+++ b/t/t7513-interpret-trailers.sh
@@ -0,0 +1,336 @@
+#!/bin/sh
+#
+# Copyright (c) 2013 Christian Couder
+#
+
+test_description='git interpret-trailers'
+
+. ./test-lib.sh
+
+# When we want one trailing space at the end of each line, let's use sed
+# to make sure that these spaces are not removed by any automatic tool.
+
+test_expect_success 'setup' '
+	cat >basic_message <<-\EOF &&
+		subject
+
+		body
+	EOF
+	cat >complex_message_body <<-\EOF &&
+		my subject
+
+		my body which is long
+		and contains some special
+		chars like : = ? !
+
+	EOF
+	sed -e "s/ Z\$/ /" >complex_message_trailers <<-\EOF
+		Fixes: Z
+		Acked-by: Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+'
+
+test_expect_success 'without config' '
+	sed -e "s/ Z\$/ /" >expected <<-\EOF &&
+		ack: Peff
+		Reviewed-by: Z
+		Acked-by: Johan
+	EOF
+	git interpret-trailers "ack = Peff" "Reviewed-by" "Acked-by: Johan" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--trim-empty without config' '
+	cat >expected <<-\EOF &&
+		ack: Peff
+		Acked-by: Johan
+	EOF
+	git interpret-trailers --trim-empty "ack = Peff" "Reviewed-by" "Acked-by: Johan" "sob:" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'with config setup' '
+	git config trailer.ack.key "Acked-by: " &&
+	cat >expected <<-\EOF &&
+		Acked-by: Peff
+	EOF
+	git interpret-trailers --trim-empty "ack = Peff" >actual &&
+	test_cmp expected actual &&
+	git interpret-trailers --trim-empty "Acked-by = Peff" >actual &&
+	test_cmp expected actual &&
+	git interpret-trailers --trim-empty "Acked-by :Peff" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'with config setup and = sign' '
+	git config trailer.ack.key "Acked-by= " &&
+	cat >expected <<-\EOF &&
+		Acked-by= Peff
+	EOF
+	git interpret-trailers --trim-empty "ack = Peff" >actual &&
+	test_cmp expected actual &&
+	git interpret-trailers --trim-empty "Acked-by= Peff" >actual &&
+	test_cmp expected actual &&
+	git interpret-trailers --trim-empty "Acked-by : Peff" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'with config setup and # sign' '
+	git config trailer.bug.key "Bug #" &&
+	cat >expected <<-\EOF &&
+		Bug #42
+	EOF
+	git interpret-trailers --trim-empty "bug = 42" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'with commit basic message' '
+	git interpret-trailers <basic_message >actual &&
+	test_cmp basic_message actual
+'
+
+test_expect_success 'with commit complex message' '
+	cat complex_message_body complex_message_trailers >complex_message &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by= Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'with commit complex message and args' '
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by= Z
+		Acked-by= Peff
+		Reviewed-by: Z
+		Signed-off-by: Z
+		Bug #42
+	EOF
+	git interpret-trailers "ack: Peff" "bug: 42" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'with commit complex message, args and --trim-empty' '
+	cat complex_message_body >expected &&
+	cat >>expected <<-\EOF &&
+		Acked-by= Peff
+		Bug #42
+	EOF
+	git interpret-trailers --trim-empty "ack: Peff" "bug: 42" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "where = before"' '
+	git config trailer.bug.where "before" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Bug #42
+		Fixes: Z
+		Acked-by= Z
+		Acked-by= Peff
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers "ack: Peff" "bug: 42" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "where = before" for a token in the middle of the message' '
+	git config trailer.review.key "Reviewed-by:" &&
+	git config trailer.review.where "before" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Bug #42
+		Fixes: Z
+		Acked-by= Z
+		Acked-by= Peff
+		Reviewed-by: Johan
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers "ack: Peff" "bug: 42" "review: Johan" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "where = before" and --trim-empty' '
+	cat complex_message_body >expected &&
+	cat >>expected <<-\EOF &&
+		Bug #46
+		Bug #42
+		Acked-by= Peff
+		Reviewed-by: Johan
+	EOF
+	git interpret-trailers --trim-empty "ack: Peff" "bug: 42" "review: Johan" "Bug: 46" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'the default is "ifExists = addIfDifferent"' '
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Bug #42
+		Fixes: Z
+		Acked-by= Z
+		Acked-by= Peff
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers "ack: Peff" "review:" "bug: 42" "ack: Peff" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifExists = addIfDifferent"' '
+	git config trailer.review.ifExists "addIfDifferent" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Bug #42
+		Fixes: Z
+		Acked-by= Z
+		Acked-by= Peff
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers "ack: Peff" "review:" "bug: 42" "ack: Peff" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifExists = addIfDifferentNeighbor"' '
+	git config trailer.ack.ifExists "addIfDifferentNeighbor" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Bug #42
+		Fixes: Z
+		Acked-by= Z
+		Acked-by= Peff
+		Acked-by= Junio
+		Acked-by= Peff
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers "ack: Peff" "review:" "ack: Junio" "bug: 42" "ack: Peff" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifExists = addIfDifferentNeighbor" and --trim-empty' '
+	git config trailer.ack.ifExists "addIfDifferentNeighbor" &&
+	cat complex_message_body >expected &&
+	cat >>expected <<-\EOF &&
+		Bug #42
+		Acked-by= Peff
+		Acked-by= Junio
+		Acked-by= Peff
+	EOF
+	git interpret-trailers --trim-empty "ack: Peff" "Acked-by= Peff" "review:" "ack: Junio" "bug: 42" "ack: Peff" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifExists = add"' '
+	git config trailer.ack.ifExists "add" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Bug #42
+		Fixes: Z
+		Acked-by= Z
+		Acked-by= Peff
+		Acked-by= Peff
+		Acked-by= Junio
+		Acked-by= Peff
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers "ack: Peff" "Acked-by= Peff" "review:" "ack: Junio" "bug: 42" "ack: Peff" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifExists = overwrite"' '
+	git config trailer.fix.key "Fixes:" &&
+	git config trailer.fix.ifExists "overwrite" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Bug #42
+		Fixes: 22
+		Acked-by= Z
+		Acked-by= Junio
+		Acked-by= Peff
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers "review:" "fix=53" "ack: Junio" "fix=22" "bug: 42" "ack: Peff" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifExists = doNothing"' '
+	git config trailer.fix.ifExists "doNothing" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Bug #42
+		Fixes: Z
+		Acked-by= Z
+		Acked-by= Junio
+		Acked-by= Peff
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers "review:" "fix=53" "ack: Junio" "fix=22" "bug: 42" "ack: Peff" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'the default is "ifMissing = add"' '
+	git config trailer.cc.key "Cc: " &&
+	git config trailer.cc.where "before" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Bug #42
+		Cc: Linus
+		Fixes: Z
+		Acked-by= Z
+		Acked-by= Junio
+		Acked-by= Peff
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers "review:" "fix=53" "cc=Linus" "ack: Junio" "fix=22" "bug: 42" "ack: Peff" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifMissing = add"' '
+	git config trailer.cc.ifMissing "add" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Cc: Linus
+		Bug #42
+		Fixes: Z
+		Acked-by= Z
+		Acked-by= Junio
+		Acked-by= Peff
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers "review:" "fix=53" "ack: Junio" "fix=22" "bug: 42" "cc=Linus" "ack: Peff" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifMissing = doNothing"' '
+	git config trailer.cc.ifMissing "doNothing" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Bug #42
+		Fixes: Z
+		Acked-by= Z
+		Acked-by= Junio
+		Acked-by= Peff
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers "review:" "fix=53" "cc=Linus" "ack: Junio" "fix=22" "bug: 42" "ack: Peff" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_done
-- 
1.9.0.164.g3aa33cd.dirty

[PATCH v9 09/11] trailer: execute command from 'trailer.<name>.command'

[Christian Couder]

Let the user specify a command that will give on its standard output
the value to use for the specified trailer.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 trailer.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/trailer.c b/trailer.c
index 5d2b3ad..bc96643 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "run-command.h"
 #include "trailer.h"
 /*
  * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
@@ -13,11 +14,14 @@ struct conf_info {
 	char *name;
 	char *key;
 	char *command;
+	unsigned command_uses_arg : 1;
 	enum action_where where;
 	enum action_if_exists if_exists;
 	enum action_if_missing if_missing;
 };
 
+#define TRAILER_ARG_STRING "$ARG"
+
 struct trailer_item {
 	struct trailer_item *previous;
 	struct trailer_item *next;
@@ -59,6 +63,13 @@ static inline int contains_only_spaces(const char *str)
 	return !*s;
 }
 
+static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *b)
+{
+	const char *ptr = strstr(sb->buf, a);
+	if (ptr)
+		strbuf_splice(sb, ptr - sb->buf, strlen(a), b, strlen(b));
+}
+
 static void free_trailer_item(struct trailer_item *item)
 {
 	free(item->conf.name);
@@ -402,6 +413,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
 		if (conf->command)
 			warning(_("more than one %s"), conf_key);
 		conf->command = xstrdup(value);
+		conf->command_uses_arg = !!strstr(conf->command, TRAILER_ARG_STRING);
 		break;
 	case TRAILER_WHERE:
 		if (set_where(conf, value))
@@ -438,6 +450,44 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char *tra
 	return 0;
 }
 
+static int read_from_command(struct child_process *cp, struct strbuf *buf)
+{
+	if (run_command(cp))
+		return error("running trailer command '%s' failed", cp->argv[0]);
+	if (strbuf_read(buf, cp->out, 1024) < 1)
+		return error("reading from trailer command '%s' failed", cp->argv[0]);
+	strbuf_trim(buf);
+	return 0;
+}
+
+static const char *apply_command(const char *command, const char *arg)
+{
+	struct strbuf cmd = STRBUF_INIT;
+	struct strbuf buf = STRBUF_INIT;
+	struct child_process cp;
+	const char *argv[] = {NULL, NULL};
+	const char *result = "";
+
+	strbuf_addstr(&cmd, command);
+	if (arg)
+		strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
+
+	argv[0] = cmd.buf;
+	memset(&cp, 0, sizeof(cp));
+	cp.argv = argv;
+	cp.env = local_repo_env;
+	cp.no_stdin = 1;
+	cp.out = -1;
+	cp.use_shell = 1;
+
+	if (read_from_command(&cp, &buf))
+		strbuf_release(&buf);
+	else
+		result = strbuf_detach(&buf, NULL);
+
+	strbuf_release(&cmd);
+	return result;
+}
 
 static void duplicate_conf(struct conf_info *dst, struct conf_info *src)
 {
@@ -460,6 +510,10 @@ static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,
 		duplicate_conf(&new->conf, &conf_item->conf);
 		new->token = xstrdup(conf_item->conf.key);
 		free(tok);
+		if (conf_item->conf.command_uses_arg || !val) {
+			new->value = apply_command(conf_item->conf.command, val);
+			free(val);
+		}
 	} else
 		new->token = tok;
 
@@ -515,12 +569,21 @@ static struct trailer_item *process_command_line_args(int argc, const char **arg
 	int i;
 	struct trailer_item *arg_tok_first = NULL;
 	struct trailer_item *arg_tok_last = NULL;
+	struct trailer_item *item;
 
 	for (i = 0; i < argc; i++) {
 		struct trailer_item *new = create_trailer_item(argv[i]);
 		add_trailer_item(&arg_tok_first, &arg_tok_last, new);
 	}
 
+	/* Add conf commands that don't use $ARG */
+	for (item = first_conf_item; item; item = item->next) {
+		if (item->conf.command && !item->conf.command_uses_arg) {
+			struct trailer_item *new = new_trailer_item(item, NULL, NULL);
+			add_trailer_item(&arg_tok_first, &arg_tok_last, new);
+		}
+	}
+
 	return arg_tok_first;
 }
 
-- 
1.9.0.164.g3aa33cd.dirty

[PATCH v9 10/11] trailer: add tests for commands in config file

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t7513-interpret-trailers.sh | 71 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 417a4f3..0a1f3b6 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -333,4 +333,75 @@ test_expect_success 'using "ifMissing = doNothing"' '
 	test_cmp expected actual
 '
 
+test_expect_success 'with simple command' '
+	git config trailer.sign.key "Signed-off-by: " &&
+	git config trailer.sign.where "after" &&
+	git config trailer.sign.ifExists "addIfDifferentNeighbor" &&
+	git config trailer.sign.command "echo \"A U Thor <author@example.com>\"" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by= Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+		Signed-off-by: A U Thor <author@example.com>
+	EOF
+	git interpret-trailers "review:" "fix=22" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'with command using commiter information' '
+	git config trailer.sign.ifExists "addIfDifferent" &&
+	git config trailer.sign.command "echo \"\$GIT_COMMITTER_NAME <\$GIT_COMMITTER_EMAIL>\"" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by= Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+		Signed-off-by: C O Mitter <committer@example.com>
+	EOF
+	git interpret-trailers "review:" "fix=22" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'with command using author information' '
+	git config trailer.sign.key "Signed-off-by: " &&
+	git config trailer.sign.where "after" &&
+	git config trailer.sign.ifExists "addIfDifferentNeighbor" &&
+	git config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by= Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+		Signed-off-by: A U Thor <author@example.com>
+	EOF
+	git interpret-trailers "review:" "fix=22" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'setup a commit' '
+	echo "Content of the first commit." > a.txt &&
+	git add a.txt &&
+	git commit -m "Add file a.txt"
+'
+
+test_expect_success 'with command using $ARG' '
+	git config trailer.fix.ifExists "overwrite" &&
+	git config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$ARG" &&
+	FIXED=$(git log -1 --oneline --format="%h (%s)" --abbrev-commit --abbrev=14 HEAD) &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-EOF &&
+		Fixes: $FIXED
+		Acked-by= Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+		Signed-off-by: A U Thor <author@example.com>
+	EOF
+	git interpret-trailers "review:" "fix=HEAD" <complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
1.9.0.164.g3aa33cd.dirty

[PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-interpret-trailers.txt | 123 +++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)
 create mode 100644 Documentation/git-interpret-trailers.txt

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
new file mode 100644
index 0000000..75ae386
--- /dev/null
+++ b/Documentation/git-interpret-trailers.txt
@@ -0,0 +1,123 @@
+git-interpret-trailers(1)
+=========================
+
+NAME
+----
+git-interpret-trailers - help add stuctured information into commit messages
+
+SYNOPSIS
+--------
+[verse]
+'git interpret-trailers' [--trim-empty] [(<token>[(=|:)<value>])...]
+
+DESCRIPTION
+-----------
+Help add RFC 822-like headers, called 'trailers', at the end of the
+otherwise free-form part of a commit message.
+
+This command is a filter. It reads the standard input for a commit
+message and applies the `token` arguments, if any, to this
+message. The resulting message is emited on the standard output.
+
+Some configuration variables control the way the `token` arguments are
+applied to the message and the way any existing trailer in the message
+is changed. They also make it possible to automatically add some
+trailers.
+
+By default, a 'token=value' or 'token:value' argument will be added
+only if no trailer with the same (token, value) pair is already in the
+message. The 'token' and 'value' parts will be trimmed to remove
+starting and trailing whitespace, and the resulting trimmed 'token'
+and 'value' will appear in the message like this:
+
+------------------------------------------------
+token: value
+------------------------------------------------
+
+By default, if there are already trailers with the same 'token', the
+new trailer will appear just after the last trailer with the same
+'token'. Otherwise it will appear at the end of the message.
+
+Note that 'trailers' do not follow and are not intended to follow many
+rules that are in RFC 822. For example they do not follow the line
+breaking rules, the encoding rules and probably many other rules.
+
+OPTIONS
+-------
+--trim-empty::
+	If the 'value' part of any trailer contains only whitespace,
+	the whole trailer will be removed from the resulting message.
+
+CONFIGURATION VARIABLES
+-----------------------
+
+trailer.<token>.key::
+	This 'key' will be used instead of 'token' in the
+	trailer. After some alphanumeric characters, it can contain
+	some non alphanumeric characters like ':', '=' or '#' that will
+	be used instead of ':' to separate the token from the value in
+	the trailer, though the default ':' is more standard.
+
+trailer.<token>.where::
+	This can be either `after`, which is the default, or
+	`before`. If it is `before`, then a trailer with the specified
+	token, will appear before, instead of after, other trailers
+	with the same token, or otherwise at the beginning, instead of
+	at the end, of all the trailers.
+
+trailer.<token>.ifexist::
+	This option makes it possible to choose what action will be
+	performed when there is already at least one trailer with the
+	same token in the message.
++
+The valid values for this option are: `addIfDifferent` (this is the
+default), `addIfDifferentNeighbor`, `add`, `overwrite` or `doNothing`.
++
+With `addIfDifferent`, a new trailer will be added only if no trailer
+with the same (token, value) pair is already in the message.
++
+With `addIfDifferentNeighbor`, a new trailer will be added only if no
+trailer with the same (token, value) pair is above or below the line
+where the new trailer will be added.
++
+With `add`, a new trailer will be added, even if some trailers with
+the same (token, value) pair are already in the message.
++
+With `overwrite`, the new trailer will overwrite an existing trailer
+with the same token.
++
+With `doNothing`, nothing will be done, that is no new trailer will be
+added if there is already one with the same token in the message.
+
+trailer.<token>.ifmissing::
+	This option makes it possible to choose what action will be
+	performed when there is not yet any trailer with the same
+	token in the message.
++
+The valid values for this option are: `add` (this is the default) and
+`doNothing`.
++
+With `add`, a new trailer will be added.
++
+With `doNothing`, nothing will be done.
+
+trailer.<token>.command::
+	This option can be used to specify a shell command that will
+	be used to automatically add or modify a trailer with the
+	specified 'token'.
++
+When this option is specified, it is like if a special 'token=value'
+argument is added at the end of the command line, where 'value' will
+be given by the standard output of the specified command.
++
+If the command contains the `$ARG` string, this string will be
+replaced with the 'value' part of an existing trailer with the same
+token, if any, before the command is launched.
+
+SEE ALSO
+--------
+linkgit:git-commit[1]
+
+GIT
+---
+Part of the linkgit:git[1] suite
-- 
1.9.0.164.g3aa33cd.dirty

Re: [PATCH v9 08/11] trailer: add tests for "git interpret-trailers"

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Christian Couder <chriscool@tuxfamily.org> writes:
>
>> +test_expect_success '--trim-empty without config' '
>> +	cat >expected <<-\EOF &&
>> +		ack: Peff
>> +		Acked-by: Johan
>> +	EOF
>> +	git interpret-trailers --trim-empty "ack = Peff" "Reviewed-by" "Acked-by: Johan" "sob:" >actual &&
>> +	test_cmp expected actual
>> +'
>
> Let's avoid these overlong and unreadable lines by doing something
> like this (just one hunk shown for illustration):
>
>  		ack: Peff
>  		Acked-by: Johan
>  	EOF
> -	git interpret-trailers --trim-empty "ack = Peff" "Reviewed-by" "Acked-by: Johan" "sob:" >actual &&
> +	git interpret-trailers --trim-empty "ack = Peff" \
> +		"Reviewed-by" "Acked-by: Johan" "sob:" >actual &&
>  	test_cmp expected actual
>  '
>  

I'll squash in the line-chopping fix to this, and also fill a
missing whitespace in 3/11 before pushing it out to 'pu'.  Hopefully
the result can be later merged to 'next' without further rerolls.

Thanks.

Re: [PATCH v9 08/11] trailer: add tests for "git interpret-trailers"

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> +test_expect_success '--trim-empty without config' '
> +	cat >expected <<-\EOF &&
> +		ack: Peff
> +		Acked-by: Johan
> +	EOF
> +	git interpret-trailers --trim-empty "ack = Peff" "Reviewed-by" "Acked-by: Johan" "sob:" >actual &&
> +	test_cmp expected actual
> +'

Let's avoid these overlong and unreadable lines by doing something
like this (just one hunk shown for illustration):

 		ack: Peff
 		Acked-by: Johan
 	EOF
-	git interpret-trailers --trim-empty "ack = Peff" "Reviewed-by" "Acked-by: Johan" "sob:" >actual &&
+	git interpret-trailers --trim-empty "ack = Peff" \
+		"Reviewed-by" "Acked-by: Johan" "sob:" >actual &&
 	test_cmp expected actual
 '
 

I've queued the series on 'pu' with the 'chop-overlong-lines' and
another minor fix squashed in; hopefully we can merge to 'next'
soonish.

Thanks.

Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'

[Jonathan Nieder]

(culling cc list)
Hi,

Christian Couder wrote:

> [Subject: Documentation: add documentation for 'git interpret-trailers']
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>

This should be squashed into the patch that introduces the
interpret-trailers command, IMHO (or if it should be reviewed
separately, it can be an earlier patch).  That way, someone looking at
when the command was introduced and wanting to understand what it was
originally meant to do has the information close by.

Thanks for picking up the 'git commit --fixes' topic and your steady
work improving the series.

[...]
> --- /dev/null
> +++ b/Documentation/git-interpret-trailers.txt
> @@ -0,0 +1,123 @@
> +git-interpret-trailers(1)
> +=========================
> +
> +NAME
> +----
> +git-interpret-trailers - help add stuctured information into commit messages
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'git interpret-trailers' [--trim-empty] [(<token>[(=|:)<value>])...]
> +
> +DESCRIPTION
> +-----------
> +Help add RFC 822-like headers, called 'trailers', at the end of the
> +otherwise free-form part of a commit message.
> +
> +This command is a filter. It reads the standard input for a commit
> +message and applies the `token` arguments, if any, to this
> +message. The resulting message is emited on the standard output.

Do you have an example?  Does it work like this?

	$ git interpret-trailers 'signoff=Jonathan Nieder <jrnieder@gmail.com>' <<EOF
	> foo bar baz qux
	> EOF
	foo bar baz qux

	Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
	$

A short EXAMPLES section could help.

If I am understanding it correctly, would a name like 'git add-trailers'
work?  How do I read back the trailers later?

[...]
> +By default, a 'token=value' or 'token:value' argument will be added
> +only if

Why support both '=' and ':'?  Using just one would make it easier to
grep through scripts to see who is adding signoffs.

Hope that helps,
Jonathan

Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'

[Christian Couder]

Hi,

On Wed, Apr 2, 2014 at 2:39 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> (culling cc list)
> Hi,
>
> Christian Couder wrote:
>
>> [Subject: Documentation: add documentation for 'git interpret-trailers']
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>
> This should be squashed into the patch that introduces the
> interpret-trailers command, IMHO (or if it should be reviewed
> separately, it can be an earlier patch).  That way, someone looking at
> when the command was introduced and wanting to understand what it was
> originally meant to do has the information close by.

Well, the series is not very long, so this patch is quite close to the
beginning anyway.

> Thanks for picking up the 'git commit --fixes' topic and your steady
> work improving the series.
>
> [...]
>> --- /dev/null
>> +++ b/Documentation/git-interpret-trailers.txt
>> @@ -0,0 +1,123 @@
>> +git-interpret-trailers(1)
>> +=========================
>> +
>> +NAME
>> +----
>> +git-interpret-trailers - help add stuctured information into commit messages
>> +
>> +SYNOPSIS
>> +--------
>> +[verse]
>> +'git interpret-trailers' [--trim-empty] [(<token>[(=|:)<value>])...]
>> +
>> +DESCRIPTION
>> +-----------
>> +Help add RFC 822-like headers, called 'trailers', at the end of the
>> +otherwise free-form part of a commit message.
>> +
>> +This command is a filter. It reads the standard input for a commit
>> +message and applies the `token` arguments, if any, to this
>> +message. The resulting message is emited on the standard output.
>
> Do you have an example?  Does it work like this?
>
>         $ git interpret-trailers 'signoff=Jonathan Nieder <jrnieder@gmail.com>' <<EOF
>         > foo bar baz qux
>         > EOF
>         foo bar baz qux
>
>         Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>         $

Yeah, that's the idea. But you need to run something like:

$ git config trailer.signoff.key "Signed-off-by:"

to configure it properly first.

By the way trying your example, I found that it is not currently
adding an empty line before the s-o-b.
I will have a look.

> A short EXAMPLES section could help.

Yeah, it is planned, but not yet implemented, as written in patch 0/11:

"The following features are planned but not yet implemented:
        - add more tests related to commands
        - add examples in documentation
        - integration with "git commit""

> If I am understanding it correctly, would a name like 'git add-trailers'
> work?

It could work but it can modify, not just add trailers.

> How do I read back the trailers later?

Why do you want to read them back?
Right now it should be used in hooks to modify commit messages.

> [...]
>> +By default, a 'token=value' or 'token:value' argument will be added
>> +only if
>
> Why support both '=' and ':'?  Using just one would make it easier to
> grep through scripts to see who is adding signoffs.

That was already discussed previously.
The reason is that people are used to "token=value" for command line
arguments, but trailers appears in the result as "token: value", so it
is better for the user if we support both.

Thanks,
Christian.

Re: [PATCH v9 08/11] trailer: add tests for "git interpret-trailers"

[Christian Couder]

On Wed, Apr 2, 2014 at 12:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> +test_expect_success '--trim-empty without config' '
>> +     cat >expected <<-\EOF &&
>> +             ack: Peff
>> +             Acked-by: Johan
>> +     EOF
>> +     git interpret-trailers --trim-empty "ack = Peff" "Reviewed-by" "Acked-by: Johan" "sob:" >actual &&
>> +     test_cmp expected actual
>> +'
>
> Let's avoid these overlong and unreadable lines by doing something
> like this (just one hunk shown for illustration):
>
>                 ack: Peff
>                 Acked-by: Johan
>         EOF
> -       git interpret-trailers --trim-empty "ack = Peff" "Reviewed-by" "Acked-by: Johan" "sob:" >actual &&
> +       git interpret-trailers --trim-empty "ack = Peff" \
> +               "Reviewed-by" "Acked-by: Johan" "sob:" >actual &&
>         test_cmp expected actual
>  '

Ok.

> I've queued the series on 'pu' with the 'chop-overlong-lines' and
> another minor fix squashed in; hopefully we can merge to 'next'
> soonish.

Great!

Thanks,
Christian.

Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> "The following features are planned but not yet implemented:
>         - add more tests related to commands
>         - add examples in documentation
>         - integration with "git commit""

I was planning to merge the series to 'next', but perhaps we should
wait at least for the first two items (I do not think the third one
is necessary to block the series).

>>> +By default, a 'token=value' or 'token:value' argument will be added
>>> +only if
>>
>> Why support both '=' and ':'?  Using just one would make it easier to
>> grep through scripts to see who is adding signoffs.
>
> That was already discussed previously.

I do recall it was discussed previously, but given that a new reader
posed the same question, I am not sure if the end result in this
patch under discussion sufficiently answers the question in a
satisfactory way.

Thanks.

Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Christian Couder <christian.couder@gmail.com> writes:
> ...
>>> Why support both '=' and ':'?  Using just one would make it easier to
>>> grep through scripts to see who is adding signoffs.
>>
>> That was already discussed previously.
>
> I do recall it was discussed previously, but given that a new reader
> posed the same question, I am not sure if the end result in this
> patch under discussion sufficiently answers the question in a
> satisfactory way.
>
> Thanks.

Sorry, I sent out only a half-response before finishing.

>> The reason is that people are used to "token=value" for command line
>> arguments, but trailers appears in the result as "token: value", so it
>> is better for the user if we support both.

While I do understand the part before ", so" on the second line, I
do not see why that leads to the conclusion at all.

Yes, because it is a well-established convention to separate option
name with its parameter with '=', accepting "--option=parameter"
makes sense.  That may result in a string "Option: parameter" added
to the output from the command.  I am not sure why that output has
anything to do with how the command line should be specified.

Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'

[Christian Couder]

On Sat, Apr 5, 2014 at 12:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Christian Couder <christian.couder@gmail.com> writes:
>> "The following features are planned but not yet implemented:
>>         - add more tests related to commands
>>         - add examples in documentation
>>         - integration with "git commit""
>
> I was planning to merge the series to 'next', but perhaps we should
> wait at least for the first two items (I do not think the third one
> is necessary to block the series).

I will send soon a new version of the series with more tests and fixes.
It will also contains a patch that adds an empty line before the
trailers in the output if there is not already one.
After that I plan to work on adding examples to the documentation.

>>> Why support both '=' and ':'?  Using just one would make it easier to
>>> grep through scripts to see who is adding signoffs.
>>
>> That was already discussed previously.
>
> I do recall it was discussed previously, but given that a new reader
> posed the same question, I am not sure if the end result in this
> patch under discussion sufficiently answers the question in a
> satisfactory way.
>
>> The reason is that people are used to "token=value" for command line
>> arguments, but trailers appears in the result as "token: value", so it
>> is better for the user if we support both.
>
> While I do understand the part before ", so" on the second line, I
> do not see why that leads to the conclusion at all.
>
> Yes, because it is a well-established convention to separate option
> name with its parameter with '=', accepting "--option=parameter"
> makes sense.  That may result in a string "Option: parameter" added
> to the output from the command.  I am not sure why that output has
> anything to do with how the command line should be specified.

First accepting both ':' and '=' means one can see the "git
interpret-trailers" as acting on trailers only. Not just on trailers
from the intput message and option parameters from the command line.
But from trailers both from the input message and being passed as
arguments.
In my opinion it is good if it can be seen this way, as it may
simplifies the user's mental model of how the command works.

And second there is also a practical advantage, as the user can
copy-paste trailers directly from other messages into the command line
to pass them as arguments to "git interpret-trailers" without the need
to replace the ':' with '='. Even if this command is not often used
directly by users, it might simplify scripts using it.

Third there is a technical advantage which is that the code that
parses arguments from the command line can be the same as the code
that parses trailers from the input message.

Thanks,
Christian.

Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Sat, Apr 5, 2014 at 12:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Christian Couder <christian.couder@gmail.com> writes:
>>> "The following features are planned but not yet implemented:
>>>         - add more tests related to commands
>>>         - add examples in documentation
>>>         - integration with "git commit""
>>
>> I was planning to merge the series to 'next', but perhaps we should
>> wait at least for the first two items (I do not think the third one
>> is necessary to block the series).
>
> I will send soon a new version of the series with more tests and fixes.
> It will also contains a patch that adds an empty line before the
> trailers in the output if there is not already one.

Ah, yes, that one was mentioned in the reviews, I remember.

> After that I plan to work on adding examples to the documentation.

OK, thanks.

> First accepting both ':' and '=' means one can see the "git
> interpret-trailers" as acting on trailers only. Not just on trailers
> from the intput message and option parameters from the command line.

Sorry, you lost me.  What does "acting on trailers only" really
mean?  Do you mean the command should/can be run without any command
line options, pick up the existing "Signed-off-by:" and friends in
its input and emit its output, somehow taking these existing ones as
its instruction regarding how to transform the input to its output?

> And second there is also a practical advantage, as the user can
> copy-paste trailers directly from other messages into the command line
> to pass them as arguments to "git interpret-trailers" without the need
> to replace the ':' with '='. Even if this command is not often used
> directly by users, it might simplify scripts using it.
>
> Third there is a technical advantage which is that the code that
> parses arguments from the command line can be the same as the code
> that parses trailers from the input message.

I do not see these two as valid arguments to make the command line
more complex to the end users---who now need to know that only this
command treats its command line in a funny way, accepting a colon in
place of an equal sign.

A different way to sell a colon, e.g.

    Consider the instruction sed takes on its command line.
    (e.g. "sed 's/frotz/nitfol/' <xyzzy").  In the most general
    form, you would always give it as the value of an '-e' option
    (e.g. "sed -e 's/frotz/nitfol' <xyzzy"), but you are allowed to
    be loose in limited occassions.  "Key:value" is like that, and
    in the most general form, it actually needs to be spelled as
    "-e 'Key:value'".

is possible, but I do not think it is a particularly good analogy,
because what you have as the alternative is "Key=value", and not
"-e 'Key:value'", or "--Key=value" (the last would probably be the 
most natural way to express this).

Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'

[Christian Couder]

From: Junio C Hamano <gitster@pobox.com>
>
> Christian Couder <christian.couder@gmail.com> writes:
> 
>> First accepting both ':' and '=' means one can see the "git
>> interpret-trailers" as acting on trailers only. Not just on trailers
>> from the intput message and option parameters from the command line.
> 
> Sorry, you lost me.  What does "acting on trailers only" really
> mean?

It means that the command can seen as processing only trailers, (from
stdin and from its arguments), sorry if I used the wrong verb.

> Do you mean the command should/can be run without any command
> line options, pick up the existing "Signed-off-by:" and friends in
> its input and emit its output, somehow taking these existing ones as
> its instruction regarding how to transform the input to its output?
>
>> And second there is also a practical advantage, as the user can
>> copy-paste trailers directly from other messages into the command line
>> to pass them as arguments to "git interpret-trailers" without the need
>> to replace the ':' with '='. Even if this command is not often used
>> directly by users, it might simplify scripts using it.
>>
>> Third there is a technical advantage which is that the code that
>> parses arguments from the command line can be the same as the code
>> that parses trailers from the input message.
> 
> I do not see these two as valid arguments to make the command line
> more complex to the end users

I don't think that it makes the command more complex to the end users.

> ---who now need to know that only this
> command treats its command line in a funny way, accepting a colon in
> place of an equal sign.

It accepts both. So if they think that it is like a regular command,
which uses '=' for (key, value) pairs, it will work, and if they think
it works on trailers, which use ':' for (key, value) pairs, it will
also work.

> A different way to sell a colon, e.g.
> 
>     Consider the instruction sed takes on its command line.
>     (e.g. "sed 's/frotz/nitfol/' <xyzzy").  In the most general
>     form, you would always give it as the value of an '-e' option
>     (e.g. "sed -e 's/frotz/nitfol' <xyzzy"), but you are allowed to
>     be loose in limited occassions.  "Key:value" is like that, and
>     in the most general form, it actually needs to be spelled as
>     "-e 'Key:value'".
> 
> is possible, but I do not think it is a particularly good analogy,
> because what you have as the alternative is "Key=value", and not
> "-e 'Key:value'", or "--Key=value" (the last would probably be the 
> most natural way to express this).

The analogy that I would use is rather that Perl lets people use
's:foo:bar:' as well as 's=foo=bar=' instead of 's/foo/bar/' if they
prefer.

Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

>> I do not see these two as valid arguments to make the command line
>> more complex to the end users
>
> I don't think that it makes the command more complex to the end users.
>
>> ---who now need to know that only this
>> command treats its command line in a funny way, accepting a colon in
>> place of an equal sign.

I meant that it makes learning the "command line syntax of Git" more
complex to new users.  If one is too narrowly focused on this single
command, it may not seem that "the command" is not made more
complex, but I am more interested in making sure that the entire Git
command line experience does not get unnecessarily harder to learn.

Re: [PATCH v9 11/11] Documentation: add documentation for 'git interpret-trailers'

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> From: Junio C Hamano <gitster@pobox.com>
>>
>> A different way to sell a colon, e.g.
>> 
>>     Consider the instruction sed takes on its command line.
>>     (e.g. "sed 's/frotz/nitfol/' <xyzzy").  In the most general
>>     form, you would always give it as the value of an '-e' option
>>     (e.g. "sed -e 's/frotz/nitfol' <xyzzy"), but you are allowed to
>>     be loose in limited occassions.  "Key:value" is like that, and
>>     in the most general form, it actually needs to be spelled as
>>     "-e 'Key:value'".
>> 
>> is possible, but I do not think it is a particularly good analogy,
>> because what you have as the alternative is "Key=value", and not
>> "-e 'Key:value'", or "--Key=value" (the last would probably be the 
>> most natural way to express this).
>
> The analogy that I would use is rather that Perl lets people use
> 's:foo:bar:' as well as 's=foo=bar=' instead of 's/foo/bar/' if they
> prefer.

I could *almost* buy that, but that does not hold as you are not
allowing (and I do not think you need to in this case) the user to
pick any termination character like "s|foo|bar|".  The only thing
you are doing is forbidding both ":" and "=" from the set of allowed
characters for labels.

However.

I think we could buy the syntax if the "Key:value" form were the
*only* form, *without* accepting "Key=value".

The latter is a poor attempt to pretend as if it is a normal command
line option, but because that form does not even take double-dashes
at the beginning, it even fails to mimic as a command line option.

It would be one way to reduce the unnecessary cognitive load from
the users when learning the Git command line argument convention to
reject the "key=value" form and only stick to "key: value" form.
After all, because of the shape of the footer we add to the log
message (i.e. a keyword label followed by a colon followed by a SP
followed by the value), it is clear that we can use ":" as the
separator without inconveniencing the users who want to use some
unusual characters in the label part, but there is no strong reason
to reject an equal sign in the label.