Message ID | 20181111235831.44824-3-nbelakovski@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | refactoring branch colorization to ref-filter | expand |
nbelakovski@gmail.com writes: > diff --git a/color.h b/color.h > index 98894d6a17..857653df73 100644 > --- a/color.h > +++ b/color.h > @@ -42,6 +42,24 @@ struct strbuf; > #define GIT_COLOR_FAINT_BLUE "\033[2;34m" > #define GIT_COLOR_FAINT_MAGENTA "\033[2;35m" > #define GIT_COLOR_FAINT_CYAN "\033[2;36m" > +#define GIT_COLOR_LIGHT_RED "\033[91m" > +#define GIT_COLOR_LIGHT_GREEN "\033[92m" > +#define GIT_COLOR_LIGHT_YELLOW "\033[93m" > +#define GIT_COLOR_LIGHT_BLUE "\033[94m" > +#define GIT_COLOR_LIGHT_MAGENTA "\033[95m" > +#define GIT_COLOR_LIGHT_CYAN "\033[96m" > +#define GIT_COLOR_BOLD_LIGHT_RED "\033[1;91m" > +#define GIT_COLOR_BOLD_LIGHT_GREEN "\033[1;92m" > +#define GIT_COLOR_BOLD_LIGHT_YELLOW "\033[1;93m" > +#define GIT_COLOR_BOLD_LIGHT_BLUE "\033[1;94m" > +#define GIT_COLOR_BOLD_LIGHT_MAGENTA "\033[1;95m" > +#define GIT_COLOR_BOLD_LIGHT_CYAN "\033[1;96m" > +#define GIT_COLOR_FAINT_LIGHT_RED "\033[2;91m" > +#define GIT_COLOR_FAINT_LIGHT_GREEN "\033[2;92m" > +#define GIT_COLOR_FAINT_LIGHT_YELLOW "\033[2;93m" > +#define GIT_COLOR_FAINT_LIGHT_BLUE "\033[2;94m" > +#define GIT_COLOR_FAINT_LIGHT_MAGENTA "\033[2;95m" > +#define GIT_COLOR_FAINT_LIGHT_CYAN "\033[2;96m" Hopefully you made sure that there is no other topic in-flight that touch this area before doing this change? Otherwise you'd be creating pointless merge conflict by futzing with spaces. Ditto for an earlier hunk of this patch. Thanks.
On Mon, Nov 12, 2018 at 07:20:28PM +0900, Junio C Hamano wrote: > nbelakovski@gmail.com writes: > > > diff --git a/color.h b/color.h > > index 98894d6a17..857653df73 100644 > > --- a/color.h > > +++ b/color.h > > @@ -42,6 +42,24 @@ struct strbuf; > > #define GIT_COLOR_FAINT_BLUE "\033[2;34m" > > #define GIT_COLOR_FAINT_MAGENTA "\033[2;35m" > > #define GIT_COLOR_FAINT_CYAN "\033[2;36m" > > +#define GIT_COLOR_LIGHT_RED "\033[91m" > > +#define GIT_COLOR_LIGHT_GREEN "\033[92m" > > +#define GIT_COLOR_LIGHT_YELLOW "\033[93m" > > +#define GIT_COLOR_LIGHT_BLUE "\033[94m" > > +#define GIT_COLOR_LIGHT_MAGENTA "\033[95m" > > +#define GIT_COLOR_LIGHT_CYAN "\033[96m" > > +#define GIT_COLOR_BOLD_LIGHT_RED "\033[1;91m" > > +#define GIT_COLOR_BOLD_LIGHT_GREEN "\033[1;92m" > > +#define GIT_COLOR_BOLD_LIGHT_YELLOW "\033[1;93m" > > +#define GIT_COLOR_BOLD_LIGHT_BLUE "\033[1;94m" > > +#define GIT_COLOR_BOLD_LIGHT_MAGENTA "\033[1;95m" > > +#define GIT_COLOR_BOLD_LIGHT_CYAN "\033[1;96m" > > +#define GIT_COLOR_FAINT_LIGHT_RED "\033[2;91m" > > +#define GIT_COLOR_FAINT_LIGHT_GREEN "\033[2;92m" > > +#define GIT_COLOR_FAINT_LIGHT_YELLOW "\033[2;93m" > > +#define GIT_COLOR_FAINT_LIGHT_BLUE "\033[2;94m" > > +#define GIT_COLOR_FAINT_LIGHT_MAGENTA "\033[2;95m" > > +#define GIT_COLOR_FAINT_LIGHT_CYAN "\033[2;96m" > > Hopefully you made sure that there is no other topic in-flight that > touch this area before doing this change? Otherwise you'd be > creating pointless merge conflict by futzing with spaces. This hunk confused me for a minute, too. It's not changing spaces, but just adding a bunch of color variants. It would be nice if we could just do this with a run-time parse_color("bold red") or whatever, but we use these as static initializers. We don't strictly need anything more than FAINT_LIGHT_GREEN here. I don't have a strong opinion on adding just what we need versus being more complete. > Ditto for an earlier hunk of this patch. Yeah, I think this does apply to the earlier hunk that defines branch_colors[]. -Peff
On Mon, Nov 12, 2018 at 07:14:23AM -0500, Jeff King wrote: > just adding a bunch of color variants. It would be nice if we could just > do this with a run-time parse_color("bold red") or whatever, but we use > these as static initializers. I suggested those colors, but now, I think this needs to be configurable. I suggested using green and dim green as the obvious theoretical choice but after using it for a while I found out that both shades are way too similar, making it really hard to tell by glancing at the output, especially when they're not side by side. If we continue with two dual green approach, current branch needs to be at least bold. But I'm not sure if it's enough. I've been trying some other colors, and cyan feels neutral-ish. I think: GIT_COLOR_BOLD_GREEN /* CURRENT */ GIT_COLOR_CYAN /* WORKTREE */ makes an ok combination. But I can see where personal preference starts to play a role here, as the logical solution isn't good enough. Which makes the case for being able to configure a bit stronger. Cheers, Rafael Ascensão
Rafael Ascensão <rafa.almas@gmail.com> writes: > But I can see where personal preference starts to play a role here, as > the logical solution isn't good enough. Which makes the case for being > able to configure a bit stronger. Yeah, our preference over time has always been "do not add to our default color palette to make the default output too colourful; instead allow the user to specify their choice". If this feature can be added like that, that would be preferrable, and if cyan (which usuallly is used to present "less interesting" piece of information and in our default palette) works well enough, maybe we should use that?
On Mon, Nov 12, 2018 at 06:07:18PM +0000, Rafael Ascensão wrote: > On Mon, Nov 12, 2018 at 07:14:23AM -0500, Jeff King wrote: > > just adding a bunch of color variants. It would be nice if we could just > > do this with a run-time parse_color("bold red") or whatever, but we use > > these as static initializers. > > I suggested those colors, but now, I think this needs to be > configurable. I think they are configurable in that patch, since it provides "worktree" as a n entry in color_branch_slots. But yeah, every color we add needs to be configurable, and this is really just about defaults. > I suggested using green and dim green as the obvious theoretical choice > but after using it for a while I found out that both shades are way too > similar, making it really hard to tell by glancing at the output, > especially when they're not side by side. > > If we continue with two dual green approach, current branch needs to be > at least bold. But I'm not sure if it's enough. > > I've been trying some other colors, and cyan feels neutral-ish. Yeah, cyan seems pretty reasonable to me. -Peff
OK, I see 3 votes for cyan and 4-5 people participating in the thread, so I'll make it cyan in the next revision. On Tue, Nov 13, 2018 at 3:49 PM Jeff King <peff@peff.net> wrote: > > On Mon, Nov 12, 2018 at 06:07:18PM +0000, Rafael Ascensão wrote: > > > On Mon, Nov 12, 2018 at 07:14:23AM -0500, Jeff King wrote: > > > just adding a bunch of color variants. It would be nice if we could just > > > do this with a run-time parse_color("bold red") or whatever, but we use > > > these as static initializers. > > > > I suggested those colors, but now, I think this needs to be > > configurable. > > I think they are configurable in that patch, since it provides > "worktree" as a n entry in color_branch_slots. But yeah, every color we > add needs to be configurable, and this is really just about defaults. > > > I suggested using green and dim green as the obvious theoretical choice > > but after using it for a while I found out that both shades are way too > > similar, making it really hard to tell by glancing at the output, > > especially when they're not side by side. > > > > If we continue with two dual green approach, current branch needs to be > > at least bold. But I'm not sure if it's enough. > > > > I've been trying some other colors, and cyan feels neutral-ish. > > Yeah, cyan seems pretty reasonable to me. > > -Peff
diff --git a/builtin/branch.c b/builtin/branch.c index 0c55f7f065..34f44c82d7 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -42,11 +42,12 @@ static struct object_id head_oid; static int branch_use_color = -1; static char branch_colors[][COLOR_MAXLEN] = { GIT_COLOR_RESET, - GIT_COLOR_NORMAL, /* PLAIN */ - GIT_COLOR_RED, /* REMOTE */ - GIT_COLOR_NORMAL, /* LOCAL */ - GIT_COLOR_GREEN, /* CURRENT */ - GIT_COLOR_BLUE, /* UPSTREAM */ + GIT_COLOR_NORMAL, /* PLAIN */ + GIT_COLOR_RED, /* REMOTE */ + GIT_COLOR_NORMAL, /* LOCAL */ + GIT_COLOR_GREEN, /* CURRENT */ + GIT_COLOR_BLUE, /* UPSTREAM */ + GIT_COLOR_FAINT_LIGHT_GREEN, /* WORKTREE */ }; enum color_branch { BRANCH_COLOR_RESET = 0, @@ -54,7 +55,8 @@ enum color_branch { BRANCH_COLOR_REMOTE = 2, BRANCH_COLOR_LOCAL = 3, BRANCH_COLOR_CURRENT = 4, - BRANCH_COLOR_UPSTREAM = 5 + BRANCH_COLOR_UPSTREAM = 5, + BRANCH_COLOR_WORKTREE = 6 }; static const char *color_branch_slots[] = { @@ -64,6 +66,7 @@ static const char *color_branch_slots[] = { [BRANCH_COLOR_LOCAL] = "local", [BRANCH_COLOR_CURRENT] = "current", [BRANCH_COLOR_UPSTREAM] = "upstream", + [BRANCH_COLOR_WORKTREE] = "worktree", }; static struct string_list output = STRING_LIST_INIT_DUP; @@ -342,9 +345,10 @@ static char *build_format(struct ref_filter *filter, int maxwidth, const char *r struct strbuf local = STRBUF_INIT; struct strbuf remote = STRBUF_INIT; - strbuf_addf(&local, "%%(if)%%(HEAD)%%(then)* %s%%(else) %s%%(end)", - branch_get_color(BRANCH_COLOR_CURRENT), - branch_get_color(BRANCH_COLOR_LOCAL)); + strbuf_addf(&local, "%%(if)%%(HEAD)%%(then)* %s%%(else)%%(if)%%(worktree)%%(then)+ %s%%(else) %s%%(end)%%(end)", + branch_get_color(BRANCH_COLOR_CURRENT), + branch_get_color(BRANCH_COLOR_WORKTREE), + branch_get_color(BRANCH_COLOR_LOCAL)); strbuf_addf(&remote, " %s", branch_get_color(BRANCH_COLOR_REMOTE)); diff --git a/color.h b/color.h index 98894d6a17..857653df73 100644 --- a/color.h +++ b/color.h @@ -42,6 +42,24 @@ struct strbuf; #define GIT_COLOR_FAINT_BLUE "\033[2;34m" #define GIT_COLOR_FAINT_MAGENTA "\033[2;35m" #define GIT_COLOR_FAINT_CYAN "\033[2;36m" +#define GIT_COLOR_LIGHT_RED "\033[91m" +#define GIT_COLOR_LIGHT_GREEN "\033[92m" +#define GIT_COLOR_LIGHT_YELLOW "\033[93m" +#define GIT_COLOR_LIGHT_BLUE "\033[94m" +#define GIT_COLOR_LIGHT_MAGENTA "\033[95m" +#define GIT_COLOR_LIGHT_CYAN "\033[96m" +#define GIT_COLOR_BOLD_LIGHT_RED "\033[1;91m" +#define GIT_COLOR_BOLD_LIGHT_GREEN "\033[1;92m" +#define GIT_COLOR_BOLD_LIGHT_YELLOW "\033[1;93m" +#define GIT_COLOR_BOLD_LIGHT_BLUE "\033[1;94m" +#define GIT_COLOR_BOLD_LIGHT_MAGENTA "\033[1;95m" +#define GIT_COLOR_BOLD_LIGHT_CYAN "\033[1;96m" +#define GIT_COLOR_FAINT_LIGHT_RED "\033[2;91m" +#define GIT_COLOR_FAINT_LIGHT_GREEN "\033[2;92m" +#define GIT_COLOR_FAINT_LIGHT_YELLOW "\033[2;93m" +#define GIT_COLOR_FAINT_LIGHT_BLUE "\033[2;94m" +#define GIT_COLOR_FAINT_LIGHT_MAGENTA "\033[2;95m" +#define GIT_COLOR_FAINT_LIGHT_CYAN "\033[2;96m" #define GIT_COLOR_BG_RED "\033[41m" #define GIT_COLOR_BG_GREEN "\033[42m" #define GIT_COLOR_BG_YELLOW "\033[43m" diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 478b82cf9b..e404f6e23c 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -292,7 +292,7 @@ test_expect_success 'git branch --list -v with --abbrev' ' test_expect_success 'git branch --column' ' COLUMNS=81 git branch --column=column >actual && cat >expected <<\EOF && - a/b/c bam foo l * master n o/p r + a/b/c + bam foo l * master n o/p r abc bar j/k m/m master2 o/o q EOF test_cmp expected actual @@ -307,7 +307,7 @@ test_expect_success 'git branch --column with an extremely long branch name' ' cat >expected <<EOF && a/b/c abc - bam ++ bam bar foo j/k @@ -332,7 +332,7 @@ test_expect_success 'git branch with column.*' ' git config --unset column.branch && git config --unset column.ui && cat >expected <<\EOF && - a/b/c bam foo l * master n o/p r + a/b/c + bam foo l * master n o/p r abc bar j/k m/m master2 o/o q EOF test_cmp expected actual @@ -349,7 +349,7 @@ test_expect_success 'git branch -v with column.ui ignored' ' cat >expected <<\EOF && a/b/c abc - bam ++ bam bar foo j/k diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh index ee6787614c..06771fac64 100755 --- a/t/t3203-branch-output.sh +++ b/t/t3203-branch-output.sh @@ -240,6 +240,27 @@ test_expect_success 'git branch --format option' ' test_i18ncmp expect actual ' +test_expect_success '"add" a worktree' ' + mkdir worktree_dir && + git worktree add -b master_worktree worktree_dir master +' + +cat >expect <<'EOF' +* <GREEN>(HEAD detached from fromtag)<RESET> + ambiguous<RESET> + branch-one<RESET> + branch-two<RESET> + master<RESET> ++ <FAINT;LGREEN>master_worktree<RESET> + ref-to-branch<RESET> -> branch-one + ref-to-remote<RESET> -> origin/branch-one +EOF +test_expect_success TTY 'worktree colors correct' ' + test_terminal git branch >actual.raw && + test_decode_color <actual.raw >actual && + test_cmp expect actual +' + test_expect_success "set up color tests" ' echo "<RED>master<RESET>" >expect.color && echo "master" >expect.bare && diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 78d8c3783b..2831a42a88 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -61,6 +61,12 @@ test_decode_color () { if (n == 45) return "BMAGENTA"; if (n == 46) return "BCYAN"; if (n == 47) return "BWHITE"; + if (n == 91) return "LRED"; + if (n == 92) return "LGREEN"; + if (n == 93) return "LYELLOW"; + if (n == 94) return "LBLUE"; + if (n == 95) return "LMAGENTA"; + if (n == 96) return "LCYAN"; } { while (match($0, /\033\[[0-9;]*m/) != 0) {