{"thread":{"id":"61520","subject":"[PATCH] add-patch: response to unknown command","startedAt":"2024-05-21T00:37:57Z","lastAt":"2024-05-23T15:58:30Z","messageCount":21,"participants":["Rubén Justo","Patrick Steinhardt","Junio C Hamano","Taylor Blau","Eric Sunshine","Dragan Simic"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"495137","messageId":"1dbe4c61-d75f-45d9-95d2-ac8acae22c56@gmail.com","threadId":"61520","inReplyTo":null,"subject":"[PATCH] add-patch: response to unknown command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-05-21T00:37:54Z","receivedAt":"2024-05-21T00:37:57Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In 26998ed2a2 (add-patch: response to unknown command, 2024-04-29) we\nintroduced an error message that displays the invalid command entered by\nthe user.\n\nWe process a line received from the user, but we only accept\nsingle-character commands.\n\nTo avoid confusion, include in the error message only the first\ncharacter received.\n\nSigned-off-by: Rubén Justo \n---\n add-patch.c | 4 ++--\n t/t3701-add-interactive.sh | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 2252895c28..d408a85353 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1692,8 +1692,8 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\t\t\t \"%.*s\", (int)(eol - p), p);\n \t\t\t}\n \t\t} else {\n-\t\t\terr(s, _(\"Unknown command '%s' (use '?' for help)\"),\n-\t\t\t s->answer.buf);\n+\t\t\terr(s, _(\"Unknown command '%c' (use '?' for help)\"),\n+\t\t\t s->answer.buf[0]);\n \t\t}\n \t}\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 28a95a775d..6f5d3085af 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -60,7 +60,7 @@ test_expect_success 'warn about add.interactive.useBuiltin' '\n \n test_expect_success 'unknown command' '\n \ttest_when_finished \"git reset --hard; rm -f command\" &&\n-\techo W >command &&\n+\techo WW >command &&\n \tgit add -N command &&\n \tgit diff command >expect &&\n \tcat >>expect <<-EOF &&\n-- \n2.45.1.217.gdb529f37a6\n"},{"id":"495147","messageId":"ZkxHLE_8OpYvmViY@tanuki","threadId":"61520","inReplyTo":"1dbe4c61-d75f-45d9-95d2-ac8acae22c56@gmail.com","subject":"Re: [PATCH] add-patch: response to unknown command","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-21T07:03:08Z","receivedAt":"2024-05-21T07:03:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, May 21, 2024 at 02:37:54AM +0200, Rubén Justo wrote:\n> In 26998ed2a2 (add-patch: response to unknown command, 2024-04-29) we\n> introduced an error message that displays the invalid command entered by\n> the user.\n> \n> We process a line received from the user, but we only accept\n> single-character commands.\n> \n> To avoid confusion, include in the error message only the first\n> character received.\n\nI'm a bit on the edge here. Is it really less confusing if we confront\nthe user with a command that they have never even provided in the first\nplace? They implicitly specified the first letter, only, but the user\nfirst needs to be aware that we discard everything but the first letter\nin the first place.\n\nIs it even sensible that we don't complain about trailing garbage in the\nuser's answer? Shouldn't we rather fix that and make the accepted\nanswers more strict, such that if the response is longer than a single\ncharacter we point that out?\n\nPatrick\n"},{"id":"495159","messageId":"b7f9de4d-bd5b-40e3-8ee8-977f507b616d@gmail.com","threadId":"61520","inReplyTo":"ZkxHLE_8OpYvmViY@tanuki","subject":"Re: [PATCH] add-patch: response to unknown command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-05-21T12:59:07Z","receivedAt":"2024-05-21T12:59:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Tue, May 21, 2024 at 09:03:08AM +0200, Patrick Steinhardt wrote:\n> On Tue, May 21, 2024 at 02:37:54AM +0200, Rubén Justo wrote:\n> > In 26998ed2a2 (add-patch: response to unknown command, 2024-04-29) we\n> > introduced an error message that displays the invalid command entered by\n> > the user.\n> > \n> > We process a line received from the user, but we only accept\n> > single-character commands.\n> > \n> > To avoid confusion, include in the error message only the first\n> > character received.\n> \n> I'm a bit on the edge here. Is it really less confusing if we confront\n> the user with a command that they have never even provided in the first\n> place?\n\nI think so, by giving the user what we find wrong and implicitly telling\nthem what it is.\n\n> Shouldn't we rather fix that and make the accepted\n> answers more strict, such that if the response is longer than a single\n> character we point that out?\n\nThat's reasonable, but maybe we're going to break someone's workflow?\n\nAt any rate, my main goal is to avoid the '%s' in the message.\n\n> \n> Patrick\n"},{"id":"495173","messageId":"xmqqr0dvb1sh.fsf_-_@gitster.g","threadId":"61520","inReplyTo":"ZkxHLE_8OpYvmViY@tanuki","subject":"Re* [PATCH] add-patch: response to unknown command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-21T15:52:14Z","receivedAt":"2024-05-21T15:52:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> I'm a bit on the edge here. Is it really less confusing if we confront\n> the user with a command that they have never even provided in the first\n> place? They implicitly specified the first letter, only, but the user\n> first needs to be aware that we discard everything but the first letter\n> in the first place.\n\nI share your doubt. If what the user said (e.g. \"ues\") when they\nwanted to say \"yes\", I find \"You said 'u', which I do not understand\" \nmore confusiong than \"You said 'ues', which I do not understand\".\n\n> Is it even sensible that we don't complain about trailing garbage in the\n> user's answer? Shouldn't we rather fix that and make the accepted\n> answers more strict, such that if the response is longer than a single\n> character we point that out?\n\nI personally guess that it is unlikely that folks are taking\nadvantage of the fact that everything but the first is ignored, and\nI cannot think of a reason why folks prefer that behaviour offhand.\n\nIf 'q' and 'a' are next to each other on the user's keyboard, there\nis a plausible chance that we see 'qa' when the user who wanted to\nsay 'a' fat-fingered and we ended up doing the 'q' thing instead,\nand we may want to prevent such problems from happening.\n\nInstead of ignoring, we _could_ take 'yn' and apply 'y' to the\ncurrent question, and then 'n' to the next question without\nprompting (or showing prompt and answer together without taking\nfurther answer), and claim that it is a typesaving feature, but\nit is dubious users can sensibly choose the answer to a prompt\nthey haven't seen.\n\nSo, I am inclined to be supportive on that \"tighten multi-byte\ninput\" idea, but as I said the above is based on a mere \"I cannot\nthink of ... offhand\", so we need to see if people have reasonable\nuse cases to object first.\n\n------- >8 ------------- >8 ------------- >8 ------------- >8 -------\nSubject: add-patch: enforce only one-letter response to prompts\n\nIn an \"git add -p\" session, especially when we are not using the\nsingle-char mode, we may see 'qa' as a response to a prompt\n\n (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?\n\nand then just do the 'q' thing (i.e. quit the session), ignoring\neverything other than the first byte.\n\nIf 'q' and 'a' are next to each other on the user's keyboard, there\nis a plausible chance that we see 'qa' when the user who wanted to\nsay 'a' fat-fingered and we ended up doing the 'q' thing instead.\n\nAs we didn't think of a good reason during the review discussion why\nwe want to accept excess letters only to ignore them, it appears to\nbe a safe change to simply reject input that is longer than just one\nbyte.\n\nKeep the \"use only the first byte, downcased\" behaviour when we ask\nyes/no question, though. Neither on Qwerty or on Dvorak, 'y' and\n'n' are not close to each other.\n\nSigned-off-by: Junio C Hamano \n---\n add-patch.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git c/add-patch.c w/add-patch.c\nindex 2252895c28..7126bc5d70 100644\n--- c/add-patch.c\n+++ w/add-patch.c\n@@ -1227,6 +1227,7 @@ static int prompt_yesno(struct add_p_state *s, const char *prompt)\n \t\tfflush(stdout);\n \t\tif (read_single_character(s) == EOF)\n \t\t\treturn -1;\n+\t\t/* do not limit to 1-byte input to allow 'no' etc. */\n \t\tswitch (tolower(s->answer.buf[0])) {\n \t\tcase 'n': return 0;\n \t\tcase 'y': return 1;\n@@ -1509,6 +1510,10 @@ static int patch_update_file(struct add_p_state *s,\n \n \t\tif (!s->answer.len)\n \t\t\tcontinue;\n+\t\tif (1 < s->answer.len) {\n+\t\t\terror(_(\"only one letter is expected, got '%s'\"), s->answer.buf);\n+\t\t\tcontinue;\n+\t\t}\n \t\tch = tolower(s->answer.buf[0]);\n \t\tif (ch == 'y') {\n \t\t\thunk->use = USE_HUNK;\n\n"},{"id":"495245","messageId":"Zk0fvOpOapsAkWSd@nand.local","threadId":"61520","inReplyTo":"xmqqr0dvb1sh.fsf_-_@gitster.g","subject":"Re: Re* [PATCH] add-patch: response to unknown command","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-05-21T22:27:08Z","receivedAt":"2024-05-21T22:27:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, May 21, 2024 at 08:52:14AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n>\n> > I'm a bit on the edge here. Is it really less confusing if we confront\n> > the user with a command that they have never even provided in the first\n> > place? They implicitly specified the first letter, only, but the user\n> > first needs to be aware that we discard everything but the first letter\n> > in the first place.\n>\n> I share your doubt. If what the user said (e.g. \"ues\") when they\n> wanted to say \"yes\", I find \"You said 'u', which I do not understand\"\n> more confusiong than \"You said 'ues', which I do not understand\".\n\nSame here. The below patch provides compelling reasoning and has my:\n\n Acked-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"495250","messageId":"xmqqr0duix3q.fsf@gitster.g","threadId":"61520","inReplyTo":"Zk0fvOpOapsAkWSd@nand.local","subject":"Re: Re* [PATCH] add-patch: response to unknown command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-21T23:06:17Z","receivedAt":"2024-05-21T23:06:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>>\n>> I share your doubt. If what the user said (e.g. \"ues\") when they\n>> wanted to say \"yes\", I find \"You said 'u', which I do not understand\"\n>> more confusiong than \"You said 'ues', which I do not understand\".\n>\n> Same here. The below patch provides compelling reasoning and has my:\n>\n> Acked-by: Taylor Blau \n\nHeh, this breaks '/' command hence t3701.45 as it takes an argument\nhence not limited to a single letter. I wonder how singlekey folks\ninvoke that feature, though ;-)\n"},{"id":"495251","messageId":"xmqqh6eqiwgf.fsf@gitster.g","threadId":"61520","inReplyTo":"xmqqr0dvb1sh.fsf_-_@gitster.g","subject":"[PATCH v2] add-patch: enforce only one-letter response to prompts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-21T23:20:16Z","receivedAt":"2024-05-21T23:20:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In an \"git add -p\" session, especially when we are not using the\nsingle-char mode, we may see 'qa' as a response to a prompt\n\n (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?\n\nand then just do the 'q' thing (i.e. quit the session), ignoring\neverything other than the first byte.\n\nIf 'q' and 'a' are next to each other on the user's keyboard, there\nis a plausible chance that we see 'qa' when the user who wanted to\nsay 'a' fat-fingered and we ended up doing the 'q' thing instead.\n\nAs we didn't think of a good reason during the review discussion why\nwe want to accept excess letters only to ignore them, it appears to\nbe a safe change to simply reject input that is longer than just one\nbyte.\n\nThe two exceptions are the 'g' command that takes a hunk number, and\nthe '/' command that takes a regular expression. They has to be\naccompanied by their operands (this makes me wonder how users who\nset the interactive.singlekey configuration feed these operands---it\nturns out that we notice there is no operand and give them another\nchance to type the operand separately, without using single key\ninput this time), so we accept a string that is more than one byte\nlong.\n\nKeep the \"use only the first byte, downcased\" behaviour when we ask\nyes/no question, though. Neither on Qwerty or on Dvorak, 'y' and\n'n' are not close to each other.\n\nSigned-off-by: Junio C Hamano \n---\n * This version fixes the breakage in t3701 where we exercise the\n '/' command. Further code inspection reveals that 'g' also needs\n to be special cased.\n\n The previous iteration was .\n\n add-patch.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 79eda168eb..a6c3367d59 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1228,6 +1228,7 @@ static int prompt_yesno(struct add_p_state *s, const char *prompt)\n \t\tfflush(stdout);\n \t\tif (read_single_character(s) == EOF)\n \t\t\treturn -1;\n+\t\t/* do not limit to 1-byte input to allow 'no' etc. */\n \t\tswitch (tolower(s->answer.buf[0])) {\n \t\tcase 'n': return 0;\n \t\tcase 'y': return 1;\n@@ -1506,6 +1507,12 @@ static int patch_update_file(struct add_p_state *s,\n \t\tif (!s->answer.len)\n \t\t\tcontinue;\n \t\tch = tolower(s->answer.buf[0]);\n+\n+\t\t/* 'g' takes a hunk number, '/' takes a regexp */\n+\t\tif (1 < s->answer.len && (ch != 'g' && ch != '/')) {\n+\t\t\terror(_(\"only one letter is expected, got '%s'\"), s->answer.buf);\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (ch == 'y') {\n \t\t\thunk->use = USE_HUNK;\n soft_increment:\n-- \n2.45.1-216-g4365c6fcf9\n\n"},{"id":"495254","messageId":"CAPig+cTcmpm5kHLwOzcJ4RfmfJwfO1qB4VVcngcvh=_zL5mm9w@mail.gmail.com","threadId":"61520","inReplyTo":"xmqqh6eqiwgf.fsf@gitster.g","subject":"Re: [PATCH v2] add-patch: enforce only one-letter response to prompts","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-05-21T23:36:23Z","receivedAt":"2024-05-21T23:36:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 21, 2024 at 7:20 PM Junio C Hamano wrote:\n> In an \"git add -p\" session, especially when we are not using the\n> single-char mode, we may see 'qa' as a response to a prompt\n>\n> (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?\n>\n> and then just do the 'q' thing (i.e. quit the session), ignoring\n> everything other than the first byte.\n>\n> If 'q' and 'a' are next to each other on the user's keyboard, there\n> is a plausible chance that we see 'qa' when the user who wanted to\n> say 'a' fat-fingered and we ended up doing the 'q' thing instead.\n>\n> As we didn't think of a good reason during the review discussion why\n> we want to accept excess letters only to ignore them, it appears to\n> be a safe change to simply reject input that is longer than just one\n> byte.\n>\n> The two exceptions are the 'g' command that takes a hunk number, and\n> the '/' command that takes a regular expression. They has to be\n\ns/has/have/\n\n> accompanied by their operands (this makes me wonder how users who\n> set the interactive.singlekey configuration feed these operands---it\n> turns out that we notice there is no operand and give them another\n> chance to type the operand separately, without using single key\n> input this time), so we accept a string that is more than one byte\n> long.\n>\n> Keep the \"use only the first byte, downcased\" behaviour when we ask\n> yes/no question, though. Neither on Qwerty or on Dvorak, 'y' and\n> 'n' are not close to each other.\n>\n> Signed-off-by: Junio C Hamano \n"},{"id":"495258","messageId":"xmqqcypeisca.fsf@gitster.g","threadId":"61520","inReplyTo":"CAPig+cTcmpm5kHLwOzcJ4RfmfJwfO1qB4VVcngcvh=_zL5mm9w@mail.gmail.com","subject":"Re: [PATCH v2] add-patch: enforce only one-letter response to prompts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-22T00:49:09Z","receivedAt":"2024-05-22T00:49:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> The two exceptions are the 'g' command that takes a hunk number, and\n>> the '/' command that takes a regular expression. They has to be\n>\n> s/has/have/\n\nThanks for a typofix.\n\nInput to possibly update this part ...\n\n>> As we didn't think of a good reason during the review discussion why\n>> we want to accept excess letters only to ignore them,...\n\n... of the proposed log message, or convince us that it is not such\na great idea, is also welcome.\n\nThanks. Will queue but keep out of 'next' for a few more days.\n"},{"id":"495265","messageId":"fbb9c7d3e7c2129bc1526dfa5a8eca0c@manjaro.org","threadId":"61520","inReplyTo":"xmqqh6eqiwgf.fsf@gitster.g","subject":"Re: [PATCH v2] add-patch: enforce only one-letter response to prompts","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-05-22T06:40:47Z","receivedAt":"2024-05-22T06:40:49Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello Junio,\n\nPlease see my comments below.\n\nOn 2024-05-22 01:20, Junio C Hamano wrote:\n> In an \"git add -p\" session, especially when we are not using the\n\ns/In an/In a/\n\n> single-char mode, we may see 'qa' as a response to a prompt\n\nPerhaps s/single-char/single-character/\n\n> \n> (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?\n> \n> and then just do the 'q' thing (i.e. quit the session), ignoring\n> everything other than the first byte.\n> \n> If 'q' and 'a' are next to each other on the user's keyboard, there\n> is a plausible chance that we see 'qa' when the user who wanted to\n> say 'a' fat-fingered and we ended up doing the 'q' thing instead.\n> \n> As we didn't think of a good reason during the review discussion why\n> we want to accept excess letters only to ignore them, it appears to\n> be a safe change to simply reject input that is longer than just one\n> byte.\n> \n> The two exceptions are the 'g' command that takes a hunk number, and\n> the '/' command that takes a regular expression. They has to be\n> accompanied by their operands (this makes me wonder how users who\n> set the interactive.singlekey configuration feed these operands---it\n> turns out that we notice there is no operand and give them another\n> chance to type the operand separately, without using single key\n> input this time), so we accept a string that is more than one byte\n> long.\n> \n> Keep the \"use only the first byte, downcased\" behaviour when we ask\n> yes/no question, though. Neither on Qwerty or on Dvorak, 'y' and\n> 'n' are not close to each other.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> * This version fixes the breakage in t3701 where we exercise the\n> '/' command. Further code inspection reveals that 'g' also needs\n> to be special cased.\n> \n> The previous iteration was .\n> \n> add-patch.c | 7 +++++++\n> 1 file changed, 7 insertions(+)\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index 79eda168eb..a6c3367d59 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1228,6 +1228,7 @@ static int prompt_yesno(struct add_p_state *s,\n> const char *prompt)\n> \t\tfflush(stdout);\n> \t\tif (read_single_character(s) == EOF)\n> \t\t\treturn -1;\n> +\t\t/* do not limit to 1-byte input to allow 'no' etc. */\n> \t\tswitch (tolower(s->answer.buf[0])) {\n> \t\tcase 'n': return 0;\n> \t\tcase 'y': return 1;\n> @@ -1506,6 +1507,12 @@ static int patch_update_file(struct add_p_state \n> *s,\n> \t\tif (!s->answer.len)\n> \t\t\tcontinue;\n> \t\tch = tolower(s->answer.buf[0]);\n> +\n> +\t\t/* 'g' takes a hunk number, '/' takes a regexp */\n> +\t\tif (1 < s->answer.len && (ch != 'g' && ch != '/')) {\n\nTo me, \"s->answer.len > 1\" would be much more readable, and\nI was surprised a bit to see the flipped variant. This made\nme curious; would you, please, let me know why do you prefer\nthis form?\n\n> +\t\t\terror(_(\"only one letter is expected, got '%s'\"), s->answer.buf);\n> +\t\t\tcontinue;\n> +\t\t}\n> \t\tif (ch == 'y') {\n> \t\t\thunk->use = USE_HUNK;\n> soft_increment:\n\nThe patch is looking good to me, and I find it good that it\nimproves the strictness of the user input, which should also\nimprove the overall user experience.\n"},{"id":"495298","messageId":"Zk3R4MuCWOYVz3_B@tanuki","threadId":"61520","inReplyTo":"xmqqh6eqiwgf.fsf@gitster.g","subject":"Re: [PATCH v2] add-patch: enforce only one-letter response to prompts","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-22T11:07:12Z","receivedAt":"2024-05-22T11:07:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, May 21, 2024 at 04:20:16PM -0700, Junio C Hamano wrote:\n> In an \"git add -p\" session, especially when we are not using the\n> single-char mode, we may see 'qa' as a response to a prompt\n> \n> (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?\n> \n> and then just do the 'q' thing (i.e. quit the session), ignoring\n> everything other than the first byte.\n> \n> If 'q' and 'a' are next to each other on the user's keyboard, there\n> is a plausible chance that we see 'qa' when the user who wanted to\n> say 'a' fat-fingered and we ended up doing the 'q' thing instead.\n\nI think it's a good idea regardless of the layout. There are tons of\nlayouts out there that are very esoteric (I for one use NEO2, which most\nnobody has ever heard of), and I'm sure you will find at least one\nlayout where characters are positioned such that you can fat finger\nthings.\n\nAnother argument that is independent of fat fingering is that it\npotentially allows us to expand this feature with multi-byte verbs going\nforward.\n\n[snip]\n> Keep the \"use only the first byte, downcased\" behaviour when we ask\n> yes/no question, though. Neither on Qwerty or on Dvorak, 'y' and\n> 'n' are not close to each other.\n\nJust to prove my point: Workman layout has them right next to each other\n:) What we make of that information is a different question though.\n\n> diff --git a/add-patch.c b/add-patch.c\n> index 79eda168eb..a6c3367d59 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1228,6 +1228,7 @@ static int prompt_yesno(struct add_p_state *s, const char *prompt)\n> \t\tfflush(stdout);\n> \t\tif (read_single_character(s) == EOF)\n> \t\t\treturn -1;\n> +\t\t/* do not limit to 1-byte input to allow 'no' etc. */\n> \t\tswitch (tolower(s->answer.buf[0])) {\n> \t\tcase 'n': return 0;\n> \t\tcase 'y': return 1;\n> @@ -1506,6 +1507,12 @@ static int patch_update_file(struct add_p_state *s,\n> \t\tif (!s->answer.len)\n> \t\t\tcontinue;\n> \t\tch = tolower(s->answer.buf[0]);\n> +\n> +\t\t/* 'g' takes a hunk number, '/' takes a regexp */\n> +\t\tif (1 < s->answer.len && (ch != 'g' && ch != '/')) {\n\nI find this condition a bit unusual and thus hard to read. If it instead\nsaid `s->answer.len != 1` then it would be way easier to comprehend.\n\nAlso, none of the branches othar than for 'g' and '/' use `s->answer`,\nso this should be safe. I also very much agree with the general idea of\nthis patch.\n\n> +\t\t\terror(_(\"only one letter is expected, got '%s'\"), s->answer.buf);\n> +\t\t\tcontinue;\n> +\t\t}\n> \t\tif (ch == 'y') {\n> \t\t\thunk->use = USE_HUNK;\n> soft_increment:\n\nI assume we also want a test for this new behaviour, right?\n\nPatrick\n"},{"id":"495303","messageId":"xmqqzfsh6cjf.fsf@gitster.g","threadId":"61520","inReplyTo":"fbb9c7d3e7c2129bc1526dfa5a8eca0c@manjaro.org","subject":"Re: [PATCH v2] add-patch: enforce only one-letter response to prompts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-22T16:23:32Z","receivedAt":"2024-05-22T16:23:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> Hello Junio,\n>\n> Please see my comments below.\n>\n> On 2024-05-22 01:20, Junio C Hamano wrote:\n>> In an \"git add -p\" session, especially when we are not using the\n>\n> s/In an/In a/\n\nGood eyes.\n\n>\n>> single-char mode, we may see 'qa' as a response to a prompt\n>\n> Perhaps s/single-char/single-character/\n\nI shouldn't have been loose in the language. Rather, we should say\n\"single key mode\", as the knob to control the feature is the\n\"interactive.singlekey\" variable.\n\n>> +\t\t/* 'g' takes a hunk number, '/' takes a regexp */\n>> +\t\tif (1 < s->answer.len && (ch != 'g' && ch != '/')) {\n>\n> To me, \"s->answer.len > 1\" would be much more readable, and\n> I was surprised a bit to see the flipped variant. This made\n> me curious; would you, please, let me know why do you prefer\n> this form?\n\n\"textual order should reflect actual order\" (read CodingGuidelines).\n\nFor more backstory,\n\n https://lore.kernel.org/git/?q=%22textual+order%22+%22actual+order%22\n\n\nThanks.\n"},{"id":"495304","messageId":"xmqqv8356ccb.fsf@gitster.g","threadId":"61520","inReplyTo":"Zk3R4MuCWOYVz3_B@tanuki","subject":"Re: [PATCH v2] add-patch: enforce only one-letter response to prompts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-22T16:27:48Z","receivedAt":"2024-05-22T16:27:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> +\t\t/* 'g' takes a hunk number, '/' takes a regexp */\n>> +\t\tif (1 < s->answer.len && (ch != 'g' && ch != '/')) {\n>\n> I find this condition a bit unusual and thus hard to read. If it instead\n> said `s->answer.len != 1` then it would be way easier to comprehend.\n\nWe have already eliminated the \"it is 0\" case, .len cannot be\nnegative, and the case we really care about is \"is it not just\none?\", so I agree with you that the inequality comparison with 1 is\neasier to grok.\n\n> I assume we also want a test for this new behaviour, right?\n\nHmph, yeah. 'g' has already been tested (that was what led me to do\nthe v2), but we probably should do 'qa' or something.\n\nThanks.\n"},{"id":"495305","messageId":"xmqqikz56a6o.fsf_-_@gitster.g","threadId":"61520","inReplyTo":"xmqqh6eqiwgf.fsf@gitster.g","subject":"[PATCH v3] add-patch: enforce only one-letter response to prompts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-22T17:14:23Z","receivedAt":"2024-05-22T17:14:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In a \"git add -p\" session, especially when we are not using the\nsingle-key mode, we may see 'qa' as a response to a prompt\n\n (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?\n\nand then just do the 'q' thing (i.e. quit the session), ignoring\neverything other than the first byte.\n\nIf 'q' and 'a' are next to each other on the user's keyboard, there\nis a plausible chance that we see 'qa' when the user who wanted to\nsay 'a' fat-fingered and we ended up doing the 'q' thing instead.\n\nAs we didn't think of a good reason during the review discussion why\nwe want to accept excess letters only to ignore them, it appears to\nbe a safe change to simply reject input that is longer than just one\nbyte.\n\nThe two exceptions are the 'g' command that takes a hunk number, and\nthe '/' command that takes a regular expression. They have to be\naccompanied by their operands (this makes me wonder how users who\nset the interactive.singlekey configuration feed these operands---it\nturns out that we notice there is no operand and give them another\nchance to type the operand separately, without using single key\ninput this time), so we accept a string that is more than one byte\nlong.\n\nKeep the \"use only the first byte, downcased\" behaviour when we ask\nyes/no question, though. Neither on Qwerty or on Dvorak, 'y' and\n'n' are not close to each other.\n\nSigned-off-by: Junio C Hamano \n---\n\n The whole range-diff is not worth sharing as the bulk of it show\n the new tests. The part that shows the changes to the proposed log\n message and the code looks like this:\n\n @@ Metadata\n ## Commit message ##\n add-patch: enforce only one-letter response to prompts\n \n - In an \"git add -p\" session, especially when we are not using the\n - single-char mode, we may see 'qa' as a response to a prompt\n + In a \"git add -p\" session, especially when we are not using the\n + single-key mode, we may see 'qa' as a response to a prompt\n \n (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?\n \n @@ add-patch.c: static int patch_update_file(struct add_p_state *s,\n \t\t\tcontinue;\n \t\tch = tolower(s->answer.buf[0]);\n +\n -+\t\t/* 'g' takes a hunk number, '/' takes a regexp */\n -+\t\tif (1 < s->answer.len && (ch != 'g' && ch != '/')) {\n ++\t\t/* 'g' takes a hunk number and '/' takes a regexp */\n ++\t\tif (s->answer.len != 1 && (ch != 'g' && ch != '/')) {\n +\t\t\terror(_(\"only one letter is expected, got '%s'\"), s->answer.buf);\n +\t\t\tcontinue;\n +\t\t}\n \t\tif (ch == 'y') {\n \t\t\thunk->use = USE_HUNK;\n soft_increment:\n \n add-patch.c | 7 +++++++\n t/t3701-add-interactive.sh | 38 ++++++++++++++++++++++++++++++++++++--\n 2 files changed, 43 insertions(+), 2 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 79eda168eb..7242da2c03 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1228,6 +1228,7 @@ static int prompt_yesno(struct add_p_state *s, const char *prompt)\n \t\tfflush(stdout);\n \t\tif (read_single_character(s) == EOF)\n \t\t\treturn -1;\n+\t\t/* do not limit to 1-byte input to allow 'no' etc. */\n \t\tswitch (tolower(s->answer.buf[0])) {\n \t\tcase 'n': return 0;\n \t\tcase 'y': return 1;\n@@ -1506,6 +1507,12 @@ static int patch_update_file(struct add_p_state *s,\n \t\tif (!s->answer.len)\n \t\t\tcontinue;\n \t\tch = tolower(s->answer.buf[0]);\n+\n+\t\t/* 'g' takes a hunk number and '/' takes a regexp */\n+\t\tif (s->answer.len != 1 && (ch != 'g' && ch != '/')) {\n+\t\t\terror(_(\"only one letter is expected, got '%s'\"), s->answer.buf);\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (ch == 'y') {\n \t\t\thunk->use = USE_HUNK;\n soft_increment:\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 0b5339ac6c..61f5e9eec0 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -144,6 +144,14 @@ test_expect_success 'revert works (commit)' '\n \tgrep \"unchanged *+3/-0 file\" output\n '\n \n+test_expect_success 'reject multi-key input' '\n+\tsaved=$(git hash-object -w file) &&\n+\ttest_when_finished \"git cat-file blob $saved >file\" &&\n+\techo an extra line >>file &&\n+\ttest_write_lines aa | git add -p >actual 2>error &&\n+\ttest_grep \"error: .* got ${SQ}aa${SQ}\" error\n+'\n+\n test_expect_success 'setup expected' '\n \tcat >expected <<-\\EOF\n \tEOF\n@@ -511,7 +519,7 @@ test_expect_success 'split hunk setup' '\n \ttest_write_lines 10 15 20 21 22 23 24 30 40 50 60 >test\n '\n \n-test_expect_success 'goto hunk' '\n+test_expect_success 'goto hunk 1 with \"g 1\"' '\n \ttest_when_finished \"git reset\" &&\n \ttr _ \" \" >expect <<-EOF &&\n \t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? + 1: -1,2 +1,3 +15\n@@ -527,7 +535,20 @@ test_expect_success 'goto hunk' '\n \ttest_cmp expect actual.trimmed\n '\n \n-test_expect_success 'navigate to hunk via regex' '\n+test_expect_success 'goto hunk 1 with \"g1\"' '\n+\ttest_when_finished \"git reset\" &&\n+\ttr _ \" \" >expect <<-EOF &&\n+\t_10\n+\t+15\n+\t_20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?_\n+\tEOF\n+\ttest_write_lines s y g1 | git add -p >actual &&\n+\ttail -n 4 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n+test_expect_success 'navigate to hunk via regex /pattern' '\n \ttest_when_finished \"git reset\" &&\n \ttr _ \" \" >expect <<-EOF &&\n \t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? @@ -1,2 +1,3 @@\n@@ -541,6 +562,19 @@ test_expect_success 'navigate to hunk via regex' '\n \ttest_cmp expect actual.trimmed\n '\n \n+test_expect_success 'navigate to hunk via regex / pattern' '\n+\ttest_when_finished \"git reset\" &&\n+\ttr _ \" \" >expect <<-EOF &&\n+\t_10\n+\t+15\n+\t_20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?_\n+\tEOF\n+\ttest_write_lines s y / 1,2 | git add -p >actual &&\n+\ttail -n 4 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n test_expect_success 'split hunk \"add -p (edit)\"' '\n \t# Split, say Edit and do nothing. Then:\n \t#\n-- \n2.45.1-216-g4365c6fcf9\n"},{"id":"495309","messageId":"e9f41db1-741c-413f-81ce-a86b1802e507@gmail.com","threadId":"61520","inReplyTo":"xmqqikz56a6o.fsf_-_@gitster.g","subject":"Re: [PATCH v3] add-patch: enforce only one-letter response to prompts","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-05-22T17:38:51Z","receivedAt":"2024-05-22T17:39:07Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Wed, May 22, 2024 at 10:14:23AM -0700, Junio C Hamano wrote:\n\n> +\t\tif (s->answer.len != 1 && (ch != 'g' && ch != '/')) {\n\nThis \"len!=1\" introduces a nice dose of sanity in the UI.\n\n> +\t\t\terror(_(\"only one letter is expected, got '%s'\"), s->answer.buf);\n\nHere, perhaps you want to do:\n\n\t\t\terr(s, _(\"Only one letter is expected, got '%s'\"), s->answer.buf);\n"},{"id":"495316","messageId":"11abab810253d654119fab69adf44fab@manjaro.org","threadId":"61520","inReplyTo":"xmqqzfsh6cjf.fsf@gitster.g","subject":"Re: [PATCH v2] add-patch: enforce only one-letter response to prompts","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-05-22T19:03:20Z","receivedAt":"2024-05-22T19:03:23Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-05-22 18:23, Junio C Hamano wrote:\n> Dragan Simic writes:\n>> On 2024-05-22 01:20, Junio C Hamano wrote:\n>>> single-char mode, we may see 'qa' as a response to a prompt\n>> \n>> Perhaps s/single-char/single-character/\n> \n> I shouldn't have been loose in the language. Rather, we should say\n> \"single key mode\", as the knob to control the feature is the\n> \"interactive.singlekey\" variable.\n\nYes, \"single-key mode\" is better; \"when interactive.singlekey\nis not enabled\" may be even a bit better. Not worth a reroll,\nof course.\n\n>>> +\t\t/* 'g' takes a hunk number, '/' takes a regexp */\n>>> +\t\tif (1 < s->answer.len && (ch != 'g' && ch != '/')) {\n>> \n>> To me, \"s->answer.len > 1\" would be much more readable, and\n>> I was surprised a bit to see the flipped variant. This made\n>> me curious; would you, please, let me know why do you prefer\n>> this form?\n> \n> \"textual order should reflect actual order\" (read CodingGuidelines).\n> \n> For more backstory,\n> \n> \n> https://lore.kernel.org/git/?q=%22textual+order%22+%22actual+order%22\n\nThat's exactly what I assumed, but frankly, in this particular case\nI really can't force myself, despite trying quite hard, into liking\nit. It's simply strange to me.\n"},{"id":"495322","messageId":"xmqq4jap4pgh.fsf@gitster.g","threadId":"61520","inReplyTo":"e9f41db1-741c-413f-81ce-a86b1802e507@gmail.com","subject":"Re: [PATCH v3] add-patch: enforce only one-letter response to prompts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-22T19:27:26Z","receivedAt":"2024-05-22T19:27:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> Here, perhaps you want to do:\n>\n> \t\t\terr(s, _(\"Only one letter is expected, got '%s'\"), s->answer.buf);\n\nTrue. The end-user errors are reported with err() in the\nsurrounding code.\n\nI however was hoping that this can be based on v2.44.0, which\npredates 9d225b02 (add-patch: do not show UI messages on stderr,\n2024-04-29), which makes the testing of it a bit cumbersome.\n\n"},{"id":"495325","messageId":"xmqqzfsh37gp.fsf@gitster.g","threadId":"61520","inReplyTo":"11abab810253d654119fab69adf44fab@manjaro.org","subject":"Re: [PATCH v2] add-patch: enforce only one-letter response to prompts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-22T20:41:26Z","receivedAt":"2024-05-22T20:41:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n>> For more backstory,\n>> https://lore.kernel.org/git/?q=%22textual+order%22+%22actual+order%22\n>\n> That's exactly what I assumed, but frankly, in this particular case\n> I really can't force myself, despite trying quite hard, into liking\n> it. It's simply strange to me.\n\nYou asked me why, and the reason was given to you. End of story.\n\nI never asked you to like it and you do not have to like it ;-).\n"},{"id":"495334","messageId":"xmqqh6ep1pwz.fsf_-_@gitster.g","threadId":"61520","inReplyTo":"xmqqikz56a6o.fsf_-_@gitster.g","subject":"[PATCH v4] add-patch: enforce only one-letter response to prompts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-22T21:45:48Z","receivedAt":"2024-05-22T21:45:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In a \"git add -p\" session, especially when we are not using the\nsingle-key mode, we may see 'qa' as a response to a prompt\n\n (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?\n\nand then just do the 'q' thing (i.e. quit the session), ignoring\neverything other than the first byte.\n\nIf 'q' and 'a' are next to each other on the user's keyboard, there\nis a plausible chance that we see 'qa' when the user who wanted to\nsay 'a' fat-fingered and we ended up doing the 'q' thing instead.\n\nAs we didn't think of a good reason during the review discussion why\nwe want to accept excess letters only to ignore them, it appears to\nbe a safe change to simply reject input that is longer than just one\nbyte.\n\nThe two exceptions are the 'g' command that takes a hunk number, and\nthe '/' command that takes a regular expression. They have to be\naccompanied by their operands (this makes me wonder how users who\nset the interactive.singlekey configuration feed these operands---it\nturns out that we notice there is no operand and give them another\nchance to type the operand separately, without using single key\ninput this time), so we accept a string that is more than one byte\nlong.\n\nKeep the \"use only the first byte, downcased\" behaviour when we ask\nyes/no question, though. Neither on Qwerty or on Dvorak, 'y' and\n'n' are not close to each other.\n\nSigned-off-by: Junio C Hamano \n---\n * Hopefully the final iteration. The differences are:\n\n - The end-user facing \"here is what is wrong with your input\"\n message is given with err() to be consistent with other such\n messages.\n\n - I gave up basing this on v2.44.0, as it is a new feature that\n does not have to be merged down to older maintenance tracks.\n This is now based on 80dbfac2 (Merge branch\n 'rj/add-p-typo-reaction', 2024-05-08), which is before v2.45.1\n but has modern enough t3701 and add-patch.c:err() sends its\n output to the standard output stream.\n\n - The tests for 'g' and '/' to check both the stuck and the split\n forms have been updated for the more recent prompt that\n includes 'p'.\n\n - The test for multi-key sequence expects the err() output on the\n standard output stream.\n\n As an experiment, this message has the range-diff at the end, not\n before the primary part of the patch text. I think this format\n should be easier to read for reviewers.\n\n add-patch.c | 7 +++++++\n t/t3701-add-interactive.sh | 38 ++++++++++++++++++++++++++++++++++++--\n 2 files changed, 43 insertions(+), 2 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 2252895c28..814de57c4a 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1227,6 +1227,7 @@ static int prompt_yesno(struct add_p_state *s, const char *prompt)\n \t\tfflush(stdout);\n \t\tif (read_single_character(s) == EOF)\n \t\t\treturn -1;\n+\t\t/* do not limit to 1-byte input to allow 'no' etc. */\n \t\tswitch (tolower(s->answer.buf[0])) {\n \t\tcase 'n': return 0;\n \t\tcase 'y': return 1;\n@@ -1510,6 +1511,12 @@ static int patch_update_file(struct add_p_state *s,\n \t\tif (!s->answer.len)\n \t\t\tcontinue;\n \t\tch = tolower(s->answer.buf[0]);\n+\n+\t\t/* 'g' takes a hunk number and '/' takes a regexp */\n+\t\tif (s->answer.len != 1 && (ch != 'g' && ch != '/')) {\n+\t\t\terr(s, _(\"Only one letter is expected, got '%s'\"), s->answer.buf);\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (ch == 'y') {\n \t\t\thunk->use = USE_HUNK;\n soft_increment:\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 28a95a775d..6624a4f7c0 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -160,6 +160,14 @@ test_expect_success 'revert works (commit)' '\n \tgrep \"unchanged *+3/-0 file\" output\n '\n \n+test_expect_success 'reject multi-key input' '\n+\tsaved=$(git hash-object -w file) &&\n+\ttest_when_finished \"git cat-file blob $saved >file\" &&\n+\techo an extra line >>file &&\n+\ttest_write_lines aa | git add -p >actual &&\n+\ttest_grep \"is expected, got ${SQ}aa${SQ}\" actual\n+'\n+\n test_expect_success 'setup expected' '\n \tcat >expected <<-\\EOF\n \tEOF\n@@ -526,7 +534,7 @@ test_expect_success 'split hunk setup' '\n \ttest_write_lines 10 15 20 21 22 23 24 30 40 50 60 >test\n '\n \n-test_expect_success 'goto hunk' '\n+test_expect_success 'goto hunk 1 with \"g 1\"' '\n \ttest_when_finished \"git reset\" &&\n \ttr _ \" \" >expect <<-EOF &&\n \t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? + 1: -1,2 +1,3 +15\n@@ -542,7 +550,20 @@ test_expect_success 'goto hunk' '\n \ttest_cmp expect actual.trimmed\n '\n \n-test_expect_success 'navigate to hunk via regex' '\n+test_expect_success 'goto hunk 1 with \"g1\"' '\n+\ttest_when_finished \"git reset\" &&\n+\ttr _ \" \" >expect <<-EOF &&\n+\t_10\n+\t+15\n+\t_20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\tEOF\n+\ttest_write_lines s y g1 | git add -p >actual &&\n+\ttail -n 4 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n+test_expect_success 'navigate to hunk via regex /pattern' '\n \ttest_when_finished \"git reset\" &&\n \ttr _ \" \" >expect <<-EOF &&\n \t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,2 +1,3 @@\n@@ -556,6 +577,19 @@ test_expect_success 'navigate to hunk via regex' '\n \ttest_cmp expect actual.trimmed\n '\n \n+test_expect_success 'navigate to hunk via regex / pattern' '\n+\ttest_when_finished \"git reset\" &&\n+\ttr _ \" \" >expect <<-EOF &&\n+\t_10\n+\t+15\n+\t_20\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\tEOF\n+\ttest_write_lines s y / 1,2 | git add -p >actual &&\n+\ttail -n 4 actual.trimmed &&\n+\ttest_cmp expect actual.trimmed\n+'\n+\n test_expect_success 'split hunk \"add -p (edit)\"' '\n \t# Split, say Edit and do nothing. Then:\n \t#\n-- \n2.45.1-216-g4365c6fcf9\n\n(Range diff relative to v3)\n\n1: 13d42e5db6 ! 1: de62120664 add-patch: enforce only one-letter response to prompts\n @@ add-patch.c: static int patch_update_file(struct add_p_state *s,\n +\n +\t\t/* 'g' takes a hunk number and '/' takes a regexp */\n +\t\tif (s->answer.len != 1 && (ch != 'g' && ch != '/')) {\n -+\t\t\terror(_(\"only one letter is expected, got '%s'\"), s->answer.buf);\n ++\t\t\terr(s, _(\"Only one letter is expected, got '%s'\"), s->answer.buf);\n +\t\t\tcontinue;\n +\t\t}\n \t\tif (ch == 'y') {\n @@ t/t3701-add-interactive.sh: test_expect_success 'revert works (commit)' '\n +\tsaved=$(git hash-object -w file) &&\n +\ttest_when_finished \"git cat-file blob $saved >file\" &&\n +\techo an extra line >>file &&\n -+\ttest_write_lines aa | git add -p >actual 2>error &&\n -+\ttest_grep \"error: .* got ${SQ}aa${SQ}\" error\n ++\ttest_write_lines aa | git add -p >actual &&\n ++\ttest_grep \"is expected, got ${SQ}aa${SQ}\" actual\n +'\n +\n test_expect_success 'setup expected' '\n @@ t/t3701-add-interactive.sh: test_expect_success 'split hunk setup' '\n +test_expect_success 'goto hunk 1 with \"g 1\"' '\n \ttest_when_finished \"git reset\" &&\n \ttr _ \" \" >expect <<-EOF &&\n - \t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? + 1: -1,2 +1,3 +15\n + \t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? + 1: -1,2 +1,3 +15\n @@ t/t3701-add-interactive.sh: test_expect_success 'goto hunk' '\n \ttest_cmp expect actual.trimmed\n '\n @@ t/t3701-add-interactive.sh: test_expect_success 'goto hunk' '\n +\t_10\n +\t+15\n +\t_20\n -+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?_\n ++\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n +\tEOF\n +\ttest_write_lines s y g1 | git add -p >actual &&\n +\ttail -n 4 actual.trimmed &&\n @@ t/t3701-add-interactive.sh: test_expect_success 'goto hunk' '\n +test_expect_success 'navigate to hunk via regex /pattern' '\n \ttest_when_finished \"git reset\" &&\n \ttr _ \" \" >expect <<-EOF &&\n - \t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? @@ -1,2 +1,3 @@\n + \t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,2 +1,3 @@\n @@ t/t3701-add-interactive.sh: test_expect_success 'navigate to hunk via regex' '\n \ttest_cmp expect actual.trimmed\n '\n @@ t/t3701-add-interactive.sh: test_expect_success 'navigate to hunk via regex' '\n +\t_10\n +\t+15\n +\t_20\n -+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?_\n ++\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n +\tEOF\n +\ttest_write_lines s y / 1,2 | git add -p >actual &&\n +\ttail -n 4 actual.trimmed &&\n"},{"id":"495353","messageId":"Zk7UsJjhY_FV2z8C@tanuki","threadId":"61520","inReplyTo":"xmqqh6ep1pwz.fsf_-_@gitster.g","subject":"Re: [PATCH v4] add-patch: enforce only one-letter response to prompts","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-23T05:31:28Z","receivedAt":"2024-05-23T05:31:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 22, 2024 at 02:45:48PM -0700, Junio C Hamano wrote:\n> In a \"git add -p\" session, especially when we are not using the\n> single-key mode, we may see 'qa' as a response to a prompt\n> \n> (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?\n> \n> and then just do the 'q' thing (i.e. quit the session), ignoring\n> everything other than the first byte.\n> \n> If 'q' and 'a' are next to each other on the user's keyboard, there\n> is a plausible chance that we see 'qa' when the user who wanted to\n> say 'a' fat-fingered and we ended up doing the 'q' thing instead.\n> \n> As we didn't think of a good reason during the review discussion why\n> we want to accept excess letters only to ignore them, it appears to\n> be a safe change to simply reject input that is longer than just one\n> byte.\n> \n> The two exceptions are the 'g' command that takes a hunk number, and\n> the '/' command that takes a regular expression. They have to be\n> accompanied by their operands (this makes me wonder how users who\n> set the interactive.singlekey configuration feed these operands---it\n> turns out that we notice there is no operand and give them another\n> chance to type the operand separately, without using single key\n> input this time), so we accept a string that is more than one byte\n> long.\n> \n> Keep the \"use only the first byte, downcased\" behaviour when we ask\n> yes/no question, though. Neither on Qwerty or on Dvorak, 'y' and\n> 'n' are not close to each other.\n> \n> Signed-off-by: Junio C Hamano \n\nThis version looks good to me, thanks!\n\n> ---\n> * Hopefully the final iteration. The differences are:\n> \n> - The end-user facing \"here is what is wrong with your input\"\n> message is given with err() to be consistent with other such\n> messages.\n> \n> - I gave up basing this on v2.44.0, as it is a new feature that\n> does not have to be merged down to older maintenance tracks.\n> This is now based on 80dbfac2 (Merge branch\n> 'rj/add-p-typo-reaction', 2024-05-08), which is before v2.45.1\n> but has modern enough t3701 and add-patch.c:err() sends its\n> output to the standard output stream.\n> \n> - The tests for 'g' and '/' to check both the stuck and the split\n> forms have been updated for the more recent prompt that\n> includes 'p'.\n> \n> - The test for multi-key sequence expects the err() output on the\n> standard output stream.\n> \n> As an experiment, this message has the range-diff at the end, not\n> before the primary part of the patch text. I think this format\n> should be easier to read for reviewers.\n\nHuh, interesting. I do like that format better indeed. You did that\nmanually instead of using `--range-diff`, right?\n\nPatrick\n"},{"id":"495415","messageId":"xmqq5xv4wme6.fsf@gitster.g","threadId":"61520","inReplyTo":"Zk7UsJjhY_FV2z8C@tanuki","subject":"Re: [PATCH v4] add-patch: enforce only one-letter response to prompts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-23T15:58:25Z","receivedAt":"2024-05-23T15:58:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> As an experiment, this message has the range-diff at the end, not\n>> before the primary part of the patch text. I think this format\n>> should be easier to read for reviewers.\n>\n> Huh, interesting. I do like that format better indeed. You did that\n> manually instead of using `--range-diff`, right?\n\nYes.\n\nTo me, \"format-patch --range-diff\" is a lot more cumbersome to use\nthan running \"format-patch\", open the result in Emacs, and then\ndoing \"\\C-u \\M-! git range-diff ...\" to insert its output, as I'll\nbe opening it in the editor for typofixes anyway.\n\n\n\n"}]}