Re: [PATCH 8/8] shortlog: allow multiple groups to be specified

From: Martin Ågren <martin.agren@gmail.com>
Date: Sep 26, 2020, 12:48 UTC
Message-ID: <CAN0heSp5ssE1_-0B4VMGcuOaW63EpWXRVp4E4gy_7zVZ1rWkew@mail.gmail.com>
In-Reply-To: <20200925070550.GH62741@coredump.intra.peff.net>

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

> +static inline int has_multi_bits(unsigned n)
> +{
> +       return (n & (n - 1)) != 0;
> +}

There's a HAS_MULTI_BITS macro in git-compat-util.h. I like how you came up with the exact same name. It even makes me wonder if you are actively avoiding it in favor of a re-implementation, but I can't see why.

Show 9 quoted lines

>         enum {
> -               SHORTLOG_GROUP_AUTHOR = 0,
> -               SHORTLOG_GROUP_COMMITTER,
> -               SHORTLOG_GROUP_TRAILER
> -       } group;
> +               SHORTLOG_GROUP_AUTHOR = (1 << 0),
> +               SHORTLOG_GROUP_COMMITTER = (1 << 1),
> +               SHORTLOG_GROUP_TRAILER = (1 << 2)
> +       } groups;

Coming back to the comment on 2/8, adding a comma at the end wouldn't reduce patch noise here and now, but whenever someone touches this place the next time.

Of my comments in the last few mails, probably the only actionable one is the truncated sentence in the trailer iterator header file.

Martin