Re: [PATCH v2 4/6] shortlog: support arbitrary commit format `--group`s
- From
Jeff King <peff@peff.net>
- Date
- Oct 21, 2022, 05:48 UTC
- Message-ID
- <Y1IyppKatD7piHOO@coredump.intra.peff.net>
- In-Reply-To
- <4a36c0ca4e840d53e2fd257e2d97498ced6fb28c.1666320509.git.me@ttaylorr.com>
On Thu, Oct 20, 2022 at 11:11:38PM -0400, Taylor Blau wrote:
Show 18 quoted lines
> +static void insert_records_from_format(struct shortlog *log,
> + struct strset *dups,
> + struct commit *commit,
> + struct pretty_print_context *ctx,
> + const char *oneline)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + struct string_list_item *item;
> +
> + for_each_string_list_item(item, &log->format) {
> + strbuf_reset(&buf);
> +
> + format_commit_message(commit, item->string, &buf, ctx);
> +
> + if (!(log->format.nr > 1 || log->trailers.nr) ||
> + strset_add(dups, buf.buf))
> + insert_one_record(log, buf.buf, oneline);
> + }Just to be clear, since I talked about this conditional in the other thread: what you have here is correct, and now seeing it again in context, I think it's OK in terms of readability.
Show 17 quoted lines
> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh > index 7547da539d..13ac0bac64 100755 > --- a/t/t4201-shortlog.sh > +++ b/t/t4201-shortlog.sh > @@ -244,6 +244,36 @@ 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="format:%cN (%cd)" \ > + HEAD >actual && > + cat >expect <<-\EOF && > + 4 C O Mitter (2005) > + 1 Sin Nombre (2005) > + EOF > + test_cmp expect actual > +'
OK, so this is a basic test of the feature, and makes sure the --date support works.
> +test_expect_success 'shortlog --group=<format> DWIM' ' > + git shortlog -s --date="format:%Y" --group="%cN (%cd)" HEAD >actual && > + test_cmp expect actual > +'
And this is the same thing but without "format:". Good...
Show 9 quoted lines
> +test_expect_success 'shortlog multiple --group=format' ' > + git shortlog -s --date="format:%Y" --group="format:%cN (%cd)" \ > + HEAD >actual && > + cat >expect <<-\EOF && > + 4 C O Mitter (2005) > + 1 Sin Nombre (2005) > + EOF > + test_cmp expect actual > +'
...but this one seems redundant. It is not using multiple formats. And you test that later, in the "can match multiple format groups" test. Is this one just leftover from development?
> +test_expect_success 'shortlog bogus --group' ' > + test_must_fail git shortlog --group=bogus HEAD 2>err && > + grep "unknown group type" err > +'
This one is a nice inclusion. I was surprised we didn't have it already. :)
Show 13 quoted lines
> +test_expect_success 'shortlog can match multiple format groups' ' > + cat >expect <<-\EOF && > + 2 User B <b@example.com> > + 1 A U Thor <author@example.com> > + 1 User A <a@example.com> > + EOF > + git shortlog -ns \ > + --group="%(trailers:valueonly,key=some-trailer)" \ > + --group="%(trailers:valueonly,key=another-trailer)" \ > + -2 HEAD >actual.raw && > + grep -v "^$" actual.raw >actual && > + test_cmp expect actual > +'
And this one is correct, though I think avoiding trailers is more to the point; see the other mail I sent.
-Peff