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

[Taylor Blau <me@ttaylorr.com>]

On Mon, Oct 10, 2022 at 09:12:46PM -0400, Jeff King wrote:
> On Mon, Oct 10, 2022 at 08:34:12PM -0400, Taylor Blau wrote:
>
> > diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt
> > index 4982ceee21..ca15643f45 100644
> > --- a/Documentation/git-shortlog.txt
> > +++ b/Documentation/git-shortlog.txt
> > @@ -59,6 +59,8 @@ OPTIONS
> >     example, if your project uses `Reviewed-by` trailers, you might want
> >     to see who has been reviewing with
> >     `git shortlog -ns --group=trailer:reviewed-by`.
> > + - `<format>`, any string accepted by the `--format` option of 'git log'.
> > +   (See the "PRETTY FORMATS" section of linkgit:git-log[1].)
>
> I have a slight preference here to call this:
>
>   --group=format:<format>

That seems very reasonable, thanks!

> > @@ -243,6 +266,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
> >  	if (log->groups & SHORTLOG_GROUP_TRAILER) {
> >  		insert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);
> >  	}
> > +	insert_records_from_format(log, &dups, commit, &ctx, oneline_str);
>
> Hmm. I see why we don't need to guard this with:
>
>   if (log->groups & SHORTLOG_GROUP_FORMAT)
>
> since our helper function is a noop if log->format is empty. But I
> wonder if:
>
>   - it's more clear to match the others (although it looks like they are
>     going away later, so that potentially becomes a non-issue)
>
>   - it's useful to have a conditional that lets us skip any setup work.
>     For trailers, in particular, we call logmsg_reencode(), which is
>     potentially expensive. OTOH, it would be easy for the helper
>     function to just return early when log->format.nr is 0.

In this case, `insert_records_from_format()` is cheap when
log->format.nr is 0. It is limited to setting up a strbuf to
STRBUF_INIT, and then calling strbuf_release() on it before returning.

And, indeed, the remaining conditionals go away by the final patch, so
you may want to decide then if it looks good to you or not.

> > diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
> > index 3095b1b2ff..2417ac8896 100755
> > --- a/t/t4201-shortlog.sh
> > +++ b/t/t4201-shortlog.sh
> > @@ -237,6 +237,15 @@ test_expect_success 'shortlog --group=trailer:signed-off-by' '
> >  	test_cmp expect actual
> >  '
> >
> > +test_expect_success 'shortlog --group=<format>' '
> > +	git shortlog -s --date="format:%Y" --group="%cN (%cd)" HEAD >actual &&
> > +	cat >expect <<-\EOF &&
> > +	     4	C O Mitter (2005)
> > +	     1	Sin Nombre (2005)
> > +	EOF
> > +	test_cmp expect actual
> > +'
>
> Makes sense. Two other tests that might be worth including:
>
>   - "shortlog --group=bogus" generates an error (we might already have
>     such a test; I didn't check)

We didn't have such a test before, so adding one is a good call, thanks!

>   - since the multiple-option behavior is so subtle, maybe show a case
>     where two formats partially overlap. A plausible one is "--group=%aN
>     --group=%cN", but the test setup might need tweaked to cover both.
>     There's an existing "multiple groups" test that might come in handy.

Interesting. I was starting to write that test up, but then realized
that this will be covered by the end of the series, since the
`--group=trailer` machinery is reimplemented in terms of the new format
group.

So if the existing "shortlog can match multiple groups" test keeps
passing, we did our job correctly ;-).

Thanks,
Taylor