Merge "Protocol V2: respect MAX_HAVES only once we got at least one ACK"

org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java

@@ -22,10 +22,14 @@ import static org.junit.Assert.assertThrows;
 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.ReflogEntry;
 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 class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 				.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<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();

org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java

@@ -503,12 +503,16 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 		}
 		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 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 					// 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 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 
 		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) {