SZEDER Gábor wrote:

On Thu, Nov 10, 2022 at 07:06:00PM +0000, Victoria Dye via GitGitGadget wrote:

quoted

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

quoted

     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.