diff mbox series

[v8,2/3] branch: update output to include worktree info

Message ID 20190219083123.27686-3-nbelakovski@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v8,1/3] ref-filter: add worktreepath atom | expand

Commit Message

Nickolai Belakovski Feb. 19, 2019, 8:31 a.m. UTC 
From: Nickolai Belakovski <nbelakovski@gmail.com>

The output of git branch is modified to mark branches checkout out in a
linked worktree with a "+" and color them in cyan (in contrast to the
current branch, which will still be denoted with a "*" and colored in green)

This is meant to communicate to the user that the branches that are
marked or colored will behave differently from other branches if the user
attempts to check them out or delete them, since branches checked out in
another worktree cannot be checked out or deleted.

Signed-off-by: Nickolai Belakovski <nbelakovski@gmail.com>
---
 Documentation/git-branch.txt |  6 ++++--
 builtin/branch.c             | 12 ++++++++----
 t/t3200-branch.sh            |  8 ++++----
 t/t3203-branch-output.sh     | 21 +++++++++++++++++++++
 4 files changed, 37 insertions(+), 10 deletions(-)

Comments

Jeff King Feb. 21, 2019, 12:44 p.m. UTC | #1 
On Tue, Feb 19, 2019 at 05:31:22PM +0900, nbelakovski@gmail.com wrote:

> From: Nickolai Belakovski <nbelakovski@gmail.com>
> 
> The output of git branch is modified to mark branches checkout out in a

s/checkout out/checked out/ ?

> linked worktree with a "+" and color them in cyan (in contrast to the
> current branch, which will still be denoted with a "*" and colored in green)
> 
> This is meant to communicate to the user that the branches that are
> marked or colored will behave differently from other branches if the user
> attempts to check them out or delete them, since branches checked out in
> another worktree cannot be checked out or deleted.

I think this makes sense to have. You cannot "git checkout" such a
marked branch (since it would conflict with the other worktree), and
that alone seems to make it worth marking in the output.

> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 3bd83a7cbd..f2e5a07d64 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -26,8 +26,10 @@ DESCRIPTION
>  -----------
>  
>  If `--list` is given, or if there are no non-option arguments, existing
> -branches are listed; the current branch will be highlighted with an
> -asterisk.  Option `-r` causes the remote-tracking branches to be listed,
> +branches are listed; the current branch will be highlighted in green and
> +marked with an asterisk.  Any branches checked out in linked worktrees will
> +be highlighted in cyan and marked with a plus sign. Option `-r` causes the
> +remote-tracking branches to be listed,

This makes sense to me.

> -	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)%%(worktreepath)%%(then)+ %s%%(else)  %s%%(end)%%(end)",
> +			branch_get_color(BRANCH_COLOR_CURRENT),
> +			branch_get_color(BRANCH_COLOR_WORKTREE),
> +			branch_get_color(BRANCH_COLOR_LOCAL));

Makes sense. The long line is ugly. Our format does not support breaking
long lines, though we could break the C string, I think, like:

  strbuf_add(&local,
	     "%%(if)%%(HEAD)"
	       "%%(then)* %s"
	     "%%(else)%(if)%%(worktreepath)"
	       "%%(then)+ %s"
	     "%%(else)"
	       "%%(then)  %s"
	     "%%(end)%%(end)");

That's pretty ugly, too, but it at least shows the conditional
structure.

> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> index ee6787614c..94ab05ad59 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
> +'

This mkdir gets deleted in the next patch. It should just not be added
here.

> +cat >expect <<'EOF'
> +* <GREEN>(HEAD detached from fromtag)<RESET>
> +  ambiguous<RESET>
> +  branch-one<RESET>
> +  branch-two<RESET>
> +  master<RESET>
> ++ <CYAN>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
> +'

We are not testing the auto-color behavior here, so I think you could
just use "git branch --color >actual.raw" and drop the TTY prerequisite.
That's shorter and simpler, and will let your test run on more
platforms.

-Peff
Nickolai Belakovski March 14, 2019, 5:45 a.m. UTC | #2 
On Thu, Feb 21, 2019 at 4:44 AM Jeff King <peff@peff.net> wrote:
>
> On Tue, Feb 19, 2019 at 05:31:22PM +0900, nbelakovski@gmail.com wrote:
>
> > From: Nickolai Belakovski <nbelakovski@gmail.com>
> >
> > The output of git branch is modified to mark branches checkout out in a
>
> s/checkout out/checked out/ ?
>
Yes, thanks
>
> > -     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)%%(worktreepath)%%(then)+ %s%%(else)  %s%%(end)%%(end)",
> > +                     branch_get_color(BRANCH_COLOR_CURRENT),
> > +                     branch_get_color(BRANCH_COLOR_WORKTREE),
> > +                     branch_get_color(BRANCH_COLOR_LOCAL));
>
> Makes sense. The long line is ugly. Our format does not support breaking
> long lines, though we could break the C string, I think, like:
>
>   strbuf_add(&local,
>              "%%(if)%%(HEAD)"
>                "%%(then)* %s"
>              "%%(else)%(if)%%(worktreepath)"
>                "%%(then)+ %s"
>              "%%(else)"
>                "%%(then)  %s"
>              "%%(end)%%(end)");
>
> That's pretty ugly, too, but it at least shows the conditional
> structure.
True, but other lines within that file are about as long. I'd feel
that I should make all of them reflect the conditional structure if
I'm going to make one of them reflect it. Granted none of the others
have nested if's, but personally I think it's OK as is. The nested if
is short enough.
>
> >
> > +test_expect_success '"add" a worktree' '
> > +     mkdir worktree_dir &&
> > +     git worktree add -b master_worktree worktree_dir master
> > +'
>
> This mkdir gets deleted in the next patch. It should just not be added
> here.
Whoops, removed
>
> > +cat >expect <<'EOF'
> > +* <GREEN>(HEAD detached from fromtag)<RESET>
> > +  ambiguous<RESET>
> > +  branch-one<RESET>
> > +  branch-two<RESET>
> > +  master<RESET>
> > ++ <CYAN>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
> > +'
>
> We are not testing the auto-color behavior here, so I think you could
> just use "git branch --color >actual.raw" and drop the TTY prerequisite.
> That's shorter and simpler, and will let your test run on more
> platforms.
>
Done locally, will be part of v9

> -Peff
diff mbox series

Patch

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 3bd83a7cbd..f2e5a07d64 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -26,8 +26,10 @@  DESCRIPTION
 -----------
 
 If `--list` is given, or if there are no non-option arguments, existing
-branches are listed; the current branch will be highlighted with an
-asterisk.  Option `-r` causes the remote-tracking branches to be listed,
+branches are listed; the current branch will be highlighted in green and
+marked with an asterisk.  Any branches checked out in linked worktrees will
+be highlighted in cyan and marked with a plus sign. Option `-r` causes the
+remote-tracking branches to be listed,
 and option `-a` shows both local and remote branches. If a `<pattern>`
 is given, it is used as a shell wildcard to restrict the output to
 matching branches. If multiple patterns are given, a branch is shown if
diff --git a/builtin/branch.c b/builtin/branch.c
index 1be727209b..c2a86362bb 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -47,6 +47,7 @@  static char branch_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_NORMAL,       /* LOCAL */
 	GIT_COLOR_GREEN,        /* CURRENT */
 	GIT_COLOR_BLUE,         /* UPSTREAM */
+	GIT_COLOR_CYAN,         /* 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)%%(worktreepath)%%(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/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..94ab05ad59 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>
++ <CYAN>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 &&