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

On Wed, Nov 9, 2016 at 4:52 AM, Jacob Keller [off-list ref] wrote:

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

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.

This does not trim whitespace what so ever and requires an exact match.
I don't see the reason behind trimming whitespace though.

quoted

 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?

If we use an union then we'll be saving space, but that comes at the
disadvantage that
we'd need an indicator to show which subatom we're using (i.e
if_equals or not_equals)
for the checks made in the code below.

At the expense of keeping two pointers we avoid the extra indicator
while keeping the code
elegant. Either ways not too keen on this, up for discussion :)

quoted

        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.

same as above, we utilize the fact that its easier to check the strings like

    if (if_equals)
        ...
    else (not_equals)
        ...

else we'd have to have an indicator to know which to check.

-- 
Regards,
Karthik Nayak