From 7c1f79fc16115cc971de571e4203ca78984352a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Sun, 6 Dec 2020 01:24:45 +0100 Subject: [PATCH 1/5] pretty format %(trailers) test: split a long line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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, especially as follow-up commits will copy this test as a template. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/t4205-log-pretty-formats.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 204c149d5a..5e5452212d 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -717,7 +717,12 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' ' test_expect_success 'pretty format %(trailers:separator) changes separator' ' git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual && - printf "XSigned-off-by: A U Thor \0Acked-by: A U Thor \0[ v2 updated patch description ]\0Signed-off-by: A U Thor X" >expect && + ( + printf "XSigned-off-by: A U Thor \0" && + printf "Acked-by: A U Thor \0" && + printf "[ v2 updated patch description ]\0" && + printf "Signed-off-by: A U Thor X" + ) >expect && test_cmp expect actual ' From 2762e17117fdd7ab73eb3f2970620a6f91500d01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 9 Dec 2020 16:52:05 +0100 Subject: [PATCH 2/5] pretty format %(trailers) doc: avoid repetition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 general documentation at the top. It then suffices to refer to it by listing the options as "opt[=]". I'm also changing it to upper-case "[=]" from "[=val]" for consistency with "" It took me a couple of readings to realize that these options were referring back to the "only" option's treatment of boolean values. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- Documentation/pretty-formats.txt | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 84bbc7439a..973b6c7d48 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -252,7 +252,15 @@ endif::git-rev-list[] interpreted by linkgit:git-interpret-trailers[1]. The `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" +sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean +option is given with no value, it's enabled. ++ ** 'key=': 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 @@ -261,27 +269,21 @@ endif::git-rev-list[] desired it can be disabled with `only=false`. E.g., `%(trailers:key=Reviewed-by)` shows trailer lines with key `Reviewed-by`. -** 'only[=val]': select whether non-trailer lines from the trailer - block should be included. The `only` keyword may optionally be - followed by an equal sign and one of `true`, `on`, `yes` to omit or - `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[=]': select whether non-trailer lines from the trailer + block should be included. ** 'separator=': specify a separator inserted between trailer lines. When this option is not given each trailer line is terminated with a line feed character. The string SEP may contain the literal formatting codes described above. To use comma as separator one must use `%x2C` as it would otherwise be parsed as - next option. If separator option is given multiple times only the - last one is used. E.g., `%(trailers:key=Ticket,separator=%x2C )` + next option. E.g., `%(trailers:key=Ticket,separator=%x2C )` shows all trailer lines whose key is "Ticket" separated by a comma and a space. -** '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[=]': 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[=]': 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 revision traversal engine. For example, the `%g*` reflog options will From 8b966a0506f8b5aef7c6038fe518db45bc4a1852 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 9 Dec 2020 16:52:06 +0100 Subject: [PATCH 3/5] pretty-format %(trailers): fix broken standalone "valueonly" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix %(trailers:valueonly) being a noop due to on overly eager optimization in format_trailer_info() which skips custom formatting if no custom options are given. 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. 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. 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. 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 Signed-off-by: Junio C Hamano --- t/t4205-log-pretty-formats.sh | 28 ++++++++++++++++++++++++++++ trailer.c | 3 ++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 5e5452212d..cb09a13249 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -605,6 +605,12 @@ 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 && { @@ -715,7 +721,29 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' ' test_cmp expect actual ' +test_expect_success '%(trailers:valueonly) shows only values' ' + git log --no-walk --pretty="format:%(trailers:valueonly)" >actual && + test_write_lines \ + "A U Thor " \ + "A U Thor " \ + "[ v2 updated patch description ]" \ + "A U Thor" \ + " " >expect && + test_cmp expect actual +' + 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 \0" && + printf "Acked-by: A U Thor \0" && + printf "[ v2 updated patch description ]\0" && + printf "Signed-off-by: A U Thor\n 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 \0" && diff --git a/trailer.c b/trailer.c index 3f7391d793..d2d01015b1 100644 --- a/trailer.c +++ b/trailer.c @@ -1131,7 +1131,8 @@ 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 && !opts->filter && !opts->separator) { + 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; From 9d87d5ae026433a2993fdb757fc80dbdcb078310 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 9 Dec 2020 16:52:07 +0100 Subject: [PATCH 4/5] pretty format %(trailers): add a "keyonly" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add support for a "keyonly". This allows for easier parsing out of the key and value. Before if you didn't want to make assumptions about how the key was formatted. You'd need to parse it out as e.g.: --pretty=format:'%H%x00%(trailers:separator=%x00%x00)' \ '%x00%(trailers:separator=%x00%x00,valueonly)' And then proceed to deduce keys by looking at those two and subtracting the value plus the hardcoded ": " separator from the non-valueonly %(trailers) line. Now it's possible to simply do: --pretty=format:'%H%x00%(trailers:separator=%x00%x00,keyonly)' \ '%x00%(trailers:separator=%x00%x00,valueonly)' Which at least reduces it to a state machine where you get N keys and correlate them with N values. Even better would be to have a way to 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 Signed-off-by: Junio C Hamano --- Documentation/pretty-formats.txt | 4 ++-- pretty.c | 1 + t/t4205-log-pretty-formats.sh | 31 ++++++++++++++++++++++++++++++- trailer.c | 9 ++++++--- trailer.h | 1 + 5 files changed, 40 insertions(+), 6 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 973b6c7d48..5eac36500d 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -282,8 +282,8 @@ option is given with no value, it's enabled. ** 'unfold[=]': 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[=]': skip over the key part of the trailer line and only - show the value part. +** 'keyonly[=]': only show the key part of the trailer. +** 'valueonly[=]': 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 diff --git a/pretty.c b/pretty.c index 7a7708a0ea..1237ee0e45 100644 --- a/pretty.c +++ b/pretty.c @@ -1451,6 +1451,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ opts.separator = &sepbuf; } else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) && + !match_placeholder_bool_arg(arg, "keyonly", &arg, &opts.key_only) && !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only)) break; } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index cb09a13249..4c9f6eb794 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -715,6 +715,22 @@ test_expect_success '%(trailers:key) without value is error' ' test_cmp expect actual ' +test_expect_success '%(trailers:keyonly) shows only keys' ' + git log --no-walk --pretty="format:%(trailers:keyonly)" >actual && + test_write_lines \ + "Signed-off-by" \ + "Acked-by" \ + "[ v2 updated patch description ]" \ + "Signed-off-by" >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:key=foo,keyonly) shows only key' ' + git log --no-walk --pretty="format:%(trailers:key=Acked-by,keyonly)" >actual && + echo "Acked-by" >expect && + test_cmp expect actual +' + 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 " >expect && @@ -732,6 +748,12 @@ test_expect_success '%(trailers:valueonly) shows only values' ' 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_cmp expect actual +' + test_expect_success 'pretty format %(trailers:separator) changes separator' ' git log --no-walk --pretty=format:"X%(trailers:separator=%x00)X" >actual && ( @@ -754,7 +776,7 @@ test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa test_cmp expect actual ' -test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' ' +test_expect_success 'pretty format %(trailers) combining separator/key/keyonly/valueonly' ' git commit --allow-empty -F - <<-\EOF && Important fix @@ -781,6 +803,13 @@ test_expect_success 'pretty format %(trailers) combining separator/key/valueonly "Does not close any tickets" \ "Another fix #567, #890" \ "Important fix #1234" >expect && + test_cmp expect actual && + + git log --pretty="%s% (trailers:separator=%x2c%x20,key=Closes,keyonly)" HEAD~3.. >actual && + test_write_lines \ + "Does not close any tickets" \ + "Another fix Closes, Closes" \ + "Important fix Closes" >expect && test_cmp expect actual ' diff --git a/trailer.c b/trailer.c index d2d01015b1..889b419a4f 100644 --- a/trailer.c +++ b/trailer.c @@ -1132,7 +1132,7 @@ 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; @@ -1154,8 +1154,11 @@ 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_addbuf(out, &tok); + if (!opts->key_only && !opts->value_only) + strbuf_addstr(out, ": "); + if (!opts->key_only) + strbuf_addbuf(out, &val); if (!opts->separator) strbuf_addch(out, '\n'); } diff --git a/trailer.h b/trailer.h index cd93e7ddea..d2f28776be 100644 --- a/trailer.h +++ b/trailer.h @@ -71,6 +71,7 @@ struct process_trailer_options { int only_input; int unfold; int no_divider; + int key_only; int value_only; const struct strbuf *separator; int (*filter)(const struct strbuf *, void *); From 058761f1c19b58afd6906672fc013327eb2096c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 9 Dec 2020 16:52:08 +0100 Subject: [PATCH 5/5] pretty format %(trailers): add a "key_value_separator" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- Documentation/pretty-formats.txt | 4 ++++ pretty.c | 9 +++++++++ t/t4205-log-pretty-formats.sh | 33 ++++++++++++++++++++++++++++++++ trailer.c | 11 ++++++++--- trailer.h | 1 + 5 files changed, 55 insertions(+), 3 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 5eac36500d..6b59e28d44 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -284,6 +284,10 @@ option is given with no value, it's enabled. `%(trailers:only,unfold=true)` unfolds and shows all trailer lines. ** 'keyonly[=]': only show the key part of the trailer. ** 'valueonly[=]': only show the value part of the trailer. +** 'key_value_separator=': specify a separator inserted between + trailer lines. When this option is not given each trailer key-value + pair is separated by ": ". Otherwise it shares the same semantics + as 'separator=' above. 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 1237ee0e45..05eef7fda0 100644 --- a/pretty.c +++ b/pretty.c @@ -1418,6 +1418,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; struct string_list filter_list = STRING_LIST_INIT_NODUP; struct strbuf sepbuf = STRBUF_INIT; + struct strbuf kvsepbuf = STRBUF_INIT; size_t ret = 0; opts.no_divider = 1; @@ -1449,6 +1450,14 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ strbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL); free(fmt); opts.separator = &sepbuf; + } else if (match_placeholder_arg_value(arg, "key_value_separator", &arg, &argval, &arglen)) { + char *fmt; + + strbuf_reset(&kvsepbuf); + fmt = xstrndup(argval, arglen); + strbuf_expand(&kvsepbuf, fmt, strbuf_expand_literal_cb, NULL); + free(fmt); + opts.key_value_separator = &kvsepbuf; } else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) && !match_placeholder_bool_arg(arg, "keyonly", &arg, &opts.key_only) && diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 4c9f6eb794..749bc1431a 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -776,6 +776,39 @@ 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 \n" && + printf "Acked-by\0A U Thor \n" && + printf "[ v2 updated patch description ]\n" && + printf "Signed-off-by\0A U Thor\n \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 \n" && + printf "Acked-by\0A U Thor \n" && + printf "[ v2 updated patch description ]\n" && + printf "Signed-off-by\0A U Thor \nX" + ) >expect && + test_cmp expect actual +' + +test_expect_success 'pretty format %(trailers:separator,key_value_separator) changes both separators' ' + git log --no-walk --pretty=format:"%(trailers:separator=%x00,key_value_separator=%x00%x00,unfold)" >actual && + ( + printf "Signed-off-by\0\0A U Thor \0" && + printf "Acked-by\0\0A U Thor \0" && + printf "[ v2 updated patch description ]\0" && + printf "Signed-off-by\0\0A U Thor " + ) >expect && + test_cmp expect actual +' + test_expect_success 'pretty format %(trailers) combining separator/key/keyonly/valueonly' ' git commit --allow-empty -F - <<-\EOF && Important fix diff --git a/trailer.c b/trailer.c index 889b419a4f..249ed618ed 100644 --- a/trailer.c +++ b/trailer.c @@ -1132,7 +1132,8 @@ 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; @@ -1155,8 +1156,12 @@ static void format_trailer_info(struct strbuf *out, strbuf_addbuf(out, opts->separator); if (!opts->value_only) strbuf_addbuf(out, &tok); - if (!opts->key_only && !opts->value_only) - strbuf_addstr(out, ": "); + if (!opts->key_only && !opts->value_only) { + if (opts->key_value_separator) + strbuf_addbuf(out, opts->key_value_separator); + else + strbuf_addstr(out, ": "); + } if (!opts->key_only) strbuf_addbuf(out, &val); if (!opts->separator) diff --git a/trailer.h b/trailer.h index d2f28776be..795d2fccfd 100644 --- a/trailer.h +++ b/trailer.h @@ -74,6 +74,7 @@ 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; };