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

[Jeff King <peff@peff.net>]

On Thu, Oct 20, 2022 at 11:11:38PM -0400, Taylor Blau wrote:

> +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.

> 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...

> +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. :)

> +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