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

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

On Fri, 25 Sep 2020 at 09:09, Jeff King [off-list ref] wrote:

quoted

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

quoted

        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