On Fri, Jul 2, 2021 at 9:28 PM Eric Sunshine [off-list ref] 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;
    }
}