Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees
Hi Carlo
On 20/01/2023 11:35, Carlo Marcelo Arenas Belón wrote:
Show 28 quoted lines
> 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.
Show 46 quoted lines
> 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).
Show 8 quoted lines
> 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/
Show 24 quoted lines
> 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
Show 10 quoted lines
> /*
> * 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.
Show 15 quoted lines
> } 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
Show 38 quoted lines
> }
>
> 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?
Show 10 quoted lines
> (
> 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.
Show 9 quoted lines
> + (
> + 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
Show 10 quoted lines
> (
> 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
> )
> '
>
Carlo Marcelo Arenas Belón
