| Message ID | 20230412063118.GC1681312@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | v0 multiple-symref infinite loop fix and test cleanup | expand |
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
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh index ce7a9f1594..4e21ab60bf 100755 --- a/t/t5512-ls-remote.sh +++ b/t/t5512-ls-remote.sh @@ -300,44 +300,23 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' ' test_cmp expect actual ' -test_lazy_prereq GIT_DAEMON ' - test_bool_env GIT_TEST_GIT_DAEMON true -' - -# This test spawns a daemon, so run it only if the user would be OK with -# testing with git-daemon. -test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' ' - test_set_port JGIT_DAEMON_PORT && - JGIT_DAEMON_PID= && - git init --bare empty.git && - >empty.git/git-daemon-export-ok && - mkfifo jgit_daemon_output && - { - jgit daemon --port="$JGIT_DAEMON_PORT" . >jgit_daemon_output & - JGIT_DAEMON_PID=$! - } && - test_when_finished kill "$JGIT_DAEMON_PID" && - { - read line && - case $line in - Exporting*) - ;; - *) - echo "Expected: Exporting" && - false;; - esac && - read line && - case $line in - "Listening on"*) - ;; - *) - echo "Expected: Listening on" && - false;; - esac - } <jgit_daemon_output && +test_expect_success 'indicate no refs in v0 standards-compliant empty remote' ' + # Git does not produce an output like this, but it does match the + # standard and is produced by other implementations like JGit. So + # hard-code the case we care about. + # + # The actual capabilities do not matter; there are none that would + # change how ls-remote behaves. + oid=0000000000000000000000000000000000000000 && + test-tool pkt-line pack >input.q <<-EOF && + $oid capabilities^{}Qcaps-go-here + 0000 + EOF + q_to_nul <input.q >input && + # --exit-code asks the command to exit with 2 when no # matching refs are found. - test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git + test_expect_code 2 git ls-remote --exit-code --upload-pack=./cat-input . ' test_expect_success 'ls-remote works outside repository' '
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. - it's complicated and slow. We had to do some fifo trickery to handle races, and this one test makes up 40% of the runtime of the total script. Instead, let's just hard-code the response that's of interest to us. That will test exactly what we want for every run. Signed-off-by: Jeff King <peff@peff.net> --- t/t5512-ls-remote.sh | 51 +++++++++++++------------------------------- 1 file changed, 15 insertions(+), 36 deletions(-)