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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 4, 2022, 05:03 UTC
Message-ID: <CAPig+cSVzewXpk+eDSC-W-+Q8X_7ikZXXeSQbmpHBcdLCU5svw@mail.gmail.com>
In-Reply-To: <20221104010242.11555-4-jacobabel@nullpo.dev>

On Thu, Nov 3, 2022 at 9:07 PM Jacob Abel <jacobabel@nullpo.dev> wrote:

Show 17 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

> +       /*
> +        * 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).