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

From: Jeff King <peff@peff.net>
Date: Oct 21, 2022, 05:03 UTC
Message-ID: <Y1IoLBvVfo+pIC+6@coredump.intra.peff.net>
In-Reply-To: <Y1H9xdDJS+xKW8mS@nand.local>

On Thu, Oct 20, 2022 at 10:02:45PM -0400, Taylor Blau wrote:

Show 24 quoted lines

> > > > -	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. ;)

Show 22 quoted lines

> 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