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>,
"Rubén Justo" <rjusto@gmail.com>,
"Eric Sunshine" <sunshine@sunshineco.com>
Subject: Re: [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees
Date: Thu, 19 Jan 2023 14:21:05 +0000 [thread overview]
Message-ID: <6a0499d4-ad6e-56b7-de72-0f8a3697b534@dunelm.org.uk> (raw)
In-Reply-To: <20230119055325.1013-1-carenas@gmail.com>
Hi Carlo
Thanks for working on this
On 19/01/2023 05:53, 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
> by expanding on the existing checks that are being used by their non
> force variants.
>
> Unlike what it is done for `git switch` and `git checkout`, that have
> an historical exception to ignore other workspaces 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.
I think it would be helpful to spell out the behavior of
git checkout $current_branch
git checkout -B $current_branch [<commit>]
git checkout -B $current_branch --ignore-other-worktrees [<commit>]
when the current branch is and is not checked out in another worktree
as the tests are hard to follow because they rely on worktrees set up
previous tests.
> Reported-by: Jinwook Jeong <vustthat@gmail.com>
> Helped-by: Rubén Justo <rjusto@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> @@ -1818,10 +1831,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
> strbuf_release(&buf);
> }
>
> - if (opts->patch_mode || opts->pathspec.nr)
> + if (opts->patch_mode || opts->pathspec.nr) {
> + free(check_branch_path);
> return checkout_paths(opts, new_branch_info);
> + }
> else
> - return checkout_branch(opts, new_branch_info);
> + return checkout_branch(opts, new_branch_info, check_branch_path);
> }
I found the ownership of check_branch_path confusing here. I think it
would be clearer to do
if (opts->patch_mode || opts->pathspec.nr)
ret = checkout_path(...);
else
ret = checkout_branch(...);
free(check_branch_path);
return ret;
> [...]
> +test_expect_success 'but die if using force without --ignore-other-worktrees' '
I'm not sure from the title what this test is checking. Having added
"git worktree list" and run it is checking that when the current branch
is checked out elsewhere we require --ignore-other-worktrees when
resetting the current branch.
> + (
> + cd there &&
> + 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 newmaain > )
I tried running
git switch -C main newbranch
from the main worktree (which is the only worktree that has branch
'main' checked out) to check that we did not error out when the branch
is not checked out elsewhere and was surprised it failed with
fatal: 'newbranch' is already checked out at '/dev/shm/trash
directory.t2400-worktree-add/short-hand'
It works as expected without this patch so it looks like there is a bug
somewhere.
Best Wishes
Phillip
next prev parent reply other threads:[~2023-01-19 14:21 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 [this message]
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
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=6a0499d4-ad6e-56b7-de72-0f8a3697b534@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).