Re: [PATCH v3 00/15] ref-filter: use parsing functions

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Jan 7, 2016 at 1:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> If we really want to avoid having separate subject_atom_parser() and
> body_atom_parser(), they can be folded into the same function and it
> becomes necessary to switch on atom->name like you did in the code
> being discussed in the quoted part above.  For that, as Eric said,
> skipping '*' manually would not be a big deal, as that should not
> happen so often in the code _anyway_.  It is not a good idea to
> switch on atom->name inside contents_atom_parser() like you did.
> You are better off having separate {subject,body}_atom_parser()
> functions.
>
> For one thing, you are not reusing or sharing any code by squishing
> these three functions into one.  A conceptually larger problem is
> that you are adding two extra !strcmp() calls to figure out the
> caller _already_ knows (notice I said this is "conceptual", this is
> not about performance).  parse_ref_filter_atom() knows that it is a
> "%(subject)" or "%(subject:...)" atom, but because you throw away
> that information and call contents_atom_parser() by saying that it
> is one of the contents, subject or body, the called function has to
> redo strcmp in order to figure it out itself.

Thanks for spelling this all out. I had come to the same conclusion
yesterday after posting my message but never got back to the computer
long enough to compose a proper email explaining the issue. I fully
agree that this would be a better design.