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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 15, 2022, 21:29 UTC
Message-ID: <CAPig+cRuJVN2Hc-oNF10sx=ZzArb8skXUQ8m5Qek2e-o4c9VEg@mail.gmail.com>
In-Reply-To: <221115.86iljfkjjo.gmgdl@evledraar.gmail.com>

On Tue, Nov 15, 2022 at 4:13 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 26 quoted lines

> 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>
> > ---
> > +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.

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).

Show 30 quoted lines

> > +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.