@matheusbsbs / matheus-bagliano / commits / 8b966a0506

pretty-format %(trailers): fix broken standalone "valueonly"

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 <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Dec 9, 2020 at 16:52 UTC 8b966a0506f8b5aef7c6038fe518db45bc4a1852
2 files changed +30 -1
t/t4205-log-pretty-formats.sh
+28
@@ -605,6 +605,12 @@ test_expect_success 'pretty format %(trailers) shows trailers' '
605 test_cmp expect actual
606 '
607
608 +test_expect_success 'pretty format %(trailers:) enables no options' '
609 + git log --no-walk --pretty="%(trailers:)" >actual &&
610 + # "expect" the same as the test above
611 + test_cmp expect actual
612 +'
613 +
614 test_expect_success '%(trailers:only) shows only "key: value" trailers' '
615 git log --no-walk --pretty="%(trailers:only)" >actual &&
616 {
@@ -715,7 +721,29 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
721 test_cmp expect actual
722 '
723
724 +test_expect_success '%(trailers:valueonly) shows only values' '
725 + git log --no-walk --pretty="format:%(trailers:valueonly)" >actual &&
726 + test_write_lines \
727 + "A U Thor <author@example.com>" \
728 + "A U Thor <author@example.com>" \
729 + "[ v2 updated patch description ]" \
730 + "A U Thor" \
731 + " <author@example.com>" >expect &&
732 + test_cmp expect actual
733 +'
734 +
735 test_expect_success 'pretty format %(trailers:separator) changes separator' '
736 + git log --no-walk --pretty=format:"X%(trailers:separator=%x00)X" >actual &&
737 + (
738 + printf "XSigned-off-by: A U Thor <author@example.com>\0" &&
739 + printf "Acked-by: A U Thor <author@example.com>\0" &&
740 + printf "[ v2 updated patch description ]\0" &&
741 + printf "Signed-off-by: A U Thor\n <author@example.com>X"
742 + ) >expect &&
743 + test_cmp expect actual
744 +'
745 +
746 +test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separator' '
747 git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual &&
748 (
749 printf "XSigned-off-by: A U Thor <author@example.com>\0" &&
trailer.c
+2 -1
@@ -1131,7 +1131,8 @@ static void format_trailer_info(struct strbuf *out,
1131 size_t i;
1132
1133 /* If we want the whole block untouched, we can take the fast path. */
1134 - if (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) {
1134 + if (!opts->only_trailers && !opts->unfold && !opts->filter &&
1135 + !opts->separator && !opts->value_only) {
1136 strbuf_add(out, info->trailer_start,
1137 info->trailer_end - info->trailer_start);
1138 return;