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

> -----Original Message-----
> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On
> Behalf Of Junio C Hamano
> Sent: Thursday, May 11, 2017 4:16 AM
> To: Jean-Noel Avila <jn.avila@free.fr>
> Cc: git@vger.kernel.org; rashmipai36@gmail.com
> Subject: Re: [PATCH v2 1/3] usability: don't ask questions if no reply is
> required
>
> Jean-Noel Avila <jn.avila@free.fr> writes:
>
> > 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.

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

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

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