Re: [PATCH v2] add-patch: response to invalid command
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 20, 2024, 17:53 UTC
- Message-ID
- <xmqqr0ez3opi.fsf@gitster.g>
- In-Reply-To
- <6d421c67-9e10-4a7b-9782-38ba8e9da915@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> When the user enters an invalid command, we respond with a list of > accepted commands; the response we give to the command '?'.
I am guessing that you want to say with the part of the sentence after the semicolon that the list of accepted commands and the explanation for them is the same as what we give when the user asks '?', but that is after reading the above twice. I wonder
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 `?`.is easier to understand?
> 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.
I am not sure readers would understand what you are trying to refer to with "or a malfunctioning interface component". I don't, at least. Either rewrite it to be understandable, or drop it. I think dropping it is sufficient in this case.
Show 6 quoted lines
> 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.
"stop ... text and" -> "instead of ... text,"
It would give a better contrast between the current UI and the proposed new one.
> Include a reminder about the current command '?' in the new message, to > guide the user if they want help.
OK.
Show 22 quoted lines
> 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);
> }
> }Looking good.
Show 9 quoted lines
> 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=" "
Good to see what is already used in the existing tests reused.
Show 10 quoted lines
> 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" &&Generally, use of && in test_when_finished is a contradiction. Even if other things fail, you want "command" to be gone befor ethe next step, and that is why you use test_when_finished to arrange it to be cleaned. Similarly, even if "git reset command" fails here, you would want command to be cleaned.
What is the expected state of the test repository before we start this test? If it is that "git status -uno" would be silent (i.e., no paths known to the index and the HEAD are modified in the index or in the working tree, but there may be random untracked cruft left in the working tree, like "expect" and stuff), then it makes more sense to make it easier to read that expectation by doing
test_when_finished "git reset --hard; rm -f command"
here. Instead of pretending that we care about making the minimum damage by selectively resetting the index, we make it clear that we are giving the _next_ test a pristine state of the index and the working tree as we inherited from the previous test.
Show 28 quoted lines
> + 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 @@Otherwise, looking very good. Will queue.
Thanks.