Git Mailing List Archive mirror
 help / color / mirror / Atom feed
From: Jacob Abel <jacobabel@nullpo.dev>
To: "Rubén Justo" <rjusto@gmail.com>
Cc: "Eric Sunshine" <sunshine@sunshineco.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	git@vger.kernel.org, "Taylor Blau" <me@ttaylorr.com>
Subject: Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
Date: Wed, 23 Nov 2022 05:37:21 +0000	[thread overview]
Message-ID: <20221123053708.rrbcphg7uukafdsg@phi> (raw)
In-Reply-To: <e027dc5b-dbb9-c511-038e-5440d59d976c@gmail.com>

On 22/11/23 03:43AM, Rubén Justo wrote:
> 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");


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

>
> 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/


  reply	other threads:[~2022-11-23  5:37 UTC|newest]

Thread overview: 132+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-04  1:02 [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Jacob Abel
2022-11-04  1:03 ` [PATCH 1/4] worktree add: Include -B in usage docs Jacob Abel
2022-11-04  3:05   ` Eric Sunshine
2022-11-04  4:24     ` Jacob Abel
2022-11-04  1:03 ` [PATCH 2/4] builtin/worktree.c: Update checkout_worktree() to use git-worktree Jacob Abel
2022-11-04  1:32   ` Ævar Arnfjörð Bjarmason
2022-11-04  3:58     ` Jacob Abel
2022-11-04 20:45     ` Taylor Blau
2022-11-04  1:03 ` [PATCH 3/4] worktree add: add --orphan flag Jacob Abel
2022-11-04  1:33   ` Ævar Arnfjörð Bjarmason
2022-11-04  4:11     ` Jacob Abel
2022-11-04  5:03   ` Eric Sunshine
2022-11-04 16:41     ` Jacob Abel
2022-11-10  4:13       ` Eric Sunshine
2022-11-10 21:21         ` Jacob Abel
2022-11-04  1:03 ` [PATCH 4/4] worktree add: Add unit tests for --orphan Jacob Abel
2022-11-04  1:37   ` Ævar Arnfjörð Bjarmason
2022-11-04  4:17     ` Jacob Abel
2022-11-04  4:33 ` [PATCH 0/4] worktree: Support `--orphan` when creating new worktrees Eric Sunshine
2022-11-04  4:47   ` Jacob Abel
2022-11-04  4:50   ` Jacob Abel
2022-11-04 21:34 ` [PATCH v2 0/2] " Jacob Abel
2022-11-04 21:34   ` [PATCH v2 1/2] worktree add: Include -B in usage docs Jacob Abel
2022-11-04 21:34   ` [PATCH v2 2/2] worktree add: add --orphan flag Jacob Abel
2022-11-10 23:32   ` [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Jacob Abel
2022-11-10 23:32     ` [PATCH v3 1/2] worktree add: Include -B in usage docs Jacob Abel
2022-11-10 23:32     ` [PATCH v3 2/2] worktree add: add --orphan flag Jacob Abel
2022-11-15 21:08       ` Ævar Arnfjörð Bjarmason
2022-11-15 21:29         ` Eric Sunshine
2022-11-15 22:35           ` Ævar Arnfjörð Bjarmason
2022-11-16  0:19             ` Eric Sunshine
2022-11-19  3:13               ` Jacob Abel
2022-11-19  3:09             ` Jacob Abel
2022-11-19 11:50               ` Ævar Arnfjörð Bjarmason
2022-11-19  1:44         ` Jacob Abel
2022-11-22  6:00           ` Eric Sunshine
2022-11-22 23:09             ` Jacob Abel
2022-11-15 22:09       ` Ævar Arnfjörð Bjarmason
2022-11-19  2:57         ` Jacob Abel
2022-11-19 11:50           ` Ævar Arnfjörð Bjarmason
2022-11-16  0:39     ` [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Eric Sunshine
2022-11-17 10:00       ` Ævar Arnfjörð Bjarmason
2022-11-19  3:47         ` Jacob Abel
2022-11-19 11:48           ` Ævar Arnfjörð Bjarmason
2022-11-22  5:16             ` Eric Sunshine
2022-11-22 23:26               ` Jacob Abel
2022-11-22 23:55                 ` Ævar Arnfjörð Bjarmason
2022-11-23  2:47                   ` Jacob Abel
2022-11-23  2:43                 ` Rubén Justo
2022-11-23  5:37                   ` Jacob Abel [this message]
2022-11-23  7:35                     ` Rubén Justo
2022-11-22 14:45           ` Phillip Wood
2022-11-23  4:21             ` Jacob Abel
2022-12-12  1:42     ` [PATCH v4 0/3] " Jacob Abel
2022-12-12  1:42       ` [PATCH v4 1/3] worktree add: Include -B in usage docs Jacob Abel
2022-12-12  1:42       ` [PATCH v4 2/3] worktree add: add --orphan flag Jacob Abel
2022-12-12  8:11         ` Ævar Arnfjörð Bjarmason
2022-12-12 14:55           ` Jacob Abel
2022-12-12 18:14             ` Ævar Arnfjörð Bjarmason
2022-12-12 22:39               ` Jacob Abel
2022-12-12  1:43       ` [PATCH v4 3/3] worktree add: Add hint to use --orphan when bad ref Jacob Abel
2022-12-12  8:35         ` Ævar Arnfjörð Bjarmason
2022-12-12 14:59           ` Jacob Abel
2022-12-12 18:16             ` Ævar Arnfjörð Bjarmason
2022-12-12 18:35               ` Eric Sunshine
2022-12-12 22:36                 ` Jacob Abel
2022-12-12 22:38               ` Jacob Abel
2022-12-20  2:37       ` [PATCH v5 0/4] worktree: Support `--orphan` when creating new worktrees Jacob Abel
2022-12-20  2:37         ` [PATCH v5 1/4] worktree add: Include -B in usage docs Jacob Abel
2022-12-20  3:42           ` Junio C Hamano
2022-12-20 23:24             ` Jacob Abel
2022-12-20  2:37         ` [PATCH v5 2/4] worktree add: refactor opt exclusion tests Jacob Abel
2022-12-20  4:00           ` Junio C Hamano
2022-12-20 23:29             ` Jacob Abel
2022-12-20  2:38         ` [PATCH v5 3/4] worktree add: add --orphan flag Jacob Abel
2022-12-20  4:19           ` Junio C Hamano
2022-12-21  0:17             ` Jacob Abel
2022-12-20  2:38         ` [PATCH v5 4/4] worktree add: Add hint to use --orphan when bad ref Jacob Abel
2022-12-20  6:18           ` Junio C Hamano
2022-12-21  0:42             ` Jacob Abel
2022-12-28  6:16         ` [PATCH v6 0/4] worktree: Support `--orphan` when creating new worktrees Jacob Abel
2022-12-28  6:16           ` [PATCH v6 1/4] worktree add: include -B in usage docs Jacob Abel
2022-12-28  6:16           ` [PATCH v6 2/4] worktree add: refactor opt exclusion tests Jacob Abel
2022-12-28 12:54             ` Junio C Hamano
2022-12-29  6:51               ` Jacob Abel
2022-12-29 10:07                 ` Junio C Hamano
2022-12-29 20:48                   ` Jacob Abel
2023-01-06  6:31                   ` Jacob Abel
2023-01-06 12:34                     ` Junio C Hamano
2023-01-07  4:45                       ` Jacob Abel
2022-12-28  6:17           ` [PATCH v6 3/4] worktree add: add --orphan flag Jacob Abel
2022-12-28  6:17           ` [PATCH v6 4/4] worktree add: add hint to direct users towards --orphan Jacob Abel
2023-01-06 14:19             ` Phillip Wood
2022-12-28  8:01           ` [PATCH v6 0/4] worktree: Support `--orphan` when creating new worktrees Ævar Arnfjörð Bjarmason
2022-12-29  6:38             ` Jacob Abel
2022-12-29 10:42               ` Ævar Arnfjörð Bjarmason
2022-12-29 21:22                 ` Jacob Abel
2023-01-07  4:58           ` [PATCH v7 " Jacob Abel
2023-01-07  4:59             ` [PATCH v7 1/4] worktree add: include -B in usage docs Jacob Abel
2023-01-07  4:59             ` [PATCH v7 2/4] worktree add: refactor opt exclusion tests Jacob Abel
2023-01-08  7:13               ` Junio C Hamano
2023-01-08 15:08                 ` Jacob Abel
2023-01-07  4:59             ` [PATCH v7 3/4] worktree add: add --orphan flag Jacob Abel
2023-01-07  4:59             ` [PATCH v7 4/4] worktree add: add hint to direct users towards --orphan Jacob Abel
2023-01-09 12:26             ` [PATCH v7 0/4] worktree: Support `--orphan` when creating new worktrees Ævar Arnfjörð Bjarmason
2023-01-09 17:11               ` Jacob Abel
2023-01-09 17:21                 ` Ævar Arnfjörð Bjarmason
2023-01-09 17:26                   ` Jacob Abel
2023-01-09 17:32             ` [PATCH v8 " Jacob Abel
2023-01-09 17:32               ` [PATCH v8 1/4] worktree add: include -B in usage docs Jacob Abel
2023-01-09 17:33               ` [PATCH v8 2/4] worktree add: refactor opt exclusion tests Jacob Abel
2023-01-09 17:33               ` [PATCH v8 3/4] worktree add: add --orphan flag Jacob Abel
2023-01-13 10:20                 ` Phillip Wood
2023-01-13 17:32                   ` Junio C Hamano
2023-01-14 22:47                   ` Jacob Abel
2023-01-15  3:09                     ` Junio C Hamano
2023-01-15  3:41                       ` rsbecker
2023-01-15  3:49                         ` Junio C Hamano
2023-01-18 22:46                           ` 'Jacob Abel'
2023-01-18 22:18                       ` Jacob Abel
2023-01-19 15:32                         ` Ævar Arnfjörð Bjarmason
2023-01-19 16:32                           ` Junio C Hamano
2023-01-16 10:47                     ` Phillip Wood
2023-01-18 22:40                       ` Jacob Abel
2023-01-19 16:18                         ` Phillip Wood
2023-01-19 22:20                           ` Jacob Abel
2023-01-09 17:33               ` [PATCH v8 4/4] worktree add: add hint to direct users towards --orphan Jacob Abel
2023-01-09 19:20               ` [PATCH v8 0/4] worktree: Support `--orphan` when creating new worktrees Ævar Arnfjörð Bjarmason
2023-01-13 17:34                 ` Junio C Hamano
  -- strict thread matches above, loose matches on Subject: below --
2022-12-08 21:41 [PATCH v3 0/2] " Jacob Abel
2022-12-08 22:00 ` rsbecker
2022-12-12  0:38   ` 'Jacob Abel'

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221123053708.rrbcphg7uukafdsg@phi \
    --to=jacobabel@nullpo.dev \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=me@ttaylorr.com \
    --cc=rjusto@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).