Re: [PATCH 2/8] shortlog: refactor committer/author grouping

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 25, 2020, 20:05 UTC
Message-ID: <CAPig+cTfYmNAP_65RDa_fZOyuQEH65HuLs-UYSPT0yJ=s6BftA@mail.gmail.com>
In-Reply-To: <20200925070211.GB62741@coredump.intra.peff.net>

On Fri, Sep 25, 2020 at 3:02 AM Jeff King <peff@peff.net> wrote:

Show 24 quoted lines

> In preparation for adding more grouping types, let's
> refactor the committer/author grouping code. In particular:
>
>   - the master option is now "--group", to make it clear
>     that the various group types are mutually exclusive. The
>     "--committer" option is an alias for "--group=committer".
>
>   - we keep an enum rather than a binary flag, to prepare
>     for more values
>
>   - we prefer switch statements to ternary assignment, since
>     other group types will need more custom code
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt
> @@ -47,9 +47,18 @@ OPTIONS
> +--group=<type>::
> +       By default, `shortlog` collects and collates author identities;
> +       using `--group` will collect other fields.
> +       `<type>` is one of:
> ++
> + - `author`, commits are grouped by author (this is the default)
> + - `committer`, commits are grouped by committer (the same as `-c`)

I had trouble interpreting "(this is the default)". It made me think that <type> is optional:

    --group[=<type>]

but that isn't the case at all. Instead, it means that if --group isn't specified, then grouping is done by `author` by default. It also repeats what the general description of --group already says with regard to the default, thus it is redundant to say it again when describing the `author` type. Therefore, perhaps drop "(this is the default)" altogether?