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>

1 files changed, 17 insertions, 3 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
index 2802c0712e..50f57130c3 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/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;
 		}