Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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 <chakrabortyabhradeep79@xxxxxxxxx>
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@xxxxxxxxx>".
>> ---
>> 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));
