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

[Junio C Hamano <gitster@pobox.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.

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

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

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

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

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