Re* [PATCH] add-patch: response to unknown command

Commit Message

Junio C Hamano May 21, 2024, 3:52 p.m. UTC

Patrick Steinhardt <[e-mail]> writes:

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

I share your doubt.  If what the user said (e.g. "ues") when they
wanted to say "yes", I find "You said 'u', which I do not understand" 
more confusiong than "You said 'ues', which I do not understand".

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

I personally guess that it is unlikely that folks are taking
advantage of the fact that everything but the first is ignored, and
I cannot think of a reason why folks prefer that behaviour offhand.

If 'q' and 'a' are next to each other on the user's keyboard, there
is a plausible chance that we see 'qa' when the user who wanted to
say 'a' fat-fingered and we ended up doing the 'q' thing instead,
and we may want to prevent such problems from happening.

Instead of ignoring, we _could_ take 'yn' and apply 'y' to the
current question, and then 'n' to the next question without
prompting (or showing prompt and answer together without taking
further answer), and claim that it is a typesaving feature, but
it is dubious users can sensibly choose the answer to a prompt
they haven't seen.

So, I am inclined to be supportive on that "tighten multi-byte
input" idea, but as I said the above is based on a mere "I cannot
think of ... offhand", so we need to see if people have reasonable
use cases to object first.

------- >8 ------------- >8 ------------- >8 ------------- >8 -------
Subject: add-patch: enforce only one-letter response to prompts

In an "git add -p" session, especially when we are not using the
single-char mode, we may see 'qa' as a response to a prompt

  (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?

and then just do the 'q' thing (i.e. quit the session), ignoring
everything other than the first byte.

If 'q' and 'a' are next to each other on the user's keyboard, there
is a plausible chance that we see 'qa' when the user who wanted to
say 'a' fat-fingered and we ended up doing the 'q' thing instead.

As we didn't think of a good reason during the review discussion why
we want to accept excess letters only to ignore them, it appears to
be a safe change to simply reject input that is longer than just one
byte.

Keep the "use only the first byte, downcased" behaviour when we ask
yes/no question, though.  Neither on Qwerty or on Dvorak, 'y' and
'n' are not close to each other.

Signed-off-by: Junio C Hamano <[e-mail]>
---
 add-patch.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Taylor Blau May 21, 2024, 10:27 p.m. UTC | #1

On Tue, May 21, 2024 at 08:52:14AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <[e-mail]> writes:
>
> > 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.
>
> I share your doubt.  If what the user said (e.g. "ues") when they
> wanted to say "yes", I find "You said 'u', which I do not understand"
> more confusiong than "You said 'ues', which I do not understand".

Same here. The below patch provides compelling reasoning and has my:

  Acked-by: Taylor Blau <[e-mail]>

Thanks,
Taylor

Junio C Hamano May 21, 2024, 11:06 p.m. UTC | #2

Taylor Blau <[e-mail]> writes:

>>
>> I share your doubt.  If what the user said (e.g. "ues") when they
>> wanted to say "yes", I find "You said 'u', which I do not understand"
>> more confusiong than "You said 'ues', which I do not understand".
>
> Same here. The below patch provides compelling reasoning and has my:
>
>   Acked-by: Taylor Blau <[e-mail]>

Heh, this breaks '/' command hence t3701.45 as it takes an argument
hence not limited to a single letter.  I wonder how singlekey folks
invoke that feature, though ;-)

Patch

diff --git c/add-patch.c w/add-patch.c
index 2252895c28..7126bc5d70 100644
--- c/add-patch.c
+++ w/add-patch.c
@@ -1227,6 +1227,7 @@  static int prompt_yesno(struct add_p_state *s, const char *prompt)
 		fflush(stdout);
 		if (read_single_character(s) == EOF)
 			return -1;
+		/* do not limit to 1-byte input to allow 'no' etc. */
 		switch (tolower(s->answer.buf[0])) {
 		case 'n': return 0;
 		case 'y': return 1;
@@ -1509,6 +1510,10 @@  static int patch_update_file(struct add_p_state *s,
 
 		if (!s->answer.len)
 			continue;
+		if (1 < s->answer.len) {
+			error(_("only one letter is expected, got '%s'"), s->answer.buf);
+			continue;
+		}
 		ch = tolower(s->answer.buf[0]);
 		if (ch == 'y') {
 			hunk->use = USE_HUNK;