worktree add: shouldn't dwim if -b or -B is given - #2192
Open
yoichi wants to merge 1 commit into
Open
Conversation
yoichi
force-pushed
the
worktree-add-should-not-dwim-with-b
branch
from
August 4, 2026 12:46
1cd8461 to
908a32f
Compare
Author
|
/preview |
|
Preview email sent as pull.2192.git.1785851850309.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2192.git.1785852032626.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
>
> 'git worktree add <path> <branch>' DWIMs <branch> to a
> remote-tracking branch when neither -b, -B, nor --detach
> is given.
>
> However, 'git worktree add -b <new-branch> <path> <branch>' can
> still DWIM <branch>, causing <new-branch> to be ignored.
>
> This is a regression introduced in v2.42.0
> (128e5496b325640f0a09cc1d5b1e346c069b410f).
>
> Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
> ---
> worktree add: shouldn't dwim if -b or -B is given
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2192%2Fyoichi%2Fworktree-add-should-not-dwim-with-b-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2192/yoichi/worktree-add-should-not-dwim-with-b-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2192
>
> builtin/worktree.c | 2 +-
> t/t2400-worktree-add.sh | 10 ++++++++++
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 654d27c3e1..3204afdb12 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -897,7 +897,7 @@ static int add(int ac, const char **av, const char *prefix,
>
> /* DWIM: Infer --orphan when repo has no refs. */
> opts.orphan = (!s) && dwim_orphan(&opts, !!opt_track, 1);
> - } else if (ac == 2) {
> + } else if (ac == 2 && !new_branch) {
> struct object_id oid;
> struct commit *commit;
> char *remote;
This part checks 'branch' (assigned from av[1] earlier) to see if it
names a commit. When it does not, the code checks if it is the name
of a unique remote-tracking branch; if it is, the code uses that as
'branch', which is the origin to be used to fork 'new_branch' (av[1]
in this case) from.
Your observation is correct that this would overwrite 'new_branch'
if it were supplied.
Stepping back a bit, though, does this change the behavior when
'branch' *does* resolve to a commit (hence, the DWIM is already
bypassed and 'new_branch' or 'branch' are not nuked)? When 'ac' is
equal to 2 and 'new_branch' is supplied, we used to call:
if (!strcmp(branch, "HEAD"))
can_use_local_refs(&opts);
inside the block you are now skipping. It looks to me that this
patch also changes behavior when the user says:
$ git worktree add -b <new-branch> <path> HEAD
by not calling can_use_local_refs(), whose only effect in this
context is that it may issue a warning() to the user. I do not know
offhand what the ramifications of this difference are.
I wonder if we want to skip only the dwim part inside of this "else if"
arm, e.g.
diff --git i/builtin/worktree.c w/builtin/worktree.c
index 654d27c3e1..2205f4e9b2 100644
--- i/builtin/worktree.c
+++ w/builtin/worktree.c
@@ -898,6 +898,7 @@ static int add(int ac, const char **av, const char *prefix,
/* DWIM: Infer --orphan when repo has no refs. */
opts.orphan = (!s) && dwim_orphan(&opts, !!opt_track, 1);
} else if (ac == 2) {
+ if (!newbranch) {
struct object_id oid;
struct commit *commit;
char *remote;
@@ -910,6 +911,7 @@ static int add(int ac, const char **av, const char *prefix,
branch = new_branch_to_free = remote;
}
}
+ }
if (!strcmp(branch, "HEAD"))
can_use_local_refs(&opts);
Note that above diff is with broken indentation to help reduce the
patch noise to illustrate where the new block boundary would be.
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index 87b926728a..9cbf84861d 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -621,6 +621,16 @@ test_expect_success '"add" <path> <branch> dwims' '
> )
> '
>
> +test_expect_success '"add" <path> <branch> does not dwim with -b' '
> + test_when_finished rm -rf repo_upstream repo_dwim foo &&
> + setup_remote_repo repo_upstream repo_dwim &&
> + git init repo_dwim &&
> + (
> + cd repo_dwim &&
> + test_must_fail git worktree add -b branch ../foo foo
> + )
> +'
Two comments.
* You call setup_remote_repo(), which takes two names, runs
'git init' on each of them, and prepares those repositories. Do
you need to run 'git init repo_dwim' to initialize it again?
What for?
* Expecting a failure to create a worktree may be fine, but can we
say how we expect the command to fail as well? Perhaps you can
capture the error message while ensuring that the command fails,
and then check the error message to make sure it failed in the
expected way, or something like that.
Thanks. |
yoichi
force-pushed
the
worktree-add-should-not-dwim-with-b
branch
from
August 5, 2026 11:01
908a32f to
b00f6c2
Compare
|
Yoichi Nakayama wrote on the Git mailing list (how to reply to this email): On Wed, Aug 5, 2026 at 2:11 AM Junio C Hamano <gitster@pobox.com> wrote:
> Stepping back a bit, though, does this change the behavior when
> 'branch' *does* resolve to a commit (hence, the DWIM is already
> bypassed and 'new_branch' or 'branch' are not nuked)? When 'ac' is
> equal to 2 and 'new_branch' is supplied, we used to call:
>
> if (!strcmp(branch, "HEAD"))
> can_use_local_refs(&opts);
>
> inside the block you are now skipping. It looks to me that this
> patch also changes behavior when the user says:
>
> $ git worktree add -b <new-branch> <path> HEAD
>
> by not calling can_use_local_refs(), whose only effect in this
> context is that it may issue a warning() to the user. I do not know
> offhand what the ramifications of this difference are.
It is better to emit warnings for the given arguments whenever possible,
so I think we should not skip the step mentioned above.
I'll modify the condition.
> > diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> > index 87b926728a..9cbf84861d 100755
> > --- a/t/t2400-worktree-add.sh
> > +++ b/t/t2400-worktree-add.sh
> > @@ -621,6 +621,16 @@ test_expect_success '"add" <path> <branch> dwims' '
> > )
> > '
> >
> > +test_expect_success '"add" <path> <branch> does not dwim with -b' '
> > + test_when_finished rm -rf repo_upstream repo_dwim foo &&
> > + setup_remote_repo repo_upstream repo_dwim &&
> > + git init repo_dwim &&
> > + (
> > + cd repo_dwim &&
> > + test_must_fail git worktree add -b branch ../foo foo
> > + )
> > +'
>
> Two comments.
>
> * You call setup_remote_repo(), which takes two names, runs
> 'git init' on each of them, and prepares those repositories. Do
> you need to run 'git init repo_dwim' to initialize it again?
> What for?
'git init' was unnecessary. It was included because I copied
an existing test; I'll remove it.
> * Expecting a failure to create a worktree may be fine, but can we
> say how we expect the command to fail as well? Perhaps you can
> capture the error message while ensuring that the command fails,
> and then check the error message to make sure it failed in the
> expected way, or something like that.
Expected error is 'fatal: invalid reference: foo'. I'll add test_grep.
Thanks,
--
Yoichi NAKAYAMA |
|
User |
Author
|
/submit |
|
Submitted as pull.2192.v2.git.1785932208004.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
'git worktree add <path> <branch>' DWIMs <branch> to a remote-tracking branch when neither -b, -B, nor --detach is given. However, 'git worktree add -b <new-branch> <path> <branch>' can still DWIM <branch>, causing <new-branch> to be ignored. This is a regression introduced in v2.42.0 (128e549). Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
yoichi
force-pushed
the
worktree-add-should-not-dwim-with-b
branch
from
August 5, 2026 12:32
b00f6c2 to
477ea27
Compare
Author
|
/submit |
|
Submitted as pull.2192.v3.git.1785934486496.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Yoichi Nakayama wrote on the Git mailing list (how to reply to this email): On Wed, Aug 5, 2026 at 9:16 PM Yoichi NAKAYAMA via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Range-diff vs v1:
>
> 1: 908a32fb56 ! 1: b00f6c2fa1 worktree add: shouldn't dwim if -b or -B is given
> @@ Metadata
> ## Commit message ##
> worktree add: shouldn't dwim if -b or -B is given
>
> - 'git worktree add <path> <branch>' DWIMs <branch> to a
> + git worktree add <path> <branch> DWIMs <branch> to a
> remote-tracking branch when neither -b, -B, nor --detach
> is given.
>
> - However, 'git worktree add -b <new-branch> <path> <branch>' can
> + However, git worktree add -b <new-branch> <path> <branch> can
> still DWIM <branch>, causing <new-branch> to be ignored.
>
> This is a regression introduced in v2.42.0
I accidentally removed the quotes in the commit message. I'll fix it.
--
Yoichi NAKAYAMA |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
>
> 'git worktree add <path> <branch>' DWIMs <branch> to a
> remote-tracking branch when neither -b, -B, nor --detach
> is given.
>
> However, 'git worktree add -b <new-branch> <path> <branch>' can
> still DWIM <branch>, causing <new-branch> to be ignored.
>
> This is a regression introduced in v2.42.0
> (128e5496b325640f0a09cc1d5b1e346c069b410f).
I'll commit with the latter half of the above message rewritten like
this:
However, 'git worktree add -b <new-branch> <path> <branch>' can
still DWIM <branch>, causing <new-branch> to be ignored. This is a
regression introduced by 128e5496b3 (worktree add: extend DWIM to
infer --orphan, 2023-05-17), which appeared in Git 2.42.
As we refer to an individual commit using "log -1
--pretty=reference" format.
> Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
> ---
> worktree add: shouldn't dwim if -b or -B is given
The change looks good to me.
Thanks.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2192%2Fyoichi%2Fworktree-add-should-not-dwim-with-b-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2192/yoichi/worktree-add-should-not-dwim-with-b-v3
> Pull-Request: https://github.com/gitgitgadget/git/pull/2192
>
> Range-diff vs v2:
>
> 1: b00f6c2fa1 ! 1: 477ea27c8b worktree add: shouldn't dwim if -b or -B is given
> @@ Metadata
> ## Commit message ##
> worktree add: shouldn't dwim if -b or -B is given
>
> - git worktree add <path> <branch> DWIMs <branch> to a
> + 'git worktree add <path> <branch>' DWIMs <branch> to a
> remote-tracking branch when neither -b, -B, nor --detach
> is given.
>
> - However, git worktree add -b <new-branch> <path> <branch> can
> + However, 'git worktree add -b <new-branch> <path> <branch>' can
> still DWIM <branch>, causing <new-branch> to be ignored.
>
> This is a regression introduced in v2.42.0
>
>
> builtin/worktree.c | 22 ++++++++++++----------
> t/t2400-worktree-add.sh | 10 ++++++++++
> 2 files changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 654d27c3e1..cc46c1b415 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -898,16 +898,18 @@ static int add(int ac, const char **av, const char *prefix,
> /* DWIM: Infer --orphan when repo has no refs. */
> opts.orphan = (!s) && dwim_orphan(&opts, !!opt_track, 1);
> } else if (ac == 2) {
> - struct object_id oid;
> - struct commit *commit;
> - char *remote;
> -
> - commit = lookup_commit_reference_by_name(branch);
> - if (!commit) {
> - remote = unique_tracking_name(branch, &oid, NULL);
> - if (remote) {
> - new_branch = branch;
> - branch = new_branch_to_free = remote;
> + if (!new_branch) {
> + struct object_id oid;
> + struct commit *commit;
> + char *remote;
> +
> + commit = lookup_commit_reference_by_name(branch);
> + if (!commit) {
> + remote = unique_tracking_name(branch, &oid, NULL);
> + if (remote) {
> + new_branch = branch;
> + branch = new_branch_to_free = remote;
> + }
> }
> }
>
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index 87b926728a..ba3bec078f 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -621,6 +621,16 @@ test_expect_success '"add" <path> <branch> dwims' '
> )
> '
>
> +test_expect_success '"add" <path> <branch> does not dwim with -b' '
> + test_when_finished rm -rf repo_upstream repo_dwim wt &&
> + setup_remote_repo repo_upstream repo_dwim &&
> + (
> + cd repo_dwim &&
> + test_must_fail git worktree add -b branch ../wt foo 2>actual &&
> + test_grep "^fatal: invalid reference: foo" actual
> + )
> +'
> +
> test_expect_success '"add" <path> <branch> dwims with checkout.defaultRemote' '
> test_when_finished rm -rf repo_upstream repo_dwim foo &&
> setup_remote_repo repo_upstream repo_dwim &&
>
> base-commit: 5b2471720c93ee30e5764a19f3d3b3ae9ec9712a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CC: Jacob Abel jacobabel@nullpo.dev
cc: Yoichi Nakayama yoichi.nakayama@gmail.com