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

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 21, 2022, 02:39 UTC
Message-ID: <Y1IGeuudJj18sDPz@nand.local>
In-Reply-To: <Y0TOpVF+Y70YJHzx@coredump.intra.peff.net>

On Mon, Oct 10, 2022 at 10:02:13PM -0400, Jeff King wrote:

Show 11 quoted lines

> > >   - 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.
>
> True, if we follow through on that. ;)

So, obviously we ended up not following through on that ;-).

I tried to leverage the existing test as much as possible, to the point that the new test is pretty hacky. But I think that the result is cute, and it does save us from having to modify the downstream tests (or create unreachable commits, initialize another repository, etc).

It looks something like this:

--- 8< ---

diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 0abe5354fc..566d581e1b 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -356,6 +356,19 @@ test_expect_success 'shortlog can match multiple groups' '
 	test_cmp expect actual
 '

+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,separator=%0x00,key=some-trailer)" \
+		--group="%(trailers:valueonly,separator=%0x00,key=another-trailer)" \
+		-2 HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'set up option selection tests' '
 	git commit --allow-empty -F - <<-\EOF
 	subject
--- >8 ---

The gross bit there really is the 'separator=%0x00' thing, since the
"trailers" pretty format gives us a NL character already. I suppose that
you could do something like this on top instead:

--- >8 ---
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 566d581e1b..13ac0bac64 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -363,9 +363,10 @@ test_expect_success 'shortlog can match multiple format groups' '
 	     1	User A <a@example.com>
 	EOF
 	git shortlog -ns \
-		--group="%(trailers:valueonly,separator=%0x00,key=some-trailer)" \
-		--group="%(trailers:valueonly,separator=%0x00,key=another-trailer)" \
-		-2 HEAD >actual &&
+		--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
 '
--- 8< ---

which I think I prefer slightly.

If this is all too hacky for your (or anybody's!) taste, let me know.
But I think ultimately this results in a smaller, more easily digestible
diff overall.

Thanks,
Taylor