[PATCH v4] add-patch: enforce only one-letter response to prompts
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(-)
Show changes to 2 files +43 −2
add-patch.c, t/t3701-add-interactive.sh
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 &&
Rubén Justo
