On Fri, Jul 2, 2021 at 9:33 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
> On Thu, Jul 01 2021, ZheNing Hu via GitGitGadget wrote:
> > + if ((!format->cat_file_mode && used_atom[at].atom_type == ATOM_REST) ||
> > + (format->cat_file_mode && (used_atom[at].atom_type == ATOM_FLAG ||
> > + used_atom[at].atom_type == ATOM_HEAD ||
> > + used_atom[at].atom_type == ATOM_PUSH ||
> > + used_atom[at].atom_type == ATOM_REFNAME ||
> > + used_atom[at].atom_type == ATOM_SYMREF ||
> > + used_atom[at].atom_type == ATOM_UPSTREAM ||
> > + used_atom[at].atom_type == ATOM_WORKTREEPATH)))
> > + die(_("this command reject atom %%(%.*s)"), (int)(ep - sp - 2), sp + 2);
>
> Maybe it was brought up already, but those ATOM_* are mostly continuous
> where they're declared, Maybe worth making it so having a comment like:
>
> /* These need to be in order, used in verify_ref_format */
>
> And then just doing:
>
> used_atom[at].atom_type >= FIRST_OF_THOSE &&
> used_atom[at].atom_type >= LAST_OF_THOSE
>
> Maybe it's too magical and peeking under the hood of an enum...
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.
