git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Phillip Wood <phillip.wood123@gmail.com>
To: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>, git@vger.kernel.org
Cc: pclouds@gmail.com, gitster@pobox.com,
	"Jinwook Jeong" <vustthat@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Rubén Justo" <rjusto@gmail.com>,
	"Phillip Wood" <phillip.wood123@gmail.com>
Subject: Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees
Date: Fri, 20 Jan 2023 15:08:02 +0000	[thread overview]
Message-ID: <8f24fc3c-c30f-dc70-5a94-5ee4ed3de102@dunelm.org.uk> (raw)
In-Reply-To: <20230120113553.24655-1-carenas@gmail.com>

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
>   	)
>   '
>   

  reply	other threads:[~2023-01-20 15:08 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-16 17:28 [PATCH] builtin/checkout: check the branch used in -B with worktrees Carlo Marcelo Arenas Belón
2023-01-16 22:18 ` Eric Sunshine
2023-01-17  0:53 ` Rubén Justo
2023-01-18  5:44   ` Carlo Arenas
2023-01-18  6:15 ` [PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees Carlo Marcelo Arenas Belón
2023-01-18  6:52   ` Junio C Hamano
2023-01-18  7:58     ` Carlo Arenas
2023-01-18 16:10       ` Junio C Hamano
2023-01-18 22:55   ` Junio C Hamano
2023-01-19  5:53   ` [PATCH v3] " Carlo Marcelo Arenas Belón
2023-01-19  7:23     ` Re* " Junio C Hamano
2023-01-19  7:41       ` Carlo Arenas
2023-01-19 14:21     ` Phillip Wood
2023-01-20  3:10     ` Junio C Hamano
2023-01-20  3:53       ` Carlo Arenas
2023-01-20  4:39         ` Carlo Arenas
2023-01-20 11:35     ` [PATCH v4] " Carlo Marcelo Arenas Belón
2023-01-20 15:08       ` Phillip Wood [this message]
2023-01-20 22:12         ` Carlo Arenas
2023-01-27 14:46           ` Phillip Wood
2023-05-14 20:21             ` Rubén Justo
2023-03-23  0:06         ` Junio C Hamano
2023-03-24  3:49           ` Carlo Arenas
2023-05-14 20:24       ` Rubén Justo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8f24fc3c-c30f-dc70-5a94-5ee4ed3de102@dunelm.org.uk \
    --to=phillip.wood123@gmail.com \
    --cc=carenas@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=rjusto@gmail.com \
    --cc=sunshine@sunshineco.com \
    --cc=vustthat@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).