Re: [PATCH 5/7] shortlog: implement `--group=author` in terms of `--group=<format>`

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 21, 2022, 22:05 UTC
Message-ID: <Y1MXqATtx5OTQtNU@nand.local>
In-Reply-To: <Y1IoLBvVfo+pIC+6@coredump.intra.peff.net>

On Fri, Oct 21, 2022 at 01:03:40AM -0400, Jeff King wrote:

Show 12 quoted lines

> Right. I wondered if it might be a little clearer to drop the outer "!",
> which yields:
>
>   if ((log->format.nr <= 1 && !log->trailers.nr) ||
>       strset_add(dups, buf.buf))
>
> but it is not really any less confusing. If we gave that first part of
> the conditional a name, like:
>
>   if (!needs_dedup(log) || strset_add(dups, buf.buf))
>
> maybe that is better. I dunno.

Yeah, I think that this is best. I briefly wondered whether it would be better to invert the check _and_ name it, but `if (does_not_need_dedup(log))` is too wordy.

> Regardless, what you wrote above is correct. Thanks for catching it.

No problem, it was an honest mistake.

Thanks, Taylor