On Fri, Oct 21, 2022 at 01:38:28AM -0400, Jeff King wrote: > 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
