[PATCH] add-patch: response to unknown command

[PATCH] add-patch: response to unknown command

[Rubén Justo]

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

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 &&
-- 
2.45.1.217.gdb529f37a6

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Rubén Justo]

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

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> 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 <gitster@pobox.com>
---
 add-patch.c | 5 +++++
 1 file changed, 5 insertions(+)

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;

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

[Taylor Blau]

On Tue, May 21, 2024 at 08:52:14AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> 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 <me@ttaylorr.com>

Thanks,
Taylor

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> 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 <me@ttaylorr.com>

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 v2] add-patch: enforce only one-letter response to prompts

[Junio C Hamano]

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.

The two exceptions are the 'g' command that takes a hunk number, and
the '/' command that takes a regular expression.  They has to be
accompanied by their operands (this makes me wonder how users who
set the interactive.singlekey configuration feed these operands---it
turns out that we notice there is no operand and give them another
chance to type the operand separately, without using single key
input this time), so we accept a string that is more than one byte
long.

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 <gitster@pobox.com>
---
 * This version fixes the breakage in t3701 where we exercise the
   '/' command.  Further code inspection reveals that 'g' also needs
   to be special cased.

   The previous iteration was <xmqqr0dvb1sh.fsf_-_@gitster.g>.

 add-patch.c | 7 +++++++
 1 file changed, 7 insertions(+)

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 != '/')) {
+			error(_("only one letter is expected, got '%s'"), s->answer.buf);
+			continue;
+		}
 		if (ch == 'y') {
 			hunk->use = USE_HUNK;
 soft_increment:
-- 
2.45.1-216-g4365c6fcf9

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

[Eric Sunshine]

On Tue, May 21, 2024 at 7:20 PM Junio C Hamano <gitster@pobox.com> wrote:
> 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.
>
> The two exceptions are the 'g' command that takes a hunk number, and
> the '/' command that takes a regular expression.  They has to be

s/has/have/

> accompanied by their operands (this makes me wonder how users who
> set the interactive.singlekey configuration feed these operands---it
> turns out that we notice there is no operand and give them another
> chance to type the operand separately, without using single key
> input this time), so we accept a string that is more than one byte
> long.
>
> 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 <gitster@pobox.com>

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> The two exceptions are the 'g' command that takes a hunk number, and
>> the '/' command that takes a regular expression.  They has to be
>
> s/has/have/

Thanks for a typofix.

Input to possibly update this part ...

>> As we didn't think of a good reason during the review discussion why
>> we want to accept excess letters only to ignore them,...

...  of the proposed log message, or convince us that it is not such
a great idea, is also welcome.

Thanks.  Will queue but keep out of 'next' for a few more days.

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

[Dragan Simic]

Hello Junio,

Please see my comments below.

On 2024-05-22 01:20, Junio C Hamano wrote:
> In an "git add -p" session, especially when we are not using the

s/In an/In a/

> single-char mode, we may see 'qa' as a response to a prompt

Perhaps s/single-char/single-character/

> 
>   (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.
> 
> The two exceptions are the 'g' command that takes a hunk number, and
> the '/' command that takes a regular expression.  They has to be
> accompanied by their operands (this makes me wonder how users who
> set the interactive.singlekey configuration feed these operands---it
> turns out that we notice there is no operand and give them another
> chance to type the operand separately, without using single key
> input this time), so we accept a string that is more than one byte
> long.
> 
> 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 <gitster@pobox.com>
> ---
>  * This version fixes the breakage in t3701 where we exercise the
>    '/' command.  Further code inspection reveals that 'g' also needs
>    to be special cased.
> 
>    The previous iteration was <xmqqr0dvb1sh.fsf_-_@gitster.g>.
> 
>  add-patch.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> 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 != '/')) {

To me, "s->answer.len > 1" would be much more readable, and
I was surprised a bit to see the flipped variant.  This made
me curious; would you, please, let me know why do you prefer
this form?

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

The patch is looking good to me, and I find it good that it
improves the strictness of the user input, which should also
improve the overall user experience.

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2642 bytes --]

On Tue, May 21, 2024 at 04:20:16PM -0700, Junio C Hamano wrote:
> 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.

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> Hello Junio,
>
> Please see my comments below.
>
> On 2024-05-22 01:20, Junio C Hamano wrote:
>> In an "git add -p" session, especially when we are not using the
>
> s/In an/In a/

Good eyes.

>
>> single-char mode, we may see 'qa' as a response to a prompt
>
> Perhaps s/single-char/single-character/

I shouldn't have been loose in the language.  Rather, we should say
"single key mode", as the knob to control the feature is the
"interactive.singlekey" variable.

>> +		/* 'g' takes a hunk number, '/' takes a regexp */
>> +		if (1 < s->answer.len && (ch != 'g' && ch != '/')) {
>
> To me, "s->answer.len > 1" would be much more readable, and
> I was surprised a bit to see the flipped variant.  This made
> me curious; would you, please, let me know why do you prefer
> this form?

"textual order should reflect actual order" (read CodingGuidelines).

For more backstory,

    https://lore.kernel.org/git/?q=%22textual+order%22+%22actual+order%22


Thanks.

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> +		/* '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.

We have already eliminated the "it is 0" case, .len cannot be
negative, and the case we really care about is "is it not just
one?", so I agree with you that the inequality comparison with 1 is
easier to grok.

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

Hmph, yeah.  'g' has already been tested (that was what led me to do
the v2), but we probably should do 'qa' or something.

Thanks.

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

[Junio C Hamano]

In a "git add -p" session, especially when we are not using the
single-key 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.

The two exceptions are the 'g' command that takes a hunk number, and
the '/' command that takes a regular expression.  They have to be
accompanied by their operands (this makes me wonder how users who
set the interactive.singlekey configuration feed these operands---it
turns out that we notice there is no operand and give them another
chance to type the operand separately, without using single key
input this time), so we accept a string that is more than one byte
long.

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 <gitster@pobox.com>
---

 The whole range-diff is not worth sharing as the bulk of it show
 the new tests.  The part that shows the changes to the proposed log
 message and the code looks like this:

    @@ Metadata
      ## Commit message ##
         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
    +    In a "git add -p" session, especially when we are not using the
    +    single-key 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,?]?
     
    @@ add-patch.c: static int patch_update_file(struct add_p_state *s,
      			continue;
      		ch = tolower(s->answer.buf[0]);
     +
    -+		/* 'g' takes a hunk number, '/' takes a regexp */
    -+		if (1 < s->answer.len && (ch != 'g' && ch != '/')) {
    ++		/* 'g' takes a hunk number and '/' takes a regexp */
    ++		if (s->answer.len != 1 && (ch != 'g' && ch != '/')) {
     +			error(_("only one letter is expected, got '%s'"), s->answer.buf);
     +			continue;
     +		}
      		if (ch == 'y') {
      			hunk->use = USE_HUNK;
      soft_increment:
     
 add-patch.c                |  7 +++++++
 t/t3701-add-interactive.sh | 38 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 79eda168eb..7242da2c03 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 and '/' takes a regexp */
+		if (s->answer.len != 1 && (ch != 'g' && ch != '/')) {
+			error(_("only one letter is expected, got '%s'"), s->answer.buf);
+			continue;
+		}
 		if (ch == 'y') {
 			hunk->use = USE_HUNK;
 soft_increment:
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 0b5339ac6c..61f5e9eec0 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -144,6 +144,14 @@ test_expect_success 'revert works (commit)' '
 	grep "unchanged *+3/-0 file" output
 '
 
+test_expect_success 'reject multi-key input' '
+	saved=$(git hash-object -w file) &&
+	test_when_finished "git cat-file blob $saved >file" &&
+	echo an extra line >>file &&
+	test_write_lines aa | git add -p >actual 2>error &&
+	test_grep "error: .* got ${SQ}aa${SQ}" error
+'
+
 test_expect_success 'setup expected' '
 	cat >expected <<-\EOF
 	EOF
@@ -511,7 +519,7 @@ test_expect_success 'split hunk setup' '
 	test_write_lines 10 15 20 21 22 23 24 30 40 50 60 >test
 '
 
-test_expect_success 'goto hunk' '
+test_expect_success 'goto hunk 1 with "g 1"' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
 	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? + 1:  -1,2 +1,3          +15
@@ -527,7 +535,20 @@ test_expect_success 'goto hunk' '
 	test_cmp expect actual.trimmed
 '
 
-test_expect_success 'navigate to hunk via regex' '
+test_expect_success 'goto hunk 1 with "g1"' '
+	test_when_finished "git reset" &&
+	tr _ " " >expect <<-EOF &&
+	_10
+	+15
+	_20
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?_
+	EOF
+	test_write_lines s y g1 | git add -p >actual &&
+	tail -n 4 <actual >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
+test_expect_success 'navigate to hunk via regex /pattern' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
 	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? @@ -1,2 +1,3 @@
@@ -541,6 +562,19 @@ test_expect_success 'navigate to hunk via regex' '
 	test_cmp expect actual.trimmed
 '
 
+test_expect_success 'navigate to hunk via regex / pattern' '
+	test_when_finished "git reset" &&
+	tr _ " " >expect <<-EOF &&
+	_10
+	+15
+	_20
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?_
+	EOF
+	test_write_lines s y / 1,2 | git add -p >actual &&
+	tail -n 4 <actual >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
 test_expect_success 'split hunk "add -p (edit)"' '
 	# Split, say Edit and do nothing.  Then:
 	#
-- 
2.45.1-216-g4365c6fcf9

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

[Rubén Justo]

On Wed, May 22, 2024 at 10:14:23AM -0700, Junio C Hamano wrote:

> +		if (s->answer.len != 1 && (ch != 'g' && ch != '/')) {

This "len!=1" introduces a nice dose of sanity in the UI.

> +			error(_("only one letter is expected, got '%s'"), s->answer.buf);

Here, perhaps you want to do:

			err(s, _("Only one letter is expected, got '%s'"), s->answer.buf);

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

[Dragan Simic]

On 2024-05-22 18:23, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>> On 2024-05-22 01:20, Junio C Hamano wrote:
>>> single-char mode, we may see 'qa' as a response to a prompt
>> 
>> Perhaps s/single-char/single-character/
> 
> I shouldn't have been loose in the language.  Rather, we should say
> "single key mode", as the knob to control the feature is the
> "interactive.singlekey" variable.

Yes, "single-key mode" is better; "when interactive.singlekey
is not enabled" may be even a bit better.  Not worth a reroll,
of course.

>>> +		/* 'g' takes a hunk number, '/' takes a regexp */
>>> +		if (1 < s->answer.len && (ch != 'g' && ch != '/')) {
>> 
>> To me, "s->answer.len > 1" would be much more readable, and
>> I was surprised a bit to see the flipped variant.  This made
>> me curious; would you, please, let me know why do you prefer
>> this form?
> 
> "textual order should reflect actual order" (read CodingGuidelines).
> 
> For more backstory,
> 
>     
> https://lore.kernel.org/git/?q=%22textual+order%22+%22actual+order%22

That's exactly what I assumed, but frankly, in this particular case
I really can't force myself, despite trying quite hard, into liking
it.  It's simply strange to me.

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

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> Here, perhaps you want to do:
>
> 			err(s, _("Only one letter is expected, got '%s'"), s->answer.buf);

True.  The end-user errors are reported with err() in the
surrounding code.

I however was hoping that this can be based on v2.44.0, which
predates 9d225b02 (add-patch: do not show UI messages on stderr,
2024-04-29), which makes the testing of it a bit cumbersome.

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

>> For more backstory,
>>     https://lore.kernel.org/git/?q=%22textual+order%22+%22actual+order%22
>
> That's exactly what I assumed, but frankly, in this particular case
> I really can't force myself, despite trying quite hard, into liking
> it.  It's simply strange to me.

You asked me why, and the reason was given to you.  End of story.

I never asked you to like it and you do not have to like it ;-).

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

[Junio C Hamano]

In a "git add -p" session, especially when we are not using the
single-key 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.

The two exceptions are the 'g' command that takes a hunk number, and
the '/' command that takes a regular expression.  They have to be
accompanied by their operands (this makes me wonder how users who
set the interactive.singlekey configuration feed these operands---it
turns out that we notice there is no operand and give them another
chance to type the operand separately, without using single key
input this time), so we accept a string that is more than one byte
long.

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 <gitster@pobox.com>
---
 * Hopefully the final iteration.  The differences are:

   - The end-user facing "here is what is wrong with your input"
     message is given with err() to be consistent with other such
     messages.

   - I gave up basing this on v2.44.0, as it is a new feature that
     does not have to be merged down to older maintenance tracks.
     This is now based on 80dbfac2 (Merge branch
     'rj/add-p-typo-reaction', 2024-05-08), which is before v2.45.1
     but has modern enough t3701 and add-patch.c:err() sends its
     output to the standard output stream.

   - The tests for 'g' and '/' to check both the stuck and the split
     forms have been updated for the more recent prompt that
     includes 'p'.

   - The test for multi-key sequence expects the err() output on the
     standard output stream.

   As an experiment, this message has the range-diff at the end, not
   before the primary part of the patch text.  I think this format
   should be easier to read for reviewers.

 add-patch.c                |  7 +++++++
 t/t3701-add-interactive.sh | 38 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 2252895c28..814de57c4a 100644
--- a/add-patch.c
+++ b/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;
@@ -1510,6 +1511,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 and '/' takes a regexp */
+		if (s->answer.len != 1 && (ch != 'g' && ch != '/')) {
+			err(s, _("Only one letter is expected, got '%s'"), s->answer.buf);
+			continue;
+		}
 		if (ch == 'y') {
 			hunk->use = USE_HUNK;
 soft_increment:
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 28a95a775d..6624a4f7c0 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -160,6 +160,14 @@ test_expect_success 'revert works (commit)' '
 	grep "unchanged *+3/-0 file" output
 '
 
+test_expect_success 'reject multi-key input' '
+	saved=$(git hash-object -w file) &&
+	test_when_finished "git cat-file blob $saved >file" &&
+	echo an extra line >>file &&
+	test_write_lines aa | git add -p >actual &&
+	test_grep "is expected, got ${SQ}aa${SQ}" actual
+'
+
 test_expect_success 'setup expected' '
 	cat >expected <<-\EOF
 	EOF
@@ -526,7 +534,7 @@ test_expect_success 'split hunk setup' '
 	test_write_lines 10 15 20 21 22 23 24 30 40 50 60 >test
 '
 
-test_expect_success 'goto hunk' '
+test_expect_success 'goto hunk 1 with "g 1"' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
 	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? + 1:  -1,2 +1,3          +15
@@ -542,7 +550,20 @@ test_expect_success 'goto hunk' '
 	test_cmp expect actual.trimmed
 '
 
-test_expect_success 'navigate to hunk via regex' '
+test_expect_success 'goto hunk 1 with "g1"' '
+	test_when_finished "git reset" &&
+	tr _ " " >expect <<-EOF &&
+	_10
+	+15
+	_20
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	EOF
+	test_write_lines s y g1 | git add -p >actual &&
+	tail -n 4 <actual >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
+test_expect_success 'navigate to hunk via regex /pattern' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
 	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,2 +1,3 @@
@@ -556,6 +577,19 @@ test_expect_success 'navigate to hunk via regex' '
 	test_cmp expect actual.trimmed
 '
 
+test_expect_success 'navigate to hunk via regex / pattern' '
+	test_when_finished "git reset" &&
+	tr _ " " >expect <<-EOF &&
+	_10
+	+15
+	_20
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	EOF
+	test_write_lines s y / 1,2 | git add -p >actual &&
+	tail -n 4 <actual >actual.trimmed &&
+	test_cmp expect actual.trimmed
+'
+
 test_expect_success 'split hunk "add -p (edit)"' '
 	# Split, say Edit and do nothing.  Then:
 	#
-- 
2.45.1-216-g4365c6fcf9

(Range diff relative to v3)

1:  13d42e5db6 ! 1:  de62120664 add-patch: enforce only one-letter response to prompts
    @@ add-patch.c: static int patch_update_file(struct add_p_state *s,
     +
     +		/* 'g' takes a hunk number and '/' takes a regexp */
     +		if (s->answer.len != 1 && (ch != 'g' && ch != '/')) {
    -+			error(_("only one letter is expected, got '%s'"), s->answer.buf);
    ++			err(s, _("Only one letter is expected, got '%s'"), s->answer.buf);
     +			continue;
     +		}
      		if (ch == 'y') {
    @@ t/t3701-add-interactive.sh: test_expect_success 'revert works (commit)' '
     +	saved=$(git hash-object -w file) &&
     +	test_when_finished "git cat-file blob $saved >file" &&
     +	echo an extra line >>file &&
    -+	test_write_lines aa | git add -p >actual 2>error &&
    -+	test_grep "error: .* got ${SQ}aa${SQ}" error
    ++	test_write_lines aa | git add -p >actual &&
    ++	test_grep "is expected, got ${SQ}aa${SQ}" actual
     +'
     +
      test_expect_success 'setup expected' '
    @@ t/t3701-add-interactive.sh: test_expect_success 'split hunk setup' '
     +test_expect_success 'goto hunk 1 with "g 1"' '
      	test_when_finished "git reset" &&
      	tr _ " " >expect <<-EOF &&
    - 	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? + 1:  -1,2 +1,3          +15
    + 	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? + 1:  -1,2 +1,3          +15
     @@ t/t3701-add-interactive.sh: test_expect_success 'goto hunk' '
      	test_cmp expect actual.trimmed
      '
    @@ t/t3701-add-interactive.sh: test_expect_success 'goto hunk' '
     +	_10
     +	+15
     +	_20
    -+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?_
    ++	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
     +	EOF
     +	test_write_lines s y g1 | git add -p >actual &&
     +	tail -n 4 <actual >actual.trimmed &&
    @@ t/t3701-add-interactive.sh: test_expect_success 'goto hunk' '
     +test_expect_success 'navigate to hunk via regex /pattern' '
      	test_when_finished "git reset" &&
      	tr _ " " >expect <<-EOF &&
    - 	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? @@ -1,2 +1,3 @@
    + 	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,2 +1,3 @@
     @@ t/t3701-add-interactive.sh: test_expect_success 'navigate to hunk via regex' '
      	test_cmp expect actual.trimmed
      '
    @@ t/t3701-add-interactive.sh: test_expect_success 'navigate to hunk via regex' '
     +	_10
     +	+15
     +	_20
    -+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?_
    ++	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
     +	EOF
     +	test_write_lines s y / 1,2 | git add -p >actual &&
     +	tail -n 4 <actual >actual.trimmed &&

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2789 bytes --]

On Wed, May 22, 2024 at 02:45:48PM -0700, Junio C Hamano wrote:
> In a "git add -p" session, especially when we are not using the
> single-key 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.
> 
> The two exceptions are the 'g' command that takes a hunk number, and
> the '/' command that takes a regular expression.  They have to be
> accompanied by their operands (this makes me wonder how users who
> set the interactive.singlekey configuration feed these operands---it
> turns out that we notice there is no operand and give them another
> chance to type the operand separately, without using single key
> input this time), so we accept a string that is more than one byte
> long.
> 
> 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 <gitster@pobox.com>

This version looks good to me, thanks!

> ---
>  * Hopefully the final iteration.  The differences are:
> 
>    - The end-user facing "here is what is wrong with your input"
>      message is given with err() to be consistent with other such
>      messages.
> 
>    - I gave up basing this on v2.44.0, as it is a new feature that
>      does not have to be merged down to older maintenance tracks.
>      This is now based on 80dbfac2 (Merge branch
>      'rj/add-p-typo-reaction', 2024-05-08), which is before v2.45.1
>      but has modern enough t3701 and add-patch.c:err() sends its
>      output to the standard output stream.
> 
>    - The tests for 'g' and '/' to check both the stuck and the split
>      forms have been updated for the more recent prompt that
>      includes 'p'.
> 
>    - The test for multi-key sequence expects the err() output on the
>      standard output stream.
> 
>    As an experiment, this message has the range-diff at the end, not
>    before the primary part of the patch text.  I think this format
>    should be easier to read for reviewers.

Huh, interesting. I do like that format better indeed. You did that
manually instead of using `--range-diff`, right?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>>    As an experiment, this message has the range-diff at the end, not
>>    before the primary part of the patch text.  I think this format
>>    should be easier to read for reviewers.
>
> Huh, interesting. I do like that format better indeed. You did that
> manually instead of using `--range-diff`, right?

Yes.

To me, "format-patch --range-diff" is a lot more cumbersome to use
than running "format-patch", open the result in Emacs, and then
doing "\C-u \M-! git range-diff ..." to insert its output, as I'll
be opening it in the editor for typofixes anyway.