Re: [PATCH v3 2/2] worktree add: add --orphan flag

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Nov 10 2022, Jacob Abel wrote:

> Adds support for creating an orphan branch when adding a new worktree.
> This functionality is equivalent to git switch's --orphan flag.
>
> The original reason this feature was implemented was to allow a user
> to initialise a new repository using solely the worktree oriented
> workflow. Example usage included below.
>
> $ GIT_DIR=".git" git init --bare
> $ git worktree add --orphan master master/
>
> Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
> ---
>  Documentation/git-worktree.txt | 14 +++++++-
>  builtin/worktree.c             | 64 ++++++++++++++++++++++++++++++----
>  t/t2400-worktree-add.sh        | 45 ++++++++++++++++++++++++
>  3 files changed, 115 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 4dd658012b..1310bfb564 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -10,7 +10,7 @@ SYNOPSIS
>  --------
>  [verse]
>  'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]]
> -		   [[-b | -B] <new-branch>] <path> [<commit-ish>]
> +		   [[-b | -B | --orphan] <new-branch>] <path> [<commit-ish>]
>  'git worktree list' [-v | --porcelain [-z]]
>  'git worktree lock' [--reason <string>] <worktree>
>  'git worktree move' <worktree> <new-path>
> @@ -95,6 +95,14 @@ exist, a new branch based on `HEAD` is automatically created as if
>  `-b <branch>` was given.  If `<branch>` does exist, it will be checked out
>  in the new worktree, if it's not checked out anywhere else, otherwise the
>  command will refuse to create the worktree (unless `--force` is used).
> ++
> +------------
> +$ git worktree add --orphan <branch> <path>
> +------------
> ++
> +Create a worktree containing an orphan branch named `<branch>` with a
> +clean working directory.  See `--orphan` in linkgit:git-switch[1] for
> +more details.

Seeing as "git switch" is still marked "EXPERIMENTAL", it may be prudent
in general to avoid linking to it in lieu of "git checkout".

In this case in particular though the "more details" are almost
completely absent from the "git-switch" docs, and they don't (which is
their won flaw) link to the more detailed "git-checkout" docs.

But for this patch, it seems much better to link to the "checkout" docs,
no?

> +--orphan <new-branch>::
> +	With `add`, create a new orphan branch named `<new-branch>` in the new
> +	worktree. See `--orphan` in linkgit:git-switch[1] for details.

Ditto.

> +test_expect_success '"add" --orphan/-b mutually exclusive' '
> +	test_must_fail git worktree add --orphan poodle -b poodle bamboo
> +'
> +
> +test_expect_success '"add" --orphan/-B mutually exclusive' '
> +	test_must_fail git worktree add --orphan poodle -B poodle bamboo
> +'
> +
> +test_expect_success '"add" --orphan/--detach mutually exclusive' '
> +	test_must_fail git worktree add --orphan poodle --detach bamboo
> +'
> +
> +test_expect_success '"add" --orphan/--no-checkout mutually exclusive' '
> +	test_must_fail git worktree add --orphan poodle --no-checkout bamboo
> +'
> +
> +test_expect_success '"add" -B/--detach mutually exclusive' '
> +	test_must_fail git worktree add -B poodle --detach bamboo main
> +'
> +

This would be much better as a for-loop:

for opt in -b -B ...
do
	test_expect_success "...$opt" '<test here, uses $opt>'
done

Note the ""-quotes for the description, and '' for the test, that's not
a mistake, we eval() the latter.

> +test_expect_success '"add --orphan" fails if the branch already exists' '
> +	test_when_finished "git branch -D existingbranch" &&
> +	test_when_finished "git worktree remove -f -f orphandir" &&
> +	git worktree add -b existingbranch orphandir main &&
> +	test_must_fail git worktree add --orphan existingbranch orphandir2 &&
> +	test ! -d orphandir2

I'm not sure about "worktree" behavior, but maybe this "test ! -d" wants
to be a "test_path_is_missing"?

In general we try to test what a thing is, not what it isn't, in this
case don't we fail to create the dir entirely? So "not exists" would
cover it?