Re: [PATCH 3/5] pretty format %(trailers): add a "keyonly"

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 5, 2020, 12:26 UTC
Message-ID: <87mtysxwu6.fsf@evledraar.gmail.com>
In-Reply-To: <CAP8UFD1gTOKLs55ceVwsDW=uSyW4wx_9eF9Wra5KVP8B19Jx_Q@mail.gmail.com>

On Sat, Dec 05 2020, Christian Couder wrote:

Show 36 quoted lines

> On Sat, Dec 5, 2020 at 2:39 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>> 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.
>
> Well explained.
>
>> diff --git a/trailer.c b/trailer.c
>> index b00b35ea0eb..40f31e4dfc2 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -1233,8 +1233,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_addstr(out, tok.buf);
>
> Maybe `strbuf_addbuf(out, &tok);`

Much better, thanks.

Show 8 quoted lines

>> +                               if (!opts->key_only && !opts->value_only)
>> +                                       strbuf_addstr(out, ": ");
>> +                               if (!opts->key_only)
>> +                                       strbuf_addbuf(out, &val);
>
> The above is probably correct, but it feels strange to write the key
> after the separator and the value, and that the key is in a variable
> called "val".

We write them in the order "key -> sep -> value". with the logic of:
    
    if (!no_key)
        write_key();
    if (!no_sep)
        write_sep();
    if (!no_value)
        write_value();

So the &val here really is the value part.