Subject: Re: [PATCH v2] add usage-strings check and amend remaining usage strings

From: Junio C Hamano <[email-masked]>
Date: Wed, 23 Feb 2022 13:17:39 -0800

Eric Sunshine <[email-masked]> writes:

> This is a relatively minor observation, but it might make sense to
> split this into two patches, the first of which fixes the offending
> usage strings, and the second which adds the check to parse-options.c
> to prevent more offending strings from entering the project in the
> future.

Yeah, that sounds like a quite sensible split.

I notice that the real-looking name

>> Signed-off-by: Abhradeep Chakraborty <[email-masked]>

does not match with the in-body "From:" that has less real-looking.
Please fix the in-body "From:" if this is rerolled so that both
mention the same "Human Readable Name <[email-masked]>".

>> ---
>> diff --git a/parse-options.c b/parse-options.c
>> @@ -492,6 +492,15 @@ static void parse_options_check(const struct option *opts)
>> +               if (opts->type != OPTION_GROUP && opts->help &&
>> +                       !(starts_with(opts->help, "HEAD") ||
>> +                         starts_with(opts->help, "GPG") ||
>> +                         starts_with(opts->help, "DEPRECATED") ||
>> +                         starts_with(opts->help, "SHA1")) &&
>> +                         (opts->help[0] >= 65 && opts->help[0] <= 90))
>> +                       err |= optbug(opts, xstrfmt("help should not start with capital letter unless needed: %s", opts->help));
>
> This list of hardcoded exceptions may become a maintenance burden. I
> can figure out why OPTION_GROUP is treated specially here, but why use
> magic numbers 65 and 90 rather than a more obvious function like
> isupper()?
>
> Perhaps instead of hardcoding an exception list and magic numbers, we
> can use a simple heuristic instead. For instance, if the first two
> characters of the help string are uppercase, then assume it is an
> acronym (i.e. "GPG") or special name (i.e. "HEAD"), thus allowed.
> Maybe something like this:
>
>     if (opts->type != OPTION_GROUP && opts->help &&
>         opts->help[0] && isupper(opts->help[0]) &&
>         !(opts->help[1] && isupper(opts->help[1])))
>

Much better than what was posted, but such a heuristic deserves some
in-code comment to check why we see the first two.

>> +               if (opts->help && !ends_with(opts->help, "...") && ends_with(opts->help, "."))
>> +                       err |= optbug(opts, xstrfmt("help should not end with a dot: %s", opts->help));