Merge "Protocol V2: respect MAX_HAVES only once we got at least one ACK"
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 f2c7bc6..def52ff 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;
@@ -1266,6 +1271,64 @@
}
@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<Repository> 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();
Transport t = Transport.open(dst, brokenURI)) {
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 fa0c0c6..d344dea 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 @@
}
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 @@
// 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 @@
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) {
