Re: [PATCH 02/15] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)

From: Karthik Nayak <karthik.188@gmail.com>
Date: Mar 8, 2016, 05:41 UTC
Message-ID: <CAOLa=ZSFD39HXSw8jo1=ehiKpeqjLUSfc0NkRJD0BSbqAvDwuQ@mail.gmail.com>
In-Reply-To: <xmqq7fhdoqd3.fsf@gitster.mtv.corp.google.com>

On Tue, Mar 8, 2016 at 4:19 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 40 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 41e73f0..440e270 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -22,6 +22,8 @@ struct align {
>>  };
>>
>>  struct if_then_else {
>> +     const char *if_equals,
>> +             *not_equals;
>>       unsigned int then_atom_seen : 1,
>>               else_atom_seen : 1,
>>               condition_satisfied : 1;
>> @@ -411,6 +413,14 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat
>>  {
>>       struct ref_formatting_stack *new;
>>       struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);
>> +     const char *valp;
>> +
>> +     if (skip_prefix(atomv->s, "equals=", &valp))
>> +             if_then_else->if_equals = valp;
>> +     else if (skip_prefix(atomv->s, "notequals=", &valp))
>> +             if_then_else->not_equals = valp;
>> +     else if (atomv->s[0])
>> +             die(_("format: unknown format if:%s"), atomv->s);
>>
>>       push_stack_element(&state->stack);
>>       new = state->stack;
>
> The fact that you are pushing stack element here tells me that this
> "handler" is run once for each 'ref' that we prepare output for
> (i.e. unlike the helper functions called "parser" that are called
> only once while preparing used_atom[] array).
>
> It somehow feels that this goes against the overall design you did
> in the earlier topic to pre-parse as much as possible when you
> prepare the used_atom array and avoid parsing at the runtime.  Am I
> misreading the patch?

You're correct, the "handler" functions run once for each "ref". But WRT to the %(if)...%(then)...%(else)...%(end) atoms, it needs to be. Because each outcome of these atoms depend on the current outcome of the fields used between then WRT to the current ref.

Although we could somehow go about parsing the "equals=" / " notequals=" part of the %(if) atom beforehand to ensure that we do not end up repeating that each time for every atom.

This could be done by parsing the %(if) atom before hand, probably with a if_atom_parser, storing the "equals=" / " notequals=" value into used_atom[i] and providing this used_atom[i] to the if_atom_handler. This would require extending the prototype of the 'handler' functions to also pass used_atom[i]. Which seems like a good thing, considering that this may enable future 'handler' functions to follow the same route and parse whatever can be parsed beforehand.

-- 
Regards,
Karthik Nayak