Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required

From: Junio C Hamano <gitster@pobox.com>
Date: May 11, 2017, 03:16 UTC
Message-ID: <xmqqa86kccca.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170503210726.24121-1-jn.avila@free.fr>

Jean-Noel Avila <jn.avila@free.fr> writes:

Show 12 quoted lines

> diff --git a/builtin/am.c b/builtin/am.c
> index a95dd8b4e..f5afa438d 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1312,7 +1312,7 @@ static int parse_mail(struct am_state *state, const char *mail)
>  	}
>  
>  	if (is_empty_file(am_path(state, "patch"))) {
> -		printf_ln(_("Patch is empty. Was it split wrong?"));
> +		printf_ln(_("Patch is empty. It may have been split wrong."));
>  		die_user_resolve(state);
>  	}

While I do not belong to "we should feel free to ask rhetorical questions" camp, I do not mind this particular rewrite. An obvious alternative is just to stop the sentence with "Patch is empty."

At this point in the code, we do not even know why we are seeing an empty patch, and "perhaps it was incorrectly split" is not a particularly useful idle speculation that would help the user who sees it.

Show 6 quoted lines

> @@ -1940,7 +1940,7 @@ static void am_resolve(struct am_state *state)
>  
>  	if (unmerged_cache()) {
>  		printf_ln(_("You still have unmerged paths in your index.\n"
> -			"Did you forget to use 'git add'?"));
> +			"You might want to use 'git add' on them."));

This case is *not* an "rhetorical question is the most succinct way to convey the information" situation; I think this rewrite is a definite improvement. "You might want to 'git add' them" may be more succinct, though.

Show 10 quoted lines

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index bfa5419f3..05037b9b6 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1287,7 +1287,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  		 */
>  		if (opts.new_branch && argc == 1)
>  			die(_("Cannot update paths and switch to branch '%s' at the same time.\n"
> -			      "Did you intend to checkout '%s' which can not be resolved as commit?"),
> +			      "'%s' can not be resolved as commit, but it should."),

I am not sure a firm statement "but it should" is an improvement. This message is given when the user says:

    $ git checkout -b newone naster

And "but it should" is appropriate when it is a mistyped "I want to create and checkout 'newone' branch at the same commit as 'master' branch", i.e.

    $ git checkout -b newone master

The reason why the message begins with "Cannot update paths and ..." is because it could be a mistyped "I want to grab the file 'naster' out of 'newone' branch", i.e. the user meant to say this:

    $ git checkout newone naster

IOW, the current error message is hedging its bets, because it does not want to exclude the possibility that "-b" is there by mistake (as opposed to 'naster' is the typo).

If we ignore that possibility and assume that 'naster' is the typo (iow, the user did mean "-b"), then your updated message makes sense. But if we commit to "the user meant -b", we could make the message even more helpful by being more direct, e.g.

	die("'%s' is not a commit and a branch '%s' cannot be created from it",
	    argv[0], opts.new_branch);

Show 13 quoted lines

> diff --git a/help.c b/help.c
> index bc6cd19cf..4658a55c6 100644
> --- a/help.c
> +++ b/help.c
> @@ -411,8 +411,8 @@ const char *help_unknown_cmd(const char *cmd)
>  
>  	if (SIMILAR_ENOUGH(best_similarity)) {
>  		fprintf_ln(stderr,
> -			   Q_("\nDid you mean this?",
> -			      "\nDid you mean one of these?",
> +			   Q_("\nThe most approaching command is",
> +			      "\nThe most approaching commands are",
>  			   n));

With "closest" or "most similar", as others pointed out, I think this may be an improvement.

Thanks.