Hi Carlo
On 20/01/2023 22:12, Carlo Arenas wrote:
On Fri, Jan 20, 2023 at 7:08 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
On 20/01/2023 11:35, Carlo Marcelo Arenas Belón wrote:
Commands `git switch -C` and `git checkout -B` neglect to check whether
the provided branch is already checked out in some other worktree, as
shown by the following:
$ git worktree list
.../foo beefb00f [main]
$ git worktree add ../other
Preparing worktree (new branch 'other')
HEAD is now at beefb00f first
$ cd ../other
$ git switch -C main
Switched to and reset branch 'main'
$ git worktree list
.../foo beefb00f [main]
.../other beefb00f [main]
Fix this problem by teaching `git switch -C` and `git checkout -B` to
check whether the branch in question is already checked out elsewhere.
Unlike what it is done for `git switch` and `git checkout`, that have
an historical exception to ignore other worktrees if the branch to
check is the current one (as required when called as part of other
tools), the logic implemented is more strict and will require the user
to invoke the command with `--ignore-other-worktrees` to explicitly
indicate they want the risky behaviour.
This matches the current behaviour of `git branch -f` and is safer; for
more details see the tests in t2400.
As I said before, it would be much easier for everyone else to
understand the changes if you wrote out what they were rather than
saying "look at the tests". I do appreciate the improved test
descriptions though - thanks for that.
Apologies on that, I tried to come up with something that would
describe the change of behaviour other than the paragraph above and
couldn't come out with a better explanation except reading the tests
(which I know is complicated by the fact they are interlinked).
The behaviour I am changing is also not documented (other than by the
test) and might have been added as a quirk to keep the scripted rebase
and bisect going when confronted with branches that were checked out
in multiple worktrees, and as such might had not be intended for
`switch`, and might not be needed anymore either.
Using`checkout` for simplicity, but also applies to `switch`,
% git worktree list
.../base 6a45aba [main]
% git worktree add -f ../other main
Preparing worktree (checking out 'main')
HEAD is now at 6a45aba init
% cd ../other
% git checkout main
Already on 'main'
% git checkout -B main
fatal: 'main' is already checked out at '.../base'
Thanks for explaining that. If there is no <start-point> given we don't
reset the branch so it seems a bit harsh to error out here. For "git
checkout -B <branch> <start-point>" when <branch> is checked out in
another worktree requiring --ignore-other-worktrees makes sense.
% git checkout --ignore-other-worktrees -B main
Already on 'main'
The change of behaviour only applies to -B and it actually matches the
documentation better.
@@ -1533,13 +1534,13 @@ static int checkout_branch(struct checkout_opts *opts,
if (!opts->can_switch_when_in_progress)
die_if_some_operation_in_progress();
- if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
- !opts->ignore_other_worktrees) {
+ if (!opts->ignore_other_worktrees && !opts->force_detach &&
+ check_branch_path && ref_exists(check_branch_path)) {
I think check_branch_path is NULL if opts->ignore_other_worktrees is set
so we could maybe lose "!opts->ignore_other_worktrees" here (or possibly
below where you set check_branch_path).
opts->ignore_other_worktrees was kept from the original expression;
you are correct that is not needed anymore, but I thought it didn't
hurt and made the code intention clearer (meaning it is obvious to
anyone new to the code that this code will be skipped if that flag is
set), would using an assert or a comment be a better option?
It's a good point that it makes the intention clearer, maybe we should
just leave it as it is.
/*
* Extract branch name from command line arguments, so
* all that is left is pathspecs.
@@ -1741,6 +1751,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
new_branch_info, opts, &rev);
argv += n;
argc -= n;
+
+ if (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)
+ check_branch_path = xstrdup(new_branch_info->path);
I'm a bit confused what this is doing.
The branch we are interested in might come from 2 places, either it is
a parameter to -b, which was picked up before, or it is the argument
to the command itself, which was detected above.
Oh, of course. I was so focused on the -b that I'd forgotten we need the
same check when we're checking out an existing branch - thanks for
putting me right.
If both are provided, we want to make sure to use the one from -b, or
will have the bug you sharply spotted before, which was frankly
embarrassing.
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index d587e0b20d..7ab7e87440 100755
@@ -133,17 +136,34 @@ test_expect_success SYMLINKS 'die the same branch is already checked out (symlin
test_must_fail git -C here checkout newmain
'
-test_expect_success 'not die the same branch is already checked out' '
+test_expect_success 'allow creating multiple worktrees for same branch with force' '
+ git worktree add --force anothernewmain newmain
+'
+
+test_expect_success 'allow checkout/reset from the conflicted branch' '
I'm not sure what "the conflicted branch" means (it reminds we of merge
conflicts).
by "conflicted" I meant one that is checked out in more than one worktree
I think it would be clearer so say that rather than "conflicted" which
has a strong association with merge conflicts.
Best Wishes
Phillip
