Re: [PATCH v3 14/15] ref-filter: introduce contents_atom_parser()

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> writes:

> Introduce contents_atom_parser() which will parse the '%(contents)'
> atom and store information into the 'used_atom' structure based on the
> modifiers used along with the atom.
>
> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
>  ref-filter.c | 74 +++++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 46 insertions(+), 28 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 32b4674..9e61530 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -39,6 +39,10 @@ static struct used_atom {
>  		struct align align;
>  		enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
>  			remote_ref;
> +		struct {
> +			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
> +			unsigned int nlines;
> +		} contents;
>  	} u;
>  } *used_atom;
>  static int used_atom_cnt, need_tagged, need_symref;
> @@ -96,6 +100,35 @@ static void remote_ref_atom_parser(struct used_atom *atom)
>  		die(_("unrecognized format: %%(%s)"), atom->name);
>  }
>  
> +static void contents_atom_parser(struct used_atom *atom)
> +{
> +	const char * buf;
> +
> +	if (match_atom_name(atom->name, "subject", &buf) && !buf) {
> +		atom->u.contents.option = C_SUB;
> +		return;
> +	} else if (match_atom_name(atom->name, "body", &buf) && !buf) {
> +		atom->u.contents.option = C_BODY_DEP;
> +		return;
> +	} if (!match_atom_name(atom->name, "contents", &buf))
> +		  die("BUG: parsing non-'contents'");

Did you really intend to say "if" here, not "else if"?

I also wonder if the "&& !buf" in the first two are correct.  What
should happen to "%(subject:foo)", which gives you a non-empty buf?
It may or may not be an error, but it should not fall thru and cause
"BUG:parsing non-contents", should it?

> +	if (!buf)
> +		atom->u.contents.option = C_BARE;
> +	else if (!strcmp(buf, "body"))
> +		atom->u.contents.option = C_BODY;
> +...