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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 22, 2022, 17:16 UTC
Message-ID: <CAPig+cRq7H2bnkcU-V5uiWA9z=FLvxj3ji0bhO3DMX9HfptHtQ@mail.gmail.com>
In-Reply-To: <pull.1147.v2.git.1645545507689.gitgitgadget@gmail.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/

Show 8 quoted lines

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

Show 11 quoted lines

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