SZEDER Gábor wrote:
> On Thu, Nov 10, 2022 at 07:06:00PM +0000, Victoria Dye via GitGitGadget wrote:
>> Changes since V2
>> ================
>>
>> * Cleaned up option handling & provided more informative error messages in
>> 'test-tool cache-tree'. The changes don't affect any behavior in the
>> added tests & 'test-tool cache-tree' won't be used outside of
>> development, but the improvements here will help future readers avoid
>> propagating error-prone implementations.
>> * Note that the suggestion to change the "unknown subcommand" error to a
>> 'usage()' error was not taken, as it would be somewhat cumbersome to
>> use a formatted string with it.
>
> I'm not sure I understand what's cumbersome. It's as simple as:
>
> if (...) {
> error(_("unknown subcommand: `%s'"), argv[0]);
> usage_with_options(test_cache_tree_usage, options);
> }
To be honest, the cumbersome approach I was thinking of was 'sprintf()'-ing
the subcommand into the string and calling 'usage()' with that - your
suggestion is certainly much simpler. However, as a matter of personal
preference, I still think the 'die()' is sufficient in the context of this
test helper (especially given that other test helpers do the same).
>
>> This is in line with other custom
>> subcommand parsing in Git, such as in 'fsmonitor--daemon.c'.
>
> The option parsing in 'fsmonitor--daemon.c' is broken, please don't
> consider it as an example to follow.
While I understand your desire to helpfully guide users, I don't see
anything to suggest that particular example is "broken." The error conveys
the cause of the problem to a user, who could then run without arguments (or
with -h) to see what the valid subcommands are. And, in the case of this
test helper, I'm not particularly concerned with perfecting the (already
subjective) user experience, given that it's an internal-only tool.
If there are examples of proper usage patterns that future commands should
follow, I'd recommend updating 'CodingGuidelines' and/or
'MyFirstContribution' to mention them. Codifying recommendations like that
can help avoid churn in reviews and, long-term, push Git to align on a
uniform style.
