[PATCH v3 00/17] Add interpret-trailers builtin

[PATCH v3 00/17] 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 implement features for these trailers first in a
new command rather than in builtin/commit.c, because this way the
prepare-commit-msg and commit-msg hooks can reuse this command.

2) Current state:

Currently the usage string of this command is:

git interpret-trailers [--trim-empty] [--infile=file] [<token[=value]>...]

The following features are implemented:

        - the result is printed on stdout
        - the [<token[=value]>...] arguments are interpreted
        - a commit message passed using the "--infile=file" option is interpreted
        - if "--infile" is not used, a commit message is read from stdin
        - 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
        - ditto for GIT_{AUTHOR,COMMITTER}_{NAME,EMAIL} env variables
        - there are some tests
        - (new) 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 2:

* the declared after statement are fixed
* the style issues in for loops are fixed
* there is some documentation


Christian Couder (17):
  Add data structures and basic functions for commit trailers
  trailer: process trailers from file and arguments
  trailer: read and process config information
  trailer: process command line trailer arguments
  strbuf: add strbuf_isspace()
  trailer: parse trailers from input file
  trailer: put all the processing together and print
  trailer: add interpret-trailers command
  trailer: add tests for "git interpret-trailers"
  trailer: if no input file is passed, read from stdin
  trailer: add new_trailer_item() function
  strbuf: add strbuf_replace()
  trailer: execute command from 'trailer.<name>.command'
  trailer: add tests for trailer command
  trailer: set author and committer env variables
  trailer: add tests for commands using env variables
  Documentation: add documentation for 'git interpret-trailers'

 .gitignore                               |   1 +
 Documentation/git-interpret-trailers.txt | 137 +++++++
 Makefile                                 |   2 +
 builtin.h                                |   1 +
 builtin/interpret-trailers.c             |  36 ++
 git.c                                    |   1 +
 strbuf.c                                 |  14 +
 strbuf.h                                 |   4 +
 t/t7513-interpret-trailers.sh            | 262 +++++++++++++
 trailer.c                                | 638 +++++++++++++++++++++++++++++++
 trailer.h                                |   6 +
 11 files changed, 1102 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.8.5.2.201.gacc5987

[PATCH v3 01/17] Add data structures and basic functions for commit trailers

[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 | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)
 create mode 100644 trailer.c

diff --git a/Makefile b/Makefile
index b4af1e2..ec90feb 100644
--- a/Makefile
+++ b/Makefile
@@ -871,6 +871,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..d80d047
--- /dev/null
+++ b/trailer.c
@@ -0,0 +1,47 @@
+#include "cache.h"
+/*
+ * Copyright (c) 2013 Christian Couder <chriscool@tuxfamily.org>
+ */
+
+enum action_where { WHERE_AFTER, WHERE_BEFORE };
+enum action_if_exist { EXIST_ADD_IF_DIFFERENT, EXIST_ADD_IF_DIFFERENT_NEIGHBOR,
+		       EXIST_ADD, EXIST_OVERWRITE, EXIST_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_exist if_exist;
+	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]));
+	return len + 1;
+}
-- 
1.8.5.2.201.gacc5987

[PATCH v3 02/17] trailer: process trailers from file and arguments

[Christian Couder]

This patch implements the logic that process trailers
from file and arguments.

At the beginning trailers from file are in their own
infile_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 infile_tok list.

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

diff --git a/trailer.c b/trailer.c
index d80d047..d88de3f 100644
--- a/trailer.c
+++ b/trailer.c
@@ -45,3 +45,192 @@ static size_t alnum_len(const char *buf, size_t len) {
 	while (--len >= 0 && !isalnum(buf[len]));
 	return len + 1;
 }
+
+static void add_arg_to_infile(struct trailer_item *infile_tok,
+			      struct trailer_item *arg_tok)
+{
+	if (arg_tok->conf->where == WHERE_AFTER) {
+		arg_tok->next = infile_tok->next;
+		infile_tok->next = arg_tok;
+		arg_tok->previous = infile_tok;
+		if (arg_tok->next)
+			arg_tok->next->previous = arg_tok;
+	} else {
+		arg_tok->previous = infile_tok->previous;
+		infile_tok->previous = arg_tok;
+		arg_tok->next = infile_tok;
+		if (arg_tok->previous)
+			arg_tok->previous->next = arg_tok;
+	}
+}
+
+static int check_if_different(struct trailer_item *infile_tok,
+			      struct trailer_item *arg_tok,
+			      int alnum_len, int check_all)
+{
+	enum action_where where = arg_tok->conf->where;
+	do {
+		if (!infile_tok)
+			return 1;
+		if (same_trailer(infile_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
+		 */
+		infile_tok = (where == WHERE_AFTER) ? infile_tok->previous : infile_tok->next;
+	} while (check_all);
+	return 1;
+}
+
+static void apply_arg_if_exist(struct trailer_item *infile_tok,
+			       struct trailer_item *arg_tok,
+			       int alnum_len)
+{
+	switch(arg_tok->conf->if_exist) {
+	case EXIST_DO_NOTHING:
+		free(arg_tok);
+		break;
+	case EXIST_OVERWRITE:
+		free((char *)infile_tok->value);
+		infile_tok->value = xstrdup(arg_tok->value);
+		free(arg_tok);
+		break;
+	case EXIST_ADD:
+		add_arg_to_infile(infile_tok, arg_tok);
+		break;
+	case EXIST_ADD_IF_DIFFERENT:
+		if (check_if_different(infile_tok, arg_tok, alnum_len, 1))
+			add_arg_to_infile(infile_tok, arg_tok);
+		else
+			free(arg_tok);
+		break;
+	case EXIST_ADD_IF_DIFFERENT_NEIGHBOR:
+		if (check_if_different(infile_tok, arg_tok, alnum_len, 0))
+			add_arg_to_infile(infile_tok, arg_tok);
+		else
+			free(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_infile_tok(struct trailer_item *infile_tok,
+			       struct trailer_item **arg_tok_first,
+			       enum action_where where)
+{
+	struct trailer_item *arg_tok;
+	struct trailer_item *next_arg;
+
+	int tok_alnum_len = alnum_len(infile_tok->token, strlen(infile_tok->token));
+	for (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {
+		next_arg = arg_tok->next;
+		if (same_token(infile_tok, arg_tok, tok_alnum_len) &&
+		    arg_tok->conf->where == where) {
+			/* Remove arg_tok from list */
+			remove_from_list(arg_tok, arg_tok_first);
+			/* Apply arg */
+			apply_arg_if_exist(infile_tok, arg_tok, tok_alnum_len);
+			/*
+			 * If arg has been added to infile,
+			 * then we need to process it too now.
+			 */
+			if ((where == WHERE_AFTER ? infile_tok->next : infile_tok->previous) == arg_tok)
+				infile_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 **infile_tok_first,
+				 struct trailer_item **infile_tok_last,
+				 struct trailer_item *arg_tok)
+{
+	struct trailer_item **infile_tok;
+	enum action_where where;
+
+	switch(arg_tok->conf->if_missing) {
+	case MISSING_DO_NOTHING:
+		free(arg_tok);
+		break;
+	case MISSING_ADD:
+		where = arg_tok->conf->where;
+		infile_tok = (where == WHERE_AFTER) ? infile_tok_last : infile_tok_first;
+		if (*infile_tok) {
+			add_arg_to_infile(*infile_tok, arg_tok);
+			*infile_tok = arg_tok;
+		} else {
+			*infile_tok_first = arg_tok;
+			*infile_tok_last = arg_tok;
+		}
+		break;
+	}
+}
+
+static void process_trailers_lists(struct trailer_item **infile_tok_first,
+				   struct trailer_item **infile_tok_last,
+				   struct trailer_item **arg_tok_first)
+{
+	struct trailer_item *infile_tok;
+	struct trailer_item *arg_tok;
+
+	if (!*arg_tok_first)
+		return;
+
+	/* Process infile from end to start */
+	for (infile_tok = *infile_tok_last; infile_tok; infile_tok = infile_tok->previous) {
+		process_infile_tok(infile_tok, arg_tok_first, WHERE_AFTER);
+	}
+
+	update_last(infile_tok_last);
+
+	if (!*arg_tok_first)
+		return;
+
+	/* Process infile from start to end */
+	for (infile_tok = *infile_tok_first; infile_tok; infile_tok = infile_tok->next) {
+		process_infile_tok(infile_tok, arg_tok_first, WHERE_BEFORE);
+	}
+
+	update_first(infile_tok_first);
+
+	/* Process args left */
+	while (*arg_tok_first) {
+		arg_tok = remove_first(arg_tok_first);
+		apply_arg_if_missing(infile_tok_first, infile_tok_last, arg_tok);
+	}
+}
-- 
1.8.5.2.201.gacc5987

[PATCH v3 03/17] trailer: read and process config information

[Christian Couder]

This patch implements reading the configuration
to get trailer information, and then processing
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 | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 129 insertions(+)

diff --git a/trailer.c b/trailer.c
index d88de3f..e7d8244 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);
@@ -234,3 +236,130 @@ static void process_trailers_lists(struct trailer_item **infile_tok_first,
 		apply_arg_if_missing(infile_tok_first, infile_tok_last, arg_tok);
 	}
 }
+
+static int set_where(struct conf_info *item, const char *value)
+{
+	if (!strcasecmp("after", value)) {
+		item->where = WHERE_AFTER;
+	} else if (!strcasecmp("before", value)) {
+		item->where = WHERE_BEFORE;
+	} else
+		return 1;
+	return 0;
+}
+
+static int set_if_exist(struct conf_info *item, const char *value)
+{
+	if (!strcasecmp("addIfDifferent", value)) {
+		item->if_exist = EXIST_ADD_IF_DIFFERENT;
+	} else if (!strcasecmp("addIfDifferentNeighbor", value)) {
+		item->if_exist = EXIST_ADD_IF_DIFFERENT_NEIGHBOR;
+	} else if (!strcasecmp("add", value)) {
+		item->if_exist = EXIST_ADD;
+	} else if (!strcasecmp("overwrite", value)) {
+		item->if_exist = EXIST_OVERWRITE;
+	} else if (!strcasecmp("doNothing", value)) {
+		item->if_exist = EXIST_DO_NOTHING;
+	} else
+		return 1;
+	return 0;
+}
+
+static int set_if_missing(struct conf_info *item, const char *value)
+{
+	if (!strcasecmp("doNothing", value)) {
+		item->if_missing = MISSING_DO_NOTHING;
+	} else if (!strcasecmp("add", value)) {
+		item->if_missing = MISSING_ADD;
+	} else
+		return 1;
+	return 0;
+}
+
+enum trailer_info_type { TRAILER_VALUE, TRAILER_COMMAND, TRAILER_WHERE,
+			 TRAILER_IF_EXIST, TRAILER_IF_MISSING };
+
+static int set_name_and_type(const char *conf_key, const char *suffix,
+			     enum trailer_info_type type,
+			     char **pname, enum trailer_info_type *ptype)
+{
+	int ret = ends_with(conf_key, suffix);
+	if (ret) {
+		*pname = xstrndup(conf_key, strlen(conf_key) - strlen(suffix));
+		*ptype = type;
+	}
+	return ret;
+}
+
+static struct trailer_item *get_conf_item(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 = xcalloc(sizeof(struct conf_info), 1);
+	item->conf->name = xstrdup(name);
+
+	if (!previous) {
+		first_conf_item = item;
+	} else {
+		previous->next = item;
+		item->previous = previous;
+	}
+
+	return item;
+}
+
+static int git_trailer_config(const char *conf_key, const char *value, void *cb)
+{
+	if (starts_with(conf_key, "trailer.")) {
+		const char *orig_conf_key = conf_key;
+		struct trailer_item *item;
+		struct conf_info *conf;
+		char *name;
+		enum trailer_info_type type;
+
+		conf_key += 8;
+		if (!set_name_and_type(conf_key, ".key", TRAILER_VALUE, &name, &type) &&
+		    !set_name_and_type(conf_key, ".command", TRAILER_COMMAND, &name, &type) &&
+		    !set_name_and_type(conf_key, ".where", TRAILER_WHERE, &name, &type) &&
+		    !set_name_and_type(conf_key, ".ifexist", TRAILER_IF_EXIST, &name, &type) &&
+		    !set_name_and_type(conf_key, ".ifmissing", TRAILER_IF_MISSING, &name, &type))
+			return 0;
+
+		item = get_conf_item(name);
+		conf = item->conf;
+
+		if (type == TRAILER_VALUE) {
+			if (conf->key)
+				warning(_("more than one %s"), orig_conf_key);
+			conf->key = xstrdup(value);
+		} else if (type == TRAILER_COMMAND) {
+			if (conf->command)
+				warning(_("more than one %s"), orig_conf_key);
+			conf->command = xstrdup(value);
+		} else if (type == TRAILER_WHERE) {
+			if (set_where(conf, value))
+				warning(_("unknown value '%s' for key '%s'"), value, orig_conf_key);
+		} else if (type == TRAILER_IF_EXIST) {
+			if (set_if_exist(conf, value))
+				warning(_("unknown value '%s' for key '%s'"), value, orig_conf_key);
+		} else if (type == TRAILER_IF_MISSING) {
+			if (set_if_missing(conf, value))
+				warning(_("unknown value '%s' for key '%s'"), value, orig_conf_key);
+		} else {
+			die("internal bug in trailer.c");
+		}
+	}
+	return 0;
+}
-- 
1.8.5.2.201.gacc5987

[PATCH v3 04/17] trailer: process command line trailer arguments

[Christian Couder]

This patch parses 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 | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/trailer.c b/trailer.c
index e7d8244..89377f2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -363,3 +363,80 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
 	}
 	return 0;
 }
+
+static void parse_trailer(struct strbuf *tok, struct strbuf *val, const char *trailer)
+{
+	char *end = strchr(trailer, '=');
+	if (!end)
+		end = strchr(trailer, ':');
+	if (end) {
+		strbuf_add(tok, trailer, end - trailer);
+		strbuf_trim(tok);
+		strbuf_addstr(val, end + 1);
+		strbuf_trim(val);
+	} else {
+		strbuf_addstr(tok, trailer);
+		strbuf_trim(tok);
+	}
+}
+
+static struct trailer_item *create_trailer_item(const char *string)
+{
+	struct strbuf tok = STRBUF_INIT;
+	struct strbuf val = STRBUF_INIT;
+	struct trailer_item *new;
+	struct trailer_item *item;
+	int tok_alnum_len;
+
+	parse_trailer(&tok, &val, string);
+
+	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)) {
+			new = xcalloc(sizeof(struct trailer_item), 1);
+			new->conf = item->conf;
+			new->token = xstrdup(item->conf->key);
+			new->value = strbuf_detach(&val, NULL);
+			strbuf_release(&tok);
+			return new;
+		}
+	}
+
+	new = xcalloc(sizeof(struct trailer_item), 1);
+	new->conf = xcalloc(sizeof(struct conf_info), 1);
+	new->token = strbuf_detach(&tok, NULL);
+	new->value = strbuf_detach(&val, NULL);
+
+	return new;
+}
+
+static void add_trailer_item(struct trailer_item **first,
+			     struct trailer_item **last,
+			     struct trailer_item *new)
+{
+	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.8.5.2.201.gacc5987

[PATCH v3 05/17] strbuf: add strbuf_isspace()

[Christian Couder]

This helper function checks if a strbuf
contains only space chars or not.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 strbuf.c | 7 +++++++
 strbuf.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index 83caf4a..2124bb8 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -124,6 +124,13 @@ void strbuf_ltrim(struct strbuf *sb)
 	sb->buf[sb->len] = '\0';
 }
 
+int strbuf_isspace(struct strbuf *sb)
+{
+	char *b;
+	for (b = sb->buf; *b && isspace(*b); b++);
+	return !*b;
+}
+
 struct strbuf **strbuf_split_buf(const char *str, size_t slen,
 				 int terminator, int max)
 {
diff --git a/strbuf.h b/strbuf.h
index 73e80ce..02bff3a 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -42,6 +42,7 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len) {
 extern void strbuf_trim(struct strbuf *);
 extern void strbuf_rtrim(struct strbuf *);
 extern void strbuf_ltrim(struct strbuf *);
+extern int strbuf_isspace(struct strbuf *);
 extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);
 
 /*
-- 
1.8.5.2.201.gacc5987

[PATCH v3 06/17] trailer: parse trailers from input file

[Christian Couder]

This patch reads trailers from an input file, parses
them and puts the result into a doubly linked list.

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

diff --git a/trailer.c b/trailer.c
index 89377f2..9ea08a7 100644
--- a/trailer.c
+++ b/trailer.c
@@ -440,3 +440,65 @@ static struct trailer_item *process_command_line_args(int argc, const char **arg
 
 	return arg_tok_first;
 }
+
+static struct strbuf **read_input_file(const char *infile)
+{
+	struct strbuf sb = STRBUF_INIT;
+
+	if (strbuf_read_file(&sb, infile, 0) < 0)
+		die_errno(_("could not read input file '%s'"), infile);
+
+	return strbuf_split(&sb, '\n');
+}
+
+/*
+ * 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 count, start, empty = 1;
+
+	/* Get the line count */
+	for (count = 0; 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 (strbuf_isspace(lines[start])) {
+			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_input_file(const char *infile,
+			       struct trailer_item **infile_tok_first,
+			       struct trailer_item **infile_tok_last)
+{
+	struct strbuf **lines = read_input_file(infile);
+	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(infile_tok_first, infile_tok_last, new);
+	}
+}
-- 
1.8.5.2.201.gacc5987

[PATCH v3 07/17] 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 | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/trailer.c b/trailer.c
index 9ea08a7..2678b3e 100644
--- a/trailer.c
+++ b/trailer.c
@@ -48,6 +48,26 @@ static size_t alnum_len(const char *buf, size_t len) {
 	return len + 1;
 }
 
+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_infile(struct trailer_item *infile_tok,
 			      struct trailer_item *arg_tok)
 {
@@ -502,3 +522,23 @@ static void process_input_file(const char *infile,
 		add_trailer_item(infile_tok_first, infile_tok_last, new);
 	}
 }
+
+void process_trailers(const char *infile, int trim_empty, int argc, const char **argv)
+{
+	struct trailer_item *infile_tok_first = NULL;
+	struct trailer_item *infile_tok_last = NULL;
+	struct trailer_item *arg_tok_first;
+
+	git_config(git_trailer_config, NULL);
+
+	/* Print the non trailer part of infile */
+	if (infile) {
+		process_input_file(infile, &infile_tok_first, &infile_tok_last);
+	}
+
+	arg_tok_first = process_command_line_args(argc, argv);
+
+	process_trailers_lists(&infile_tok_first, &infile_tok_last, &arg_tok_first);
+
+	print_all(infile_tok_first, trim_empty);
+}
-- 
1.8.5.2.201.gacc5987

[PATCH v3 08/17] 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 | 36 ++++++++++++++++++++++++++++++++++++
 git.c                        |  1 +
 trailer.h                    |  6 ++++++
 6 files changed, 46 insertions(+)
 create mode 100644 builtin/interpret-trailers.c
 create mode 100644 trailer.h

diff --git a/.gitignore b/.gitignore
index b5f9def..c870ada 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 ec90feb..a91465e 100644
--- a/Makefile
+++ b/Makefile
@@ -935,6 +935,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 d4afbfe..30f4c30 100644
--- a/builtin.h
+++ b/builtin.h
@@ -71,6 +71,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..f79bffa
--- /dev/null
+++ b/builtin/interpret-trailers.c
@@ -0,0 +1,36 @@
+/*
+ * Builtin "git interpret-trailers"
+ *
+ * Copyright (c) 2013 Christian Couder <chriscool@tuxfamily.org>
+ *
+ */
+
+#include "cache.h"
+#include "builtin.h"
+#include "parse-options.h"
+#include "strbuf.h"
+#include "trailer.h"
+
+static const char * const git_interpret_trailers_usage[] = {
+	N_("git interpret-trailers [--trim-empty] [--infile=file] [<token[=value]>...]"),
+	NULL
+};
+
+int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
+{
+	const char *infile = NULL;
+	int trim_empty = 0;
+
+	struct option options[] = {
+		OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty trailers")),
+		OPT_FILENAME(0, "infile", &infile, N_("use message from file")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			     git_interpret_trailers_usage, 0);
+
+	process_trailers(infile, trim_empty, argc, argv);
+
+	return 0;
+}
diff --git a/git.c b/git.c
index 3799514..1420b58 100644
--- a/git.c
+++ b/git.c
@@ -383,6 +383,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "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 },
diff --git a/trailer.h b/trailer.h
new file mode 100644
index 0000000..9db4459
--- /dev/null
+++ b/trailer.h
@@ -0,0 +1,6 @@
+#ifndef TRAILER_H
+#define TRAILER_H
+
+void process_trailers(const char *infile, int trim_empty, int argc, const char **argv);
+
+#endif /* TRAILER_H */
-- 
1.8.5.2.201.gacc5987

[PATCH v3 09/17] trailer: add tests for "git interpret-trailers"

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t7513-interpret-trailers.sh | 208 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 208 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..8be333c
--- /dev/null
+++ b/t/t7513-interpret-trailers.sh
@@ -0,0 +1,208 @@
+#!/bin/sh
+#
+# Copyright (c) 2013 Christian Couder
+#
+
+test_description='git interpret-trailers'
+
+. ./test-lib.sh
+
+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
+
+# 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.
+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' '
+	printf "ack: Peff\nReviewed-by: \nAcked-by: Johan\n" >expected &&
+	git interpret-trailers "ack = Peff" "Reviewed-by" "Acked-by: Johan" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--trim-empty without config' '
+	printf "ack: Peff\nAcked-by: Johan\n" >expected &&
+	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: " &&
+	printf "Acked-by: Peff\n" >expected &&
+	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= " &&
+	printf "Acked-by= Peff\n" >expected &&
+	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 #" &&
+	printf "Bug #42\n" >expected &&
+	git interpret-trailers --trim-empty "bug = 42" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'with commit basic message' '
+	git interpret-trailers --infile 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 &&
+	printf "Fixes: \nAcked-by= \nReviewed-by: \nSigned-off-by: \n" >>expected &&
+	git interpret-trailers --infile complex_message >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'with commit complex message and args' '
+	cat complex_message_body >expected &&
+	printf "Fixes: \nAcked-by= \nAcked-by= Peff\nReviewed-by: \nSigned-off-by: \nBug #42\n" >>expected &&
+	git interpret-trailers --infile complex_message "ack: Peff" "bug: 42" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'with commit complex message, args and --trim-empty' '
+	cat complex_message_body >expected &&
+	printf "Acked-by= Peff\nBug #42\n" >>expected &&
+	git interpret-trailers --trim-empty --infile complex_message "ack: Peff" "bug: 42" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "where = before"' '
+	git config trailer.bug.where "before" &&
+	cat complex_message_body >expected &&
+	printf "Bug #42\nFixes: \nAcked-by= \nAcked-by= Peff\nReviewed-by: \nSigned-off-by: \n" >>expected &&
+	git interpret-trailers --infile complex_message "ack: Peff" "bug: 42" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "where = before" for a token in the middle of infile' '
+	git config trailer.review.key "Reviewed-by:" &&
+	git config trailer.review.where "before" &&
+	cat complex_message_body >expected &&
+	printf "Bug #42\nFixes: \nAcked-by= \nAcked-by= Peff\nReviewed-by: Johan\nReviewed-by: \nSigned-off-by: \n" >>expected &&
+	git interpret-trailers --infile complex_message "ack: Peff" "bug: 42" "review: Johan" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "where = before" and --trim-empty' '
+	cat complex_message_body >expected &&
+	printf "Bug #46\nBug #42\nAcked-by= Peff\nReviewed-by: Johan\n" >>expected &&
+	git interpret-trailers --infile complex_message --trim-empty "ack: Peff" "bug: 42" "review: Johan" "Bug: 46"  >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'the default is "ifExist = addIfDifferent"' '
+	cat complex_message_body >expected &&
+	printf "Bug #42\nFixes: \nAcked-by= \nAcked-by= Peff\nReviewed-by: \nSigned-off-by: \n" >>expected &&
+	git interpret-trailers --infile complex_message "ack: Peff" "review:" "bug: 42" "ack: Peff" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifExist = addIfDifferent"' '
+	git config trailer.review.ifExist "addIfDifferent" &&
+	cat complex_message_body >expected &&
+	printf "Bug #42\nFixes: \nAcked-by= \nAcked-by= Peff\nReviewed-by: \nSigned-off-by: \n" >>expected &&
+	git interpret-trailers --infile complex_message "ack: Peff" "review:" "bug: 42" "ack: Peff" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifExist = addIfDifferentNeighbor"' '
+	git config trailer.ack.ifExist "addIfDifferentNeighbor" &&
+	cat complex_message_body >expected &&
+	printf "Bug #42\nFixes: \nAcked-by= \nAcked-by= Peff\nAcked-by= Junio\nAcked-by= Peff\nReviewed-by: \nSigned-off-by: \n" >>expected &&
+	git interpret-trailers --infile complex_message "ack: Peff" "review:" "ack: Junio" "bug: 42" "ack: Peff" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifExist = addIfDifferentNeighbor" and --trim-empty' '
+	git config trailer.ack.ifExist "addIfDifferentNeighbor" &&
+	cat complex_message_body >expected &&
+	printf "Bug #42\nAcked-by= Peff\nAcked-by= Junio\nAcked-by= Peff\n" >>expected &&
+	git interpret-trailers --infile complex_message --trim-empty "ack: Peff" "Acked-by= Peff" "review:" "ack: Junio" "bug: 42" "ack: Peff" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifExist = add"' '
+	git config trailer.ack.ifExist "add" &&
+	cat complex_message_body >expected &&
+	printf "Bug #42\nFixes: \nAcked-by= \nAcked-by= Peff\nAcked-by= Peff\nAcked-by= Junio\nAcked-by= Peff\nReviewed-by: \nSigned-off-by: \n" >>expected &&
+	git interpret-trailers --infile complex_message "ack: Peff" "Acked-by= Peff" "review:" "ack: Junio" "bug: 42" "ack: Peff" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifExist = overwrite"' '
+	git config trailer.fix.key "Fixes:" &&
+	git config trailer.fix.ifExist "overwrite" &&
+	cat complex_message_body >expected &&
+	printf "Bug #42\nFixes: 22\nAcked-by= \nAcked-by= Junio\nAcked-by= Peff\nReviewed-by: \nSigned-off-by: \n" >>expected &&
+	git interpret-trailers --infile complex_message "review:" "fix=53" "ack: Junio" "fix=22" "bug: 42" "ack: Peff" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifExist = doNothing"' '
+	git config trailer.fix.ifExist "doNothing" &&
+	cat complex_message_body >expected &&
+	printf "Bug #42\nFixes: \nAcked-by= \nAcked-by= Junio\nAcked-by= Peff\nReviewed-by: \nSigned-off-by: \n" >>expected &&
+	git interpret-trailers --infile complex_message "review:" "fix=53" "ack: Junio" "fix=22" "bug: 42" "ack: Peff" >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 &&
+	printf "Bug #42\nCc: Linus\nFixes: \nAcked-by= \nAcked-by= Junio\nAcked-by= Peff\nReviewed-by: \nSigned-off-by: \n" >>expected &&
+	git interpret-trailers --infile complex_message "review:" "fix=53" "cc=Linus" "ack: Junio" "fix=22" "bug: 42" "ack: Peff" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifMissing = add"' '
+	git config trailer.Cc.ifMissing "add" &&
+	cat complex_message_body >expected &&
+	printf "Cc: Linus\nBug #42\nFixes: \nAcked-by= \nAcked-by= Junio\nAcked-by= Peff\nReviewed-by: \nSigned-off-by: \n" >>expected &&
+	git interpret-trailers --infile complex_message "review:" "fix=53" "ack: Junio" "fix=22" "bug: 42" "cc=Linus" "ack: Peff" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'using "ifMissing = doNothing"' '
+	git config trailer.Cc.ifMissing "doNothing" &&
+	cat complex_message_body >expected &&
+	printf "Bug #42\nFixes: \nAcked-by= \nAcked-by= Junio\nAcked-by= Peff\nReviewed-by: \nSigned-off-by: \n" >>expected &&
+	git interpret-trailers --infile complex_message "review:" "fix=53" "cc=Linus" "ack: Junio" "fix=22" "bug: 42" "ack: Peff" >actual &&
+	test_cmp expected actual
+'
+
+test_done
-- 
1.8.5.2.201.gacc5987

[PATCH v3 10/17] trailer: if no input file is passed, read from stdin

[Christian Couder]

It is simpler and more natural if the "git interpret-trailers"
is made a filter as its output already goes to sdtout.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/interpret-trailers.c  |  2 +-
 t/t7513-interpret-trailers.sh |  7 +++++++
 trailer.c                     | 15 +++++++++------
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index f79bffa..37237f7 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -23,7 +23,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 
 	struct option options[] = {
 		OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty trailers")),
-		OPT_FILENAME(0, "infile", &infile, N_("use message from file")),
+		OPT_FILENAME(0, "infile", &infile, N_("use message from file, instead of stdin")),
 		OPT_END()
 	};
 
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 8be333c..f5ef81f 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -205,4 +205,11 @@ test_expect_success 'using "ifMissing = doNothing"' '
 	test_cmp expected actual
 '
 
+test_expect_success 'with input from stdin' '
+	cat complex_message_body >expected &&
+	printf "Bug #42\nFixes: \nAcked-by= \nAcked-by= Junio\nAcked-by= Peff\nReviewed-by: \nSigned-off-by: \n" >>expected &&
+	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
diff --git a/trailer.c b/trailer.c
index 2678b3e..d581371 100644
--- a/trailer.c
+++ b/trailer.c
@@ -465,8 +465,13 @@ static struct strbuf **read_input_file(const char *infile)
 {
 	struct strbuf sb = STRBUF_INIT;
 
-	if (strbuf_read_file(&sb, infile, 0) < 0)
-		die_errno(_("could not read input file '%s'"), infile);
+	if (infile) {
+		if (strbuf_read_file(&sb, infile, 0) < 0)
+			die_errno(_("could not read input file '%s'"), infile);
+	} else {
+		if (strbuf_read(&sb, fileno(stdin), 0) < 0)
+			die_errno(_("could not read from stdin"));
+	}
 
 	return strbuf_split(&sb, '\n');
 }
@@ -531,10 +536,8 @@ void process_trailers(const char *infile, int trim_empty, int argc, const char *
 
 	git_config(git_trailer_config, NULL);
 
-	/* Print the non trailer part of infile */
-	if (infile) {
-		process_input_file(infile, &infile_tok_first, &infile_tok_last);
-	}
+	/* Print the non trailer part of infile (or stdin if infile is NULL) */
+	process_input_file(infile, &infile_tok_first, &infile_tok_last);
 
 	arg_tok_first = process_command_line_args(argc, argv);
 
-- 
1.8.5.2.201.gacc5987

[PATCH v3 11/17] trailer: add new_trailer_item() function

[Christian Couder]

This is a small refactoring to prepare for the next steps.

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

diff --git a/trailer.c b/trailer.c
index d581371..6eec3ce 100644
--- a/trailer.c
+++ b/trailer.c
@@ -400,11 +400,27 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val, const char *tr
 	}
 }
 
+static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,
+					     const char* tok, const char* val)
+{
+	struct trailer_item *new = xcalloc(sizeof(struct trailer_item), 1);
+	new->value = val;
+
+	if (conf_item) {
+		new->conf = conf_item->conf;
+		new->token = xstrdup(conf_item->conf->key);
+	} else {
+		new->conf = xcalloc(sizeof(struct conf_info), 1);
+		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 *new;
 	struct trailer_item *item;
 	int tok_alnum_len;
 
@@ -416,21 +432,12 @@ static struct trailer_item *create_trailer_item(const char *string)
 	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)) {
-			new = xcalloc(sizeof(struct trailer_item), 1);
-			new->conf = item->conf;
-			new->token = xstrdup(item->conf->key);
-			new->value = strbuf_detach(&val, NULL);
 			strbuf_release(&tok);
-			return new;
+			return new_trailer_item(item, NULL, strbuf_detach(&val, NULL));
 		}
 	}
 
-	new = xcalloc(sizeof(struct trailer_item), 1);
-	new->conf = xcalloc(sizeof(struct conf_info), 1);
-	new->token = strbuf_detach(&tok, NULL);
-	new->value = strbuf_detach(&val, NULL);
-
-	return new;
+	return new_trailer_item(NULL, strbuf_detach(&tok, NULL), strbuf_detach(&val, NULL));;
 }
 
 static void add_trailer_item(struct trailer_item **first,
-- 
1.8.5.2.201.gacc5987

[PATCH v3 12/17] strbuf: add strbuf_replace()

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 strbuf.c | 7 +++++++
 strbuf.h | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index 2124bb8..e45e513 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -197,6 +197,13 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
 	strbuf_setlen(sb, sb->len + dlen - len);
 }
 
+void strbuf_replace(struct strbuf *sb, const char *a, const char *b)
+{
+	char *ptr = strstr(sb->buf, a);
+	if (ptr)
+		strbuf_splice(sb, ptr - sb->buf, strlen(a), b, strlen(b));
+}
+
 void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)
 {
 	strbuf_splice(sb, pos, 0, data, len);
diff --git a/strbuf.h b/strbuf.h
index 02bff3a..38faf70 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -111,6 +111,9 @@ extern void strbuf_remove(struct strbuf *, size_t pos, size_t len);
 extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,
                           const void *, size_t);
 
+/* first occurence of a replaced with b */
+extern void strbuf_replace(struct strbuf *, const char *a, const char *b);
+
 extern void strbuf_add_commented_lines(struct strbuf *out, const char *buf, size_t size);
 
 extern void strbuf_add(struct strbuf *, const void *, size_t);
-- 
1.8.5.2.201.gacc5987

[PATCH v3 13/17] trailer: execute command from 'trailer.<name>.command'

[Christian Couder]

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

diff --git a/trailer.c b/trailer.c
index 6eec3ce..dc81a01 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "run-command.h"
 /*
  * Copyright (c) 2013 Christian Couder <chriscool@tuxfamily.org>
  */
@@ -12,11 +13,14 @@ struct conf_info {
 	char *name;
 	char *key;
 	char *command;
+	unsigned command_uses_arg : 1;
 	enum action_where where;
 	enum action_if_exist if_exist;
 	enum action_if_missing if_missing;
 };
 
+#define TRAILER_ARG_STRING "$ARG"
+
 struct trailer_item {
 	struct trailer_item *previous;
 	struct trailer_item *next;
@@ -368,6 +372,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
 			if (conf->command)
 				warning(_("more than one %s"), orig_conf_key);
 			conf->command = xstrdup(value);
+			conf->command_uses_arg = !!strstr(conf->command, TRAILER_ARG_STRING);
 		} else if (type == TRAILER_WHERE) {
 			if (set_where(conf, value))
 				warning(_("unknown value '%s' for key '%s'"), value, orig_conf_key);
@@ -400,6 +405,45 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val, const char *tr
 	}
 }
 
+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 struct trailer_item *new_trailer_item(struct trailer_item *conf_item,
 					     const char* tok, const char* val)
 {
@@ -409,6 +453,8 @@ static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,
 	if (conf_item) {
 		new->conf = conf_item->conf;
 		new->token = xstrdup(conf_item->conf->key);
+		if (conf_item->conf->command_uses_arg || !val)
+			new->value = apply_command(conf_item->conf->command, val);
 	} else {
 		new->conf = xcalloc(sizeof(struct conf_info), 1);
 		new->token = tok;
@@ -459,12 +505,22 @@ 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.8.5.2.201.gacc5987

[PATCH v3 14/17] trailer: add tests for trailer command

[Christian Couder]

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

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index f5ef81f..2d50b7a 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -212,4 +212,31 @@ test_expect_success 'with input from stdin' '
 	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.ifExist "addIfDifferentNeighbor" &&
+	git config trailer.sign.command "echo \"A U Thor <author@example.com>\"" &&
+	cat complex_message_body >expected &&
+	printf "Fixes: \nAcked-by= \nReviewed-by: \nSigned-off-by: \nSigned-off-by: A U Thor <author@example.com>\n" >>expected &&
+	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.ifExist "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 &&
+	printf "Fixes: $FIXED\nAcked-by= \nReviewed-by: \nSigned-off-by: \nSigned-off-by: A U Thor <author@example.com>\n" >>expected &&
+	git interpret-trailers "review:" "fix=HEAD" < complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
1.8.5.2.201.gacc5987

[PATCH v3 15/17] trailer: set author and committer env variables

[Christian Couder]

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

diff --git a/trailer.c b/trailer.c
index dc81a01..6c2a2b9 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "run-command.h"
+#include "argv-array.h"
 /*
  * Copyright (c) 2013 Christian Couder <chriscool@tuxfamily.org>
  */
@@ -415,14 +416,40 @@ static int read_from_command(struct child_process *cp, struct strbuf *buf)
 	return 0;
 }
 
+static void setup_ac_env(struct argv_array *env, const char *ac_name, const char *ac_mail, const char *(*read)(int))
+{
+	if (!getenv(ac_name) || !getenv(ac_mail)) {
+		struct ident_split ident;
+		const char *namebuf, *mailbuf;
+		int namelen, maillen;
+		const char *ac_info = read(IDENT_NO_DATE);
+
+		if (split_ident_line(&ident, ac_info, strlen(ac_info)))
+			return;
+
+		namelen = ident.name_end - ident.name_begin;
+		namebuf = ident.name_begin;
+
+		maillen = ident.mail_end - ident.mail_begin;
+		mailbuf = ident.mail_begin;
+
+		argv_array_pushf(env, "%s=%.*s", ac_name, namelen, namebuf);
+		argv_array_pushf(env, "%s=%.*s", ac_mail, maillen, mailbuf);
+	}
+}
+
 static const char *apply_command(const char *command, const char *arg)
 {
+	struct argv_array env = ARGV_ARRAY_INIT;
 	struct strbuf cmd = STRBUF_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	struct child_process cp;
 	const char *argv[] = {NULL, NULL};
 	const char *result = "";
 
+	setup_ac_env(&env, "GIT_AUTHOR_NAME", "GIT_AUTHOR_EMAIL", git_author_info);
+	setup_ac_env(&env, "GIT_COMMITTER_NAME", "GIT_COMMITTER_EMAIL", git_committer_info);
+
 	strbuf_addstr(&cmd, command);
 	if (arg)
 		strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
@@ -430,7 +457,7 @@ static const char *apply_command(const char *command, const char *arg)
 	argv[0] = cmd.buf;
 	memset(&cp, 0, sizeof(cp));
 	cp.argv = argv;
-	cp.env = local_repo_env;
+	cp.env = env.argv;
 	cp.no_stdin = 1;
 	cp.out = -1;
 	cp.use_shell = 1;
@@ -441,6 +468,7 @@ static const char *apply_command(const char *command, const char *arg)
 		result = strbuf_detach(&buf, NULL);
 
 	strbuf_release(&cmd);
+	argv_array_clear(&env);
 	return result;
 }
 
-- 
1.8.5.2.201.gacc5987

[PATCH v3 16/17] trailer: add tests for commands using env variables

[Christian Couder]

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

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 2d50b7a..00894a8 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -223,6 +223,26 @@ test_expect_success 'with simple command' '
 	test_cmp expected actual
 '
 
+test_expect_success 'with command using commiter information' '
+	git config trailer.sign.ifExist "addIfDifferent" &&
+	git config trailer.sign.command "echo \"\$GIT_COMMITTER_NAME <\$GIT_COMMITTER_EMAIL>\"" &&
+	cat complex_message_body >expected &&
+	printf "Fixes: \nAcked-by= \nReviewed-by: \nSigned-off-by: \nSigned-off-by: C O Mitter <committer@example.com>\n" >>expected &&
+	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.ifExist "addIfDifferentNeighbor" &&
+	git config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	cat complex_message_body >expected &&
+	printf "Fixes: \nAcked-by= \nReviewed-by: \nSigned-off-by: \nSigned-off-by: A U Thor <author@example.com>\n" >>expected &&
+	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 &&
-- 
1.8.5.2.201.gacc5987

[PATCH v3 17/17] Documentation: add documentation for 'git interpret-trailers'

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-interpret-trailers.txt | 137 +++++++++++++++++++++++++++++++
 1 file changed, 137 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..f74843e
--- /dev/null
+++ b/Documentation/git-interpret-trailers.txt
@@ -0,0 +1,137 @@
+git-interpret-trailers(1)
+=========================
+
+NAME
+----
+git-interpret-trailers - help add stuctured information into commit messages
+
+SYNOPSIS
+--------
+[verse]
+'git interpret-trailers' [--trim-empty] [--infile=file] [<token[=value]>...]
+
+DESCRIPTION
+-----------
+Help add RFC 822 like headers, called 'trailers', at the end of the
+otherwise free-form part of a commit message.
+
+Unless `--infile=file` is used, this command is a filter. It reads the
+standard input for a commit message and apply 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 white spaces, 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
+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.
+
+Trailers have become a de facto standard way to add helpful structured
+information into commit messages. For example the well known
+"Signed-off-by: " trailer is used by many projects like the Linux
+kernel and Git.
+
+OPTIONS
+-------
+--trim-empty::
+	If the 'value' part of any trailer contains onlywhite spaces,
+	the whole trailer will be removed from the resulting message.
+
+----infile=file::
+	Read the commit message from `file` instead of the standard
+	input.
+
+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 chose 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 chose 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.
++
+The following environment variables are set when the command is run:
+GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, GIT_COMMITTER_NAME,
+GIT_COMMITTER_EMAIL.
+
+SEE ALSO
+--------
+linkgit:git-commit[1]
+
+GIT
+---
+Part of the linkgit:git[1] suite
-- 
1.8.5.2.201.gacc5987

Re: [PATCH v3 17/17] Documentation: add documentation for 'git interpret-trailers'

[Eric Sunshine]

On Sun, Jan 26, 2014 at 12:00 PM, Christian Couder
<chriscool@tuxfamily.org> wrote:
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
> new file mode 100644
> index 0000000..f74843e
> --- /dev/null
> +++ b/Documentation/git-interpret-trailers.txt
> @@ -0,0 +1,137 @@
> +git-interpret-trailers(1)
> +=========================
> +
> +NAME
> +----
> +git-interpret-trailers - help add stuctured information into commit messages
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'git interpret-trailers' [--trim-empty] [--infile=file] [<token[=value]>...]

Would it be more consistent with existing documentation to format this as so?

  [--infile=<file>] [<token>[=<value>]]...

> +DESCRIPTION
> +-----------
> +Help add RFC 822 like headers, called 'trailers', at the end of the

s/822 like/822-like/

Was the suggestion, early in the discussion, to use "footer" rather
than "trailer" dismissed?

> +otherwise free-form part of a commit message.
> +
> +Unless `--infile=file` is used, this command is a filter. It reads the
> +standard input for a commit message and apply the `token` arguments,

s/apply/applies/

> +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 white spaces, and the resulting trimmed 'token'

Other git documentation uniformly spells it as "whitespace" rather
than "white spaces".

> +and 'value' will appear in the message like this:
> +
> +------------------------------------------------
> +token: value
> +------------------------------------------------
> +
> +By default, if there are already trailers with the same 'token' the
> +trailer will appear just after the last trailer with the same

It might be a bit clearer to say "the _new_ trailer will appear...".

> +'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.
> +
> +Trailers have become a de facto standard way to add helpful structured
> +information into commit messages. For example the well known
> +"Signed-off-by: " trailer is used by many projects like the Linux
> +kernel and Git.

This "justification" paragraph might make more sense near or at the
very the top of the Description section.

> +OPTIONS
> +-------
> +--trim-empty::
> +       If the 'value' part of any trailer contains onlywhite spaces,

s/onlywhite spaces/only whitespace/

> +       the whole trailer will be removed from the resulting message.
> +
> +----infile=file::
> +       Read the commit message from `file` instead of the standard
> +       input.
> +
> +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 chose what action will be

s/chose/choose/

> +       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 chose what action will be

s/chose/choose/

> +       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.

What happens if the text returned by the command has multiple lines?
Should the documentation say something about this?

> +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.
> ++
> +The following environment variables are set when the command is run:
> +GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, GIT_COMMITTER_NAME,
> +GIT_COMMITTER_EMAIL.
> +
> +SEE ALSO
> +--------
> +linkgit:git-commit[1]
> +
> +GIT
> +---
> +Part of the linkgit:git[1] suite
> --
> 1.8.5.2.201.gacc5987

Re: [PATCH v3 17/17] Documentation: add documentation for 'git interpret-trailers'

[Christian Couder]

From: Eric Sunshine <sunshine@sunshineco.com>
>
> On Sun, Jan 26, 2014 at 12:00 PM, Christian Couder
> <chriscool@tuxfamily.org> wrote:
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>> diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
>> new file mode 100644
>> index 0000000..f74843e
>> --- /dev/null
>> +++ b/Documentation/git-interpret-trailers.txt
>> @@ -0,0 +1,137 @@
>> +git-interpret-trailers(1)
>> +=========================
>> +
>> +NAME
>> +----
>> +git-interpret-trailers - help add stuctured information into commit messages
>> +
>> +SYNOPSIS
>> +--------
>> +[verse]
>> +'git interpret-trailers' [--trim-empty] [--infile=file] [<token[=value]>...]
> 
> Would it be more consistent with existing documentation to format this as so?
> 
>   [--infile=<file>] [<token>[=<value>]]...

No, it would be very inconsistent:

$ grep '\.\.\.\]' *.txt | wc -l
103
$ grep '\]\.\.\.' *.txt | wc -l
0

>> +DESCRIPTION
>> +-----------
>> +Help add RFC 822 like headers, called 'trailers', at the end of the
> 
> s/822 like/822-like/

Ok.

> Was the suggestion, early in the discussion, to use "footer" rather
> than "trailer" dismissed?

During the early discussions people initially talked about "footer"
while Junio and others talked about "trailer".

See:

http://thread.gmane.org/gmane.comp.version-control.git/236429/
http://thread.gmane.org/gmane.comp.version-control.git/236770/

I have no preference for one or the other, but by default I use what
Junio uses.

>> +otherwise free-form part of a commit message.
>> +
>> +Unless `--infile=file` is used, this command is a filter. It reads the
>> +standard input for a commit message and apply the `token` arguments,
> 
> s/apply/applies/

Ok.

>> +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 white spaces, and the resulting trimmed 'token'
> 
> Other git documentation uniformly spells it as "whitespace" rather
> than "white spaces".

You are right I will change that.

>> +and 'value' will appear in the message like this:
>> +
>> +------------------------------------------------
>> +token: value
>> +------------------------------------------------
>> +
>> +By default, if there are already trailers with the same 'token' the
>> +trailer will appear just after the last trailer with the same
> 
> It might be a bit clearer to say "the _new_ trailer will appear...".

Ok.

>> +'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.
>> +
>> +Trailers have become a de facto standard way to add helpful structured
>> +information into commit messages. For example the well known
>> +"Signed-off-by: " trailer is used by many projects like the Linux
>> +kernel and Git.
> 
> This "justification" paragraph might make more sense near or at the
> very the top of the Description section.

Yeah, or maybe I can remove it entirely.

>> +OPTIONS
>> +-------
>> +--trim-empty::
>> +       If the 'value' part of any trailer contains onlywhite spaces,
> 
> s/onlywhite spaces/only whitespace/

Ok.

>> +       the whole trailer will be removed from the resulting message.
>> +
>> +----infile=file::
>> +       Read the commit message from `file` instead of the standard
>> +       input.
>> +
>> +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 chose what action will be
> 
> s/chose/choose/

Ok.

>> +       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 chose what action will be
> 
> s/chose/choose/

Ok.

>> +       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.
> 
> What happens if the text returned by the command has multiple lines?

The 'value' part of trailer will be on multiple lines.

> Should the documentation say something about this?

I think it is better left as unspecified because we might want to
print a warning, or perhaps error out in this case.

Thanks,
Christian.

Re: [PATCH v3 00/17] Add interpret-trailers builtin

[Junio C Hamano]

Is this from the same Christian?

The series seems to have unusually high rate of style violations
compared to the usual submission, like these:

ERROR: open brace '{' following function declarations go on the next line
#78: FILE: trailer.c:44:
+static size_t alnum_len(const char *buf, size_t len) {

ERROR: trailing statements should be on next line
#79: FILE: trailer.c:45:
+	while (--len >= 0 && !isalnum(buf[len]));

ERROR: space required before the open parenthesis '('
#70: FILE: trailer.c:90:
+	switch(arg_tok->conf->if_exist) {

WARNING: braces {} are not necessary for any arm of this statement
#66: FILE: trailer.c:270:
+	if (!strcasecmp("doNothing", value)) {
[...]
+	} else if (!strcasecmp("add", value)) {
[...]
+	} else
[...]

ERROR: that open brace { should be on the previous line
#96: FILE: trailer.c:300:
+	for (previous = NULL, item = first_conf_item;
+	     item;
+	     previous = item, item = item->next)
+	{

Just trying to see if there is an impersonation ;-)

Re: [PATCH v3 17/17] Documentation: add documentation for 'git interpret-trailers'

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

>>> +'git interpret-trailers' [--trim-empty] [--infile=file] [<token[=value]>...]
>> 
>> Would it be more consistent with existing documentation to format this as so?
>> 
>>   [--infile=<file>] [<token>[=<value>]]...
>
> No, it would be very inconsistent:
>
> $ grep '\.\.\.\]' *.txt | wc -l
> 103
> $ grep '\]\.\.\.' *.txt | wc -l
> 0

I have a feeling that you are missing the point Eric is making.  The
value given to the --infile option can be anything, i.e. 'file'
there is a placeholder, hence "--infile=<file>" not "--infile=file"
as you wrote.

Also I think "[<token>[=<value>]]..." is the correct way to spell
that there can be 0 or more "<token>[=<value>]".  "<token[=value]>"
in the original does not make any sense, as <> is meant to say "this
thing is a placeholder", and we do not try to say, with the string
inside <>, what shape that placeholder takes.  In fact '=' part is
_not_ a placeholder but is required syntactically when you want to
supply a value to the token, so the original doubly is incorrect.

I find it a bad taste to allow unbound set of <token> on the LHS of
'=' on the command line, but that is a separate issue in the design,
not in the documentation of the design.

Thanks.

Re: [PATCH v3 00/17] Add interpret-trailers builtin

[Christian Couder]

From: Junio C Hamano <gitster@pobox.com>
>
> Is this from the same Christian?

Yes, ...

> The series seems to have unusually high rate of style violations
> compared to the usual submission, like these:
> 
> ERROR: open brace '{' following function declarations go on the next line
> #78: FILE: trailer.c:44:
> +static size_t alnum_len(const char *buf, size_t len) {
> 
> ERROR: trailing statements should be on next line
> #79: FILE: trailer.c:45:
> +	while (--len >= 0 && !isalnum(buf[len]));
> 
> ERROR: space required before the open parenthesis '('
> #70: FILE: trailer.c:90:
> +	switch(arg_tok->conf->if_exist) {
> 
> WARNING: braces {} are not necessary for any arm of this statement
> #66: FILE: trailer.c:270:
> +	if (!strcasecmp("doNothing", value)) {
> [...]
> +	} else if (!strcasecmp("add", value)) {
> [...]
> +	} else
> [...]
> 
> ERROR: that open brace { should be on the previous line
> #96: FILE: trailer.c:300:
> +	for (previous = NULL, item = first_conf_item;
> +	     item;
> +	     previous = item, item = item->next)
> +	{
> 
> Just trying to see if there is an impersonation ;-)

... I guess the artistic part of my mind is trying to come out somehow.

Thanks,
Christian.

Re: [PATCH v3 17/17] Documentation: add documentation for 'git interpret-trailers'

[Christian Couder]

From: Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v3 17/17] Documentation: add documentation for 'git interpret-trailers'
Date: Mon, 27 Jan 2014 13:20:18 -0800

> Christian Couder <chriscool@tuxfamily.org> writes:
> 
>>>> +'git interpret-trailers' [--trim-empty] [--infile=file] [<token[=value]>...]
>>> 
>>> Would it be more consistent with existing documentation to format this as so?
>>> 
>>>   [--infile=<file>] [<token>[=<value>]]...
>>
>> No, it would be very inconsistent:
>>
>> $ grep '\.\.\.\]' *.txt | wc -l
>> 103
>> $ grep '\]\.\.\.' *.txt | wc -l
>> 0
> 
> I have a feeling that you are missing the point Eric is making.

Yeah I realize I missed some of his points. Sorry about that.

> The
> value given to the --infile option can be anything, i.e. 'file'
> there is a placeholder, hence "--infile=<file>" not "--infile=file"
> as you wrote.

Ok, I agree with that.

> Also I think "[<token>[=<value>]]..." is the correct way to spell
> that there can be 0 or more "<token>[=<value>]".

Perhaps but it is not very consistent with what there is in other
synopsis strings. For example the commands that accept a path have a
synopsis that ends with "[--] [<path>...]" and not "[--] [<path>]...".

Perhaps [(<token>[=<value>])...] is more correct, but not worth the
added complexity.

> "<token[=value]>"
> in the original does not make any sense, as <> is meant to say "this
> thing is a placeholder", and we do not try to say, with the string
> inside <>, what shape that placeholder takes.  In fact '=' part is
> _not_ a placeholder but is required syntactically when you want to
> supply a value to the token, so the original doubly is incorrect.

Yeah perhaps, except that in the code ':' is accepted instead of '=',
and the documention says that.

So perhaps [(<token>[(=|:)<value>])...] is even more correct.

> I find it a bad taste to allow unbound set of <token> on the LHS of
> '=' on the command line, but that is a separate issue in the design,
> not in the documentation of the design.

I don't understand this sentence, sorry.

Thanks,
Christian.

Re: [PATCH v3 17/17] Documentation: add documentation for 'git interpret-trailers'

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

>> I find it a bad taste to allow unbound set of <token> on the LHS of
>> '=' on the command line, but that is a separate issue in the design,
>> not in the documentation of the design.
>
> I don't understand this sentence, sorry.

It is a bad design taste to structure the command line argument in
such a way that it takes

	git cmd xyzzy=frotz nitfol=rezrov some other args

where these 'xyzzy', 'nitfol', etc. form an unbound set.  It
prevents future enhancements to the command from allowing anything
that contain '=' in "some other args" part.

Allowing not just '=' but also ':' makes it even worse.

But these are issues in the design itself.  Not the issue in the
patch 17/17 that is trying to document the design.  This patch under
discussion documents the bad design correctly ;-)

Re: [PATCH v3 17/17] Documentation: add documentation for 'git interpret-trailers'

[Eric Sunshine]

Patch 17/17 of v4 failed to arrive in my inbox for some reason, so
I'll just reply to v3 since there's another error I noticed which is
still present in v4, plus a comment specific to v4 (see below).

On Mon, Jan 27, 2014 at 3:33 PM, Christian Couder
<chriscool@tuxfamily.org> wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
>> On Sun, Jan 26, 2014 at 12:00 PM, Christian Couder
>> <chriscool@tuxfamily.org> wrote:
>>> +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 white spaces, and the resulting trimmed 'token'
>>
>> Other git documentation uniformly spells it as "whitespace" rather
>> than "white spaces".
>
> You are right I will change that.

The rest of git documentation consistently spells it "whitespace", but
v4 uses "whitespaces".

>>> +----infile=file::
>>> +       Read the commit message from `file` instead of the standard
>>> +       input.

I didn't notice this when reviewing v3, and the same text appears in
v4. There are a couple extra hyphens before 'infile', and you want <
and > around "file", so:

s/----infile=file/--infile=<file>/