On Sat, Dec 5, 2020 at 2:39 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
