Hi,
On Sat, Aug 22, 2020 at 12:47 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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@xxxxxxxxx/#t
