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

[Jeff King <peff@peff.net>]

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

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

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