Subject: Re: [PATCH] add-patch: response to invalid option

From: [email-masked]
Date: Thu, 18 Apr 2024 16:11:33 +0100

On 17/04/2024 16:05, Junio C Hamano wrote:
> [email-masked] writes:
> 
>> I tend to associate "option" with a command-line argument, not
>> interactive input to a program.
> 
> "git add --help" is a bit mixed.  The choices offered by "git add
> -i" are called "subcommand" (see "INTERACTIVE MODE" section), but
> the choices you give to the prompt "patch" subcommand gives you are
> presented with "You can select one of the following options and type
> return".  So "option" is not too wrong, even though it is a word
> used in other contexts as well.  I am OK with "option", but if I
> were adding this new error message, I probably would have said
> "unknown command".

I think "unknown command" is a good suggestion, I take your point about 
"unknown key" not being so clear for users who do not use single-key input.

Best Wishes

Phillip

> In any case, whether you said option, command, or key , it is so
> obvious from the context that we could even say "error: 'W' not
> known, use '?' for help" without any noun there, so it would not
> matter too much which noun you pick.
> 
> I'd still avoid "key", though, because to those who do not do
> single-key input, myself included, it does not match their user
> experience, and it is even more so if they forgot or do not even
> know that they could choose to use single-key input.
> 
>> The test you are proposing only tests the last of these changes. We
>> should be aiming to write tests that (a) verify all of the changes
>> introduced by a commit (b) are likely to detect regressions to those
>> changes (c) are reasonably efficient, for example if it is possible to
>> test more than one key with a single "add -p" process we should do
>> so. As this is an interactive program I have a strong preference for
>> testing what the user sees printed to their screen, not just what
>> happens to come out on stderr.
> 
> I do agree with these three points, but I do not have a strong
> opinion on the new test that was added by the patch when judging
> with them used as a yardstick.
> 
> Thanks.