From: Karthik Nayak <karthik.188@gmail.com>
Date: Sun, 20 Nov 2016 07:23:17 GMT
Subject: Re: [PATCH v7 03/17] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
Message-ID: <CAOLa=ZQ1wPhd=aXck5e+Zd0020CbEwaohVX4eOGjy50BTqsTpA@mail.gmail.com>
In-Reply-To: <37c2cbf2-7160-49d7-f8f1-3b65d9ecf9ec@gmail.com>

On Sat, Nov 19, 2016 at 1:28 AM, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 08.11.2016 o 21:11, Karthik Nayak pisze:
>> 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>'.
>                   ^^^^^^^^
>
> s/differnt/different/   <-- typo
>

Will change that.

>>
>> 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.
>
> So %(if) is actually %(if:notempty) ?  Just kidding.
>

It's not a bug, it's a feature ;)

>> +     Append ":equals=<string>" or ":notequals=<string>" to compare
>> +     the value between the %(if:...) and %(then) atoms with the
>> +     given string.
>>
>>  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;
>
> I guess using anonymous structs from C11 here...
>
>>       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;
>> +             } if_then_else;
>
> ...to avoid code duplication there is rather out of question?
>

I believe it holds better context without the use of anonymous structs/unions.
But that's my perspective, I wouldn't mind changing it.

>>               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))
>> +             ;
>
> Those ';' should be perfectly aligned, isn't it?
>

This should be dropped with the new changes made on this patch.

-- 
Regards,
Karthik Nayak