| Message ID | 20181118114427.1397-3-anders@0x63.nu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | %(trailers) improvements in pretty format | expand |
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.
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.
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
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.
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
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
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}
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(-)