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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 2, 2021, 19:28 UTC
Message-ID: <CAPig+cQinUsEZqEDb2_zhi37SdYuBCVEznmFbXPJuFB5cFznKg@mail.gmail.com>
In-Reply-To: <87czs0alhg.fsf@evledraar.gmail.com>

On Fri, Jul 2, 2021 at 9:33 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 22 quoted lines

> 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.