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

On Tue, Nov 15 2022, Eric Sunshine wrote:

Aside from what it says now: 1/2 of what I'm saying is that linking to it while it says it's "EXPERIMENTAL" might be either jumping the gun.

Or maybe we should just declare it non-"EXPERIMENTAL", but in any case this unrelated topic might want to avoid that altogether and just link to the "checkout" version.

A quick grep of our docs (for linkgit:git-switch) that this would be the first mention outside of user-manual.txt where we link to it when it's not in the context of "checkout or switch", or where we're explaining something switch-specific (i.e. the "suggestDetachingHead" advice).

Having said that I don't really care, just a suggestion...

I think I was partially confused by skimming the SYNOPSIS and thinking this supported <start-point> like checkout, which as I found in https://lore.kernel.org/git/221115.86edu3kfqz.gmgdl@evledraar.gmail.com/ just seems to be a missing assertion where we want to die() if that's provided in this mode.

What I also found a bit confusing (but maybe it's just me) is that the "with a clean working directory" seemed at first to be drawing a distinction between this behavior and that of "git switch", but from poking at it some more it seems to be expressing "this is like git switch's --orphan" with that.

I think instead of "clean working tree" it would be better to talk about "tracked files", as "git switch --orphan" does, which AFAICT is what it means. But then again the reason "switch" does that is because you have *existing* tracked files, which inherently doesn't apply for "worktree".

Hrm.

So, I guess it depends on your mental model of this operation, but at least I think it's more intuitive to explain it in terms of "git checkout --orphan", not "git switch --orphan". I.e.:

	Create a worktree containing an orphan branch named
	`<branch>`. This works like linkgit:git-checkout[1]'s `--orphan'
	option, except '<start-point>` isn't supported, and the "clear
	the index" doesn't apply (as "worktree add" will always have a
	new index)".

Whereas defining this in terms of git-switch's "All tracked files are removed" might just be more confusing. What files? Since it's "worktree add" there weren't any in the first place.

Anyway, I don't mind it as it is, but maybe the above write-up helps for #leftoverbits if we ever want to unify these docs. I.e. AFAICT we could:

 * Link from git-worktree to git-checkout, saying the above
 * Link from git-switch to git-checkout, ditto, but that we also "remove
   tracked files [of the current HEAD]".

Yeah, it's probably not worth it. This is partially cleaning up existing
tests, but maybe:
	
	diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
	index 93c340f4aff..5acfd48f418 100755
	--- a/t/t2400-worktree-add.sh
	+++ b/t/t2400-worktree-add.sh
	@@ -298,37 +298,21 @@ test_expect_success '"add" no auto-vivify with --detach and <branch> omitted' '
	 	test_must_fail git -C mish/mash symbolic-ref HEAD
	 '
	 
	-test_expect_success '"add" -b/-B mutually exclusive' '
	-	test_must_fail git worktree add -b poodle -B poodle bamboo main
	-'
	-
	-test_expect_success '"add" -b/--detach mutually exclusive' '
	-	test_must_fail git worktree add -b poodle --detach bamboo main
	-'
	-
	-test_expect_success '"add" -B/--detach mutually exclusive' '
	-	test_must_fail git worktree add -B poodle --detach bamboo main
	-'
	-
	-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
	-'
	+test_wt_add_excl() {
	+	local opts="$@" &&
	+	test_expect_success "'worktree add' with '$opts' has mutually exclusive options" '
	+		test_must_fail git worktree add $opts
	+	'
	+}
	+test_wt_add_excl -b poodle -B poodle bamboo main
	+test_wt_add_excl -b poodle --orphan poodle bamboo
	+test_wt_add_excl -b poodle --detach bamboo main
	+test_wt_add_excl -B poodle --detach bamboo main
	+test_wt_add_excl -B poodle --detach bamboo main
	+test_wt_add_excl -B poodle --orphan poodle bamboo
	+test_wt_add_excl --orphan poodle --detach bamboo
	+test_wt_add_excl --orphan poodle --no-checkout bamboo
	+test_wt_add_excl --orphan poodle bamboo main
	 
	 test_expect_success '"add -B" fails if the branch is checked out' '
	 	git rev-parse newmain >before &&
	
I re-arranged that a bit, but probably not worth a loop. I *did* spot in
doing that that if I sort the options I end up with a duplicate test,
i.e. we test "-B poodle --detach bamboo main" twice.

That seems to be added by mistake in 2/2, i.e. it's the existing test you can see in the diff context, just added at the end.