Re: [PATCH v2 1/3] worktree: improve message when creating a new worktree

[Duy Nguyen]

On Fri, Feb 9, 2018 at 7:08 PM, Duy Nguyen  wrote:
> On Fri, Feb 9, 2018 at 6:27 PM, Thomas Gummerer  wrote:
>> This would loose the information about the identifier of the worktree,
>> but from a coarse look at the man page it doesn't seem like we
>> advertise that widely
>>
>> ...
>>
>> So given that maybe it would even be better to hide the part about the
>> identifier, as it seems more like an implementation detail than
>> relevant to the end user?
>
> Exactly. I'd rather hide it. I haven't found any good reason that a
> user needs to know these IDs unless they poke into $GIT_DIR/worktrees,
> but they should not need to.

Just FYI. I mentioned elsewhere [1] the possibility of a new extended
ref syntax to refer to e.g. HEAD from a different worktree. Such
syntax may make use of this id (which also means we might need to give
the user control over these ids and not just always auto generate
them).

But that's for the future. If/When that thing comes, we can worry
about displaying id here then. For now I still think it's ok to hide
it.

[1] https://public-inbox.org/git/20180210095952.GA9035@ash/T/#u
-- 
Duy

Re: [PATCH v2 1/3] worktree: improve message when creating a new worktree

[Duy Nguyen]

On Fri, Feb 9, 2018 at 6:27 PM, Thomas Gummerer  wrote:
> This would loose the information about the identifier of the worktree,
> but from a coarse look at the man page it doesn't seem like we
> advertise that widely
>
> ...
>
> So given that maybe it would even be better to hide the part about the
> identifier, as it seems more like an implementation detail than
> relevant to the end user?

Exactly. I'd rather hide it. I haven't found any good reason that a
user needs to know these IDs unless they poke into $GIT_DIR/worktrees,
but they should not need to.
-- 
Duy

Re: [PATCH v2 1/3] worktree: improve message when creating a new worktree

[Thomas Gummerer]

On 02/07, Eric Sunshine wrote:
> On Sun, Feb 4, 2018 at 9:12 PM, Duy Nguyen  wrote:
> > As a former translator, I'm not thrilled to see a sentence broken into
> > two pieces like this. I'm not a Japanese translator, but I think this
> > sentence is translated differently when the translator sees the whole
> > line "Preparing ..., setting ...".
> >
> > I think the purpose of "Preparing..." in the first place is to show
> > something when git is busy checkout out the worktree. As long as we
> > print it before git-reset, we should be good.
> 
> The original message was "Enter " which had the potential to
> confuse someone into thinking the working directory had changed[1], so
> it was changed to "Preparing...". The reason for keeping that message
> (rather than dropping it outright) was to provide context to messages
> printed after it, especially messages such as "HEAD is now at..."
> which might otherwise confuse the reader into thinking that HEAD in
> the current worktree changed rather than HEAD in the new
> worktree[2,3].

Thanks for the background!  In that light, since we're already
customizing the "HEAD is now at..." message, is it worth dropping the
message now, maybe using something like "Worktree HEAD is now at ...",
or something similar?

This would loose the information about the identifier of the worktree,
but from a coarse look at the man page it doesn't seem like we
advertise that widely (The only thing related to it I could find is

If the last path components in the working tree's path is unique among
working trees, it can be used to identify worktrees. For example if
you only have two working trees, at "/abc/def/ghi" and "/abc/def/ggg",
then "ghi" or "def/ghi" is enough to point to the former working tree.

for which we don't need to print the identifier anywhere.  We don't
seem to mention the case where the last part is unique, and we add a
number to make the identifier unique.

So given that maybe it would even be better to hide the part about the
identifier, as it seems more like an implementation detail than
relevant to the end user?

> [1]: https://public-inbox.org/git/55a8f4b1.9060...@drmicha.warpmail.net/
> [2]: 
> https://public-inbox.org/git/capig+crshwmmf9ccubrrdccw4kvg9peouxp5vqpsgfxzmxh...@mail.gmail.com/
> [3]: 
> https://public-inbox.org/git/capig+csls4-ukicvmbsknero_fyd722hs1_u6qztrim8cio...@mail.gmail.com/

Re: [PATCH v2 1/3] worktree: improve message when creating a new worktree

[Eric Sunshine]

On Sun, Feb 4, 2018 at 9:12 PM, Duy Nguyen  wrote:
> As a former translator, I'm not thrilled to see a sentence broken into
> two pieces like this. I'm not a Japanese translator, but I think this
> sentence is translated differently when the translator sees the whole
> line "Preparing ..., setting ...".
>
> I think the purpose of "Preparing..." in the first place is to show
> something when git is busy checkout out the worktree. As long as we
> print it before git-reset, we should be good.

The original message was "Enter " which had the potential to
confuse someone into thinking the working directory had changed[1], so
it was changed to "Preparing...". The reason for keeping that message
(rather than dropping it outright) was to provide context to messages
printed after it, especially messages such as "HEAD is now at..."
which might otherwise confuse the reader into thinking that HEAD in
the current worktree changed rather than HEAD in the new
worktree[2,3].

[1]: https://public-inbox.org/git/55a8f4b1.9060...@drmicha.warpmail.net/
[2]: 
https://public-inbox.org/git/capig+crshwmmf9ccubrrdccw4kvg9peouxp5vqpsgfxzmxh...@mail.gmail.com/
[3]: 
https://public-inbox.org/git/capig+csls4-ukicvmbsknero_fyd722hs1_u6qztrim8cio...@mail.gmail.com/

Re: [PATCH v2 1/3] worktree: improve message when creating a new worktree

[Junio C Hamano]

Duy Nguyen  writes:

> On Sun, Feb 04, 2018 at 10:13:03PM +, Thomas Gummerer wrote:
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> index 7cef5b120b..d1549e441d 100644
>> --- a/builtin/worktree.c
>> +++ b/builtin/worktree.c
>> @@ -303,7 +303,7 @@ 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);
>> +fprintf(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);
>> @@ -320,10 +320,19 @@ static int add_worktree(const char *path, const char 
>> *refname,
>>  if (ret)
>>  goto done;
>>  
>> +fprintf(stderr, _(", setting HEAD to %s"),
>
> As a former translator, I'm not thrilled to see a sentence broken into
> two pieces like this. I'm not a Japanese translator, but I think this
> sentence is translated differently when the translator sees the whole
> line "Preparing ..., setting ...".
>
> Since the code between the first fprintf and this one should not take
> long to execute, perhaps we can just delete the first printf and print
> a whole [*] sentence here?

Yeah, also it is a bit troubling that "Preparing" is an incomplete line,
and when update-ref/symblic-ref call fails, will stay to be incomplete.

> I think the purpose of "Preparing..." in the first place is to show
> something when git is busy checkout out the worktree. As long as we
> print it before git-reset, we should be good.
>
>> +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);
>
> [*] Yes I know it's not "whole" because of this piece. But this one is
> more or less a separate sentence already so it's probably ok.

Doing all the additional fprintf/fputc at the same place (instead of
across update-ref/symbolic-ref) would make it easier to follow and
also allow it to make "whole" even with that piece.  Prepare the
abbreviated commit->object.oid.hash and FMT_ONELINE in the same
strbuf and say that the HEAD is there.  I think it also makes sense
to split it into two sentences, so from that point of view, just
dropping the change in the "Preparing" hunk and then saying "HEAD is
now at '9876fdea title of the commit'" after update/symbolic-ref may
also be a good change.  That automatically removes the "what hapepns
to the rest of the incomplete line when run_command() fails?" issue.

> Is it a bit too long to print everything in one line though?
> CMIT_FMT_ONELINE could already fill 70 columns easily.
>
>> +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.16.1.101.gde0f0111ea
>> 

Re: [PATCH v2 1/3] worktree: improve message when creating a new worktree

[Thomas Gummerer]

On 02/05, Duy Nguyen wrote:
> On Sun, Feb 04, 2018 at 10:13:03PM +, Thomas Gummerer wrote:
> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > index 7cef5b120b..d1549e441d 100644
> > --- a/builtin/worktree.c
> > +++ b/builtin/worktree.c
> > @@ -303,7 +303,7 @@ 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);
> > +   fprintf(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);
> > @@ -320,10 +320,19 @@ static int add_worktree(const char *path, const char 
> > *refname,
> > if (ret)
> > goto done;
> >  
> > +   fprintf(stderr, _(", setting HEAD to %s"),
> 
> As a former translator, I'm not thrilled to see a sentence broken into
> two pieces like this. I'm not a Japanese translator, but I think this
> sentence is translated differently when the translator sees the whole
> line "Preparing ..., setting ...".

Good point, I didn't think about the sentence structure other
languages could require here. 

> Since the code between the first fprintf and this one should not take
> long to execute, perhaps we can just delete the first printf and print
> a whole [*] sentence here?
> 
> I think the purpose of "Preparing..." in the first place is to show
> something when git is busy checkout out the worktree. As long as we
> print it before git-reset, we should be good.
> 
> > +   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);
> 
> [*] Yes I know it's not "whole" because of this piece. But this one is
> more or less a separate sentence already so it's probably ok.
> 
> Is it a bit too long to print everything in one line though?
> CMIT_FMT_ONELINE could already fill 70 columns easily.

Yeah, it does get a bit long.  Maybe just splitting the sentences here
would be the best solution, then we also won't have to worry about the
split not being good for translation.  I'll do that.

> > +   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.16.1.101.gde0f0111ea
> > 

Re: [PATCH v2 1/3] worktree: improve message when creating a new worktree

[Duy Nguyen]

On Sun, Feb 04, 2018 at 10:13:03PM +, Thomas Gummerer wrote:
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 7cef5b120b..d1549e441d 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -303,7 +303,7 @@ 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);
> + fprintf(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);
> @@ -320,10 +320,19 @@ static int add_worktree(const char *path, const char 
> *refname,
>   if (ret)
>   goto done;
>  
> + fprintf(stderr, _(", setting HEAD to %s"),

As a former translator, I'm not thrilled to see a sentence broken into
two pieces like this. I'm not a Japanese translator, but I think this
sentence is translated differently when the translator sees the whole
line "Preparing ..., setting ...".

Since the code between the first fprintf and this one should not take
long to execute, perhaps we can just delete the first printf and print
a whole [*] sentence here?

I think the purpose of "Preparing..." in the first place is to show
something when git is busy checkout out the worktree. As long as we
print it before git-reset, we should be good.

> + 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);

[*] Yes I know it's not "whole" because of this piece. But this one is
more or less a separate sentence already so it's probably ok.

Is it a bit too long to print everything in one line though?
CMIT_FMT_ONELINE could already fill 70 columns easily.

> + 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.16.1.101.gde0f0111ea
> 

[PATCH v2 1/3] 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.

Fix these inconsistencies by making the 'git reset --hard' call quiet,
and printing the message ourselves instead.

Signed-off-by: Thomas Gummerer 
---

We might want to do something similar for the 'git branch' command in
the 'add()' function, which currently prints some output if a branch
is set up to track a remote.  I couldn't find a good way to convey
that information in the output here, without making it too verbose,
and it's probably not great to loose that output either.

If anyone has any suggestions for that, I'd be glad to hear them :)

 builtin/worktree.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7cef5b120b..d1549e441d 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -303,7 +303,7 @@ 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);
+   fprintf(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);
@@ -320,10 +320,19 @@ static int add_worktree(const char *path, const char 
*refname,
if (ret)
goto done;
 
+   fprintf(stderr, _(", setting HEAD to %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.16.1.101.gde0f0111ea