On Thu, Oct 20, 2022 at 10:02:45PM -0400, Taylor Blau wrote:
> > > > - if (log->groups & SHORTLOG_GROUP_AUTHOR) {
> > > > - strbuf_reset(&ident);
> > > > - format_commit_message(commit,
> > > > - log->email ? "%aN <%aE>" : "%aN",
> > > > - &ident, &ctx);
> > > > - if (!HAS_MULTI_BITS(log->groups) ||
> > > > - strset_add(&dups, ident.buf))
> > > > - insert_one_record(log, ident.buf, oneline_str);
> > > > - }
> > >
> > > This loses the HAS_MULTI_BITS() optimization. The idea there is that if
> > > you have a single group-by that can't produce multiple outputs, then
> > > there's no need to do duplicate detection.
> > >
> > > The equivalent in an all-formats world is something like:
> > >
> > > log.format.nr > 1 && !log.trailers.nr
> > >
> > > (because trailers are special in that one trailer key can produce
> > > multiple idents for a single commit).
>
> Hmm. Shouldn't that "&& !log.trailers.nr" be an "|| log.trailers.nr"? It
> doesn't seem to make sense to say "there are things that could produce
> multiple outputs" if there's more than one format _and_ no trailers.
Yeah. I was thinking of it as "is it OK to not de-dup", but of course it
is the other way around because of the "!". And regardless of which way
you write the conditional, the two sides must agree. ;)
> The logic should read "there are things that could produce multiple
> outputs if there is more than one format *or* at least one trailer".
>
> So I think the right change would be:
>
> --- >8 ---
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index 95ceab7649..7e1b56e2aa 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -216,7 +216,8 @@ static void insert_records_from_format(struct shortlog *log,
>
> format_commit_message(commit, item->string, &buf, ctx);
>
> - if (strset_add(dups, buf.buf))
> + if (!(log->format.nr > 1 || log->trailers.nr) ||
> + strset_add(dups, buf.buf))
> insert_one_record(log, buf.buf, oneline);
> }
> --- 8< ---
>
> Yeah?
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.
Regardless, what you wrote above is correct. Thanks for catching it.
-Peff
