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