From 0d7d98620f3c521d0471db888b1c7734f0f44eff Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Sun, 3 Jan 2021 23:07:18 +0100 Subject: [PATCH] Protocol V2: respect MAX_HAVES only once we got at least one ACK The negotiation in the git protocol contains a cutoff: if the client has sent more than MAX_HAVES "have" lines without getting an ACK, it gives up and sends a "done". MAX_HAVES is 256. However, this cutoff must kick in only if at least one ACK has been received. Otherwise the client may give up way too early, which makes the server send all its history. See [1]. This was missed when protocol V2 was implemented for fetching in JGit in commit 0853a241. Compare also C git commit 0b07eecf6ed.[2] C git had the same bug.[3][4] [1] https://github.com/git/git/blob/6c430a647cb9/Documentation/technical/pack-protocol.txt#L385 [2] https://github.com/git/git/commit/0b07eecf6ed [3] https://lore.kernel.org/git/b7f5bfb9-61fb-2552-4399-b744428728e4@suse.cz/ [4] https://lore.kernel.org/git/20200422084254.GA27502@furthur.local/ Bug: 553083 Change-Id: I1f4e2cc16b5eed6971d981d472329185abb9e4a9 Signed-off-by: Thomas Wolf --- .../http/test/SmartClientSmartServerTest.java | 63 +++++++++++++++++++ .../transport/BasePackFetchConnection.java | 12 +++- 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java index f2c7bc63c..def52ff6b 100644 --- a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java +++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java @@ -22,10 +22,14 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.io.OutputStreamWriter; import java.io.PrintWriter; +import java.io.Writer; import java.net.URI; import java.net.URISyntaxException; +import java.nio.charset.StandardCharsets; import java.text.MessageFormat; import java.util.Collections; import java.util.EnumSet; @@ -70,6 +74,7 @@ import org.eclipse.jgit.lib.ReflogReader; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.StoredConfig; +import org.eclipse.jgit.lib.TextProgressMonitor; import org.eclipse.jgit.revwalk.RevBlob; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; @@ -1265,6 +1270,64 @@ public void testFetch_TooManyLocalCommits() throws Exception { .getResponseHeader(HDR_CONTENT_TYPE)); } + @Test + public void testFetch_MaxHavesCutoffAfterAckOnly() throws Exception { + // Bootstrap by doing the clone. + // + TestRepository dst = createTestRepository(); + try (Transport t = Transport.open(dst.getRepository(), remoteURI)) { + t.fetch(NullProgressMonitor.INSTANCE, mirror(master)); + } + assertEquals(B, dst.getRepository().exactRef(master).getObjectId()); + + // Force enough into the local client that enumeration will + // need more than MAX_HAVES (256) haves to be sent. The server + // doesn't know any of these, so it will never ACK. The client + // should keep going. + // + // If it does, client and server will find a common commit, and + // the pack file will contain exactly the one commit object Z + // we create on the remote, which we can test for via the progress + // monitor, which should have something like + // "Receiving objects: 100% (1/1)". If the client sends a "done" + // too early, the server will send more objects, and we'll have + // a line like "Receiving objects: 100% (8/8)". + TestRepository.BranchBuilder b = dst.branch(master); + // The client will send 32 + 64 + 128 + 256 + 512 haves. Only the + // last one will be a common commit. If the cutoff kicks in too + // early (after 480), we'll get too many objects in the fetch. + for (int i = 0; i < 992; i++) + b.commit().tick(3600 /* 1 hour */).message("c" + i).create(); + + // Create a new commit on the remote. + // + RevCommit Z; + try (TestRepository tr = new TestRepository<>( + remoteRepository)) { + b = tr.branch(master); + Z = b.commit().message("Z").create(); + } + + // Now incrementally update. + // + ByteArrayOutputStream buffer = new ByteArrayOutputStream(); + Writer writer = new OutputStreamWriter(buffer, StandardCharsets.UTF_8); + TextProgressMonitor monitor = new TextProgressMonitor(writer); + try (Transport t = Transport.open(dst.getRepository(), remoteURI)) { + t.fetch(monitor, mirror(master)); + } + assertEquals(Z, dst.getRepository().exactRef(master).getObjectId()); + + String progressMessages = new String(buffer.toByteArray(), + StandardCharsets.UTF_8); + Pattern expected = Pattern + .compile("Receiving objects:\\s+100% \\(1/1\\)\n"); + if (!expected.matcher(progressMessages).find()) { + System.out.println(progressMessages); + fail("Expected only one object to be sent"); + } + } + @Test public void testInitialClone_BrokenServer() throws Exception { try (Repository dst = createBareRepository(); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java index fa0c0c667..d344deac2 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java @@ -503,12 +503,16 @@ private boolean sendNextHaveBatch(FetchStateV2 fetchState, } fetchState.havesTotal += n; if (n == 0 - || fetchState.havesWithoutAck > MAX_HAVES + || (fetchState.hadAcks + && fetchState.havesWithoutAck > MAX_HAVES) || fetchState.havesTotal > maxHaves) { output.writeString("done\n"); //$NON-NLS-1$ output.end(); return true; } + // Increment only after the test above. Of course we have no ACKs yet + // for the newly added "have"s, so it makes no sense to count them + // against the MAX_HAVES limit. fetchState.havesWithoutAck += n; output.end(); fetchState.incHavesToSend(statelessRPC); @@ -555,6 +559,7 @@ private boolean readAcknowledgments(FetchStateV2 fetchState, // markCommon appends the object to the "state" markCommon(walk.parseAny(returnedId), ack, true); fetchState.havesWithoutAck = 0; + fetchState.hadAcks = true; } else if (ack == AckNackResult.ACK_READY) { gotReady = true; } @@ -1035,6 +1040,11 @@ private static class FetchStateV2 { long havesTotal; + // Set to true if we got at least one ACK in protocol V2. + boolean hadAcks; + + // Counts haves without ACK. Use as cutoff for negotiation only once + // hadAcks == true. long havesWithoutAck; void incHavesToSend(boolean statelessRPC) {