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

From: Jeff King <peff@peff.net>
Date: Oct 21, 2022, 05:50 UTC
Message-ID: <Y1IzQprA+QQFQXGz@coredump.intra.peff.net>
In-Reply-To: <f0044682be70f876f6f92455f1fa46dd2e8aa09f.1666320509.git.me@ttaylorr.com>

On Thu, Oct 20, 2022 at 11:11:41PM -0400, Taylor Blau wrote:

Show 7 quoted lines

> Instead of handling SHORTLOG_GROUP_AUTHOR separately, reimplement it as
> a special case of the new `--group=<format>` mode, where the author mode
> is a shorthand for `--group='%aN <%aE>'.
> 
> Note that we still need to keep the SHORTLOG_GROUP_AUTHOR enum since it
> has a different meaning in `read_from_stdin()`, where it is still used
> for a different purpose.

Yep, makes sense. The implementation all looks correct to me, although:

Show 11 quoted lines

> diff --git a/builtin/log.c b/builtin/log.c
> index ee19dc5d45..b4d5420217 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1334,6 +1334,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
>  	log.in2 = 4;
>  	log.file = rev->diffopt.file;
>  	log.groups = SHORTLOG_GROUP_AUTHOR;
> +	shortlog_finish_setup(&log);
>  	for (i = 0; i < nr; i++)
>  		shortlog_add_commit(&log, list[i]);

I'd probably have pulled out this "finish_setup()" stuff into its own patch. I can live with it either way, though. The advantage of having it here is that it becomes obvious why this caller needs it (whereas if done as a preparatory patch, it would just do the trailer sorting, which make_cover_letter() obviously does not need).

-Peff