Re: [GSoC][PATCH v15 7/9] builtin/refs: add verify subcommand

Patrick Steinhardt [off-list ref] writes:

On Tue, Aug 06, 2024 at 09:15:28AM -0700, Junio C Hamano wrote:

quoted

Patrick Steinhardt [off-list ref] writes:

quoted

quoted

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