threads / patch / 33502

patchbranch: colour upstream branches

Subject: [PATCH] branch: colour upstream branches

## tl;dr

10 messages between Apr 14, 2013 and Apr 15, 2013. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Felipe Contreras· Apr 14, 2013, 01:46 UTC · lore
It's hard to see them among so much output otherwise.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/branch.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)
Show changes to builtin/branch.c +20 −7
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
Thomas Rast· Apr 14, 2013, 19:07 UTC · re: Felipe Contreras · lore

Re: [PATCH] branch: colour upstream branches

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 27 quoted lines
> It's hard to see them among so much output otherwise.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  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
Duy Nguyen· Apr 14, 2013, 22:31 UTC · re: Felipe Contreras · lore

Re: [PATCH] branch: colour upstream branches

On Sun, Apr 14, 2013 at 11:46 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 12 quoted lines
> +       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

Felipe Contreras· Apr 14, 2013, 23:22 UTC · re: Duy Nguyen · lore

Re: [PATCH] branch: colour upstream branches

On Sun, Apr 14, 2013 at 5:31 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 16 quoted lines
> 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?
-- 
Felipe Contreras
Duy Nguyen· Apr 14, 2013, 23:46 UTC · re: Felipe Contreras · lore

Re: [PATCH] branch: colour upstream branches

On Mon, Apr 15, 2013 at 9:22 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 19 quoted lines
> 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. -- Duy

Felipe Contreras· Apr 14, 2013, 23:54 UTC · re: Duy Nguyen · lore

Re: [PATCH] branch: colour upstream branches

On Sun, Apr 14, 2013 at 6:46 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 24 quoted lines
> 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?
-- 
Felipe Contreras
Duy Nguyen· Apr 15, 2013, 00:55 UTC · re: Felipe Contreras · lore

Re: [PATCH] branch: colour upstream branches

On Mon, Apr 15, 2013 at 9:54 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 27 quoted lines
> 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. -- Duy

Felipe Contreras· Apr 15, 2013, 01:24 UTC · re: Duy Nguyen · lore

Re: [PATCH] branch: colour upstream branches

On Sun, Apr 14, 2013 at 7:55 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 32 quoted lines
> 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)
Cheers.
-- 
Felipe Contreras
Duy Nguyen· Apr 15, 2013, 01:31 UTC · re: Felipe Contreras · lore

Re: [PATCH] branch: colour upstream branches

On Mon, Apr 15, 2013 at 11:24 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 38 quoted lines
> 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 the size of fancy when strbuf would solve it nicely? -- Duy

Felipe Contreras· Apr 15, 2013, 01:35 UTC · re: Duy Nguyen · lore

Re: [PATCH] branch: colour upstream branches

On Sun, Apr 14, 2013 at 8:31 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 44 quoted lines
> 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

← back to recent threads