Re: [PATCH v17 08/14] ref-filter: add support for %(contents:lines=X)

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 10, 2015, 17:14 UTC
Message-ID: <xmqqtwr242of.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1441900110-4015-9-git-send-email-Karthik.188@gmail.com>

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

Show 9 quoted lines

> diff --git a/ref-filter.c b/ref-filter.c
> index 7d2732a..b098b16 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -56,6 +56,7 @@ static struct {
>  	{ "color" },
>  	{ "align" },
>  	{ "end" },
> +	{ "contents:lines" },

Do we even need "contents:lines" and existing other "contents:blah" in this list in the first place? If they are needed, group them together, not append at the end.

I wonder how this code sensibly can parse "%(contents:lines=6)". After splitting the format string at %( and closing ), the code calls parse_ref_filter_atom() and the rule that helper function uses to figure out the atom-name proper (which is to be checked against the valid_atom[] array) is to find the first colon, so

    %(contents:lines=6)

would cause "contents:lines=6" to be fed parse_ref_filter_atom(), it cheks if "contents" is in the valid_atom[] array (it is), and stores the whole thing in used_atom[].

So in that sense, match_atom_name() would do the right thing, but that would make any reader of this code realize that she never saw "contents:lines" entry in valid_atom[] array being used during this process.