From: Karthik Nayak <karthik.188@gmail.com>
Date: Sat, 12 Nov 2016 09:19:19 GMT
Subject: Re: [PATCH v7 03/17] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
Message-ID: <CAOLa=ZQqe3vEj_428d41vd_4kfjzsm87Wam6Zm2dhXWkPdJ8Rw@mail.gmail.com>
In-Reply-To: <xmqqoa1m5335.fsf@gitster.mtv.corp.google.com>

Hello,

On Fri, Nov 11, 2016 at 4:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>>> @@ -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.
>
> I am not sure if storage is an issue, but I tend to agree that it
> would be semantically cleaner if this was done as a pair of <what
> operation uses this string constant?, the string constant>, and the
> former would be enum { COMPARE_EQUAL, COMPARE_UNEQUAL}.
>
> You could later enhance the comparison operator more easily with
> such an arrangement (e.g. if-equals-case-insensitively).

That's a rather good point you make there, I think using an enum with a string
constant would be ideal for future extensibility too. Thanks for the
explanation,
I shall change it.

-- 
Regards,
Karthik Nayak