| Message ID | 20230116172824.93218-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | builtin/checkout: check the branch used in -B with worktrees | expand |
On Mon, Jan 16, 2023 at 12:30 PM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote: > builtin/checkout: check the branch used in -B with worktrees Thanks for working on this and coming up with a fix. As mentioned earlier, I had started looking into it[1], but lacked the time to disentangle the logic, so I'm glad to see a patch arrive so quickly. > When multiple worktrees are being used, checkout/switch check > that the target branch is not already checked out with code that > evolved from 8d9fdd7087 (worktree.c: check whether branch is rebased > in another worktree, 2016-04-22), but that logic wasn't valid for > -B/-C > > Avoid reusing the same `branch_info` structure for the checks and > assumming that it will be rejected later if is a new branch that > already exists as that doesn't apply to -B/-C. Even though I'm familiar with the bug report[2] which sparked this patch, I find the above description somewhat hard to digest; the high-level problem it is addressing doesn't jump off the page at me. Perhaps it can be rewritten something like this: checkout/switch: disallow checking out same branch in multiple worktrees Commands `git switch -C` and `git checkout -B` neglect to check whether the branch being forcibly created is already checked out in some other worktree, which can result in the undesirable situation of the same branch being checked out in multiple worktrees. For instance: $ 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. after which you might include some details which you wrote about initially. > Reported-by: Jinwook Jeong <vustthat@gmail.com> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > diff --git a/builtin/checkout.c b/builtin/checkout.c > @@ -1533,13 +1534,12 @@ static int checkout_branch(struct checkout_opts *opts, > - 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); > + (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path))) > + die_if_checked_out(check_branch_info->path, 1); This variable name change (`new_branch_info` => `check_branch_info`) helps make the code clearer. Good. (I had found it more than a little confusing to have similar named variables `new_branch_info` and `opts->new_branch` even though they are unrelated and have very different purposes.) > diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh > @@ -125,6 +125,13 @@ test_expect_success 'die the same branch is already checked out' ' > +test_expect_success 'die the same branch is already checked out (checkout -B)' ' > + ( > + cd here && > + test_must_fail git checkout -B newmain > + ) > +' Although `git switch` and `git checkout` currently share implementation, that might not always be the case going forward. As such, this test could be made a bit more robust by testing both commands rather than just `git-checkout`. So, perhaps: test_expect_success 'die the same branch is already checked out' ' ( cd here && test_must_fail git checkout -B newmain && test_must_fail git switch -C newmain ) ' > +test_expect_success 'not die on re-checking out current branch (checkout -B)' ' > + ( > + cd there && > + git checkout -B newmain > + ) > +' Good to see you considered this case too. (I had tested it myself manually when trying out your patch.) [1]: https://lore.kernel.org/git/CAPig+cQc1+D9gH7BAC-r03bGKWx3a9jpPyLuP-ehH-X2P+fV6Q@mail.gmail.com/ [2]: https://lore.kernel.org/git/CAA3Q-aaO=vcZd9VLFr8UP-g06be80eUWN_GjygfyGkYmrLx9yQ@mail.gmail.com/
Hi Carlo. Thank you for working on this. On 16/1/23 18:28, Carlo Marcelo Arenas Belón wrote: > @@ -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); > + (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path))) > + die_if_checked_out(check_branch_info->path, 1); You adjusted this block to accommodate the problem with "checkout -B", which is sensible, but we may need to do something different here. What we are doing here, if I understand it correctly, is dying if the branch is _not checked out in the current worktree_ and _checked out in any other worktree_. Which is OK for normal checkout/switch. But IMHO with "checkout -B" we have to die if the branch is checked out in any other worktree, regardless of whether or not it is checked out in the current working tree. Perhaps the scenario where the user has the same branch checked out in multiple worktrees and tries to reset in one of them, is one of those where we could use the "if it hurts, don't do it". But we are providing the "--ignore-other-worktrees" modifier, so I think we should disallow the "-B" if the branch is checked out in any other worktree, and let the user use "--ignore-other-worktrees" if he wants to go wild. But.. die_if_checked_out() does not correctly honor the "ignore_current_worktree" in this situation. I have submitted a patch to fix this, in case you want to consider all of this. Un saludo.
On Mon, Jan 16, 2023 at 4:53 PM Rubén Justo <rjusto@gmail.com> wrote: > On 16/1/23 18:28, Carlo Marcelo Arenas Belón wrote: > > > @@ -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); > > + (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path))) > > + die_if_checked_out(check_branch_info->path, 1); > > What we are doing here, if I understand it correctly, is dying if the > branch is _not checked out in the current worktree_ and _checked out in > any other worktree_. Which is OK for normal checkout/switch. I think the exception was added to `checkout` only, where it is definitely needed, but switch probably should be more strict as it is not "plumbing" and as you pointed out there is already a UI option to override its safety. > But IMHO with "checkout -B" we have to die if the branch is checked out > in any other worktree, regardless of whether or not it is checked out in > the current working tree. I have to admit, I thought about that too, but then avoided making a change as checkout behaviour affects a lot of other places, but in retrospect I think that in this case it might be worth the change of behaviour, since it is connected with the bugfix. Before, the operation was allowed and the logic never tried to prevent it (which is why in my first look, I thought it might have been intentional), but once Eric pointed out it was a bug, then the obvious conclusion would be to prevent it with the extended logic, as you pointed out. > Perhaps the scenario where the user has the same branch checked out in > multiple worktrees and tries to reset in one of them, is one of those > where we could use the "if it hurts, don't do it". But we are providing > the "--ignore-other-worktrees" modifier, so I think we should disallow > the "-B" if the branch is checked out in any other worktree, and let > the user use "--ignore-other-worktrees" if he wants to go wild. v2 includes this and AFAIK nothing is broken yet. Carlo PS. As explained before, tightening `switch` might be a good idea, but it is obviously punted for this change and will be addressed later.
diff --git a/builtin/checkout.c b/builtin/checkout.c index 3fa29a08ee..94dcd617ef 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); + (!(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..283ba7607e 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -125,6 +125,13 @@ test_expect_success 'die the same branch is already checked out' ' ) ' +test_expect_success 'die the same branch is already checked out (checkout -B)' ' + ( + cd here && + test_must_fail git checkout -B newmain + ) +' + test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' ' head=$(git -C there rev-parse --git-path HEAD) && ref=$(git -C there symbolic-ref HEAD) && @@ -147,6 +154,13 @@ test_expect_success 'not die on re-checking out current branch' ' ) ' +test_expect_success 'not die on re-checking out current branch (checkout -B)' ' + ( + cd there && + git checkout -B newmain + ) +' + test_expect_success '"add" from a bare repo' ' ( git clone --bare . bare &&
When multiple worktrees are being used, checkout/switch check that the target branch is not already checked out with code that evolved from 8d9fdd7087 (worktree.c: check whether branch is rebased in another worktree, 2016-04-22), but that logic wasn't valid for -B/-C Avoid reusing the same `branch_info` structure for the checks and assumming that it will be rejected later if is a new branch that already exists as that doesn't apply to -B/-C. Reported-by: Jinwook Jeong <vustthat@gmail.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- builtin/checkout.c | 22 ++++++++++++++++------ t/t2400-worktree-add.sh | 14 ++++++++++++++ 2 files changed, 30 insertions(+), 6 deletions(-)