Re: [PATCH v3 04/23] ref-filter: make valid_atom as function parameter

[Оля Тележная]

2018-02-15 8:23 GMT+03:00 Jeff King :
> On Mon, Feb 12, 2018 at 08:08:54AM +, 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.
>
> Hmm. So I see where you're going here, but I think in the end we'd want
> to have a single valid_atom list again, and we wouldn't need this.
>
> And indeed, it looks like this goes away in patch 17. Can we
> reorganize/rebase the series so that we avoid dead-end directions like
> this?

I tried to do that, but in my opinion it's easier to understand
everything in current version. I need to squash too many items so that
commits do not look atomic, and it's really hard to understand what is
going on. Now we have that helper commit that will be cancelled later,
and other logic is more clear for readers.

>
> -Peff

Re: [PATCH v3 04/23] ref-filter: make valid_atom as function parameter

[Jeff King]

On Mon, Feb 12, 2018 at 08:08:54AM +, 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.

Hmm. So I see where you're going here, but I think in the end we'd want
to have a single valid_atom list again, and we wouldn't need this.

And indeed, it looks like this goes away in patch 17. Can we
reorganize/rebase the series so that we avoid dead-end directions like
this?

-Peff