Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing

From: Hariom verma <hariom18599@gmail.com>
Date: Aug 23, 2020, 19:25 UTC
Message-ID: <CA+CkUQ8Gst2RTaXY6t+ytWu_9Pu7eqnRYRrnawRwYd_NN=u0Lg@mail.gmail.com>
In-Reply-To: <xmqqeenz95bj.fsf@gitster.c.googlers.com>

Hi,

On Sat, Aug 22, 2020 at 12:47 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

>
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > ...an alternative would have been something like:
> >
> >     else if (!strcmp(arg, "trailers")) {
> >         if (trailers_atom_parser(format, atom, NULL, err))
> >             return -1;
> >     } else if (skip_prefix(arg, "trailers:", &arg)) {
> >         if (trailers_atom_parser(format, atom, arg, err))
> >             return -1;
> >     }
> >
> > which is quite simple to reason about (though has the cost of a tiny
> > bit of duplication).
>
> Yeah, that looks quite simple and straight-forward.

No doubt, it looks good for "contents:trailers".

What if In future we would like to expand functionalities of other 'contents' options?

Recently, I sent a patch series "Improvements to ref-filter"[1]. A patch in this patch series introduced "sanitize" modifier to "subject" atom. i.e "%(subject:sanitize)".

What if in the future we also want "%(contents:subject:sanitize)" to work?
We can duplicate code again. Something like:
```
} else if (!strcmp(arg, "trailers")) {
        if (trailers_atom_parser(format, atom, NULL, err))
            return -1;
} else if (skip_prefix(arg, "trailers:", &arg)) {
        if (trailers_atom_parser(format, atom, arg, err))
            return -1;
} else if (!strcmp(arg, "subject")) {
        if (subject_atom_parser(format, atom, NULL, err))
            return -1;
} else if (skip_prefix(arg, "subject:", &arg)) {
        if (subject_atom_parser(format, atom, arg, err))
            return -1;
}
```

OR

We can just simply use helper. Something like:
```
else if (check_format_field(arg, "subject", &arg)) {
    if (subject_atom_parser(format, atom, arg, err))
        return -1;
} else if (check_format_field(arg, "trailers", &arg)) {
    if (trailers_atom_parser(format, atom, arg, err))
        return -1;
```
We can use this helper any number of times, whenever there is a need.

Sorry, I missed saying this earlier. But I don't prefer duplicating the code here.

Thanks, Hariom

[1]: https://public-inbox.org/git/pull.684.v4.git.1598046110.gitgitgadget@gmail.com/#t