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

From: Jacob Abel <jacobabel@nullpo.dev>
Date: Nov 23, 2022, 05:37 UTC
Message-ID: <20221123053708.rrbcphg7uukafdsg@phi>
In-Reply-To: <e027dc5b-dbb9-c511-038e-5440d59d976c@gmail.com>

On 22/11/23 03:43AM, Rubén Justo wrote:

Show 52 quoted lines

> On 22-nov-2022 23:26:57, Jacob Abel wrote:
> > On 22/11/22 12:16AM, Eric Sunshine wrote:
> > > 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")) {
>
> I haven't read the full thread and sorry to enter this way in the
> conversation, but this line got my attention.

No worries. It's always nice to have more eyes to catch mistakes.

> This needs to be "HEAD", in capital letters.

Ah yes. I wasn't paying attention when I copied it into my MUA and must have accidentally typed `ggvGu` instead of `ggvGy` and lowercased it before I copied it (thanks vim/user error). It should be:

    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 7 quoted lines

>
> Thank you for working on this, this is a thing that has hit me several
> times.
>
> The first impression got me thinking.. Why do we need this advise?
> Why not make the orphan branch right away? And why the argument for the
> --orphan option?

I went into my concerns with further overloading `worktree add -b/-B` and `worktree add` (DWYM) over on the other side of this thread [1]. I won't echo it all here but I wanted to mention a few things.

As for why we want the advise, by not short circuiting with the advise and instead just trying to DWYM, we can catch the following edge case:

A user less well acquainted with git tries out worktrees on a new project (no branches). They create multiple worktrees and since there are no branches, they are all orphans. Unless they've read the docs, they are now accustomed to this "new worktrees have no history" behavior. Then they make a commit on one of the orphans and the behavior changes and all new worktrees derive from that branch unless `git worktree add` is run from inside another worktree with a non-orphan branch.

There's more to it in the other thread but it gets kinda messy for the user if they walk off the well trodden path inadvertently. I'd like to avoid that all together where possible.

As for the argument, the reason is so that the syntax matches `git switch --orphan <new_branch>` (and the `git checkout` variant).

> I like what this new flag allows: make a new orphan branch when we
> are in any branch.  But if we are already in an orphan branch (like the
> initial) what's the user's expectation?

Like mentioned above (and in [1]), further overloading DWYM and `-b` impacts the already somewhat complex/unclear expectations for `git worktree add`.

When using the flag and not adding to `-b` and DWYM, we can short circuit this confusion for the most part by requiring the user to explicitly request `--orphan`.

As for creating a new orphan in a repo with existing branches but from a worktree containing an orphan branch, that fails cleanly as shown below:

    # in worktree with orphan branch
    % git worktree add -b foobar ../foobar
    Preparing worktree (new branch 'foobar')
    fatal: invalid reference: foobar

and in the next revision should fail with the following:

    # in worktree with orphan branch
    % git worktree add -b foobar ../foobar
    Preparing worktree (new branch 'foobar')
    hint: If you meant to create a new orphan branch for this repository,
    hint: e.g. 'foobar', you can do so using the --orphan option:
    hint:
    hint:   git worktree add --orphan foobar ../foobar/
    hint:
    fatal: invalid reference: foobar

> Maybe we can use the new flag to indicate that the user unconditionally
> wants an orphan branch, and use the rest of the arguments as they are,
> '-b' included.

I wouldn't necessarily be opposed to this however I do think changing it from `--orphan <new_branch>` to `--orphan -b <new_branch>` would be a departure from the syntax used in `git switch` and `git checkout` and that may make it harder for users already familar with those other commands.

Show 39 quoted lines

>
> This needs more work, but something like this:
>
> --- >8 ---
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index d774ff192a..1ea8d05c2f 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -406,7 +406,7 @@ static int add_worktree(const char *path, const char *refname,
>
>  	/* is 'refname' a branch or commit? */
>  	if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
> -	    ref_exists(symref.buf)) {
> +	    (opts->orphan_branch || ref_exists(symref.buf))) {
>  		is_branch = 1;
>  		if (!opts->force)
>  			die_if_checked_out(symref.buf, 0);
> @@ -738,18 +738,8 @@ static int add(int ac, const char **av, const char *prefix)
>
>  	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 && !lookup_commit_reference_by_name("HEAD")) {
> +		branch = opts.orphan_branch = new_branch;
>  	} else if (new_branch) {
>  		struct child_process cp = CHILD_PROCESS_INIT;
>  		cp.git_cmd = 1;

1. https://lore.kernel.org/git/20221123042052.t42jmsqjxgx2k3th@phi/