| Message ID | 20230119055325.1013-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] checkout/switch: disallow checking out same branch in multiple worktrees | expand |
Hi Carlo Thanks for working on this On 19/01/2023 05:53, 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 > by expanding on the existing checks that are being used by their non > force variants. > > Unlike what it is done for `git switch` and `git checkout`, that have > an historical exception to ignore other workspaces 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. I think it would be helpful to spell out the behavior of git checkout $current_branch git checkout -B $current_branch [<commit>] git checkout -B $current_branch --ignore-other-worktrees [<commit>] when the current branch is and is not checked out in another worktree as the tests are hard to follow because they rely on worktrees set up previous tests. > Reported-by: Jinwook Jeong <vustthat@gmail.com> > Helped-by: Rubén Justo <rjusto@gmail.com> > Helped-by: Eric Sunshine <sunshine@sunshineco.com> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > @@ -1818,10 +1831,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix, > strbuf_release(&buf); > } > > - if (opts->patch_mode || opts->pathspec.nr) > + if (opts->patch_mode || opts->pathspec.nr) { > + free(check_branch_path); > return checkout_paths(opts, new_branch_info); > + } > else > - return checkout_branch(opts, new_branch_info); > + return checkout_branch(opts, new_branch_info, check_branch_path); > } I found the ownership of check_branch_path confusing here. I think it would be clearer to do if (opts->patch_mode || opts->pathspec.nr) ret = checkout_path(...); else ret = checkout_branch(...); free(check_branch_path); return ret; > [...] > +test_expect_success 'but die if using force without --ignore-other-worktrees' ' I'm not sure from the title what this test is checking. Having added "git worktree list" and run it is checking that when the current branch is checked out elsewhere we require --ignore-other-worktrees when resetting the current branch. > + ( > + cd there && > + test_must_fail git checkout -B newmain && > + test_must_fail git switch -C newmain && > + git checkout --ignore-other-worktrees -B newmain && > + git switch --ignore-other-worktrees -C newmaain > ) I tried running git switch -C main newbranch from the main worktree (which is the only worktree that has branch 'main' checked out) to check that we did not error out when the branch is not checked out elsewhere and was surprised it failed with fatal: 'newbranch' is already checked out at '/dev/shm/trash directory.t2400-worktree-add/short-hand' It works as expected without this patch so it looks like there is a bug somewhere. Best Wishes Phillip
Merging this on top of 'seen' that used to pass the CI tests
https://github.com/git/git/actions/runs/3963948890
now fails many tests
https://github.com/git/git/actions/runs/3964312300
Funny thing is that nothing fails locally for me on x86_64 Linux
with gcc-12. Anything rings a bell?
Thanks.
I think it only fails its own test, and I can't reproduce locally either with macOS and clang. I was going to propose a newer version but it seems to be similarly affected: https://github.com/carenas/git/actions/runs/3964409976 It will be better to drop it for now; will produce one without the heisenbug, somehow Carlo
I should have known; the test is affected by the bug Rubén is trying to fix in: https://lore.kernel.org/git/eeb0c778-af0a-235c-f009-bca3abafdb15@gmail.com/ Where the order that the worktrees are found is not deterministic and so the ignore_current_worktree flag to die_if_checked_out() might trigger incorrectly. Carlo
diff --git a/builtin/checkout.c b/builtin/checkout.c index 5963e1b74b..467e194701 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1476,7 +1476,8 @@ static void die_if_some_operation_in_progress(void) } static int checkout_branch(struct checkout_opts *opts, - struct branch_info *new_branch_info) + struct branch_info *new_branch_info, + char *check_branch_path) { if (opts->pathspec.nr) die(_("paths cannot be used with switching branches")); @@ -1535,15 +1536,15 @@ 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 (check_branch_path && !opts->force_detach && !opts->ignore_other_worktrees) { int flag; char *head_ref = resolve_refdup("HEAD", 0, NULL, &flag); - if (head_ref && - (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path))) - die_if_checked_out(new_branch_info->path, 1); + if (opts->new_branch_force || (head_ref && + (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_path)))) + die_if_checked_out(check_branch_path, 1); free(head_ref); } + free(check_branch_path); if (!new_branch_info->commit && opts->new_branch) { struct object_id rev; @@ -1630,6 +1631,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, struct branch_info *new_branch_info) { int parseopt_flags = 0; + char *check_branch_path = NULL; opts->overwrite_ignore = 1; opts->prefix = prefix; @@ -1741,6 +1743,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, !opts->new_branch; int n = parse_branchname_arg(argc, argv, dwim_ok, new_branch_info, opts, &rev); + check_branch_path = xstrdup_or_null(new_branch_info->path); argv += n; argc -= n; } else if (!opts->accept_ref && opts->from_treeish) { @@ -1753,8 +1756,18 @@ static int checkout_main(int argc, const char **argv, const char *prefix, opts, &rev, opts->from_treeish); + check_branch_path = xstrdup_or_null(new_branch_info->path); if (!opts->source_tree) die(_("reference is not a tree: %s"), opts->from_treeish); + } else if (opts->new_branch && !opts->ignore_other_worktrees) { + struct object_id rev; + + if (!get_oid_mb(opts->new_branch, &rev)) { + struct strbuf buf = STRBUF_INIT; + strbuf_branchname(&buf, opts->new_branch, INTERPRET_BRANCH_LOCAL); + strbuf_splice(&buf, 0, 0, "refs/heads/", 11); + check_branch_path = strbuf_detach(&buf, NULL); + } } if (argc) { @@ -1818,10 +1831,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix, strbuf_release(&buf); } - if (opts->patch_mode || opts->pathspec.nr) + if (opts->patch_mode || opts->pathspec.nr) { + free(check_branch_path); return checkout_paths(opts, new_branch_info); + } else - return checkout_branch(opts, new_branch_info); + return checkout_branch(opts, new_branch_info, check_branch_path); } int cmd_checkout(int argc, const char **argv, const char *prefix) diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index d587e0b20d..a66f9bb30c 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -121,7 +121,10 @@ test_expect_success '"add" worktree creating new branch' ' test_expect_success 'die the same branch is already checked out' ' ( cd here && - test_must_fail git checkout newmain + test_must_fail git checkout newmain && + test_must_fail git checkout -B newmain && + test_must_fail git switch newmain && + test_must_fail git switch -C newmain ) ' @@ -143,7 +146,18 @@ test_expect_success 'not die the same branch is already checked out' ' test_expect_success 'not die on re-checking out current branch' ' ( cd there && - git checkout newmain + git checkout newmain && + git switch newmain + ) +' + +test_expect_success 'but die if using force without --ignore-other-worktrees' ' + ( + cd there && + test_must_fail git checkout -B newmain && + test_must_fail git switch -C newmain && + git checkout --ignore-other-worktrees -B newmain && + git switch --ignore-other-worktrees -C newmain ) '
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 by expanding on the existing checks that are being used by their non force variants. Unlike what it is done for `git switch` and `git checkout`, that have an historical exception to ignore other workspaces 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. Reported-by: Jinwook Jeong <vustthat@gmail.com> Helped-by: Rubén Justo <rjusto@gmail.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- Changes since v2 * A leak free implementation * More details in commit as suggested by Junio Changes since v1 * A much better commit message * Changes to the tests as suggested by Eric * Changes to the logic as suggested by Rubén builtin/checkout.c | 31 +++++++++++++++++++++++-------- t/t2400-worktree-add.sh | 18 ++++++++++++++++-- 2 files changed, 39 insertions(+), 10 deletions(-)