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