Patrick Steinhardt <ps@xxxxxx> writes:
> On Tue, Aug 06, 2024 at 09:15:28AM -0700, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@xxxxxx> writes:
>>
>> >> + if (argc)
>> >> + usage(_("'git refs verify' takes no arguments"));
>> >
>> > Junio has posted a patch series [1] where he wants to get rid of
>> > messages that simply say "no arguments" or "too many arguments".
>> > ...
>> > So I'd propose to make this:
>> >
>> > argc = parse_options(argc, argv, prefix, options, verify_usage, 0);
>> > if (argc)
>> > usage(_("unknown argument: '%s'", argv[0]));
>>
>> I probably should have said that I am fully behind the intent
>> against "too many arguments", but I am not 100% behind the
>> particular messaging used in the patch series I sent out.
>>
>> One potential complaint I expected to hear, for example, was that "a
>> is unknown" given when you said "git cmd a a a a a" is not all that
>> clear ;-). To alleviate, you would have to say "git cmd takes only
>> 2 arguments" if 'a' you are complaining about is the third one.
>>
>> Also, many people would consider that "unexpected argument" is
>> better than "unknown argument".
>>
>> I personally think the message above is absolutely clear and good.
>>
>> You say that 'git refs verify' takes no arguments, and for somebody
>> who said "git refs verify a b c d e", there is no doubt that all of
>> these a b c d e are unwanted. And there is no room to misinterpret
>> the message as "'git refs' is ok but 'git refs verify' is already
>> unwelcome with extra argument", either [*].
>>
>> In short, I think the message in the patch here is good, and it is
>> the other "war on 'too many arguments'" series whose messages need
>> to be thought further.
>
> Just to clarify: with "the patch" you probably refer to the current
> version that Jialuo has, right? In other words, keep the current version
> that he has and adapt the message in the future, when we have decided
> what to do about those "too many arguments" messages?
I meant that I think (1) that "'git refs verify' takes no arguments"
is a good message, and (2) that there is no further change needed to
the patch that started this review thread, regardless of how we want
to deal with "too many arguments" messages.
> If so, then the only two I had were some spurious newlines. I'm not sure
> whether these really would be worth rerolling the whole patch series.
Yup, those blank lines were annoying while scanning the patches, but
they alone would not be something that makes a reroll _required_. A
reroll that clearly shows that the incremental change since the last
round is only these blank line changes is not too much to process,
so "not worth the reviewer time" is not a huge reason to avoid it,
either. I'd see that it is up to how perfectionist the patch
submitter wants to be ;-)
Thanks.
