Re: [PATCH v2] add-patch: enforce only one-letter response to prompts

From: Patrick Steinhardt <ps@pks.im>
Date: May 22, 2024, 11:07 UTC
Message-ID: <Zk3R4MuCWOYVz3_B@tanuki>
In-Reply-To: <xmqqh6eqiwgf.fsf@gitster.g>

On Tue, May 21, 2024 at 04:20:16PM -0700, Junio C Hamano wrote:

Show 11 quoted lines

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

I think it's a good idea regardless of the layout. There are tons of layouts out there that are very esoteric (I for one use NEO2, which most nobody has ever heard of), and I'm sure you will find at least one layout where characters are positioned such that you can fat finger things.

Another argument that is independent of fat fingering is that it potentially allows us to expand this feature with multi-byte verbs going forward.

[snip]

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

Just to prove my point: Workman layout has them right next to each other :) What we make of that information is a different question though.

Show 19 quoted lines

> diff --git a/add-patch.c b/add-patch.c
> index 79eda168eb..a6c3367d59 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1228,6 +1228,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;
> @@ -1506,6 +1507,12 @@ static int patch_update_file(struct add_p_state *s,
>  		if (!s->answer.len)
>  			continue;
>  		ch = tolower(s->answer.buf[0]);
> +
> +		/* 'g' takes a hunk number, '/' takes a regexp */
> +		if (1 < s->answer.len && (ch != 'g' && ch != '/')) {

I find this condition a bit unusual and thus hard to read. If it instead said `s->answer.len != 1` then it would be way easier to comprehend.

Also, none of the branches othar than for 'g' and '/' use `s->answer`, so this should be safe. I also very much agree with the general idea of this patch.

Show 6 quoted lines

> +			error(_("only one letter is expected, got '%s'"), s->answer.buf);
> +			continue;
> +		}
>  		if (ch == 'y') {
>  			hunk->use = USE_HUNK;
>  soft_increment:

I assume we also want a test for this new behaviour, right?

Patrick