| Message ID | 1dbe4c61-d75f-45d9-95d2-ac8acae22c56@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add-patch: response to unknown command | expand |
On Tue, May 21, 2024 at 02:37:54AM +0200, Rubén Justo wrote: > In 26998ed2a2 (add-patch: response to unknown command, 2024-04-29) we > introduced an error message that displays the invalid command entered by > the user. > > We process a line received from the user, but we only accept > single-character commands. > > To avoid confusion, include in the error message only the first > character received. I'm a bit on the edge here. Is it really less confusing if we confront the user with a command that they have never even provided in the first place? They implicitly specified the first letter, only, but the user first needs to be aware that we discard everything but the first letter in the first place. Is it even sensible that we don't complain about trailing garbage in the user's answer? Shouldn't we rather fix that and make the accepted answers more strict, such that if the response is longer than a single character we point that out? Patrick
On Tue, May 21, 2024 at 09:03:08AM +0200, Patrick Steinhardt wrote: > On Tue, May 21, 2024 at 02:37:54AM +0200, Rubén Justo wrote: > > In 26998ed2a2 (add-patch: response to unknown command, 2024-04-29) we > > introduced an error message that displays the invalid command entered by > > the user. > > > > We process a line received from the user, but we only accept > > single-character commands. > > > > To avoid confusion, include in the error message only the first > > character received. > > I'm a bit on the edge here. Is it really less confusing if we confront > the user with a command that they have never even provided in the first > place? I think so, by giving the user what we find wrong and implicitly telling them what it is. > Shouldn't we rather fix that and make the accepted > answers more strict, such that if the response is longer than a single > character we point that out? That's reasonable, but maybe we're going to break someone's workflow? At any rate, my main goal is to avoid the '%s' in the message. > > Patrick
diff --git a/add-patch.c b/add-patch.c index 2252895c28..d408a85353 100644 --- a/add-patch.c +++ b/add-patch.c @@ -1692,8 +1692,8 @@ static int patch_update_file(struct add_p_state *s, "%.*s", (int)(eol - p), p); } } else { - err(s, _("Unknown command '%s' (use '?' for help)"), - s->answer.buf); + err(s, _("Unknown command '%c' (use '?' for help)"), + s->answer.buf[0]); } } diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 28a95a775d..6f5d3085af 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -60,7 +60,7 @@ test_expect_success 'warn about add.interactive.useBuiltin' ' test_expect_success 'unknown command' ' test_when_finished "git reset --hard; rm -f command" && - echo W >command && + echo WW >command && git add -N command && git diff command >expect && cat >>expect <<-EOF &&
In 26998ed2a2 (add-patch: response to unknown command, 2024-04-29) we introduced an error message that displays the invalid command entered by the user. We process a line received from the user, but we only accept single-character commands. To avoid confusion, include in the error message only the first character received. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- add-patch.c | 4 ++-- t/t3701-add-interactive.sh | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)