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

[Taylor Blau <me@ttaylorr.com>]

On Mon, Oct 10, 2022 at 09:48:47PM -0400, Taylor Blau wrote:
> > > diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> > > index f708d96558..aac8c7afa4 100644
> > > --- a/builtin/shortlog.c
> > > +++ b/builtin/shortlog.c
> > > @@ -245,15 +245,6 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
> > >  	}
> > >  	oneline_str = oneline.len ? oneline.buf : "<none>";
> > >
> > > -	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.

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?

Thanks,
Taylor