Re: [PATCH v3 0/5] Skip 'cache_tree_update()' when 'prime_cache_tree()' is called immediate after

From: Victoria Dye <vdye@github.com>
Date: Nov 10, 2022, 20:54 UTC
Message-ID: <9c3b71d2-6481-e702-329c-33ee988dd7ce@github.com>
In-Reply-To: <20221110195029.GD1159673@szeder.dev>

SZEDER Gábor wrote:

Show 19 quoted lines

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

Show 6 quoted lines

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