| Message ID | 20201205013918.18981-5-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Sat, Dec 5, 2020 at 2:39 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > Fix %(trailers:valueonly) being a noop due to on overly eager s/on/an/ > 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). It seems that you mean this part of the above patch: /* 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) { but this could perhaps be clearer with: "When new trailer options were added, a check that the new option is not used needed to be added at the start of the format_trailer_info() function to see if we could take the fast path of writing the whole trailer as is." instead of: "When new trailer options were added they needed to be listed at the start of the format_trailer_info() function." > When d9b936db522 (pretty: add support for "valueonly" option in > %(trailers), 2019-01-28) was added this was omitted by mistake. Maybe: s/was added this was/was added, this check was/ > 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. > > 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. s/to add new flags/to add a new check for each new option/ > 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. Yeah, I think so too. I wonder if it is worth checking that "%(trailers:)" still works in the same way as "%(trailers)" though. > struct process_trailer_options { > + int have_options; > int in_place; > int trim_empty; > int only_trailers; If all of these are booleans, we might want to save a few bytes at one point by using bit fields.
diff --git a/pretty.c b/pretty.c index 590f37489f6..d989a6ae712 100644 --- a/pretty.c +++ b/pretty.c @@ -1426,6 +1426,8 @@ 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; diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 5dd080c19b2..e1100082b34 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -737,6 +737,17 @@ 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 <author@example.com>" \ + "A U Thor <author@example.com>" \ + "[ v2 updated patch description ]" \ + "A U Thor" \ + " <author@example.com>" >expect && + 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 && diff --git a/trailer.c b/trailer.c index 40f31e4dfc2..da95e1f3c66 100644 --- a/trailer.c +++ b/trailer.c @@ -1206,8 +1206,7 @@ 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 (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) { + if (!opts->have_options) { strbuf_add(out, info->trailer_start, info->trailer_end - info->trailer_start); return; diff --git a/trailer.h b/trailer.h index d4507b4ef2a..e348c970ce7 100644 --- a/trailer.h +++ b/trailer.h @@ -65,6 +65,7 @@ struct new_trailer_item { }; struct process_trailer_options { + int have_options; int in_place; int trim_empty; int only_trailers;
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). 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. 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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- pretty.c | 2 ++ t/t4205-log-pretty-formats.sh | 11 +++++++++++ trailer.c | 3 +-- trailer.h | 1 + 4 files changed, 15 insertions(+), 2 deletions(-)