* [PATCH 1/4] usability: don't ask questions if no reply is required @ 2017-05-03 16:29 Jean-Noel Avila 2017-05-03 16:29 ` [PATCH 2/4] usability: fix am and checkout for nevermind questions Jean-Noel Avila ` (6 more replies) 0 siblings, 7 replies; 41+ messages in thread From: Jean-Noel Avila @ 2017-05-03 16:29 UTC (permalink / raw) To: git; +Cc: Jean-Noel Avila As described in the bug report at https://github.com/git/git-scm.com/issues/999 the user was disconcerted by the question asked by the program not requiring a reply from the user. To improve the general usability of the Git suite, The following rule was applied: if the sentence * appears in a non-interactive session * is printed last before exit * is a question addressing the user ("you") the sentence is turned into affirmative and proposes the option. Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> --- help.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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)); for (i = 0; i < n; i++) -- 2.12.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 2/4] usability: fix am and checkout for nevermind questions 2017-05-03 16:29 [PATCH 1/4] usability: don't ask questions if no reply is required Jean-Noel Avila @ 2017-05-03 16:29 ` Jean-Noel Avila 2017-05-03 16:51 ` Jonathan Nieder 2017-05-03 16:29 ` [PATCH 3/4] read-tree.c: rework UI when merging no trees Jean-Noel Avila ` (5 subsequent siblings) 6 siblings, 1 reply; 41+ messages in thread From: Jean-Noel Avila @ 2017-05-03 16:29 UTC (permalink / raw) To: git; +Cc: Jean-Noel Avila Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> --- builtin/am.c | 4 ++-- builtin/checkout.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) 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); } @@ -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.")); die_user_resolve(state); } 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."), opts.new_branch, argv[0]); if (opts.force_detach) -- 2.12.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] usability: fix am and checkout for nevermind questions 2017-05-03 16:29 ` [PATCH 2/4] usability: fix am and checkout for nevermind questions Jean-Noel Avila @ 2017-05-03 16:51 ` Jonathan Nieder 2017-05-03 18:35 ` Jean-Noël AVILA 0 siblings, 1 reply; 41+ messages in thread From: Jonathan Nieder @ 2017-05-03 16:51 UTC (permalink / raw) To: Jean-Noel Avila; +Cc: git Jean-Noel Avila wrote: > Subject: usability: fix am and checkout for nevermind questions > > Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> Thanks for working on improving Git's UX. I agree with the goal in general (we should not gratuitously surprise users) but I think I lack context for appreciating this particular example. This is a good place to describe the motivation behind the patch and what effective change it would have. [...] > +++ b/builtin/am.c [...] > 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.")); [...] > 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.")); [...] > 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."), In the current state I think this patch makes things worse (questions are not automatically a bad thing), which would make it especially useful to see more about the motivation so we can find out whether there's another way. Thanks, Jonathan ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] usability: fix am and checkout for nevermind questions 2017-05-03 16:51 ` Jonathan Nieder @ 2017-05-03 18:35 ` Jean-Noël AVILA 0 siblings, 0 replies; 41+ messages in thread From: Jean-Noël AVILA @ 2017-05-03 18:35 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git Le mercredi 3 mai 2017 09:51:58 CEST, vous avez écrit : > Jean-Noel Avila wrote: > > Subject: usability: fix am and checkout for nevermind questions > > > > Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> > > Thanks for working on improving Git's UX. I agree with the goal in > general (we should not gratuitously surprise users) but I think I > lack context for appreciating this particular example. > > This is a good place to describe the motivation behind the patch and > what effective change it would have. > > [...] > > > +++ b/builtin/am.c > > [...] > > > 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.")); > > [...] > > > 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.")); > > [...] > > > 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."), > > In the current state I think this patch makes things worse (questions > are not automatically a bad thing), which would make it especially > useful to see more about the motivation so we can find out whether > there's another way. > I am not a UX designer, but for me, in the context of interaction with a command line program, any question that does not accept a reply is bad design. That also means that any command that does not run interactively should not ask questions. The shell interface is too informal to allow being loose on the program side. Comparatively to a GUI, where a label is formally informative and a popping-up dialog box asks for user input. This patch should indeed be squashed with the first one. They are small changes in strings printed when dying. They would share the more extended commit message. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 3/4] read-tree.c: rework UI when merging no trees 2017-05-03 16:29 [PATCH 1/4] usability: don't ask questions if no reply is required Jean-Noel Avila 2017-05-03 16:29 ` [PATCH 2/4] usability: fix am and checkout for nevermind questions Jean-Noel Avila @ 2017-05-03 16:29 ` Jean-Noel Avila 2017-05-03 17:04 ` Jonathan Nieder 2017-05-03 16:29 ` [PATCH 4/4] git-filter-branch: be assertative on dying message Jean-Noel Avila ` (4 subsequent siblings) 6 siblings, 1 reply; 41+ messages in thread From: Jean-Noel Avila @ 2017-05-03 16:29 UTC (permalink / raw) To: git; +Cc: Jean-Noel Avila The initial test was inherited from a previous commit, but it is no longer needed, given the following switch case. Moreover, the question sentence ending the program has been replace by an assertative one. Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> --- builtin/read-tree.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/builtin/read-tree.c b/builtin/read-tree.c index 23e212ee8..05296997c 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -226,9 +226,10 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) setup_work_tree(); if (opts.merge) { - if (stage < 2) - die("just how do you expect me to merge %d trees?", stage-1); switch (stage - 1) { + case 0: + die("there are no trees to merge!"); + break; case 1: opts.fn = opts.prefix ? bind_merge : oneway_merge; break; -- 2.12.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 3/4] read-tree.c: rework UI when merging no trees 2017-05-03 16:29 ` [PATCH 3/4] read-tree.c: rework UI when merging no trees Jean-Noel Avila @ 2017-05-03 17:04 ` Jonathan Nieder 2017-05-03 18:39 ` Jean-Noël AVILA 0 siblings, 1 reply; 41+ messages in thread From: Jonathan Nieder @ 2017-05-03 17:04 UTC (permalink / raw) To: Jean-Noel Avila; +Cc: git Hi, Jean-Noel Avila wrote: > Subject: read-tree.c: rework UI when merging no trees nit: this is about user-facing behavior, not an implementation detail, so the part before the colon can be the command that changed (read-tree:). nit: the word "rework" is dangerous in a commit message in the same way as the word "fix" --- it stands for "make better", in a vague way that leaves the reader guessing about how. Usually a more specific description can work better. > The initial test was inherited from a previous commit, but it is no > longer needed, given the following switch case. Moreover, the question > sentence ending the program has been replace by an assertative one. > > Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> This can have a simpler, short-and-sweet motivation: read-tree -m: make error message for merging 0 trees less smart-alecky "git read-tree -m" requires a tree argument to name the tree to be merged in. Git uses a cutesy error message to say so and why: $ git read-tree -m warning: read-tree: emptying the index with no arguments is deprecated; use --empty fatal: just how do you expect me to merge 0 trees? $ git read-tree -m --empty fatal: just how do you expect me to merge 0 trees? When lucky, that could produce an ah-hah moment for the user, but it's more likely to irritate and distract them. Instead, tell the user plainly that the tree argument is required. Also document this requirement in the git-read-tree(1) manpage where there is room to explain it in a more straightforward way. Unfortunately both 'git read-tree -h' and 'git read-tree --help' say nothing about this. Ideas for wording there? Thanks and hope that helps, Jonathan ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 3/4] read-tree.c: rework UI when merging no trees 2017-05-03 17:04 ` Jonathan Nieder @ 2017-05-03 18:39 ` Jean-Noël AVILA 0 siblings, 0 replies; 41+ messages in thread From: Jean-Noël AVILA @ 2017-05-03 18:39 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git Le mercredi 3 mai 2017, 10:04:01 CEST Jonathan Nieder a écrit : > Hi, > > Jean-Noel Avila wrote: > > Subject: read-tree.c: rework UI when merging no trees > > nit: this is about user-facing behavior, not an implementation detail, > so the part before the colon can be the command that changed > (read-tree:). > > nit: the word "rework" is dangerous in a commit message in the same > way as the word "fix" --- it stands for "make better", in a vague way > that leaves the reader guessing about how. Usually a more specific > description can work better. > In fact, this patch is two fold: * reword the question in the die() call. I realize now that when passed to die(), the string is prepended with "fatal:". That's an hint that the question does not require a reply, but ruling out any doubt would be better. * rework the local logic which was inherited from history. This is functionally equivalent to the previous version, just cleaner. > > The initial test was inherited from a previous commit, but it is no > > longer needed, given the following switch case. Moreover, the question > > sentence ending the program has been replace by an assertative one. > > > > Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> > > This can have a simpler, short-and-sweet motivation: > > read-tree -m: make error message for merging 0 trees less smart-alecky > > "git read-tree -m" requires a tree argument to name the tree to be > merged in. Git uses a cutesy error message to say so and why: > > $ git read-tree -m > warning: read-tree: emptying the index with no arguments is deprecated; > use --empty fatal: just how do you expect me to merge 0 trees? > $ git read-tree -m --empty > fatal: just how do you expect me to merge 0 trees? > > When lucky, that could produce an ah-hah moment for the user, but it's > more likely to irritate and distract them. > > Instead, tell the user plainly that the tree argument is required. Also > document this requirement in the git-read-tree(1) manpage where there is > room to explain it in a more straightforward way. > Thank you very much for this message! May I s-o-b you? As hinted, I'll add the documentation part. ;-) > Unfortunately both 'git read-tree -h' and 'git read-tree --help' say nothing > about this. Ideas for wording there? Next pach series will propose this. > > Thanks and hope that helps, > Jonathan ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 4/4] git-filter-branch: be assertative on dying message 2017-05-03 16:29 [PATCH 1/4] usability: don't ask questions if no reply is required Jean-Noel Avila 2017-05-03 16:29 ` [PATCH 2/4] usability: fix am and checkout for nevermind questions Jean-Noel Avila 2017-05-03 16:29 ` [PATCH 3/4] read-tree.c: rework UI when merging no trees Jean-Noel Avila @ 2017-05-03 16:29 ` Jean-Noel Avila 2017-05-03 17:07 ` Jonathan Nieder 2017-05-03 16:47 ` [PATCH 1/4] usability: don't ask questions if no reply is required Jonathan Nieder ` (3 subsequent siblings) 6 siblings, 1 reply; 41+ messages in thread From: Jean-Noel Avila @ 2017-05-03 16:29 UTC (permalink / raw) To: git; +Cc: Jean-Noel Avila Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> --- git-filter-branch.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-filter-branch.sh b/git-filter-branch.sh index 2b8cdba15..dd3a605d0 100755 --- a/git-filter-branch.sh +++ b/git-filter-branch.sh @@ -239,7 +239,7 @@ git rev-parse --no-flags --revs-only --symbolic-full-name \ sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads test -s "$tempdir"/heads || - die "Which ref do you want to rewrite?" + die "You must specify a ref to rewrite" GIT_INDEX_FILE="$(pwd)/../index" export GIT_INDEX_FILE -- 2.12.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] git-filter-branch: be assertative on dying message 2017-05-03 16:29 ` [PATCH 4/4] git-filter-branch: be assertative on dying message Jean-Noel Avila @ 2017-05-03 17:07 ` Jonathan Nieder 0 siblings, 0 replies; 41+ messages in thread From: Jonathan Nieder @ 2017-05-03 17:07 UTC (permalink / raw) To: Jean-Noel Avila; +Cc: git Hi, Jean-Noel Avila wrote: > Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> As with the previous patches, this is a good place to put the motivation for the patch. > --- > git-filter-branch.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/git-filter-branch.sh b/git-filter-branch.sh > index 2b8cdba15..dd3a605d0 100755 > --- a/git-filter-branch.sh > +++ b/git-filter-branch.sh > @@ -239,7 +239,7 @@ git rev-parse --no-flags --revs-only --symbolic-full-name \ > sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads > > test -s "$tempdir"/heads || > - die "Which ref do you want to rewrite?" > + die "You must specify a ref to rewrite" I find both the old and the new messages pretty uncompelling. The user got the usage wrong but we don't know what they were trying to do --- e.g. maybe they specified the ref to rewrite but in the wrong place. Would e.g. a simple call to 'usage' work? Thanks, Jonathan ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/4] usability: don't ask questions if no reply is required 2017-05-03 16:29 [PATCH 1/4] usability: don't ask questions if no reply is required Jean-Noel Avila ` (2 preceding siblings ...) 2017-05-03 16:29 ` [PATCH 4/4] git-filter-branch: be assertative on dying message Jean-Noel Avila @ 2017-05-03 16:47 ` Jonathan Nieder 2017-05-03 16:58 ` Stefan Beller 2017-05-03 17:37 ` Jean-Noël AVILA 2017-05-03 21:07 ` [PATCH v2 1/3] " Jean-Noel Avila ` (2 subsequent siblings) 6 siblings, 2 replies; 41+ messages in thread From: Jonathan Nieder @ 2017-05-03 16:47 UTC (permalink / raw) To: Jean-Noel Avila; +Cc: git Hi, Jean-Noel Avila wrote: > As described in the bug report at > > https://github.com/git/git-scm.com/issues/999 External issue tracker URLs have been known to change or disappear and we try to make commit messages self-contained instead of relying on them. It is common to put a 'Requested-by:' footer or sentence saying 'Requested at <url> by <person>' near the bottom of a commit message for attribution and context. Relying on the bug report more heavily like this example (instead of including any relevant information) makes it harder for a reader to understand the patch easily in one place. In other words, instead of asking the reader to read the bug report, please include pertinent information the reader needs to understand the patch here so they don't have to. > the user was disconcerted by the question asked by the program not > requiring a reply from the user. To improve the general usability of > the Git suite, The following rule was applied: > > if the sentence > * appears in a non-interactive session > * is printed last before exit > * is a question addressing the user ("you") > > the sentence is turned into affirmative and proposes the option. > > Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> > --- > help.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > 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)); For what it's worth, I find the new text harder to understand than the old text. From the bug report: Now git says git: 'stahs' is not a git command. See 'git --help'. Did you mean this? stash Git asked if i meant git stash. and i entered yes. and git printed the character y infinite times. If I'm reading that correctly, the problem is not that questions are alarming but that Git did not cope well with the answer. When I try to reproduce it, I get $ git stahs WARNING: You called a Git command named 'stahs', which does not exist. Continuing under the assumption that you meant 'stash' in 5.0 seconds automatically... which is much clearer. After commenting out "[help] autocorrect = 50" in my ~/.config/git/config, I get $ git stahs git: 'stahs' is not a git command. See 'git --help'. Did you mean this? stash which does seem improvable, at least for consistency with the autocorrect case. For example, would something like $ git stahs fatal: You called a Git command named 'stahs', which does not exist. hint: Did you mean 'git stash'? work better? And the autocorrect case could say something like $ git stahs warning: You called a Git command named 'stahs', which does not exist. warning: Continuing under the assumption that you meant 'stash' warning: in 5.0 seconds automatically... Is contact information for the bug reporter available so we can try out different wordings and see what works for them? Thanks and hope that helps, Jonathan ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/4] usability: don't ask questions if no reply is required 2017-05-03 16:47 ` [PATCH 1/4] usability: don't ask questions if no reply is required Jonathan Nieder @ 2017-05-03 16:58 ` Stefan Beller 2017-05-03 17:37 ` Jean-Noël AVILA 1 sibling, 0 replies; 41+ messages in thread From: Stefan Beller @ 2017-05-03 16:58 UTC (permalink / raw) To: Jonathan Nieder, rashmipai36; +Cc: Jean-Noel Avila, git@vger.kernel.org +cc rashmipai36@gmail.com On Wed, May 3, 2017 at 9:47 AM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Hi, > > Jean-Noel Avila wrote: > >> As described in the bug report at >> >> https://github.com/git/git-scm.com/issues/999 > > External issue tracker URLs have been known to change or disappear and > we try to make commit messages self-contained instead of relying on > them. It is common to put a 'Requested-by:' footer or sentence saying > 'Requested at <url> by <person>' near the bottom of a commit message > for attribution and context. Relying on the bug report more heavily > like this example (instead of including any relevant information) > makes it harder for a reader to understand the patch easily in > one place. > > In other words, instead of asking the reader to read the bug report, > please include pertinent information the reader needs to > understand the patch here so they don't have to. > >> the user was disconcerted by the question asked by the program not >> requiring a reply from the user. To improve the general usability of >> the Git suite, The following rule was applied: >> >> if the sentence >> * appears in a non-interactive session >> * is printed last before exit >> * is a question addressing the user ("you") >> >> the sentence is turned into affirmative and proposes the option. >> >> Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> >> --- >> help.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> 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)); > > For what it's worth, I find the new text harder to understand than the > old text. > > From the bug report: > > Now git says git: 'stahs' is not a git command. See 'git --help'. > Did you mean this? > > stash > > Git asked if i meant git stash. and i entered yes. and git > printed the character y infinite times. > > If I'm reading that correctly, the problem is not that questions are > alarming but that Git did not cope well with the answer. When I try > to reproduce it, I get > > $ git stahs > WARNING: You called a Git command named 'stahs', which does not exist. > Continuing under the assumption that you meant 'stash' > in 5.0 seconds automatically... > > which is much clearer. After commenting out "[help] autocorrect = 50" in my > ~/.config/git/config, I get > > $ git stahs > git: 'stahs' is not a git command. See 'git --help'. > > Did you mean this? > stash > > which does seem improvable, at least for consistency with the > autocorrect case. For example, would something like > > $ git stahs > fatal: You called a Git command named 'stahs', which does not exist. > hint: Did you mean 'git stash'? > > work better? And the autocorrect case could say something like > > $ git stahs > warning: You called a Git command named 'stahs', which does not exist. > warning: Continuing under the assumption that you meant 'stash' > warning: in 5.0 seconds automatically... > > Is contact information for the bug reporter available so we can try out > different wordings and see what works for them? yes, cc'd. Also see https://public-inbox.org/git/CAOqCAXSOZCG8mijV+yATtmC1PFGYiOSqtraSdbhbP2rRHBO_Qg@mail.gmail.com > > Thanks and hope that helps, > Jonathan ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/4] usability: don't ask questions if no reply is required 2017-05-03 16:47 ` [PATCH 1/4] usability: don't ask questions if no reply is required Jonathan Nieder 2017-05-03 16:58 ` Stefan Beller @ 2017-05-03 17:37 ` Jean-Noël AVILA 1 sibling, 0 replies; 41+ messages in thread From: Jean-Noël AVILA @ 2017-05-03 17:37 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git Le mercredi 3 mai 2017, 09:47:44 CEST Jonathan Nieder a écrit : > Hi, > > Jean-Noel Avila wrote: > > As described in the bug report at > > > > https://github.com/git/git-scm.com/issues/999 > > External issue tracker URLs have been known to change or disappear and > we try to make commit messages self-contained instead of relying on > them. It is common to put a 'Requested-by:' footer or sentence saying > 'Requested at <url> by <person>' near the bottom of a commit message > for attribution and context. Relying on the bug report more heavily > like this example (instead of including any relevant information) > makes it harder for a reader to understand the patch easily in > one place. > > In other words, instead of asking the reader to read the bug report, > please include pertinent information the reader needs to > understand the patch here so they don't have to. Ok. Will include more context in the commit message and just provide the BT as an additional link. > > > the user was disconcerted by the question asked by the program not > > requiring a reply from the user. To improve the general usability of > > the Git suite, The following rule was applied: > > > > if the sentence > > > > * appears in a non-interactive session > > * is printed last before exit > > * is a question addressing the user ("you") > > > > the sentence is turned into affirmative and proposes the option. > > > > Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> > > --- > > > > help.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > 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)); > > For what it's worth, I find the new text harder to understand than the > old text. > > From the bug report: > > Now git says git: 'stahs' is not a git command. See 'git --help'. > Did you mean this? > > stash > > Git asked if i meant git stash. and i entered yes. and git > printed the character y infinite times. > > If I'm reading that correctly, the problem is not that questions are > alarming but that Git did not cope well with the answer. When I try > to reproduce it, I get No, I don't think that the questions are alarming. The whole point is that Git no longer runs when the user enters its reply. In the case of the bug report, the user was unlucky to type in the name of the shell command `yes` because he was thinking that Git was still running interactively, due to the question at the end of the run. So this patch series'aim is simply to get rid of asking questions just before exiting. Even if a question might seem more user friendly, it's insufficiently formal to indicate to the user that there's no point replying. The question was just a hint, and it should presented as such. To be fair, I'm not accustomed enough to the code to know exactly in which cases the given strings are occurring (except here). All the patch series tries to tackle this at different levels. Maybe squashing them all would be better for understanding. > > $ git stahs > WARNING: You called a Git command named 'stahs', which does not exist. > Continuing under the assumption that you meant 'stash' > in 5.0 seconds automatically... > > which is much clearer. After commenting out "[help] autocorrect = 50" in my > ~/.config/git/config, I get > > $ git stahs > git: 'stahs' is not a git command. See 'git --help'. > > Did you mean this? > stash > > which does seem improvable, at least for consistency with the > autocorrect case. For example, would something like > > $ git stahs > fatal: You called a Git command named 'stahs', which does not exist. > hint: Did you mean 'git stash'? > > work better? And the autocorrect case could say something like Would adding a "hint:" prefix be enough to provide context? I don't think so. I'd prefer to be clearer on the objectives of the printed information, even at the risk of being clumsy. > > $ git stahs > warning: You called a Git command named 'stahs', which does not exist. > warning: Continuing under the assumption that you meant 'stash' > warning: in 5.0 seconds automatically... > > Is contact information for the bug reporter available so we can try out > different wordings and see what works for them? I guess so. The discussion on github is still open and only depends on the willingness of the reporter to reply. > > Thanks and hope that helps, > Jonathan ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 1/3] usability: don't ask questions if no reply is required 2017-05-03 16:29 [PATCH 1/4] usability: don't ask questions if no reply is required Jean-Noel Avila ` (3 preceding siblings ...) 2017-05-03 16:47 ` [PATCH 1/4] usability: don't ask questions if no reply is required Jonathan Nieder @ 2017-05-03 21:07 ` Jean-Noel Avila 2017-05-03 21:07 ` [PATCH v2 2/3] read-tree -m: make error message for merging 0 trees less smart aleck Jean-Noel Avila ` (3 more replies) 2017-05-11 12:06 ` [PATCH v3 " Jean-Noel Avila 2017-05-12 13:03 ` [PATCH v4 1/3] usability: don't ask questions if no reply is required Jean-Noel Avila 6 siblings, 4 replies; 41+ messages in thread From: Jean-Noel Avila @ 2017-05-03 21:07 UTC (permalink / raw) To: git; +Cc: rashmipai36, Jean-Noel Avila There has been a bug report by a corporate user that stated that "spelling mistake of stash followed by a yes prints character 'y' infinite times." This analysis was false. When the spelling of a command contains errors, the git program tries to help the user by providing candidates which are close to the unexisting command. E.g Git prints the following: git: 'stahs' is not a git command. See 'git --help'. Did you mean this? stash and then exits. The problem with this hint is that it is not formally indicated as an hint and the user is in fact encouraged to reply to the question, whereas the Git command is already finished. The user was unlucky enough that it was the command he was looking for, and replied "yes" on the command line, effectively launching the `yes` program. The initial error is that the Git programs, when launched in command-line mode (without interaction) must not ask questions, because these questions would normally require a user input as a reply while they won't handle indeed. That's a source of confusion on UX level. To improve the general usability of the Git suite, the following rule was applied: if the sentence * appears in a non-interactive session * is printed last before exit * is a question addressing the user ("you") the sentence is turned into affirmative and proposes the option. The basic rewording of the question sentences has been extended to other spots found in the source. Requested at https://github.com/git/git-scm.com/issues/999 by rpai1 Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> --- builtin/am.c | 4 ++-- builtin/checkout.c | 2 +- help.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) 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); } @@ -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.")); die_user_resolve(state); } 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."), opts.new_branch, argv[0]); if (opts.force_detach) 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)); for (i = 0; i < n; i++) -- 2.12.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v2 2/3] read-tree -m: make error message for merging 0 trees less smart aleck 2017-05-03 21:07 ` [PATCH v2 1/3] " Jean-Noel Avila @ 2017-05-03 21:07 ` Jean-Noel Avila 2017-05-11 3:46 ` Junio C Hamano 2017-05-03 21:07 ` [PATCH v2 3/3] git-filter-branch: make the error msg when missing branch more open Jean-Noel Avila ` (2 subsequent siblings) 3 siblings, 1 reply; 41+ messages in thread From: Jean-Noel Avila @ 2017-05-03 21:07 UTC (permalink / raw) To: git; +Cc: rashmipai36, Jean-Noel Avila, Jonathan Nieder "git read-tree -m" requires a tree argument to name the tree to be merged in. Git uses a cutesy error message to say so and why: $ git read-tree -m warning: read-tree: emptying the index with no arguments is deprecated; use --empty fatal: just how do you expect me to merge 0 trees? $ git read-tree -m --empty fatal: just how do you expect me to merge 0 trees? When lucky, that could produce an ah-hah moment for the user, but it's more likely to irritate and distract them. Instead, tell the user plainly that the tree argument is required. Also document this requirement in the git-read-tree(1) manpage where there is room to explain it in a more straightforward way. Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Documentation/git-read-tree.txt | 8 ++++---- builtin/read-tree.c | 7 ++++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt index ed9d63ef4..7cd9c6306 100644 --- a/Documentation/git-read-tree.txt +++ b/Documentation/git-read-tree.txt @@ -135,10 +135,10 @@ OPTIONS Merging ------- -If `-m` is specified, 'git read-tree' can perform 3 kinds of -merge, a single tree merge if only 1 tree is given, a -fast-forward merge with 2 trees, or a 3-way merge if 3 trees are -provided. +If `-m` is specified, at least one tree must be given on the command +line. 'git read-tree' can perform 3 kinds of merge, a single tree +merge if only 1 tree is given, a fast-forward merge with 2 trees, or a +3-way merge if 3 trees are provided. Single Tree Merge diff --git a/builtin/read-tree.c b/builtin/read-tree.c index 23e212ee8..68c5b0ca4 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -132,7 +132,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) OPT_BOOL(0, "empty", &read_empty, N_("only empty the index")), OPT__VERBOSE(&opts.verbose_update, N_("be verbose")), - OPT_GROUP(N_("Merging")), + OPT_GROUP(N_("Merging (needs at least one tree-ish")), OPT_BOOL('m', NULL, &opts.merge, N_("perform a merge in addition to a read")), OPT_BOOL(0, "trivial", &opts.trivial_merges_only, @@ -226,9 +226,10 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) setup_work_tree(); if (opts.merge) { - if (stage < 2) - die("just how do you expect me to merge %d trees?", stage-1); switch (stage - 1) { + case 0: + die("you must specify at least one tree to merge"); + break; case 1: opts.fn = opts.prefix ? bind_merge : oneway_merge; break; -- 2.12.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v2 2/3] read-tree -m: make error message for merging 0 trees less smart aleck 2017-05-03 21:07 ` [PATCH v2 2/3] read-tree -m: make error message for merging 0 trees less smart aleck Jean-Noel Avila @ 2017-05-11 3:46 ` Junio C Hamano 2017-05-11 4:31 ` [PATCH] read-tree: "read-tree -m --empty" does not make sense Junio C Hamano 0 siblings, 1 reply; 41+ messages in thread From: Junio C Hamano @ 2017-05-11 3:46 UTC (permalink / raw) To: Jean-Noel Avila; +Cc: git, rashmipai36, Jonathan Nieder Jean-Noel Avila <jn.avila@free.fr> writes: > "git read-tree -m" requires a tree argument to name the tree to be > merged in. Git uses a cutesy error message to say so and why: > > $ git read-tree -m > warning: read-tree: emptying the index with no arguments is > deprecated; use --empty > fatal: just how do you expect me to merge 0 trees? > $ git read-tree -m --empty > fatal: just how do you expect me to merge 0 trees? This shows another issue. The "emptying ... is deprecated" message shouldn't be given when -m is present. I am not saying that that needs to be fixed by you and/or as a part of this patch. Just something I noticed while reviewing the patch. > Merging > ------- > -If `-m` is specified, 'git read-tree' can perform 3 kinds of > -merge, a single tree merge if only 1 tree is given, a > -fast-forward merge with 2 trees, or a 3-way merge if 3 trees are > -provided. > +If `-m` is specified, at least one tree must be given on the command > +line. 'git read-tree' can perform 3 kinds of merge, a single tree > +merge if only 1 tree is given, a fast-forward merge with 2 trees, or a > +3-way merge if 3 trees are provided. It may not incorrect per-se, but the existing enumeration already say 1, 2 and 3 are the valid choices, so "at least one" may be redundant. One incorrectness that needs to be changed is "if 3 trees are provided"; it is "if 3 or more trees". Again, not the topic of your change, but this one you may want to address while you are at it. > if (opts.merge) { > - if (stage < 2) > - die("just how do you expect me to merge %d trees?", stage-1); > switch (stage - 1) { > + case 0: Could "stage" be 0 (or negative) when we come here? If so, this rewrite may no longer diagnose the error correctly in such a case. ... goes and looks ... I think it begins with either 0 or 1 and then only counts up, so we should be safe. Rolling it in the switch() like this patch does makes it easier to follow what is going on, I think. > + die("you must specify at least one tree to merge"); > + break; > case 1: > opts.fn = opts.prefix ? bind_merge : oneway_merge; > break; Thanks. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH] read-tree: "read-tree -m --empty" does not make sense 2017-05-11 3:46 ` Junio C Hamano @ 2017-05-11 4:31 ` Junio C Hamano 0 siblings, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2017-05-11 4:31 UTC (permalink / raw) To: git; +Cc: Jean-Noel Avila fb1bb965 ("read-tree: deprecate syntax without tree-ish args", 2010-09-10) wanted to deprecate "git read-tree" without any tree, which used to be the way to empty the index, and encourage use of "git read-tree --empty" instead. However, when used with "-m", "--empty" does not make any sense, either, simply because merging 0 trees will result in a different error anyway. Omit the deprecation warning and let the code to emit real error message diagnose the error. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Junio C Hamano <gitster@pobox.com> writes: > Jean-Noel Avila <jn.avila@free.fr> writes: > >> "git read-tree -m" requires a tree argument to name the tree to be >> merged in. Git uses a cutesy error message to say so and why: >> >> $ git read-tree -m >> warning: read-tree: emptying the index with no arguments is >> deprecated; use --empty >> fatal: just how do you expect me to merge 0 trees? >> $ git read-tree -m --empty >> fatal: just how do you expect me to merge 0 trees? > > This shows another issue. The "emptying ... is deprecated" message > shouldn't be given when -m is present. > > I am not saying that that needs to be fixed by you and/or as a part > of this patch. Just something I noticed while reviewing the patch. builtin/read-tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/read-tree.c b/builtin/read-tree.c index 23e212ee8c..284de743c3 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -210,7 +210,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) die("failed to unpack tree object %s", arg); stage++; } - if (nr_trees == 0 && !read_empty) + if (!nr_trees && !read_empty && !opts.merge) warning("read-tree: emptying the index with no arguments is deprecated; use --empty"); else if (nr_trees > 0 && read_empty) die("passing trees as arguments contradicts --empty"); -- 2.13.0-336-gf73534b083 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v2 3/3] git-filter-branch: make the error msg when missing branch more open 2017-05-03 21:07 ` [PATCH v2 1/3] " Jean-Noel Avila 2017-05-03 21:07 ` [PATCH v2 2/3] read-tree -m: make error message for merging 0 trees less smart aleck Jean-Noel Avila @ 2017-05-03 21:07 ` Jean-Noel Avila 2017-05-11 3:53 ` Junio C Hamano 2017-05-04 8:52 ` [PATCH v2 1/3] usability: don't ask questions if no reply is required Kerry, Richard 2017-05-11 3:16 ` Junio C Hamano 3 siblings, 1 reply; 41+ messages in thread From: Jean-Noel Avila @ 2017-05-03 21:07 UTC (permalink / raw) To: git; +Cc: rashmipai36, Jean-Noel Avila git-filter-branch requires the specification of a branch by one way or another. If no branch appears to have been specified, we know the user got the usage wrong but we don't know what they were trying to do --- e.g. maybe they specified the ref to rewrite but in the wrong place. The safest solution is to just print the usage in this case. Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> --- git-filter-branch.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-filter-branch.sh b/git-filter-branch.sh index 2b8cdba15..bda2bae23 100755 --- a/git-filter-branch.sh +++ b/git-filter-branch.sh @@ -239,7 +239,7 @@ git rev-parse --no-flags --revs-only --symbolic-full-name \ sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads test -s "$tempdir"/heads || - die "Which ref do you want to rewrite?" + usage GIT_INDEX_FILE="$(pwd)/../index" export GIT_INDEX_FILE -- 2.12.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v2 3/3] git-filter-branch: make the error msg when missing branch more open 2017-05-03 21:07 ` [PATCH v2 3/3] git-filter-branch: make the error msg when missing branch more open Jean-Noel Avila @ 2017-05-11 3:53 ` Junio C Hamano 0 siblings, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2017-05-11 3:53 UTC (permalink / raw) To: Jean-Noel Avila; +Cc: git, rashmipai36 Jean-Noel Avila <jn.avila@free.fr> writes: > git-filter-branch requires the specification of a branch by one way or > another. If no branch appears to have been specified, we know the user > got the usage wrong but we don't know what they were trying to do --- > e.g. maybe they specified the ref to rewrite but in the wrong place. > > The safest solution is to just print the usage in this case. > > Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> > --- > git-filter-branch.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/git-filter-branch.sh b/git-filter-branch.sh > index 2b8cdba15..bda2bae23 100755 > --- a/git-filter-branch.sh > +++ b/git-filter-branch.sh > @@ -239,7 +239,7 @@ git rev-parse --no-flags --revs-only --symbolic-full-name \ > sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads > > test -s "$tempdir"/heads || > - die "Which ref do you want to rewrite?" > + usage > > GIT_INDEX_FILE="$(pwd)/../index" > export GIT_INDEX_FILE I tend to agree with Ævar on this one. It is not apparent to the end user after this change what exactly was wrong in the input; for that matter, it is not even clear that the command is refusing to run because it found problem with the input. Trying to move away from asking "I didn't get that, what did you mean?" is one thing, and that can be done by saying "no ref to rewrite given" or something. We may want to make it into a more "positive" nudge, telling the user what to do, e.g. "give me the refs to rewrite." Thanks. ^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v2 1/3] usability: don't ask questions if no reply is required 2017-05-03 21:07 ` [PATCH v2 1/3] " Jean-Noel Avila 2017-05-03 21:07 ` [PATCH v2 2/3] read-tree -m: make error message for merging 0 trees less smart aleck Jean-Noel Avila 2017-05-03 21:07 ` [PATCH v2 3/3] git-filter-branch: make the error msg when missing branch more open Jean-Noel Avila @ 2017-05-04 8:52 ` Kerry, Richard 2017-05-04 9:09 ` Ævar Arnfjörð Bjarmason 2017-05-04 9:41 ` Jean-Noël AVILA 2017-05-11 3:16 ` Junio C Hamano 3 siblings, 2 replies; 41+ messages in thread From: Kerry, Richard @ 2017-05-04 8:52 UTC (permalink / raw) To: git@vger.kernel.org May I suggest that " The most approaching commands" doesn't make much sense as English (I don't think a command can "approach"). Perhaps it should be " The most appropriate commands". Regards, Richard. Richard Kerry BNCS Engineer, SI SOL Telco & Media Vertical Practice T: +44 (0)20 3618 2669 M: +44 (0)7812 325518 Lync: +44 (0) 20 3618 0778 Room G300, Stadium House, Wood Lane, London, W12 7TA richard.kerry@atos.net -----Original Message----- From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On Behalf Of Jean-Noel Avila Sent: Wednesday, May 03, 2017 10:07 PM To: git@vger.kernel.org Cc: rashmipai36@gmail.com; Jean-Noel Avila <jn.avila@free.fr> Subject: [PATCH v2 1/3] usability: don't ask questions if no reply is required There has been a bug report by a corporate user that stated that "spelling mistake of stash followed by a yes prints character 'y' infinite times." This analysis was false. When the spelling of a command contains errors, the git program tries to help the user by providing candidates which are close to the unexisting command. E.g Git prints the following: git: 'stahs' is not a git command. See 'git --help'. Did you mean this? stash and then exits. The problem with this hint is that it is not formally indicated as an hint and the user is in fact encouraged to reply to the question, whereas the Git command is already finished. The user was unlucky enough that it was the command he was looking for, and replied "yes" on the command line, effectively launching the `yes` program. The initial error is that the Git programs, when launched in command-line mode (without interaction) must not ask questions, because these questions would normally require a user input as a reply while they won't handle indeed. That's a source of confusion on UX level. To improve the general usability of the Git suite, the following rule was applied: if the sentence * appears in a non-interactive session * is printed last before exit * is a question addressing the user ("you") the sentence is turned into affirmative and proposes the option. The basic rewording of the question sentences has been extended to other spots found in the source. Requested at https://github.com/git/git-scm.com/issues/999 by rpai1 Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> --- builtin/am.c | 4 ++-- builtin/checkout.c | 2 +- help.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) 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); } @@ -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.")); die_user_resolve(state); } 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."), opts.new_branch, argv[0]); if (opts.force_detach) 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)); for (i = 0; i < n; i++) -- 2.12.0 Atos, Atos Consulting, Worldline and Canopy The Open Cloud Company are trading names used by the Atos group. The following trading entities are registered in England and Wales: Atos IT Services UK Limited (registered number 01245534), Atos Consulting Limited (registered number 04312380), Atos Worldline UK Limited (registered number 08514184) and Canopy The Open Cloud Company Limited (registration number 08011902). The registered office for each is at 4 Triton Square, Regent’s Place, London, NW1 3HG.The VAT No. for each is: GB232327983. This e-mail and the documents attached are confidential and intended solely for the addressee, and may contain confidential or privileged information. If you receive this e-mail in error, you are not authorised to copy, disclose, use or retain it. Please notify the sender immediately and delete this email from your systems. As emails may be intercepted, amended or lost, they are not secure. Atos therefore can accept no liability for any errors or their content. Although Atos endeavours to maintain a virus-free network, we do not warrant that this transmission is virus-free and can accept no liability for any damages resulting from any virus transmitted. The risks are deemed to be accepted by everyone who communicates with Atos by email. ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required 2017-05-04 8:52 ` [PATCH v2 1/3] usability: don't ask questions if no reply is required Kerry, Richard @ 2017-05-04 9:09 ` Ævar Arnfjörð Bjarmason 2017-05-04 10:14 ` Kerry, Richard 2017-05-04 9:41 ` Jean-Noël AVILA 1 sibling, 1 reply; 41+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-05-04 9:09 UTC (permalink / raw) To: Kerry, Richard; +Cc: git@vger.kernel.org On Thu, May 4, 2017 at 10:52 AM, Kerry, Richard <richard.kerry@atos.net> wrote: > > May I suggest that " The most approaching commands" doesn't make much sense as English (I don't think a command can "approach"). > Perhaps it should be " The most appropriate commands". I had the same concern, saying "appropriate" is IMO also confusing. The point of this UI is not to point out what you should be running, which "appropriate" implies, but just "we couldn't find what you meant, did you mean one of these?". I think nothing needs to change here. The whole premise here is that a program should never ask a question when you can't give an answer, I think that's nonsense. There's such a thing as a rhetorical question, and sometimes using that form is the most obvious & succinct way to put things. Which is not to say that phrasing these things as a non-question can't be better, but the suggestions so far just seem more complex. Also keep in mind that a huge part of the user base for git using the English UI consists of non-native speakers, and when in doubt we should definitely be picking simpler English like "did you mean?" v.s. alternatives with >10 character more obscure words. > Richard Kerry > BNCS Engineer, SI SOL Telco & Media Vertical Practice > > T: +44 (0)20 3618 2669 > M: +44 (0)7812 325518 > Lync: +44 (0) 20 3618 0778 > Room G300, Stadium House, Wood Lane, London, W12 7TA > richard.kerry@atos.net > > > > > -----Original Message----- > From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On Behalf Of Jean-Noel Avila > Sent: Wednesday, May 03, 2017 10:07 PM > To: git@vger.kernel.org > Cc: rashmipai36@gmail.com; Jean-Noel Avila <jn.avila@free.fr> > Subject: [PATCH v2 1/3] usability: don't ask questions if no reply is required > > There has been a bug report by a corporate user that stated that "spelling mistake of stash followed by a yes prints character 'y' > infinite times." > > This analysis was false. When the spelling of a command contains errors, the git program tries to help the user by providing candidates which are close to the unexisting command. E.g Git prints the > following: > > git: 'stahs' is not a git command. See 'git --help'. > Did you mean this? > > stash > > and then exits. > > The problem with this hint is that it is not formally indicated as an hint and the user is in fact encouraged to reply to the question, whereas the Git command is already finished. > > The user was unlucky enough that it was the command he was looking for, and replied "yes" on the command line, effectively launching the `yes` program. > > The initial error is that the Git programs, when launched in command-line mode (without interaction) must not ask questions, because these questions would normally require a user input as a reply while they won't handle indeed. That's a source of confusion on UX level. > > To improve the general usability of the Git suite, the following rule was applied: > > if the sentence > * appears in a non-interactive session > * is printed last before exit > * is a question addressing the user ("you") > > the sentence is turned into affirmative and proposes the option. > > The basic rewording of the question sentences has been extended to other spots found in the source. > > Requested at https://github.com/git/git-scm.com/issues/999 by rpai1 > > Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> > --- > builtin/am.c | 4 ++-- > builtin/checkout.c | 2 +- > help.c | 4 ++-- > 3 files changed, 5 insertions(+), 5 deletions(-) > > 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); > } > > @@ -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.")); > die_user_resolve(state); > } > > 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."), > opts.new_branch, argv[0]); > > if (opts.force_detach) > 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)); > > for (i = 0; i < n; i++) > -- > 2.12.0 > > Atos, Atos Consulting, Worldline and Canopy The Open Cloud Company are trading names used by the Atos group. The following trading entities are registered in England and Wales: Atos IT Services UK Limited (registered number 01245534), Atos Consulting Limited (registered number 04312380), Atos Worldline UK Limited (registered number 08514184) and Canopy The Open Cloud Company Limited (registration number 08011902). The registered office for each is at 4 Triton Square, Regent’s Place, London, NW1 3HG.The VAT No. for each is: GB232327983. > > This e-mail and the documents attached are confidential and intended solely for the addressee, and may contain confidential or privileged information. If you receive this e-mail in error, you are not authorised to copy, disclose, use or retain it. Please notify the sender immediately and delete this email from your systems. As emails may be intercepted, amended or lost, they are not secure. Atos therefore can accept no liability for any errors or their content. Although Atos endeavours to maintain a virus-free network, we do not warrant that this transmission is virus-free and can accept no liability for any damages resulting from any virus transmitted. The risks are deemed to be accepted by everyone who communicates with Atos by email. ^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v2 1/3] usability: don't ask questions if no reply is required 2017-05-04 9:09 ` Ævar Arnfjörð Bjarmason @ 2017-05-04 10:14 ` Kerry, Richard 2017-05-09 8:18 ` Jean-Noël AVILA 0 siblings, 1 reply; 41+ messages in thread From: Kerry, Richard @ 2017-05-04 10:14 UTC (permalink / raw) To: git@vger.kernel.org My point was to ensure that where English is used on-screen it should make sense, which in this particular case it didn't (a French idiom which, on using an automatic translator, didn't make sense in English). The same of course applies to other languages used on-screen. I agree about ensuring that the application doesn't elicit a response that it won't, or can't, actually handle. A rhetorical question is fine, so long as it is clear that the program won't accept any further input. Though I don't agree about the issue of the length of words, as presented to a non-native speaker. Sometimes a longer word can be very specific in its meaning, and can be looked up in a dictionary if the reader is not familiar with it. Sometimes using shorter words can result in a less clear meaning, or perhaps be an idiomatic usage, which might be missed by a non-native speaker. Regards, Richard. Richard Kerry BNCS Engineer, SI SOL Telco & Media Vertical Practice T: +44 (0)20 3618 2669 M: +44 (0)7812 325518 4 Triton Square, Regent’s Place, London NW1 3HG richard.kerry@atos.net This e-mail and the documents attached are confidential and intended solely for the addressee; it may also be privileged. If you receive this e-mail in error, please notify the sender immediately and destroy it. As its integrity cannot be secured on the Internet, the Atos group liability cannot be triggered for the message content. Although the sender endeavours to maintain a computer virus-free network, the sender does not warrant that this transmission is virus-free and will not be liable for any damages resulting from any virus transmitted. ________________________________________ From: Ævar Arnfjörð Bjarmason [avarab@gmail.com] Sent: 04 May 2017 10:09 To: Kerry, Richard Cc: git@vger.kernel.org Subject: Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required On Thu, May 4, 2017 at 10:52 AM, Kerry, Richard <richard.kerry@atos.net> wrote: > > May I suggest that " The most approaching commands" doesn't make much sense as English (I don't think a command can "approach"). > Perhaps it should be " The most appropriate commands". I had the same concern, saying "appropriate" is IMO also confusing. The point of this UI is not to point out what you should be running, which "appropriate" implies, but just "we couldn't find what you meant, did you mean one of these?". I think nothing needs to change here. The whole premise here is that a program should never ask a question when you can't give an answer, I think that's nonsense. There's such a thing as a rhetorical question, and sometimes using that form is the most obvious & succinct way to put things. Which is not to say that phrasing these things as a non-question can't be better, but the suggestions so far just seem more complex. Also keep in mind that a huge part of the user base for git using the English UI consists of non-native speakers, and when in doubt we should definitely be picking simpler English like "did you mean?" v.s. alternatives with >10 character more obscure words. > Richard Kerry > BNCS Engineer, SI SOL Telco & Media Vertical Practice > > T: +44 (0)20 3618 2669 > M: +44 (0)7812 325518 > Lync: +44 (0) 20 3618 0778 > Room G300, Stadium House, Wood Lane, London, W12 7TA > richard.kerry@atos.net > > > > > -----Original Message----- > From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On Behalf Of Jean-Noel Avila > Sent: Wednesday, May 03, 2017 10:07 PM > To: git@vger.kernel.org > Cc: rashmipai36@gmail.com; Jean-Noel Avila <jn.avila@free.fr> > Subject: [PATCH v2 1/3] usability: don't ask questions if no reply is required > > There has been a bug report by a corporate user that stated that "spelling mistake of stash followed by a yes prints character 'y' > infinite times." > > This analysis was false. When the spelling of a command contains errors, the git program tries to help the user by providing candidates which are close to the unexisting command. E.g Git prints the > following: > > git: 'stahs' is not a git command. See 'git --help'. > Did you mean this? > > stash > > and then exits. > > The problem with this hint is that it is not formally indicated as an hint and the user is in fact encouraged to reply to the question, whereas the Git command is already finished. > > The user was unlucky enough that it was the command he was looking for, and replied "yes" on the command line, effectively launching the `yes` program. > > The initial error is that the Git programs, when launched in command-line mode (without interaction) must not ask questions, because these questions would normally require a user input as a reply while they won't handle indeed. That's a source of confusion on UX level. > > To improve the general usability of the Git suite, the following rule was applied: > > if the sentence > * appears in a non-interactive session > * is printed last before exit > * is a question addressing the user ("you") > > the sentence is turned into affirmative and proposes the option. > > The basic rewording of the question sentences has been extended to other spots found in the source. > > Requested at https://github.com/git/git-scm.com/issues/999 by rpai1 > > Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> > --- > builtin/am.c | 4 ++-- > builtin/checkout.c | 2 +- > help.c | 4 ++-- > 3 files changed, 5 insertions(+), 5 deletions(-) > > 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); > } > > @@ -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.")); > die_user_resolve(state); > } > > 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."), > opts.new_branch, argv[0]); > > if (opts.force_detach) > 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)); > > for (i = 0; i < n; i++) > -- > 2.12.0 > > Atos, Atos Consulting, Worldline and Canopy The Open Cloud Company are trading names used by the Atos group. The following trading entities are registered in England and Wales: Atos IT Services UK Limited (registered number 01245534), Atos Consulting Limited (registered number 04312380), Atos Worldline UK Limited (registered number 08514184) and Canopy The Open Cloud Company Limited (registration number 08011902). The registered office for each is at 4 Triton Square, Regent’s Place, London, NW1 3HG.The VAT No. for each is: GB232327983. > > This e-mail and the documents attached are confidential and intended solely for the addressee, and may contain confidential or privileged information. If you receive this e-mail in error, you are not authorised to copy, disclose, use or retain it. Please notify the sender immediately and delete this email from your systems. As emails may be intercepted, amended or lost, they are not secure. Atos therefore can accept no liability for any errors or their content. Although Atos endeavours to maintain a virus-free network, we do not warrant that this transmission is virus-free and can accept no liability for any damages resulting from any virus transmitted. The risks are deemed to be accepted by everyone who communicates with Atos by email. Atos, Atos Consulting, Worldline and Canopy The Open Cloud Company are trading names used by the Atos group. The following trading entities are registered in England and Wales: Atos IT Services UK Limited (registered number 01245534), Atos Consulting Limited (registered number 04312380), Atos Worldline UK Limited (registered number 08514184) and Canopy The Open Cloud Company Limited (registration number 08011902). The registered office for each is at 4 Triton Square, Regent’s Place, London, NW1 3HG.The VAT No. for each is: GB232327983. This e-mail and the documents attached are confidential and intended solely for the addressee, and may contain confidential or privileged information. If you receive this e-mail in error, you are not authorised to copy, disclose, use or retain it. Please notify the sender immediately and delete this email from your systems. As emails may be intercepted, amended or lost, they are not secure. Atos therefore can accept no liability for any errors or their content. Although Atos endeavours to maintain a virus-free network, we do not warrant that this transmission is virus-free and can accept no liability for any damages resulting from any virus transmitted. The risks are deemed to be accepted by everyone who communicates with Atos by email. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required 2017-05-04 10:14 ` Kerry, Richard @ 2017-05-09 8:18 ` Jean-Noël AVILA 2017-05-09 9:21 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 41+ messages in thread From: Jean-Noël AVILA @ 2017-05-09 8:18 UTC (permalink / raw) To: git; +Cc: Kerry, Richard Le jeudi 4 mai 2017, 10:14:58 CEST Kerry, Richard a écrit : > > My point was to ensure that where English is used on-screen it should make sense, which in this particular case it didn't (a French idiom which, on using an automatic translator, didn't make sense in English). The same of course applies to other languages used on-screen. > > I agree about ensuring that the application doesn't elicit a response that it won't, or can't, actually handle. A rhetorical question is fine, so long as it is clear that the program won't accept any further input. > > Though I don't agree about the issue of the length of words, as presented to a non-native speaker. Sometimes a longer word can be very specific in its meaning, and can be looked up in a dictionary if the reader is not familiar with it. Sometimes using shorter words can result in a less clear meaning, or perhaps be an idiomatic usage, which might be missed by a non-native speaker. > Thanks. So what's the status of this patch series? I don't buy the idea of rhetorical HMI. That's a sure way to confuse non-native speakers. Please note that I kept the questions when there is a following text. Only questions addressing the user at the end of output have been rephrased. For the "do you mean" questions, the proposition would then simply be: "the most similar command is:" or "the most similar commands are:". and then what about the other patches? Thanks > Regards, > Richard. > > > > > Richard Kerry > BNCS Engineer, SI SOL Telco & Media Vertical Practice > T: +44 (0)20 3618 2669 > M: +44 (0)7812 325518 > 4 Triton Square, Regent’s Place, London NW1 3HG > richard.kerry@atos.net > > > This e-mail and the documents attached are confidential and intended solely for the addressee; it may also be privileged. If you receive this e-mail in error, please notify the sender immediately and destroy it. As its integrity cannot be secured on the Internet, the Atos group liability cannot be triggered for the message content. Although the sender endeavours to maintain a computer virus-free network, the sender does not warrant that this transmission is virus-free and will not be liable for any damages resulting from any virus transmitted. > > ________________________________________ > From: Ævar Arnfjörð Bjarmason [avarab@gmail.com] > Sent: 04 May 2017 10:09 > To: Kerry, Richard > Cc: git@vger.kernel.org > Subject: Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required > > On Thu, May 4, 2017 at 10:52 AM, Kerry, Richard <richard.kerry@atos.net> wrote: > > > > May I suggest that " The most approaching commands" doesn't make much sense as English (I don't think a command can "approach"). > > Perhaps it should be " The most appropriate commands". > > I had the same concern, saying "appropriate" is IMO also confusing. > The point of this UI is not to point out what you should be running, > which "appropriate" implies, but just "we couldn't find what you > meant, did you mean one of these?". > > I think nothing needs to change here. The whole premise here is that a > program should never ask a question when you can't give an answer, I > think that's nonsense. There's such a thing as a rhetorical question, > and sometimes using that form is the most obvious & succinct way to > put things. > > Which is not to say that phrasing these things as a non-question can't > be better, but the suggestions so far just seem more complex. > > Also keep in mind that a huge part of the user base for git using the > English UI consists of non-native speakers, and when in doubt we > should definitely be picking simpler English like "did you mean?" v.s. > alternatives with >10 character more obscure words. > > > Richard Kerry > > BNCS Engineer, SI SOL Telco & Media Vertical Practice > > > > T: +44 (0)20 3618 2669 > > M: +44 (0)7812 325518 > > Lync: +44 (0) 20 3618 0778 > > Room G300, Stadium House, Wood Lane, London, W12 7TA > > richard.kerry@atos.net > > > > > > > > > > -----Original Message----- > > From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On Behalf Of Jean-Noel Avila > > Sent: Wednesday, May 03, 2017 10:07 PM > > To: git@vger.kernel.org > > Cc: rashmipai36@gmail.com; Jean-Noel Avila <jn.avila@free.fr> > > Subject: [PATCH v2 1/3] usability: don't ask questions if no reply is required > > > > There has been a bug report by a corporate user that stated that "spelling mistake of stash followed by a yes prints character 'y' > > infinite times." > > > > This analysis was false. When the spelling of a command contains errors, the git program tries to help the user by providing candidates which are close to the unexisting command. E.g Git prints the > > following: > > > > git: 'stahs' is not a git command. See 'git --help'. > > Did you mean this? > > > > stash > > > > and then exits. > > > > The problem with this hint is that it is not formally indicated as an hint and the user is in fact encouraged to reply to the question, whereas the Git command is already finished. > > > > The user was unlucky enough that it was the command he was looking for, and replied "yes" on the command line, effectively launching the `yes` program. > > > > The initial error is that the Git programs, when launched in command-line mode (without interaction) must not ask questions, because these questions would normally require a user input as a reply while they won't handle indeed. That's a source of confusion on UX level. > > > > To improve the general usability of the Git suite, the following rule was applied: > > > > if the sentence > > * appears in a non-interactive session > > * is printed last before exit > > * is a question addressing the user ("you") > > > > the sentence is turned into affirmative and proposes the option. > > > > The basic rewording of the question sentences has been extended to other spots found in the source. > > > > Requested at https://github.com/git/git-scm.com/issues/999 by rpai1 > > > > Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> > > --- > > builtin/am.c | 4 ++-- > > builtin/checkout.c | 2 +- > > help.c | 4 ++-- > > 3 files changed, 5 insertions(+), 5 deletions(-) > > > > 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); > > } > > > > @@ -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.")); > > die_user_resolve(state); > > } > > > > 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."), > > opts.new_branch, argv[0]); > > > > if (opts.force_detach) > > 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)); > > > > for (i = 0; i < n; i++) > > -- > > 2.12.0 > > > > Atos, Atos Consulting, Worldline and Canopy The Open Cloud Company are trading names used by the Atos group. The following trading entities are registered in England and Wales: Atos IT Services UK Limited (registered number 01245534), Atos Consulting Limited (registered number 04312380), Atos Worldline UK Limited (registered number 08514184) and Canopy The Open Cloud Company Limited (registration number 08011902). The registered office for each is at 4 Triton Square, Regent’s Place, London, NW1 3HG.The VAT No. for each is: GB232327983. > > > > This e-mail and the documents attached are confidential and intended solely for the addressee, and may contain confidential or privileged information. If you receive this e-mail in error, you are not authorised to copy, disclose, use or retain it. Please notify the sender immediately and delete this email from your systems. As emails may be intercepted, amended or lost, they are not secure. Atos therefore can accept no liability for any errors or their content. Although Atos endeavours to maintain a virus-free network, we do not warrant that this transmission is virus-free and can accept no liability for any damages resulting from any virus transmitted. The risks are deemed to be accepted by everyone who communicates with Atos by email. > Atos, Atos Consulting, Worldline and Canopy The Open Cloud Company are trading names used by the Atos group. The following trading entities are registered in England and Wales: Atos IT Services UK Limited (registered number 01245534), Atos Consulting Limited (registered number 04312380), Atos Worldline UK Limited (registered number 08514184) and Canopy The Open Cloud Company Limited (registration number 08011902). The registered office for each is at 4 Triton Square, Regent’s Place, London, NW1 3HG.The VAT No. for each is: GB232327983. > > This e-mail and the documents attached are confidential and intended solely for the addressee, and may contain confidential or privileged information. If you receive this e-mail in error, you are not authorised to copy, disclose, use or retain it. Please notify the sender immediately and delete this email from your systems. As emails may be intercepted, amended or lost, they are not secure. Atos therefore can accept no liability for any errors or their content. Although Atos endeavours to maintain a virus-free network, we do not warrant that this transmission is virus-free and can accept no liability for any damages resulting from any virus transmitted. The risks are deemed to be accepted by everyone who communicates with Atos by email. > ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required 2017-05-09 8:18 ` Jean-Noël AVILA @ 2017-05-09 9:21 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 41+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-05-09 9:21 UTC (permalink / raw) To: Jean-Noël AVILA; +Cc: Git Mailing List, Kerry, Richard On Tue, May 9, 2017 at 10:18 AM, Jean-Noël AVILA <jn.avila@free.fr> wrote: > Le jeudi 4 mai 2017, 10:14:58 CEST Kerry, Richard a écrit : >> >> My point was to ensure that where English is used on-screen it should make sense, which in this particular case it didn't (a French idiom which, on using an automatic translator, didn't make sense in English). The same of course applies to other languages used on-screen. >> >> I agree about ensuring that the application doesn't elicit a response that it won't, or can't, actually handle. A rhetorical question is fine, so long as it is clear that the program won't accept any further input. >> >> Though I don't agree about the issue of the length of words, as presented to a non-native speaker. Sometimes a longer word can be very specific in its meaning, and can be looked up in a dictionary if the reader is not familiar with it. Sometimes using shorter words can result in a less clear meaning, or perhaps be an idiomatic usage, which might be missed by a non-native speaker. >> > > Thanks. So what's the status of this patch series? I don't buy the idea of rhetorical HMI. That's a sure way to confuse non-native speakers. Please note that I kept the questions when there is a following text. Only questions addressing the user at the end of output have been rephrased. > > For the "do you mean" questions, the proposition would then simply be: "the most similar command is:" or "the most similar commands are:". > > and then what about the other patches? When you submit patches you can monitor the next "What's cooking" mail for the status. See "ja/do-not..." here: https://public-inbox.org/git/xmqqlgq77pse.fsf@gitster.mtv.corp.google.com/ It got picked up for the "pu" branch. You can fetch git.git and see it there. My feedback on the 3: * 1/3: Mostly covered above. I did notice after my last comment that every time gcc wants to suggest you should do something different (e.g. misspelled variable or macro) it'll say "did you mean?" similar to what git does now. While I think this is a rather tragic story of *nix usability ("user gets asked a question, types yes, gets a few GB/s of y as output") the main UX problem is surely that the user in question didn't understand from the terminal output when the program had exited & wasn't interactive anymore. But overall this seems like optimizing for a really obscure edge case at the expense of making the wording more clever. I don't think "did you mean?" will confuse non-native speakers, as the bug report shows the user in question has a reasonable command of English, they're fundimentally confused about how the shell interface works. * 2/3: Looks great, surprised it took so long for someone to remove that cutsey but bad message. * 3/3: I think this partly makes things slightly worse. I.e. now you get a specific error message about refs being missing, after it shows you the entire usage info, so you don't know if you e.g. misspelled a command-line flag or what. I couldn't find any pattern in the existing shell scripts for "print usage with custom message" thoug. >> Regards, >> Richard. >> >> >> >> >> Richard Kerry >> BNCS Engineer, SI SOL Telco & Media Vertical Practice >> T: +44 (0)20 3618 2669 >> M: +44 (0)7812 325518 >> 4 Triton Square, Regent’s Place, London NW1 3HG >> richard.kerry@atos.net >> >> >> This e-mail and the documents attached are confidential and intended solely for the addressee; it may also be privileged. If you receive this e-mail in error, please notify the sender immediately and destroy it. As its integrity cannot be secured on the Internet, the Atos group liability cannot be triggered for the message content. Although the sender endeavours to maintain a computer virus-free network, the sender does not warrant that this transmission is virus-free and will not be liable for any damages resulting from any virus transmitted. >> >> ________________________________________ >> From: Ævar Arnfjörð Bjarmason [avarab@gmail.com] >> Sent: 04 May 2017 10:09 >> To: Kerry, Richard >> Cc: git@vger.kernel.org >> Subject: Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required >> >> On Thu, May 4, 2017 at 10:52 AM, Kerry, Richard <richard.kerry@atos.net> wrote: >> > >> > May I suggest that " The most approaching commands" doesn't make much sense as English (I don't think a command can "approach"). >> > Perhaps it should be " The most appropriate commands". >> >> I had the same concern, saying "appropriate" is IMO also confusing. >> The point of this UI is not to point out what you should be running, >> which "appropriate" implies, but just "we couldn't find what you >> meant, did you mean one of these?". >> >> I think nothing needs to change here. The whole premise here is that a >> program should never ask a question when you can't give an answer, I >> think that's nonsense. There's such a thing as a rhetorical question, >> and sometimes using that form is the most obvious & succinct way to >> put things. >> >> Which is not to say that phrasing these things as a non-question can't >> be better, but the suggestions so far just seem more complex. >> >> Also keep in mind that a huge part of the user base for git using the >> English UI consists of non-native speakers, and when in doubt we >> should definitely be picking simpler English like "did you mean?" v.s. >> alternatives with >10 character more obscure words. >> >> > Richard Kerry >> > BNCS Engineer, SI SOL Telco & Media Vertical Practice >> > >> > T: +44 (0)20 3618 2669 >> > M: +44 (0)7812 325518 >> > Lync: +44 (0) 20 3618 0778 >> > Room G300, Stadium House, Wood Lane, London, W12 7TA >> > richard.kerry@atos.net >> > >> > >> > >> > >> > -----Original Message----- >> > From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On Behalf Of Jean-Noel Avila >> > Sent: Wednesday, May 03, 2017 10:07 PM >> > To: git@vger.kernel.org >> > Cc: rashmipai36@gmail.com; Jean-Noel Avila <jn.avila@free.fr> >> > Subject: [PATCH v2 1/3] usability: don't ask questions if no reply is required >> > >> > There has been a bug report by a corporate user that stated that "spelling mistake of stash followed by a yes prints character 'y' >> > infinite times." >> > >> > This analysis was false. When the spelling of a command contains errors, the git program tries to help the user by providing candidates which are close to the unexisting command. E.g Git prints the >> > following: >> > >> > git: 'stahs' is not a git command. See 'git --help'. >> > Did you mean this? >> > >> > stash >> > >> > and then exits. >> > >> > The problem with this hint is that it is not formally indicated as an hint and the user is in fact encouraged to reply to the question, whereas the Git command is already finished. >> > >> > The user was unlucky enough that it was the command he was looking for, and replied "yes" on the command line, effectively launching the `yes` program. >> > >> > The initial error is that the Git programs, when launched in command-line mode (without interaction) must not ask questions, because these questions would normally require a user input as a reply while they won't handle indeed. That's a source of confusion on UX level. >> > >> > To improve the general usability of the Git suite, the following rule was applied: >> > >> > if the sentence >> > * appears in a non-interactive session >> > * is printed last before exit >> > * is a question addressing the user ("you") >> > >> > the sentence is turned into affirmative and proposes the option. >> > >> > The basic rewording of the question sentences has been extended to other spots found in the source. >> > >> > Requested at https://github.com/git/git-scm.com/issues/999 by rpai1 >> > >> > Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> >> > --- >> > builtin/am.c | 4 ++-- >> > builtin/checkout.c | 2 +- >> > help.c | 4 ++-- >> > 3 files changed, 5 insertions(+), 5 deletions(-) >> > >> > 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); >> > } >> > >> > @@ -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.")); >> > die_user_resolve(state); >> > } >> > >> > 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."), >> > opts.new_branch, argv[0]); >> > >> > if (opts.force_detach) >> > 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)); >> > >> > for (i = 0; i < n; i++) >> > -- >> > 2.12.0 >> > >> > Atos, Atos Consulting, Worldline and Canopy The Open Cloud Company are trading names used by the Atos group. The following trading entities are registered in England and Wales: Atos IT Services UK Limited (registered number 01245534), Atos Consulting Limited (registered number 04312380), Atos Worldline UK Limited (registered number 08514184) and Canopy The Open Cloud Company Limited (registration number 08011902). The registered office for each is at 4 Triton Square, Regent’s Place, London, NW1 3HG.The VAT No. for each is: GB232327983. >> > >> > This e-mail and the documents attached are confidential and intended solely for the addressee, and may contain confidential or privileged information. If you receive this e-mail in error, you are not authorised to copy, disclose, use or retain it. Please notify the sender immediately and delete this email from your systems. As emails may be intercepted, amended or lost, they are not secure. Atos therefore can accept no liability for any errors or their content. Although Atos endeavours to maintain a virus-free network, we do not warrant that this transmission is virus-free and can accept no liability for any damages resulting from any virus transmitted. The risks are deemed to be accepted by everyone who communicates with Atos by email. >> Atos, Atos Consulting, Worldline and Canopy The Open Cloud Company are trading names used by the Atos group. The following trading entities are registered in England and Wales: Atos IT Services UK Limited (registered number 01245534), Atos Consulting Limited (registered number 04312380), Atos Worldline UK Limited (registered number 08514184) and Canopy The Open Cloud Company Limited (registration number 08011902). The registered office for each is at 4 Triton Square, Regent’s Place, London, NW1 3HG.The VAT No. for each is: GB232327983. >> >> This e-mail and the documents attached are confidential and intended solely for the addressee, and may contain confidential or privileged information. If you receive this e-mail in error, you are not authorised to copy, disclose, use or retain it. Please notify the sender immediately and delete this email from your systems. As emails may be intercepted, amended or lost, they are not secure. Atos therefore can accept no liability for any errors or their content. Although Atos endeavours to maintain a virus-free network, we do not warrant that this transmission is virus-free and can accept no liability for any damages resulting from any virus transmitted. The risks are deemed to be accepted by everyone who communicates with Atos by email. >> > > ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required 2017-05-04 8:52 ` [PATCH v2 1/3] usability: don't ask questions if no reply is required Kerry, Richard 2017-05-04 9:09 ` Ævar Arnfjörð Bjarmason @ 2017-05-04 9:41 ` Jean-Noël AVILA 1 sibling, 0 replies; 41+ messages in thread From: Jean-Noël AVILA @ 2017-05-04 9:41 UTC (permalink / raw) To: git; +Cc: Kerry, Richard Le jeudi 4 mai 2017, 08:52:43 CEST Kerry, Richard a écrit : > > May I suggest that " The most approaching commands" doesn't make much sense as English (I don't think a command can "approach"). > Perhaps it should be " The most appropriate commands". > > > Regards, > Richard. > Thank you for your proposition. "approaching" is a frenchism doubled with google translate (sorry!). Maybe "similar" would also work. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required 2017-05-03 21:07 ` [PATCH v2 1/3] " Jean-Noel Avila ` (2 preceding siblings ...) 2017-05-04 8:52 ` [PATCH v2 1/3] usability: don't ask questions if no reply is required Kerry, Richard @ 2017-05-11 3:16 ` Junio C Hamano 2017-05-11 10:10 ` Kerry, Richard 3 siblings, 1 reply; 41+ messages in thread From: Junio C Hamano @ 2017-05-11 3:16 UTC (permalink / raw) To: Jean-Noel Avila; +Cc: git, rashmipai36 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. ^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v2 1/3] usability: don't ask questions if no reply is required 2017-05-11 3:16 ` Junio C Hamano @ 2017-05-11 10:10 ` Kerry, Richard 2017-05-11 10:28 ` Konstantin Khomoutov 0 siblings, 1 reply; 41+ messages in thread From: Kerry, Richard @ 2017-05-11 10:10 UTC (permalink / raw) To: git@vger.kernel.org Some more grammar/usage notes ..... > -----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. s/split wrong/split wrongly/ Though the further discussion suggests that part of the phrase might best be removed entirely. > > @@ -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. "You might want to use 'git add' on them." It isn't about what you *want* to use, it's what you *need* to use, isn't it? And I'm not happy about "on them". I'm not sure quite why, but the phrasing seems odd. How about "You might need to use 'git add'.", or "You might need to use 'git add' first.", or "'git add' needs to be used to add files." , or "'git add' needs to be used before any other git command may be used.". > > 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); "'%s' can not be resolved as commit, but it should." It should what ? Be resolved, or commit? Or something else? The further comments above suggest that it might even be just "'%s' can not be resolved." > > 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. Richard Kerry BNCS Engineer, SI SOL Telco & Media Vertical Practice T: +44 (0)20 3618 2669 M: +44 (0)7812 325518 Lync: +44 (0) 20 3618 0778 Room G300, Stadium House, Wood Lane, London, W12 7TA richard.kerry@atos.net Atos, Atos Consulting, Worldline and Canopy The Open Cloud Company are trading names used by the Atos group. The following trading entities are registered in England and Wales: Atos IT Services UK Limited (registered number 01245534), Atos Consulting Limited (registered number 04312380), Atos Worldline UK Limited (registered number 08514184) and Canopy The Open Cloud Company Limited (registration number 08011902). The registered office for each is at 4 Triton Square, Regent’s Place, London, NW1 3HG.The VAT No. for each is: GB232327983. This e-mail and the documents attached are confidential and intended solely for the addressee, and may contain confidential or privileged information. If you receive this e-mail in error, you are not authorised to copy, disclose, use or retain it. Please notify the sender immediately and delete this email from your systems. As emails may be intercepted, amended or lost, they are not secure. Atos therefore can accept no liability for any errors or their content. Although Atos endeavours to maintain a virus-free network, we do not warrant that this transmission is virus-free and can accept no liability for any damages resulting from any virus transmitted. The risks are deemed to be accepted by everyone who communicates with Atos by email. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required 2017-05-11 10:10 ` Kerry, Richard @ 2017-05-11 10:28 ` Konstantin Khomoutov 2017-05-11 10:33 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 41+ messages in thread From: Konstantin Khomoutov @ 2017-05-11 10:28 UTC (permalink / raw) To: Kerry, Richard; +Cc: git@vger.kernel.org On Thu, May 11, 2017 at 10:10:05AM +0000, Kerry, Richard wrote: [...] > > > @@ -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. > > "You might want to use 'git add' on them." It isn't about what you > *want* to use, it's what you *need* to use, isn't it? And I'm not > happy about "on them". I'm not sure quite why, but the phrasing seems > odd. How about "You might need to use 'git add'.", or "You might need > to use 'git add' first.", or "'git add' needs to be used to add > files." , or "'git add' needs to be used before any other git command > may be used.". Why not just You should run `git add` on each file with resolved conflicts to mark them as such. I'm not an English speaker but IMHO this phrasing concentrates on the essense of the problem. It's far from being succint, unfortunately. I also wonder what to do with "deleted by them" state of certain files which are also "unmerged" but `git add`-ing them would be a wrong thing to do if we want to accept the upstream's decision to delete the file. So maybe something like You might run `git rm` on a file to accept "deleted by them" for it. appended to the original hint would be good. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required 2017-05-11 10:28 ` Konstantin Khomoutov @ 2017-05-11 10:33 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 41+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-05-11 10:33 UTC (permalink / raw) To: Konstantin Khomoutov; +Cc: Kerry, Richard, git@vger.kernel.org On Thu, May 11, 2017 at 12:28 PM, Konstantin Khomoutov <kostix+git@007spb.ru> wrote: > On Thu, May 11, 2017 at 10:10:05AM +0000, Kerry, Richard wrote: > > [...] >> > > @@ -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. >> >> "You might want to use 'git add' on them." It isn't about what you >> *want* to use, it's what you *need* to use, isn't it? And I'm not >> happy about "on them". I'm not sure quite why, but the phrasing seems >> odd. How about "You might need to use 'git add'.", or "You might need >> to use 'git add' first.", or "'git add' needs to be used to add >> files." , or "'git add' needs to be used before any other git command >> may be used.". > > Why not just > > You should run `git add` on each file with resolved conflicts to mark > them as such. > > I'm not an English speaker but IMHO this phrasing concentrates on the > essense of the problem. It's far from being succint, unfortunately. > > I also wonder what to do with "deleted by them" state of certain files > which are also "unmerged" but `git add`-ing them would be a wrong thing > to do if we want to accept the upstream's decision to delete the file. > So maybe something like > > You might run `git rm` on a file to accept "deleted by them" for it. > > appended to the original hint would be good. I think something like this sounds much better. I think being a bit more verbose is good here, if you know how to solve conflicts you just go "oops, forgot", but for confused users who don't know how, it's better to explain things a bit more verbosely. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v3 1/3] usability: don't ask questions if no reply is required 2017-05-03 16:29 [PATCH 1/4] usability: don't ask questions if no reply is required Jean-Noel Avila ` (4 preceding siblings ...) 2017-05-03 21:07 ` [PATCH v2 1/3] " Jean-Noel Avila @ 2017-05-11 12:06 ` Jean-Noel Avila 2017-05-11 12:06 ` [PATCH v3 2/3] read-tree -m: make error message for merging 0 trees less smart aleck Jean-Noel Avila 2017-05-11 12:06 ` [PATCH v3 3/3] git-filter-branch: Jean-Noel Avila 2017-05-12 13:03 ` [PATCH v4 1/3] usability: don't ask questions if no reply is required Jean-Noel Avila 6 siblings, 2 replies; 41+ messages in thread From: Jean-Noel Avila @ 2017-05-11 12:06 UTC (permalink / raw) To: git; +Cc: Jean-Noel Avila There has been a bug report by a corporate user that stated that "spelling mistake of stash followed by a yes prints character 'y' infinite times." This analysis was false. When the spelling of a command contains errors, the git program tries to help the user by providing candidates which are close to the unexisting command. E.g Git prints the following: git: 'stahs' is not a git command. See 'git --help'. Did you mean this? stash and then exits. The problem with this hint is that it is not formally indicated as an hint and the user is in fact encouraged to reply to the question, whereas the Git command is already finished. The user was unlucky enough that it was the command he was looking for, and replied "yes" on the command line, effectively launching the `yes` program. The initial error is that the Git programs, when launched in command-line mode (without interaction) must not ask questions, because these questions would normally require a user input as a reply that they won't handle indeed. That's a source of confusion on UX level. To improve the general usability of the Git suite, the following rule was applied: if the sentence * appears in a non-interactive session * is printed last before exit * is a question addressing the user ("you") the sentence is turned into affirmative and proposes the option. The basic rewording of the question sentences has been extended to other spots found in the source. Requested at https://github.com/git/git-scm.com/issues/999 by rpai1 Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> --- builtin/am.c | 5 +++-- builtin/checkout.c | 5 ++--- help.c | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index a95dd8b4e..dd60fad1e 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.")); die_user_resolve(state); } @@ -1940,7 +1940,8 @@ 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 should 'git add' each file with resolved conflicts to mark them as such.\n" + "You might run `git rm` on a file to accept \"deleted by them\" for it.")); die_user_resolve(state); } diff --git a/builtin/checkout.c b/builtin/checkout.c index bfa5419f3..85c04d252 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1286,9 +1286,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) * new_branch && argc > 1 will be caught later. */ 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?"), - opts.new_branch, argv[0]); + die(_("'%s' is not a commit and a branch '%s' cannot be created from it"), + argv[0], opts.new_branch); if (opts.force_detach) die(_("git checkout: --detach does not take a path argument '%s'"), diff --git a/help.c b/help.c index bc6cd19cf..a07f01e6f 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 similar command is", + "\nThe most similar commands are", n)); for (i = 0; i < n; i++) -- 2.13.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 2/3] read-tree -m: make error message for merging 0 trees less smart aleck 2017-05-11 12:06 ` [PATCH v3 " Jean-Noel Avila @ 2017-05-11 12:06 ` Jean-Noel Avila 2017-05-11 19:08 ` Jonathan Nieder 2017-05-11 12:06 ` [PATCH v3 3/3] git-filter-branch: Jean-Noel Avila 1 sibling, 1 reply; 41+ messages in thread From: Jean-Noel Avila @ 2017-05-11 12:06 UTC (permalink / raw) To: git; +Cc: Jean-Noel Avila, Jonathan Nieder "git read-tree -m" requires a tree argument to name the tree to be merged in. Git uses a cutesy error message to say so and why: $ git read-tree -m warning: read-tree: emptying the index with no arguments is deprecated; use --empty fatal: just how do you expect me to merge 0 trees? $ git read-tree -m --empty fatal: just how do you expect me to merge 0 trees? When lucky, that could produce an ah-hah moment for the user, but it's more likely to irritate and distract them. Instead, tell the user plainly that the tree argument is required. Also document this requirement in the git-read-tree(1) manpage where there is room to explain it in a more straightforward way. Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Documentation/git-read-tree.txt | 8 ++++---- builtin/read-tree.c | 7 ++++--- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt index ed9d63ef4..97df00043 100644 --- a/Documentation/git-read-tree.txt +++ b/Documentation/git-read-tree.txt @@ -135,10 +135,10 @@ OPTIONS Merging ------- -If `-m` is specified, 'git read-tree' can perform 3 kinds of -merge, a single tree merge if only 1 tree is given, a -fast-forward merge with 2 trees, or a 3-way merge if 3 trees are -provided. +If `-m` is specified, at least one tree must be given on the command +line. 'git read-tree' can perform 3 kinds of merge, a single tree +merge if only 1 tree is given, a fast-forward merge with 2 trees, or a +3-way merge if 3 or more trees are provided. Single Tree Merge diff --git a/builtin/read-tree.c b/builtin/read-tree.c index 23e212ee8..de1a58d17 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -132,7 +132,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) OPT_BOOL(0, "empty", &read_empty, N_("only empty the index")), OPT__VERBOSE(&opts.verbose_update, N_("be verbose")), - OPT_GROUP(N_("Merging")), + OPT_GROUP(N_("Merging (needs at least one tree-ish")), OPT_BOOL('m', NULL, &opts.merge, N_("perform a merge in addition to a read")), OPT_BOOL(0, "trivial", &opts.trivial_merges_only, @@ -226,9 +226,10 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) setup_work_tree(); if (opts.merge) { - if (stage < 2) - die("just how do you expect me to merge %d trees?", stage-1); switch (stage - 1) { + case 0: + die(_("you must specify at least one tree to merge")); + break; case 1: opts.fn = opts.prefix ? bind_merge : oneway_merge; break; -- 2.13.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/3] read-tree -m: make error message for merging 0 trees less smart aleck 2017-05-11 12:06 ` [PATCH v3 2/3] read-tree -m: make error message for merging 0 trees less smart aleck Jean-Noel Avila @ 2017-05-11 19:08 ` Jonathan Nieder 2017-05-12 6:28 ` Junio C Hamano 0 siblings, 1 reply; 41+ messages in thread From: Jonathan Nieder @ 2017-05-11 19:08 UTC (permalink / raw) To: Jean-Noel Avila; +Cc: git Hi, Jean-Noel Avila wrote: > Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> > Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> Please remove my sign-off. I didn't write or carry this patch. If you want to acknowledge my contribution, you can use something like Helped-by, but it's not necessary. [...] > +++ b/Documentation/git-read-tree.txt > @@ -135,10 +135,10 @@ OPTIONS > > Merging > ------- > -If `-m` is specified, 'git read-tree' can perform 3 kinds of > -merge, a single tree merge if only 1 tree is given, a > -fast-forward merge with 2 trees, or a 3-way merge if 3 trees are > -provided. > +If `-m` is specified, at least one tree must be given on the command > +line. As I mentioned before, this sentence feels redundant and doesn't fix the real problem of the `-m` reference elsewhere in this file not pointing to this section. [...] > --- a/builtin/read-tree.c > +++ b/builtin/read-tree.c > @@ -132,7 +132,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) > OPT_BOOL(0, "empty", &read_empty, > N_("only empty the index")), > OPT__VERBOSE(&opts.verbose_update, N_("be verbose")), > - OPT_GROUP(N_("Merging")), > + OPT_GROUP(N_("Merging (needs at least one tree-ish")), This also seems a little too much of a special detail to put in the prominent section title. If you run "git read-tree -h", where would you expect to find this information? The "git read-tree -h" output turns out to not be useful for much more than a reminder of supported options --- it doesn't give a useful overview of the usage, since the usage string at the start is very long. That's unfortunate but it seems outside the scope of this patch. Probably the simplest thing is to drop this hunk from the patch. [...] > @@ -226,9 +226,10 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) > setup_work_tree(); > > if (opts.merge) { > - if (stage < 2) > - die("just how do you expect me to merge %d trees?", stage-1); > switch (stage - 1) { > + case 0: > + die(_("you must specify at least one tree to merge")); > + break; This part looks good. Thanks for your patient work. Jonathan ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/3] read-tree -m: make error message for merging 0 trees less smart aleck 2017-05-11 19:08 ` Jonathan Nieder @ 2017-05-12 6:28 ` Junio C Hamano 2017-05-12 12:54 ` Jean-Noël AVILA 0 siblings, 1 reply; 41+ messages in thread From: Junio C Hamano @ 2017-05-12 6:28 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Jean-Noel Avila, git Jonathan Nieder <jrnieder@gmail.com> writes: >> @@ -226,9 +226,10 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) >> setup_work_tree(); >> >> if (opts.merge) { >> - if (stage < 2) >> - die("just how do you expect me to merge %d trees?", stage-1); >> switch (stage - 1) { >> + case 0: >> + die(_("you must specify at least one tree to merge")); >> + break; > > This part looks good. Thanks. Modulo _(""); I do not think other messages from read-tree are marked for i18n (yet). ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/3] read-tree -m: make error message for merging 0 trees less smart aleck 2017-05-12 6:28 ` Junio C Hamano @ 2017-05-12 12:54 ` Jean-Noël AVILA 0 siblings, 0 replies; 41+ messages in thread From: Jean-Noël AVILA @ 2017-05-12 12:54 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jonathan Nieder On Friday, 12 May 2017, 15:28:53 CEST Junio C Hamano wrote: > Jonathan Nieder <jrnieder@gmail.com> writes: > > >> @@ -226,9 +226,10 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) > >> setup_work_tree(); > >> > >> if (opts.merge) { > >> - if (stage < 2) > >> - die("just how do you expect me to merge %d trees?", stage-1); > >> switch (stage - 1) { > >> + case 0: > >> + die(_("you must specify at least one tree to merge")); > >> + break; > > > > This part looks good. > > Thanks. Modulo _(""); I do not think other messages from read-tree > are marked for i18n (yet). > The documentation is already i18n, but not the dying messages. This can take place in a specific patch series. Thanks. Will reroll. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v3 3/3] git-filter-branch: 2017-05-11 12:06 ` [PATCH v3 " Jean-Noel Avila 2017-05-11 12:06 ` [PATCH v3 2/3] read-tree -m: make error message for merging 0 trees less smart aleck Jean-Noel Avila @ 2017-05-11 12:06 ` Jean-Noel Avila 2017-05-12 6:27 ` Junio C Hamano 1 sibling, 1 reply; 41+ messages in thread From: Jean-Noel Avila @ 2017-05-11 12:06 UTC (permalink / raw) To: git; +Cc: Jean-Noel Avila git-filter-branch requires the specification of a branch by one way or another. If no branch appears to have been specified, we know the user got the usage wrong but we don't know what they were trying to do --- e.g. maybe they specified the ref to rewrite but in the wrong place. In this case, just state that the branch specification is missing. Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> --- git-filter-branch.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-filter-branch.sh b/git-filter-branch.sh index 2b8cdba15..aafaf708d 100755 --- a/git-filter-branch.sh +++ b/git-filter-branch.sh @@ -239,7 +239,7 @@ git rev-parse --no-flags --revs-only --symbolic-full-name \ sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads test -s "$tempdir"/heads || - die "Which ref do you want to rewrite?" + die "You must specify a ref to rewrite." GIT_INDEX_FILE="$(pwd)/../index" export GIT_INDEX_FILE -- 2.13.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v3 3/3] git-filter-branch: 2017-05-11 12:06 ` [PATCH v3 3/3] git-filter-branch: Jean-Noel Avila @ 2017-05-12 6:27 ` Junio C Hamano 0 siblings, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2017-05-12 6:27 UTC (permalink / raw) To: Jean-Noel Avila; +Cc: git Jean-Noel Avila <jn.avila@free.fr> writes: > Subject: Re: [PATCH v3 3/3] git-filter-branch: Forgot the body of the single-liner summary? In the meantime I'll queue this as: Subject: git-filter-branch: be more direct in an error message > test -s "$tempdir"/heads || > - die "Which ref do you want to rewrite?" > + die "You must specify a ref to rewrite." Sounds OK, even though this (both the old and the new phrasing) makes me wonder if the program can rewrite only one ref, or it can accept more than one. Thanks. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v4 1/3] usability: don't ask questions if no reply is required 2017-05-03 16:29 [PATCH 1/4] usability: don't ask questions if no reply is required Jean-Noel Avila ` (5 preceding siblings ...) 2017-05-11 12:06 ` [PATCH v3 " Jean-Noel Avila @ 2017-05-12 13:03 ` Jean-Noel Avila 2017-05-12 13:03 ` [PATCH v4 2/3] read-tree -m: make error message for merging 0 trees less smart aleck Jean-Noel Avila ` (2 more replies) 6 siblings, 3 replies; 41+ messages in thread From: Jean-Noel Avila @ 2017-05-12 13:03 UTC (permalink / raw) To: git; +Cc: Jean-Noel Avila There has been a bug report by a corporate user that stated that "spelling mistake of stash followed by a yes prints character 'y' infinite times." This analysis was false. When the spelling of a command contains errors, the git program tries to help the user by providing candidates which are close to the unexisting command. E.g Git prints the following: git: 'stahs' is not a git command. See 'git --help'. Did you mean this? stash and then exits. The problem with this hint is that it is not formally indicated as an hint and the user is in fact encouraged to reply to the question, whereas the Git command is already finished. The user was unlucky enough that it was the command he was looking for, and replied "yes" on the command line, effectively launching the `yes` program. The initial error is that the Git programs, when launched in command-line mode (without interaction) must not ask questions, because these questions would normally require a user input as a reply that they won't handle indeed. That's a source of confusion on UX level. To improve the general usability of the Git suite, the following rule was applied: if the sentence * appears in a non-interactive session * is printed last before exit * is a question addressing the user ("you") the sentence is turned into affirmative and proposes the option. The basic rewording of the question sentences has been extended to other spots found in the source. Requested at https://github.com/git/git-scm.com/issues/999 by rpai1 Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> --- builtin/am.c | 5 +++-- builtin/checkout.c | 5 ++--- help.c | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index a95dd8b4e..dd60fad1e 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.")); die_user_resolve(state); } @@ -1940,7 +1940,8 @@ 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 should 'git add' each file with resolved conflicts to mark them as such.\n" + "You might run `git rm` on a file to accept \"deleted by them\" for it.")); die_user_resolve(state); } diff --git a/builtin/checkout.c b/builtin/checkout.c index bfa5419f3..85c04d252 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1286,9 +1286,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) * new_branch && argc > 1 will be caught later. */ 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?"), - opts.new_branch, argv[0]); + die(_("'%s' is not a commit and a branch '%s' cannot be created from it"), + argv[0], opts.new_branch); if (opts.force_detach) die(_("git checkout: --detach does not take a path argument '%s'"), diff --git a/help.c b/help.c index bc6cd19cf..a07f01e6f 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 similar command is", + "\nThe most similar commands are", n)); for (i = 0; i < n; i++) -- 2.13.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v4 2/3] read-tree -m: make error message for merging 0 trees less smart aleck 2017-05-12 13:03 ` [PATCH v4 1/3] usability: don't ask questions if no reply is required Jean-Noel Avila @ 2017-05-12 13:03 ` Jean-Noel Avila 2017-05-12 13:03 ` [PATCH v4 3/3] git-filter-branch: be more direct in an error message Jean-Noel Avila 2017-05-12 22:36 ` [PATCH v4 1/3] usability: don't ask questions if no reply is required Junio C Hamano 2 siblings, 0 replies; 41+ messages in thread From: Jean-Noel Avila @ 2017-05-12 13:03 UTC (permalink / raw) To: git; +Cc: Jean-Noel Avila "git read-tree -m" requires a tree argument to name the tree to be merged in. Git uses a cutesy error message to say so and why: $ git read-tree -m warning: read-tree: emptying the index with no arguments is deprecated; use --empty fatal: just how do you expect me to merge 0 trees? $ git read-tree -m --empty fatal: just how do you expect me to merge 0 trees? When lucky, that could produce an ah-hah moment for the user, but it's more likely to irritate and distract them. Instead, tell the user plainly that the tree argument is required. Also document that more than 3 trees can be merged. Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> --- Documentation/git-read-tree.txt | 7 +++---- builtin/read-tree.c | 5 +++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt index ed9d63ef4..7e20b0c21 100644 --- a/Documentation/git-read-tree.txt +++ b/Documentation/git-read-tree.txt @@ -135,10 +135,9 @@ OPTIONS Merging ------- -If `-m` is specified, 'git read-tree' can perform 3 kinds of -merge, a single tree merge if only 1 tree is given, a -fast-forward merge with 2 trees, or a 3-way merge if 3 trees are -provided. +If `-m` is specified, 'git read-tree' can perform 3 kinds of merge, a +single tree merge if only 1 tree is given, a fast-forward merge with 2 +trees, or a 3-way merge if 3 or more trees are provided. Single Tree Merge diff --git a/builtin/read-tree.c b/builtin/read-tree.c index 23e212ee8..383442567 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -226,9 +226,10 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix) setup_work_tree(); if (opts.merge) { - if (stage < 2) - die("just how do you expect me to merge %d trees?", stage-1); switch (stage - 1) { + case 0: + die("you must specify at least one tree to merge"); + break; case 1: opts.fn = opts.prefix ? bind_merge : oneway_merge; break; -- 2.13.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v4 3/3] git-filter-branch: be more direct in an error message 2017-05-12 13:03 ` [PATCH v4 1/3] usability: don't ask questions if no reply is required Jean-Noel Avila 2017-05-12 13:03 ` [PATCH v4 2/3] read-tree -m: make error message for merging 0 trees less smart aleck Jean-Noel Avila @ 2017-05-12 13:03 ` Jean-Noel Avila 2017-05-12 22:36 ` [PATCH v4 1/3] usability: don't ask questions if no reply is required Junio C Hamano 2 siblings, 0 replies; 41+ messages in thread From: Jean-Noel Avila @ 2017-05-12 13:03 UTC (permalink / raw) To: git; +Cc: Jean-Noel Avila git-filter-branch requires the specification of a branch by one way or another. If no branch appears to have been specified, we know the user got the usage wrong but we don't know what they were trying to do --- e.g. maybe they specified the ref to rewrite but in the wrong place. In this case, just state that the branch specification is missing. Signed-off-by: Jean-Noel Avila <jn.avila@free.fr> --- git-filter-branch.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-filter-branch.sh b/git-filter-branch.sh index 2b8cdba15..aafaf708d 100755 --- a/git-filter-branch.sh +++ b/git-filter-branch.sh @@ -239,7 +239,7 @@ git rev-parse --no-flags --revs-only --symbolic-full-name \ sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads test -s "$tempdir"/heads || - die "Which ref do you want to rewrite?" + die "You must specify a ref to rewrite." GIT_INDEX_FILE="$(pwd)/../index" export GIT_INDEX_FILE -- 2.13.0 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v4 1/3] usability: don't ask questions if no reply is required 2017-05-12 13:03 ` [PATCH v4 1/3] usability: don't ask questions if no reply is required Jean-Noel Avila 2017-05-12 13:03 ` [PATCH v4 2/3] read-tree -m: make error message for merging 0 trees less smart aleck Jean-Noel Avila 2017-05-12 13:03 ` [PATCH v4 3/3] git-filter-branch: be more direct in an error message Jean-Noel Avila @ 2017-05-12 22:36 ` Junio C Hamano 2017-05-13 15:37 ` Johannes Sixt 2 siblings, 1 reply; 41+ messages in thread From: Junio C Hamano @ 2017-05-12 22:36 UTC (permalink / raw) To: Jean-Noel Avila; +Cc: git Thanks, all three patches look good. Will queue. Let's merge them to 'next' soonish and eventually down to 'master' and 'maint'. Thanks. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 1/3] usability: don't ask questions if no reply is required 2017-05-12 22:36 ` [PATCH v4 1/3] usability: don't ask questions if no reply is required Junio C Hamano @ 2017-05-13 15:37 ` Johannes Sixt 2017-05-15 2:18 ` Junio C Hamano 0 siblings, 1 reply; 41+ messages in thread From: Johannes Sixt @ 2017-05-13 15:37 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jean-Noel Avila, git Am 13.05.2017 um 00:36 schrieb Junio C Hamano: > Thanks, all three patches look good. Will queue. > > Let's merge them to 'next' soonish and eventually down to 'master' > and 'maint'. The patches change translated strings. You should probably wait for an update of their translations before you release a maintenance version with these changes. -- Hannes ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 1/3] usability: don't ask questions if no reply is required 2017-05-13 15:37 ` Johannes Sixt @ 2017-05-15 2:18 ` Junio C Hamano 0 siblings, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2017-05-15 2:18 UTC (permalink / raw) To: Johannes Sixt; +Cc: Jean-Noel Avila, git Johannes Sixt <j6t@kdbg.org> writes: > Am 13.05.2017 um 00:36 schrieb Junio C Hamano: >> Thanks, all three patches look good. Will queue. >> >> Let's merge them to 'next' soonish and eventually down to 'master' >> and 'maint'. > > The patches change translated strings. You should probably wait for an > update of their translations before you release a maintenance version > with these changes. Yup, thanks. ^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2017-05-15 2:18 UTC | newest] Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-03 16:29 [PATCH 1/4] usability: don't ask questions if no reply is required Jean-Noel Avila 2017-05-03 16:29 ` [PATCH 2/4] usability: fix am and checkout for nevermind questions Jean-Noel Avila 2017-05-03 16:51 ` Jonathan Nieder 2017-05-03 18:35 ` Jean-Noël AVILA 2017-05-03 16:29 ` [PATCH 3/4] read-tree.c: rework UI when merging no trees Jean-Noel Avila 2017-05-03 17:04 ` Jonathan Nieder 2017-05-03 18:39 ` Jean-Noël AVILA 2017-05-03 16:29 ` [PATCH 4/4] git-filter-branch: be assertative on dying message Jean-Noel Avila 2017-05-03 17:07 ` Jonathan Nieder 2017-05-03 16:47 ` [PATCH 1/4] usability: don't ask questions if no reply is required Jonathan Nieder 2017-05-03 16:58 ` Stefan Beller 2017-05-03 17:37 ` Jean-Noël AVILA 2017-05-03 21:07 ` [PATCH v2 1/3] " Jean-Noel Avila 2017-05-03 21:07 ` [PATCH v2 2/3] read-tree -m: make error message for merging 0 trees less smart aleck Jean-Noel Avila 2017-05-11 3:46 ` Junio C Hamano 2017-05-11 4:31 ` [PATCH] read-tree: "read-tree -m --empty" does not make sense Junio C Hamano 2017-05-03 21:07 ` [PATCH v2 3/3] git-filter-branch: make the error msg when missing branch more open Jean-Noel Avila 2017-05-11 3:53 ` Junio C Hamano 2017-05-04 8:52 ` [PATCH v2 1/3] usability: don't ask questions if no reply is required Kerry, Richard 2017-05-04 9:09 ` Ævar Arnfjörð Bjarmason 2017-05-04 10:14 ` Kerry, Richard 2017-05-09 8:18 ` Jean-Noël AVILA 2017-05-09 9:21 ` Ævar Arnfjörð Bjarmason 2017-05-04 9:41 ` Jean-Noël AVILA 2017-05-11 3:16 ` Junio C Hamano 2017-05-11 10:10 ` Kerry, Richard 2017-05-11 10:28 ` Konstantin Khomoutov 2017-05-11 10:33 ` Ævar Arnfjörð Bjarmason 2017-05-11 12:06 ` [PATCH v3 " Jean-Noel Avila 2017-05-11 12:06 ` [PATCH v3 2/3] read-tree -m: make error message for merging 0 trees less smart aleck Jean-Noel Avila 2017-05-11 19:08 ` Jonathan Nieder 2017-05-12 6:28 ` Junio C Hamano 2017-05-12 12:54 ` Jean-Noël AVILA 2017-05-11 12:06 ` [PATCH v3 3/3] git-filter-branch: Jean-Noel Avila 2017-05-12 6:27 ` Junio C Hamano 2017-05-12 13:03 ` [PATCH v4 1/3] usability: don't ask questions if no reply is required Jean-Noel Avila 2017-05-12 13:03 ` [PATCH v4 2/3] read-tree -m: make error message for merging 0 trees less smart aleck Jean-Noel Avila 2017-05-12 13:03 ` [PATCH v4 3/3] git-filter-branch: be more direct in an error message Jean-Noel Avila 2017-05-12 22:36 ` [PATCH v4 1/3] usability: don't ask questions if no reply is required Junio C Hamano 2017-05-13 15:37 ` Johannes Sixt 2017-05-15 2:18 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).