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

Karthik Nayak [off-list ref] writes:

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?

On Tue, Mar 8, 2016 at 4:19 AM, Junio C Hamano [off-list ref] wrote:

Karthik Nayak [off-list ref] writes:

quoted

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

On Mon, Mar 7, 2016 at 9:41 PM, Karthik Nayak [off-list ref] wrote:

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.

Yeah, there is no argument against that part at all.  My comment was purely
about preparsing the if_equals field. All other fields are preparsed in *parse()
helper functions and it looked strange that only these two are scanned in the
handle() function every time it is evaluated for each ref, only to yield the
same result.

On Tue, Mar 8, 2016 at 11:34 AM, Junio C Hamano [off-list ref] wrote:

On Mon, Mar 7, 2016 at 9:41 PM, Karthik Nayak [off-list ref] wrote:

quoted

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.

Yeah, there is no argument against that part at all.  My comment was purely
about preparsing the if_equals field. All other fields are preparsed in *parse()
helper functions and it looked strange that only these two are scanned in the
handle() function every time it is evaluated for each ref, only to yield the
same result.

True, I definitely agree, will fix that in the next iteration, thanks.

-- 
Regards,
Karthik Nayak