Implement multi_ack_detailed protocol extension

The multi_ack_detailed extension breaks out the "ACK %s continue" status
code into "ACK %s common" and "ACK %s ready" states, making it easier to
discover which objects are truely common, and which objects are simply
on a chain the server doesn't care learning about.

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

org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PacketLineInTest.java

@@ -225,6 +225,28 @@ public void testReadACK_ACKcontinue1() throws IOException {
 		assertEOF();
 	}
 
+	public void testReadACK_ACKcommon1() throws IOException {
+		final ObjectId expid = ObjectId
+				.fromString("fcfcfb1fd94829c1a1704f894fc111d14770d34e");
+		final MutableObjectId actid = new MutableObjectId();
+
+		init("0038ACK fcfcfb1fd94829c1a1704f894fc111d14770d34e common\n");
+		assertSame(PacketLineIn.AckNackResult.ACK_COMMON, in.readACK(actid));
+		assertTrue(actid.equals(expid));
+		assertEOF();
+	}
+
+	public void testReadACK_ACKready1() throws IOException {
+		final ObjectId expid = ObjectId
+				.fromString("fcfcfb1fd94829c1a1704f894fc111d14770d34e");
+		final MutableObjectId actid = new MutableObjectId();
+
+		init("0037ACK fcfcfb1fd94829c1a1704f894fc111d14770d34e ready\n");
+		assertSame(PacketLineIn.AckNackResult.ACK_READY, in.readACK(actid));
+		assertTrue(actid.equals(expid));
+		assertEOF();
+	}
+
 	public void testReadACK_Invalid1() {
 		init("HELO");
 		try {
@@ -246,6 +268,17 @@ public void testReadACK_Invalid2() {
 	}
 
 	public void testReadACK_Invalid3() {
+		String s = "ACK fcfcfb1fd94829c1a1704f894fc111d14770d34e neverhappen";
+		init("003d" + s + "\n");
+		try {
+			in.readACK(new MutableObjectId());
+			fail("incorrectly accepted unsupported ACK status");
+		} catch (IOException e) {
+			assertEquals("Expected ACK/NAK, got: " + s, e.getMessage());
+		}
+	}
+
+	public void testReadACK_Invalid4() {
 		init("0000");
 		try {
 			in.readACK(new MutableObjectId());

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

@@ -1,4 +1,5 @@
 /*
+ * Copyright (C) 2008-2009, Google Inc.
  * Copyright (C) 2008, Robin Rosenberg <robin.rosenberg@dewire.com>
  * Copyright (C) 2008, Shawn O. Pearce <spearce@spearce.org>
  * and other copyright owners as documented in the project's IP log.
@@ -110,6 +111,8 @@ abstract class BasePackFetchConnection extends BasePackConnection implements
 
 	static final String OPTION_MULTI_ACK = "multi_ack";
 
+	static final String OPTION_MULTI_ACK_DETAILED = "multi_ack_detailed";
+
 	static final String OPTION_THIN_PACK = "thin-pack";
 
 	static final String OPTION_SIDE_BAND = "side-band";
@@ -122,6 +125,10 @@ abstract class BasePackFetchConnection extends BasePackConnection implements
 
 	static final String OPTION_NO_PROGRESS = "no-progress";
 
+	static enum MultiAck {
+		OFF, CONTINUE, DETAILED;
+	}
+
 	private final RevWalk walk;
 
 	/** All commits that are immediately reachable by a local ref. */
@@ -136,7 +143,7 @@ abstract class BasePackFetchConnection extends BasePackConnection implements
 	/** Marks a commit listed in the advertised refs. */
 	final RevFlag ADVERTISED;
 
-	private boolean multiAck;
+	private MultiAck multiAck = MultiAck.OFF;
 
 	private boolean thinPack;
 
@@ -335,7 +342,14 @@ private String enableCapabilities() {
 			includeTags = wantCapability(line, OPTION_INCLUDE_TAG);
 		if (allowOfsDelta)
 			wantCapability(line, OPTION_OFS_DELTA);
-		multiAck = wantCapability(line, OPTION_MULTI_ACK);
+
+		if (wantCapability(line, OPTION_MULTI_ACK_DETAILED))
+			multiAck = MultiAck.DETAILED;
+		else if (wantCapability(line, OPTION_MULTI_ACK))
+			multiAck = MultiAck.CONTINUE;
+		else
+			multiAck = MultiAck.OFF;
+
 		if (thinPack)
 			thinPack = wantCapability(line, OPTION_THIN_PACK);
 		if (wantCapability(line, OPTION_SIDE_BAND_64K))
@@ -353,13 +367,12 @@ private void negotiate(final ProgressMonitor monitor) throws IOException,
 		int havesSinceLastContinue = 0;
 		boolean receivedContinue = false;
 		boolean receivedAck = false;
-		boolean sendHaves = true;
 
 		negotiateBegin();
-		while (sendHaves) {
+		SEND_HAVES: for (;;) {
 			final RevCommit c = walk.next();
 			if (c == null)
-				break;
+				break SEND_HAVES;
 
 			pckOut.writeString("have " + c.getId().name() + "\n");
 			havesSent++;
@@ -388,31 +401,31 @@ private void negotiate(final ProgressMonitor monitor) throws IOException,
 				continue;
 			}
 
-			for (;;) {
+			READ_RESULT: for (;;) {
 				final PacketLineIn.AckNackResult anr;
 
 				anr = pckIn.readACK(ackId);
-				if (anr == PacketLineIn.AckNackResult.NAK) {
+				switch (anr) {
+				case NAK:
 					// More have lines are necessary to compute the
 					// pack on the remote side. Keep doing that.
 					//
 					resultsPending--;
-					break;
+					break READ_RESULT;
-				}
 
-				if (anr == PacketLineIn.AckNackResult.ACK) {
+				case ACK:
 					// The remote side is happy and knows exactly what
 					// to send us. There is no further negotiation and
 					// we can break out immediately.
 					//
-					multiAck = false;
+					multiAck = MultiAck.OFF;
 					resultsPending = 0;
 					receivedAck = true;
-					sendHaves = false;
+					break SEND_HAVES;
-					break;
-				}
 
-				if (anr == PacketLineIn.AckNackResult.ACK_CONTINUE) {
+				case ACK_CONTINUE:
+				case ACK_COMMON:
+				case ACK_READY:
 					// The server knows this commit (ackId). We don't
 					// need to send any further along its ancestry, but
 					// we need to continue to talk about other parts of
@@ -422,6 +435,7 @@ private void negotiate(final ProgressMonitor monitor) throws IOException,
 					receivedAck = true;
 					receivedContinue = true;
 					havesSinceLastContinue = 0;
+					break;
 				}
 
 				if (monitor.isCancelled())
@@ -450,23 +464,36 @@ private void negotiate(final ProgressMonitor monitor) throws IOException,
 			// there is one more result expected from the done we
 			// just sent to the remote.
 			//
-			multiAck = false;
+			multiAck = MultiAck.OFF;
 			resultsPending++;
 		}
 
-		while (resultsPending > 0 || multiAck) {
+		READ_RESULT: while (resultsPending > 0 || multiAck != MultiAck.OFF) {
 			final PacketLineIn.AckNackResult anr;
 
 			anr = pckIn.readACK(ackId);
 			resultsPending--;
 
-			if (anr == PacketLineIn.AckNackResult.ACK)
+			switch (anr) {
-				break; // commit negotiation is finished.
+			case NAK:
-
+				// A NAK is a response to an end we queued earlier
-			if (anr == PacketLineIn.AckNackResult.ACK_CONTINUE) {
+				// we eat it and look for another ACK/NAK message.
-				// There must be a normal ACK following this.
 				//
-				multiAck = true;
+				break;
+
+			case ACK:
+				// A solitary ACK at this point means the remote won't
+				// speak anymore, but is going to send us a pack now.
+				//
+				break READ_RESULT;
+
+			case ACK_CONTINUE:
+			case ACK_COMMON:
+			case ACK_READY:
+				// We will expect a normal ACK to break out of the loop.
+				//
+				multiAck = MultiAck.CONTINUE;
+				break;
 			}
 
 			if (monitor.isCancelled())

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

@@ -64,7 +64,11 @@ static enum AckNackResult {
 		/** ACK */
 		ACK,
 		/** ACK + continue */
-		ACK_CONTINUE
+		ACK_CONTINUE,
+		/** ACK + common */
+		ACK_COMMON,
+		/** ACK + ready */
+		ACK_READY;
 	}
 
 	private final InputStream in;
@@ -88,9 +92,16 @@ AckNackResult readACK(final MutableObjectId returnedId) throws IOException {
 			return AckNackResult.NAK;
 		if (line.startsWith("ACK ")) {
 			returnedId.fromString(line.substring(4, 44));
-			if (line.indexOf("continue", 44) != -1)
+			if (line.length() == 44)
+				return AckNackResult.ACK;
+
+			final String arg = line.substring(44);
+			if (arg.equals(" continue"))
 				return AckNackResult.ACK_CONTINUE;
-			return AckNackResult.ACK;
+			else if (arg.equals(" common"))
+				return AckNackResult.ACK_COMMON;
+			else if (arg.equals(" ready"))
+				return AckNackResult.ACK_READY;
 		}
 		throw new PackProtocolException("Expected ACK/NAK, got: " + line);
 	}

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

@@ -43,6 +43,8 @@
 
 package org.eclipse.jgit.transport;
 
+import static org.eclipse.jgit.transport.BasePackFetchConnection.MultiAck;
+
 import java.io.BufferedOutputStream;
 import java.io.EOFException;
 import java.io.IOException;
@@ -80,6 +82,8 @@ public class UploadPack {
 
 	static final String OPTION_MULTI_ACK = BasePackFetchConnection.OPTION_MULTI_ACK;
 
+	static final String OPTION_MULTI_ACK_DETAILED = BasePackFetchConnection.OPTION_MULTI_ACK_DETAILED;
+
 	static final String OPTION_THIN_PACK = BasePackFetchConnection.OPTION_THIN_PACK;
 
 	static final String OPTION_SIDE_BAND = BasePackFetchConnection.OPTION_SIDE_BAND;
@@ -142,7 +146,7 @@ public class UploadPack {
 
 	private final RevFlagSet SAVE;
 
-	private boolean multiAck;
+	private MultiAck multiAck = MultiAck.OFF;
 
 	/**
 	 * Create a new pack upload for an open repository.
@@ -247,7 +251,14 @@ private void service() throws IOException {
 		recvWants();
 		if (wantAll.isEmpty())
 			return;
-		multiAck = options.contains(OPTION_MULTI_ACK);
+
+		if (options.contains(OPTION_MULTI_ACK_DETAILED))
+			multiAck = MultiAck.DETAILED;
+		else if (options.contains(OPTION_MULTI_ACK))
+			multiAck = MultiAck.CONTINUE;
+		else
+			multiAck = MultiAck.OFF;
+
 		negotiate();
 		sendPack();
 	}
@@ -255,6 +266,7 @@ private void service() throws IOException {
 	private void sendAdvertisedRefs() throws IOException {
 		final RefAdvertiser adv = new RefAdvertiser(pckOut, walk, ADVERTISED);
 		adv.advertiseCapability(OPTION_INCLUDE_TAG);
+		adv.advertiseCapability(OPTION_MULTI_ACK_DETAILED);
 		adv.advertiseCapability(OPTION_MULTI_ACK);
 		adv.advertiseCapability(OPTION_OFS_DELTA);
 		adv.advertiseCapability(OPTION_SIDE_BAND);
@@ -335,7 +347,7 @@ private void negotiate() throws IOException {
 			}
 
 			if (line == PacketLineIn.END) {
-				if (commonBase.isEmpty() || multiAck)
+				if (commonBase.isEmpty() || multiAck != MultiAck.OFF)
 					pckOut.writeString("NAK\n");
 				pckOut.flush();
 			} else if (line.startsWith("have ") && line.length() == 45) {
@@ -343,23 +355,39 @@ private void negotiate() throws IOException {
 				if (matchHave(id)) {
 					// Both sides have the same object; let the client know.
 					//
-					if (multiAck) {
+					last = id;
-						last = id;
+					switch (multiAck) {
+					case OFF:
+						if (commonBase.size() == 1)
+							pckOut.writeString("ACK " + id.name() + "\n");
+						break;
+					case CONTINUE:
 						pckOut.writeString("ACK " + id.name() + " continue\n");
-					} else if (commonBase.size() == 1)
+						break;
-						pckOut.writeString("ACK " + id.name() + "\n");
+					case DETAILED:
-				} else {
+						pckOut.writeString("ACK " + id.name() + " common\n");
+						break;
+					}
+				} else if (okToGiveUp()) {
 					// They have this object; we don't.
 					//
-					if (multiAck && okToGiveUp())
+					switch (multiAck) {
+					case OFF:
+						break;
+					case CONTINUE:
 						pckOut.writeString("ACK " + id.name() + " continue\n");
+						break;
+					case DETAILED:
+						pckOut.writeString("ACK " + id.name() + " ready\n");
+						break;
+					}
 				}
 
 			} else if (line.equals("done")) {
 				if (commonBase.isEmpty())
 					pckOut.writeString("NAK\n");
 
-				else if (multiAck)
+				else if (multiAck != MultiAck.OFF)
 					pckOut.writeString("ACK " + last.name() + "\n");
 				break;