Re: [PATCH] branch: colour upstream branches

[Felipe Contreras <felipe.contreras@gmail.com>]

On Sun, Apr 14, 2013 at 8:31 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Mon, Apr 15, 2013 at 11:24 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Sun, Apr 14, 2013 at 7:55 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> On Mon, Apr 15, 2013 at 9:54 AM, Felipe Contreras
>>> <felipe.contreras@gmail.com> wrote:
>>>> On Sun, Apr 14, 2013 at 6:46 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>>>>> On Mon, Apr 15, 2013 at 9:22 AM, Felipe Contreras
>>>>> <felipe.contreras@gmail.com> wrote:
>>>>>> On Sun, Apr 14, 2013 at 5:31 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>>>>>>> On Sun, Apr 14, 2013 at 11:46 AM, Felipe Contreras
>>>>>>> <felipe.contreras@gmail.com> wrote:
>>>>>>>> +       char fancy[80];
>>>>>>>>
>>>>>>>> -       if (show_upstream_ref)
>>>>>>>> +       if (show_upstream_ref) {
>>>>>>>>                 ref = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
>>>>>>>> +               if (want_color(branch_use_color))
>>>>>>>> +                       snprintf(fancy, sizeof(fancy), "%s%s%s",
>>>>>>>> +                                       GIT_COLOR_BLUE, ref, GIT_COLOR_RESET);
>>>>>>>> +               else
>>>>>>>> +                       strncpy(fancy, ref, sizeof(fancy));
>>>>>>>> +       }
>>>>>>>> +
>>>>>>>
>>>>>>> Please use strbuf for "fancy".
>>>>>>
>>>>>> Why? We would need to initialize and free it. What's the advantage?
>>>>>
>>>>> From a quick glance, I don't see any gurantee that "ref" (plus ansi
>>>>> codes) will always fit in 80 bytes.
>>>>
>>>> Would changing it to 1024 (MAXREFLEN) fix it?
>>>
>>> You still need to take ansi codes into account. I think it's easier to
>>> just use strbuf.
>>
>> I'm not sure what you mean. If there was an issue with snprintf, then
>> there would be with this in refs.c:
>>
>>         if (recursion > MAXDEPTH || strlen(refname) > MAXREFLEN)
>
> I mean GIT_COLOR_BLUE and GIT_COLOR_RESET take some bytes from
> "fancy". In the stretched case where ref takes all MAXREFLEN, then
> fancy must be a little bit bigger. Why do we need to think hard about

I don't see that as a big issue, since it's a matter of finding the
maximum length of those, but the argument from Jeff convinces me; I
would not like to add extra code for the snprintf corner cases.

Cheers.

-- 
Felipe Contreras