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

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Feb 22, 2022 at 11:27 AM Abhradeep Chakraborty via
GitGitGadget <gitgitgadget@gmail.com> wrote:
> Usage strings for git (sub)command flags has a style guide that
> suggests - first letter should not capitalized (unless requied)

s/requied/required/

> and it should skip full-stop at the end of line. But there are
> some files where usage-strings do not follow the above mentioned
> guide. Moreover, there are no checks to verify if all usage strings
> are following the guide/convention or not.
>
> Amend the usage strings that don't follow the convention/guide and
> add a check in the `parse_options_check()` function in `parse-options.c`
> to check the usage strings against the style guide.

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.

> Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
> ---
> 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])))

> +               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));