* [PATCH] branch: colour upstream branches
@ 2013-04-14 1:46 Felipe Contreras
2013-04-14 19:07 ` Thomas Rast
2013-04-14 22:31 ` Duy Nguyen
0 siblings, 2 replies; 10+ messages in thread
From: Felipe Contreras @ 2013-04-14 1:46 UTC (permalink / raw)
To: git; +Cc: Nguyễn Thái Ngọc Duy, Felipe Contreras
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);
+ 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] branch: colour upstream branches
2013-04-14 1:46 [PATCH] branch: colour upstream branches Felipe Contreras
@ 2013-04-14 19:07 ` Thomas Rast
2013-04-14 22:31 ` Duy Nguyen
1 sibling, 0 replies; 10+ messages in thread
From: Thomas Rast @ 2013-04-14 19:07 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Nguyễn Thái Ngọc Duy
Felipe Contreras <felipe.contreras@gmail.com> writes:
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] branch: colour upstream branches
2013-04-14 1:46 [PATCH] branch: colour upstream branches Felipe Contreras
2013-04-14 19:07 ` Thomas Rast
@ 2013-04-14 22:31 ` Duy Nguyen
2013-04-14 23:22 ` Felipe Contreras
1 sibling, 1 reply; 10+ messages in thread
From: Duy Nguyen @ 2013-04-14 22:31 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Git Mailing List
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". I also agree with Thomas that the color
should not be hard coded.
--
Duy
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] branch: colour upstream branches
2013-04-14 22:31 ` Duy Nguyen
@ 2013-04-14 23:22 ` Felipe Contreras
2013-04-14 23:46 ` Duy Nguyen
0 siblings, 1 reply; 10+ messages in thread
From: Felipe Contreras @ 2013-04-14 23:22 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
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?
--
Felipe Contreras
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] branch: colour upstream branches
2013-04-14 23:22 ` Felipe Contreras
@ 2013-04-14 23:46 ` Duy Nguyen
2013-04-14 23:54 ` Felipe Contreras
0 siblings, 1 reply; 10+ messages in thread
From: Duy Nguyen @ 2013-04-14 23:46 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Git Mailing List
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.
--
Duy
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] branch: colour upstream branches
2013-04-14 23:46 ` Duy Nguyen
@ 2013-04-14 23:54 ` Felipe Contreras
2013-04-15 0:55 ` Duy Nguyen
0 siblings, 1 reply; 10+ messages in thread
From: Felipe Contreras @ 2013-04-14 23:54 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
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?
--
Felipe Contreras
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] branch: colour upstream branches
2013-04-14 23:54 ` Felipe Contreras
@ 2013-04-15 0:55 ` Duy Nguyen
2013-04-15 1:24 ` Felipe Contreras
0 siblings, 1 reply; 10+ messages in thread
From: Duy Nguyen @ 2013-04-15 0:55 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Git Mailing List
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.
--
Duy
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] branch: colour upstream branches
2013-04-15 0:55 ` Duy Nguyen
@ 2013-04-15 1:24 ` Felipe Contreras
2013-04-15 1:31 ` Duy Nguyen
0 siblings, 1 reply; 10+ messages in thread
From: Felipe Contreras @ 2013-04-15 1:24 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
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)
Cheers.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] branch: colour upstream branches
2013-04-15 1:24 ` Felipe Contreras
@ 2013-04-15 1:31 ` Duy Nguyen
2013-04-15 1:35 ` Felipe Contreras
0 siblings, 1 reply; 10+ messages in thread
From: Duy Nguyen @ 2013-04-15 1:31 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Git Mailing List
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
the size of fancy when strbuf would solve it nicely?
--
Duy
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] branch: colour upstream branches
2013-04-15 1:31 ` Duy Nguyen
@ 2013-04-15 1:35 ` Felipe Contreras
0 siblings, 0 replies; 10+ messages in thread
From: Felipe Contreras @ 2013-04-15 1:35 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-04-15 1:35 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-14 1:46 [PATCH] branch: colour upstream branches Felipe Contreras
2013-04-14 19:07 ` Thomas Rast
2013-04-14 22:31 ` Duy Nguyen
2013-04-14 23:22 ` Felipe Contreras
2013-04-14 23:46 ` Duy Nguyen
2013-04-14 23:54 ` Felipe Contreras
2013-04-15 0:55 ` Duy Nguyen
2013-04-15 1:24 ` Felipe Contreras
2013-04-15 1:31 ` Duy Nguyen
2013-04-15 1:35 ` Felipe Contreras
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).