| Message ID | 20230120113553.24655-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] checkout/switch: disallow checking out same branch in multiple worktrees | expand |
Hi Carlo 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. > Reported-by: Jinwook Jeong <vustthat@gmail.com> > Helped-by: Eric Sunshine <sunshine@sunshineco.com> > Helped-by: Rubén Justo <rjusto@gmail.com> > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > Changes since v3 > * Code and Tests improvements as suggested by Phillip > * Disable unreliable test that triggers a known bug > > 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 | 32 ++++++++++++++++++++++++-------- > t/t2400-worktree-add.sh | 34 +++++++++++++++++++++++++++------- > 2 files changed, 51 insertions(+), 15 deletions(-) > > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 3fa29a08ee..0688652f99 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, > + char *check_branch_path) > { > if (opts->pathspec.nr) > die(_("paths cannot be used with switching branches")); > @@ -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). > 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); I don't think it is worth a re-roll but the reformatting here is unfortunate. If you add the new condition at the end it is clearer what is being changed. if ((head_ref && (!(flag & REF_IS_YMREF) || strcmp(head_ref, check_branch_path))) || opts->new_branch_force) preserves the original code structure so one can see we've added a new condition and done s/new_branch_info->path/check_branch_path/ > free(head_ref); > } > > @@ -1627,7 +1628,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix, > const char * const usagestr[], > struct branch_info *new_branch_info) > { > + int ret; > int parseopt_flags = 0; > + char *check_branch_path = NULL; > > opts->overwrite_ignore = 1; > opts->prefix = prefix; > @@ -1717,6 +1720,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix, > opts->new_branch = argv0 + 1; > } > > + if (opts->new_branch && !opts->ignore_other_worktrees) { > + 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); > + } This block will run whenever -b/-B is given which is good > /* > * 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. > } else if (!opts->accept_ref && opts->from_treeish) { > struct object_id rev; > > @@ -1817,9 +1830,12 @@ 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); > + ret = checkout_paths(opts, new_branch_info); > else > - return checkout_branch(opts, new_branch_info); > + ret = checkout_branch(opts, new_branch_info, check_branch_path); > + > + free(check_branch_path); > + return ret; This is clearer now - thanks > } > > 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..7ab7e87440 100755 > --- a/t/t2400-worktree-add.sh > +++ b/t/t2400-worktree-add.sh > @@ -118,14 +118,17 @@ test_expect_success '"add" worktree creating new branch' ' > ) > ' > > -test_expect_success 'die the same branch is already checked out' ' > +test_expect_success 'die if 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 > ) > ' > > -test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' ' > +test_expect_success SYMLINKS 'die if the same branch is already checked out (symlink)' ' > head=$(git -C there rev-parse --git-path HEAD) && > ref=$(git -C there symbolic-ref HEAD) && > rm "$head" && > @@ -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). Is this just testing that "checkout -b/B <branch> <start-point>" works? > ( > cd here && > - git worktree add --force anothernewmain newmain > + git checkout -b conflictedmain newmain && > + git checkout -B conflictedmain newmain && > + git switch -C conflictedmain newmain > + ) > +' > + > +test_expect_success 'and not die on re-checking out current branch even if conflicted' ' I think 'allow re-checking out ...' would be clearer, again I'm not sure what's conflicted here. > + ( > + cd there && > + git checkout newmain && > + git switch newmain > ) > ' > > -test_expect_success 'not die on re-checking out current branch' ' > +test_expect_failure 'unless using force without --ignore-other-worktrees' ' This test passes for me - what's the reason for changing from test_expect_success to test_expect_failure? Thanks for working on this Phillip > ( > cd there && > - git checkout newmain > + 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 > ) > ' >
On Fri, Jan 20, 2023 at 7:08 AM Phillip Wood <phillip.wood123@gmail.com> 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' % 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? > > /* > > * 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. 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 > Is this just testing that "checkout -b/B <branch> > <start-point>" works? yes, but most importantly that we chose the right branch to check if both are provided and <start-point> is also a branch > > ( > > cd here && > > - git worktree add --force anothernewmain newmain > > + git checkout -b conflictedmain newmain && > > + git checkout -B conflictedmain newmain && > > + git switch -C conflictedmain newmain > > + ) > > +' > > + > > +test_expect_success 'and not die on re-checking out current branch even if conflicted' ' > > I think 'allow re-checking out ...' would be clearer, again I'm not sure > what's conflicted here. ok > > + ( > > + cd there && > > + git checkout newmain && > > + git switch newmain > > ) > > ' > > > > -test_expect_success 'not die on re-checking out current branch' ' > > +test_expect_failure 'unless using force without --ignore-other-worktrees' ' > > This test passes for me - what's the reason for changing from > test_expect_success to test_expect_failure? It also works for me, and for Junio, but somehow it didn't in the last runs from the CI and you could reproduce locally by going to the tree created above in the example I provided and doing: % cd ../base % git checkout -B main which should fail after finding that main is already checked out in `other`, but does not because when looking at the worktrees would first find the current one and not die, never aware there is another worktree with that same branch. the bug is the same one that Rubén is trying to address for rebase and that you commented on as well and that was mentioned before in this thread: https://lore.kernel.org/git/eeb0c778-af0a-235c-f009-bca3abafdb15@gmail.com/ Carlo
Hi Carlo On 20/01/2023 22:12, Carlo Arenas wrote: > On Fri, Jan 20, 2023 at 7:08 AM Phillip Wood <phillip.wood123@gmail.com> 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
Phillip Wood <phillip.wood123@gmail.com> writes: > Hi Carlo > ... > 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".... > ... >> + 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). > ... >> ... >> + 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. > ... >> +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). Is this just testing that "checkout -b/B <branch> > <start-point>" works? > ... >> +test_expect_success 'and not die on re-checking out current branch even if conflicted' ' > > I think 'allow re-checking out ...' would be clearer, again I'm not > sure what's conflicted here. > ... >> -test_expect_success 'not die on re-checking out current branch' ' >> +test_expect_failure 'unless using force without --ignore-other-worktrees' ' > > This test passes for me - what's the reason for changing from > test_expect_success to test_expect_failure? > > Thanks for working on this This topic has been dormant for a full two months. Aside from comments by Phillip (quoted above), are there remaining things to be done and issues to be resolved before we can see v5? Thanks.
On Wed, Mar 22, 2023 at 5:06 PM Junio C Hamano <gitster@pobox.com> wrote: > > Phillip Wood <phillip.wood123@gmail.com> writes: > > > Hi Carlo > > ... > > 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".... > > ... > >> + 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). > > ... > >> ... > >> + 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. > > ... > >> +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). Is this just testing that "checkout -b/B <branch> > > <start-point>" works? > > ... > >> +test_expect_success 'and not die on re-checking out current branch even if conflicted' ' > > > > I think 'allow re-checking out ...' would be clearer, again I'm not > > sure what's conflicted here. > > ... > >> -test_expect_success 'not die on re-checking out current branch' ' > >> +test_expect_failure 'unless using force without --ignore-other-worktrees' ' > > > > This test passes for me - what's the reason for changing from > > test_expect_success to test_expect_failure? > > > > Thanks for working on this > > are there remaining things > to be done and issues to be resolved before we can see v5? it interacted with another branch (rj/avoid-switching-to-already-used-branch) that was just recently merged to master. it also was probably too aggressive as pointed[1] by Phillip after my explanation of the change of behaviour as quoted: >> 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. I wasn't sure on how to proceed from there, but will come with a v5 to discuss further. Carlo [1] https://lore.kernel.org/git/a848b7d5-fd40-b043-7ed9-1672f65312e6@dunelm.org.uk/
On 27-ene-2023 14:46:45, Phillip Wood wrote: > Hi Carlo > > On 20/01/2023 22:12, Carlo Arenas wrote: > > On Fri, Jan 20, 2023 at 7:08 AM Phillip Wood <phillip.wood123@gmail.com> 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. We say in the documentation: + If `-B` is given, `<new-branch>` is created if it doesn't exist; otherwise, it is reset. This is the transactional equivalent of + ------------ $ git branch -f <branch> [<start-point>] $ git checkout <branch> ------------ + and, since 55c4a67307 (Prevent force-updating of the current branch, 2011-08-20), we return with error on "git branch -f <current-branch> [...]". Therefore, when the current branch is checked out in multiple worktrees, it seems consequent to error on "git checkout -B <current_branch> [...]". But we want to allow "git checkout -B <current_branch>", without <start-point>, as a way to say "git reset --keep", see: 39bd6f7261 (Allow checkout -B <current-branch> to update the current branch, 2011-11-26). Your suggestion not to error is not only reasonable, but also seems desirable. Thanks.
On 20/1/23 12: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. > > Reported-by: Jinwook Jeong <vustthat@gmail.com> > Helped-by: Eric Sunshine <sunshine@sunshineco.com> > Helped-by: Rubén Justo <rjusto@gmail.com> > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > Changes since v3 > * Code and Tests improvements as suggested by Phillip > * Disable unreliable test that triggers a known bug > > 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 | 32 ++++++++++++++++++++++++-------- > t/t2400-worktree-add.sh | 34 +++++++++++++++++++++++++++------- > 2 files changed, 51 insertions(+), 15 deletions(-) > > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 3fa29a08ee..0688652f99 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, > + char *check_branch_path) > { > if (opts->pathspec.nr) > die(_("paths cannot be used with switching branches")); > @@ -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)) { > 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); > } > > @@ -1627,7 +1628,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix, > const char * const usagestr[], > struct branch_info *new_branch_info) > { > + int ret; > int parseopt_flags = 0; > + char *check_branch_path = NULL; > > opts->overwrite_ignore = 1; > opts->prefix = prefix; > @@ -1717,6 +1720,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix, > opts->new_branch = argv0 + 1; > } > > + if (opts->new_branch && !opts->ignore_other_worktrees) { > + 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); > + } > /* > * 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); > } else if (!opts->accept_ref && opts->from_treeish) { > struct object_id rev; > > @@ -1817,9 +1830,12 @@ 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); > + ret = checkout_paths(opts, new_branch_info); > else > - return checkout_branch(opts, new_branch_info); > + ret = checkout_branch(opts, new_branch_info, check_branch_path); > + > + free(check_branch_path); > + return ret; > } > > 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..7ab7e87440 100755 > --- a/t/t2400-worktree-add.sh > +++ b/t/t2400-worktree-add.sh > @@ -118,14 +118,17 @@ test_expect_success '"add" worktree creating new branch' ' > ) > ' > > -test_expect_success 'die the same branch is already checked out' ' > +test_expect_success 'die if 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 > ) > ' > > -test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' ' > +test_expect_success SYMLINKS 'die if the same branch is already checked out (symlink)' ' > head=$(git -C there rev-parse --git-path HEAD) && > ref=$(git -C there symbolic-ref HEAD) && > rm "$head" && > @@ -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' ' > ( > cd here && > - git worktree add --force anothernewmain newmain > + git checkout -b conflictedmain newmain && > + git checkout -B conflictedmain newmain && > + git switch -C conflictedmain newmain > + ) > +' > + > +test_expect_success 'and not die on re-checking out current branch even if conflicted' ' > + ( > + cd there && > + git checkout newmain && > + git switch newmain > ) > ' > > -test_expect_success 'not die on re-checking out current branch' ' > +test_expect_failure 'unless using force without --ignore-other-worktrees' ' The fix to what caused this test to occasionally fail is already in 'master'. This test should now succeed consistently. > ( > cd there && > - git checkout newmain > + 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 > ) > ' > > Thanks for continuing to work on this.
diff --git a/builtin/checkout.c b/builtin/checkout.c index 3fa29a08ee..0688652f99 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, + char *check_branch_path) { if (opts->pathspec.nr) die(_("paths cannot be used with switching branches")); @@ -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)) { 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); } @@ -1627,7 +1628,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix, const char * const usagestr[], struct branch_info *new_branch_info) { + int ret; int parseopt_flags = 0; + char *check_branch_path = NULL; opts->overwrite_ignore = 1; opts->prefix = prefix; @@ -1717,6 +1720,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix, opts->new_branch = argv0 + 1; } + if (opts->new_branch && !opts->ignore_other_worktrees) { + 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); + } /* * 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); } else if (!opts->accept_ref && opts->from_treeish) { struct object_id rev; @@ -1817,9 +1830,12 @@ 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); + ret = checkout_paths(opts, new_branch_info); else - return checkout_branch(opts, new_branch_info); + ret = checkout_branch(opts, new_branch_info, check_branch_path); + + free(check_branch_path); + return ret; } 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..7ab7e87440 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -118,14 +118,17 @@ test_expect_success '"add" worktree creating new branch' ' ) ' -test_expect_success 'die the same branch is already checked out' ' +test_expect_success 'die if 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 ) ' -test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' ' +test_expect_success SYMLINKS 'die if the same branch is already checked out (symlink)' ' head=$(git -C there rev-parse --git-path HEAD) && ref=$(git -C there symbolic-ref HEAD) && rm "$head" && @@ -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' ' ( cd here && - git worktree add --force anothernewmain newmain + git checkout -b conflictedmain newmain && + git checkout -B conflictedmain newmain && + git switch -C conflictedmain newmain + ) +' + +test_expect_success 'and not die on re-checking out current branch even if conflicted' ' + ( + cd there && + git checkout newmain && + git switch newmain ) ' -test_expect_success 'not die on re-checking out current branch' ' +test_expect_failure 'unless using force without --ignore-other-worktrees' ' ( cd there && - git checkout newmain + 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. 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. Reported-by: Jinwook Jeong <vustthat@gmail.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Helped-by: Rubén Justo <rjusto@gmail.com> Helped-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- Changes since v3 * Code and Tests improvements as suggested by Phillip * Disable unreliable test that triggers a known bug 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 | 32 ++++++++++++++++++++++++-------- t/t2400-worktree-add.sh | 34 +++++++++++++++++++++++++++------- 2 files changed, 51 insertions(+), 15 deletions(-)