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

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

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.

Sure, will do that.

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. So either we stick
to the structure
with unsigned bits or we introduce a pseudo value in the enum. I
prefer the former.



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

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

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

+               if (!num_ours && !num_theirs)
+                       *s = "";
+               else if (!num_ours)
+                       *s = xstrfmt("[behind %d]", num_theirs);
+               else if (!num_theirs)
+                       *s = xstrfmt("[ahead %d]", num_ours);
+               else
+                       *s = xstrfmt("[ahead %d, behind %d]",
+                                    num_ours, num_theirs);

Tangent: These xstrfmt()'d strings are getting leaked, right? Is that
something that we need to worry about (if, for instance, a repository
contains a lot of tracking refs)? Should there be a NEEDSWORK comment
here regarding the issue?

This is sort of a problem with most of the values in ref-filter, we dynamically
allocate memory and do not free it, since the program exits soon after and
we leave it to the Operating System to do the garbage collection.

I'm not worried about memory dynamically allocated for the used_atom[]
array being leaked (and cleaned up automatically at program exit), but
rather about memory being leaked for each processed reference, which
might become substantial for a project with a lot of references.

quoted

Not sure if we'd want to work on this though.

It's likely outside the scope of the current patch series anyhow, and
probably not something that needs to be tackled right away (or perhaps
ever), which is why a NEEDSWORK comment might be appropriate, as a
reminder that the situation could be improved.

Yes I got what you're saying. I'm talking about other values which are
dynamically
allocated in ref-filter itself.

For e.g.

When we use the deref option
v->s = xstrfmt("%s^{}", refname);

or the color option
v->s = xstrdup(color);

So seems like we need a way to go around all of these.

-- 
Regards,
Karthik Nayak