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

[phillip.wood123@gmail.com]

Hi Rubén

This is looking good, I've left a couple of comments on the test.

On 21/04/2024 10:51, Rubén Justo wrote:
> When the user gives an unknown command to the "add -p" prompt, the list
> of accepted commands with their explanation is given.  This is the same
> output they get when they say '?'.
> 
> However, the unknown command may be due to a user input error rather
> than the user not knowing the valid command.
> 
> To reduce the likelihood of user confusion and error repetition, instead
> of displaying the list of accepted commands, display a short error
> message with the unknown 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>

> +test_expect_success 'unknown command' '
> +	test_when_finished "git reset --hard; rm -f command" &&
> +	echo W >command &&

I find separating out writing this file from where it is used makes it 
harder to see the input to "git add -p" using test_write_lines as you 
did in v1 matches the existing tests and is perfectly fine.

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

It is really good to see us testing the whole of the program output now 
but have you tested this with GIT_TEST_DEFAULT_HASH=sha256? All the 
other tests use diff_cmp() to avoid test failures caused by the index 
line depending on the hash function. Alternatively you could run "git 
diff" to create that part of the expected output.

It is a shame we're not testing '?' as we have changed the 
implementation - before this commit '?' was just another unknown command 
as far as the implementation was concerned, now we treat it differently. 
It would be nice to see some test coverage for the 'p' command you 
recently added as well at some point.

>   test_expect_success 'setup patch' '
> -	SP=" " &&

Nice attention to detail, this is not needed now SP is defined at the 
beginning of the test file.

Best Wishes

Phillip