On Sat, Jul 03 2021, ZheNing Hu wrote:

Christian Couder [off-list ref] 于2021年7月3日周六 上午6:11写道：

quoted

On Fri, Jul 2, 2021 at 9:28 PM Eric Sunshine [off-list ref] wrote:

quoted

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.