From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sun, 12 Sep 2021 04:45:50 GMT
Subject: Re: [PATCH v3 2/6] blame: replace usage end blurb with better option spec
Message-ID: <CAPig+cRzzMEFKMEW1dMB=Y++kuoFUnLgRatV7_hFZ5BnSbpD+A@mail.gmail.com>
In-Reply-To: <patch-v3-2.6-036eb0efb5b-20210911T190239Z-avarab@gmail.com>

On Sat, Sep 11, 2021 at 3:10 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Change the "git blame -h" output to be consistent with "git bundle
> -h"'s, i.e. before this we'd emit:

Just a couple tiny, tiny nits (which may or may not be worth a re-roll)...

>     $ git blame -h
>     usage: git blame [<options>] [<rev-opts>] [<rev>] [--] <file>
>
>         <rev-opts> are documented in git-rev-list(1)
>     [...]
>
> Now instead of that we'll emit:
>
>     $ git blame -h
>     usage: git blame [<options>] [<git rev-list args>] [<rev>] [--] <file>

This is lacking a hyphen between `git` and `rev-list`...

> This makes it consistent with the usage spec used for "git bundle":
>
>     $ git bundle -h
>     usage: git bundle create [<options>] <file> <git-rev-list args>

...whereas this has a hyphen between the two.

> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/builtin/blame.c b/builtin/blame.c
> @@ -29,12 +29,8 @@
> -       N_("<rev-opts> are documented in git-rev-list(1)"),
> +       N_("git blame [<options>] [<git rev-list args>] [<rev>] [--] <file>"),

Ditto regarding missing hyphen.

> @@ -1107,7 +1103,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>                                     nth_line_cb, &sb, lno, anchor,
>                                     &bottom, &top, sb.path,
>                                     the_repository->index))
> -                       usage(blame_usage);
> +                       usage_msg_opt(_("Invalid -L <range> parameter"),
> +                                     blame_opt_usage, options);

builltin/blame.c seems to be pretty consistent about starting error
and warning messages with a lowercase letter, so this perhaps should
follow suit. Also, I think you can drop "parameter" without losing
clarity:

    invalid -L range

would likely be good enough.

>                 if ((!lno && (top || bottom)) || lno < bottom)
>                         die(Q_("file %s has only %lu line",
>                                "file %s has only %lu lines",