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

From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
Date: Feb 23, 2022, 11:59 UTC
Message-ID: <20220223115925.4081-1-chakrabortyabhradeep79@gmail.com>
In-Reply-To: <CAPig+cRq7H2bnkcU-V5uiWA9z=FLvxj3ji0bhO3DMX9HfptHtQ@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> wrote:

> s/requied/required/

Thanks for pointing out. Will fix it.

Show 5 quoted lines

> 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. Anyhow, not necessarily worth a reroll.

I made a single commit because both changes are small. But I think You're right - I should split it into two.

> 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()?

Hmm, this was a quick fix came into my mind. So, I didn't look at other (and better) options for this. Will fix it :)

Show 9 quoted lines

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

Okay, got it!

Thanks :)