Re: [PATCH 5/7] shortlog: implement `--group=author` in terms of `--group=<format>`
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Oct 21, 2022, 02:02 UTC
- Message-ID
- <Y1H9xdDJS+xKW8mS@nand.local>
- In-Reply-To
- <Y0TLf/J22ioQ5UCt@nand.local>
On Mon, Oct 10, 2022 at 09:48:47PM -0400, Taylor Blau wrote:
Show 28 quoted lines
> > > 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