Re: [PATCH v2 2/6] shortlog: make trailer insertion a noop when appropriate

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 21, 2022, 21:57 UTC
Message-ID: <Y1MV4DEa8y0I5W/8@nand.local>
In-Reply-To: <Y1IwVIoVsl1FVklI@coredump.intra.peff.net>

On Fri, Oct 21, 2022 at 01:38:28AM -0400, Jeff King wrote:

Show 19 quoted lines

> On Thu, Oct 20, 2022 at 11:11:32PM -0400, Taylor Blau wrote:
>
> > When there are no trailers to insert, it is natural that
> > insert_records_from_trailers() should return without having done any
> > work.
> >
> > But instead we guard this call unnecessarily by first checking whether
> > `log->groups` has the `SHORTLOG_GROUP_TRAILER` bit set.
> >
> > Prepare to match a similar pattern in the future where a function which
> > inserts records of a certain type does no work when no specifiers
> > matching that type are given.
>
> The patch looks good. And knowing what the rest of the series looks
> like, this is clearly the right thing to do. But I wonder if the
> rationale would be easier if it came at the end. Then rather than
> "prepare to match a similar pattern in the future", you can just say
> "it's weird that we check the bit for SHORTLOG_GROUP_TRAILER but nothing
> else; let's make them consistent".

Hmm. I see what you're saying, but in fact by this point in the series there are two other spots in shortlog_add_commit() that check whether individual bits are set (one for SHORTLOG_GROUP_AUTHOR, and another for SHORTLOG_GROUP_COMMITTER).

So I think that the semi-awkward commit message may in fact be the least awkward thing to do here ;-).

Thanks, Taylor