[v5,2/3] branch: Mark and color a branch differently if it is checked out in a linked worktree
diff mbox series

Message ID 20190106002619.54741-3-nbelakovski@gmail.com
State New
Headers show
Series
  • Untitled series #62625
Related show

Commit Message

Nickolai Belakovski Jan. 6, 2019, 12:26 a.m. UTC
From: Nickolai Belakovski <nbelakovski@gmail.com>

In order to more clearly display which branches are active, 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 simplify workflows related to worktree, particularly
due to the limitations of not being able to check out the same branch in
two worktrees and the inability to delete a branch checked out in a
worktree. When performing branch operations like checkout and delete, it
would be useful to know more readily if the branches in which the user
is interested are already checked out in a worktree.

The git worktree list command contains the relevant information, however
this is a much less frquently used command than git branch.
---
 Documentation/git-branch.txt | 15 ++++++++-------
 builtin/branch.c             | 12 ++++++++----
 t/t3200-branch.sh            |  8 ++++----
 t/t3203-branch-output.sh     | 21 +++++++++++++++++++++
 4 files changed, 41 insertions(+), 15 deletions(-)

Comments

Junio C Hamano Jan. 7, 2019, 7:04 p.m. UTC | #1
nbelakovski@gmail.com writes:

> From: Nickolai Belakovski <nbelakovski@gmail.com>
>
> In order to more clearly display which branches are active, 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 simplify workflows related to worktree, particularly
> due to the limitations of not being able to check out the same branch in
> two worktrees and the inability to delete a branch checked out in a
> worktree. When performing branch operations like checkout and delete, it
> would be useful to know more readily if the branches in which the user
> is interested are already checked out in a worktree.

I do not think it is warranted to paint the safety features as
"limitations".

A branch that is checked out in another worktree cannot be checked
out to be worked on, as that will make the checkout of the other
worktree out of sync.  If you want to work on that branch, you can
either (1) go to that worktree that has a checkout of that branch
and work there, or (2) go to that worktree that has a checkout of
that branch, check out a different branch there, come back to the
worktree you want to work in and check out that branch.  Knowing
where that other worktree is is the first step in either case.

And a branch that is checked out in a worktree cannot be removed, as
it is a sign that it is still being worked on for a branch to have
been checked out somewhere.  If you do want to remove that branch,
you need to go to that worktree that has a checkout of that branch,
check out a different branch there, and then remove it.  Again,
knowing where that other worktree is is the fist thing you need to
know.

But then I am not sure if the feature being added by these patches
is a good match for that justification.

For one thing, it would be more direct and helpful way for

	git checkout one-branch
	git branch -d one-branch

to say "The branch `one-branch` is checked out in a worktree at
$DIRECTORY" when they refused to go ahead.  And that would eliminate
the need for this new feature to help these two use cases.

In fact, these two command already behave that way, so the paragraph
I just commented on is not a good justification for this new feature
at all.

Besides, showing "That branch is checked out somewhere" would not
help user to decide "ah, if I want to work on that branch, I need to
chdir to that directory" with the patch in question, as it only
shows "It is checked out _somewhere_" without saying where.

> The git worktree list command contains the relevant information, however
> this is a much less frquently used command than git branch.

It is not a good justification.  If the "relevant information" given
by the command is necessary one, the user can run that command.  If
the situation where that "relevant information" becomes necessary is
rare, the command is run much less frequently is not a problem---it
is expected.  And overloading a more frequently used command with
information that is less frequently wanted is actually not a great
design.

A more relevant justification may be that even though the
information can already be found in "worktree list" output, it would
give us flexibility in presentation to allow the custom format in
for-each-ref to show it.

So, I am between moderately Meh to fairly negative on this step; Meh
in the sense that "thanks to the previous step, we _could_ do this,
it does not give incorrect information, and it makes the output more
cheerful, but it does not add that much useful and actionable piece
of information".
Philip Oakley Jan. 10, 2019, 9:42 p.m. UTC | #2
On 07/01/2019 19:04, Junio C Hamano wrote:
> nbelakovski@gmail.com writes:
>
>> From: Nickolai Belakovski <nbelakovski@gmail.com>
>>
>> In order to more clearly display which branches are active, 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 simplify workflows related to worktree, particularly
>> due to the limitations of not being able to check out the same branch in
>> two worktrees and the inability to delete a branch checked out in a
>> worktree. When performing branch operations like checkout and delete, it
>> would be useful to know more readily if the branches in which the user
>> is interested are already checked out in a worktree.
> I do not think it is warranted to paint the safety features as
> "limitations".

Is this not just a case of needing to clarify that this is 'safety' 
related to the _users_ mental model (or lack of) relative to the limited 
information that was previously given by the branch command's list.

You are right that there is no data safety issue, but users make 
mistakes when they misunderstand the situation.

>
> A branch that is checked out in another worktree cannot be checked
> out to be worked on, as that will make the checkout of the other
> worktree out of sync.  If you want to work on that branch, you can
> either (1) go to that worktree that has a checkout of that branch
> and work there, or (2) go to that worktree that has a checkout of
> that branch, check out a different branch there, come back to the
> worktree you want to work in and check out that branch.  Knowing
> where that other worktree is is the first step in either case.
>
> And a branch that is checked out in a worktree cannot be removed, as
> it is a sign that it is still being worked on for a branch to have
> been checked out somewhere.

I'm not sure that all users will recognise the signs, which I think is 
one reason for the value of the patch.


>    If you do want to remove that branch,
> you need to go to that worktree that has a checkout of that branch,
> check out a different branch there, and then remove it.  Again,
> knowing where that other worktree is is the fist thing you need to
> know.
>
> But then I am not sure if the feature being added by these patches
> is a good match for that justification.
I'd agree that the justification needs clarified.
>
> For one thing, it would be more direct and helpful way for
>
> 	git checkout one-branch
> 	git branch -d one-branch
>
> to say "The branch `one-branch` is checked out in a worktree at
> $DIRECTORY" when they refused to go ahead.  And that would eliminate
> the need for this new feature to help these two use cases.
>
> In fact, these two command already behave that way, so the paragraph
> I just commented on is not a good justification for this new feature
> at all.
>
> Besides, showing "That branch is checked out somewhere" would not
> help user to decide "ah, if I want to work on that branch, I need to
> chdir to that directory" with the patch in question, as it only
> shows "It is checked out _somewhere_" without saying where.
>
>> The git worktree list command contains the relevant information, however
>> this is a much less frquently used command than git branch.
> It is not a good justification.  If the "relevant information" given
> by the command is necessary one, the user can run that command.  If
> the situation where that "relevant information" becomes necessary is
> rare, the command is run much less frequently is not a problem---it
> is expected.  And overloading a more frequently used command with
> information that is less frequently wanted is actually not a great
> design.
But leaving the older command unaware of the newer developments and the 
user unwise as to its missing info is equally a poor situation.
>
> A more relevant justification may be that even though the
> information can already be found in "worktree list" output, it would
> give us flexibility in presentation to allow the custom format in
> for-each-ref to show it.
>
> So, I am between moderately Meh to fairly negative on this step; Meh
> in the sense that "thanks to the previous step, we _could_ do this,
> it does not give incorrect information, and it makes the output more
> cheerful, but it does not add that much useful and actionable piece
> of information".

The patch did appear to me as being a proper update to the branch 
command to include the information about the branches in the other worktrees

Philip
Nickolai Belakovski Jan. 13, 2019, 1:41 a.m. UTC | #3
On Thu, Jan 10, 2019 at 1:43 PM Philip Oakley <philipoakley@iee.org> wrote:
>
> On 07/01/2019 19:04, Junio C Hamano wrote:
> > I do not think it is warranted to paint the safety features as
> > "limitations".
>
> Is this not just a case of needing to clarify that this is 'safety'
> related to the _users_ mental model (or lack of) relative to the limited
> information that was previously given by the branch command's list.
>
> You are right that there is no data safety issue, but users make
> mistakes when they misunderstand the situation.

Not trying to paint anything one way or another. I found that these
features got in the way of my workflows and didn't see any immediate
reason why they had to exist. Thinking about it a bit more, is it
unreasonable to delete a branch even if it's checked out in a worktree
as long as the user uses git branch --delete --force or -D? This would
leave the worktree in a detached head state, but all the data would be
untouched. The output of deletion could mention that the branch had
been checked out in a worktree so that the user is fully informed.

Checking out the same branch in two worktrees should be technically
possible to implement safely, but I don't see a use case for having
the same branch checked out in multiple worktrees anyway. Why use
multiple worktrees at that point? If a user really wants the same
contents in two directories they can work around this
limitation/safety feature by just making another branch pointing at
the same commit. But anyway I'm just explaining why I chose the word
'limitation'.


> >    If you do want to remove that branch,
> > you need to go to that worktree that has a checkout of that branch,
> > check out a different branch there, and then remove it.  Again,
> > knowing where that other worktree is is the fist thing you need to
> > know.

This just seems silly to me when git branch --delete has a --force
option. But that's off topic.

> >
> >> The git worktree list command contains the relevant information, however
> >> this is a much less frquently used command than git branch.
> > It is not a good justification.  If the "relevant information" given
> > by the command is necessary one, the user can run that command.  If
> > the situation where that "relevant information" becomes necessary is
> > rare, the command is run much less frequently is not a problem---it
> > is expected.  And overloading a more frequently used command with
> > information that is less frequently wanted is actually not a great
> > design.
> But leaving the older command unaware of the newer developments and the
> user unwise as to its missing info is equally a poor situation.
> >
> > A more relevant justification may be that even though the
> > information can already be found in "worktree list" output, it would
> > give us flexibility in presentation to allow the custom format in
> > for-each-ref to show it.
> >
> > So, I am between moderately Meh to fairly negative on this step; Meh
> > in the sense that "thanks to the previous step, we _could_ do this,
> > it does not give incorrect information, and it makes the output more
> > cheerful, but it does not add that much useful and actionable piece
> > of information".

Allow me to add some color to my original commit message. The point of
this patch is so that the user is not surprised when they see the
message that this branch is checked out in another worktree when
trying to delete it or check it out, since they have presumably run
git branch recently and seen the formatted output indicating that a
branch they may want to delete/checkout is checked out in a worktree.
This was my frustration that prompted me to dive into this in the
first place - I'm cleaning up my branches and all of a sudden git
decides it doesn't want to let me delete one because it's checked out
somewhere else, even though I know I don't care about it because I
know the branch has already been merged upstream, or is old, or
whatever. I thought, if git branch output could at least let me know
that it's going to treat some branches differently, I can be proactive
about things and go to my worktree and delete the branch, or skip
trying to clean it up or not check it out.

I'll pursue the above-mentioned topic of allowing git branch -D to
allow the user to delete branches checked out in a worktree
separately, but even if that goes through, I think this patch would
still be useful in that it tells me that I can't check out the
branches that are colored in cyan.

Does that make more sense?
Junio C Hamano Jan. 14, 2019, 6:18 p.m. UTC | #4
Nickolai Belakovski <nbelakovski@gmail.com> writes:

> Not trying to paint anything one way or another. I found that these
> features got in the way of my workflows and didn't see any immediate
> reason why they had to exist. Thinking about it a bit more, is it
> unreasonable to delete a branch even if it's checked out in a worktree
> as long as the user uses git branch --delete --force or -D?

[For ease of discussion, let's assume that our worktree has a
checkout of branch B, and you are mucking with that branch in
another worktree connected to the same repository]

It is probably a sane enhancement to allow but require --force to
delete branch B in the other worktree.  It is a different matter to
allow "git branch -m AnotherBranch B" run in the other worktree to
overwrite branch B--it will be a disaster if we allowed it and then
committed what we have in our worktree.

> This would
> leave the worktree in a detached head state,

It does not.  It will leave us on branch B that is unborn, and the
next commit will start that branch with a root commit.

> but all the data would be
> untouched. 

As far as the person, who is working in our worktree, is concerned,
she wanted to extend the history of branch B before you deleted it
in another worktree.  But because you deleted B while she was
looking the other way, she ended up creating a root commit, losing
all the history behind that branch.  I'd grant you that "--force"
will give us an excuse when she complains, though.

Most likely, you and she are the same person, but one big point in
the ability to work in multiple worktrees is to allow the user to
switch context and multi-task.  When she gives branch B its own
worktree, she does so because she wants to have a stable place to
work on it, without getting affected by other things she does to the
repository in other worktrees.
Nickolai Belakovski Jan. 18, 2019, 10:18 p.m. UTC | #5
I will start a separate thread containing these replies for the
potential change to allow deleting branches checked out in worktrees.

Getting back on track for this series, specifically this 2/3 patch,
how do you feel about it? As I pointed out the goal is to communicate
to the user that the branches marked/colored will behave differently
from the other branches if the user tries to delete them or check them
out.

Patch
diff mbox series

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index bf5316ffa9..b3eca6ffdc 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -26,13 +26,14 @@  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,
-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
-it matches any of the patterns.  Note that when providing a
-`<pattern>`, you must use `--list`; otherwise the command is interpreted
+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 it matches any of the patterns.  Note that when providing
+a `<pattern>`, you must use `--list`; otherwise the command is interpreted
 as branch creation.
 
 With `--contains`, shows only the branches that contain the named commit
diff --git a/builtin/branch.c b/builtin/branch.c
index 0c55f7f065..2a24153b78 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 &&