Re: [PATCH v2 2/5] pretty: allow showing specific trailers

From: Anders Waldenborg <anders@0x63.nu>
Date: Nov 5, 2018, 08:26 UTC
Message-ID: <875zxckk1g.fsf@0x63.nu>
In-Reply-To: <CAPig+cQeUxxvgNGVc_iZ4v0U77obFu6-q0QbtzTJdnEep8eq+Q@mail.gmail.com>

Eric Sunshine writes:

> Should the code tolerate a trailing colon? (Genuine question; it's
> easy to do and would be more user-friendly.)

I would make sense to allow the trailing colon, it is easy enough to just strip that away when reading the argument.

However I'm not sure how that would fit together with the possibility to later lifting it to a regexp, hard to strip a trailing colon from a regexp in a generic way.

> What happens if 'key=...' is specified multiple times?

My first thought was to simply disallow that. But that seemed hard to fit into current model where errors just means don't expand.

I would guess that most useful and intuitive to user would be to handle multiple key arguments by showing any of those keys.

> Thinking further on the last two points, should <val> be a regular expression?

It probably would make sense. I can see how the regexp '^.*-by$' would be useful (but glob style matching would suffice in that case).

Also handling multi-matching with an alternation group would be elegant %(trailers:key="(A|B)"). Except for the fact that the parser would need to understand some kind of quoting, which seems like an major undertaking.

I guess having it as a regular exception would also mean that it needs to get some way to cache the re so it is compiled once, and not for each expansion.

Show 9 quoted lines

>
>> +               free(opts.filter_key);
>
> If I understand correctly, this is making a copy of <val> so that it
> will be NUL-terminated since the code added to trailer.c uses a simple
> strcasecmp() to match it. Would it make sense to avoid the copy by
> adding fields 'opts.filter_key' and 'opts.filter_key_len' and using
> strncasecmp() instead? (Genuine question; not necessarily a request
> for change.)

I'm also not very happy about that copy.

Just using strncasecmp would cause it to be prefix match, no?

But if changing matching semantics to handle multiple key options to something else I'm thinking opts.filter_key would be replaced with a opts.filter callback function, and that part would need to be rewritten anyway.

Show 14 quoted lines

>
>> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
>> @@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' '
>> +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
>> +       git log --no-walk --pretty="%(trailers:key=Acked-by)" >actual &&
>> +       {
>> +               echo "Acked-by: A U Thor <author@example.com>" &&
>> +               echo
>> +       } >expect &&
>> +       test_cmp expect actual
>> +'
>
> I guess these new tests are modeled after one or two existing tests
> which use a series of 'echo' statements

I guess I could change it to "--pretty=format:%(trailers:key=Acked-by)" to get separator semantics and avoid that extra blank line, making it simpler.