Re: [PATCH v4 1/4] worktree: improve message when creating a new worktree

[Thomas Gummerer <t.gummerer@gmail.com>]

On 03/20, Eric Sunshine wrote:
> On Sat, Mar 17, 2018 at 6:22 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > [...]
> > Fix these inconsistencies, and no longer show the identifier by making
> > the 'git reset --hard' call quiet, and printing the message directly
> > from the builtin command instead.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > @@ -303,8 +303,6 @@ static int add_worktree(const char *path, const char *refname,
> >         strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
> >         write_file(sb.buf, "../..");
> >
> > -       fprintf_ln(stderr, _("Preparing %s (identifier %s)"), path, name);
> 
> A minor regression with this change is that error messages from
> git-update-ref or git-symbolic-ref -- which could be emitted after
> this point but before the new "worktree HEAD is now at..." message --
> are now somewhat orphaned. I'm not sure that it matters, though.

If those commands fail, we would now not print the "worktree HEAD is
now at..." message, but go directly to "done", and clean up the
working tree.

So while we no longer emit the "Preparing worktree" header, the user
should still be aware that they are creating a new worktree, and that
the error happened while creating a new worktree.  From a (admittedly
very quick) look the error messages would all make sense in this
context, without an additional message.

Printing the "worktree HEAD is now at ..." message before that
wouldn't make much sense, as we may not even have a new working tree
at the end.  We could add the message back, but that would also put us
back at three lines of output.  I think I prefer the more concise
version here in the normal case, and I think we can live with the
slight regression.  But if others have a strong preference for the
current way I'm happy to add that message back.

> >         argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
> >         argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
> >         cp.git_cmd = 1;
> > @@ -320,10 +318,19 @@ static int add_worktree(const char *path, const char *refname,
> > +       fprintf(stderr, _("worktree HEAD is now at %s"),
> > +               find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
> 
> I wonder if this should say "new worktree HEAD is now at..." to
> clearly indicate that it is talking about HEAD in the _new_ worktree,
> not HEAD in the current worktree.

Yeah I aggree that's nicer.  Will change.

> > +       strbuf_reset(&sb);
> > +       pp_commit_easy(CMIT_FMT_ONELINE, commit, &sb);
> > +       if (sb.len > 0)
> > +               fprintf(stderr, " %s", sb.buf);
> > +       fputc('\n', stderr);