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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 5, 2018, 20:15 UTC
Message-ID: <xmqqlgg79oqs.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180205021202.GA17847@duynguyen.dek-tpc.internal>

Duy Nguyen <pclouds@gmail.com> writes:

Show 28 quoted lines

> On Sun, Feb 04, 2018 at 10:13:03PM +0000, 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(&sb, "%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(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
>>  	argv_array_pushf(&child_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.

Show 13 quoted lines

> 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(&sb);
>> +	pp_commit_easy(CMIT_FMT_ONELINE, commit, &sb);
>> +	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.

Show 16 quoted lines

> 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(&cp.args);
>> -		argv_array_pushl(&cp.args, "reset", "--hard", NULL);
>> +		argv_array_pushl(&cp.args, "reset", "--hard", "--quiet", NULL);
>>  		cp.env = child_env.argv;
>>  		ret = run_command(&cp);
>>  		if (ret)
>> -- 
>> 2.16.1.101.gde0f0111ea
>>