Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees

From: Jacob Abel <jacobabel@nullpo.dev>
Date: Nov 22, 2022, 23:26 UTC
Message-ID: <20221122232647.2jdsp5kioq7muymb@phi>
In-Reply-To: <CAPig+cTGATAYCpFcW2F6byf827-_TOyN1FNLfFCm0NdiReYVpg@mail.gmail.com>

On 22/11/22 12:16AM, Eric Sunshine wrote:

Show 31 quoted lines

> On Sat, Nov 19, 2022 at 6:49 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> > On Sat, Nov 19 2022, Jacob Abel wrote:
> > > I'd support adding an `advise()` for at least the basic case where you try to
> > > create a worktree and no branches currently exist in the repository.
> > > i.e. something like this:
> > >
> > >     % git -C foo.git worktree add foobar/
> > >     hint: If you meant to create a new initial branch for this repository,
> > >     hint: e.g. 'main', you can do so using the --orphan option:
> > >     hint:
> > >     hint:   git worktree add --orphan main main/
> > >     hint:
> > >     fatal: invalid reference: 'foobar'
> > > and
> > >     % git -C foo.git worktree add -b foobar foobardir/
> > >     hint: If you meant to create a new initial branch for this repository,
> > >     hint: e.g. 'main', you can do so using the --orphan option:
> > >     hint:
> > >     hint:   git worktree add --orphan main main/
> > >     hint:
> > >     fatal: invalid reference: 'foobar'
> >
> > I think those would make sense, yes.
>
> Yes, this sort of advice could go a long way toward addressing my
> discoverability concerns. (I think, too, we should be able to
> dynamically customize the advice to mention "foobar" rather than
> "main" in order to more directly help the user.) Along with that,
> explaining this use-case in the git-worktree documentation would also
> be valuable for improving discoverability.

Perfect. I think I've got this working already on my end using more or less the following:

    diff --git a/builtin/worktree.c b/builtin/worktree.c
    index 71786b72f6..f65b63d9d2 100644
    --- a/builtin/worktree.c
    +++ b/builtin/worktree.c
    @@ -736,7 +736,21 @@ static int add(int ac, const char **av, const char *prefix)
        if (!opts.quiet)
            print_preparing_worktree_line(opts.detach, branch, new_branch, !!new_branch_force);

    -	if (new_branch && !opts.orphan_branch) {
    +	if (opts.orphan_branch) {
    +		branch = new_branch;
    +	} else if (!lookup_commit_reference_by_name("head")) {
    +		/*
    +		 * if head does not reference a valid commit, only worktrees
    +		 * based on orphan branches can be created.
    +		 */
    +		advise("if you meant to create a new orphan branch for this repository,\n"
    +			 "e.g. '%s', you can do so using the --orphan option:\n"
    +			 "\n"
    +			 "	git worktree add --orphan %s %s\n"
    +			 "\n",
    +			 new_branch, new_branch, path);
    +		die(_("invalid reference: %s"), new_branch);
    +	} else if (new_branch) {
            struct child_process cp = child_process_init;
            cp.git_cmd = 1;
            strvec_push(&cp.args, "branch");

Show 16 quoted lines

>
> Updating the commit message of patch [2/2] to explain this more fully
> would also be helpful for reviewers. It wasn't clear to me, for
> instance, during initial reviews and discussion that you were adding
> --orphan to make this use-case possible. Simply including in the
> commit message an example usage and associated error of the current
> implementation:
>
>     % git init --bare foo.git
>     % git -C foo.git worktree add -b main bar
>     Preparing worktree (new branch 'main')
>     fatal: not a valid object name: 'HEAD'
>     %
>
> would go a long way to help reviewers understand what this series is
> trying to achieve (at least it would have helped me).

Will do.

Show 11 quoted lines

>
> > > Would there be any other circumstances where we'd definitely want an `advise()`?
> > > Generally I'd assume that outside of those two circumstances, most users will
> > > rarely intend to make an orphan without already knowing they absolutely need to
> > > make an orphan.
> >
> > I'm not familiar enough with the use-cases & workflow around "worktree"
> > to say, sorry.
>
> It's probably fine to limit this advice to `git worktree add`,
> certainly for an initial implementation.

Perfect. I'll work on getting the next revision for the patchset out then.