worktree add: shouldn't dwim if -b or -B is given

[yoichi]

CC: Jacob Abel jacobabel@nullpo.dev

[yoichi]

/preview

[gitgitgadget]

Preview email sent as pull.2192.git.1785851850309.gitgitgadget@gmail.com

[yoichi]

/submit

[gitgitgadget]

Submitted as pull.2192.git.1785852032626.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2192/yoichi/worktree-add-should-not-dwim-with-b-v1

To fetch this version to local tag pr-2192/yoichi/worktree-add-should-not-dwim-with-b-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2192/yoichi/worktree-add-should-not-dwim-with-b-v1

[gitgitgadget]

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.