Re: [PATCH] branch: colour upstream branches

2013-04-14 Thread Felipe Contreras 
On Sun, Apr 14, 2013 at 8:31 PM, Duy Nguyen  wrote:
> On Mon, Apr 15, 2013 at 11:24 AM, Felipe Contreras
>  wrote:
>> On Sun, Apr 14, 2013 at 7:55 PM, Duy Nguyen  wrote:
>>> On Mon, Apr 15, 2013 at 9:54 AM, Felipe Contreras
>>>  wrote:
 On Sun, Apr 14, 2013 at 6:46 PM, Duy Nguyen  wrote:
> On Mon, Apr 15, 2013 at 9:22 AM, Felipe Contreras
>  wrote:
>> On Sun, Apr 14, 2013 at 5:31 PM, Duy Nguyen  wrote:
>>> On Sun, Apr 14, 2013 at 11:46 AM, Felipe Contreras
>>>  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] branch: colour upstream branches

2013-04-14 Thread Duy Nguyen 
On Mon, Apr 15, 2013 at 11:24 AM, Felipe Contreras
 wrote:
> On Sun, Apr 14, 2013 at 7:55 PM, Duy Nguyen  wrote:
>> On Mon, Apr 15, 2013 at 9:54 AM, Felipe Contreras
>>  wrote:
>>> On Sun, Apr 14, 2013 at 6:46 PM, Duy Nguyen  wrote:
 On Mon, Apr 15, 2013 at 9:22 AM, Felipe Contreras
  wrote:
> On Sun, Apr 14, 2013 at 5:31 PM, Duy Nguyen  wrote:
>> On Sun, Apr 14, 2013 at 11:46 AM, Felipe Contreras
>>  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
the size of fancy when strbuf would solve it nicely?
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] branch: colour upstream branches

2013-04-14 Thread Felipe Contreras 
On Sun, Apr 14, 2013 at 7:55 PM, Duy Nguyen  wrote:
> On Mon, Apr 15, 2013 at 9:54 AM, Felipe Contreras
>  wrote:
>> On Sun, Apr 14, 2013 at 6:46 PM, Duy Nguyen  wrote:
>>> On Mon, Apr 15, 2013 at 9:22 AM, Felipe Contreras
>>>  wrote:
 On Sun, Apr 14, 2013 at 5:31 PM, Duy Nguyen  wrote:
> On Sun, Apr 14, 2013 at 11:46 AM, Felipe Contreras
>  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)

Cheers.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] branch: colour upstream branches

2013-04-14 Thread Duy Nguyen 
On Mon, Apr 15, 2013 at 9:54 AM, Felipe Contreras
 wrote:
> On Sun, Apr 14, 2013 at 6:46 PM, Duy Nguyen  wrote:
>> On Mon, Apr 15, 2013 at 9:22 AM, Felipe Contreras
>>  wrote:
>>> On Sun, Apr 14, 2013 at 5:31 PM, Duy Nguyen  wrote:
 On Sun, Apr 14, 2013 at 11:46 AM, Felipe Contreras
  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.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] branch: colour upstream branches

2013-04-14 Thread Felipe Contreras 
On Sun, Apr 14, 2013 at 6:46 PM, Duy Nguyen  wrote:
> On Mon, Apr 15, 2013 at 9:22 AM, Felipe Contreras
>  wrote:
>> On Sun, Apr 14, 2013 at 5:31 PM, Duy Nguyen  wrote:
>>> On Sun, Apr 14, 2013 at 11:46 AM, Felipe Contreras
>>>  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?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] branch: colour upstream branches

2013-04-14 Thread Duy Nguyen 
On Mon, Apr 15, 2013 at 9:22 AM, Felipe Contreras
 wrote:
> On Sun, Apr 14, 2013 at 5:31 PM, Duy Nguyen  wrote:
>> On Sun, Apr 14, 2013 at 11:46 AM, Felipe Contreras
>>  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.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] branch: colour upstream branches

2013-04-14 Thread Felipe Contreras 
On Sun, Apr 14, 2013 at 5:31 PM, Duy Nguyen  wrote:
> On Sun, Apr 14, 2013 at 11:46 AM, Felipe Contreras
>  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?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] branch: colour upstream branches

2013-04-14 Thread Duy Nguyen 
On Sun, Apr 14, 2013 at 11:46 AM, Felipe Contreras
 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". I also agree with Thomas that the color
should not be hard coded.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] branch: colour upstream branches

2013-04-14 Thread Thomas Rast 
Felipe Contreras  writes:

> It's hard to see them among so much output otherwise.
>
> Signed-off-by: Felipe Contreras 
> ---
>  builtin/branch.c | 27 ---
>  1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 00d17d2..a1cdc29 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -417,32 +417,45 @@ static void fill_tracking_info(struct strbuf *stat, 
> const char *branch_name,
>   int ours, theirs;
>   char *ref = NULL;
>   struct branch *branch = branch_get(branch_name);
> + char fancy[80];
>  
>   if (!stat_tracking_info(branch, &ours, &theirs)) {
>   if (branch && branch->merge && branch->merge[0]->dst &&
> - show_upstream_ref)
> - strbuf_addf(stat, "[%s] ",
> - shorten_unambiguous_ref(branch->merge[0]->dst, 0));
> + show_upstream_ref) {
> + ref = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
> + if (want_color(branch_use_color))
> + strbuf_addf(stat, "[%s%s%s] ",
> + GIT_COLOR_BLUE, ref, 
> GIT_COLOR_RESET);

AFAICS you are hardcoding the color?  The other bits of branch colors
are configurable through branch.color.*, so why not this?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] branch: colour upstream branches

2013-04-13 Thread Felipe Contreras 
It's hard to see them among so much output otherwise.

Signed-off-by: Felipe Contreras 
---
 builtin/branch.c | 27 ---
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 00d17d2..a1cdc29 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -417,32 +417,45 @@ static void fill_tracking_info(struct strbuf *stat, const 
char *branch_name,
int ours, theirs;
char *ref = NULL;
struct branch *branch = branch_get(branch_name);
+   char fancy[80];
 
if (!stat_tracking_info(branch, &ours, &theirs)) {
if (branch && branch->merge && branch->merge[0]->dst &&
-   show_upstream_ref)
-   strbuf_addf(stat, "[%s] ",
-   shorten_unambiguous_ref(branch->merge[0]->dst, 0));
+   show_upstream_ref) {
+   ref = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
+   if (want_color(branch_use_color))
+   strbuf_addf(stat, "[%s%s%s] ",
+   GIT_COLOR_BLUE, ref, 
GIT_COLOR_RESET);
+   else
+   strbuf_addf(stat, "[%s] ", ref);
+   }
return;
}
 
-   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));
+   }
+
if (!ours) {
if (ref)
-   strbuf_addf(stat, _("[%s: behind %d]"), ref, theirs);
+   strbuf_addf(stat, _("[%s: behind %d]"), fancy, theirs);
else
strbuf_addf(stat, _("[behind %d]"), theirs);
 
} else if (!theirs) {
if (ref)
-   strbuf_addf(stat, _("[%s: ahead %d]"), ref, ours);
+   strbuf_addf(stat, _("[%s: ahead %d]"), fancy, ours);
else
strbuf_addf(stat, _("[ahead %d]"), ours);
} else {
if (ref)
strbuf_addf(stat, _("[%s: ahead %d, behind %d]"),
-   ref, ours, theirs);
+   fancy, ours, theirs);
else
strbuf_addf(stat, _("[ahead %d, behind %d]"),
ours, theirs);
-- 
1.8.2.1.641.gd16d37a

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html