Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

["Оля Тележная" <olyatelezhnaya@gmail.com>]

2018-01-16 0:42 GMT+03:00 Jeff King <peff@peff.net>:
> On Wed, Jan 10, 2018 at 09:36:41AM +0000, Olga Telezhnaya wrote:
>
>> Make valid_atom as a function parameter,
>> there could be another variable further.
>> Need that for further reusing of formatting logic in cat-file.c.
>>
>> We do not need to allow users to pass their own valid_atom variable in
>> global functions like verify_ref_format because in the end we want to
>> have same set of valid atoms for all commands. But, as a first step
>> of migrating, I create further another version of valid_atom
>> for cat-file.
>
> I agree in the end we'd want a single valid_atom list. It doesn't look
> like we hit that end state in this series, though.
>
> I guess I'm not quite clear on why we're not adding these new atoms to
> ref-filter (and for-each-ref) right away, though. We already have the
> first three (name, type, and size), and we'd just need to support
> %(rest) and %(deltabase).
>
> I think %(rest) doesn't really make sense for for-each-ref (we're not
> reading any input), but it could expand to the empty string by default
> (or even throw an error if the caller asks us not to support it).
>
> IOW, the progression I'd expect in a series like this is:
>
>   1. Teach ref-filter.c to support everything that cat-file can do.
>
>   2. Convert cat-file to use ref-filter.c.
>
> -Peff

I agree, I even made this and it's working fine:
https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
But I decided not to add that to patch because I expand the
functionality of several commands (not only cat-file and
for-each-ref), and I need to support all new functionality in a proper
way, make these error messages, test everything and - the hardest one
- support many new commands for cat-file. As I understand, it is not
possible unless we finally move to ref-filter and print results also
there. Oh, and I also need to rewrite docs in that case. And I decided
to apply this in another patch. But, please, say your opinion, maybe
we could do that here in some way.

Olga