[PATCH 0/7] v0 multiple-symref infinite loop fix and test cleanup
- From
Jeff King <peff@peff.net>
- Date
- Apr 12, 2023, 06:23 UTC
- Message-ID
- <20230412062300.GA838367@coredump.intra.peff.net>
- In-Reply-To
- <xmqqa5ze9glg.fsf@gitster.g>
On Tue, Apr 11, 2023 at 03:52:43PM -0700, Junio C Hamano wrote:
Show 10 quoted lines
> > +test_expect_success 'v0 clients can handle multiple symrefs' ' > > + # Git will not generate multiple symref entries for v0 these days, but it > > + # is technically allowed, and we did so until d007dbf7d6 (Revert > > + # "upload-pack: send non-HEAD symbolic refs", 2013-11-18). Test the > > + # client handling here by faking that older behavior. > > Yeah, I remember that fix where somebody had tons of symbolic refs > and busted the protocol limit. Is "multiple symref" used here > because it is the easiest to reproduce the issue, or have we saw > such a potentially broken server in the wild?
Both. :) It was what we saw in the wild, but it's also one of only two capabilities that can cause the problem, because it requires passing an offset parameter, which we only do for capabilities we expect to see multiple times. The other is "object-format", but we only ever print it once. More details in the commit message to follow.
Show 7 quoted lines
> > + # Note that our oid is hard-coded to always be sha1, and not using > > + # test_oid. Since our fake capabilities line does not have an > > + # object-format entry, the client will always use sha1 mode. > > It probably is OK to run the test in that "undeclared - assume > SHA-1" mode, even though I think we give an explicit "object-format" > capability even when talking from the SHA-1 repository these days.
We do, but I think it is OK to ignore that. The test will continue to work even in a world where sha256 becomes the default. If we eventually remove all vestiges of backwards-compatible sha1 support, it will have to be updated, but I'm OK with that (keep in mind that the test is also v0-only, as the v2 parser is totally different).
Show 11 quoted lines
> > I also wondered if we tested this multiple-symref case for protocol v2 > > (where it works fine), but it looks like we may not. There are earlier > > tests which _would_ trigger it, but we force them into v0 mode, due to > > b2f73b70b2 (t5512: compensate for v0 only sending HEAD symrefs, > > 2019-02-25). I think we really should be letting ls-remote use the > > protocol it prefers (v2 by default, and v0 if the suite is run in that > > mode), and the expected output should be adjusted based on the mode. > > I'll see if I can do that as well, to make this a two-patch series. > > Thanks. I really appreciate your being almost always thorough and > wish more contributors took inspirations.
Well, no good deed goes unpunished. :) These tests have not aged well, so there were a number of fixes to make. Here's the series I came up with.
The first one is the bug fix; I put it at the front because it's obviously the most urgent. Patches 2-6 are test cleanups, and as such should not be very risky, but I didn't want to hold up the fix. But they do depend on the helper script introduced by patch 1, so they can't be applied separately.
Patch 7 is a cleanup in the code which should have no behavior change. It could be applied separately (or even dropped if you don't like it).
[1/7]: v0 protocol: fix infinite loop when parsing multi-valued capabilities
[2/7]: t5512: stop referring to "v1" protocol
[3/7]: t5512: stop using jgit for capabilities^{} test
[4/7]: t5512: add v2 support for "ls-remote --symref" test
[5/7]: t5512: allow any protocol version for filtered symref test
[6/7]: t5512: test "ls-remote --heads --symref" filtering with v0 and v2
[7/7]: v0 protocol: use size_t for capability length/offsetbuiltin/receive-pack.c | 2 +- connect.c | 26 ++++---- connect.h | 4 +- fetch-pack.c | 4 +- send-pack.c | 2 +- t/t5512-ls-remote.sh | 148 ++++++++++++++++++++++------------------- transport.c | 2 +- upload-pack.c | 2 +- 8 files changed, 101 insertions(+), 89 deletions(-)
-Peff