| Message ID | 20230118061527.76218-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] checkout/switch: disallow checking out same branch in multiple worktrees | expand |
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > As reflected on the tests, this will change the behaviour of those > commands when they are invoked in a worktree that has that requested > branch checked out, as that matches the logic used by branch, is safer > (assuming both commands are user facing) and can be overriden with an > existing flag. ... meaning you can "--force", or something else? Allowing an existing option to be used as the safety valve does make sense, especially if the option is something users are already familiar with (like "--force") and naturally expected to work. There might need an documentation update. Back when "checkout -b" and "branch" was written, there wasn't "multiple worktrees connected to a single repository" hence there was no need to provide safety against checking out the same branch in two different places. "git branch" might have learned th give that safety while "git checkout -b", which _ought_ to be equivalent to "git branch" followed by "git checkout", might have forgot to do so. After this change, it may still be correct to say that "checkout -b" is equivalent to "branch" followed by "checkout", but if the documentation to "branch" talks about this safety, it probably deserves to be mentioned in the documentation to "checkout -b", as well, if only to give an appropriate place to talk about how to override it "with an existing flag". Thanks.
On Tue, Jan 17, 2023 at 10:52 PM Junio C Hamano <gitster@pobox.com> wrote: > > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > > As reflected on the tests, this will change the behaviour of those > > commands when they are invoked in a worktree that has that requested > > branch checked out, as that matches the logic used by branch, is safer > > (assuming both commands are user facing) and can be overriden with an > > existing flag. > > ... meaning you can "--force", or something else? Allowing an > existing option to be used as the safety valve does make sense, > especially if the option is something users are already familiar > with (like "--force") and naturally expected to work. the following is the way to override: $ git checkout --ignore-other-worktrees -B foo > There might need an documentation update. Back when "checkout -b" > and "branch" was written, there wasn't "multiple worktrees connected > to a single repository" hence there was no need to provide safety > against checking out the same branch in two different places. "git > branch" might have learned to give that safety while "git checkout > -b", which _ought_ to be equivalent to "git branch" followed by "git > checkout", might have forgot to do so. Not sure if it was originally forgotten, but it is definitely working now; this change only fixes the uppercase (-B) version. > After this change, it may > still be correct to say that "checkout -b" is equivalent to "branch" > followed by "checkout", but if the documentation to "branch" talks > about this safety, it probably deserves to be mentioned in the > documentation to "checkout -b", as well, if only to give an appropriate > place to talk about how to override it "with an existing flag". Interestingly, when the flag was added in 1d0fa898ea (checkout: add --ignore-other-wortrees, 2015-01-03), it was only added to `checkout`. `git branch` has no flag and will die even when `-f` is used Carlo
Carlo Arenas <carenas@gmail.com> writes: > the following is the way to override: > > $ git checkout --ignore-other-worktrees -B foo My points were that (1) the option is way too unintuitive to anybody, other than those who stared at the implementation of the problematic logic for too long, and that (2) it wasn't mentioned in the proposed log message or documentation updates. If "--force" is made to mean that, it might be easier to discover to the users, but I have no strong opinion on that (meaning: there may be downsides to allow use of "--force" to override this safety, given that "-B" is already considered as a "forcing" version of "-b"). > `git branch` has no flag and will die even when `-f` is used If "--force" does not force it, I suspect it should be considered a bug. Thanks.
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > 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 I queued this topic at the tip of 'seen' as 2fe0b4e3 (Merge branch 'cb/checkout-same-branch-twice' into seen, 2023-01-18), on top of 4ea8693b (Merge branch 'mc/credential-helper-auth-headers' into seen, 2023-01-18). - 4ea8693b - https://github.com/git/git/actions/runs/3952916442 - 2fe0b4e3 - https://github.com/git/git/actions/runs/3953521066 Comparing these two runs, inclusion of this topic seems to introduce new leaks, as t1408 and t2018 (neither of which was touched by this topic) that used to pass are now failing. > builtin/checkout.c | 24 +++++++++++++++++------- > t/t2400-worktree-add.sh | 18 ++++++++++++++++-- > 2 files changed, 33 insertions(+), 9 deletions(-) Thanks.
diff --git a/builtin/checkout.c b/builtin/checkout.c index 3fa29a08ee..58a956392b 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1474,7 +1474,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, + struct branch_info *check_branch_info) { if (opts->pathspec.nr) die(_("paths cannot be used with switching branches")); @@ -1533,13 +1534,12 @@ 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_info->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_info->path)))) + die_if_checked_out(check_branch_info->path, 1); free(head_ref); } @@ -1628,6 +1628,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, struct branch_info *new_branch_info) { int parseopt_flags = 0; + struct branch_info check_branch_info = { 0 }; opts->overwrite_ignore = 1; opts->prefix = prefix; @@ -1739,6 +1740,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_info = *new_branch_info; argv += n; argc -= n; } else if (!opts->accept_ref && opts->from_treeish) { @@ -1751,8 +1753,16 @@ static int checkout_main(int argc, const char **argv, const char *prefix, opts, &rev, opts->from_treeish); + check_branch_info = *new_branch_info; if (!opts->source_tree) die(_("reference is not a tree: %s"), opts->from_treeish); + } else if (opts->new_branch) { + struct object_id rev; + + if (!get_oid_mb(opts->new_branch, &rev)) + setup_new_branch_info_and_source_tree(&check_branch_info, + opts, &rev, + opts->new_branch); } if (argc) { @@ -1819,7 +1829,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix, if (opts->patch_mode || opts->pathspec.nr) return checkout_paths(opts, new_branch_info); else - return checkout_branch(opts, new_branch_info); + return checkout_branch(opts, new_branch_info, &check_branch_info); } 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 example: $ 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. As reflected on the tests, this will change the behaviour of those commands when they are invoked in a worktree that has that requested branch checked out, as that matches the logic used by branch, is safer (assuming both commands are user facing) and can be overriden with an existing flag. 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 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 | 24 +++++++++++++++++------- t/t2400-worktree-add.sh | 18 ++++++++++++++++-- 2 files changed, 33 insertions(+), 9 deletions(-)