Re: [PATCH v8 2/4] worktree: improve message when creating a new worktree

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Apr 23, 2018 at 3:38 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> Currently 'git worktree add' produces output like the following:
>
>     Preparing ../foo (identifier foo)
>     HEAD is now at 26da330922 <title>
>
> The '../foo' is the path where the worktree is created, which the user
> has just given on the command line.  The identifier is an internal
> implementation detail, which is not particularly relevant for the user
> and indeed isn't mentioned explicitly anywhere in the man page.
>
> Instead of this message, print a message that gives the user a bit more
> detail of what exactly 'git worktree' is doing.  There are various dwim
> modes which perform some magic under the hood, which should be
> helpful to users.  Just from the output of the command it is not always
> visible to users what exactly has happened.
>
> Help the users a bit more by modifying the "Preparing ..." message and
> adding some additional information of what 'git worktree add' did under
> the hood, while not displaying the identifier anymore.
>
> Currently this ends up in three different cases:

You now enumerate four cases, not three. Perhaps: "There are several
different cases:"

>   - 'git worktree add -b ...' or 'git worktree add <path>', both of
>     which create a new branch, either through the user explicitly
>     requesting it, or through 'git worktree add' implicitly creating
>     it.  This will end up with the following output:
>
>       Preparing worktree (new branch '<branch>')
>       HEAD is now at 26da330922 <title>
>
>   - 'git worktree add -B ...', which may either create a new branch if
>     the branch with the given name does not exist yet, or resets an
>     existing branch to the current HEAD, or the commit-ish given.
>     Depending on which action is taken, we'll end up with the following
>     output:
>
>       Preparing worktree (resetting branch 'next'; was at caa68db14)
>       HEAD is now at 26da330922 <title>

For consistency with the other examples: s/next/<branch>/

>     or:
>
>       Preparing worktree (new branch '<branch>')
>       HEAD is now at 26da330922 <title>
>
>   - 'git worktree add --detach' or 'git worktree add <path>
>     <commit-ish>', both of which create a new worktree with a detached
>     HEAD, for which we will print the following output:
>
>       Preparing worktree (detached HEAD 26da330922)
>       HEAD is now at 26da330922 <title>
>
>   - 'git worktree add <path> <local-branch>', which checks out the
>     branch prints the following output:

s/prints/and prints/

>       Preparing worktree (checking out '<branch>')
>       HEAD is now at 47007d5 <title>

s/<branch>/<local-branch>/ would probably be clearer since you use
<local-branch> in the example command.

> [...]
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -355,6 +353,40 @@ static int add_worktree(const char *path, const char *refname,
> +static void print_preparing_worktree_line(int detach,
> +                                         const char *branch,
> +                                         const char *new_branch,
> +                                         const char *new_branch_force)
> +{
> +       if (new_branch_force) {
> +               struct commit *commit = lookup_commit_reference_by_name(new_branch_force);

Nit: By the time this function is called, 'new_branch' contains the
same value as 'new_branch_force' if "-B" was used. 'new_branch' _is_
the name of the new branch, forced or not, and it's easier to reason
about 'new_branch_force' as a mere boolean indicating whether "force"
is in effect. This suggests that the final argument to this function
should be a boolean (int) named either 'force' or 'force_new_branch',
and that 'new_branch' should be used everywhere the code needs to
reference the new branch name (forced or not).

Not worth a re-roll.

> +               if (!commit)
> +                       printf_ln(_("Preparing worktree (new branch '%s')"), new_branch_force);
> +               else
> +                       printf_ln(_("Preparing worktree (resetting branch '%s'; was at %s)"),
> +                                 new_branch_force,
> +                                 find_unique_abbrev(commit->object.oid.hash,
> +                                                    DEFAULT_ABBREV));
> +       } else if (new_branch) {
> +               printf_ln(_("Preparing worktree (new branch '%s')"), new_branch);
> +       } else {
> +               struct strbuf s = STRBUF_INIT;
> +               if (!detach && !strbuf_check_branch_ref(&s, branch) &&
> +                   ref_exists(s.buf))
> +                       printf_ln(_("Preparing worktree (checking out '%s')"),
> +                                 branch);
> +               else {
> +                       struct commit *commit = lookup_commit_reference_by_name(branch);
> +                       if (!commit)
> +                               die(_("invalid reference: %s"), branch);
> +                       printf_ln(_("Preparing worktree (detached HEAD %s)"),
> +                                 find_unique_abbrev(commit->object.oid.hash,
> +                                                    DEFAULT_ABBREV));
> +               }

It's too bad this final case duplicates so much code from
add_worktree(). Perhaps some future refactoring patch (not part of
this series) can consolidate the code.

> +               strbuf_release(&s);
> +       }
> +}
> @@ -434,6 +466,8 @@ static int add(int ac, const char **av, const char *prefix)
> +       print_preparing_worktree_line(opts.detach, branch, new_branch, new_branch_force);
> +
>         if (new_branch) {
>                 struct child_process cp = CHILD_PROCESS_INIT;