On Wed, Apr 12, 2023 at 02:31:18AM -0400, Jeff King wrote:
> 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
