git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-04 21:34 ` [PATCH v2 0/2] " Jacob Abel
@ 2022-11-10 23:32   ` Jacob Abel
  2022-11-16  0:39     ` Eric Sunshine
  0 siblings, 1 reply; 17+ messages in thread
From: Jacob Abel @ 2022-11-10 23:32 UTC (permalink / raw)
  To: git
  Cc: Jacob Abel, Ævar Arnfjörð Bjarmason, Eric Sunshine,
	Taylor Blau

While working with the worktree based git workflow, I realised that setting
up a new git repository required switching between the traditional and
worktree based workflows. Searching online I found a SO answer [1] which
seemed to support this and which indicated that adding support for this should
not be technically difficult.

This patchset has two parts:

  * adding `-B` to the usage docs (noticed during dev and it seemed too small
    to justify a separate submission)
  * adding orphan branch functionality (as is present in `git-switch`)
    to `git-worktree-add`

Changes from v2:

  * Changed orphan creation behavior to match `git switch --orphan` instead of
    `git checkout --orphan` [2][3]. As a result `--orphan` no longer accepts a
    `<commit-ish>` and creates the orphan branch with a clean working directory.
  * Removed the `opts.implicit` flag as it is no longer needed and
    `opts.orphan_branch` can be used instead.
  * No longer set `opts.force` when creating an orphan branch (as checkout can
    no longer fail in a way that `--force` would prevent).
  * Updated tests to no longer provide a `<commit-ish>`.
  * Removed no longer relevant test.
  * Added additional cleanup to tests.

1. https://stackoverflow.com/a/68717229/15064705/
2. https://lore.kernel.org/git/CAPig+cSVzewXpk+eDSC-W-+Q8X_7ikZXXeSQbmpHBcdLCU5svw@mail.gmail.com/
3. https://lore.kernel.org/git/20221110212132.3se4imsksjo3gsso@phi/

Jacob Abel (2):
  worktree add: Include -B in usage docs
  worktree add: add --orphan flag

 Documentation/git-worktree.txt | 14 +++++++-
 builtin/worktree.c             | 64 ++++++++++++++++++++++++++++++----
 t/t2400-worktree-add.sh        | 45 ++++++++++++++++++++++++
 3 files changed, 115 insertions(+), 8 deletions(-)

Range-diff against v2:
1:  f35d78cfb4 = 1:  f35d78cfb4 worktree add: Include -B in usage docs
2:  653be67e8a ! 2:  c040c87c6d worktree add: add --orphan flag
    @@ Commit message
         worktree add: add --orphan flag

         Adds support for creating an orphan branch when adding a new worktree.
    -    This functionality is equivalent to git checkout's --orphan flag.
    +    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
    @@ Documentation/git-worktree.txt: exist, a new branch based on `HEAD` is automatic
      command will refuse to create the worktree (unless `--force` is used).
     ++
     +------------
    -+$ git worktree add --orphan <branch> <path> [<commit-ish>]
    ++$ git worktree add --orphan <branch> <path>
     +------------
     ++
    -+Create a worktree containing an orphan branch named `<branch>` based
    -+on `<commit-ish>`. If `<commit-ish>` is not specified, the new orphan branch
    -+will be created based on `HEAD`.
    -++
    -+Note that unlike with `-b` or `-B`, this operation will succeed even if
    -+`<commit-ish>` is a branch that is currently checked out somewhere else.
    ++Create a worktree containing an orphan branch named `<branch>` with a
    ++clean working directory.  See `--orphan` in linkgit:git-switch[1] for
    ++more details.

      list::

    @@ Documentation/git-worktree.txt: This can also be set up as the default behaviour

     +--orphan <new-branch>::
     +	With `add`, create a new orphan branch named `<new-branch>` in the new
    -+	worktree based on `<commit-ish>`. If `<commit-ish>` is omitted, it
    -+	defaults to `HEAD`.
    ++	worktree. See `--orphan` in linkgit:git-switch[1] for details.
     +
      --porcelain::
      	With `list`, output in an easy-to-parse format for scripts.
    @@ builtin/worktree.c: struct add_opts {
      	int detach;
      	int quiet;
      	int checkout;
    -+	int implicit;
     +	const char *orphan_branch;
      	const char *keep_locked;
      };
    @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
      	}
      	commit = lookup_commit_reference_by_name(refname);
     -	if (!commit)
    -+	if (!commit && !opts->implicit)
    ++	if (!commit && !opts->orphan_branch)
      		die(_("invalid reference: %s"), refname);

      	name = worktree_basename(path, &len);
    @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
      	if (ret)
      		goto done;

    --	if (opts->checkout &&
    --	    (ret = checkout_worktree(opts, &child_env)))
    --		goto done;
    -+	if (opts->checkout) {
    -+		ret = checkout_worktree(opts, &child_env);
    -+		if (opts->orphan_branch && !ret)
    -+			ret = make_worktree_orphan(opts, &child_env);
    -+		if (ret)
    -+			goto done;
    -+	}
    -
    - 	is_junk = 0;
    - 	FREE_AND_NULL(junk_work_tree);
    ++	if (opts->orphan_branch &&
    ++	    (ret = make_worktree_orphan(opts, &child_env)))
    ++		goto done;
    ++
    + 	if (opts->checkout &&
    + 	    (ret = checkout_worktree(opts, &child_env)))
    + 		goto done;
     @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refname,
      	 * Hook failure does not warrant worktree deletion, so run hook after
      	 * is_junk is cleared, but do return appropriate code when hook fails.
    @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
      		OPT_BOOL(0, "checkout", &opts.checkout, N_("populate the new working tree")),
      		OPT_BOOL(0, "lock", &keep_locked, N_("keep the new working tree locked")),
     @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
    - 	memset(&opts, 0, sizeof(opts));
    - 	opts.checkout = 1;
      	ac = parse_options(ac, av, prefix, options, git_worktree_add_usage, 0);
    --	if (!!opts.detach + !!new_branch + !!new_branch_force > 1)
    --		die(_("options '%s', '%s', and '%s' cannot be used together"), "-b", "-B", "--detach");
    -+	opts.implicit = ac < 2;
    -+
    + 	if (!!opts.detach + !!new_branch + !!new_branch_force > 1)
    + 		die(_("options '%s', '%s', and '%s' cannot be used together"), "-b", "-B", "--detach");
     +	if (!!opts.detach + !!new_branch + !!new_branch_force + !!opts.orphan_branch > 1)
     +		die(_("options '%s', '%s', '%s', and '%s' cannot be used together"),
     +		    "-b", "-B", "--orphan", "--detach");
    @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
      		die(_("the option '%s' requires '%s'"), "--reason", "--lock");
      	if (lock_reason)
     @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
    - 		usage_with_options(git_worktree_add_usage, options);
    -
    - 	path = prefix_filename(prefix, av[0]);
    --	branch = ac < 2 ? "HEAD" : av[1];
    -+	branch = opts.implicit ? "HEAD" : av[1];
    -
      	if (!strcmp(branch, "-"))
      		branch = "@{-1}";

    @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix)
     +	 * are staged, opts.orphan_branch should be treated as both a boolean
     +	 * indicating that `--orphan` was selected and as the name of the new
     +	 * orphan branch from this point on.
    -+	 *
    -+	 * When creating a new orphan, force checkout regardless of whether
    -+	 * the existing branch is already checked out.
     +	 */
     +	if (opts.orphan_branch) {
     +		new_branch = opts.orphan_branch;
    -+		opts.force = 1;
     +	}
     +
     +	if (ac < 2 && !new_branch && !opts.detach && !opts.orphan_branch) {
    @@ t/t2400-worktree-add.sh: test_expect_success '"add" -B/--detach mutually exclusi
      '

     +test_expect_success '"add" --orphan/-b mutually exclusive' '
    -+	test_must_fail git worktree add --orphan poodle -b poodle bamboo main
    ++	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 main
    ++	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 main
    ++	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 main
    ++	test_must_fail git worktree add --orphan poodle --no-checkout bamboo
     +'
     +
     +test_expect_success '"add" -B/--detach mutually exclusive' '
    @@ t/t2400-worktree-add.sh: test_expect_success 'add --quiet' '

     +test_expect_success '"add --orphan"' '
     +	test_when_finished "git worktree remove -f -f orphandir" &&
    -+	git worktree add --orphan neworphan orphandir main &&
    ++	git worktree add --orphan neworphan orphandir &&
     +	echo refs/heads/neworphan >expected &&
     +	git -C orphandir symbolic-ref HEAD >actual &&
    -+	test_cmp expected actual &&
    -+	git -C orphandir diff main
    ++	test_cmp expected actual
     +'
     +
     +test_expect_success '"add --orphan" fails if the branch already exists' '
    ++	test_when_finished "git branch -D existingbranch" &&
     +	test_when_finished "git worktree remove -f -f orphandir" &&
     +	git worktree add -b existingbranch orphandir main &&
    -+	test_must_fail git worktree add --orphan existingbranch orphandir2 main &&
    ++	test_must_fail git worktree add --orphan existingbranch orphandir2 &&
     +	test ! -d orphandir2
     +'
     +
    -+test_expect_success '"add --orphan" fails if the commit-ish doesnt exist' '
    -+	test_must_fail git worktree add --orphan badcommitish orphandir eee2222 &&
    -+	test ! -d orphandir
    -+'
    -+
     +test_expect_success '"add --orphan" with empty repository' '
     +	test_when_finished "rm -rf empty_repo" &&
     +	echo refs/heads/newbranch >expected &&
--
2.37.4



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-10 23:32   ` [PATCH v3 " Jacob Abel
@ 2022-11-16  0:39     ` Eric Sunshine
  2022-11-17 10:00       ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 17+ messages in thread
From: Eric Sunshine @ 2022-11-16  0:39 UTC (permalink / raw)
  To: Jacob Abel; +Cc: git, Ævar Arnfjörð Bjarmason, Taylor Blau

On Thu, Nov 10, 2022 at 6:32 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> While working with the worktree based git workflow, I realised that setting
> up a new git repository required switching between the traditional and
> worktree based workflows. Searching online I found a SO answer [1] which
> seemed to support this and which indicated that adding support for this should
> not be technically difficult.
>
>   * adding orphan branch functionality (as is present in `git-switch`)
>     to `git-worktree-add`

I haven't had a chance yet to read v3, but can we take a step back for
a moment and look at this topic from a slightly different angle?
Setting aside the value of adding --orphan to `git worktree add`
(which, I'm perfectly fine with, as mentioned earlier), I have a
question about whether the solution proposed by this series is the
best we can do.

As I understand it, the actual problem this series wants to solve is
that it's not possible to create a new worktree from an empty bare
repository; for instance:

    % 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'
    %

This series addresses that shortcoming by adding --orphan, so that the
following works:

    % git init --bare foo.git
    % git -C foo.git worktree add --orphan main bar
    Preparing worktree (new branch 'main')
    %

However, is this really the best and most user-friendly and most
discoverable solution? Is it likely that users are somehow going to
instinctively use --orphan when they see the "fatal: not a valid
object name: 'HEAD'" error message?

Wouldn't a better solution be to somehow fix `git worktree add -b
<branch>` so that it just works rather than erroring out? I haven't
delved into the implementation to determine if this is possible, but
if it is, it seems a far superior "fix" for the problem shown above
since it requires no extra effort on the user's part, and doesn't
raise any discoverability red-flags (since nothing needs to be
"discovered" if `-b <branch>` works as expected in the first place).

If fixing `-b <branch>` to "just work" is possible, then --orphan is
no longer a needed workaround but becomes "icing on the cake".

> Changes from v2:
>
>   * Changed orphan creation behavior to match `git switch --orphan` instead of
>     `git checkout --orphan` [2][3]. As a result `--orphan` no longer accepts a
>     `<commit-ish>` and creates the orphan branch with a clean working directory.

Thanks for making this change.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-16  0:39     ` Eric Sunshine
@ 2022-11-17 10:00       ` Ævar Arnfjörð Bjarmason
  2022-11-19  3:47         ` Jacob Abel
  0 siblings, 1 reply; 17+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-17 10:00 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Jacob Abel, git, Taylor Blau


On Tue, Nov 15 2022, Eric Sunshine wrote:

> On Thu, Nov 10, 2022 at 6:32 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
>> While working with the worktree based git workflow, I realised that setting
>> up a new git repository required switching between the traditional and
>> worktree based workflows. Searching online I found a SO answer [1] which
>> seemed to support this and which indicated that adding support for this should
>> not be technically difficult.
>>
>>   * adding orphan branch functionality (as is present in `git-switch`)
>>     to `git-worktree-add`
>
> I haven't had a chance yet to read v3, but can we take a step back for
> a moment and look at this topic from a slightly different angle?
> Setting aside the value of adding --orphan to `git worktree add`
> (which, I'm perfectly fine with, as mentioned earlier), I have a
> question about whether the solution proposed by this series is the
> best we can do.
>
> As I understand it, the actual problem this series wants to solve is
> that it's not possible to create a new worktree from an empty bare
> repository; for instance:
>
>     % 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'
>     %
>
> This series addresses that shortcoming by adding --orphan, so that the
> following works:
>
>     % git init --bare foo.git
>     % git -C foo.git worktree add --orphan main bar
>     Preparing worktree (new branch 'main')
>     %
>
> However, is this really the best and most user-friendly and most
> discoverable solution? Is it likely that users are somehow going to
> instinctively use --orphan when they see the "fatal: not a valid
> object name: 'HEAD'" error message?
>
> Wouldn't a better solution be to somehow fix `git worktree add -b
> <branch>` so that it just works rather than erroring out? I haven't
> delved into the implementation to determine if this is possible, but
> if it is, it seems a far superior "fix" for the problem shown above
> since it requires no extra effort on the user's part, and doesn't
> raise any discoverability red-flags (since nothing needs to be
> "discovered" if `-b <branch>` works as expected in the first place).
>
> If fixing `-b <branch>` to "just work" is possible, then --orphan is
> no longer a needed workaround but becomes "icing on the cake".

That's a really good point, and we *could* "fix" that.

But I don't see how to do it without overloading "-b" even further, in a
way that some users either might not mean, or at least would be
confusing.

E.g. one script "manually clones" a repo because it does "git init",
"git remote set-url", "git fetch" etc. Another one makes worktrees from
those fresh checkouts once set up.

If we "DWYM" here that second step will carry forward the bad state
instead of erroring early.

I haven't fully thought this throuh, so maybe it's fine, just
wondering...

...an alternate way to perhaps to do this would be to detect this
situation in add(), and emit an advise() telling the user that maybe
they want to use "--orphan" for this?





^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  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 14:45           ` Phillip Wood
  0 siblings, 2 replies; 17+ messages in thread
From: Jacob Abel @ 2022-11-19  3:47 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Eric Sunshine, git, Taylor Blau

On 22/11/17 11:00AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Nov 15 2022, Eric Sunshine wrote:
>
> > On Thu, Nov 10, 2022 at 6:32 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> >> While working with the worktree based git workflow, I realised that setting
> >> up a new git repository required switching between the traditional and
> >> worktree based workflows. Searching online I found a SO answer [1] which
> >> seemed to support this and which indicated that adding support for this should
> >> not be technically difficult.
> >>
> >>   * adding orphan branch functionality (as is present in `git-switch`)
> >>     to `git-worktree-add`
> >
> > I haven't had a chance yet to read v3, but can we take a step back for
> > a moment and look at this topic from a slightly different angle?
> > Setting aside the value of adding --orphan to `git worktree add`
> > (which, I'm perfectly fine with, as mentioned earlier), I have a
> > question about whether the solution proposed by this series is the
> > best we can do.
> >
> > As I understand it, the actual problem this series wants to solve is
> > that it's not possible to create a new worktree from an empty bare
> > repository; for instance:
> >
> >     % 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'
> >     %
> >
> > This series addresses that shortcoming by adding --orphan, so that the
> > following works:
> >
> >     % git init --bare foo.git
> >     % git -C foo.git worktree add --orphan main bar
> >     Preparing worktree (new branch 'main')
> >     %
> >
> > However, is this really the best and most user-friendly and most
> > discoverable solution? Is it likely that users are somehow going to
> > instinctively use --orphan when they see the "fatal: not a valid
> > object name: 'HEAD'" error message?
> >
> > Wouldn't a better solution be to somehow fix `git worktree add -b
> > <branch>` so that it just works rather than erroring out? I haven't
> > delved into the implementation to determine if this is possible, but
> > if it is, it seems a far superior "fix" for the problem shown above
> > since it requires no extra effort on the user's part, and doesn't
> > raise any discoverability red-flags (since nothing needs to be
> > "discovered" if `-b <branch>` works as expected in the first place).
> >
> > If fixing `-b <branch>` to "just work" is possible, then --orphan is
> > no longer a needed workaround but becomes "icing on the cake".
>
> That's a really good point, and we *could* "fix" that.
>
> But I don't see how to do it without overloading "-b" even further, in a
> way that some users either might not mean, or at least would be
> confusing.
>
> E.g. one script "manually clones" a repo because it does "git init",
> "git remote set-url", "git fetch" etc. Another one makes worktrees from
> those fresh checkouts once set up.
>
> If we "DWYM" here that second step will carry forward the bad state
> instead of erroring early.
>
> I haven't fully thought this throuh, so maybe it's fine, just
> wondering...
>
> ...an alternate way to perhaps to do this would be to detect this
> situation in add(), and emit an advise() telling the user that maybe
> they want to use "--orphan" for this?
>

Prior to writing this patch, I tried to determine if there was a succinct way
to make `-b` "just work" however I wasn't able to find one that wouldn't
introduce unintuitive behavior. My conclusion was that it was probably best
to break it out into a separate command as the other tools had.

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 init --bare foo.git
    % git -C foo.git branch --list

    % 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 init --bare foo.git
    % git -C foo.git --no-pager branch --list

    % 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'

but not in the following circumstances:

    % git init --bare foo.git
    % ...
    % git -C foo.git --no-pager branch --list
    + foo
      bar
    % git -C foo.git worktree add foobar/
    Preparing worktree (new branch 'foobar')
    HEAD is now at 319605f8f0 This is a commit message

or

    % git init --bare foo.git
    % ...
    % git -C foo.git --no-pager branch --list
    + foo
      bar
    % git -C foo.git worktree add -b foobar foobardir/
    Preparing worktree (new branch 'foobar)
    HEAD is now at 319605f8f0 This is a commit message

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.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  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 14:45           ` Phillip Wood
  1 sibling, 1 reply; 17+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-19 11:48 UTC (permalink / raw)
  To: Jacob Abel; +Cc: Eric Sunshine, git, Taylor Blau


On Sat, Nov 19 2022, Jacob Abel wrote:

> On 22/11/17 11:00AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Tue, Nov 15 2022, Eric Sunshine wrote:
>>
>> > On Thu, Nov 10, 2022 at 6:32 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
>> >> While working with the worktree based git workflow, I realised that setting
>> >> up a new git repository required switching between the traditional and
>> >> worktree based workflows. Searching online I found a SO answer [1] which
>> >> seemed to support this and which indicated that adding support for this should
>> >> not be technically difficult.
>> >>
>> >>   * adding orphan branch functionality (as is present in `git-switch`)
>> >>     to `git-worktree-add`
>> >
>> > I haven't had a chance yet to read v3, but can we take a step back for
>> > a moment and look at this topic from a slightly different angle?
>> > Setting aside the value of adding --orphan to `git worktree add`
>> > (which, I'm perfectly fine with, as mentioned earlier), I have a
>> > question about whether the solution proposed by this series is the
>> > best we can do.
>> >
>> > As I understand it, the actual problem this series wants to solve is
>> > that it's not possible to create a new worktree from an empty bare
>> > repository; for instance:
>> >
>> >     % 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'
>> >     %
>> >
>> > This series addresses that shortcoming by adding --orphan, so that the
>> > following works:
>> >
>> >     % git init --bare foo.git
>> >     % git -C foo.git worktree add --orphan main bar
>> >     Preparing worktree (new branch 'main')
>> >     %
>> >
>> > However, is this really the best and most user-friendly and most
>> > discoverable solution? Is it likely that users are somehow going to
>> > instinctively use --orphan when they see the "fatal: not a valid
>> > object name: 'HEAD'" error message?
>> >
>> > Wouldn't a better solution be to somehow fix `git worktree add -b
>> > <branch>` so that it just works rather than erroring out? I haven't
>> > delved into the implementation to determine if this is possible, but
>> > if it is, it seems a far superior "fix" for the problem shown above
>> > since it requires no extra effort on the user's part, and doesn't
>> > raise any discoverability red-flags (since nothing needs to be
>> > "discovered" if `-b <branch>` works as expected in the first place).
>> >
>> > If fixing `-b <branch>` to "just work" is possible, then --orphan is
>> > no longer a needed workaround but becomes "icing on the cake".
>>
>> That's a really good point, and we *could* "fix" that.
>>
>> But I don't see how to do it without overloading "-b" even further, in a
>> way that some users either might not mean, or at least would be
>> confusing.
>>
>> E.g. one script "manually clones" a repo because it does "git init",
>> "git remote set-url", "git fetch" etc. Another one makes worktrees from
>> those fresh checkouts once set up.
>>
>> If we "DWYM" here that second step will carry forward the bad state
>> instead of erroring early.
>>
>> I haven't fully thought this throuh, so maybe it's fine, just
>> wondering...
>>
>> ...an alternate way to perhaps to do this would be to detect this
>> situation in add(), and emit an advise() telling the user that maybe
>> they want to use "--orphan" for this?
>>
>
> Prior to writing this patch, I tried to determine if there was a succinct way
> to make `-b` "just work" however I wasn't able to find one that wouldn't
> introduce unintuitive behavior. My conclusion was that it was probably best
> to break it out into a separate command as the other tools had.
>
> 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 init --bare foo.git
>     % git -C foo.git branch --list
>
>     % 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 init --bare foo.git
>     % git -C foo.git --no-pager branch --list
>
>     % 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.

> but not in the following circumstances:
>
>     % git init --bare foo.git
>     % ...
>     % git -C foo.git --no-pager branch --list
>     + foo
>       bar
>     % git -C foo.git worktree add foobar/
>     Preparing worktree (new branch 'foobar')
>     HEAD is now at 319605f8f0 This is a commit message
>
> or
>
>     % git init --bare foo.git
>     % ...
>     % git -C foo.git --no-pager branch --list
>     + foo
>       bar
>     % git -C foo.git worktree add -b foobar foobardir/
>     Preparing worktree (new branch 'foobar)
>     HEAD is now at 319605f8f0 This is a commit message

*nod*

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-19 11:48           ` Ævar Arnfjörð Bjarmason
@ 2022-11-22  5:16             ` Eric Sunshine
  2022-11-22 23:26               ` Jacob Abel
  0 siblings, 1 reply; 17+ messages in thread
From: Eric Sunshine @ 2022-11-22  5:16 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Jacob Abel, git, Taylor Blau

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.

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-19  3:47         ` Jacob Abel
  2022-11-19 11:48           ` Ævar Arnfjörð Bjarmason
@ 2022-11-22 14:45           ` Phillip Wood
  2022-11-23  4:21             ` Jacob Abel
  1 sibling, 1 reply; 17+ messages in thread
From: Phillip Wood @ 2022-11-22 14:45 UTC (permalink / raw)
  To: Jacob Abel, Ævar Arnfjörð Bjarmason
  Cc: Eric Sunshine, git, Taylor Blau

On 19/11/2022 03:47, Jacob Abel wrote:
> On 22/11/17 11:00AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Tue, Nov 15 2022, Eric Sunshine wrote:
>>
>>> On Thu, Nov 10, 2022 at 6:32 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
>>>> While working with the worktree based git workflow, I realised that setting
>>>> up a new git repository required switching between the traditional and
>>>> worktree based workflows. Searching online I found a SO answer [1] which
>>>> seemed to support this and which indicated that adding support for this should
>>>> not be technically difficult.
>>>>
>>>>    * adding orphan branch functionality (as is present in `git-switch`)
>>>>      to `git-worktree-add`
>>>
>>> I haven't had a chance yet to read v3, but can we take a step back for
>>> a moment and look at this topic from a slightly different angle?
>>> Setting aside the value of adding --orphan to `git worktree add`
>>> (which, I'm perfectly fine with, as mentioned earlier), I have a
>>> question about whether the solution proposed by this series is the
>>> best we can do.
>>>
>>> As I understand it, the actual problem this series wants to solve is
>>> that it's not possible to create a new worktree from an empty bare
>>> repository; for instance:
>>>
>>>      % 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'
>>>      %
>>>
>>> This series addresses that shortcoming by adding --orphan, so that the
>>> following works:
>>>
>>>      % git init --bare foo.git
>>>      % git -C foo.git worktree add --orphan main bar
>>>      Preparing worktree (new branch 'main')
>>>      %
>>>
>>> However, is this really the best and most user-friendly and most
>>> discoverable solution? Is it likely that users are somehow going to
>>> instinctively use --orphan when they see the "fatal: not a valid
>>> object name: 'HEAD'" error message?
>>>
>>> Wouldn't a better solution be to somehow fix `git worktree add -b
>>> <branch>` so that it just works rather than erroring out? I haven't
>>> delved into the implementation to determine if this is possible, but
>>> if it is, it seems a far superior "fix" for the problem shown above
>>> since it requires no extra effort on the user's part, and doesn't
>>> raise any discoverability red-flags (since nothing needs to be
>>> "discovered" if `-b <branch>` works as expected in the first place).
>>>
>>> If fixing `-b <branch>` to "just work" is possible, then --orphan is
>>> no longer a needed workaround but becomes "icing on the cake".
>>
>> That's a really good point, and we *could* "fix" that.
>>
>> But I don't see how to do it without overloading "-b" even further, in a
>> way that some users either might not mean, or at least would be
>> confusing.
>>
>> E.g. one script "manually clones" a repo because it does "git init",
>> "git remote set-url", "git fetch" etc. Another one makes worktrees from
>> those fresh checkouts once set up.
>>
>> If we "DWYM" here that second step will carry forward the bad state
>> instead of erroring early.

Wouldn't the first script error out if there was a problem?

>> I haven't fully thought this throuh, so maybe it's fine, just
>> wondering...
>>
>> ...an alternate way to perhaps to do this would be to detect this
>> situation in add(), and emit an advise() telling the user that maybe
>> they want to use "--orphan" for this?
>>
> 
> Prior to writing this patch, I tried to determine if there was a succinct way
> to make `-b` "just work" however I wasn't able to find one that wouldn't
> introduce unintuitive behavior.

Can you say a bit more about what the unintuitive behavior was? As I 
understand it the problem is that "git branch" errors out when HEAD is a 
symbolic ref pointing to a ref that does not exist. I think we can use 
read_ref() to check for that before running "git branch" and act 
accordingly. We might want to check if HEAD matches init.defaultBranch 
and only do an orphan checkout in the new worktree in that case.

> My conclusion was that it was probably best
> to break it out into a separate command as the other tools had.
> 
> 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 init --bare foo.git
>      % git -C foo.git branch --list
> 
>      % 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 init --bare foo.git
>      % git -C foo.git --no-pager branch --list
> 
>      % 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'
> 
> but not in the following circumstances:
> 
>      % git init --bare foo.git
>      % ...
>      % git -C foo.git --no-pager branch --list
>      + foo
>        bar
>      % git -C foo.git worktree add foobar/
>      Preparing worktree (new branch 'foobar')
>      HEAD is now at 319605f8f0 This is a commit message
> 
> or
> 
>      % git init --bare foo.git
>      % ...
>      % git -C foo.git --no-pager branch --list
>      + foo
>        bar
>      % git -C foo.git worktree add -b foobar foobardir/
>      Preparing worktree (new branch 'foobar)
>      HEAD is now at 319605f8f0 This is a commit message
> 
> 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 don't think it matters if the repository is bare so I think it would 
be good to advise() on

	% git init foo
	% git -C foo worktree add bar

Best Wishes

Phillip

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  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:43                 ` Rubén Justo
  0 siblings, 2 replies; 17+ messages in thread
From: Jacob Abel @ 2022-11-22 23:26 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Ævar Arnfjörð Bjarmason, git, Taylor Blau

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")) {
    +		/*
    +		 * 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");

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

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  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
  1 sibling, 1 reply; 17+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-22 23:55 UTC (permalink / raw)
  To: Jacob Abel; +Cc: Eric Sunshine, git, Taylor Blau


On Tue, Nov 22 2022, 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")) {
>     +		/*
>     +		 * 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);

We don't consistently check for this, unfortunately (but I have some
local patches for it), but to add an advice you should:

 * Add it to Documentation/config/advice.txt (in sorted order)
 * Add the corresponding enum to advice.h
 * And to the advice.c listing
 * Then use advise_if_enabled(<that new enum>, ...) in cases such as this one.
 * End your message with a suggstion about how to disable the advice:
   git grep -W -F 'git config advice.' -- '*.c'

That's rather tedious, sorry, but that's the extent of the current
boilerplate...

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-22 23:26               ` Jacob Abel
  2022-11-22 23:55                 ` Ævar Arnfjörð Bjarmason
@ 2022-11-23  2:43                 ` Rubén Justo
  2022-11-23  5:37                   ` Jacob Abel
  1 sibling, 1 reply; 17+ messages in thread
From: Rubén Justo @ 2022-11-23  2:43 UTC (permalink / raw)
  To: Jacob Abel, Eric Sunshine
  Cc: Ævar Arnfjörð Bjarmason, git, Taylor Blau

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.  This needs to be "HEAD",
in capital letters.

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

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.

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;

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-22 23:55                 ` Ævar Arnfjörð Bjarmason
@ 2022-11-23  2:47                   ` Jacob Abel
  0 siblings, 0 replies; 17+ messages in thread
From: Jacob Abel @ 2022-11-23  2:47 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Eric Sunshine, git, Taylor Blau

On 22/11/23 12:55AM, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Nov 22 2022, 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")) {
> >     +		/*
> >     +		 * 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);
>
> We don't consistently check for this, unfortunately (but I have some
> local patches for it), but to add an advice you should:
>
>  * Add it to Documentation/config/advice.txt (in sorted order)
>  * Add the corresponding enum to advice.h
>  * And to the advice.c listing
>  * Then use advise_if_enabled(<that new enum>, ...) in cases such as this one.
>  * End your message with a suggstion about how to disable the advice:
>    git grep -W -F 'git config advice.' -- '*.c'
>
> That's rather tedious, sorry, but that's the extent of the current
> boilerplate...

Noted. Will do.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-22 14:45           ` Phillip Wood
@ 2022-11-23  4:21             ` Jacob Abel
  0 siblings, 0 replies; 17+ messages in thread
From: Jacob Abel @ 2022-11-23  4:21 UTC (permalink / raw)
  To: phillip.wood
  Cc: Ævar Arnfjörð Bjarmason, Eric Sunshine, git,
	Taylor Blau

On 22/11/22 02:45PM, Phillip Wood wrote:
> On 19/11/2022 03:47, Jacob Abel wrote:
> > On 22/11/17 11:00AM, Ævar Arnfjörð Bjarmason wrote:
> >>
> >> On Tue, Nov 15 2022, Eric Sunshine wrote:
> >>
> >>> On Thu, Nov 10, 2022 at 6:32 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> >>>> While working with the worktree based git workflow, I realised that setting
> >>>> up a new git repository required switching between the traditional and
> >>>> worktree based workflows. Searching online I found a SO answer [1] which
> >>>> seemed to support this and which indicated that adding support for this should
> >>>> not be technically difficult.
> >>>>
> >>>>    * adding orphan branch functionality (as is present in `git-switch`)
> >>>>      to `git-worktree-add`
> >>>
> >>> I haven't had a chance yet to read v3, but can we take a step back for
> >>> a moment and look at this topic from a slightly different angle?
> >>> Setting aside the value of adding --orphan to `git worktree add`
> >>> (which, I'm perfectly fine with, as mentioned earlier), I have a
> >>> question about whether the solution proposed by this series is the
> >>> best we can do.
> >>>
> >>> As I understand it, the actual problem this series wants to solve is
> >>> that it's not possible to create a new worktree from an empty bare
> >>> repository; for instance:
> >>>
> >>>      % 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'
> >>>      %
> >>>
> >>> This series addresses that shortcoming by adding --orphan, so that the
> >>> following works:
> >>>
> >>>      % git init --bare foo.git
> >>>      % git -C foo.git worktree add --orphan main bar
> >>>      Preparing worktree (new branch 'main')
> >>>      %
> >>>
> >>> However, is this really the best and most user-friendly and most
> >>> discoverable solution? Is it likely that users are somehow going to
> >>> instinctively use --orphan when they see the "fatal: not a valid
> >>> object name: 'HEAD'" error message?
> >>>
> >>> Wouldn't a better solution be to somehow fix `git worktree add -b
> >>> <branch>` so that it just works rather than erroring out? I haven't
> >>> delved into the implementation to determine if this is possible, but
> >>> if it is, it seems a far superior "fix" for the problem shown above
> >>> since it requires no extra effort on the user's part, and doesn't
> >>> raise any discoverability red-flags (since nothing needs to be
> >>> "discovered" if `-b <branch>` works as expected in the first place).
> >>>
> >>> If fixing `-b <branch>` to "just work" is possible, then --orphan is
> >>> no longer a needed workaround but becomes "icing on the cake".
> >>
> >> That's a really good point, and we *could* "fix" that.
> >>
> >> But I don't see how to do it without overloading "-b" even further, in a
> >> way that some users either might not mean, or at least would be
> >> confusing.
> >>
> >> E.g. one script "manually clones" a repo because it does "git init",
> >> "git remote set-url", "git fetch" etc. Another one makes worktrees from
> >> those fresh checkouts once set up.
> >>
> >> If we "DWYM" here that second step will carry forward the bad state
> >> instead of erroring early.
>
> Wouldn't the first script error out if there was a problem?
>
> >> I haven't fully thought this throuh, so maybe it's fine, just
> >> wondering...
> >>
> >> ...an alternate way to perhaps to do this would be to detect this
> >> situation in add(), and emit an advise() telling the user that maybe
> >> they want to use "--orphan" for this?
> >>
> >
> > Prior to writing this patch, I tried to determine if there was a succinct way
> > to make `-b` "just work" however I wasn't able to find one that wouldn't
> > introduce unintuitive behavior.
>
> Can you say a bit more about what the unintuitive behavior was? As I
> understand it the problem is that "git branch" errors out when HEAD is a
> symbolic ref pointing to a ref that does not exist. I think we can use
> read_ref() to check for that before running "git branch" and act
> accordingly. We might want to check if HEAD matches init.defaultBranch
> and only do an orphan checkout in the new worktree in that case.

The main issue is that creating an orphan branch is very rarely what the user
intends to do. To modify `-b` to automatically create an orphan would require
that you set the behavior so that `-b` (and DWYM) creates a new orphan branch
only when the repository has no branches (i.e. a fresh init repo).

This has the effect that the command will perform separate operations depending
on the state of the repository and when mixed in with other commands it quickly
becomes confusing.

In the directory shown below:

    ../
    ./
    .git/

with `.git` containing a bare repository with no branches,

    % git worktree add foobar/

would now create a worktree `foobar/` with orphan branch `foobar` (which
technically speaking doesn't exist until a commit is made).

This behavior continues to apply until your first commit.

However after the following:

    % git worktree add foo/
    % cd foo/
    # create files
    % git add .
    % cd ../
    % git worktree add bar/
    % cd bar/
    # create files
    % cd ../foo/
    % git commit -m "foo commit"
    % cd ../bar/
    % git add .
    % git commit -m "bar commit"
    % cd ../foobar/
    # create files
    % git add .
    % git commit -m "foobar commit"

In that same directory:

    ../
    ./
    .git/
    foobar/ <- on branch foobar @ "foobar commit"
    foo/    <- on branch foo    @ "foo commit"
    bar/    <- on branch bar    @ "bar commit"

that same command now creates a branch which is based on whichever reference
HEAD happens to now refer to. In the case of a directory which is not a working
tree, it's not always clear to me what HEAD should actually point to. So now
what should the following do:

    % git worktree add what_am_i/

The user has just created 3 worktrees containing orphan branches. Wouldn't the
user now reasonably expect that from this directory with no working tree that
the above command would also create an orphan branch?

And when it doesn't, which branch is the history based off of? Which one will
the user expect?

- worktree foobar/ which was created first but with the most recent initial commit?
- worktree foo/ where the user has been working the longest?
- worktree bar/ which was created last but which had the earliest initial commit?

This isn't necessarily due to this change in particular but rather that this
change would expose users to an edge case where they can run into really
unintuitive behavior.

Of course this is a bit of an unusual use example but I'd rather warn & direct
the user when they need to do something slightly different/unusual to handle an
edge case rather than risk users getting weird behavior that leaves them turning
to Stack Overflow when they encounter an edge case.

>
> > My conclusion was that it was probably best
> > to break it out into a separate command as the other tools had.
> >
> > 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 init --bare foo.git
> >      % git -C foo.git branch --list
> >
> >      % 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 init --bare foo.git
> >      % git -C foo.git --no-pager branch --list
> >
> >      % 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'
> >
> > but not in the following circumstances:
> >
> >      % git init --bare foo.git
> >      % ...
> >      % git -C foo.git --no-pager branch --list
> >      + foo
> >        bar
> >      % git -C foo.git worktree add foobar/
> >      Preparing worktree (new branch 'foobar')
> >      HEAD is now at 319605f8f0 This is a commit message
> >
> > or
> >
> >      % git init --bare foo.git
> >      % ...
> >      % git -C foo.git --no-pager branch --list
> >      + foo
> >        bar
> >      % git -C foo.git worktree add -b foobar foobardir/
> >      Preparing worktree (new branch 'foobar)
> >      HEAD is now at 319605f8f0 This is a commit message
> >
> > 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 don't think it matters if the repository is bare so I think it would
> be good to advise() on
>
> 	% git init foo
> 	% git -C foo worktree add bar
>
> Best Wishes
>
> Phillip

Correct. It shouldn't matter between bare and non-bare. I tend to prefer bare repos
when working with worktrees which is why I wrote it that way but I'm definitely
intending that the advise() works the same for both bare and non-bare.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-23  2:43                 ` Rubén Justo
@ 2022-11-23  5:37                   ` Jacob Abel
  2022-11-23  7:35                     ` Rubén Justo
  0 siblings, 1 reply; 17+ messages in thread
From: Jacob Abel @ 2022-11-23  5:37 UTC (permalink / raw)
  To: Rubén Justo
  Cc: Eric Sunshine, Ævar Arnfjörð Bjarmason, git,
	Taylor Blau

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/


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-11-23  5:37                   ` Jacob Abel
@ 2022-11-23  7:35                     ` Rubén Justo
  0 siblings, 0 replies; 17+ messages in thread
From: Rubén Justo @ 2022-11-23  7:35 UTC (permalink / raw)
  To: Jacob Abel
  Cc: Eric Sunshine, Ævar Arnfjörð Bjarmason, git,
	Taylor Blau

On 23-nov-2022 05:37:21, Jacob Abel wrote:
> 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.
> 

Understood.  Maybe allowing a mixed DWYM...

	$ git worktree add --orphan foobar

I'll wait for your next version.

Thank you for the wrap up, and sorry again for reading the thread bottom
up.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
@ 2022-12-08 21:41 Jacob Abel
  2022-12-08 22:00 ` rsbecker
  0 siblings, 1 reply; 17+ messages in thread
From: Jacob Abel @ 2022-12-08 21:41 UTC (permalink / raw)
  To: jacobabel
  Cc: Ævar Arnfjörð Bjarmason, Eric Sunshine,
	Phillip Wood, Rubén Justo, Taylor Blau, git

Apologies for the delay on the v4 patch. Some unexpected personal stuff has kept
me away from working on this. I expect v4 to be out in the next few days.

Additionally, I've been doing some reading after writing this reply [1]. I've
realised I had a misunderstanding about how HEAD is managed. I don't think it
changes the conclusion of the reply (which is that rolling `--orphan` into DWYM
could lead to confusing behavior for users) however I think some additional
changes might be warranted:

* Add some additional text output to `worktree add` when we DWYM to make it
  clearer what action we end up making. Could possibly be hidden under a
  "verbose" flag.
* Annotate what HEAD we are looking at (worktree HEAD vs git repo HEAD) in
  conditions where this could matter.
* Expanding HEAD when it's an invalid ref (instead of just `invalid ref: HEAD`).
* Add a hint when using `worktree add`, with a bare git repo, HEAD points to an
  invalid ref, not in a worktree, and other branches exist in the repo. The hint
  would suggest using `git branch -m <branch>` to change the HEAD to an existing
  branch.
* Add a hint when there are no local branches and the user is trying to create a
  worktree off a local branch which has a remote counterpart.

I don't necessarily think any of these changes should hold up this patchset but
I think they could be worthwhile changes for the future.

And in the meantime, below are the anticipated changes for the next revision. Let
me know if it looks like I've forgotten anything.

Anticipated changes from v3:
  * Fix mistake in SYNOPSIS and `--help`. Patch for this change can be found
    in [2], by courtesy of Ævar Arnfjörð Bjarmason.
  * Collapsed sequential if statements into if-else chain & simplified
    conditions as requested in [2].
  * Simplified tests for mutually exclusive options and removed duplicate
    `-B/--detach` test case. Patch for this change can be found in [3],
    by courtesy of Ævar Arnfjörð Bjarmason.
  * Remove references to `git-switch`. Behavior is now explained fully in docs
    instead. Changes to the docs suggested in [4], by courtesy of Eric Sunshine.
  * Updated test case to use `test_path_is_missing` instead of `! test -d` [5].
  * Updated signature for `make_worktree_orphan()` and changed
    `const char *orphan_branch` in `struct add_opts` to `int orphan` (boolean)
    to simplify the patch and maintain consistency with other flags [6].
  * Added `advise()` to common cases where `--orphan` is desired [7] to address
    concerns brought up in [8][9]. Slight change from `HEAD` to `branch` as
    `HEAD` causes existing behavior to break.
  * Added tests to verify `--lock` and `--reason` work properly with
    the newly added `--orphan` flag.
  * Added tests to verify that the orphan advise [7] is emitted only at the
    proper times.
  * Added the new advice to the docs, advice.c/h, and switched to use
    `advise_if_enabled()` as requested in [10].
  * Added tests to verify the changes [7] do not interfere with existing
    behavior. ex: creating a worktree from a remote branch when HEAD is
    an orphan.

1. https://lore.kernel.org/git/20221123042052.t42jmsqjxgx2k3th@phi/
2. https://lore.kernel.org/git/221115.86edu3kfqz.gmgdl@evledraar.gmail.com/
3. https://lore.kernel.org/git/221116.86a64rkdcj.gmgdl@evledraar.gmail.com/
4. https://lore.kernel.org/git/CAPig+cQiyd9yGE_XpPZmzLQnNDMypnrEgkV7nqRZZn3j6E0APQ@mail.gmail.com/
5. https://lore.kernel.org/git/221115.86iljfkjjo.gmgdl@evledraar.gmail.com/
6. https://lore.kernel.org/git/20221119025701.syuscuoqjuqhqsoz@phi/
7. https://lore.kernel.org/git/20221119034728.m4kxh4tdpof7us7j@phi/
8. https://lore.kernel.org/git/CAPig+cTTn764ObHJuw8epOtBdTUwocVRV=tLieCa4zf-PGCegw@mail.gmail.com/
9. https://lore.kernel.org/git/221117.86sfihj3mw.gmgdl@evledraar.gmail.com/
10. https://lore.kernel.org/git/221123.86a64ia6bx.gmgdl@evledraar.gmail.com/


^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-12-08 21:41 [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Jacob Abel
@ 2022-12-08 22:00 ` rsbecker
  2022-12-12  0:38   ` 'Jacob Abel'
  0 siblings, 1 reply; 17+ messages in thread
From: rsbecker @ 2022-12-08 22:00 UTC (permalink / raw)
  To: 'Jacob Abel'
  Cc: 'Ævar Arnfjörð Bjarmason',
	'Eric Sunshine', 'Phillip Wood',
	'Rubén Justo', 'Taylor Blau', git

On December 8, 2022 4:41 PM, Jacob Abel wrote:
>Subject: Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new
>worktrees
>
>Apologies for the delay on the v4 patch. Some unexpected personal stuff has kept
>me away from working on this. I expect v4 to be out in the next few days.
>
>Additionally, I've been doing some reading after writing this reply [1]. I've realised
>I had a misunderstanding about how HEAD is managed. I don't think it changes the
>conclusion of the reply (which is that rolling `--orphan` into DWYM could lead to
>confusing behavior for users) however I think some additional changes might be
>warranted:
>
>* Add some additional text output to `worktree add` when we DWYM to make it
>  clearer what action we end up making. Could possibly be hidden under a
>  "verbose" flag.
>* Annotate what HEAD we are looking at (worktree HEAD vs git repo HEAD) in
>  conditions where this could matter.
>* Expanding HEAD when it's an invalid ref (instead of just `invalid ref: HEAD`).
>* Add a hint when using `worktree add`, with a bare git repo, HEAD points to an
>  invalid ref, not in a worktree, and other branches exist in the repo. The hint
>  would suggest using `git branch -m <branch>` to change the HEAD to an existing
>  branch.
>* Add a hint when there are no local branches and the user is trying to create a
>  worktree off a local branch which has a remote counterpart.
>
>I don't necessarily think any of these changes should hold up this patchset but I
>think they could be worthwhile changes for the future.
>
>And in the meantime, below are the anticipated changes for the next revision. Let
>me know if it looks like I've forgotten anything.
>
>Anticipated changes from v3:
>  * Fix mistake in SYNOPSIS and `--help`. Patch for this change can be found
>    in [2], by courtesy of Ævar Arnfjörð Bjarmason.
>  * Collapsed sequential if statements into if-else chain & simplified
>    conditions as requested in [2].
>  * Simplified tests for mutually exclusive options and removed duplicate
>    `-B/--detach` test case. Patch for this change can be found in [3],
>    by courtesy of Ævar Arnfjörð Bjarmason.
>  * Remove references to `git-switch`. Behavior is now explained fully in docs
>    instead. Changes to the docs suggested in [4], by courtesy of Eric Sunshine.
>  * Updated test case to use `test_path_is_missing` instead of `! test -d` [5].
>  * Updated signature for `make_worktree_orphan()` and changed
>    `const char *orphan_branch` in `struct add_opts` to `int orphan` (boolean)
>    to simplify the patch and maintain consistency with other flags [6].
>  * Added `advise()` to common cases where `--orphan` is desired [7] to address
>    concerns brought up in [8][9]. Slight change from `HEAD` to `branch` as
>    `HEAD` causes existing behavior to break.
>  * Added tests to verify `--lock` and `--reason` work properly with
>    the newly added `--orphan` flag.
>  * Added tests to verify that the orphan advise [7] is emitted only at the
>    proper times.
>  * Added the new advice to the docs, advice.c/h, and switched to use
>    `advise_if_enabled()` as requested in [10].
>  * Added tests to verify the changes [7] do not interfere with existing
>    behavior. ex: creating a worktree from a remote branch when HEAD is
>    an orphan.
>
>1. https://lore.kernel.org/git/20221123042052.t42jmsqjxgx2k3th@phi/
>2. https://lore.kernel.org/git/221115.86edu3kfqz.gmgdl@evledraar.gmail.com/
>3. https://lore.kernel.org/git/221116.86a64rkdcj.gmgdl@evledraar.gmail.com/
>4.
>https://lore.kernel.org/git/CAPig+cQiyd9yGE_XpPZmzLQnNDMypnrEgkV7nqRZZn
>3j6E0APQ@mail.gmail.com/
>5. https://lore.kernel.org/git/221115.86iljfkjjo.gmgdl@evledraar.gmail.com/
>6. https://lore.kernel.org/git/20221119025701.syuscuoqjuqhqsoz@phi/
>7. https://lore.kernel.org/git/20221119034728.m4kxh4tdpof7us7j@phi/
>8.
>https://lore.kernel.org/git/CAPig+cTTn764ObHJuw8epOtBdTUwocVRV=tLieCa4zf-
>PGCegw@mail.gmail.com/
>9. https://lore.kernel.org/git/221117.86sfihj3mw.gmgdl@evledraar.gmail.com/
>10. https://lore.kernel.org/git/221123.86a64ia6bx.gmgdl@evledraar.gmail.com/

I am not sure this is entirely related, but there is a gap in worktree configuration, specifically includeIf that I was working on, but suspended waiting for worktree code to stabilize. Do you have a sense as to whether this might run into this topic?

Regards,
Randall


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees
  2022-12-08 22:00 ` rsbecker
@ 2022-12-12  0:38   ` 'Jacob Abel'
  0 siblings, 0 replies; 17+ messages in thread
From: 'Jacob Abel' @ 2022-12-12  0:38 UTC (permalink / raw)
  To: rsbecker
  Cc: 'Ævar Arnfjörð Bjarmason',
	'Eric Sunshine', 'Phillip Wood',
	'Rubén Justo', 'Taylor Blau', git

On 22/12/08 05:00PM, rsbecker@nexbridge.com wrote:
>
> I am not sure this is entirely related, but there is a gap in worktree
> configuration, specifically includeIf that I was working on, but suspended
> waiting for worktree code to stabilize. Do you have a sense as to whether this
> might run into this topic?
>
> Regards,
> Randall

I may be mistaken as I'm still pretty new with respect to contributing to git
but I don't believe the two should conflict.


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2022-12-12  0:38 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-08 21:41 [PATCH v3 0/2] worktree: Support `--orphan` when creating new worktrees Jacob Abel
2022-12-08 22:00 ` rsbecker
2022-12-12  0:38   ` 'Jacob Abel'
  -- strict thread matches above, loose matches on Subject: below --
2022-11-04  1:02 [PATCH 0/4] " Jacob Abel
2022-11-04 21:34 ` [PATCH v2 0/2] " Jacob Abel
2022-11-10 23:32   ` [PATCH v3 " Jacob Abel
2022-11-16  0:39     ` 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
2022-11-23  7:35                     ` Rubén Justo
2022-11-22 14:45           ` Phillip Wood
2022-11-23  4:21             ` Jacob Abel

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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