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

From: Jeff King <peff@peff.net>
Date: Sep 27, 2020, 08:25 UTC
Message-ID: <20200927082503.GE1286220@coredump.intra.peff.net>
In-Reply-To: <CAN0heSp5ssE1_-0B4VMGcuOaW63EpWXRVp4E4gy_7zVZ1rWkew@mail.gmail.com>

On Sat, Sep 26, 2020 at 02:48:44PM +0200, Martin Ågren wrote:

Show 9 quoted lines

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

Heh. I _thought_ we had such a helper, and I even thought I remembered the name. But when I grepped for it, I couldn't come up with it. Silly "-i". (So I guess it's no surprise that I came up with the same implementation, which is the only good one, and the same name, which I did pull from the back of my mind).

I'll drop this in favor of the macro.

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

Agreed, will do.

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

I agree none are big, but worth addressing since I was making a re-roll anyway. Thanks.

-Peff