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

[Karthik Nayak <karthik.188@gmail.com>]

On Sun, Dec 13, 2015 at 11: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.
>>
>
> 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 <sunshine@sunshineco.com> wrote:
> On Sun, Dec 13, 2015 at 1:02 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:
>>>> +               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.
>
>> 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