Re: [PATCH 3/7] t5512: stop using jgit for capabilities^{} test

From: Jeff King <peff@peff.net>
Date: Apr 12, 2023, 09:04 UTC
Message-ID: <20230412090423.GA2187240@coredump.intra.peff.net>
In-Reply-To: <20230412063118.GC1681312@coredump.intra.peff.net>

On Wed, Apr 12, 2023 at 02:31:18AM -0400, Jeff King wrote:

Show 17 quoted lines

> Commit eb398797cd (connect: advertized capability is not a ref,
> 2016-09-09) added support for an upload-pack server responding with:
> 
>   0000000000000000000000000000000000000000        capabilities^{}
> 
> followed by a NUL and capabilities. This is not something Git itself has
> ever produced for upload-pack, but JGit does. And hence the test used
> JGit to reproduce the real-world situation. That was good for verifying
> that the incompatibility was fixed, but it's a lousy regression test for
> a few reasons:
> 
>   - hardly anybody runs it, because you have to have jgit installed
> 
>   - we're depending on jgit's behavior for the test to do anything
>     useful. In particular, this behavior is only relevant to the v0
>     protocol, but these days we ask for the v2 protocol by default. So
>     for modern jgit, this is probably testing nothing.

I was worried that changing this one might be churn. But as it turns out, it reveals that there's a bug in the code which we've been missing all this time because nobody was running the test!

When run with GIT_TEST_DEFAULT_HASH=sha256, this will fail because the code uses the local idea of the hash algorithm, rather than what the other side advertises. We have a linux-sha256 CI job, but of course it didn't have jgit installed, so it always skipped this test (until my patch, where it now reveals the bug).

The fix is just:

diff --git a/connect.c b/connect.c
index 66397cc911..c54adc652f 100644
--- a/connect.c
+++ b/connect.c
@@ -263,7 +263,8 @@ static int process_dummy_ref(const struct packet_reader *reader)
 		return 0;
 	name++;
 
-	return oideq(null_oid(), &oid) && !strcmp(name, "capabilities^{}");
+	return oideq(reader->hash_algo->null_oid, &oid) &&
+		!strcmp(name, "capabilities^{}");
 }
 
 static void check_no_capabilities(const char *line, int len)

I'll squash that in and update the commit message before I do the next
re-roll (but will still hold off a bit to get further comments).

-Peff