git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Jan 20, 2023, 15:08 UTC
Message-ID
<8f24fc3c-c30f-dc70-5a94-5ee4ed3de102@dunelm.org.uk>
In-Reply-To
<20230120113553.24655-1-carenas@gmail.com>
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
>   	)
>   '
>
Previous: Carlo Marcelo Arenas BelónNext: Carlo Arenas
Message 18 of 24 in “builtin/checkout: check the branch used in -B with worktrees”
  1. builtin/checkout: check the branch used in -B with worktreesCarlo Marcelo Arenas Belón, Jan 16, 2023
  2. Eric SunshineJan 16, 2023
  3. Rubén JustoJan 17, 2023
  4. Carlo ArenasJan 18, 2023
  5. checkout/switch: disallow checking out same branch in multiple worktreesCarlo Marcelo Arenas Belón, Jan 18, 2023
  6. Junio C HamanoJan 18, 2023
  7. Carlo ArenasJan 18, 2023
  8. Junio C HamanoJan 18, 2023
  9. Junio C HamanoJan 18, 2023
  10. checkout/switch: disallow checking out same branch in multiple worktreesCarlo Marcelo Arenas Belón, Jan 19, 2023
  11. Re* [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktreesJunio C Hamano, Jan 19, 2023
  12. Carlo ArenasJan 19, 2023
  13. Phillip WoodJan 19, 2023
  14. Junio C HamanoJan 20, 2023
  15. Carlo ArenasJan 20, 2023
  16. Carlo ArenasJan 20, 2023
  17. checkout/switch: disallow checking out same branch in multiple worktreesCarlo Marcelo Arenas Belón, Jan 20, 2023
  18. Phillip WoodJan 20, 2023
  19. Carlo ArenasJan 20, 2023
  20. Phillip WoodJan 27, 2023
  21. Rubén JustoMay 14, 2023
  22. Junio C HamanoMar 23, 2023
  23. Carlo ArenasMar 24, 2023
  24. Rubén JustoMay 14, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.