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

From: phillip.wood123@gmail.com <phillip.wood123@gmail.com>
Date: Apr 21, 2024, 13:18 UTC
Message-ID: <b1c725f3-0e92-4723-a7ca-8d001d60b89e@gmail.com>
In-Reply-To: <db1d540f-30ae-4d4c-883b-088bcfe68140@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:

Show 15 quoted lines

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

Show 14 quoted lines

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