Implement the no-done capability

Smart HTTP clients may request both multi_ack_detailed and no-done in
the same request to prevent the client from needing to send a "done"
line to the server in response to a server's "ACK %s ready".

For smart HTTP, this can save 1 full HTTP RPC in the fetch exchange,
improving overall latency when incrementally updating a client that
has not diverged very far from the remote repository.

Unfortuantely this capability cannot be enabled for the traditional
bi-directional connections.  multi_ack_detailed has the client sending
more "have" lines at the same time that the server is creating the
"ACK %s ready" and writing out the PACK stream, resulting in some race
conditions and/or deadlock, depending on how the pipe buffers are
implemented.  For very small updates, a server might actually be able
to send "ACK %s ready", then the PACK, and disconnect before the
client even finishes sending its first batch of "have" lines.  This
may cause the client to fail with a broken pipe exception.  To avoid
all of these potential problems, "no-done" is restricted only to the
smart HTTP variant of the protocol.

Change-Id: Ie0d0a39320202bc096fec2e97cb58e9efd061b2d
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java

@@ -104,6 +104,7 @@ class UploadPackServlet extends HttpServlet {
 				ServiceNotEnabledException, ServiceNotAuthorizedException {
 			UploadPack up = (UploadPack) req.getAttribute(ATTRIBUTE_HANDLER);
 			try {
+				up.setBiDirectionalPipe(false);
 				up.sendAdvertisedRefs(pck);
 			} finally {
 				up.getRevWalk().release();

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

@@ -292,8 +292,6 @@ public class SmartClientSmartServerTest extends HttpTestCase {
 				.getRequestHeader(HDR_CONTENT_LENGTH));
 		assertNull("not chunked", service
 				.getRequestHeader(HDR_TRANSFER_ENCODING));
-		assertNull("no compression (too small)", service
-				.getRequestHeader(HDR_CONTENT_ENCODING));
 
 		assertEquals(200, service.getStatus());
 		assertEquals("application/x-git-upload-pack-result", service
@@ -301,7 +299,70 @@ public class SmartClientSmartServerTest extends HttpTestCase {
 	}
 
 	@Test
-	public void testFetchUpdateExisting() throws Exception {
+	public void testFetch_FewLocalCommits() throws Exception {
+		// Bootstrap by doing the clone.
+		//
+		TestRepository dst = createTestRepository();
+		Transport t = Transport.open(dst.getRepository(), remoteURI);
+		try {
+			t.fetch(NullProgressMonitor.INSTANCE, mirror(master));
+		} finally {
+			t.close();
+		}
+		assertEquals(B, dst.getRepository().getRef(master).getObjectId());
+		List<AccessEvent> cloneRequests = getRequests();
+
+		// Only create a few new commits.
+		TestRepository.BranchBuilder b = dst.branch(master);
+		for (int i = 0; i < 4; i++)
+			b.commit().tick(3600 /* 1 hour */).message("c" + i).create();
+
+		// Create a new commit on the remote.
+		//
+		b = new TestRepository(remoteRepository).branch(master);
+		RevCommit Z = b.commit().message("Z").create();
+
+		// Now incrementally update.
+		//
+		t = Transport.open(dst.getRepository(), remoteURI);
+		try {
+			t.fetch(NullProgressMonitor.INSTANCE, mirror(master));
+		} finally {
+			t.close();
+		}
+		assertEquals(Z, dst.getRepository().getRef(master).getObjectId());
+
+		List<AccessEvent> requests = getRequests();
+		requests.removeAll(cloneRequests);
+		assertEquals(2, requests.size());
+
+		AccessEvent info = requests.get(0);
+		assertEquals("GET", info.getMethod());
+		assertEquals(join(remoteURI, "info/refs"), info.getPath());
+		assertEquals(1, info.getParameters().size());
+		assertEquals("git-upload-pack", info.getParameter("service"));
+		assertEquals(200, info.getStatus());
+		assertEquals("application/x-git-upload-pack-advertisement",
+				info.getResponseHeader(HDR_CONTENT_TYPE));
+
+		// We should have needed one request to perform the fetch.
+		//
+		AccessEvent service = requests.get(1);
+		assertEquals("POST", service.getMethod());
+		assertEquals(join(remoteURI, "git-upload-pack"), service.getPath());
+		assertEquals(0, service.getParameters().size());
+		assertNotNull("has content-length",
+				service.getRequestHeader(HDR_CONTENT_LENGTH));
+		assertNull("not chunked",
+				service.getRequestHeader(HDR_TRANSFER_ENCODING));
+
+		assertEquals(200, service.getStatus());
+		assertEquals("application/x-git-upload-pack-result",
+				service.getResponseHeader(HDR_CONTENT_TYPE));
+	}
+
+	@Test
+	public void testFetch_TooManyLocalCommits() throws Exception {
 		// Bootstrap by doing the clone.
 		//
 		TestRepository dst = createTestRepository();

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

@@ -138,6 +138,8 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 
 	static final String OPTION_NO_PROGRESS = "no-progress";
 
+	static final String OPTION_NO_DONE = "no-done";
+
 	static enum MultiAck {
 		OFF, CONTINUE, DETAILED;
 	}
@@ -169,6 +171,8 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 
 	private boolean allowOfsDelta;
 
+	private boolean noDone;
+
 	private String lockMessage;
 
 	private PackLock packLock;
@@ -408,9 +412,11 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 		if (allowOfsDelta)
 			wantCapability(line, OPTION_OFS_DELTA);
 
-		if (wantCapability(line, OPTION_MULTI_ACK_DETAILED))
+		if (wantCapability(line, OPTION_MULTI_ACK_DETAILED)) {
 			multiAck = MultiAck.DETAILED;
-		else if (wantCapability(line, OPTION_MULTI_ACK))
+			if (statelessRPC)
+				noDone = wantCapability(line, OPTION_NO_DONE);
+		} else if (wantCapability(line, OPTION_MULTI_ACK))
 			multiAck = MultiAck.CONTINUE;
 		else
 			multiAck = MultiAck.OFF;
@@ -441,13 +447,13 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 		int havesSinceLastContinue = 0;
 		boolean receivedContinue = false;
 		boolean receivedAck = false;
-		boolean negotiate = true;
+		boolean receivedReady = false;
 
 		if (statelessRPC)
 			state.writeTo(out, null);
 
 		negotiateBegin();
-		SEND_HAVES: while (negotiate) {
+		SEND_HAVES: while (!receivedReady) {
 			final RevCommit c = walk.next();
 			if (c == null)
 				break SEND_HAVES;
@@ -514,7 +520,7 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 					receivedContinue = true;
 					havesSinceLastContinue = 0;
 					if (anr == AckNackResult.ACK_READY)
-						negotiate = false;
+						receivedReady = true;
 					break;
 				}
 
@@ -540,12 +546,14 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 		if (monitor.isCancelled())
 			throw new CancelledException();
 
-		// When statelessRPC is true we should always leave SEND_HAVES
-		// loop above while in the middle of a request. This allows us
-		// to just write done immediately.
-		//
-		pckOut.writeString("done\n");
-		pckOut.flush();
+		if (!receivedReady || !noDone) {
+			// When statelessRPC is true we should always leave SEND_HAVES
+			// loop above while in the middle of a request. This allows us
+			// to just write done immediately.
+			//
+			pckOut.writeString("done\n");
+			pckOut.flush();
+		}
 
 		if (!receivedAck) {
 			// Apparently if we have never received an ACK earlier

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

@@ -100,6 +100,8 @@ public class UploadPack {
 
 	static final String OPTION_NO_PROGRESS = BasePackFetchConnection.OPTION_NO_PROGRESS;
 
+	static final String OPTION_NO_DONE = BasePackFetchConnection.OPTION_NO_DONE;
+
 	/** Database we read the objects from. */
 	private final Repository db;
 
@@ -163,6 +165,8 @@ public class UploadPack {
 	/** null if {@link #commonBase} should be examined again. */
 	private Boolean okToGiveUp;
 
+	private boolean sentReady;
+
 	/** Objects we sent in our advertisement list, clients can ask for these. */
 	private Set<ObjectId> advertised;
 
@@ -182,6 +186,8 @@ public class UploadPack {
 
 	private MultiAck multiAck = MultiAck.OFF;
 
+	private boolean noDone;
+
 	private PackWriter.Statistics statistics;
 
 	private UploadPackLogger logger;
@@ -401,9 +407,10 @@ public class UploadPack {
 			return;
 		}
 
-		if (options.contains(OPTION_MULTI_ACK_DETAILED))
+		if (options.contains(OPTION_MULTI_ACK_DETAILED)) {
 			multiAck = MultiAck.DETAILED;
-		else if (options.contains(OPTION_MULTI_ACK))
+			noDone = options.contains(OPTION_NO_DONE);
+		} else if (options.contains(OPTION_MULTI_ACK))
 			multiAck = MultiAck.CONTINUE;
 		else
 			multiAck = MultiAck.OFF;
@@ -443,6 +450,8 @@ public class UploadPack {
 		adv.advertiseCapability(OPTION_SIDE_BAND_64K);
 		adv.advertiseCapability(OPTION_THIN_PACK);
 		adv.advertiseCapability(OPTION_NO_PROGRESS);
+		if (!biDirectionalPipe)
+			adv.advertiseCapability(OPTION_NO_DONE);
 		adv.setDerefTags(true);
 		advertised = adv.send(getAdvertisedRefs());
 		adv.end();
@@ -496,6 +505,10 @@ public class UploadPack {
 				last = processHaveLines(peerHas, last);
 				if (commonBase.isEmpty() || multiAck != MultiAck.OFF)
 					pckOut.writeString("NAK\n");
+				if (noDone && sentReady) {
+					pckOut.writeString("ACK " + last.name() + "\n");
+					return true;
+				}
 				if (!biDirectionalPipe)
 					return false;
 				pckOut.flush();
@@ -538,6 +551,7 @@ public class UploadPack {
 		List<ObjectId> toParse = peerHas;
 		HashSet<ObjectId> peerHasSet = null;
 		boolean needMissing = false;
+		sentReady = false;
 
 		if (wantAll.isEmpty() && !wantIds.isEmpty()) {
 			// We have not yet parsed the want list. Parse it now.
@@ -644,7 +658,6 @@ public class UploadPack {
 		// telling us about its history.
 		//
 		boolean didOkToGiveUp = false;
-		boolean sentReady = false;
 		if (0 < missCnt) {
 			for (int i = peerHas.size() - 1; i >= 0; i--) {
 				ObjectId id = peerHas.get(i);
@@ -670,6 +683,7 @@ public class UploadPack {
 
 		if (multiAck == MultiAck.DETAILED && !didOkToGiveUp && okToGiveUp()) {
 			ObjectId id = peerHas.get(peerHas.size() - 1);
+			sentReady = true;
 			pckOut.writeString("ACK " + id.name() + " ready\n");
 			sentReady = true;
 		}