Re: [PATCH 4/5] pretty-format %(trailers): fix broken standalone "valueonly"

From: Christian Couder <christian.couder@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>,
	Anders Waldenborg <anders@0x63.nu>, Jeff King <peff@peff.net>,
	Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH 4/5] pretty-format %(trailers): fix broken standalone "valueonly"
Date: Sat, 5 Dec 2020 07:46:09 +0100	[thread overview]
Message-ID: <CAP8UFD0iXTLRc9yeyT10w8e7s-8ygPh3A645ss=gXvBrK+D5pQ@mail.gmail.com> (raw)
In-Reply-To: <20201205013918.18981-5-avarab@gmail.com>

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.