[PATCH 0/5] make interpret-trailers useful for parsing

[PATCH 0/5] make interpret-trailers useful for parsing

[Jeff King]

Parsing trailers out of a commit message is _mostly_ easy, but there
area a lot of funny corner cases (e.g., heuristics for how many
non-trailers must be present before a final paragraph isn't a trailer
block anymore).  The code in trailer.c already knows about these corner
cases, but there's no way to access it from the command line.

This series teaches interpret-trailers to parse and output just the
trailers. So now you can do:

  $ git log --format=%B -1 8d44797cc91231cd44955279040dc4a1ee0a797f |
    git interpret-trailers --parse
  Signed-off-by: Hartmut Henkel <henkel@vh-s.de>
  Helped-by: Stefan Beller <sbeller@google.com>
  Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
  Acked-by: Matthias Rüster <matthias.ruester@gmail.com>

I considered a few different approaches before deciding on
what's here:

  1. The output format is actually the normal "key: value" trailers. I
     considered something more structured like JSON. But the "key:
     value" format is quite easy to parse, once it has been normalized
     (finding the trailers, unfolding whitespace continuation, etc).

  2. This series introduces several orthogonal options which can be used
     together to achieve my goal, when there could just be a "parse"
     mode. Since interpret-trailers is plumbing, I reasoned that the
     individual options might still be useful apart from each other (for
     instance, you could re-normalize existing trailers while writing
     your new ones from a commit hook). I did add a "--parse" for
     convenience and to help point users in the right direction.

     For the same reason (and so we could build on other orthogonal
     features like --in-place and --trim-empty), I decided against
     having a separate command like "git parse-trailers".

  [1/5]: trailer: put process_trailers() options into a struct
  [2/5]: interpret-trailers: add an option to show only the trailers
  [3/5]: interpret-trailers: add an option to show only existing trailers
  [4/5]: interpret-trailers: add an option to normalize output
  [5/5]: interpret-trailers: add --parse convenience option

 Documentation/git-interpret-trailers.txt | 17 ++++++++
 builtin/interpret-trailers.c             | 34 ++++++++++++---
 t/t7513-interpret-trailers.sh            | 73 ++++++++++++++++++++++++++++++++
 trailer.c                                | 65 ++++++++++++++++++++++------
 trailer.h                                | 12 +++++-
 5 files changed, 180 insertions(+), 21 deletions(-)

-Peff

[PATCH 1/5] trailer: put process_trailers() options into a struct

[Jeff King]

We already have two options and are about to add a few more.
To avoid having a huge number of boolean arguments, let's
convert to an options struct which can be passed in.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/interpret-trailers.c | 13 ++++++-------
 trailer.c                    |  9 +++++----
 trailer.h                    |  9 ++++++++-
 3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 175f14797b..bb0d7b937a 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -18,13 +18,12 @@ static const char * const git_interpret_trailers_usage[] = {
 
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
-	int in_place = 0;
-	int trim_empty = 0;
+	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
 	struct string_list trailers = STRING_LIST_INIT_NODUP;
 
 	struct option options[] = {
-		OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
-		OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty trailers")),
+		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
+		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
@@ -36,11 +35,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
-			process_trailers(argv[i], in_place, trim_empty, &trailers);
+			process_trailers(argv[i], &opts, &trailers);
 	} else {
-		if (in_place)
+		if (opts.in_place)
 			die(_("no input file given for in-place editing"));
-		process_trailers(NULL, in_place, trim_empty, &trailers);
+		process_trailers(NULL, &opts, &trailers);
 	}
 
 	string_list_clear(&trailers, 0);
diff --git a/trailer.c b/trailer.c
index 751b56c009..0a0c2c264d 100644
--- a/trailer.c
+++ b/trailer.c
@@ -968,7 +968,8 @@ static FILE *create_in_place_tempfile(const char *file)
 	return outfile;
 }
 
-void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)
+void process_trailers(const char *file, struct process_trailer_options *opts,
+		      struct string_list *trailers)
 {
 	LIST_HEAD(head);
 	LIST_HEAD(arg_head);
@@ -980,7 +981,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 
 	read_input_file(&sb, file);
 
-	if (in_place)
+	if (opts->in_place)
 		outfile = create_in_place_tempfile(file);
 
 	/* Print the lines before the trailers */
@@ -990,14 +991,14 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 
 	process_trailers_lists(&head, &arg_head);
 
-	print_all(outfile, &head, trim_empty);
+	print_all(outfile, &head, opts->trim_empty);
 
 	free_all(&head);
 
 	/* Print the lines after the trailers as is */
 	fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
 
-	if (in_place)
+	if (opts->in_place)
 		if (rename_tempfile(&trailers_tempfile, file))
 			die_errno(_("could not rename temporary file to %s"), file);
 
diff --git a/trailer.h b/trailer.h
index 65cc5d79c6..b33c5aa57d 100644
--- a/trailer.h
+++ b/trailer.h
@@ -22,7 +22,14 @@ struct trailer_info {
 	size_t trailer_nr;
 };
 
-void process_trailers(const char *file, int in_place, int trim_empty,
+struct process_trailer_options {
+	int in_place;
+	int trim_empty;
+};
+
+#define PROCESS_TRAILER_OPTIONS_INIT {0}
+
+void process_trailers(const char *file, struct process_trailer_options *opts,
 		      struct string_list *trailers);
 
 void trailer_info_get(struct trailer_info *info, const char *str);
-- 
2.14.0.609.gd2d1f7ddf

[PATCH 2/5] interpret-trailers: add an option to show only the trailers

[Jeff King]

In theory it's easy for any reader who wants to parse
trailers to do so. But there are a lot of subtle corner
cases around what counts as a trailer, when the trailer
block begins and ends, etc. Since interpret-trailers already
has our parsing logic, let's let callers ask it to just
output the trailers.

They still have to parse the "key: value" lines, but at
least they can ignore all of the other corner cases.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt |  3 +++
 builtin/interpret-trailers.c             |  1 +
 t/t7513-interpret-trailers.sh            | 37 ++++++++++++++++++++++++++++++++
 trailer.c                                | 19 ++++++++++------
 trailer.h                                |  1 +
 5 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 31cdeaecdf..295dffbd21 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -80,6 +80,9 @@ OPTIONS
 	trailer to the input messages. See the description of this
 	command.
 
+--only-trailers::
+	Output only the trailers, not any other parts of the input.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index bb0d7b937a..afb12c11bc 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -24,6 +24,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
+		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 0c6f91c433..e5b0718ef6 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1275,4 +1275,41 @@ test_expect_success 'with cut line' '
 	test_cmp expected actual
 '
 
+test_expect_success 'only trailers' '
+	cat >expected <<-\EOF &&
+		existing: existing-value
+		sign: A U Thor <author@example.com>
+		added: added-value
+	EOF
+	git interpret-trailers \
+		--trailer added:added-value \
+		--only-trailers >actual <<-\EOF &&
+		my subject
+
+		my body
+
+		existing: existing-value
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'only-trailers omits non-trailer in middle of block' '
+	cat >expected <<-\EOF &&
+		Signed-off-by: nobody <nobody@nowhere>
+		Signed-off-by: somebody <somebody@somewhere>
+		sign: A U Thor <author@example.com>
+	EOF
+	git interpret-trailers --only-trailers >actual <<-\EOF &&
+		subject
+
+		it is important that the trailers below are signed-off-by
+		so that they meet the "25% trailers Git knows about" heuristic
+
+		Signed-off-by: nobody <nobody@nowhere>
+		this is not a trailer
+		Signed-off-by: somebody <somebody@somewhere>
+	EOF
+	test_cmp expected actual
+'
+
 test_done
diff --git a/trailer.c b/trailer.c
index 0a0c2c264d..a4ff99f98a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -164,13 +164,15 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
 		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
 }
 
-static void print_all(FILE *outfile, struct list_head *head, int trim_empty)
+static void print_all(FILE *outfile, struct list_head *head,
+		      struct process_trailer_options *opts)
 {
 	struct list_head *pos;
 	struct trailer_item *item;
 	list_for_each(pos, head) {
 		item = list_entry(pos, struct trailer_item, list);
-		if (!trim_empty || strlen(item->value) > 0)
+		if ((!opts->trim_empty || strlen(item->value) > 0) &&
+		    (!opts->only_trailers || item->token))
 			print_tok_val(outfile, item->token, item->value);
 	}
 }
@@ -897,9 +899,10 @@ static int process_input_file(FILE *outfile,
 	trailer_info_get(&info, str);
 
 	/* Print lines before the trailers as is */
-	fwrite(str, 1, info.trailer_start - str, outfile);
+	if (outfile)
+		fwrite(str, 1, info.trailer_start - str, outfile);
 
-	if (!info.blank_line_before_trailer)
+	if (outfile && !info.blank_line_before_trailer)
 		fprintf(outfile, "\n");
 
 	for (i = 0; i < info.trailer_nr; i++) {
@@ -985,18 +988,20 @@ void process_trailers(const char *file, struct process_trailer_options *opts,
 		outfile = create_in_place_tempfile(file);
 
 	/* Print the lines before the trailers */
-	trailer_end = process_input_file(outfile, sb.buf, &head);
+	trailer_end = process_input_file(opts->only_trailers ? NULL : outfile,
+					 sb.buf, &head);
 
 	process_command_line_args(&arg_head, trailers);
 
 	process_trailers_lists(&head, &arg_head);
 
-	print_all(outfile, &head, opts->trim_empty);
+	print_all(outfile, &head, opts);
 
 	free_all(&head);
 
 	/* Print the lines after the trailers as is */
-	fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
+	if (!opts->only_trailers)
+		fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
 
 	if (opts->in_place)
 		if (rename_tempfile(&trailers_tempfile, file))
diff --git a/trailer.h b/trailer.h
index b33c5aa57d..4270849d80 100644
--- a/trailer.h
+++ b/trailer.h
@@ -25,6 +25,7 @@ struct trailer_info {
 struct process_trailer_options {
 	int in_place;
 	int trim_empty;
+	int only_trailers;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.14.0.609.gd2d1f7ddf

[PATCH 3/5] interpret-trailers: add an option to show only existing trailers

[Jeff King]

It can be useful to invoke interpret-trailers for the
primary purpose of parsing existing trailers. But in that
case, we don't want to apply existing ifMissing or ifExists
rules from the config. Let's add a special mode where we
avoid applying those rules. Coupled with --only-trailers,
this gives us a reasonable parsing tool.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt |  5 +++++
 builtin/interpret-trailers.c             |  7 +++++++
 t/t7513-interpret-trailers.sh            | 15 +++++++++++++++
 trailer.c                                |  7 ++++---
 trailer.h                                |  1 +
 5 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 295dffbd21..b2a8fad248 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -83,6 +83,11 @@ OPTIONS
 --only-trailers::
 	Output only the trailers, not any other parts of the input.
 
+--only-existing::
+	Output only trailers that exist in the input; do not add any
+	from the command-line or by following configured `trailer.*`
+	rules.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index afb12c11bc..a49f94ba34 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -25,6 +25,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
+		OPT_BOOL(0, "only-existing", &opts.only_existing, N_("output only existing trailers")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
@@ -33,6 +34,12 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_interpret_trailers_usage, 0);
 
+	if (opts.only_existing && trailers.nr)
+		usage_msg_opt(
+			_("--trailer with --only-existing does not make sense"),
+			git_interpret_trailers_usage,
+			options);
+
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index e5b0718ef6..525fd53e5b 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1312,4 +1312,19 @@ test_expect_success 'only-trailers omits non-trailer in middle of block' '
 	test_cmp expected actual
 '
 
+test_expect_success 'only existing' '
+	cat >expected <<-\EOF &&
+		existing: existing-value
+	EOF
+	git interpret-trailers \
+		--only-trailers --only-existing >actual <<-\EOF &&
+		my subject
+
+		my body
+
+		existing: existing-value
+	EOF
+	test_cmp expected actual
+'
+
 test_done
diff --git a/trailer.c b/trailer.c
index a4ff99f98a..88f6efe523 100644
--- a/trailer.c
+++ b/trailer.c
@@ -991,9 +991,10 @@ void process_trailers(const char *file, struct process_trailer_options *opts,
 	trailer_end = process_input_file(opts->only_trailers ? NULL : outfile,
 					 sb.buf, &head);
 
-	process_command_line_args(&arg_head, trailers);
-
-	process_trailers_lists(&head, &arg_head);
+	if (!opts->only_existing) {
+		process_command_line_args(&arg_head, trailers);
+		process_trailers_lists(&head, &arg_head);
+	}
 
 	print_all(outfile, &head, opts);
 
diff --git a/trailer.h b/trailer.h
index 4270849d80..6356b890ba 100644
--- a/trailer.h
+++ b/trailer.h
@@ -26,6 +26,7 @@ struct process_trailer_options {
 	int in_place;
 	int trim_empty;
 	int only_trailers;
+	int only_existing;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.14.0.609.gd2d1f7ddf

[PATCH 4/5] interpret-trailers: add an option to normalize output

[Jeff King]

The point of "--only-trailers" is to give a caller an output
that's easy for them to parse. Getting rid of the
non-trailer material helps, but we still may see more
complicated syntax like whitespace continuation. Let's add
an option to normalize the output into one "key: value" line
per trailer.

As a bonus, this could be used even without --only-trailers
to clean up unusual formatting in the incoming data.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt |  5 +++++
 builtin/interpret-trailers.c             |  1 +
 t/t7513-interpret-trailers.sh            | 21 ++++++++++++++++++++
 trailer.c                                | 34 +++++++++++++++++++++++++++++++-
 trailer.h                                |  1 +
 5 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index b2a8fad248..6d867e8ab3 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -88,6 +88,11 @@ OPTIONS
 	from the command-line or by following configured `trailer.*`
 	rules.
 
+--normalize::
+	Normalize trailer output so that trailers appear exactly one per
+	line, with any existing whitespace continuation folded into a
+	single line.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index a49f94ba34..ed2d893b4f 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -26,6 +26,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
 		OPT_BOOL(0, "only-existing", &opts.only_existing, N_("output only existing trailers")),
+		OPT_BOOL(0, "normalize", &opts.normalize, N_("normalize trailer formatting")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 525fd53e5b..6ad1feb1c5 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1327,4 +1327,25 @@ test_expect_success 'only existing' '
 	test_cmp expected actual
 '
 
+test_expect_success 'normalize' '
+	cat >expected <<-\EOF &&
+		foo: continued across several lines
+	EOF
+	# pass through tr to make leading and trailing whitespace more obvious
+	tr _ " " <<-\EOF |
+		my subject
+
+		my body
+
+		foo:_
+		__continued
+		___across
+		____several
+		_____lines
+		___
+	EOF
+	git interpret-trailers --only-trailers --only-existing --normalize >actual &&
+	test_cmp expected actual
+'
+
 test_done
diff --git a/trailer.c b/trailer.c
index 88f6efe523..575aa6dcbf 100644
--- a/trailer.c
+++ b/trailer.c
@@ -887,7 +887,35 @@ static int ends_with_blank_line(const char *buf, size_t len)
 	return is_blank_line(buf + ll);
 }
 
+static void normalize_value(struct strbuf *val)
+{
+	struct strbuf out = STRBUF_INIT;
+	size_t i;
+
+	strbuf_grow(&out, val->len);
+	i = 0;
+	while (i < val->len) {
+		char c = val->buf[i++];
+		if (c == '\n') {
+			/* Collapse continuation down to a single space. */
+			while (i < val->len && isspace(val->buf[i]))
+				i++;
+			strbuf_addch(&out, ' ');
+		} else {
+			strbuf_addch(&out, c);
+		}
+	}
+
+	/* Empty lines may have left us with whitespace cruft at the edges */
+	strbuf_trim(&out);
+
+	/* output goes back to val as if we modified it in-place */
+	strbuf_swap(&out, val);
+	strbuf_release(&out);
+}
+
 static int process_input_file(FILE *outfile,
+			      int normalize,
 			      const char *str,
 			      struct list_head *head)
 {
@@ -914,12 +942,16 @@ static int process_input_file(FILE *outfile,
 		if (separator_pos >= 1) {
 			parse_trailer(&tok, &val, NULL, trailer,
 				      separator_pos);
+			if (normalize)
+				normalize_value(&val);
 			add_trailer_item(head,
 					 strbuf_detach(&tok, NULL),
 					 strbuf_detach(&val, NULL));
 		} else {
 			strbuf_addstr(&val, trailer);
 			strbuf_strip_suffix(&val, "\n");
+			if (normalize)
+				normalize_value(&val);
 			add_trailer_item(head,
 					 NULL,
 					 strbuf_detach(&val, NULL));
@@ -989,7 +1021,7 @@ void process_trailers(const char *file, struct process_trailer_options *opts,
 
 	/* Print the lines before the trailers */
 	trailer_end = process_input_file(opts->only_trailers ? NULL : outfile,
-					 sb.buf, &head);
+					 opts->normalize, sb.buf, &head);
 
 	if (!opts->only_existing) {
 		process_command_line_args(&arg_head, trailers);
diff --git a/trailer.h b/trailer.h
index 6356b890ba..a5a9c08b5f 100644
--- a/trailer.h
+++ b/trailer.h
@@ -27,6 +27,7 @@ struct process_trailer_options {
 	int trim_empty;
 	int only_trailers;
 	int only_existing;
+	int normalize;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.14.0.609.gd2d1f7ddf

[PATCH 5/5] interpret-trailers: add --parse convenience option

[Jeff King]

The last few commits have added command line options that
can turn interpret-trailers into a parsing tool. Since
they'd most often be used together, let's provide a
convenient single option for callers to invoke this mode.

This is implemented as a callback rather than a boolean so
that its effect is applied immediately, as if those options
had been specified. Later options can then override them.
E.g.:

  git interpret-trailers --parse --no-normalize

would work.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt |  4 ++++
 builtin/interpret-trailers.c             | 12 ++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 6d867e8ab3..ab2d5c7696 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -93,6 +93,10 @@ OPTIONS
 	line, with any existing whitespace continuation folded into a
 	single line.
 
+--parse::
+	A convenience alias for `--only-trailers --only-existing
+	--normalize`.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index ed2d893b4f..70ca855aa6 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -16,6 +16,16 @@ static const char * const git_interpret_trailers_usage[] = {
 	NULL
 };
 
+static int parse_opt_parse(const struct option *opt, const char *arg,
+			   int unset)
+{
+	struct process_trailer_options *v = opt->value;
+	v->only_trailers = 1;
+	v->only_existing = 1;
+	v->normalize = 1;
+	return 0;
+}
+
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
 	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
@@ -27,6 +37,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
 		OPT_BOOL(0, "only-existing", &opts.only_existing, N_("output only existing trailers")),
 		OPT_BOOL(0, "normalize", &opts.normalize, N_("normalize trailer formatting")),
+		{ OPTION_CALLBACK, 0, "parse", &opts, NULL, N_("set parsing options"),
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse },
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
-- 
2.14.0.609.gd2d1f7ddf

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Parsing trailers out of a commit message is _mostly_ easy, but there
> area a lot of funny corner cases (e.g., heuristics for how many
> non-trailers must be present before a final paragraph isn't a trailer
> block anymore).  The code in trailer.c already knows about these corner
> cases, but there's no way to access it from the command line.
>
> This series teaches interpret-trailers to parse and output just the
> trailers. So now you can do:
>
>   $ git log --format=%B -1 8d44797cc91231cd44955279040dc4a1ee0a797f |
>     git interpret-trailers --parse
>   Signed-off-by: Hartmut Henkel <henkel@vh-s.de>
>   Helped-by: Stefan Beller <sbeller@google.com>
>   Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
>   Acked-by: Matthias Rüster <matthias.ruester@gmail.com>

Thank-you, thank-you, thank-you.

The above example made me wonder if we also want a format specifier
to do the above without piping, but it turns out that we already
have "log --format=%(trailers)", so we are good ;-)

> I considered a few different approaches before deciding on
> what's here:
>
>   1. The output format is actually the normal "key: value" trailers. I
>      considered something more structured like JSON. But the "key:
>      value" format is quite easy to parse, once it has been normalized
>      (finding the trailers, unfolding whitespace continuation, etc).
>
>   2. This series introduces several orthogonal options which can be used
>      together to achieve my goal, when there could just be a "parse"
>      mode. Since interpret-trailers is plumbing, I reasoned that the
>      individual options might still be useful apart from each other (for
>      instance, you could re-normalize existing trailers while writing
>      your new ones from a commit hook). I did add a "--parse" for
>      convenience and to help point users in the right direction.
>
>      For the same reason (and so we could build on other orthogonal
>      features like --in-place and --trim-empty), I decided against
>      having a separate command like "git parse-trailers".
>
>   [1/5]: trailer: put process_trailers() options into a struct
>   [2/5]: interpret-trailers: add an option to show only the trailers
>   [3/5]: interpret-trailers: add an option to show only existing trailers
>   [4/5]: interpret-trailers: add an option to normalize output
>   [5/5]: interpret-trailers: add --parse convenience option
>
>  Documentation/git-interpret-trailers.txt | 17 ++++++++
>  builtin/interpret-trailers.c             | 34 ++++++++++++---
>  t/t7513-interpret-trailers.sh            | 73 ++++++++++++++++++++++++++++++++
>  trailer.c                                | 65 ++++++++++++++++++++++------
>  trailer.h                                | 12 +++++-
>  5 files changed, 180 insertions(+), 21 deletions(-)
>
> -Peff

Re: [PATCH 2/5] interpret-trailers: add an option to show only the trailers

[Stefan Beller]

On Wed, Aug 9, 2017 at 5:24 AM, Jeff King <peff@peff.net> wrote:
> In theory it's easy for any reader who wants to parse
> trailers to do so. But there are a lot of subtle corner
> cases around what counts as a trailer, when the trailer
> block begins and ends, etc. Since interpret-trailers already
> has our parsing logic, let's let callers ask it to just
> output the trailers.
>
> They still have to parse the "key: value" lines, but at
> least they can ignore all of the other corner cases.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
...
> +test_expect_success 'only-trailers omits non-trailer in middle of block' '
> +       cat >expected <<-\EOF &&
> +               Signed-off-by: nobody <nobody@nowhere>
> +               Signed-off-by: somebody <somebody@somewhere>
> +               sign: A U Thor <author@example.com>
> +       EOF
> +       git interpret-trailers --only-trailers >actual <<-\EOF &&
> +               subject
> +
> +               it is important that the trailers below are signed-off-by
> +               so that they meet the "25% trailers Git knows about" heuristic
> +
> +               Signed-off-by: nobody <nobody@nowhere>
> +               this is not a trailer

Please see 60ef86a162 (trailer: support values folded to multiple lines,
2016-10-21), maybe we also want to test for

VeryLongTrailerKey: long text with spaces and breaking
    the line.

For those parsing the trailer do we unbreak the line?
Such that one line equals one trailer? or is the user of the parsed
output expected to take care of this themselves?

Re: [PATCH 2/5] interpret-trailers: add an option to show only the trailers

[Stefan Beller]

On Wed, Aug 9, 2017 at 10:52 AM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Aug 9, 2017 at 5:24 AM, Jeff King <peff@peff.net> wrote:
>> In theory it's easy for any reader who wants to parse
>> trailers to do so. But there are a lot of subtle corner
>> cases around what counts as a trailer, when the trailer
>> block begins and ends, etc. Since interpret-trailers already
>> has our parsing logic, let's let callers ask it to just
>> output the trailers.
>>
>> They still have to parse the "key: value" lines, but at
>> least they can ignore all of the other corner cases.
>>
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
> ...
>> +test_expect_success 'only-trailers omits non-trailer in middle of block' '
>> +       cat >expected <<-\EOF &&
>> +               Signed-off-by: nobody <nobody@nowhere>
>> +               Signed-off-by: somebody <somebody@somewhere>
>> +               sign: A U Thor <author@example.com>
>> +       EOF
>> +       git interpret-trailers --only-trailers >actual <<-\EOF &&
>> +               subject
>> +
>> +               it is important that the trailers below are signed-off-by
>> +               so that they meet the "25% trailers Git knows about" heuristic
>> +
>> +               Signed-off-by: nobody <nobody@nowhere>
>> +               this is not a trailer
>
> Please see 60ef86a162 (trailer: support values folded to multiple lines,
> 2016-10-21), maybe we also want to test for
>
> VeryLongTrailerKey: long text with spaces and breaking
>     the line.
>
> For those parsing the trailer do we unbreak the line?
> Such that one line equals one trailer? or is the user of the parsed
> output expected to take care of this themselves?\

Nevermind, 4/5 solves that problem.

Re: [PATCH 3/5] interpret-trailers: add an option to show only existing trailers

[Stefan Beller]

On Wed, Aug 9, 2017 at 5:24 AM, Jeff King <peff@peff.net> wrote:
> It can be useful to invoke interpret-trailers for the
> primary purpose of parsing existing trailers. But in that
> case, we don't want to apply existing ifMissing or ifExists
> rules from the config. Let's add a special mode where we
> avoid applying those rules. Coupled with --only-trailers,
> this gives us a reasonable parsing tool.

I have the impression that the name is slightly misleading
because 'only' just reduces the set. it does not enhance it.
(Do we have a configuration that says "remove this trailer
anytime"?)

So maybe this is rather worded as 'exact-trailers' ?

Re: [PATCH 5/5] interpret-trailers: add --parse convenience option

[Stefan Beller]

On Wed, Aug 9, 2017 at 5:26 AM, Jeff King <peff@peff.net> wrote:
> The last few commits have added command line options that
> can turn interpret-trailers into a parsing tool. Since
> they'd most often be used together, let's provide a
> convenient single option for callers to invoke this mode.
>
> This is implemented as a callback rather than a boolean so
> that its effect is applied immediately, as if those options
> had been specified. Later options can then override them.
> E.g.:
>
>   git interpret-trailers --parse --no-normalize
>
> would work.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/git-interpret-trailers.txt |  4 ++++
>  builtin/interpret-trailers.c             | 12 ++++++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
> index 6d867e8ab3..ab2d5c7696 100644
> --- a/Documentation/git-interpret-trailers.txt
> +++ b/Documentation/git-interpret-trailers.txt
> @@ -93,6 +93,10 @@ OPTIONS
>         line, with any existing whitespace continuation folded into a
>         single line.
>
> +--parse::
> +       A convenience alias for `--only-trailers --only-existing
> +       --normalize`.

Somewhere in this series, we'd want to not just describe each
of the new knobs, but reword the initial description, too?

    git-interpret-trailers - help add structured information
        into commit messages

Maybe a s/add/handle/ s/into// is enough?

Re: [PATCH 2/5] interpret-trailers: add an option to show only the trailers

[Jonathan Tan]

On Wed, 9 Aug 2017 08:24:03 -0400
Jeff King <peff@peff.net> wrote:

> diff --git a/trailer.c b/trailer.c
> index 0a0c2c264d..a4ff99f98a 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -164,13 +164,15 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
>  		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
>  }
>  
> -static void print_all(FILE *outfile, struct list_head *head, int trim_empty)
> +static void print_all(FILE *outfile, struct list_head *head,
> +		      struct process_trailer_options *opts)

This can be const, I think. (Same thing for patch 1.)

>  {
>  	struct list_head *pos;
>  	struct trailer_item *item;
>  	list_for_each(pos, head) {
>  		item = list_entry(pos, struct trailer_item, list);
> -		if (!trim_empty || strlen(item->value) > 0)
> +		if ((!opts->trim_empty || strlen(item->value) > 0) &&
> +		    (!opts->only_trailers || item->token))
>  			print_tok_val(outfile, item->token, item->value);
>  	}
>  }
> @@ -897,9 +899,10 @@ static int process_input_file(FILE *outfile,
>  	trailer_info_get(&info, str);
>  
>  	/* Print lines before the trailers as is */
> -	fwrite(str, 1, info.trailer_start - str, outfile);
> +	if (outfile)

Any reason why you expect outfile to possibly be NULL?

> +		fwrite(str, 1, info.trailer_start - str, outfile);
>  
> -	if (!info.blank_line_before_trailer)
> +	if (outfile && !info.blank_line_before_trailer)

Same comment here.

>  		fprintf(outfile, "\n");
>  
>  	for (i = 0; i < info.trailer_nr; i++) {

Re: [PATCH 3/5] interpret-trailers: add an option to show only existing trailers

[Jonathan Tan]

On Wed, 9 Aug 2017 08:24:40 -0400
Jeff King <peff@peff.net> wrote:

> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> index e5b0718ef6..525fd53e5b 100755
> --- a/t/t7513-interpret-trailers.sh
> +++ b/t/t7513-interpret-trailers.sh
> @@ -1312,4 +1312,19 @@ test_expect_success 'only-trailers omits non-trailer in middle of block' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'only existing' '
> +	cat >expected <<-\EOF &&
> +		existing: existing-value
> +	EOF
> +	git interpret-trailers \
> +		--only-trailers --only-existing >actual <<-\EOF &&
> +		my subject
> +
> +		my body
> +
> +		existing: existing-value
> +	EOF
> +	test_cmp expected actual

Would it be worth asserting that the "sign" trailer is configured here
and would normally appear? Maybe through a grep on the output of "git
config".

> +'
> +
>  test_done
> diff --git a/trailer.c b/trailer.c
> index a4ff99f98a..88f6efe523 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -991,9 +991,10 @@ void process_trailers(const char *file, struct process_trailer_options *opts,
>  	trailer_end = process_input_file(opts->only_trailers ? NULL : outfile,
>  					 sb.buf, &head);
>  
> -	process_command_line_args(&arg_head, trailers);
> -
> -	process_trailers_lists(&head, &arg_head);
> +	if (!opts->only_existing) {
> +		process_command_line_args(&arg_head, trailers);
> +		process_trailers_lists(&head, &arg_head);
> +	}

This is a bit confusing, but it is correct, since
process_command_line_args() processes both configured trailers and
command-line trailers.

Having said that, it might be worth declaring LIST_HEAD(arg_head) inside
the if block now.

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Jacob Keller]

On Wed, Aug 9, 2017 at 10:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> Parsing trailers out of a commit message is _mostly_ easy, but there
>> area a lot of funny corner cases (e.g., heuristics for how many
>> non-trailers must be present before a final paragraph isn't a trailer
>> block anymore).  The code in trailer.c already knows about these corner
>> cases, but there's no way to access it from the command line.
>>
>> This series teaches interpret-trailers to parse and output just the
>> trailers. So now you can do:
>>
>>   $ git log --format=%B -1 8d44797cc91231cd44955279040dc4a1ee0a797f |
>>     git interpret-trailers --parse
>>   Signed-off-by: Hartmut Henkel <henkel@vh-s.de>
>>   Helped-by: Stefan Beller <sbeller@google.com>
>>   Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
>>   Acked-by: Matthias Rüster <matthias.ruester@gmail.com>
>
> Thank-you, thank-you, thank-you.
>
> The above example made me wonder if we also want a format specifier
> to do the above without piping, but it turns out that we already
> have "log --format=%(trailers)", so we are good ;-)
>

I was going to say, I thought we had a way to get trailers for a
commit via the pretty format, since that is what i used in the past.

Thanks,
Jake

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Jeff King]

On Thu, Aug 10, 2017 at 12:04:49AM -0700, Jacob Keller wrote:

> >>   $ git log --format=%B -1 8d44797cc91231cd44955279040dc4a1ee0a797f |
> >>     git interpret-trailers --parse
> >>   Signed-off-by: Hartmut Henkel <henkel@vh-s.de>
> >>   Helped-by: Stefan Beller <sbeller@google.com>
> >>   Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
> >>   Acked-by: Matthias Rüster <matthias.ruester@gmail.com>
> >
> > Thank-you, thank-you, thank-you.
> >
> > The above example made me wonder if we also want a format specifier
> > to do the above without piping, but it turns out that we already
> > have "log --format=%(trailers)", so we are good ;-)
> 
> I was going to say, I thought we had a way to get trailers for a
> commit via the pretty format, since that is what i used in the past.

I do like that you could get the trailers for many commits in a single
invocation. That doesn't matter for my current use-case, but obviously
piping through O(n) interpret-trailers invocations is a bad idea.

But there are a few difficulties with using %(trailers) for this, as it
shows everything between the start/end points of the trailer block. In
particular:

  1. You don't get any kind of normalization, so you're on your own for
     parsing things like whitespace continuation, extra spaces before
     separators, etc.

  2. It prints non-trailers that fall inside the block. For instance:

       $ git commit --allow-empty -F - <<-\EOF
       subject

       body

       Signed-off-by: me
       this is not a trailer
       Signed-off-by: you
       EOF
 
       $ git log -1 --format=%B | git interpret-trailers --parse
       Signed-off-by: me
       Signed-off-by: you

       $ git log -1 --format='%(trailers)'
       Signed-off-by: me
       this is not a trailer
       Signed-off-by: you

For (1) I think many callers would prefer to see the original
formatting. Maybe we'd need a %(trailers:normalize) or something.

I'm tempted to call (2) a bug, but I guess it's unclear whether callers
would want to see the whole block, or if they really want just the
individual trailers.

-Peff

Re: [PATCH 3/5] interpret-trailers: add an option to show only existing trailers

[Jeff King]

On Wed, Aug 09, 2017 at 11:18:19AM -0700, Stefan Beller wrote:

> On Wed, Aug 9, 2017 at 5:24 AM, Jeff King <peff@peff.net> wrote:
> > It can be useful to invoke interpret-trailers for the
> > primary purpose of parsing existing trailers. But in that
> > case, we don't want to apply existing ifMissing or ifExists
> > rules from the config. Let's add a special mode where we
> > avoid applying those rules. Coupled with --only-trailers,
> > this gives us a reasonable parsing tool.
> 
> I have the impression that the name is slightly misleading
> because 'only' just reduces the set. it does not enhance it.
> (Do we have a configuration that says "remove this trailer
> anytime"?)

No, I think you can only add trailers via ifExists or ifMissing.
I actually called this --no-config originally, because to me it meant
"do not apply config". But the processing applies also to --trailer
arguments no the command line, which is how I ended up with
--only-existing.

> So maybe this is rather worded as 'exact-trailers' ?

I'm not fond of that, as it's vague about which exact trailers we're
talking about. I also thought of something like --verbatim, but I'd
worry that would seem to conflict with --normalize.

I dunno. All of the names seem not quite descriptive enough to me.

-Peff

Re: [PATCH 3/5] interpret-trailers: add an option to show only existing trailers

[Jeff King]

On Wed, Aug 09, 2017 at 11:38:20AM -0700, Jonathan Tan wrote:

> > diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> > index e5b0718ef6..525fd53e5b 100755
> > --- a/t/t7513-interpret-trailers.sh
> > +++ b/t/t7513-interpret-trailers.sh
> > @@ -1312,4 +1312,19 @@ test_expect_success 'only-trailers omits non-trailer in middle of block' '
> >  	test_cmp expected actual
> >  '
> >  
> > +test_expect_success 'only existing' '
> > +	cat >expected <<-\EOF &&
> > +		existing: existing-value
> > +	EOF
> > +	git interpret-trailers \
> > +		--only-trailers --only-existing >actual <<-\EOF &&
> > +		my subject
> > +
> > +		my body
> > +
> > +		existing: existing-value
> > +	EOF
> > +	test_cmp expected actual
> 
> Would it be worth asserting that the "sign" trailer is configured here
> and would normally appear? Maybe through a grep on the output of "git
> config".

I'd much rather re-add it than assert it with grep hackery. Note that
its presence is implied in all of the follow-on tests, too (so I had
sort of assumed that its presence in the --only-trailers test would
imply that it was carried through to the others). We can be more
explicit, though, I guess.

> > diff --git a/trailer.c b/trailer.c
> > index a4ff99f98a..88f6efe523 100644
> > --- a/trailer.c
> > +++ b/trailer.c
> > @@ -991,9 +991,10 @@ void process_trailers(const char *file, struct process_trailer_options *opts,
> >  	trailer_end = process_input_file(opts->only_trailers ? NULL : outfile,
> >  					 sb.buf, &head);
> >  
> > -	process_command_line_args(&arg_head, trailers);
> > -
> > -	process_trailers_lists(&head, &arg_head);
> > +	if (!opts->only_existing) {
> > +		process_command_line_args(&arg_head, trailers);
> > +		process_trailers_lists(&head, &arg_head);
> > +	}
> 
> This is a bit confusing, but it is correct, since
> process_command_line_args() processes both configured trailers and
> command-line trailers.

Yes, it confused me, too. That combination is why "--trailer" is
disallowed with --only-existing (which otherwise could work together). I
didn't think it was worth refactoring for a case that I don't think
anybody would care about.

> Having said that, it might be worth declaring LIST_HEAD(arg_head) inside
> the if block now.

Agreed.

-Peff

Re: [PATCH 2/5] interpret-trailers: add an option to show only the trailers

[Jeff King]

On Wed, Aug 09, 2017 at 11:35:27AM -0700, Jonathan Tan wrote:

> > -static void print_all(FILE *outfile, struct list_head *head, int trim_empty)
> > +static void print_all(FILE *outfile, struct list_head *head,
> > +		      struct process_trailer_options *opts)
> 
> This can be const, I think. (Same thing for patch 1.)

OK. We often leave these kinds of option structs as non-const because
they can sometimes grow to carry state between functions (e.g.,
diff_opt). But it's certainly const-able now, so we can let somebody
undo it later if they want.

> > @@ -897,9 +899,10 @@ static int process_input_file(FILE *outfile,
> >  	trailer_info_get(&info, str);
> >  
> >  	/* Print lines before the trailers as is */
> > -	fwrite(str, 1, info.trailer_start - str, outfile);
> > +	if (outfile)
> 
> Any reason why you expect outfile to possibly be NULL?
> 
> > +		fwrite(str, 1, info.trailer_start - str, outfile);
> >  
> > -	if (!info.blank_line_before_trailer)
> > +	if (outfile && !info.blank_line_before_trailer)
> 
> Same comment here.

Because of this hunk from later in the file where we pass in NULL:

        /* Print the lines before the trailers */
-       trailer_end = process_input_file(outfile, sb.buf, &head);
+       trailer_end = process_input_file(opts->only_trailers ? NULL : outfile,
+                                        sb.buf, &head);

-Peff

Re: [PATCH 5/5] interpret-trailers: add --parse convenience option

[Jeff King]

On Wed, Aug 09, 2017 at 11:20:12AM -0700, Stefan Beller wrote:

> > +--parse::
> > +       A convenience alias for `--only-trailers --only-existing
> > +       --normalize`.
> 
> Somewhere in this series, we'd want to not just describe each
> of the new knobs, but reword the initial description, too?
> 
>     git-interpret-trailers - help add structured information
>         into commit messages
> 
> Maybe a s/add/handle/ s/into// is enough?

Thanks, my initial attempt actually included that, when I thought I was
going to have a unique "parsing mode". But then as it grew into
individual options, I dropped it. And forgot how modification-centric
the rest of the manpage description is. I'll try to include something in
the final commit, which is where the parsing mode is all tied together.

-Peff

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Jeff King]

On Wed, Aug 09, 2017 at 08:21:47AM -0400, Jeff King wrote:

> This series teaches interpret-trailers to parse and output just the
> trailers. So now you can do:
> 
>   $ git log --format=%B -1 8d44797cc91231cd44955279040dc4a1ee0a797f |
>     git interpret-trailers --parse
>   Signed-off-by: Hartmut Henkel <henkel@vh-s.de>
>   Helped-by: Stefan Beller <sbeller@google.com>
>   Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
>   Acked-by: Matthias Rüster <matthias.ruester@gmail.com>

And here's a v2 that addresses all of the comments except one: Stefan
suggested that --only-existing wasn't a great name. I agree, but I like
everything else less.

Summary:

  - opts arguments are now const
  - tests that depend on the value of trailer.sign.command now set it
    explicitly
  - the arg_head variable is now moved into the conditional block whewre
    it's used
  - the interpret-trailers manpage has been updated in patch 5 to make
    it clear that "add" and "parse" are the two major modes

  [1/5]: trailer: put process_trailers() options into a struct
  [2/5]: interpret-trailers: add an option to show only the trailers
  [3/5]: interpret-trailers: add an option to show only existing trailers
  [4/5]: interpret-trailers: add an option to normalize output
  [5/5]: interpret-trailers: add --parse convenience option

 Documentation/git-interpret-trailers.txt | 34 +++++++++++---
 builtin/interpret-trailers.c             | 34 +++++++++++---
 t/t7513-interpret-trailers.sh            | 76 ++++++++++++++++++++++++++++++++
 trailer.c                                | 68 ++++++++++++++++++++++------
 trailer.h                                | 13 +++++-
 5 files changed, 196 insertions(+), 29 deletions(-)

-Peff

[PATCH 1/5] trailer: put process_trailers() options into a struct

[Jeff King]

We already have two options and are about to add a few more.
To avoid having a huge number of boolean arguments, let's
convert to an options struct which can be passed in.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/interpret-trailers.c | 13 ++++++-------
 trailer.c                    | 10 ++++++----
 trailer.h                    | 10 +++++++++-
 3 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 175f14797b..bb0d7b937a 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -18,13 +18,12 @@ static const char * const git_interpret_trailers_usage[] = {
 
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
-	int in_place = 0;
-	int trim_empty = 0;
+	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
 	struct string_list trailers = STRING_LIST_INIT_NODUP;
 
 	struct option options[] = {
-		OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
-		OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty trailers")),
+		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
+		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
@@ -36,11 +35,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
-			process_trailers(argv[i], in_place, trim_empty, &trailers);
+			process_trailers(argv[i], &opts, &trailers);
 	} else {
-		if (in_place)
+		if (opts.in_place)
 			die(_("no input file given for in-place editing"));
-		process_trailers(NULL, in_place, trim_empty, &trailers);
+		process_trailers(NULL, &opts, &trailers);
 	}
 
 	string_list_clear(&trailers, 0);
diff --git a/trailer.c b/trailer.c
index 751b56c009..e21a0d1629 100644
--- a/trailer.c
+++ b/trailer.c
@@ -968,7 +968,9 @@ static FILE *create_in_place_tempfile(const char *file)
 	return outfile;
 }
 
-void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)
+void process_trailers(const char *file,
+		      const struct process_trailer_options *opts,
+		      struct string_list *trailers)
 {
 	LIST_HEAD(head);
 	LIST_HEAD(arg_head);
@@ -980,7 +982,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 
 	read_input_file(&sb, file);
 
-	if (in_place)
+	if (opts->in_place)
 		outfile = create_in_place_tempfile(file);
 
 	/* Print the lines before the trailers */
@@ -990,14 +992,14 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 
 	process_trailers_lists(&head, &arg_head);
 
-	print_all(outfile, &head, trim_empty);
+	print_all(outfile, &head, opts->trim_empty);
 
 	free_all(&head);
 
 	/* Print the lines after the trailers as is */
 	fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
 
-	if (in_place)
+	if (opts->in_place)
 		if (rename_tempfile(&trailers_tempfile, file))
 			die_errno(_("could not rename temporary file to %s"), file);
 
diff --git a/trailer.h b/trailer.h
index 65cc5d79c6..9da00bedec 100644
--- a/trailer.h
+++ b/trailer.h
@@ -22,7 +22,15 @@ struct trailer_info {
 	size_t trailer_nr;
 };
 
-void process_trailers(const char *file, int in_place, int trim_empty,
+struct process_trailer_options {
+	int in_place;
+	int trim_empty;
+};
+
+#define PROCESS_TRAILER_OPTIONS_INIT {0}
+
+void process_trailers(const char *file,
+		      const struct process_trailer_options *opts,
 		      struct string_list *trailers);
 
 void trailer_info_get(struct trailer_info *info, const char *str);
-- 
2.14.0.614.g0beb26d5e9

[PATCH 2/5] interpret-trailers: add an option to show only the trailers

[Jeff King]

In theory it's easy for any reader who wants to parse
trailers to do so. But there are a lot of subtle corner
cases around what counts as a trailer, when the trailer
block begins and ends, etc. Since interpret-trailers already
has our parsing logic, let's let callers ask it to just
output the trailers.

They still have to parse the "key: value" lines, but at
least they can ignore all of the other corner cases.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt |  3 +++
 builtin/interpret-trailers.c             |  1 +
 t/t7513-interpret-trailers.sh            | 39 ++++++++++++++++++++++++++++++++
 trailer.c                                | 19 ++++++++++------
 trailer.h                                |  1 +
 5 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 31cdeaecdf..295dffbd21 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -80,6 +80,9 @@ OPTIONS
 	trailer to the input messages. See the description of this
 	command.
 
+--only-trailers::
+	Output only the trailers, not any other parts of the input.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index bb0d7b937a..afb12c11bc 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -24,6 +24,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
+		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 0c6f91c433..90d30037b7 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1275,4 +1275,43 @@ test_expect_success 'with cut line' '
 	test_cmp expected actual
 '
 
+test_expect_success 'only trailers' '
+	git config trailer.sign.command "echo config-value" &&
+	cat >expected <<-\EOF &&
+		existing: existing-value
+		sign: config-value
+		added: added-value
+	EOF
+	git interpret-trailers \
+		--trailer added:added-value \
+		--only-trailers >actual <<-\EOF &&
+		my subject
+
+		my body
+
+		existing: existing-value
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'only-trailers omits non-trailer in middle of block' '
+	git config trailer.sign.command "echo config-value" &&
+	cat >expected <<-\EOF &&
+		Signed-off-by: nobody <nobody@nowhere>
+		Signed-off-by: somebody <somebody@somewhere>
+		sign: config-value
+	EOF
+	git interpret-trailers --only-trailers >actual <<-\EOF &&
+		subject
+
+		it is important that the trailers below are signed-off-by
+		so that they meet the "25% trailers Git knows about" heuristic
+
+		Signed-off-by: nobody <nobody@nowhere>
+		this is not a trailer
+		Signed-off-by: somebody <somebody@somewhere>
+	EOF
+	test_cmp expected actual
+'
+
 test_done
diff --git a/trailer.c b/trailer.c
index e21a0d1629..706351fc98 100644
--- a/trailer.c
+++ b/trailer.c
@@ -164,13 +164,15 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
 		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
 }
 
-static void print_all(FILE *outfile, struct list_head *head, int trim_empty)
+static void print_all(FILE *outfile, struct list_head *head,
+		      const struct process_trailer_options *opts)
 {
 	struct list_head *pos;
 	struct trailer_item *item;
 	list_for_each(pos, head) {
 		item = list_entry(pos, struct trailer_item, list);
-		if (!trim_empty || strlen(item->value) > 0)
+		if ((!opts->trim_empty || strlen(item->value) > 0) &&
+		    (!opts->only_trailers || item->token))
 			print_tok_val(outfile, item->token, item->value);
 	}
 }
@@ -897,9 +899,10 @@ static int process_input_file(FILE *outfile,
 	trailer_info_get(&info, str);
 
 	/* Print lines before the trailers as is */
-	fwrite(str, 1, info.trailer_start - str, outfile);
+	if (outfile)
+		fwrite(str, 1, info.trailer_start - str, outfile);
 
-	if (!info.blank_line_before_trailer)
+	if (outfile && !info.blank_line_before_trailer)
 		fprintf(outfile, "\n");
 
 	for (i = 0; i < info.trailer_nr; i++) {
@@ -986,18 +989,20 @@ void process_trailers(const char *file,
 		outfile = create_in_place_tempfile(file);
 
 	/* Print the lines before the trailers */
-	trailer_end = process_input_file(outfile, sb.buf, &head);
+	trailer_end = process_input_file(opts->only_trailers ? NULL : outfile,
+					 sb.buf, &head);
 
 	process_command_line_args(&arg_head, trailers);
 
 	process_trailers_lists(&head, &arg_head);
 
-	print_all(outfile, &head, opts->trim_empty);
+	print_all(outfile, &head, opts);
 
 	free_all(&head);
 
 	/* Print the lines after the trailers as is */
-	fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
+	if (!opts->only_trailers)
+		fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
 
 	if (opts->in_place)
 		if (rename_tempfile(&trailers_tempfile, file))
diff --git a/trailer.h b/trailer.h
index 9da00bedec..3cf35ced00 100644
--- a/trailer.h
+++ b/trailer.h
@@ -25,6 +25,7 @@ struct trailer_info {
 struct process_trailer_options {
 	int in_place;
 	int trim_empty;
+	int only_trailers;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.14.0.614.g0beb26d5e9

[PATCH 3/5] interpret-trailers: add an option to show only existing trailers

[Jeff King]

It can be useful to invoke interpret-trailers for the
primary purpose of parsing existing trailers. But in that
case, we don't want to apply existing ifMissing or ifExists
rules from the config. Let's add a special mode where we
avoid applying those rules. Coupled with --only-trailers,
this gives us a reasonable parsing tool.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt |  5 +++++
 builtin/interpret-trailers.c             |  7 +++++++
 t/t7513-interpret-trailers.sh            | 16 ++++++++++++++++
 trailer.c                                |  9 +++++----
 trailer.h                                |  1 +
 5 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 295dffbd21..b2a8fad248 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -83,6 +83,11 @@ OPTIONS
 --only-trailers::
 	Output only the trailers, not any other parts of the input.
 
+--only-existing::
+	Output only trailers that exist in the input; do not add any
+	from the command-line or by following configured `trailer.*`
+	rules.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index afb12c11bc..a49f94ba34 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -25,6 +25,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
+		OPT_BOOL(0, "only-existing", &opts.only_existing, N_("output only existing trailers")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
@@ -33,6 +34,12 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_interpret_trailers_usage, 0);
 
+	if (opts.only_existing && trailers.nr)
+		usage_msg_opt(
+			_("--trailer with --only-existing does not make sense"),
+			git_interpret_trailers_usage,
+			options);
+
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 90d30037b7..97f4ac0fb3 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1314,4 +1314,20 @@ test_expect_success 'only-trailers omits non-trailer in middle of block' '
 	test_cmp expected actual
 '
 
+test_expect_success 'only existing' '
+	git config trailer.sign.command "echo config-value" &&
+	cat >expected <<-\EOF &&
+		existing: existing-value
+	EOF
+	git interpret-trailers \
+		--only-trailers --only-existing >actual <<-\EOF &&
+		my subject
+
+		my body
+
+		existing: existing-value
+	EOF
+	test_cmp expected actual
+'
+
 test_done
diff --git a/trailer.c b/trailer.c
index 706351fc98..3abac3db37 100644
--- a/trailer.c
+++ b/trailer.c
@@ -976,7 +976,6 @@ void process_trailers(const char *file,
 		      struct string_list *trailers)
 {
 	LIST_HEAD(head);
-	LIST_HEAD(arg_head);
 	struct strbuf sb = STRBUF_INIT;
 	int trailer_end;
 	FILE *outfile = stdout;
@@ -992,9 +991,11 @@ void process_trailers(const char *file,
 	trailer_end = process_input_file(opts->only_trailers ? NULL : outfile,
 					 sb.buf, &head);
 
-	process_command_line_args(&arg_head, trailers);
-
-	process_trailers_lists(&head, &arg_head);
+	if (!opts->only_existing) {
+		LIST_HEAD(arg_head);
+		process_command_line_args(&arg_head, trailers);
+		process_trailers_lists(&head, &arg_head);
+	}
 
 	print_all(outfile, &head, opts);
 
diff --git a/trailer.h b/trailer.h
index 3cf35ced00..96dcbe801a 100644
--- a/trailer.h
+++ b/trailer.h
@@ -26,6 +26,7 @@ struct process_trailer_options {
 	int in_place;
 	int trim_empty;
 	int only_trailers;
+	int only_existing;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.14.0.614.g0beb26d5e9

[PATCH 4/5] interpret-trailers: add an option to normalize output

[Jeff King]

The point of "--only-trailers" is to give a caller an output
that's easy for them to parse. Getting rid of the
non-trailer material helps, but we still may see more
complicated syntax like whitespace continuation. Let's add
an option to normalize the output into one "key: value" line
per trailer.

As a bonus, this could be used even without --only-trailers
to clean up unusual formatting in the incoming data.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt |  5 +++++
 builtin/interpret-trailers.c             |  1 +
 t/t7513-interpret-trailers.sh            | 21 ++++++++++++++++++++
 trailer.c                                | 34 +++++++++++++++++++++++++++++++-
 trailer.h                                |  1 +
 5 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index b2a8fad248..6537faf887 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -88,6 +88,11 @@ OPTIONS
 	from the command-line or by following configured `trailer.*`
 	rules.
 
+--normalize::
+	Normalize trailer output so that trailers appear exactly one per
+	line, with any existing whitespace continuation folded into a
+	single line.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index a49f94ba34..ed2d893b4f 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -26,6 +26,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
 		OPT_BOOL(0, "only-existing", &opts.only_existing, N_("output only existing trailers")),
+		OPT_BOOL(0, "normalize", &opts.normalize, N_("normalize trailer formatting")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 97f4ac0fb3..ef34ee4e49 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1330,4 +1330,25 @@ test_expect_success 'only existing' '
 	test_cmp expected actual
 '
 
+test_expect_success 'normalize' '
+	cat >expected <<-\EOF &&
+		foo: continued across several lines
+	EOF
+	# pass through tr to make leading and trailing whitespace more obvious
+	tr _ " " <<-\EOF |
+		my subject
+
+		my body
+
+		foo:_
+		__continued
+		___across
+		____several
+		_____lines
+		___
+	EOF
+	git interpret-trailers --only-trailers --only-existing --normalize >actual &&
+	test_cmp expected actual
+'
+
 test_done
diff --git a/trailer.c b/trailer.c
index 3abac3db37..1ccf9996b1 100644
--- a/trailer.c
+++ b/trailer.c
@@ -887,7 +887,35 @@ static int ends_with_blank_line(const char *buf, size_t len)
 	return is_blank_line(buf + ll);
 }
 
+static void normalize_value(struct strbuf *val)
+{
+	struct strbuf out = STRBUF_INIT;
+	size_t i;
+
+	strbuf_grow(&out, val->len);
+	i = 0;
+	while (i < val->len) {
+		char c = val->buf[i++];
+		if (c == '\n') {
+			/* Collapse continuation down to a single space. */
+			while (i < val->len && isspace(val->buf[i]))
+				i++;
+			strbuf_addch(&out, ' ');
+		} else {
+			strbuf_addch(&out, c);
+		}
+	}
+
+	/* Empty lines may have left us with whitespace cruft at the edges */
+	strbuf_trim(&out);
+
+	/* output goes back to val as if we modified it in-place */
+	strbuf_swap(&out, val);
+	strbuf_release(&out);
+}
+
 static int process_input_file(FILE *outfile,
+			      int normalize,
 			      const char *str,
 			      struct list_head *head)
 {
@@ -914,12 +942,16 @@ static int process_input_file(FILE *outfile,
 		if (separator_pos >= 1) {
 			parse_trailer(&tok, &val, NULL, trailer,
 				      separator_pos);
+			if (normalize)
+				normalize_value(&val);
 			add_trailer_item(head,
 					 strbuf_detach(&tok, NULL),
 					 strbuf_detach(&val, NULL));
 		} else {
 			strbuf_addstr(&val, trailer);
 			strbuf_strip_suffix(&val, "\n");
+			if (normalize)
+				normalize_value(&val);
 			add_trailer_item(head,
 					 NULL,
 					 strbuf_detach(&val, NULL));
@@ -989,7 +1021,7 @@ void process_trailers(const char *file,
 
 	/* Print the lines before the trailers */
 	trailer_end = process_input_file(opts->only_trailers ? NULL : outfile,
-					 sb.buf, &head);
+					 opts->normalize, sb.buf, &head);
 
 	if (!opts->only_existing) {
 		LIST_HEAD(arg_head);
diff --git a/trailer.h b/trailer.h
index 96dcbe801a..0eaf2e1bdf 100644
--- a/trailer.h
+++ b/trailer.h
@@ -27,6 +27,7 @@ struct process_trailer_options {
 	int trim_empty;
 	int only_trailers;
 	int only_existing;
+	int normalize;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.14.0.614.g0beb26d5e9

[PATCH 5/5] interpret-trailers: add --parse convenience option

[Jeff King]

The last few commits have added command line options that
can turn interpret-trailers into a parsing tool. Since
they'd most often be used together, let's provide a
convenient single option for callers to invoke this mode.

This is implemented as a callback rather than a boolean so
that its effect is applied immediately, as if those options
had been specified. Later options can then override them.
E.g.:

  git interpret-trailers --parse --no-normalize

would work.

Let's also update the documentation to make clear that this
parsing mode behaves quite differently than the normal
"add trailers to the input" mode.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt | 21 ++++++++++++++-------
 builtin/interpret-trailers.c             | 12 ++++++++++++
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 6537faf887..863cb4ec5d 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -3,24 +3,27 @@ git-interpret-trailers(1)
 
 NAME
 ----
-git-interpret-trailers - help add structured information into commit messages
+git-interpret-trailers - add or parse structured information in commit messages
 
 SYNOPSIS
 --------
 [verse]
-'git interpret-trailers' [--in-place] [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]
+'git interpret-trailers' [options] [(--trailer <token>[(=|:)<value>])...] [<file>...]
+'git interpret-trailers' [options] [--parse] [<file>...]
 
 DESCRIPTION
 -----------
-Help adding 'trailers' lines, that look similar to RFC 822 e-mail
+Help parsing or adding 'trailers' lines, that look similar to RFC 822 e-mail
 headers, at the end of the otherwise free-form part of a commit
 message.
 
 This command reads some patches or commit messages from either the
-<file> arguments or the standard input if no <file> is specified. Then
-this command applies the arguments passed using the `--trailer`
-option, if any, to the commit message part of each input file. The
-result is emitted on the standard output.
+<file> arguments or the standard input if no <file> is specified. If
+`--parse` is specified, the output consists of the parsed trailers.
+
+Otherwise, the this command applies the arguments passed using the
+`--trailer` option, if any, to the commit message part of each input
+file. The result is emitted on the standard output.
 
 Some configuration variables control the way the `--trailer` arguments
 are applied to each commit message and the way any existing trailer in
@@ -93,6 +96,10 @@ OPTIONS
 	line, with any existing whitespace continuation folded into a
 	single line.
 
+--parse::
+	A convenience alias for `--only-trailers --only-existing
+	--normalize`.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index ed2d893b4f..70ca855aa6 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -16,6 +16,16 @@ static const char * const git_interpret_trailers_usage[] = {
 	NULL
 };
 
+static int parse_opt_parse(const struct option *opt, const char *arg,
+			   int unset)
+{
+	struct process_trailer_options *v = opt->value;
+	v->only_trailers = 1;
+	v->only_existing = 1;
+	v->normalize = 1;
+	return 0;
+}
+
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
 	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
@@ -27,6 +37,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
 		OPT_BOOL(0, "only-existing", &opts.only_existing, N_("output only existing trailers")),
 		OPT_BOOL(0, "normalize", &opts.normalize, N_("normalize trailer formatting")),
+		{ OPTION_CALLBACK, 0, "parse", &opts, NULL, N_("set parsing options"),
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse },
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
-- 
2.14.0.614.g0beb26d5e9

Re: [PATCH 3/5] interpret-trailers: add an option to show only existing trailers

[Stefan Beller]

On Thu, Aug 10, 2017 at 12:32 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Aug 09, 2017 at 11:18:19AM -0700, Stefan Beller wrote:
>
>> On Wed, Aug 9, 2017 at 5:24 AM, Jeff King <peff@peff.net> wrote:
>> > It can be useful to invoke interpret-trailers for the
>> > primary purpose of parsing existing trailers. But in that
>> > case, we don't want to apply existing ifMissing or ifExists
>> > rules from the config. Let's add a special mode where we
>> > avoid applying those rules. Coupled with --only-trailers,
>> > this gives us a reasonable parsing tool.
>>
>> I have the impression that the name is slightly misleading
>> because 'only' just reduces the set. it does not enhance it.
>> (Do we have a configuration that says "remove this trailer
>> anytime"?)
>
> No, I think you can only add trailers via ifExists or ifMissing.
> I actually called this --no-config originally, because to me it meant
> "do not apply config". But the processing applies also to --trailer
> arguments no the command line, which is how I ended up with
> --only-existing.
>
>> So maybe this is rather worded as 'exact-trailers' ?
>
> I'm not fond of that, as it's vague about which exact trailers we're
> talking about. I also thought of something like --verbatim, but I'd
> worry that would seem to conflict with --normalize.
>
> I dunno. All of the names seem not quite descriptive enough to me.

I meant 'exact' as in 'exactly from the patch/commit, no external
influence such as config', so maybe '--from-patch' or '--from-commit'
(which says the same as --no-config just the other way round.
Having --no- in config options as the standard is a UX disaster
IMHO as then we have to forbid the --no-no-X or reintroduce X
and flip the default)

Maybe --genuine ?

>
> -Peff

Re: [PATCH 3/5] interpret-trailers: add an option to show only existing trailers

[Jeff King]

On Thu, Aug 10, 2017 at 10:27:19AM -0700, Stefan Beller wrote:

> > I'm not fond of that, as it's vague about which exact trailers we're
> > talking about. I also thought of something like --verbatim, but I'd
> > worry that would seem to conflict with --normalize.
> >
> > I dunno. All of the names seem not quite descriptive enough to me.
> 
> I meant 'exact' as in 'exactly from the patch/commit, no external
> influence such as config', so maybe '--from-patch' or '--from-commit'
> (which says the same as --no-config just the other way round.
> Having --no- in config options as the standard is a UX disaster
> IMHO as then we have to forbid the --no-no-X or reintroduce X
> and flip the default)

Yes, that was definitely the other reason I didn't want to call it
"--no-config".  :)

It's not always from a patch or commit. The most accurate along those
lines is "--from-input". 

> Maybe --genuine ?

But in the greater context I think that's vague again; we don't know
which part of the command's operation is "genuine".

Perhaps "--exact-input" hits all of those. Or maybe "--only-input" to
match the other "--only".

I think I like that last one the best. It makes it clear that we are
looking just at the input, and not anything else. Which is exactly what
the feature does.

-Peff

Re: [PATCH 3/5] interpret-trailers: add an option to show only existing trailers

[Stefan Beller]

On Thu, Aug 10, 2017 at 10:33 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Aug 10, 2017 at 10:27:19AM -0700, Stefan Beller wrote:
>
>> > I'm not fond of that, as it's vague about which exact trailers we're
>> > talking about. I also thought of something like --verbatim, but I'd
>> > worry that would seem to conflict with --normalize.
>> >
>> > I dunno. All of the names seem not quite descriptive enough to me.
>>
>> I meant 'exact' as in 'exactly from the patch/commit, no external
>> influence such as config', so maybe '--from-patch' or '--from-commit'
>> (which says the same as --no-config just the other way round.
>> Having --no- in config options as the standard is a UX disaster
>> IMHO as then we have to forbid the --no-no-X or reintroduce X
>> and flip the default)
>
> Yes, that was definitely the other reason I didn't want to call it
> "--no-config".  :)
>
> It's not always from a patch or commit. The most accurate along those
> lines is "--from-input".
>
>> Maybe --genuine ?
>
> But in the greater context I think that's vague again; we don't know
> which part of the command's operation is "genuine".

The input of course. ;) --genuine-input.

>
> Perhaps "--exact-input" hits all of those. Or maybe "--only-input" to
> match the other "--only".
>
> I think I like that last one the best. It makes it clear that we are
> looking just at the input, and not anything else. Which is exactly what
> the feature does.

Makes sense to me,

Thanks,
Stefan

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Jeff King]

On Thu, Aug 10, 2017 at 04:02:46AM -0400, Jeff King wrote:

> On Wed, Aug 09, 2017 at 08:21:47AM -0400, Jeff King wrote:
> 
> > This series teaches interpret-trailers to parse and output just the
> > trailers. So now you can do:
> > 
> >   $ git log --format=%B -1 8d44797cc91231cd44955279040dc4a1ee0a797f |
> >     git interpret-trailers --parse
> >   Signed-off-by: Hartmut Henkel <henkel@vh-s.de>
> >   Helped-by: Stefan Beller <sbeller@google.com>
> >   Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
> >   Acked-by: Matthias Rüster <matthias.ruester@gmail.com>
> 
> And here's a v2 that addresses all of the comments except one: Stefan
> suggested that --only-existing wasn't a great name. I agree, but I like
> everything else less.

Here's a v3 that takes care of that (renaming it to --only-input).

It's otherwise the same as v2, but since the name-change ripples through
the remaining patches, I wanted to get v3 in front of people sooner
rather than later.

  [1/5]: trailer: put process_trailers() options into a struct
  [2/5]: interpret-trailers: add an option to show only the trailers
  [3/5]: interpret-trailers: add an option to show only existing trailers
  [4/5]: interpret-trailers: add an option to normalize output
  [5/5]: interpret-trailers: add --parse convenience option

 Documentation/git-interpret-trailers.txt | 34 +++++++++++---
 builtin/interpret-trailers.c             | 34 +++++++++++---
 t/t7513-interpret-trailers.sh            | 76 ++++++++++++++++++++++++++++++++
 trailer.c                                | 68 ++++++++++++++++++++++------
 trailer.h                                | 13 +++++-
 5 files changed, 196 insertions(+), 29 deletions(-)

[PATCH v3 1/5] trailer: put process_trailers() options into a struct

[Jeff King]

We already have two options and are about to add a few more.
To avoid having a huge number of boolean arguments, let's
convert to an options struct which can be passed in.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/interpret-trailers.c | 13 ++++++-------
 trailer.c                    | 10 ++++++----
 trailer.h                    | 10 +++++++++-
 3 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 175f14797b..bb0d7b937a 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -18,13 +18,12 @@ static const char * const git_interpret_trailers_usage[] = {
 
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
-	int in_place = 0;
-	int trim_empty = 0;
+	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
 	struct string_list trailers = STRING_LIST_INIT_NODUP;
 
 	struct option options[] = {
-		OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
-		OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty trailers")),
+		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
+		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
@@ -36,11 +35,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
-			process_trailers(argv[i], in_place, trim_empty, &trailers);
+			process_trailers(argv[i], &opts, &trailers);
 	} else {
-		if (in_place)
+		if (opts.in_place)
 			die(_("no input file given for in-place editing"));
-		process_trailers(NULL, in_place, trim_empty, &trailers);
+		process_trailers(NULL, &opts, &trailers);
 	}
 
 	string_list_clear(&trailers, 0);
diff --git a/trailer.c b/trailer.c
index 751b56c009..e21a0d1629 100644
--- a/trailer.c
+++ b/trailer.c
@@ -968,7 +968,9 @@ static FILE *create_in_place_tempfile(const char *file)
 	return outfile;
 }
 
-void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)
+void process_trailers(const char *file,
+		      const struct process_trailer_options *opts,
+		      struct string_list *trailers)
 {
 	LIST_HEAD(head);
 	LIST_HEAD(arg_head);
@@ -980,7 +982,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 
 	read_input_file(&sb, file);
 
-	if (in_place)
+	if (opts->in_place)
 		outfile = create_in_place_tempfile(file);
 
 	/* Print the lines before the trailers */
@@ -990,14 +992,14 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 
 	process_trailers_lists(&head, &arg_head);
 
-	print_all(outfile, &head, trim_empty);
+	print_all(outfile, &head, opts->trim_empty);
 
 	free_all(&head);
 
 	/* Print the lines after the trailers as is */
 	fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
 
-	if (in_place)
+	if (opts->in_place)
 		if (rename_tempfile(&trailers_tempfile, file))
 			die_errno(_("could not rename temporary file to %s"), file);
 
diff --git a/trailer.h b/trailer.h
index 65cc5d79c6..9da00bedec 100644
--- a/trailer.h
+++ b/trailer.h
@@ -22,7 +22,15 @@ struct trailer_info {
 	size_t trailer_nr;
 };
 
-void process_trailers(const char *file, int in_place, int trim_empty,
+struct process_trailer_options {
+	int in_place;
+	int trim_empty;
+};
+
+#define PROCESS_TRAILER_OPTIONS_INIT {0}
+
+void process_trailers(const char *file,
+		      const struct process_trailer_options *opts,
 		      struct string_list *trailers);
 
 void trailer_info_get(struct trailer_info *info, const char *str);
-- 
2.14.0.614.g0beb26d5e9

[PATCH v3 2/5] interpret-trailers: add an option to show only the trailers

[Jeff King]

In theory it's easy for any reader who wants to parse
trailers to do so. But there are a lot of subtle corner
cases around what counts as a trailer, when the trailer
block begins and ends, etc. Since interpret-trailers already
has our parsing logic, let's let callers ask it to just
output the trailers.

They still have to parse the "key: value" lines, but at
least they can ignore all of the other corner cases.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt |  3 +++
 builtin/interpret-trailers.c             |  1 +
 t/t7513-interpret-trailers.sh            | 39 ++++++++++++++++++++++++++++++++
 trailer.c                                | 19 ++++++++++------
 trailer.h                                |  1 +
 5 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 31cdeaecdf..295dffbd21 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -80,6 +80,9 @@ OPTIONS
 	trailer to the input messages. See the description of this
 	command.
 
+--only-trailers::
+	Output only the trailers, not any other parts of the input.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index bb0d7b937a..afb12c11bc 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -24,6 +24,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
+		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 0c6f91c433..90d30037b7 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1275,4 +1275,43 @@ test_expect_success 'with cut line' '
 	test_cmp expected actual
 '
 
+test_expect_success 'only trailers' '
+	git config trailer.sign.command "echo config-value" &&
+	cat >expected <<-\EOF &&
+		existing: existing-value
+		sign: config-value
+		added: added-value
+	EOF
+	git interpret-trailers \
+		--trailer added:added-value \
+		--only-trailers >actual <<-\EOF &&
+		my subject
+
+		my body
+
+		existing: existing-value
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'only-trailers omits non-trailer in middle of block' '
+	git config trailer.sign.command "echo config-value" &&
+	cat >expected <<-\EOF &&
+		Signed-off-by: nobody <nobody@nowhere>
+		Signed-off-by: somebody <somebody@somewhere>
+		sign: config-value
+	EOF
+	git interpret-trailers --only-trailers >actual <<-\EOF &&
+		subject
+
+		it is important that the trailers below are signed-off-by
+		so that they meet the "25% trailers Git knows about" heuristic
+
+		Signed-off-by: nobody <nobody@nowhere>
+		this is not a trailer
+		Signed-off-by: somebody <somebody@somewhere>
+	EOF
+	test_cmp expected actual
+'
+
 test_done
diff --git a/trailer.c b/trailer.c
index e21a0d1629..706351fc98 100644
--- a/trailer.c
+++ b/trailer.c
@@ -164,13 +164,15 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
 		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
 }
 
-static void print_all(FILE *outfile, struct list_head *head, int trim_empty)
+static void print_all(FILE *outfile, struct list_head *head,
+		      const struct process_trailer_options *opts)
 {
 	struct list_head *pos;
 	struct trailer_item *item;
 	list_for_each(pos, head) {
 		item = list_entry(pos, struct trailer_item, list);
-		if (!trim_empty || strlen(item->value) > 0)
+		if ((!opts->trim_empty || strlen(item->value) > 0) &&
+		    (!opts->only_trailers || item->token))
 			print_tok_val(outfile, item->token, item->value);
 	}
 }
@@ -897,9 +899,10 @@ static int process_input_file(FILE *outfile,
 	trailer_info_get(&info, str);
 
 	/* Print lines before the trailers as is */
-	fwrite(str, 1, info.trailer_start - str, outfile);
+	if (outfile)
+		fwrite(str, 1, info.trailer_start - str, outfile);
 
-	if (!info.blank_line_before_trailer)
+	if (outfile && !info.blank_line_before_trailer)
 		fprintf(outfile, "\n");
 
 	for (i = 0; i < info.trailer_nr; i++) {
@@ -986,18 +989,20 @@ void process_trailers(const char *file,
 		outfile = create_in_place_tempfile(file);
 
 	/* Print the lines before the trailers */
-	trailer_end = process_input_file(outfile, sb.buf, &head);
+	trailer_end = process_input_file(opts->only_trailers ? NULL : outfile,
+					 sb.buf, &head);
 
 	process_command_line_args(&arg_head, trailers);
 
 	process_trailers_lists(&head, &arg_head);
 
-	print_all(outfile, &head, opts->trim_empty);
+	print_all(outfile, &head, opts);
 
 	free_all(&head);
 
 	/* Print the lines after the trailers as is */
-	fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
+	if (!opts->only_trailers)
+		fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
 
 	if (opts->in_place)
 		if (rename_tempfile(&trailers_tempfile, file))
diff --git a/trailer.h b/trailer.h
index 9da00bedec..3cf35ced00 100644
--- a/trailer.h
+++ b/trailer.h
@@ -25,6 +25,7 @@ struct trailer_info {
 struct process_trailer_options {
 	int in_place;
 	int trim_empty;
+	int only_trailers;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.14.0.614.g0beb26d5e9

[PATCH v3 3/5] interpret-trailers: add an option to show only existing trailers

[Jeff King]

It can be useful to invoke interpret-trailers for the
primary purpose of parsing existing trailers. But in that
case, we don't want to apply existing ifMissing or ifExists
rules from the config. Let's add a special mode where we
avoid applying those rules. Coupled with --only-trailers,
this gives us a reasonable parsing tool.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt |  5 +++++
 builtin/interpret-trailers.c             |  7 +++++++
 t/t7513-interpret-trailers.sh            | 16 ++++++++++++++++
 trailer.c                                |  9 +++++----
 trailer.h                                |  1 +
 5 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 295dffbd21..7cc43b0e3e 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -83,6 +83,11 @@ OPTIONS
 --only-trailers::
 	Output only the trailers, not any other parts of the input.
 
+--only-input::
+	Output only trailers that exist in the input; do not add any
+	from the command-line or by following configured `trailer.*`
+	rules.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index afb12c11bc..2d90e0e480 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -25,6 +25,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
+		OPT_BOOL(0, "only-input", &opts.only_input, N_("do not apply config rules")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
@@ -33,6 +34,12 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_interpret_trailers_usage, 0);
 
+	if (opts.only_input && trailers.nr)
+		usage_msg_opt(
+			_("--trailer with --only-input does not make sense"),
+			git_interpret_trailers_usage,
+			options);
+
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 90d30037b7..94b6c52473 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1314,4 +1314,20 @@ test_expect_success 'only-trailers omits non-trailer in middle of block' '
 	test_cmp expected actual
 '
 
+test_expect_success 'only input' '
+	git config trailer.sign.command "echo config-value" &&
+	cat >expected <<-\EOF &&
+		existing: existing-value
+	EOF
+	git interpret-trailers \
+		--only-trailers --only-input >actual <<-\EOF &&
+		my subject
+
+		my body
+
+		existing: existing-value
+	EOF
+	test_cmp expected actual
+'
+
 test_done
diff --git a/trailer.c b/trailer.c
index 706351fc98..2b5269183a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -976,7 +976,6 @@ void process_trailers(const char *file,
 		      struct string_list *trailers)
 {
 	LIST_HEAD(head);
-	LIST_HEAD(arg_head);
 	struct strbuf sb = STRBUF_INIT;
 	int trailer_end;
 	FILE *outfile = stdout;
@@ -992,9 +991,11 @@ void process_trailers(const char *file,
 	trailer_end = process_input_file(opts->only_trailers ? NULL : outfile,
 					 sb.buf, &head);
 
-	process_command_line_args(&arg_head, trailers);
-
-	process_trailers_lists(&head, &arg_head);
+	if (!opts->only_input) {
+		LIST_HEAD(arg_head);
+		process_command_line_args(&arg_head, trailers);
+		process_trailers_lists(&head, &arg_head);
+	}
 
 	print_all(outfile, &head, opts);
 
diff --git a/trailer.h b/trailer.h
index 3cf35ced00..76c3b571bf 100644
--- a/trailer.h
+++ b/trailer.h
@@ -26,6 +26,7 @@ struct process_trailer_options {
 	int in_place;
 	int trim_empty;
 	int only_trailers;
+	int only_input;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.14.0.614.g0beb26d5e9

[PATCH v3 4/5] interpret-trailers: add an option to normalize output

[Jeff King]

The point of "--only-trailers" is to give a caller an output
that's easy for them to parse. Getting rid of the
non-trailer material helps, but we still may see more
complicated syntax like whitespace continuation. Let's add
an option to normalize the output into one "key: value" line
per trailer.

As a bonus, this could be used even without --only-trailers
to clean up unusual formatting in the incoming data.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt |  5 +++++
 builtin/interpret-trailers.c             |  1 +
 t/t7513-interpret-trailers.sh            | 21 ++++++++++++++++++++
 trailer.c                                | 34 +++++++++++++++++++++++++++++++-
 trailer.h                                |  1 +
 5 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 7cc43b0e3e..598b74efd7 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -88,6 +88,11 @@ OPTIONS
 	from the command-line or by following configured `trailer.*`
 	rules.
 
+--normalize::
+	Normalize trailer output so that trailers appear exactly one per
+	line, with any existing whitespace continuation folded into a
+	single line.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 2d90e0e480..b7592bedd9 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -26,6 +26,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
 		OPT_BOOL(0, "only-input", &opts.only_input, N_("do not apply config rules")),
+		OPT_BOOL(0, "normalize", &opts.normalize, N_("normalize trailer formatting")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 94b6c52473..37c4f37882 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1330,4 +1330,25 @@ test_expect_success 'only input' '
 	test_cmp expected actual
 '
 
+test_expect_success 'normalize' '
+	cat >expected <<-\EOF &&
+		foo: continued across several lines
+	EOF
+	# pass through tr to make leading and trailing whitespace more obvious
+	tr _ " " <<-\EOF |
+		my subject
+
+		my body
+
+		foo:_
+		__continued
+		___across
+		____several
+		_____lines
+		___
+	EOF
+	git interpret-trailers --only-trailers --only-input --normalize >actual &&
+	test_cmp expected actual
+'
+
 test_done
diff --git a/trailer.c b/trailer.c
index 2b5269183a..07ef082284 100644
--- a/trailer.c
+++ b/trailer.c
@@ -887,7 +887,35 @@ static int ends_with_blank_line(const char *buf, size_t len)
 	return is_blank_line(buf + ll);
 }
 
+static void normalize_value(struct strbuf *val)
+{
+	struct strbuf out = STRBUF_INIT;
+	size_t i;
+
+	strbuf_grow(&out, val->len);
+	i = 0;
+	while (i < val->len) {
+		char c = val->buf[i++];
+		if (c == '\n') {
+			/* Collapse continuation down to a single space. */
+			while (i < val->len && isspace(val->buf[i]))
+				i++;
+			strbuf_addch(&out, ' ');
+		} else {
+			strbuf_addch(&out, c);
+		}
+	}
+
+	/* Empty lines may have left us with whitespace cruft at the edges */
+	strbuf_trim(&out);
+
+	/* output goes back to val as if we modified it in-place */
+	strbuf_swap(&out, val);
+	strbuf_release(&out);
+}
+
 static int process_input_file(FILE *outfile,
+			      int normalize,
 			      const char *str,
 			      struct list_head *head)
 {
@@ -914,12 +942,16 @@ static int process_input_file(FILE *outfile,
 		if (separator_pos >= 1) {
 			parse_trailer(&tok, &val, NULL, trailer,
 				      separator_pos);
+			if (normalize)
+				normalize_value(&val);
 			add_trailer_item(head,
 					 strbuf_detach(&tok, NULL),
 					 strbuf_detach(&val, NULL));
 		} else {
 			strbuf_addstr(&val, trailer);
 			strbuf_strip_suffix(&val, "\n");
+			if (normalize)
+				normalize_value(&val);
 			add_trailer_item(head,
 					 NULL,
 					 strbuf_detach(&val, NULL));
@@ -989,7 +1021,7 @@ void process_trailers(const char *file,
 
 	/* Print the lines before the trailers */
 	trailer_end = process_input_file(opts->only_trailers ? NULL : outfile,
-					 sb.buf, &head);
+					 opts->normalize, sb.buf, &head);
 
 	if (!opts->only_input) {
 		LIST_HEAD(arg_head);
diff --git a/trailer.h b/trailer.h
index 76c3b571bf..55bdec8320 100644
--- a/trailer.h
+++ b/trailer.h
@@ -27,6 +27,7 @@ struct process_trailer_options {
 	int trim_empty;
 	int only_trailers;
 	int only_input;
+	int normalize;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.14.0.614.g0beb26d5e9

[PATCH v3 5/5] interpret-trailers: add --parse convenience option

[Jeff King]

The last few commits have added command line options that
can turn interpret-trailers into a parsing tool. Since
they'd most often be used together, let's provide a
convenient single option for callers to invoke this mode.

This is implemented as a callback rather than a boolean so
that its effect is applied immediately, as if those options
had been specified. Later options can then override them.
E.g.:

  git interpret-trailers --parse --no-normalize

would work.

Let's also update the documentation to make clear that this
parsing mode behaves quite differently than the normal
"add trailers to the input" mode.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt | 21 ++++++++++++++-------
 builtin/interpret-trailers.c             | 12 ++++++++++++
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 598b74efd7..54c8b081c6 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -3,24 +3,27 @@ git-interpret-trailers(1)
 
 NAME
 ----
-git-interpret-trailers - help add structured information into commit messages
+git-interpret-trailers - add or parse structured information in commit messages
 
 SYNOPSIS
 --------
 [verse]
-'git interpret-trailers' [--in-place] [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]
+'git interpret-trailers' [options] [(--trailer <token>[(=|:)<value>])...] [<file>...]
+'git interpret-trailers' [options] [--parse] [<file>...]
 
 DESCRIPTION
 -----------
-Help adding 'trailers' lines, that look similar to RFC 822 e-mail
+Help parsing or adding 'trailers' lines, that look similar to RFC 822 e-mail
 headers, at the end of the otherwise free-form part of a commit
 message.
 
 This command reads some patches or commit messages from either the
-<file> arguments or the standard input if no <file> is specified. Then
-this command applies the arguments passed using the `--trailer`
-option, if any, to the commit message part of each input file. The
-result is emitted on the standard output.
+<file> arguments or the standard input if no <file> is specified. If
+`--parse` is specified, the output consists of the parsed trailers.
+
+Otherwise, the this command applies the arguments passed using the
+`--trailer` option, if any, to the commit message part of each input
+file. The result is emitted on the standard output.
 
 Some configuration variables control the way the `--trailer` arguments
 are applied to each commit message and the way any existing trailer in
@@ -93,6 +96,10 @@ OPTIONS
 	line, with any existing whitespace continuation folded into a
 	single line.
 
+--parse::
+	A convenience alias for `--only-trailers --only-input
+	--normalize`.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index b7592bedd9..010c181492 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -16,6 +16,16 @@ static const char * const git_interpret_trailers_usage[] = {
 	NULL
 };
 
+static int parse_opt_parse(const struct option *opt, const char *arg,
+			   int unset)
+{
+	struct process_trailer_options *v = opt->value;
+	v->only_trailers = 1;
+	v->only_input = 1;
+	v->normalize = 1;
+	return 0;
+}
+
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
 	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
@@ -27,6 +37,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
 		OPT_BOOL(0, "only-input", &opts.only_input, N_("do not apply config rules")),
 		OPT_BOOL(0, "normalize", &opts.normalize, N_("normalize trailer formatting")),
+		{ OPTION_CALLBACK, 0, "parse", &opts, NULL, N_("set parsing options"),
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse },
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
-- 
2.14.0.614.g0beb26d5e9

Re: [PATCH v3 2/5] interpret-trailers: add an option to show only the trailers

[Stefan Beller]

On Thu, Aug 10, 2017 at 11:04 AM, Jeff King <peff@peff.net> wrote:
> In theory it's easy for any reader who wants to parse
> trailers to do so. But there are a lot of subtle corner
> cases around what counts as a trailer, when the trailer
> block begins and ends, etc. Since interpret-trailers already
> has our parsing logic, let's let callers ask it to just
> output the trailers.
>
> They still have to parse the "key: value" lines, but at
> least they can ignore all of the other corner cases.
>
> Signed-off-by: Jeff King <peff@peff.net>

Sorry for a sloppy review last round, upon reviewing
I found another nit.

>
> +test_expect_success 'only trailers' '
> +       git config trailer.sign.command "echo config-value" &&

You may want to use 'test_config' here, which keeps the config
only for one test. The subsequent tests seem to overwrite the
config, so this is not wrong, just not the best style.

Re: [PATCH v3 2/5] interpret-trailers: add an option to show only the trailers

[Jeff King]

On Thu, Aug 10, 2017 at 11:28:52AM -0700, Stefan Beller wrote:

> > +test_expect_success 'only trailers' '
> > +       git config trailer.sign.command "echo config-value" &&
> 
> You may want to use 'test_config' here, which keeps the config
> only for one test. The subsequent tests seem to overwrite the
> config, so this is not wrong, just not the best style.

Yeah, I actually considered that but decided to keep style with the rest
of the script. I agree the whole thing could possibly switch to
test_config, but I suspect there may be some fallouts (the style of the
rest of the script seems to assume some continuity between the tests).

-Peff

Re: [PATCH 4/5] interpret-trailers: add an option to normalize output

[Stefan Beller]

On Thu, Aug 10, 2017 at 1:03 AM, Jeff King <peff@peff.net> wrote:
> The point of "--only-trailers" is to give a caller an output
> that's easy for them to parse. Getting rid of the
> non-trailer material helps, but we still may see more
> complicated syntax like whitespace continuation. Let's add
> an option to normalize the output into one "key: value" line
> per trailer.
>
> As a bonus, this could be used even without --only-trailers
> to clean up unusual formatting in the incoming data.

This is useful for the parsing part, but for the writing part we'd
rather want to have the opposite thing, such as
'--line-break=rfc822'. But this doesn't have to be part of this
series. With this in mind, I do not quite understand the latter
use case how you would use normalized trailers without
--only-trailers?

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Stefan Beller]

On Thu, Aug 10, 2017 at 11:03 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Aug 10, 2017 at 04:02:46AM -0400, Jeff King wrote:
>
>> On Wed, Aug 09, 2017 at 08:21:47AM -0400, Jeff King wrote:
>>
>> > This series teaches interpret-trailers to parse and output just the
>> > trailers. So now you can do:
>> >
>> >   $ git log --format=%B -1 8d44797cc91231cd44955279040dc4a1ee0a797f |
>> >     git interpret-trailers --parse
>> >   Signed-off-by: Hartmut Henkel <henkel@vh-s.de>
>> >   Helped-by: Stefan Beller <sbeller@google.com>
>> >   Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
>> >   Acked-by: Matthias Rüster <matthias.ruester@gmail.com>
>>
>> And here's a v2 that addresses all of the comments except one: Stefan
>> suggested that --only-existing wasn't a great name. I agree, but I like
>> everything else less.
>
> Here's a v3 that takes care of that (renaming it to --only-input).
>
> It's otherwise the same as v2, but since the name-change ripples through
> the remaining patches, I wanted to get v3 in front of people sooner
> rather than later.
>

Looks good,

Thanks,
Stefan

Re: [PATCH 4/5] interpret-trailers: add an option to normalize output

[Jeff King]

On Thu, Aug 10, 2017 at 11:35:00AM -0700, Stefan Beller wrote:

> On Thu, Aug 10, 2017 at 1:03 AM, Jeff King <peff@peff.net> wrote:
> > The point of "--only-trailers" is to give a caller an output
> > that's easy for them to parse. Getting rid of the
> > non-trailer material helps, but we still may see more
> > complicated syntax like whitespace continuation. Let's add
> > an option to normalize the output into one "key: value" line
> > per trailer.
> >
> > As a bonus, this could be used even without --only-trailers
> > to clean up unusual formatting in the incoming data.
> 
> This is useful for the parsing part, but for the writing part we'd
> rather want to have the opposite thing, such as
> '--line-break=rfc822'. But this doesn't have to be part of this
> series. With this in mind, I do not quite understand the latter
> use case how you would use normalized trailers without
> --only-trailers?

If you prefer the normalized form (and the input was line-broken in a
way that you don't like), then this would convert to your preferred
form. I agree that you could potentially want the opposite (folding long
lines). Perhaps something like --wrap=72.

-Peff

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > The above example made me wonder if we also want a format specifier
>> > to do the above without piping, but it turns out that we already
>> > have "log --format=%(trailers)", so we are good ;-)
>> 
>> I was going to say, I thought we had a way to get trailers for a
>> commit via the pretty format, since that is what i used in the past.
>
> I do like that you could get the trailers for many commits in a single
> invocation. That doesn't matter for my current use-case, but obviously
> piping through O(n) interpret-trailers invocations is a bad idea.
> But there are a few difficulties with using %(trailers) for this,...

I think it is clear to you, but it may not be clear to others, that
I did not mean to say "because 'log --format' already knows about
it, this change to interpret-trailers is unnecessary".

> For (1) I think many callers would prefer to see the original
> formatting. Maybe we'd need a %(trailers:normalize) or something.

Thanks; that is exactly the line of thought I had in the back of my
head without even realizing when I brought up %(trailers) format
element.

Re: [PATCH 3/5] interpret-trailers: add an option to show only existing trailers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Perhaps "--exact-input" hits all of those. Or maybe "--only-input" to
> match the other "--only".
>
> I think I like that last one the best. It makes it clear that we are
> looking just at the input, and not anything else. Which is exactly what
> the feature does.

Sounds good.  Thanks.

Re: [PATCH v3 2/5] interpret-trailers: add an option to show only the trailers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Aug 10, 2017 at 11:28:52AM -0700, Stefan Beller wrote:
>
>> > +test_expect_success 'only trailers' '
>> > +       git config trailer.sign.command "echo config-value" &&
>> 
>> You may want to use 'test_config' here, which keeps the config
>> only for one test. The subsequent tests seem to overwrite the
>> config, so this is not wrong, just not the best style.
>
> Yeah, I actually considered that but decided to keep style with the rest
> of the script. I agree the whole thing could possibly switch to
> test_config, but I suspect there may be some fallouts (the style of the
> rest of the script seems to assume some continuity between the tests).

I agree with your judgment here.  As you said in the "use tool to
enforce consistent style" thread, sometimes humans need to apply
better taste than mechanical tooling could, and I view this one of a
good example.  

Even though this is not a C coding-style thing, but the essense of
the problem is the same.  That is one of the reasons why I earlier
said that we may see more style-only critique to patches if we
introduce a new tool without setting the expectation of what we want
to get out of such a tool straight.

Re: [PATCH 4/5] interpret-trailers: add an option to normalize output

[Christian Couder]

On Thu, Aug 10, 2017 at 8:37 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Aug 10, 2017 at 11:35:00AM -0700, Stefan Beller wrote:
>
>> On Thu, Aug 10, 2017 at 1:03 AM, Jeff King <peff@peff.net> wrote:
>> > The point of "--only-trailers" is to give a caller an output
>> > that's easy for them to parse. Getting rid of the
>> > non-trailer material helps, but we still may see more
>> > complicated syntax like whitespace continuation. Let's add
>> > an option to normalize the output into one "key: value" line
>> > per trailer.
>> >
>> > As a bonus, this could be used even without --only-trailers
>> > to clean up unusual formatting in the incoming data.
>>
>> This is useful for the parsing part, but for the writing part we'd
>> rather want to have the opposite thing, such as
>> '--line-break=rfc822'. But this doesn't have to be part of this
>> series. With this in mind, I do not quite understand the latter
>> use case how you would use normalized trailers without
>> --only-trailers?
>
> If you prefer the normalized form (and the input was line-broken in a
> way that you don't like), then this would convert to your preferred
> form. I agree that you could potentially want the opposite (folding long
> lines). Perhaps something like --wrap=72.

Related to this, I wonder if people might want to "normalize" in
different ways later. If that happens, we might regret having called
this option "--normalize" instead of "--one-per-line" for example.

Re: [PATCH 4/5] interpret-trailers: add an option to normalize output

[Jeff King]

On Thu, Aug 10, 2017 at 09:39:21PM +0200, Christian Couder wrote:

> > If you prefer the normalized form (and the input was line-broken in a
> > way that you don't like), then this would convert to your preferred
> > form. I agree that you could potentially want the opposite (folding long
> > lines). Perhaps something like --wrap=72.
> 
> Related to this, I wonder if people might want to "normalize" in
> different ways later. If that happens, we might regret having called
> this option "--normalize" instead of "--one-per-line" for example.

My assumption was that it would be OK to add other normalization later
if it brings us closer to the "key: value" form as a standard, and it
could fall under "--normalize", since that's what callers would want.
And that's why I didn't want to call it something like --one-per-line.

But if you are arguing that there can be many "standards" to normalize
to, I agree that's a possibility. I think we have an out by extending to
"--normalize=whatever-form" in the future.

-Peff

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Thu, Aug 10, 2017 at 11:03 AM, Jeff King <peff@peff.net> wrote:
>> On Thu, Aug 10, 2017 at 04:02:46AM -0400, Jeff King wrote:
>>
>>> On Wed, Aug 09, 2017 at 08:21:47AM -0400, Jeff King wrote:
>>>
>>> > This series teaches interpret-trailers to parse and output just the
>>> > trailers. So now you can do:
>>> >
>>> >   $ git log --format=%B -1 8d44797cc91231cd44955279040dc4a1ee0a797f |
>>> >     git interpret-trailers --parse
>>> >   Signed-off-by: Hartmut Henkel <henkel@vh-s.de>
>>> >   Helped-by: Stefan Beller <sbeller@google.com>
>>> >   Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
>>> >   Acked-by: Matthias Rüster <matthias.ruester@gmail.com>
>>>
>>> And here's a v2 that addresses all of the comments except one: Stefan
>>> suggested that --only-existing wasn't a great name. I agree, but I like
>>> everything else less.
>>
>> Here's a v3 that takes care of that (renaming it to --only-input).
>>
>> It's otherwise the same as v2, but since the name-change ripples through
>> the remaining patches, I wanted to get v3 in front of people sooner
>> rather than later.
>>
>
> Looks good,

Yeah, looks good to me too.  Thanks for the "--only-input"
discussion.

Re: [PATCH 4/5] interpret-trailers: add an option to normalize output

[Stefan Beller]

On Thu, Aug 10, 2017 at 12:39 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Thu, Aug 10, 2017 at 8:37 PM, Jeff King <peff@peff.net> wrote:
>> On Thu, Aug 10, 2017 at 11:35:00AM -0700, Stefan Beller wrote:
>>
>>> On Thu, Aug 10, 2017 at 1:03 AM, Jeff King <peff@peff.net> wrote:
>>> > The point of "--only-trailers" is to give a caller an output
>>> > that's easy for them to parse. Getting rid of the
>>> > non-trailer material helps, but we still may see more
>>> > complicated syntax like whitespace continuation. Let's add
>>> > an option to normalize the output into one "key: value" line
>>> > per trailer.
>>> >
>>> > As a bonus, this could be used even without --only-trailers
>>> > to clean up unusual formatting in the incoming data.
>>>
>>> This is useful for the parsing part, but for the writing part we'd
>>> rather want to have the opposite thing, such as
>>> '--line-break=rfc822'. But this doesn't have to be part of this
>>> series. With this in mind, I do not quite understand the latter
>>> use case how you would use normalized trailers without
>>> --only-trailers?
>>
>> If you prefer the normalized form (and the input was line-broken in a
>> way that you don't like), then this would convert to your preferred
>> form. I agree that you could potentially want the opposite (folding long
>> lines). Perhaps something like --wrap=72.
>
> Related to this, I wonder if people might want to "normalize" in
> different ways later. If that happens, we might regret having called
> this option "--normalize" instead of "--one-per-line" for example.

What is normal?

Maybe --style=[one-line, wrapped:72, rfc, json, xml, ...]
then?

If you have --one-per-line, this may be orthogonal to e.g. json
(as json can be crammed into one line IIUC), but when given the
selection you cannot combine multiple styles.

Scratch that, we actually want to combine these styles with each
other.

Re: [PATCH 4/5] interpret-trailers: add an option to normalize output

[Christian Couder]

On Thu, Aug 10, 2017 at 9:42 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Aug 10, 2017 at 09:39:21PM +0200, Christian Couder wrote:
>
>> > If you prefer the normalized form (and the input was line-broken in a
>> > way that you don't like), then this would convert to your preferred
>> > form. I agree that you could potentially want the opposite (folding long
>> > lines). Perhaps something like --wrap=72.
>>
>> Related to this, I wonder if people might want to "normalize" in
>> different ways later. If that happens, we might regret having called
>> this option "--normalize" instead of "--one-per-line" for example.
>
> My assumption was that it would be OK to add other normalization later
> if it brings us closer to the "key: value" form as a standard, and it
> could fall under "--normalize", since that's what callers would want.
> And that's why I didn't want to call it something like --one-per-line.
>
> But if you are arguing that there can be many "standards" to normalize
> to, I agree that's a possibility. I think we have an out by extending to
> "--normalize=whatever-form" in the future.

If we take `git log` as an example, we now have "--oneline" which is a
shorthand for "--pretty=oneline --abbrev-commit".
And the default for "--pretty" is called "medium".

So instead of your suggestion, we could call this option "--oneline"
now, and if other normalizations are later required we could then
create "--pretty=whatever" and say that "--oneline" is a shorthand for
"--pretty=oneline".

Re: [PATCH 4/5] interpret-trailers: add an option to normalize output

[Christian Couder]

On Thu, Aug 10, 2017 at 9:44 PM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Aug 10, 2017 at 12:39 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> On Thu, Aug 10, 2017 at 8:37 PM, Jeff King <peff@peff.net> wrote:
>>> On Thu, Aug 10, 2017 at 11:35:00AM -0700, Stefan Beller wrote:
>>>
>>>> On Thu, Aug 10, 2017 at 1:03 AM, Jeff King <peff@peff.net> wrote:
>>>> > The point of "--only-trailers" is to give a caller an output
>>>> > that's easy for them to parse. Getting rid of the
>>>> > non-trailer material helps, but we still may see more
>>>> > complicated syntax like whitespace continuation. Let's add
>>>> > an option to normalize the output into one "key: value" line
>>>> > per trailer.
>>>> >
>>>> > As a bonus, this could be used even without --only-trailers
>>>> > to clean up unusual formatting in the incoming data.
>>>>
>>>> This is useful for the parsing part, but for the writing part we'd
>>>> rather want to have the opposite thing, such as
>>>> '--line-break=rfc822'. But this doesn't have to be part of this
>>>> series. With this in mind, I do not quite understand the latter
>>>> use case how you would use normalized trailers without
>>>> --only-trailers?
>>>
>>> If you prefer the normalized form (and the input was line-broken in a
>>> way that you don't like), then this would convert to your preferred
>>> form. I agree that you could potentially want the opposite (folding long
>>> lines). Perhaps something like --wrap=72.
>>
>> Related to this, I wonder if people might want to "normalize" in
>> different ways later. If that happens, we might regret having called
>> this option "--normalize" instead of "--one-per-line" for example.
>
> What is normal?
>
> Maybe --style=[one-line, wrapped:72, rfc, json, xml, ...]
> then?

Yeah, we could go there right now (using perhaps "--pretty" or
"--format" instead of "--style", so that we are more consistent with
other commands), but on the other hand we don't know yet if the other
formats will really be needed.

> If you have --one-per-line, this may be orthogonal to e.g. json
> (as json can be crammed into one line IIUC), but when given the
> selection you cannot combine multiple styles.
>
> Scratch that, we actually want to combine these styles with each
> other.

Yeah, that's another possibility for the future. People might want a
--json option that can be used both with and without --oneline. But as
the future is difficult to predict, let's try to make it easy for us
in both cases.

And I think starting with just "--oneline" would be easier to deal
with later than "--normalize" (or "--style" or "--pretty" or
"--format") especially in the latter case.

Re: [PATCH 4/5] interpret-trailers: add an option to normalize output

[Jeff King]

On Thu, Aug 10, 2017 at 11:06:40PM +0200, Christian Couder wrote:

> >> Related to this, I wonder if people might want to "normalize" in
> >> different ways later. If that happens, we might regret having called
> >> this option "--normalize" instead of "--one-per-line" for example.
> >
> > What is normal?
> >
> > Maybe --style=[one-line, wrapped:72, rfc, json, xml, ...]
> > then?
> 
> Yeah, we could go there right now (using perhaps "--pretty" or
> "--format" instead of "--style", so that we are more consistent with
> other commands), but on the other hand we don't know yet if the other
> formats will really be needed.

But some of those things are not 1:1 mappings with normalization.  For
instance, --json presumably implies --only-trailers. Or are we proposing
to break the whole commit message down into components and output it all
as json?

-Peff

Re: [PATCH 4/5] interpret-trailers: add an option to normalize output

[Ramsay Jones]

On 10/08/17 22:10, Jeff King wrote:
> On Thu, Aug 10, 2017 at 11:06:40PM +0200, Christian Couder wrote:
> 
>>>> Related to this, I wonder if people might want to "normalize" in
>>>> different ways later. If that happens, we might regret having called
>>>> this option "--normalize" instead of "--one-per-line" for example.
>>>
>>> What is normal?
>>>
>>> Maybe --style=[one-line, wrapped:72, rfc, json, xml, ...]
>>> then?
>>
>> Yeah, we could go there right now (using perhaps "--pretty" or
>> "--format" instead of "--style", so that we are more consistent with
>> other commands), but on the other hand we don't know yet if the other
>> formats will really be needed.
> 
> But some of those things are not 1:1 mappings with normalization.  For
> instance, --json presumably implies --only-trailers. Or are we proposing
> to break the whole commit message down into components and output it all
> as json?

Hmm, to me the operation wasn't so much a normalization, rather
it was an --unfold (or maybe --un-fold ;-D). I suppose going in
the other direction could be --fold=72, or some such.

[blue is my favourite colour ... :-P ]

ATB,
Ramsay Jones

Re: [PATCH 4/5] interpret-trailers: add an option to normalize output

[Jeff King]

On Fri, Aug 11, 2017 at 12:02:49AM +0100, Ramsay Jones wrote:

> > But some of those things are not 1:1 mappings with normalization.  For
> > instance, --json presumably implies --only-trailers. Or are we proposing
> > to break the whole commit message down into components and output it all
> > as json?
> 
> Hmm, to me the operation wasn't so much a normalization, rather
> it was an --unfold (or maybe --un-fold ;-D). I suppose going in
> the other direction could be --fold=72, or some such.

But I really don't want callers to think of it as "unfold". I want it to
be "turn this into something I can parse simply". Hence if we were to
find another case where the output is irregular, I'd feel comfortable
calling that a bug and fixing it (one that I suspect but haven't tested
is that alternate separators probably should all be converted to
colons).

> [blue is my favourite colour ... :-P ]

Yes, I'm feeling that, too. :-/

-Peff

Re: [PATCH 4/5] interpret-trailers: add an option to normalize output

[Ramsay Jones]

On 11/08/17 00:10, Jeff King wrote:
> On Fri, Aug 11, 2017 at 12:02:49AM +0100, Ramsay Jones wrote:
> 
>>> But some of those things are not 1:1 mappings with normalization.  For
>>> instance, --json presumably implies --only-trailers. Or are we proposing
>>> to break the whole commit message down into components and output it all
>>> as json?
>>
>> Hmm, to me the operation wasn't so much a normalization, rather
>> it was an --unfold (or maybe --un-fold ;-D). I suppose going in
>> the other direction could be --fold=72, or some such.
> 
> But I really don't want callers to think of it as "unfold". I want it to
> be "turn this into something I can parse simply". Hence if we were to
> find another case where the output is irregular, I'd feel comfortable
> calling that a bug and fixing it (one that I suspect but haven't tested
> is that alternate separators probably should all be converted to
> colons).

Ah, yes, good point.

ATB,
Ramsay Jones

Re: [PATCH 4/5] interpret-trailers: add an option to normalize output

[Christian Couder]

On Fri, Aug 11, 2017 at 1:10 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Aug 11, 2017 at 12:02:49AM +0100, Ramsay Jones wrote:
>
>> > But some of those things are not 1:1 mappings with normalization.  For
>> > instance, --json presumably implies --only-trailers. Or are we proposing
>> > to break the whole commit message down into components and output it all
>> > as json?

Well who knows what people might want/need?
Anyway in `git log` --oneline is not a direct mapping with
--pretty=oneline as it also means --abbrev-commit, and this is not a
big problem.

>> Hmm, to me the operation wasn't so much a normalization, rather
>> it was an --unfold (or maybe --un-fold ;-D). I suppose going in
>> the other direction could be --fold=72, or some such.

Yeah, we could call that --no-fold or --no-wrap if we expect to need
--fold=72 or --wrap=72.
At least it is more descriptive than --normalize and if we later
introduce --pretty or --format we can say that it is a shorthand for
--pretty=nofold or --pretty=unfolded.

> But I really don't want callers to think of it as "unfold". I want it to
> be "turn this into something I can parse simply". Hence if we were to
> find another case where the output is irregular, I'd feel comfortable
> calling that a bug and fixing it (one that I suspect but haven't tested
> is that alternate separators probably should all be converted to
> colons).

Though "fixing" this after a release has been made might introduce a
regression for people who would already use the option on trailers
with different separators. That's also why I don't like --normalize.
We just don't know if we will need to "fix" it a lot to make sure
scripts using it will work in all cases.

If we use --no-fold or --oneline instead, we don't promise too much
from this option, so users cannot say that they expect it to work for
them in all cases.

Re: [PATCH 4/5] interpret-trailers: add an option to normalize output

[Jeff King]

On Fri, Aug 11, 2017 at 09:02:24AM +0200, Christian Couder wrote:

> > But I really don't want callers to think of it as "unfold". I want it to
> > be "turn this into something I can parse simply". Hence if we were to
> > find another case where the output is irregular, I'd feel comfortable
> > calling that a bug and fixing it (one that I suspect but haven't tested
> > is that alternate separators probably should all be converted to
> > colons).
> 
> Though "fixing" this after a release has been made might introduce a
> regression for people who would already use the option on trailers
> with different separators. That's also why I don't like --normalize.
> We just don't know if we will need to "fix" it a lot to make sure
> scripts using it will work in all cases.
> 
> If we use --no-fold or --oneline instead, we don't promise too much
> from this option, so users cannot say that they expect it to work for
> them in all cases.

But promising a normalized form is exactly what I want from the option.

That said, I'm OK to promise that via "--parse", and call this --unfold,
if you really feel strongly.

-Peff

Re: [PATCH 4/5] interpret-trailers: add an option to normalize output

[Christian Couder]

On Fri, Aug 11, 2017 at 11:06 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Aug 11, 2017 at 09:02:24AM +0200, Christian Couder wrote:
>
>> > But I really don't want callers to think of it as "unfold". I want it to
>> > be "turn this into something I can parse simply". Hence if we were to
>> > find another case where the output is irregular, I'd feel comfortable
>> > calling that a bug and fixing it (one that I suspect but haven't tested
>> > is that alternate separators probably should all be converted to
>> > colons).
>>
>> Though "fixing" this after a release has been made might introduce a
>> regression for people who would already use the option on trailers
>> with different separators. That's also why I don't like --normalize.
>> We just don't know if we will need to "fix" it a lot to make sure
>> scripts using it will work in all cases.
>>
>> If we use --no-fold or --oneline instead, we don't promise too much
>> from this option, so users cannot say that they expect it to work for
>> them in all cases.
>
> But promising a normalized form is exactly what I want from the option.
>
> That said, I'm OK to promise that via "--parse", and call this --unfold,
> if you really feel strongly.

Yeah, I think promising these kind of things via an higher level
option that is a shorthand for a mix of other basic options is much
better especially if it's clearly documented that the option mix could
change in case of bugs or improvements.

This way people who want something stable, know that they should use
their own mix of basic options. And those who are ok with something
not as stable as long as it evolves towards a specific goal, know that
they should use the higher level option.

Re: [PATCH 0/5] make interpret-trailers useful for parsing

[Jacob Keller]

On Thu, Aug 10, 2017 at 11:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>>> > The above example made me wonder if we also want a format specifier
>>> > to do the above without piping, but it turns out that we already
>>> > have "log --format=%(trailers)", so we are good ;-)
>>>
>>> I was going to say, I thought we had a way to get trailers for a
>>> commit via the pretty format, since that is what i used in the past.
>>
>> I do like that you could get the trailers for many commits in a single
>> invocation. That doesn't matter for my current use-case, but obviously
>> piping through O(n) interpret-trailers invocations is a bad idea.
>> But there are a few difficulties with using %(trailers) for this,...
>
> I think it is clear to you, but it may not be clear to others, that
> I did not mean to say "because 'log --format' already knows about
> it, this change to interpret-trailers is unnecessary".
>
>> For (1) I think many callers would prefer to see the original
>> formatting. Maybe we'd need a %(trailers:normalize) or something.
>
> Thanks; that is exactly the line of thought I had in the back of my
> head without even realizing when I brought up %(trailers) format
> element.

I'll add that I also think this patch series is good, it's useful to
have a separate command.

Thanks,
Jake

[PATCH v4 0/8] trailer parsing via interpret-trailers and %(trailers)

[Jeff King]

Here's a fourth version of my series to make the output of
interpret-trailers useful for callers who just want to parse existing
trailers.

The big change here is that I added similar features to pretty.c's
"%(trailers)", meaning you can now efficiently get the same output for
multiple commits with a single invocation. Like:

  $ git rev-list --no-walk --format='%(trailers:only:unfold)' \
    3ac53e0d13fa7483cce90eb6a1cfcdcbda5b8e35
  commit 3ac53e0d13fa7483cce90eb6a1cfcdcbda5b8e35
  Signed-off-by: H. Peter Anvin <hpa@zytor.com>
  Signed-off-by: Junio C Hamano <junkio@cox.net>

That doesn't look very exciting, but with just "%(trailers)" you would
see that the block also contains a "cherry-picked from" line. There
aren't good examples of unfolding in git.git because we don't tend to
fold. Nor does linux.git (though they do have a lot of non-trailer lines
in square brackets). The tests show off some samples of both.

I left %(trailers) exactly as it is: it shows the trailer block
verbatim, including funny syntax, etc. As soon as you use one of :only
or :unfold, we have to parse further, and as a side effect we normalize
a few bits (like whitespace around separators), because we round-trip
through the parser. I think that's fine and what callers would want. But
it did make me wonder if I should just have a single "parse" option that
does all that normalizing. Or alternatively, have %(trailers) always do
that round-trip normalizing. That's how interpret-trailers behaves,
after all(since it always parses and reconstructs the trailer block). I
felt better taking the conservative route, though, and leaving
%(trailers) alone.

There are a few other changes from v3:

  - the --normalize option is now --unfold (and I matched it with
    ":unfold" for the placeholder)

  - the process_input_file function now takes the
    process_trailer_options to check "only_trailers" instead of taking a
    hint from a NULL outfile. This is hopefully a bit more obvious.

  - when only_trailers is set, I don't even bother adding non-trailers
    from the middle of the trailer-block to the list (we know we'd never
    output them, and they can't affect the trailer rules). This is just
    a micro-optimization I noticed while writing the %(trailers) helper.

  - Similarly, we no longer unfold non-trailers. So if you have:

      key: value
        more value
      this is not a trailer
        this is also not a trailer

    we would unfold "key: value more value", but not the other two
    lines. Because without a separator, that's not really folding. I'm
    not sure it actually matters. It's hard to have non-trailers in the
    block in the first place. But I think the new behavior is the right
    thing if it ever does come up.

  [1/8]: trailer: put process_trailers() options into a struct
  [2/8]: interpret-trailers: add an option to show only the trailers
  [3/8]: interpret-trailers: add an option to show only existing trailers
  [4/8]: interpret-trailers: add an option to unfold values
  [5/8]: interpret-trailers: add --parse convenience option
  [6/8]: pretty: move trailer formatting to trailer.c
  [7/8]: t4205: refactor %(trailer) tests
  [8/8]: pretty: support normalization options for %(trailers)

 Documentation/git-interpret-trailers.txt |  33 +++++++--
 Documentation/pretty-formats.txt         |   5 +-
 builtin/interpret-trailers.c             |  34 +++++++--
 pretty.c                                 |  26 +++----
 t/t4205-log-pretty-formats.sh            |  51 ++++++++++++--
 t/t7513-interpret-trailers.sh            |  76 ++++++++++++++++++++
 trailer.c                                | 115 ++++++++++++++++++++++++++-----
 trailer.h                                |  27 +++++++-
 8 files changed, 315 insertions(+), 52 deletions(-)

-Peff

[PATCH v4 1/8] trailer: put process_trailers() options into a struct

[Jeff King]

We already have two options and are about to add a few more.
To avoid having a huge number of boolean arguments, let's
convert to an options struct which can be passed in.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/interpret-trailers.c | 13 ++++++-------
 trailer.c                    | 10 ++++++----
 trailer.h                    | 10 +++++++++-
 3 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 175f14797b..bb0d7b937a 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -18,13 +18,12 @@ static const char * const git_interpret_trailers_usage[] = {
 
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
-	int in_place = 0;
-	int trim_empty = 0;
+	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
 	struct string_list trailers = STRING_LIST_INIT_NODUP;
 
 	struct option options[] = {
-		OPT_BOOL(0, "in-place", &in_place, N_("edit files in place")),
-		OPT_BOOL(0, "trim-empty", &trim_empty, N_("trim empty trailers")),
+		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
+		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
@@ -36,11 +35,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
-			process_trailers(argv[i], in_place, trim_empty, &trailers);
+			process_trailers(argv[i], &opts, &trailers);
 	} else {
-		if (in_place)
+		if (opts.in_place)
 			die(_("no input file given for in-place editing"));
-		process_trailers(NULL, in_place, trim_empty, &trailers);
+		process_trailers(NULL, &opts, &trailers);
 	}
 
 	string_list_clear(&trailers, 0);
diff --git a/trailer.c b/trailer.c
index 751b56c009..e21a0d1629 100644
--- a/trailer.c
+++ b/trailer.c
@@ -968,7 +968,9 @@ static FILE *create_in_place_tempfile(const char *file)
 	return outfile;
 }
 
-void process_trailers(const char *file, int in_place, int trim_empty, struct string_list *trailers)
+void process_trailers(const char *file,
+		      const struct process_trailer_options *opts,
+		      struct string_list *trailers)
 {
 	LIST_HEAD(head);
 	LIST_HEAD(arg_head);
@@ -980,7 +982,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 
 	read_input_file(&sb, file);
 
-	if (in_place)
+	if (opts->in_place)
 		outfile = create_in_place_tempfile(file);
 
 	/* Print the lines before the trailers */
@@ -990,14 +992,14 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 
 	process_trailers_lists(&head, &arg_head);
 
-	print_all(outfile, &head, trim_empty);
+	print_all(outfile, &head, opts->trim_empty);
 
 	free_all(&head);
 
 	/* Print the lines after the trailers as is */
 	fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
 
-	if (in_place)
+	if (opts->in_place)
 		if (rename_tempfile(&trailers_tempfile, file))
 			die_errno(_("could not rename temporary file to %s"), file);
 
diff --git a/trailer.h b/trailer.h
index 65cc5d79c6..9da00bedec 100644
--- a/trailer.h
+++ b/trailer.h
@@ -22,7 +22,15 @@ struct trailer_info {
 	size_t trailer_nr;
 };
 
-void process_trailers(const char *file, int in_place, int trim_empty,
+struct process_trailer_options {
+	int in_place;
+	int trim_empty;
+};
+
+#define PROCESS_TRAILER_OPTIONS_INIT {0}
+
+void process_trailers(const char *file,
+		      const struct process_trailer_options *opts,
 		      struct string_list *trailers);
 
 void trailer_info_get(struct trailer_info *info, const char *str);
-- 
2.14.1.352.ge5efb0d3f3

[PATCH v4 2/8] interpret-trailers: add an option to show only the trailers

[Jeff King]

In theory it's easy for any reader who wants to parse
trailers to do so. But there are a lot of subtle corner
cases around what counts as a trailer, when the trailer
block begins and ends, etc. Since interpret-trailers already
has our parsing logic, let's let callers ask it to just
output the trailers.

They still have to parse the "key: value" lines, but at
least they can ignore all of the other corner cases.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt |  3 +++
 builtin/interpret-trailers.c             |  1 +
 t/t7513-interpret-trailers.sh            | 39 ++++++++++++++++++++++++++++++++
 trailer.c                                | 23 +++++++++++--------
 trailer.h                                |  1 +
 5 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 31cdeaecdf..295dffbd21 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -80,6 +80,9 @@ OPTIONS
 	trailer to the input messages. See the description of this
 	command.
 
+--only-trailers::
+	Output only the trailers, not any other parts of the input.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index bb0d7b937a..afb12c11bc 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -24,6 +24,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
+		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 0c6f91c433..90d30037b7 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1275,4 +1275,43 @@ test_expect_success 'with cut line' '
 	test_cmp expected actual
 '
 
+test_expect_success 'only trailers' '
+	git config trailer.sign.command "echo config-value" &&
+	cat >expected <<-\EOF &&
+		existing: existing-value
+		sign: config-value
+		added: added-value
+	EOF
+	git interpret-trailers \
+		--trailer added:added-value \
+		--only-trailers >actual <<-\EOF &&
+		my subject
+
+		my body
+
+		existing: existing-value
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'only-trailers omits non-trailer in middle of block' '
+	git config trailer.sign.command "echo config-value" &&
+	cat >expected <<-\EOF &&
+		Signed-off-by: nobody <nobody@nowhere>
+		Signed-off-by: somebody <somebody@somewhere>
+		sign: config-value
+	EOF
+	git interpret-trailers --only-trailers >actual <<-\EOF &&
+		subject
+
+		it is important that the trailers below are signed-off-by
+		so that they meet the "25% trailers Git knows about" heuristic
+
+		Signed-off-by: nobody <nobody@nowhere>
+		this is not a trailer
+		Signed-off-by: somebody <somebody@somewhere>
+	EOF
+	test_cmp expected actual
+'
+
 test_done
diff --git a/trailer.c b/trailer.c
index e21a0d1629..22146002e9 100644
--- a/trailer.c
+++ b/trailer.c
@@ -164,13 +164,15 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
 		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
 }
 
-static void print_all(FILE *outfile, struct list_head *head, int trim_empty)
+static void print_all(FILE *outfile, struct list_head *head,
+		      const struct process_trailer_options *opts)
 {
 	struct list_head *pos;
 	struct trailer_item *item;
 	list_for_each(pos, head) {
 		item = list_entry(pos, struct trailer_item, list);
-		if (!trim_empty || strlen(item->value) > 0)
+		if ((!opts->trim_empty || strlen(item->value) > 0) &&
+		    (!opts->only_trailers || item->token))
 			print_tok_val(outfile, item->token, item->value);
 	}
 }
@@ -887,7 +889,8 @@ static int ends_with_blank_line(const char *buf, size_t len)
 
 static int process_input_file(FILE *outfile,
 			      const char *str,
-			      struct list_head *head)
+			      struct list_head *head,
+			      const struct process_trailer_options *opts)
 {
 	struct trailer_info info;
 	struct strbuf tok = STRBUF_INIT;
@@ -897,9 +900,10 @@ static int process_input_file(FILE *outfile,
 	trailer_info_get(&info, str);
 
 	/* Print lines before the trailers as is */
-	fwrite(str, 1, info.trailer_start - str, outfile);
+	if (!opts->only_trailers)
+		fwrite(str, 1, info.trailer_start - str, outfile);
 
-	if (!info.blank_line_before_trailer)
+	if (!opts->only_trailers && !info.blank_line_before_trailer)
 		fprintf(outfile, "\n");
 
 	for (i = 0; i < info.trailer_nr; i++) {
@@ -914,7 +918,7 @@ static int process_input_file(FILE *outfile,
 			add_trailer_item(head,
 					 strbuf_detach(&tok, NULL),
 					 strbuf_detach(&val, NULL));
-		} else {
+		} else if (!opts->only_trailers) {
 			strbuf_addstr(&val, trailer);
 			strbuf_strip_suffix(&val, "\n");
 			add_trailer_item(head,
@@ -986,18 +990,19 @@ void process_trailers(const char *file,
 		outfile = create_in_place_tempfile(file);
 
 	/* Print the lines before the trailers */
-	trailer_end = process_input_file(outfile, sb.buf, &head);
+	trailer_end = process_input_file(outfile, sb.buf, &head, opts);
 
 	process_command_line_args(&arg_head, trailers);
 
 	process_trailers_lists(&head, &arg_head);
 
-	print_all(outfile, &head, opts->trim_empty);
+	print_all(outfile, &head, opts);
 
 	free_all(&head);
 
 	/* Print the lines after the trailers as is */
-	fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
+	if (!opts->only_trailers)
+		fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
 
 	if (opts->in_place)
 		if (rename_tempfile(&trailers_tempfile, file))
diff --git a/trailer.h b/trailer.h
index 9da00bedec..3cf35ced00 100644
--- a/trailer.h
+++ b/trailer.h
@@ -25,6 +25,7 @@ struct trailer_info {
 struct process_trailer_options {
 	int in_place;
 	int trim_empty;
+	int only_trailers;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.14.1.352.ge5efb0d3f3

[PATCH v4 3/8] interpret-trailers: add an option to show only existing trailers

[Jeff King]

It can be useful to invoke interpret-trailers for the
primary purpose of parsing existing trailers. But in that
case, we don't want to apply existing ifMissing or ifExists
rules from the config. Let's add a special mode where we
avoid applying those rules. Coupled with --only-trailers,
this gives us a reasonable parsing tool.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt |  5 +++++
 builtin/interpret-trailers.c             |  7 +++++++
 t/t7513-interpret-trailers.sh            | 16 ++++++++++++++++
 trailer.c                                |  9 +++++----
 trailer.h                                |  1 +
 5 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 295dffbd21..7cc43b0e3e 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -83,6 +83,11 @@ OPTIONS
 --only-trailers::
 	Output only the trailers, not any other parts of the input.
 
+--only-input::
+	Output only trailers that exist in the input; do not add any
+	from the command-line or by following configured `trailer.*`
+	rules.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index afb12c11bc..2d90e0e480 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -25,6 +25,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
+		OPT_BOOL(0, "only-input", &opts.only_input, N_("do not apply config rules")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
@@ -33,6 +34,12 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_interpret_trailers_usage, 0);
 
+	if (opts.only_input && trailers.nr)
+		usage_msg_opt(
+			_("--trailer with --only-input does not make sense"),
+			git_interpret_trailers_usage,
+			options);
+
 	if (argc) {
 		int i;
 		for (i = 0; i < argc; i++)
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 90d30037b7..94b6c52473 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1314,4 +1314,20 @@ test_expect_success 'only-trailers omits non-trailer in middle of block' '
 	test_cmp expected actual
 '
 
+test_expect_success 'only input' '
+	git config trailer.sign.command "echo config-value" &&
+	cat >expected <<-\EOF &&
+		existing: existing-value
+	EOF
+	git interpret-trailers \
+		--only-trailers --only-input >actual <<-\EOF &&
+		my subject
+
+		my body
+
+		existing: existing-value
+	EOF
+	test_cmp expected actual
+'
+
 test_done
diff --git a/trailer.c b/trailer.c
index 22146002e9..f97fcef3a5 100644
--- a/trailer.c
+++ b/trailer.c
@@ -977,7 +977,6 @@ void process_trailers(const char *file,
 		      struct string_list *trailers)
 {
 	LIST_HEAD(head);
-	LIST_HEAD(arg_head);
 	struct strbuf sb = STRBUF_INIT;
 	int trailer_end;
 	FILE *outfile = stdout;
@@ -992,9 +991,11 @@ void process_trailers(const char *file,
 	/* Print the lines before the trailers */
 	trailer_end = process_input_file(outfile, sb.buf, &head, opts);
 
-	process_command_line_args(&arg_head, trailers);
-
-	process_trailers_lists(&head, &arg_head);
+	if (!opts->only_input) {
+		LIST_HEAD(arg_head);
+		process_command_line_args(&arg_head, trailers);
+		process_trailers_lists(&head, &arg_head);
+	}
 
 	print_all(outfile, &head, opts);
 
diff --git a/trailer.h b/trailer.h
index 3cf35ced00..76c3b571bf 100644
--- a/trailer.h
+++ b/trailer.h
@@ -26,6 +26,7 @@ struct process_trailer_options {
 	int in_place;
 	int trim_empty;
 	int only_trailers;
+	int only_input;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.14.1.352.ge5efb0d3f3

[PATCH v4 4/8] interpret-trailers: add an option to unfold values

[Jeff King]

The point of "--only-trailers" is to give a caller an output
that's easy for them to parse. Getting rid of the
non-trailer material helps, but we still may see more
complicated syntax like whitespace continuation. Let's add
an option to unfold any continuation, giving the output as a
single "key: value" line per trailer.

As a bonus, this could be used even without --only-trailers
to clean up unusual formatting in the incoming data.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt |  4 ++++
 builtin/interpret-trailers.c             |  1 +
 t/t7513-interpret-trailers.sh            | 21 +++++++++++++++++++++
 trailer.c                                | 29 +++++++++++++++++++++++++++++
 trailer.h                                |  1 +
 5 files changed, 56 insertions(+)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 7cc43b0e3e..be948e8028 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -88,6 +88,10 @@ OPTIONS
 	from the command-line or by following configured `trailer.*`
 	rules.
 
+--unfold::
+	Remove any whitespace-continuation in trailers, so that each
+	trailer appears on a line by itself with its full content.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 2d90e0e480..922c3bad63 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -26,6 +26,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
 		OPT_BOOL(0, "only-input", &opts.only_input, N_("do not apply config rules")),
+		OPT_BOOL(0, "unfold", &opts.unfold, N_("join whitespace-continued values")),
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 94b6c52473..baf2feba98 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1330,4 +1330,25 @@ test_expect_success 'only input' '
 	test_cmp expected actual
 '
 
+test_expect_success 'unfold' '
+	cat >expected <<-\EOF &&
+		foo: continued across several lines
+	EOF
+	# pass through tr to make leading and trailing whitespace more obvious
+	tr _ " " <<-\EOF |
+		my subject
+
+		my body
+
+		foo:_
+		__continued
+		___across
+		____several
+		_____lines
+		___
+	EOF
+	git interpret-trailers --only-trailers --only-input --unfold >actual &&
+	test_cmp expected actual
+'
+
 test_done
diff --git a/trailer.c b/trailer.c
index f97fcef3a5..ed4fedc087 100644
--- a/trailer.c
+++ b/trailer.c
@@ -887,6 +887,33 @@ static int ends_with_blank_line(const char *buf, size_t len)
 	return is_blank_line(buf + ll);
 }
 
+static void unfold_value(struct strbuf *val)
+{
+	struct strbuf out = STRBUF_INIT;
+	size_t i;
+
+	strbuf_grow(&out, val->len);
+	i = 0;
+	while (i < val->len) {
+		char c = val->buf[i++];
+		if (c == '\n') {
+			/* Collapse continuation down to a single space. */
+			while (i < val->len && isspace(val->buf[i]))
+				i++;
+			strbuf_addch(&out, ' ');
+		} else {
+			strbuf_addch(&out, c);
+		}
+	}
+
+	/* Empty lines may have left us with whitespace cruft at the edges */
+	strbuf_trim(&out);
+
+	/* output goes back to val as if we modified it in-place */
+	strbuf_swap(&out, val);
+	strbuf_release(&out);
+}
+
 static int process_input_file(FILE *outfile,
 			      const char *str,
 			      struct list_head *head,
@@ -915,6 +942,8 @@ static int process_input_file(FILE *outfile,
 		if (separator_pos >= 1) {
 			parse_trailer(&tok, &val, NULL, trailer,
 				      separator_pos);
+			if (opts->unfold)
+				unfold_value(&val);
 			add_trailer_item(head,
 					 strbuf_detach(&tok, NULL),
 					 strbuf_detach(&val, NULL));
diff --git a/trailer.h b/trailer.h
index 76c3b571bf..194f85a102 100644
--- a/trailer.h
+++ b/trailer.h
@@ -27,6 +27,7 @@ struct process_trailer_options {
 	int trim_empty;
 	int only_trailers;
 	int only_input;
+	int unfold;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}
-- 
2.14.1.352.ge5efb0d3f3

[PATCH v4 5/8] interpret-trailers: add --parse convenience option

[Jeff King]

The last few commits have added command line options that
can turn interpret-trailers into a parsing tool. Since
they'd most often be used together, let's provide a
convenient single option for callers to invoke this mode.

This is implemented as a callback rather than a boolean so
that its effect is applied immediately, as if those options
had been specified. Later options can then override them.
E.g.:

  git interpret-trailers --parse --no-unfold

would work.

Let's also update the documentation to make clear that this
parsing mode behaves quite differently than the normal
"add trailers to the input" mode.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-interpret-trailers.txt | 21 ++++++++++++++-------
 builtin/interpret-trailers.c             | 12 ++++++++++++
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index be948e8028..1df8aabf51 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -3,24 +3,27 @@ git-interpret-trailers(1)
 
 NAME
 ----
-git-interpret-trailers - help add structured information into commit messages
+git-interpret-trailers - add or parse structured information in commit messages
 
 SYNOPSIS
 --------
 [verse]
-'git interpret-trailers' [--in-place] [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]
+'git interpret-trailers' [options] [(--trailer <token>[(=|:)<value>])...] [<file>...]
+'git interpret-trailers' [options] [--parse] [<file>...]
 
 DESCRIPTION
 -----------
-Help adding 'trailers' lines, that look similar to RFC 822 e-mail
+Help parsing or adding 'trailers' lines, that look similar to RFC 822 e-mail
 headers, at the end of the otherwise free-form part of a commit
 message.
 
 This command reads some patches or commit messages from either the
-<file> arguments or the standard input if no <file> is specified. Then
-this command applies the arguments passed using the `--trailer`
-option, if any, to the commit message part of each input file. The
-result is emitted on the standard output.
+<file> arguments or the standard input if no <file> is specified. If
+`--parse` is specified, the output consists of the parsed trailers.
+
+Otherwise, the this command applies the arguments passed using the
+`--trailer` option, if any, to the commit message part of each input
+file. The result is emitted on the standard output.
 
 Some configuration variables control the way the `--trailer` arguments
 are applied to each commit message and the way any existing trailer in
@@ -92,6 +95,10 @@ OPTIONS
 	Remove any whitespace-continuation in trailers, so that each
 	trailer appears on a line by itself with its full content.
 
+--parse::
+	A convenience alias for `--only-trailers --only-input
+	--unfold`.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 922c3bad63..555111a078 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -16,6 +16,16 @@ static const char * const git_interpret_trailers_usage[] = {
 	NULL
 };
 
+static int parse_opt_parse(const struct option *opt, const char *arg,
+			   int unset)
+{
+	struct process_trailer_options *v = opt->value;
+	v->only_trailers = 1;
+	v->only_input = 1;
+	v->unfold = 1;
+	return 0;
+}
+
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
 	struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
@@ -27,6 +37,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
 		OPT_BOOL(0, "only-input", &opts.only_input, N_("do not apply config rules")),
 		OPT_BOOL(0, "unfold", &opts.unfold, N_("join whitespace-continued values")),
+		{ OPTION_CALLBACK, 0, "parse", &opts, NULL, N_("set parsing options"),
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse },
 		OPT_STRING_LIST(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add")),
 		OPT_END()
-- 
2.14.1.352.ge5efb0d3f3

[PATCH v4 6/8] pretty: move trailer formatting to trailer.c

[Jeff King]

The next commit will add many features to the %(trailer)
placeholder in pretty.c. We'll need to access some internal
functions of trailer.c for that, so our options are either:

  1. expose those functions publicly

or

  2. make an entry point into trailer.c to do the formatting

Doing (2) ends up exposing less surface area, though do note
that caveats in the docstring of the new function.

Signed-off-by: Jeff King <peff@peff.net>
---
 pretty.c  | 13 ++-----------
 trailer.c | 18 ++++++++++++++++++
 trailer.h | 14 ++++++++++++++
 3 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/pretty.c b/pretty.c
index 39cad5112b..99b309bb15 100644
--- a/pretty.c
+++ b/pretty.c
@@ -871,16 +871,6 @@ const char *format_subject(struct strbuf *sb, const char *msg,
 	return msg;
 }
 
-static void format_trailers(struct strbuf *sb, const char *msg)
-{
-	struct trailer_info info;
-
-	trailer_info_get(&info, msg);
-	strbuf_add(sb, info.trailer_start,
-		   info.trailer_end - info.trailer_start);
-	trailer_info_release(&info);
-}
-
 static void parse_commit_message(struct format_commit_context *c)
 {
 	const char *msg = c->message + c->message_off;
@@ -1293,7 +1283,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	}
 
 	if (starts_with(placeholder, "(trailers)")) {
-		format_trailers(sb, msg + c->subject_off);
+		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+		format_trailers_from_commit(sb, msg + c->subject_off, &opts);
 		return strlen("(trailers)");
 	}
 
diff --git a/trailer.c b/trailer.c
index ed4fedc087..2d2b997ccc 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1091,3 +1091,21 @@ void trailer_info_release(struct trailer_info *info)
 		free(info->trailers[i]);
 	free(info->trailers);
 }
+
+static void format_trailer_info(struct strbuf *out,
+				const struct trailer_info *info,
+				const struct process_trailer_options *opts)
+{
+	strbuf_add(out, info->trailer_start,
+		   info->trailer_end - info->trailer_start);
+}
+
+void format_trailers_from_commit(struct strbuf *out, const char *msg,
+				 const struct process_trailer_options *opts)
+{
+	struct trailer_info info;
+
+	trailer_info_get(&info, msg);
+	format_trailer_info(out, &info, opts);
+	trailer_info_release(&info);
+}
diff --git a/trailer.h b/trailer.h
index 194f85a102..a172811022 100644
--- a/trailer.h
+++ b/trailer.h
@@ -40,4 +40,18 @@ void trailer_info_get(struct trailer_info *info, const char *str);
 
 void trailer_info_release(struct trailer_info *info);
 
+/*
+ * Format the trailers from the commit msg "msg" into the strbuf "out".
+ * Note two caveats about "opts":
+ *
+ *   - this is primarily a helper for pretty.c, and not
+ *     all of the flags are supported.
+ *
+ *   - this differs from process_trailers slightly in that we always format
+ *     only the trailer block itself, even if the "only_trailers" option is not
+ *     set.
+ */
+void format_trailers_from_commit(struct strbuf *out, const char *msg,
+				 const struct process_trailer_options *opts);
+
 #endif /* TRAILER_H */
-- 
2.14.1.352.ge5efb0d3f3

[PATCH v4 7/8] t4205: refactor %(trailers) tests

[Jeff King]

We currently have one test for %(trailers). In preparation
for more, let's refactor a few bits:

  - move the commit creation to its own setup step so it can
    be reused by multiple tests

  - add a trailer with whitespace continuation (to confirm
    that it is left untouched)

  - fix the sample text which claims the placeholder is %bT.
    This was switched long ago to %(trailers)

  - replace one "cat" with an "echo" when generating the
    expected output. This saves a process (and sets a better
    pattern for future tests to follow).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t4205-log-pretty-formats.sh | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 18aa1b5889..83ea85eb45 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -539,25 +539,29 @@ cat >trailers <<EOF
 Signed-off-by: A U Thor <author@example.com>
 Acked-by: A U Thor <author@example.com>
 [ v2 updated patch description ]
-Signed-off-by: A U Thor <author@example.com>
+Signed-off-by: A U Thor
+  <author@example.com>
 EOF
 
-test_expect_success 'pretty format %(trailers) shows trailers' '
+test_expect_success 'set up trailer tests' '
 	echo "Some contents" >trailerfile &&
 	git add trailerfile &&
-	git commit -F - <<-EOF &&
+	git commit -F - <<-EOF
 	trailers: this commit message has trailers
 
 	This commit is a test commit with trailers at the end. We parse this
-	message and display the trailers using %bT
+	message and display the trailers using %(trailers).
 
 	$(cat trailers)
 	EOF
-	git log --no-walk --pretty="%(trailers)" >actual &&
-	cat >expect <<-EOF &&
-	$(cat trailers)
+'
 
-	EOF
+test_expect_success 'pretty format %(trailers) shows trailers' '
+	git log --no-walk --pretty="%(trailers)" >actual &&
+	{
+		cat trailers &&
+		echo
+	} >expect &&
 	test_cmp expect actual
 '
 
-- 
2.14.1.352.ge5efb0d3f3

[PATCH v4 8/8] pretty: support normalization options for %(trailers)

[Jeff King]

The interpret-trailers command recently learned some options
to make its output easier to parse (for a caller whose only
interested in picking out the trailer values). But it's not
very efficient for asking for the trailers of many commits
in a single invocation.

We already have "%(trailers)" to do that, but it doesn't
know about unfolding or omitting non-trailers. Let's plumb
those options through, so you can have the best of both.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/pretty-formats.txt |  5 ++++-
 pretty.c                         | 15 ++++++++++++---
 t/t4205-log-pretty-formats.sh    | 33 +++++++++++++++++++++++++++++++++
 trailer.c                        | 32 ++++++++++++++++++++++++++++++--
 4 files changed, 79 insertions(+), 6 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 973d19606b..d433d50f81 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -205,7 +205,10 @@ endif::git-rev-list[]
 - '%><(<N>)', '%><|(<N>)': similar to '% <(<N>)', '%<|(<N>)'
   respectively, but padding both sides (i.e. the text is centered)
 - %(trailers): display the trailers of the body as interpreted by
-  linkgit:git-interpret-trailers[1]
+  linkgit:git-interpret-trailers[1]. If the `:only` option is given,
+  omit non-trailer lines from the trailer block.  If the `:unfold`
+  option is given, behave as if interpret-trailer's `--unfold` option
+  was given. E.g., `%(trailers:only:unfold)` to do both.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index 99b309bb15..94eab5c89e 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1064,6 +1064,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	const struct commit *commit = c->commit;
 	const char *msg = c->message;
 	struct commit_list *p;
+	const char *arg;
 	int ch;
 
 	/* these are independent of the commit */
@@ -1282,10 +1283,18 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		return 1;
 	}
 
-	if (starts_with(placeholder, "(trailers)")) {
+	if (skip_prefix(placeholder, "(trailers", &arg)) {
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
-		format_trailers_from_commit(sb, msg + c->subject_off, &opts);
-		return strlen("(trailers)");
+		while (*arg == ':') {
+			if (skip_prefix(arg, ":only", &arg))
+				opts.only_trailers = 1;
+			else if (skip_prefix(arg, ":unfold", &arg))
+				opts.unfold = 1;
+		}
+		if (*arg == ')') {
+			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
+			return arg - placeholder + 1;
+		}
 	}
 
 	return 0;	/* unknown placeholder */
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 83ea85eb45..ec5f530102 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -543,6 +543,10 @@ Signed-off-by: A U Thor
   <author@example.com>
 EOF
 
+unfold () {
+	perl -0pe 's/\n\s+/ /'
+}
+
 test_expect_success 'set up trailer tests' '
 	echo "Some contents" >trailerfile &&
 	git add trailerfile &&
@@ -565,4 +569,33 @@ test_expect_success 'pretty format %(trailers) shows trailers' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:only) shows only "key: value" trailers' '
+	git log --no-walk --pretty="%(trailers:only)" >actual &&
+	{
+		grep -v patch.description <trailers &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:unfold) unfolds trailers' '
+	git log --no-walk --pretty="%(trailers:unfold)" >actual &&
+	{
+		unfold <trailers &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success ':only and :unfold work together' '
+	git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
+	git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
+	test_cmp actual reverse &&
+	{
+		grep -v patch.description <trailers | unfold &&
+		echo
+	} >expect &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/trailer.c b/trailer.c
index 2d2b997ccc..5220d498e5 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1096,8 +1096,36 @@ static void format_trailer_info(struct strbuf *out,
 				const struct trailer_info *info,
 				const struct process_trailer_options *opts)
 {
-	strbuf_add(out, info->trailer_start,
-		   info->trailer_end - info->trailer_start);
+	int i;
+
+	/* If we want the whole block untouched, we can take the fast path. */
+	if (!opts->only_trailers && !opts->unfold) {
+		strbuf_add(out, info->trailer_start,
+			   info->trailer_end - info->trailer_start);
+		return;
+	}
+
+	for (i = 0; i < info->trailer_nr; i++) {
+		char *trailer = info->trailers[i];
+		int separator_pos = find_separator(trailer, separators);
+
+		if (separator_pos >= 1) {
+			struct strbuf tok = STRBUF_INIT;
+			struct strbuf val = STRBUF_INIT;
+
+			parse_trailer(&tok, &val, NULL, trailer, separator_pos);
+			if (opts->unfold)
+				unfold_value(&val);
+
+			strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+			strbuf_release(&tok);
+			strbuf_release(&val);
+
+		} else if (!opts->only_trailers) {
+			strbuf_addstr(out, trailer);
+		}
+	}
+
 }
 
 void format_trailers_from_commit(struct strbuf *out, const char *msg,
-- 
2.14.1.352.ge5efb0d3f3

Re: [PATCH v4 5/8] interpret-trailers: add --parse convenience option

[Martin Ågren]

On 15 August 2017 at 12:23, Jeff King <peff@peff.net> wrote:
>  SYNOPSIS
>  --------
>  [verse]
> -'git interpret-trailers' [--in-place] [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]
> +'git interpret-trailers' [options] [(--trailer <token>[(=|:)<value>])...] [<file>...]
> +'git interpret-trailers' [options] [--parse] [<file>...]
>
>  DESCRIPTION
>  -----------
> -Help adding 'trailers' lines, that look similar to RFC 822 e-mail
> +Help parsing or adding 'trailers' lines, that look similar to RFC 822 e-mail
>  headers, at the end of the otherwise free-form part of a commit
>  message.
>
>  This command reads some patches or commit messages from either the
> -<file> arguments or the standard input if no <file> is specified. Then
> -this command applies the arguments passed using the `--trailer`
> -option, if any, to the commit message part of each input file. The
> -result is emitted on the standard output.
> +<file> arguments or the standard input if no <file> is specified. If
> +`--parse` is specified, the output consists of the parsed trailers.
> +
> +Otherwise, the this command applies the arguments passed using the
> +`--trailer` option, if any, to the commit message part of each input
> +file. The result is emitted on the standard output.

"the this"

I think I get why you use --parse above (and in the synopsis), although
it kind of feels like it should be --only-input or perhaps "--only-input
(or --parse)".

--only-input is sort of not covered by the "--parse"-part above, and it
is sort of not covered in the "Otherwise", since --only-input and
--trailer are incompatible. So it is sort of lost. :-) Probably doesn't
matter much. (I'm just thinking out loud without constructive ideas.)
Those who care about such details can continue reading..

Martin

Re: [PATCH v4 5/8] interpret-trailers: add --parse convenience option

[Jeff King]

On Tue, Aug 15, 2017 at 01:26:53PM +0200, Martin Ågren wrote:

> >  This command reads some patches or commit messages from either the
> > -<file> arguments or the standard input if no <file> is specified. Then
> > -this command applies the arguments passed using the `--trailer`
> > -option, if any, to the commit message part of each input file. The
> > -result is emitted on the standard output.
> > +<file> arguments or the standard input if no <file> is specified. If
> > +`--parse` is specified, the output consists of the parsed trailers.
> > +
> > +Otherwise, the this command applies the arguments passed using the
> > +`--trailer` option, if any, to the commit message part of each input
> > +file. The result is emitted on the standard output.
> 
> "the this"

Thanks.

> I think I get why you use --parse above (and in the synopsis), although
> it kind of feels like it should be --only-input or perhaps "--only-input
> (or --parse)".

I really wanted to point people to --parse as the go-to option for the
parsing mode for the sake of simplicity (in fact I initially considered
not even exposing them at all). And I hoped that if they jumped to the
definition of --parse, that would lead them to the other options.

I dunno. I agree it is does not read particularly well. Probably the
whole description section could be rewritten to cover the newly dual
nature of the command a bit better. But I didn't want to disrupt the
existing flow too much.

-Peff

Re: [PATCH v4 5/8] interpret-trailers: add --parse convenience option

[Martin Ågren]

On 16 August 2017 at 10:20, Jeff King <peff@peff.net> wrote:
> On Tue, Aug 15, 2017 at 01:26:53PM +0200, Martin Ågren wrote:
>
>> >  This command reads some patches or commit messages from either the
>> > -<file> arguments or the standard input if no <file> is specified. Then
>> > -this command applies the arguments passed using the `--trailer`
>> > -option, if any, to the commit message part of each input file. The
>> > -result is emitted on the standard output.
>> > +<file> arguments or the standard input if no <file> is specified. If
>> > +`--parse` is specified, the output consists of the parsed trailers.
>> > +
>> > +Otherwise, the this command applies the arguments passed using the
>> > +`--trailer` option, if any, to the commit message part of each input
>> > +file. The result is emitted on the standard output.
>>
>> "the this"
>
> Thanks.
>
>> I think I get why you use --parse above (and in the synopsis), although
>> it kind of feels like it should be --only-input or perhaps "--only-input
>> (or --parse)".
>
> I really wanted to point people to --parse as the go-to option for the
> parsing mode for the sake of simplicity (in fact I initially considered
> not even exposing them at all). And I hoped that if they jumped to the
> definition of --parse, that would lead them to the other options.
>
> I dunno. I agree it is does not read particularly well. Probably the
> whole description section could be rewritten to cover the newly dual
> nature of the command a bit better. But I didn't want to disrupt the
> existing flow too much.

Certainly. It's not like I can offer a "better" way. After thinking
about this some more, I think this is a good example of "less is more".
It's possibly less precise or complete in some sense, but it's probably
much more useful and readable.