Re: [PATCH/RFC 08/10] ref-filter: introduce remote_ref_atom_parser()

From: Karthik Nayak <karthik.188@gmail.com>
Date: Dec 13, 2015, 08:53 UTC
Message-ID: <CAOLa=ZQ=GL9F8ecOfa3rQ_rXP6_eEpA+ms8zOz5TBbmY9frnaw@mail.gmail.com>
In-Reply-To: <CAPig+cRwD_W5pQSpiHR8izQ+=6stY+XwpXEJ5HU4gi+MQFaH2Q@mail.gmail.com>

On Sun, Dec 13, 2015 at 2:15 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 45 quoted lines

> On Sun, Dec 13, 2015 at 3:32 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> On Sun, Dec 13, 2015 at 6:23 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>>> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'
>>>>> and '%(push)' atoms and store information into the 'used_atom'
>>>>> structure based on the modifiers used along with the corresponding
>>>>> atom.
>>>>>
>>>>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>>>>> ---
>>>>> diff --git a/ref-filter.c b/ref-filter.c
>>>>> @@ -37,6 +37,11 @@ static struct used_atom {
>>>>>         union {
>>>>>                 const char *color;
>>>>>                 struct align align;
>>>>> +               struct {
>>>>> +                       unsigned int shorten : 1,
>>>>> +                               track : 1,
>>>>> +                               trackshort : 1;
>>>>> +               } remote_ref;
>>>>
>>>> Are 'shorten', 'track', and 'trackshort' mutually exclusive? If so, a
>>>> simple enum would be clearer than bitfields:
>>>>
>>>>     union {
>>>>         const char *color;
>>>>         struct align align;
>>>>         enum { RR_PLAIN, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
>>>>             remote_ref;
>>>>     };
>>>>
>>>> Or something.
>>
>> There's also a slight problem with using enum's with the current implementation.
>> The problem is the enum is set to 0 by default (since we use memset).
>> so the first value is set by default, not something we'd want.
>
> I'm afraid I don't see the problem. Doesn't the RR_PLAIN in the
> example cover this case?
>
>> So either we stick to the structure
>> with unsigned bits or we introduce a pseudo value in the enum. I
>> prefer the former.
>
> It's not a pseudo-value, but rather just one of the mutually exclusive states.

This example is actually fine, the next one "%(contents)" is more of the problem, the check is done in grab_sub_body_contents() where previously "contents.all" would be enough to check if we need to add contents value. Now the first enum value is selected. Maybe have a "NOT_VALID" field in the enum.

-- 
Regards,
Karthik Nayak