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

[Jacob Keller <jacob.keller@gmail.com>]

On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
>
> Implement %(if:equals=<string>) wherein the if condition is only
> satisfied if the value obtained between the %(if:...) and %(then) atom
> is the same as the given '<string>'.
>
> Similarly, implement (if:notequals=<string>) wherein the if condition
> is only satisfied if the value obtained between the %(if:...) and
> %(then) atom is differnt from the given '<string>'.
>
> This is done by introducing 'if_atom_parser()' which parses the given
> %(if) atom and then stores the data in used_atom which is later passed
> on to the used_atom of the %(then) atom, so that it can do the required
> comparisons.
>

Ok. How does this handle whitespace? The previous if implementation
treated whitespace as trimming to ignore. Does this require an exact
whitespace match? It appears by the code that strings must match
exactly. Would it make more sense to always trim the value of
whitespace first before comparison? Hmm.. I think we should avoid
doing that actually.

Otherwise this looks good. I might have implemented the storage as one
value and then a boolean indicating whether to check for equality or
inequality. But I think the current implementation is ok too, and is a
bit more elegant on the code savings.

Thanks,
Jake

> Add tests and Documentation for the same.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  Documentation/git-for-each-ref.txt |  3 +++
>  ref-filter.c                       | 43 +++++++++++++++++++++++++++++++++-----
>  t/t6302-for-each-ref-filter.sh     | 18 ++++++++++++++++
>  3 files changed, 59 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index fed8126..b7b8560 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -155,6 +155,9 @@ if::
>         evaluating the string before %(then), this is useful when we
>         use the %(HEAD) atom which prints either "*" or " " and we
>         want to apply the 'if' condition only on the 'HEAD' ref.
> +       Append ":equals=<string>" or ":notequals=<string>" to compare
> +       the value between the %(if:...) and %(then) atoms with the
> +       given string.
>

This seems to imply that it does not ignore whitespace. Ok.

>  In addition to the above, for commit and tag objects, the header
>  field names (`tree`, `parent`, `object`, `type`, and `tag`) can
> diff --git a/ref-filter.c b/ref-filter.c
> index 8392303..44481c3 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;

Ok so we add both if_equals and not_equals values. Could we re-use the
same string?

>         unsigned int then_atom_seen : 1,
>                 else_atom_seen : 1,
>                 condition_satisfied : 1;
> @@ -49,6 +51,10 @@ static struct used_atom {
>                         enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>                         unsigned int nlines;
>                 } contents;
> +               struct {
> +                       const char *if_equals,
> +                               *not_equals;


Same here, why do we need both strings here stored separately? Could
we instead store which state to check and store the string once? I'm
not sure that really buys us any storage.

> +               } if_then_else;
>                 enum { O_FULL, O_SHORT } objectname;
>         } u;
>  } *used_atom;
> @@ -169,6 +175,19 @@ static void align_atom_parser(struct used_atom *atom, const char *arg)
>         string_list_clear(&params, 0);
>  }
>
> +static void if_atom_parser(struct used_atom *atom, const char *arg)
> +{
> +       if (!arg)
> +               return;
> +       else if (skip_prefix(arg, "equals=", &atom->u.if_then_else.if_equals))
> +                ;
> +       else if (skip_prefix(arg, "notequals=", &atom->u.if_then_else.not_equals))
> +               ;

Ok so we can't ever have if_equals or not_equals at the same time.

> +       else
> +               die(_("unrecognized %%(if) argument: %s"), arg);
> +}
> +
> +
>  static struct {
>         const char *name;
>         cmp_type cmp_type;
> @@ -209,7 +228,7 @@ static struct {
>         { "color", FIELD_STR, color_atom_parser },
>         { "align", FIELD_STR, align_atom_parser },
>         { "end" },
> -       { "if" },
> +       { "if", FIELD_STR, if_atom_parser },
>         { "then" },
>         { "else" },
>  };
> @@ -410,6 +429,9 @@ 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);
>
> +       if_then_else->if_equals = atomv->atom->u.if_then_else.if_equals;
> +       if_then_else->not_equals = atomv->atom->u.if_then_else.not_equals;
> +
>         push_stack_element(&state->stack);
>         new = state->stack;
>         new->at_end = if_then_else_handler;
> @@ -441,10 +463,17 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
>                 die(_("format: %%(then) atom used after %%(else)"));
>         if_then_else->then_atom_seen = 1;
>         /*
> -        * If there exists non-empty string between the 'if' and
> -        * 'then' atom then the 'if' condition is satisfied.
> +        * If the 'equals' or 'notequals' attribute is used then
> +        * perform the required comparison. If not, only non-empty
> +        * strings satisfy the 'if' condition.
>          */
> -       if (cur->output.len && !is_empty(cur->output.buf))
> +       if (if_then_else->if_equals) {
> +               if (!strcmp(if_then_else->if_equals, cur->output.buf))
> +                       if_then_else->condition_satisfied = 1;

Ok so if somehow we end up with both set, if_equals takes precedence.
I think I can be ok with the way we handle this.

> +       } else  if (if_then_else->not_equals) {
> +               if (strcmp(if_then_else->not_equals, cur->output.buf))
> +                       if_then_else->condition_satisfied = 1;
> +       } else if (cur->output.len && !is_empty(cur->output.buf))
>                 if_then_else->condition_satisfied = 1;
>         strbuf_reset(&cur->output);
>  }
> @@ -1137,7 +1166,11 @@ static void populate_value(struct ref_array_item *ref)
>                 } else if (!strcmp(name, "end")) {
>                         v->handler = end_atom_handler;
>                         continue;
> -               } else if (!strcmp(name, "if")) {
> +               } else if (starts_with(name, "if")) {
> +                       const char *s;
> +
> +                       if (skip_prefix(name, "if:", &s))
> +                               v->s = xstrdup(s);
>                         v->handler = if_atom_handler;
>                         continue;
>                 } else if (!strcmp(name, "then")) {
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index fed3013..a09a1a4 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -403,4 +403,22 @@ test_expect_success 'ignore spaces in %(if) atom usage' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'check %(if:equals=<string>)' '
> +       git for-each-ref --format="%(if:equals=master)%(refname:short)%(then)Found master%(else)Not master%(end)" refs/heads/ >actual &&
> +       cat >expect <<-\EOF &&
> +       Found master
> +       Not master
> +       EOF
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'check %(if:notequals=<string>)' '
> +       git for-each-ref --format="%(if:notequals=master)%(refname:short)%(then)Not master%(else)Found master%(end)" refs/heads/ >actual &&
> +       cat >expect <<-\EOF &&
> +       Found master
> +       Not master
> +       EOF
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.10.2
>