Re: [PATCH 3/4] worktree add: add --orphan flag

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Nov 3, 2022 at 9:07 PM Jacob Abel <jacobabel@nullpo.dev> wrote:
> Adds support for creating an orphan branch when adding a new worktree.
> This functionality is equivalent to git checkout's --orphan flag.
> [...]
> Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -95,6 +95,17 @@ exist, a new branch based on `HEAD` is automatically created as if
> +------------
> +$ git worktree add --orphan <branch> <path> [<commit-ish>]
> +------------
> ++
> +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.

Are we sure we want to be modeling this after `git checkout --orphan`?
If I understand correctly, that option has long been considered (by
some) too clunky, which is why `git switch --orphan` was simplified to
accept only a branch name but no commit-ish, and to start the orphan
branch with an empty directory. My own feeling is that modeling it
after `git switch --orphan` is probably the way to go...

> @@ -222,6 +233,11 @@ This can also be set up as the default behaviour by using the
> +--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`.

...which would mean that this would no longer talk about `<commit-ish>`.

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -608,33 +613,52 @@ static int add(int ac, const char **av, const char *prefix)
>         struct option options[] = {
> +               OPT_STRING(0, "orphan", &opts.orphan_branch, N_("branch"),
> +                          N_("create a new unparented branch")),

The short help message for `git switch --orphan` and `git checkout
--orphan` say simply "new unparented branch", so this message should
probably follow suit (or consistency and to ease the job of
translators).

> -       if (!!opts.detach + !!new_branch + !!new_branch_force > 1)
> -               die(_("options '%s', '%s', and '%s' cannot be used together"), "-b", "-B", "--detach");
> +               die(_("options '%s', '%s', '%s', and '%s' cannot be used together"),
> +                   "-b", "-B", "--orphan", "--detach");

Good to see this interlock updated for --orphan.

> +       if (opts.orphan_branch && opt_track)
> +               die(_("'%s' cannot be used with '%s'"), "--orphan", "--track");
> +       if (opts.orphan_branch && !opts.checkout)
> +               die(_("'%s' cannot be used with '%s'"), "--orphan",
> +                   "--no-checkout");

Good to have these additional interlocks. I think, however, for the
sake of translators, we should use the same terminology as the
existing message above (i.e. "options ... cannot be used together").

> +       /*
> +        * From here on, new_branch will contain the branch to be checked out,
> +        * and new_branch_force and opts.orphan_branch will tell us which one of
> +        * -b/-B/--orphan is being used.
> +        */

This can probably be worded a bit differently to make it clear that
from this point onward, those other variables are interpreted as if
they are booleans. Moreover, we can make this even clearer by
following the example of -B in which (by necessity due to
parse-options) the local variable in add() is a `const char *`, but
its counterpart in `struct add_opts` is a boolean (int).