Thread (57 messages) flat view 57 messages, 3 authors, 2016-11-20

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

From: Jacob Keller <hidden>
Date: 2016-11-08 23:23:18

On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak [off-list ref] wrote:
From: Karthik Nayak <redacted>

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
quoted hunk ↗ jump to hunk
Add tests and Documentation for the same.

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Matthieu Moy [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
 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.
quoted hunk ↗ jump to hunk
 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?
quoted hunk ↗ jump to hunk
        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.
quoted hunk ↗ jump to hunk
+               } 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.
quoted hunk ↗ jump to hunk
+       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.
quoted hunk ↗ jump to hunk
+       } 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help