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

From: Jacob Abel <[email-masked]>
Date: Sat, 19 Nov 2022 03:09:22 +0000

On 22/11/15 11:35PM, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Nov 15 2022, Eric Sunshine wrote:
>
> > On Tue, Nov 15, 2022 at 4:13 PM Ævar Arnfjörð Bjarmason
> > <[email-masked]> wrote:
> >> 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 <[email-masked]>
> >> > ---
> >> > +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?
> >
> > Sorry, no. The important point here is that the --orphan option being
> > added to `git worktree add` closely follows the behavior of `git
> > switch --orphan`, which is quite different from the behavior of `git
> > checkout --orphan`.
> >
> > The `git switch --orphan` documentation doesn't seem particularly
> > lacking; it correctly describes the (very) simplified behavior of that
> > command over `git checkout --orphan`. I might agree that there isn't
> > much reason to link to git-switch for "more details", though, since
> > there isn't really anything else that needs to be said.
>
> 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...
>
> > If we did want to say something else here, we might copy one sentence
> > from the `git checkout --orphan` documentation:
> >
> >     The first commit made on this new branch will have no parents and
> >     it will be the root of a new history totally disconnected from all
> >     the other branches and commits.
> >
> > The same sentence could be added to `git switch --orphan`
> > documentation, but that's outside the scope of this patch series (thus
> > can be done later by someone).
>
> 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/[email-masked]/
> 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]".

Apologies for the mistake in the SYNOPSIS. As mentioned in the other replies
I've updated it as you indicated to correct that.

As for a path forwards on the referencing of either git-checkout or git-switch
from git-worktree, I think I'm leaning towards Eric's approach (in his reply
to this message) where we don't reference either and fully outline the
behavior itself.

>
> >> > +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.
> >
> > Such a loop would need to be more complex than this, wouldn't it, to
> > account for all the combinations? I'd normally agree about the loop,
> > but given that it requires extra complexity, I don't really mind
> > seeing the individual tests spelled out manually in this case; they're
> > dead simple to understand as written. I don't feel strongly either
> > way, but I also don't want to ask for extra work from the patch author
> > for a subjective change.
>
> 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.

This is much clearer and more succinct. I've applied this to 2/2 for v4.