Re: [PATCH 4/7] shortlog: support arbitrary commit format `--group`s
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Oct 11, 2022, 01:24 UTC
- Message-ID
- <Y0TF0M6UzLS9r6iM@nand.local>
- In-Reply-To
- <Y0TDDvzeCxIMFbG5@coredump.intra.peff.net>
On Mon, Oct 10, 2022 at 09:12:46PM -0400, Jeff King wrote:
Show 16 quoted lines
> 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!
Show 20 quoted lines
> > @@ -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.
Show 21 quoted lines
> > 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