[PATCH v2 0/5] pretty format %(trailers): improve machine readability

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

Per Anders's feedback in [1] this is now a stand-alone series based on
"master". The merge conflict with his series is trivial, both add or
change struct member(s) in trailer.h.

v2 changes:

 * Either incorporate all the feedback from Christian Couder on v1, or
   changed things around to e.g. eliminate the relevant code.

 * Drop the whole "have_options" approach. I saw it conflicted with
   Hariom Verma's cleanup of ref-filter.c in a bad way. I also wasn't
   running the for-each-ref tests in v1 (just interpret-trailers &
   log/pretty). Ran every commit in v2 with the whole test suite (as I
   should have done to begin with, sorry).

 * I'd accidentally left DEVOPTS=no-error on in v1 and didn't notice a
   missing struct member warning. Oops.

 * Lots of my own commit message rewording/cleanup/grammar fixes.

Ævar Arnfjörð Bjarmason (5):
  pretty format %(trailers) test: split a long line
  pretty format %(trailers) doc: avoid repetition
  pretty-format %(trailers): fix broken standalone "valueonly"
  pretty format %(trailers): add a "keyonly"
  pretty format %(trailers): add a "key_value_separator"

 Documentation/pretty-formats.txt | 34 ++++++-----
 pretty.c                         | 10 ++++
 t/t4205-log-pretty-formats.sh    | 99 +++++++++++++++++++++++++++++++-
 trailer.c                        | 15 ++++-
 trailer.h                        |  2 +
 5 files changed, 141 insertions(+), 19 deletions(-)

Range-diff:
1:  51a7a6d8cfe ! 1:  4b134a62aec pretty format %(trailers) test: split a long line
    @@ Commit message
     
         Split a very long line in a test introduced in 0b691d86851 (pretty:
         add support for separator option in %(trailers), 2019-01-28). This
    -    makes it easier to read, and it'll be used as a template in follow-up
    -    commits.
    +    makes it easier to read, especially as follow-up commits will copy
    +    this test as a template.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
2:  2340d856a90 ! 2:  0d3fe6daf6c pretty format %(trailers): avoid needless repetition
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    pretty format %(trailers): avoid needless repetition
    +    pretty format %(trailers) doc: avoid repetition
     
         Change the documentation for the various %(trailers) options so it
         isn't repeating part of the documentation for "only" about how boolean
    -    values are handled. Instead let's split the description of that into
    +    values are handled. Instead, let's split the description of that into
         general documentation at the top.
     
         It then suffices to refer to it by listing the options as
    -    "opt[=bool]". I'm also changing it to "[=bool]" from "[=val]". It took
    -    me a couple of readings to realize that while to realize that these
    -    options were referring back to the "only" option's treatment of
    -    boolean values. Let's try to make this more explicit.
    +    "opt[=<BOOL>]". I'm also changing it to upper-case "[=<BOOL>]" from
    +    "[=val]" for consistency with "<SEP>"
    +
    +    It took me a couple of readings to realize that these options were
    +    referring back to the "only" option's treatment of boolean
    +    values. Let's try to make this more explicit, and upper-case "BOOL"
    +    for consistency with the existing "<SEP>" and "<K>".
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ Documentation/pretty-formats.txt: endif::git-rev-list[]
      			  `trailers` string may be followed by a colon
     -			  and zero or more comma-separated options:
     +			  and zero or more comma-separated options.
    ++			  If any option is provided multiple times the
    ++			  last occurance wins.
     ++
    -+The boolean options accept an optional value. The values `true`,
    -+`false`, `on`, `off` etc. are all accepted. See the "boolean"
    ++The boolean options accept an optional value `[=<BOOL>]`. The values
    ++`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
     +sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
    -+option is given with no value it's enabled. If any option is provided
    -+multiple times the last occurance wins.
    ++option is given with no value, it's enabled.
     ++
      ** 'key=<K>': only show trailers with specified key. Matching is done
         case-insensitively and trailing colon is optional. If option is
    @@ Documentation/pretty-formats.txt: endif::git-rev-list[]
     -   `false`, `off`, `no` to show the non-trailer lines. If option is
     -   given without value it is enabled. If given multiple times the last
     -   value is used.
    -+** 'only[=bool]': select whether non-trailer lines from the trailer
    ++** 'only[=BOOL]': select whether non-trailer lines from the trailer
     +   block should be included.
      ** 'separator=<SEP>': specify a separator inserted between trailer
         lines. When this option is not given each trailer line is
    @@ Documentation/pretty-formats.txt: endif::git-rev-list[]
     -** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`
     -   option was given. In same way as to for `only` it can be followed
     -   by an equal sign and explicit value. E.g.,
    -+** 'unfold[=bool]': make it behave as if interpret-trailer's `--unfold`
    ++** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`
     +   option was given. E.g.,
         `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
     -** 'valueonly[=val]': skip over the key part of the trailer line and only
     -   show the value part. Also this optionally allows explicit value.
    -+** 'valueonly[=bool]': skip over the key part of the trailer line and only
    ++** 'valueonly[=BOOL]': skip over the key part of the trailer line and only
     +   show the value part.
      
      NOTE: Some placeholders may depend on other options given to the
4:  e9ca1e8d88c ! 3:  ea44eeff510 pretty-format %(trailers): fix broken standalone "valueonly"
    @@ Commit message
         pretty-format %(trailers): fix broken standalone "valueonly"
     
         Fix %(trailers:valueonly) being a noop due to on overly eager
    -    optimization. When new trailer options were added they needed to be
    -    listed at the start of the format_trailer_info() function. E.g. as was
    -    done in 250bea0c165 (pretty: allow showing specific trailers,
    -    2019-01-28).
    +    optimization in format_trailer_info() which skips custom formatting if
    +    no custom options are given.
     
    -    When d9b936db522 (pretty: add support for "valueonly" option in
    -    %(trailers), 2019-01-28) was added this was omitted by mistake. Thus
    -    %(trailers:valueonly) was a noop, instead of showing only trailer
    -    value. This wasn't caught because the tests for it always combined it
    -    with other options.
    +    When "valueonly" was added in d9b936db522 (pretty: add support for
    +    "valueonly" option in %(trailers), 2019-01-28) we forgot to add it to
    +    the list of options that optimization checks for. See e.g. the
    +    addition of "key" in 250bea0c165 (pretty: allow showing specific
    +    trailers, 2019-01-28) for a similar change where this wasn't missed.
     
    -    Let's fix the bug, and switch away from this pattern requiring us to
    -    remember to add new flags to the start of the function. Instead as
    -    soon as we see the ":" in "%(trailers:" we skip the fast path. That
    -    over-matches for "%(trailers:)", but I think that's OK.
    +    Thus the "valueonly" option in "%(trailers:valueonly)" was a noop and
    +    the output was equivalent to that of a plain "%(trailers)". This
    +    wasn't caught because the tests for it always combined it with other
    +    options.
     
    -    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +    Fix the bug by adding !opts->value_only to the list. I initially
    +    attempted to make this more future-proof by setting a flag if we got
    +    to ":" in "%(trailers:" in format_commit_one() in pretty.c. However,
    +    "%(trailers:" is also parsed in trailers_atom_parser() in
    +    ref-filter.c.
     
    - ## pretty.c ##
    -@@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
    - 		opts.no_divider = 1;
    - 
    - 		if (*arg == ':') {
    -+			/* over-matches on %(trailers:), but that's OK */
    -+			opts.have_options = 1;
    - 			arg++;
    - 			for (;;) {
    - 				const char *argval;
    +    There is an outstanding patch[1] unify those two, and such a fix, or
    +    other future-proofing, such as changing "process_trailer_options"
    +    flags into a bitfield, would conflict with that effort. Let's instead
    +    do the bare minimum here as this aspect of trailers is being actively
    +    worked on by another series.
    +
    +    Let's also test for a plain "valueonly" without any other options, as
    +    well as "separator". All the other existing options on the pretty.c
    +    path had tests where they were the only option provided. I'm also
    +    keeping a sanity test for "%(trailers:)" being the same as
    +    "%(trailers)". There's no reason to suspect it wouldn't be in the
    +    current implementation, but let's keep it in the interest of black box
    +    testing.
    +
    +    1. https://lore.kernel.org/git/pull.726.git.1599335291.gitgitgadget@gmail.com/
    +
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## t/t4205-log-pretty-formats.sh ##
    +@@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers) shows trailers' '
    + 	test_cmp expect actual
    + '
    + 
    ++test_expect_success 'pretty format %(trailers:) enables no options' '
    ++	git log --no-walk --pretty="%(trailers:)" >actual &&
    ++	# "expect" the same as the test above
    ++	test_cmp expect actual
    ++'
    ++
    + test_expect_success '%(trailers:only) shows only "key: value" trailers' '
    + 	git log --no-walk --pretty="%(trailers:only)" >actual &&
    + 	{
     @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
      	test_cmp expect actual
      '
    @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:key=foo,valueonly
     +	test_cmp expect actual
     +'
     +
    - test_expect_success '%(trailers:key=foo,keyonly,valueonly) shows nothing' '
    - 	git log --no-walk --pretty="format:%(trailers:key=Acked-by,keyonly,valueonly)" >actual &&
    - 	echo >expect &&
    + test_expect_success 'pretty format %(trailers:separator) changes separator' '
    ++	git log --no-walk --pretty=format:"X%(trailers:separator=%x00)X" >actual &&
    ++	(
    ++		printf "XSigned-off-by: A U Thor <author@example.com>\0" &&
    ++		printf "Acked-by: A U Thor <author@example.com>\0" &&
    ++		printf "[ v2 updated patch description ]\0" &&
    ++		printf "Signed-off-by: A U Thor\n  <author@example.com>X"
    ++	) >expect &&
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separator' '
    + 	git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual &&
    + 	(
    + 		printf "XSigned-off-by: A U Thor <author@example.com>\0" &&
     
      ## trailer.c ##
     @@ trailer.c: static void format_trailer_info(struct strbuf *out,
    - 	size_t origlen = out->len;
      	size_t i;
      
    --	/* If we want the whole block untouched, we can take the fast path. */
    + 	/* If we want the whole block untouched, we can take the fast path. */
     -	if (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) {
    -+	if (!opts->have_options) {
    ++	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
    ++	    !opts->separator && !opts->value_only) {
      		strbuf_add(out, info->trailer_start,
      			   info->trailer_end - info->trailer_start);
      		return;
    -
    - ## trailer.h ##
    -@@ trailer.h: struct new_trailer_item {
    - };
    - 
    - struct process_trailer_options {
    -+	int have_options;
    - 	int in_place;
    - 	int trim_empty;
    - 	int only_trailers;
3:  b71a700fa9b ! 4:  4fd193fd90c pretty format %(trailers): add a "keyonly"
    @@ Commit message
         change the ": " delimiter to something easily machine-readable (a key
         might contain ": " too). A follow-up change will add support for that.
     
    +    I don't really have a use-case for just "keyonly" myself. I suppose it
    +    would be useful in some cases as "key=*" matches case-insensitively,
    +    so a plain "keyonly" will give you the variants of the keys you
    +    matched. I'm mainly adding it to fix the inconsistency with
    +    "valueonly".
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Documentation/pretty-formats.txt ##
    -@@ Documentation/pretty-formats.txt: multiple times the last occurance wins.
    - ** 'unfold[=bool]': make it behave as if interpret-trailer's `--unfold`
    +@@ Documentation/pretty-formats.txt: option is given with no value, it's enabled.
    + ** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`
         option was given. E.g.,
         `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
    --** 'valueonly[=bool]': skip over the key part of the trailer line and only
    +-** 'valueonly[=BOOL]': skip over the key part of the trailer line and only
     -   show the value part.
    -+** 'keyonly[=bool]': only show the key part of the trailer.
    -+** 'valueonly[=bool]': only show the value part of the trailer.
    ++** 'keyonly[=BOOL]': only show the key part of the trailer.
    ++** 'valueonly[=BOOL]': only show the value part of the trailer.
      
      NOTE: Some placeholders may depend on other options given to the
      revision traversal engine. For example, the `%g*` reflog options will
    @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:key) without valu
      	test_cmp expect actual
      '
      
    -+test_expect_success '%(trailers:key=foo,keyonly) shows only keys' '
    ++test_expect_success '%(trailers:keyonly) shows only keys' '
     +	git log --no-walk --pretty="format:%(trailers:keyonly)" >actual &&
     +	test_write_lines \
     +		"Signed-off-by" \
    @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:key) without valu
      test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
      	git log --no-walk --pretty="format:%(trailers:key=Acked-by,valueonly)" >actual &&
      	echo "A U Thor <author@example.com>" >expect &&
    +@@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:valueonly) shows only values' '
      	test_cmp expect actual
      '
      
    @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:key) without valu
     +'
     +
      test_expect_success 'pretty format %(trailers:separator) changes separator' '
    - 	git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual &&
    + 	git log --no-walk --pretty=format:"X%(trailers:separator=%x00)X" >actual &&
      	(
    -@@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers:separator) changes separator' '
    +@@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa
      	test_cmp expect actual
      '
      
    @@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers) co
     
      ## trailer.c ##
     @@ trailer.c: static void format_trailer_info(struct strbuf *out,
    + 
    + 	/* If we want the whole block untouched, we can take the fast path. */
    + 	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
    +-	    !opts->separator && !opts->value_only) {
    ++	    !opts->separator && !opts->key_only && !opts->value_only) {
    + 		strbuf_add(out, info->trailer_start,
    + 			   info->trailer_end - info->trailer_start);
    + 		return;
    +@@ trailer.c: static void format_trailer_info(struct strbuf *out,
      				if (opts->separator && out->len != origlen)
      					strbuf_addbuf(out, opts->separator);
      				if (!opts->value_only)
     -					strbuf_addf(out, "%s: ", tok.buf);
     -				strbuf_addbuf(out, &val);
    -+					strbuf_addstr(out, tok.buf);
    ++					strbuf_addbuf(out, &tok);
     +				if (!opts->key_only && !opts->value_only)
     +					strbuf_addstr(out, ": ");
     +				if (!opts->key_only)
    @@ trailer.h: struct process_trailer_options {
      	int no_divider;
     +	int key_only;
      	int value_only;
    - 	int canonicalize;
      	const struct strbuf *separator;
    -+	const struct strbuf *key_value_separator;
    - 	int (*filter)(const struct strbuf *, const char *alias, void *);
    - 	void *filter_data;
    - };
    + 	int (*filter)(const struct strbuf *, void *);
5:  cd4b3b52cf3 ! 5:  6cc6fc79388 pretty format %(trailers): add a "key_value_separator"
    @@ Metadata
      ## Commit message ##
         pretty format %(trailers): add a "key_value_separator"
     
    -    As noted in a previous commit which added "keyonly" it's needlessly
    -    hard to use the "log" machinery to produce machine-readable output for
    -    %(trailers). with the combination of the existing "separator" and this
    -    new "key_value_separator" this becomes trivial, as seen by the test
    -    being added here.
    +    Add a "key_value_separator" option to the "%(trailers)" pretty format,
    +    to go along with the existing "separator" argument. In combination
    +    these two options make it trivial to produce machine-readable (e.g. \0
    +    and \0\0-delimited) format output.
    +
    +    As elaborated on in a previous commit which added "keyonly" it was
    +    needlessly tedious to extract structured data from "%(trailers)"
    +    before the addition of this "key_value_separator" option. As seen by
    +    the test being added here extracting this data now becomes trivial.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Documentation/pretty-formats.txt ##
    -@@ Documentation/pretty-formats.txt: multiple times the last occurance wins.
    +@@ Documentation/pretty-formats.txt: option is given with no value, it's enabled.
         `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
    - ** 'keyonly[=bool]': only show the key part of the trailer.
    - ** 'valueonly[=bool]': only show the value part of the trailer.
    + ** 'keyonly[=BOOL]': only show the key part of the trailer.
    + ** 'valueonly[=BOOL]': only show the value part of the trailer.
     +** 'key_value_separator=<SEP>': specify a separator inserted between
     +   trailer lines. When this option is not given each trailer key-value
    -+   pair separated by ": ". Otherwise it shares the same semantics as 
    -+   'separator=<SEP>' above.
    ++   pair is separated by ": ". Otherwise it shares the same semantics
    ++   as 'separator=<SEP>' above.
      
      NOTE: Some placeholders may depend on other options given to the
      revision traversal engine. For example, the `%g*` reflog options will
    @@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
      					   !match_placeholder_bool_arg(arg, "keyonly", &arg, &opts.key_only) &&
     
      ## t/t4205-log-pretty-formats.sh ##
    -@@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers:separator) changes separator' '
    +@@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa
      	test_cmp expect actual
      '
      
     +test_expect_success 'pretty format %(trailers:key_value_separator) changes key-value separator' '
    ++	git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00)X" >actual &&
    ++	(
    ++		printf "XSigned-off-by\0A U Thor <author@example.com>\n" &&
    ++		printf "Acked-by\0A U Thor <author@example.com>\n" &&
    ++		printf "[ v2 updated patch description ]\n" &&
    ++		printf "Signed-off-by\0A U Thor\n  <author@example.com>\nX"
    ++	) >expect &&
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_success 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' '
     +	git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00,unfold)X" >actual &&
     +	(
     +		printf "XSigned-off-by\0A U Thor <author@example.com>\n" &&
    @@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers:sep
     
      ## trailer.c ##
     @@ trailer.c: static void format_trailer_info(struct strbuf *out,
    + 
    + 	/* If we want the whole block untouched, we can take the fast path. */
    + 	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
    +-	    !opts->separator && !opts->key_only && !opts->value_only) {
    ++	    !opts->separator && !opts->key_only && !opts->value_only &&
    ++	    !opts->key_value_separator) {
    + 		strbuf_add(out, info->trailer_start,
    + 			   info->trailer_end - info->trailer_start);
    + 		return;
    +@@ trailer.c: static void format_trailer_info(struct strbuf *out,
      					strbuf_addbuf(out, opts->separator);
      				if (!opts->value_only)
    - 					strbuf_addstr(out, tok.buf);
    + 					strbuf_addbuf(out, &tok);
     -				if (!opts->key_only && !opts->value_only)
     -					strbuf_addstr(out, ": ");
     +				if (!opts->key_only && !opts->value_only) {
    @@ trailer.c: static void format_trailer_info(struct strbuf *out,
      				if (!opts->key_only)
      					strbuf_addbuf(out, &val);
      				if (!opts->separator)
    +
    + ## trailer.h ##
    +@@ trailer.h: struct process_trailer_options {
    + 	int key_only;
    + 	int value_only;
    + 	const struct strbuf *separator;
    ++	const struct strbuf *key_value_separator;
    + 	int (*filter)(const struct strbuf *, void *);
    + 	void *filter_data;
    + };
-- 
2.29.2.222.g5d2a92d10f8