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

On Sun, Dec 13, 2015 at 2:15 PM, Eric Sunshine [off-list ref] wrote:

On Sun, Dec 13, 2015 at 3:32 AM, Karthik Nayak [off-list ref] wrote:

quoted

quoted

On Sun, Dec 13, 2015 at 6:23 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:

quoted

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 <redacted>
---

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?

quoted

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