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
