[PATCH v2] add-patch: response to invalid command

From: Rubén Justo <rjusto@gmail.com>
Date: Apr 20, 2024, 11:08 UTC
Message-ID: <6d421c67-9e10-4a7b-9782-38ba8e9da915@gmail.com>
In-Reply-To: <4e2bc660-ee33-4641-aca5-783d0cefcd23@gmail.com>

When the user enters an invalid command, we respond with a list of accepted commands; the response we give to the command '?'.

However, the invalid command may be due to either a user input error or a malfunctioning interface component, rather than the user not knowing the valid command.

Our response is unlikely to provide help in such situations.

To reduce the likelihood of user confusion and error repetition, if an unrecognized command is received, stop displaying the help text and display a short error message with the invalid command received, as feedback to the user.

Include a reminder about the current command '?' in the new message, to guide the user if they want help.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

Changes since v1:

  - Use a temporary file for the lines.
  - Say explicitly in the message that '?' is an existing command.
  - Instead of "option", use "command".
  - Test for stdout, not just stderr.

 add-patch.c                |  5 ++++-
 t/t3701-add-interactive.sh | 23 ++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index a06dd18985..7be142d448 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,
 			}
 		} else if (s->answer.buf[0] == 'p') {
 			rendered_hunk_index = -1;
-		} else {
+		} else if (s->answer.buf[0] == '?') {
 			const char *p = _(help_patch_remainder), *eol = p;
 
 			color_fprintf(stdout, s->s.help_color, "%s",
@@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,
 				color_fprintf_ln(stdout, s->s.help_color,
 						 "%.*s", (int)(eol - p), p);
 			}
+		} else {
+			err(s, _("Unknown command '%s' (use '?' for help)"),
+			    s->answer.buf);
 		}
 	}
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index bc55255b0a..4c3901de17 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -7,6 +7,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
+SP=" "
+
 diff_cmp () {
 	for x
 	do
@@ -61,6 +63,26 @@ test_expect_success 'setup (initial)' '
 	echo more >>file &&
 	echo lines >>file
 '
+
+test_expect_success 'unknown command' '
+	test_when_finished "git reset command && rm command" &&
+	echo W >command &&
+	git add -N command &&
+	cat >expect <<-EOF &&
+	diff --git a/command b/command
+	new file mode 100644
+	index 0000000..a42d8ff
+	--- /dev/null
+	+++ b/command
+	@@ -0,0 +1 @@
+	+W
+	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
+	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
+	EOF
+	git add -p -- command <command >actual 2>&1 &&
+	test_cmp expect actual
+'
+
 test_expect_success 'status works (initial)' '
 	git add -i </dev/null >output &&
 	grep "+1/-0 *+2/-0 file" output
@@ -231,7 +253,6 @@ test_expect_success 'setup file' '
 '
 
 test_expect_success 'setup patch' '
-	SP=" " &&
 	NULL="" &&
 	cat >patch <<-EOF
 	@@ -1,4 +1,4 @@

-- 
2.45.0.rc0.1.gc94c838fad