[v3,2/5] pretty: allow showing specific trailers
diff mbox series

Message ID 20181118114427.1397-3-anders@0x63.nu
State New
Headers show
Series
  • %(trailers) improvements in pretty format
Related show

Commit Message

Anders Waldenborg Nov. 18, 2018, 11:44 a.m. UTC
Adds a new "key=X" option to "%(trailers)" which will cause it to only
print trailers lines which match the specified key.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt | 17 +++++++++------
 pretty.c                         | 31 ++++++++++++++++++++++++++-
 t/t4205-log-pretty-formats.sh    | 36 ++++++++++++++++++++++++++++++++
 trailer.c                        |  8 ++++---
 trailer.h                        |  2 ++
 5 files changed, 84 insertions(+), 10 deletions(-)

Comments

Junio C Hamano Nov. 20, 2018, 5:45 a.m. UTC | #1
Anders Waldenborg <anders@0x63.nu> writes:

> +  followed by a colon and zero or more comma-separated options:
> +  ** 'only': omit non-trailer lines from the trailer block.
> +  ** 'unfold': make it behave as if interpret-trailer's `--unfold`
> +     option was given.
> +  ** 'key=<K>': only show trailers with specified key. Matching is
> +     done case-insensitively and trailing colon is optional. If option
> +     is given multiple times only last one is used.
> +  ** Examples: `%(trailers:only,unfold)` unfolds and shows all trailer
> +     lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows
> +     trailer lines with key `Reviewed-by`.

The last "Examples" item does not logically belong to the other
three, which bothers me a bit.

> diff --git a/pretty.c b/pretty.c
> index aa03d5b23..aaedc8447 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1074,6 +1074,17 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,
>  	return 0;
>  }
>  
> +struct format_trailer_match_data {
> +	const char *trailer;
> +	size_t trailer_len;
> +};
> +
> +static int format_trailer_match_cb(const struct strbuf *sb, void *ud)
> +{
> +	struct format_trailer_match_data *data = ud;
> +	return data->trailer_len == sb->len && !strncasecmp (data->trailer, sb->buf, sb->len);
> +}

>  	if (skip_prefix(placeholder, "(trailers", &arg)) {
>  		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
> +		struct format_trailer_match_data filter_data;
>  		size_t ret = 0;
>  
>  		opts.no_divider = 1;
> @@ -1323,7 +1335,24 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>  					opts.only_trailers = 1;
>  				else if (match_placeholder_arg(arg, "unfold", &arg))
>  					opts.unfold = 1;
> -				else
> +				else if (skip_prefix(arg, "key=", &arg)) {
> +					const char *end = arg + strcspn(arg, ",)");
> +
> +					filter_data.trailer = arg;
> +					filter_data.trailer_len = end - arg;
> +
> +					if (filter_data.trailer_len &&
> +					    filter_data.trailer[filter_data.trailer_len - 1] == ':')
> +						filter_data.trailer_len--;
> +
> +					opts.filter = format_trailer_match_cb;
> +					opts.filter_data = &filter_data;
> +					opts.only_trailers = 1;
> +
> +					arg = end;
> +					if (*arg == ',')
> +						arg++;
> +				} else
>  					break;
>  			}

This is part of a loop that is entered after seeing "%(trailers:"
and existing one looks for 'unfold' and 'only' by using the
match_placeholder_arg() helper, which

 - returns false if the keyword is not what is being sought after;
 - returns 1 if it finds the keyword, followed by ',' or ')', and
   updates the end pointer to point at either the closing ')' or one
   place after the ','.

The new part cannot directly reuse the same helper because it
expects some non-constant string after "key=", but shouldn't we be
introducing a similar helper with extended feature to help this part
of the code to stay readable?  Exposing the minute details of the
logic to parse "key=<value>,..." while hiding the counterpart to
parse "(only|unfold),..." makes the implementation of the above loop
uneven and hard to follow.

I wonder if a helper like this would help:

static int match_placeholder(const char *to_parse, const char *keyword,
			     const char **value, size_t *valuelen,
			     const char **end)
{
	const char *p;

	if (!(skip_prefix(to_parse, keyword, &p)))
		return 0;

	if (value && valuelen) {
		/* expect "<keyword>=<value>" */
		size_t vlen;
		if (*p++ != '=')
			return 1;
		vlen = strcspn(p, ",)");
		if (!p[vlen])
			return 0;
		*value = p;
		*valuelen = vlen;
		p = p + vlen;
	}

	if (*p == ',') {
		*end = p + 1;
		return 1;
	}
	if (*p == ')') {
		*end = p;
		return 1;
	}
	return 0;
}

which would allow the existing one to become a thin wrapper

static int match_placeholder_arg(const char *a, const char *b, const char **c)
{
	return match_placeholder(a, b, NULL, NULL, c);
}

or can simply be eliminated by updating its only two callsites.

In the version you wrote, it is not clear what would happen if the
format string ends with "%(trailers:key=" (no value, no comma, not
even the closing paren).  I do not think it would fall into infinite
loop, but I do not think the code structure without the helper that
makes the loop's logic uniform would allow it to properly diagnose
such a malformed string.
Junio C Hamano Nov. 20, 2018, 5:59 a.m. UTC | #2
Anders Waldenborg <anders@0x63.nu> writes:

> +  followed by a colon and zero or more comma-separated options:
> +  ** 'only': omit non-trailer lines from the trailer block.
> +  ** 'unfold': make it behave as if interpret-trailer's `--unfold`
> +     option was given.
> +  ** 'key=<K>': only show trailers with specified key. Matching is
> +     done case-insensitively and trailing colon is optional. If option
> +     is given multiple times only last one is used.

It would be good to allow multiple keys, as

	%(trailers:key=signed-off-by,key=helped-by)

and

	%(trailers:key=signed-off-by)%(trailers:key=helped-by)

would mean quite a different thing.  The former can preserve the
order of these sign-offs and helped-bys in the original, while the
latter would have to show all the sign-offs before showing the
helped-bys, and I am not convinced if the latter is the only valid
use case.

Also, use of 'key=' automatically turns on 'only' as described, and
I tend to agree that it would a convenient default mode (i.e. when
picking certain trailers only with this mechanism, it is likely that
the user is willing to use %(subject) etc. to fill in what was lost
by the implicit use of 'only'), but at the same time, it makes me
wonder if we need to have a way to countermand an 'only' (or
'unfold' for that matter) that was given earlier, e.g.

	%(trailers:key=signed-off-by,only=no)

Thanks.
Anders Waldenborg Nov. 25, 2018, 11:02 p.m. UTC | #3
Junio C Hamano writes:
> Also, use of 'key=' automatically turns on 'only' as described, and
> I tend to agree that it would a convenient default mode (i.e. when
> picking certain trailers only with this mechanism, it is likely that
> the user is willing to use %(subject) etc. to fill in what was lost
> by the implicit use of 'only'), but at the same time, it makes me
> wonder if we need to have a way to countermand an 'only' (or
> 'unfold' for that matter) that was given earlier, e.g.
>
> 	%(trailers:key=signed-off-by,only=no)
>
> Thanks.

I'm not sure what that would mean. The non-trailer lines in the trailer
block doesn't match the key.

Take this commit as an example:

$ git show -s --pretty=format:'%(trailers)' b4d065df03049bacfbc40467b60b13e804b7d289
Helped-by: Jeff King <peff@peff.net>
[jc: took idea and log message from peff]
Signed-off-by: Junio C Hamano <gitster@pobox.com>

With 'only' it shows:
$ git show -s --pretty=format:'%(trailers:only)' b4d065df03049bacfbc40467b60b13e804b7d289
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

Now with a "key=signed-off-by" option I would imagine that as adding a
"| grep -i '^signed-off-by:'" to the end. In both cases (with and
without 'only') that would give the same result:
"Signed-off-by: Junio C Hamano <gitster@pobox.com>"


 anders
Junio C Hamano Nov. 26, 2018, 3:13 a.m. UTC | #4
Anders Waldenborg <anders@0x63.nu> writes:

> Junio C Hamano writes:
>> Also, use of 'key=' automatically turns on 'only' as described, and
>> I tend to agree that it would a convenient default mode (i.e. when
>> picking certain trailers only with this mechanism, it is likely that
>> the user is willing to use %(subject) etc. to fill in what was lost
>> by the implicit use of 'only'), but at the same time, it makes me
>> wonder if we need to have a way to countermand an 'only' (or
>> 'unfold' for that matter) that was given earlier, e.g.
>>
>> 	%(trailers:key=signed-off-by,only=no)
>>
>> Thanks.
>
> I'm not sure what that would mean. The non-trailer lines in the trailer
> block doesn't match the key.

I was confused by the "only" stuff.

When you give a key (or two), they cannot possibly name non-trailer
lines, so while it may be possible to ask "oh, by the way, I also
want non-trailer lines in addition to signed-off-by and cc lines",
the value of being able to make such a request is dubious.

The value is dubious, but logically it makes it more consistent with
the current %(trailers) that lack 'only', which is "oh by the way, I
also want non-trailer lines in addition to the trailers with
keyword", to allow a way to countermand the 'only' you flip on by
default when keywords are given.
Anders Waldenborg Nov. 26, 2018, 6:56 a.m. UTC | #5
Junio C Hamano writes:
> I was confused by the "only" stuff.
>
> When you give a key (or two), they cannot possibly name non-trailer
> lines, so while it may be possible to ask "oh, by the way, I also
> want non-trailer lines in addition to signed-off-by and cc lines",
> the value of being able to make such a request is dubious.
>
> The value is dubious, but logically it makes it more consistent with
> the current %(trailers) that lack 'only', which is "oh by the way, I
> also want non-trailer lines in addition to the trailers with
> keyword", to allow a way to countermand the 'only' you flip on by
> default when keywords are given.


Would it feel less inconsistent if it did not set the 'only_trailers'
option?

Now that I look at it again setting 'only_trailers' is more of an
implementation trick/hack to make sure it doesn't take the fast-path in
format_trailer_info (and by documenting it it justifies that hack). If
instead the 'filter' option is checked in the relevant places there
would be no need to mix up 'only' with 'filter'.

That is, do you think something like this should be squashed in?

diff --git a/pretty.c b/pretty.c
index 302e67fa8..f45ccaf51 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1360,7 +1360,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */

                                        opts.filter = format_trailer_match_cb;
                                        opts.filter_data = &filter_list;
-                                       opts.only_trailers = 1;
                                } else
                                        break;
                        }
diff --git a/trailer.c b/trailer.c
index 97c8f2762..07ca2b2c6 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out,
        size_t i;

        /* If we want the whole block untouched, we can take the fast path. */
-       if (!opts->only_trailers && !opts->unfold) {
+       if (!opts->only_trailers && !opts->unfold && !opts->filter) {
                strbuf_add(out, info->trailer_start,
                           info->trailer_end - info->trailer_start);
                return;
@@ -1156,7 +1156,7 @@ static void format_trailer_info(struct strbuf *out,
                        strbuf_release(&tok);
                        strbuf_release(&val);

-               } else if (!opts->only_trailers) {
+               } else if (!opts->only_trailers && !opts->filter) {
                        strbuf_addstr(out, trailer);
                }
        }
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 7548e1d38..ea3cd5b28 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -228,9 +228,9 @@ endif::git-rev-list[]
 ** 'key=<K>': only show trailers with specified key. Matching is done
    case-insensitively and trailing colon is optional. If option is
    given multiple times trailer lines matching any of the keys are
-   shown. Non-trailer lines in the trailer block are also hidden
-   (i.e. 'key' implies 'only'). E.g., `%(trailers:key=Reviewed-by)`
-   shows trailer lines with key `Reviewed-by`.
+   shown. Non-trailer lines in the trailer block are also hidden.
+   E.g., `%(trailers:key=Reviewed-by)` shows trailer lines with key
+   `Reviewed-by`.
 ** 'only': omit non-trailer lines from the trailer block.
 ** 'unfold': make it behave as if interpret-trailer's `--unfold`
    option was given. E.g., `%(trailers:only,unfold)` unfolds and
Junio C Hamano Nov. 26, 2018, 7:52 a.m. UTC | #6
Anders Waldenborg <anders@0x63.nu> writes:

> Would it feel less inconsistent if it did not set the 'only_trailers'
> option?

If %(trailers:key=...) did not automatically imply 'only', it would
be very consistent.

But as I already said, I think it would be less convenient, as I do
suspect that those who want specific keys would want to see only
those trailers with specific keys.

And if we want that convinience, we'd probably want a way to
optionally disable that 'only' bit when the user wants to.

And...

> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -228,9 +228,9 @@ endif::git-rev-list[]
>  ** 'key=<K>': only show trailers with specified key. Matching is done
>     case-insensitively and trailing colon is optional. If option is
>     given multiple times trailer lines matching any of the keys are
> -   shown. Non-trailer lines in the trailer block are also hidden
> -   (i.e. 'key' implies 'only'). E.g., `%(trailers:key=Reviewed-by)`
> -   shows trailer lines with key `Reviewed-by`.
> +   shown. Non-trailer lines in the trailer block are also hidden.
> +   E.g., `%(trailers:key=Reviewed-by)` shows trailer lines with key
> +   `Reviewed-by`.

... I do not think this change reduces the puzzlement felt by
readers who would have wondered how that implied 'only' can be
countermanded with the old text.  It just makes it look even less
explained to them.

If we assume that nobody would ever want to mix non-trailers when
asking specific keywords, then "them" in the above paragraph would
become an empty set, and we do not have to worry about them.  I am
not sure if Git is still such a small project to allow us rely on
such an assumption, though.

>  ** 'only': omit non-trailer lines from the trailer block.
>  ** 'unfold': make it behave as if interpret-trailer's `--unfold`
>     option was given. E.g., `%(trailers:only,unfold)` unfolds and

Patch
diff mbox series

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 417b638cd..75c2e838d 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -207,13 +207,18 @@  endif::git-rev-list[]
   than given and there are spaces on its left, use those spaces
 - '%><(<N>)', '%><|(<N>)': similar to '%<(<N>)', '%<|(<N>)'
   respectively, but padding both sides (i.e. the text is centered)
-- %(trailers[:options]): display the trailers of the body as interpreted
+- '%(trailers[:options])': display the trailers of the body as interpreted
   by linkgit:git-interpret-trailers[1]. The `trailers` string may be
-  followed by a colon and zero or more comma-separated options. 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.
+  followed by a colon and zero or more comma-separated options:
+  ** 'only': omit non-trailer lines from the trailer block.
+  ** 'unfold': make it behave as if interpret-trailer's `--unfold`
+     option was given.
+  ** 'key=<K>': only show trailers with specified key. Matching is
+     done case-insensitively and trailing colon is optional. If option
+     is given multiple times only last one is used.
+  ** Examples: `%(trailers:only,unfold)` unfolds and shows all trailer
+     lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows
+     trailer lines with key `Reviewed-by`.
 
 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 aa03d5b23..aaedc8447 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1074,6 +1074,17 @@  static int match_placeholder_arg(const char *to_parse, const char *candidate,
 	return 0;
 }
 
+struct format_trailer_match_data {
+	const char *trailer;
+	size_t trailer_len;
+};
+
+static int format_trailer_match_cb(const struct strbuf *sb, void *ud)
+{
+	struct format_trailer_match_data *data = ud;
+	return data->trailer_len == sb->len && !strncasecmp (data->trailer, sb->buf, sb->len);
+}
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 				const char *placeholder,
 				void *context)
@@ -1312,6 +1323,7 @@  static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 
 	if (skip_prefix(placeholder, "(trailers", &arg)) {
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+		struct format_trailer_match_data filter_data;
 		size_t ret = 0;
 
 		opts.no_divider = 1;
@@ -1323,7 +1335,24 @@  static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					opts.only_trailers = 1;
 				else if (match_placeholder_arg(arg, "unfold", &arg))
 					opts.unfold = 1;
-				else
+				else if (skip_prefix(arg, "key=", &arg)) {
+					const char *end = arg + strcspn(arg, ",)");
+
+					filter_data.trailer = arg;
+					filter_data.trailer_len = end - arg;
+
+					if (filter_data.trailer_len &&
+					    filter_data.trailer[filter_data.trailer_len - 1] == ':')
+						filter_data.trailer_len--;
+
+					opts.filter = format_trailer_match_cb;
+					opts.filter_data = &filter_data;
+					opts.only_trailers = 1;
+
+					arg = end;
+					if (*arg == ',')
+						arg++;
+				} else
 					break;
 			}
 		}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 978a8a66f..aba7ba206 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -598,6 +598,42 @@  test_expect_success ':only and :unfold work together' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by)" >actual &&
+	echo "Acked-by: A U Thor <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key=foo) is case insensitive' '
+	git log --no-walk --pretty="format:%(trailers:key=AcKed-bY)" >actual &&
+	echo "Acked-by: A U Thor <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key=foo:) trailing colon also works' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by:)" >actual &&
+	echo "Acked-by: A U Thor <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=nonexistant) becomes empty' '
+	git log --no-walk --pretty="x%(trailers:key=Nacked-by)x" >actual &&
+	echo "xx" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' '
+	git log --no-walk --pretty="format:%(trailers:key=Signed-Off-by)" >actual &&
+	grep -v patch.description <trailers | grep -v Acked-by >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '
+	git log --no-walk --pretty="format:%(trailers:key=Signed-Off-by,unfold)" >actual &&
+	unfold <trailers | grep Signed-off-by >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'trailer parsing not fooled by --- line' '
 	git commit --allow-empty -F - <<-\EOF &&
 	this is the subject
diff --git a/trailer.c b/trailer.c
index 0796f326b..97c8f2762 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1147,10 +1147,12 @@  static void format_trailer_info(struct strbuf *out,
 			struct strbuf val = STRBUF_INIT;
 
 			parse_trailer(&tok, &val, NULL, trailer, separator_pos);
-			if (opts->unfold)
-				unfold_value(&val);
+			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
+				if (opts->unfold)
+					unfold_value(&val);
 
-			strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+				strbuf_addf(out, "%s: %s\n", tok.buf, val.buf);
+			}
 			strbuf_release(&tok);
 			strbuf_release(&val);
 
diff --git a/trailer.h b/trailer.h
index b99773964..5255b676d 100644
--- a/trailer.h
+++ b/trailer.h
@@ -72,6 +72,8 @@  struct process_trailer_options {
 	int only_input;
 	int unfold;
 	int no_divider;
+	int (*filter)(const struct strbuf *, void *);
+	void *filter_data;
 };
 
 #define PROCESS_TRAILER_OPTIONS_INIT {0}