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

[Thomas Gummerer]

On 03/20, Eric Sunshine wrote:
> On Sat, Mar 17, 2018 at 6:22 PM, Thomas Gummerer  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 
> > ---
> > 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(, "%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(_env, "%s=%s", GIT_DIR_ENVIRONMENT, 
> > sb_git.buf);
> > argv_array_pushf(_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();
> > +   pp_commit_easy(CMIT_FMT_ONELINE, commit, );
> > +   if (sb.len > 0)
> > +   fprintf(stderr, " %s", sb.buf);
> > +   fputc('\n', stderr);

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

[Eric Sunshine]

On Sat, Mar 17, 2018 at 6:22 PM, Thomas Gummerer  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 
> ---
> 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(, "%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.

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

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

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

[Junio C Hamano]

Duy Nguyen  writes:

>> @@ -320,10 +318,19 @@ static int add_worktree(const char *path, const char 
>> *refname,
>> if (ret)
>> goto done;
>>
>> +   fprintf(stderr, _("worktree HEAD is now at %s"),
>
> We use the term "working tree" for UI and documents. "worktree" is
> only used in code comments and stuff.

Hmph, that is a bit different from what I recall.  "working tree" is
the phrase we have used and we still use to refer to those things
that are checked out (as opposed to the in-repo data).  We say
"worktree" when we want to stress the fact that we are talking about
a single instance among possibly multiple instances of the "working
tree" that are associated to a single repository.

Technically speaking, the traditional "working tree" is everything
in the directory immediately above ".git/" plus ".git/index"; a
single "worktree" consists of a bit more, as we have to count per
worktree states like .git/rebase-apply/ and .git/refs/bisect/ as
part of it.

And from that point of view, HEAD is one of those per worktree
states, so...

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

[Duy Nguyen]

On Sat, Mar 17, 2018 at 11:22 PM, Thomas Gummerer  wrote:
> Currently 'git worktree add' produces output like the following, when
> '--no-checkout' is not given:
>
> Preparing foo (identifier foo)
> HEAD is now at 26da330922 
>
> where the first line is written to stderr, and the second line coming
> from 'git reset --hard' is written to stdout, even though both lines are
> supposed to tell the user what has happened.  In addition to someone not
> familiar with 'git worktree', this might seem as if the current HEAD was
> modified, not the HEAD in the new working tree.
>
> If the '--no-checkout' flag is given, the output of 'git worktree add'
> is just:
>
> Preparing foo (identifier foo)
>
> even though the HEAD is set to a commit, which is just not checked out.
>
> The identifier is also not particularly relevant for the user at the
> moment, as it's only used internally to distinguish between different
> worktrees that have the same $(basename ).
>
> 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 
> ---
>  builtin/worktree.c | 13 ++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 7cef5b120b..e5d04f0b4b 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -303,8 +303,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;
> @@ -320,10 +318,19 @@ static int add_worktree(const char *path, const char 
> *refname,
> if (ret)
> goto done;
>
> +   fprintf(stderr, _("worktree HEAD is now at %s"),

We use the term "working tree" for UI and documents. "worktree" is
only used in code comments and stuff.

> +   find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
> +
> +   strbuf_reset();
> +   pp_commit_easy(CMIT_FMT_ONELINE, commit, );
> +   if (sb.len > 0)
> +   fprintf(stderr, " %s", sb.buf);
> +   fputc('\n', stderr);
> +
> if (opts->checkout) {
> cp.argv = NULL;
> argv_array_clear();
> -   argv_array_pushl(, "reset", "--hard", NULL);
> +   argv_array_pushl(, "reset", "--hard", "--quiet", 
> NULL);
> cp.env = child_env.argv;
> ret = run_command();
> if (ret)
> --
> 2.17.0.rc0.231.g781580f06
>
-- 
Duy

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

[Thomas Gummerer]

Currently 'git worktree add' produces output like the following, when
'--no-checkout' is not given:

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

where the first line is written to stderr, and the second line coming
from 'git reset --hard' is written to stdout, even though both lines are
supposed to tell the user what has happened.  In addition to someone not
familiar with 'git worktree', this might seem as if the current HEAD was
modified, not the HEAD in the new working tree.

If the '--no-checkout' flag is given, the output of 'git worktree add'
is just:

Preparing foo (identifier foo)

even though the HEAD is set to a commit, which is just not checked out.

The identifier is also not particularly relevant for the user at the
moment, as it's only used internally to distinguish between different
worktrees that have the same $(basename ).

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 
---
 builtin/worktree.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7cef5b120b..e5d04f0b4b 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -303,8 +303,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;
@@ -320,10 +318,19 @@ static int add_worktree(const char *path, const char 
*refname,
if (ret)
goto done;
 
+   fprintf(stderr, _("worktree HEAD is now at %s"),
+   find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
+
+   strbuf_reset();
+   pp_commit_easy(CMIT_FMT_ONELINE, commit, );
+   if (sb.len > 0)
+   fprintf(stderr, " %s", sb.buf);
+   fputc('\n', stderr);
+
if (opts->checkout) {
cp.argv = NULL;
argv_array_clear();
-   argv_array_pushl(, "reset", "--hard", NULL);
+   argv_array_pushl(, "reset", "--hard", "--quiet", NULL);
cp.env = child_env.argv;
ret = run_command();
if (ret)
-- 
2.17.0.rc0.231.g781580f06