Re: [PATCH v3 4/7] shortlog: support arbitrary commit format `--group`s

From: Jeff King <peff@peff.net>
Date: Oct 22, 2022, 00:28 UTC
Message-ID: <Y1M5OH/FiWNaKO6p@coredump.intra.peff.net>
In-Reply-To: <f75a952421ee1ab531d9977252d6594346a94583.1666391136.git.me@ttaylorr.com>

On Fri, Oct 21, 2022 at 06:25:49PM -0400, Taylor Blau wrote:

Show 8 quoted lines

> @@ -203,6 +205,32 @@ static void insert_records_from_trailers(struct shortlog *log,
>  	unuse_commit_buffer(commit, commit_buffer);
>  }
>  
> +static int shortlog_needs_dedup(const struct shortlog *log)
> +{
> +	return log->format.nr > 1 || log->trailers.nr;
> +}

So this is obviously much nicer to read. But I think there might be a weird logic error during the series. Since we haven't yet folded SHORTLOG_GROUP_AUTHOR, etc, into the "format" code, I think at this point it still needs:

  HAS_MULTI_BITS(log->groups)

at the beginning of the OR-chain. Otherwise "shortlog --group=author --format=%an" would fail to dedup, I think.

It's hard to care too much, since the end result of the series is correct, and you'd end up just removing that part of the line in the final patch. So I could go either way on re-rolling.

Sorry for not catching this in the last round.

-Peff