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

[Eric Sunshine]

On Mon, Apr 23, 2018 at 3:38 PM, Thomas Gummerer  wrote:
> Currently 'git worktree add' produces output like the following:
>
> Preparing ../foo (identifier foo)
> HEAD is now at 26da330922 
>
> 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 ', 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 '')
>   HEAD is now at 26da330922 
>
>   - '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 

For consistency with the other examples: s/next//

> or:
>
>   Preparing worktree (new branch '')
>   HEAD is now at 26da330922 
>
>   - 'git worktree add --detach' or 'git worktree add 
> ', 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 
>
>   - 'git worktree add  ', which checks out the
> branch prints the following output:

s/prints/and prints/

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

s/// would probably be clearer since you use
 in the example command.

> [...]
> Helped-by: Eric Sunshine 
> Signed-off-by: Thomas Gummerer 
> ---
> 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(, 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 

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

[Thomas Gummerer]

Currently 'git worktree add' produces output like the following:

Preparing ../foo (identifier foo)
HEAD is now at 26da330922 

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:

  - 'git worktree add -b ...' or 'git worktree add ', 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 '')
  HEAD is now at 26da330922 

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

or:

  Preparing worktree (new branch '')
  HEAD is now at 26da330922 

  - 'git worktree add --detach' or 'git worktree add 
', 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 

  - 'git worktree add  ', which checks out the
branch prints the following output:

  Preparing worktree (checking out '')
  HEAD is now at 47007d5 

Additionally currently the "Preparing ..." line is printed to stderr,
while the "HEAD is now at ..." line is printed to stdout by 'git reset
--hard', which is used internally by 'git worktree add'.  Fix this
inconsistency by printing the "Preparing ..." message to stdout as
well.  As "Preparing ..." is not an error, stdout also seems like the
more appropriate output stream.

Helped-by: Eric Sunshine 
Signed-off-by: Thomas Gummerer 
---
 builtin/worktree.c | 38 --
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 4d96a21a45..d348101216 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -301,8 +301,6 @@ static int add_worktree(const char *path, const char 
*refname,
strbuf_addf(, "%s/commondir", sb_repo.buf);
write_file(sb.buf, "../..");
 
-   fprintf_ln(stderr, _("Preparing %s (identifier %s)"), path, name);
-
argv_array_pushf(_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
argv_array_pushf(_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
cp.git_cmd = 1;
@@ -355,6 +353,40 @@ static int add_worktree(const char *path, const char 
*refname,
return ret;
 }
 
+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);
+   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(, 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