Re: [PATCH 08/15] [GSOC] ref-filter: add cat_file_mode in struct ref_format

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sat, Jul 03 2021, ZheNing Hu wrote:

> Christian Couder <christian.couder@gmail.com> 于2021年7月3日周六 上午6:11写道：
>>
>> On Fri, Jul 2, 2021 at 9:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>> >
>> > Aside from the potential maintenance burden of the `atom_type >= ...
>> > && atom_type <= ...` approach, another problem is that it increases
>> > cognitive load. The reader has to go reference the enum to fully
>> > understand the cases to which this code applies. On the other hand,
>> > the way the patch mentions the enumeration items explicitly, it is
>> > obvious at-a-glance to which cases the code applies. An additional
>> > downside of the suggestion is that the range specified by `>=` and
>> > `<=` may cause some readers to think that there is some sort of
>> > implicit relationship between the items in the range, which doesn't
>> > seem to be the case. So, I find the way it's done in the patch
>> > presently easier to comprehend.
>>
>> I agree that it's less cognitive load, but maybe it could be improved
>> using a separate function like:
>>
>> static int reject_atom(int cat_file_mode, enum atom_type atom_type)
>> {
>>     if (!cat_file_mode)
>>         return atom_type == ATOM_REST;
>>
>>     /* cat_file_mode */
>>     switch (atom_type) {
>>     case ATOM_FLAG:
>>     case ATOM_HEAD:
>>     case ATOM_PUSH:
>>     case ATOM_REFNAME:
>>     case ATOM_SYMREF:
>>     case ATOM_UPSTREAM:
>>     case ATOM_WORKTREEPATH:
>>         return 1;
>>     default:
>>         return 0;
>>     }
>> }
>
> I agree. It is clearer to use a function to wrap origin reject atoms step.
>
> Thanks.

Yeah I guess you're both right, I just found the dense giant if to be
hard to read, but having a helper is even better.