On Thu, Oct 20, 2022 at 11:11:41PM -0400, Taylor Blau wrote: > 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: > 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
