On Sun, Dec 13, 2015 at 11:32 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
> On Sun, Dec 13, 2015 at 6:23 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak <karthik.188@xxxxxxxxx> 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@xxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxx> wrote:
> On Sun, Dec 13, 2015 at 1:02 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> On Sun, Dec 13, 2015 at 6:23 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>>> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak <karthik.188@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html