From a22b8f5fac9dd9b99333d709e6ef8f09ca6cd0d7 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 3 Nov 2009 18:00:50 -0800 Subject: [PATCH] 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 --- .../jgit/transport/PacketLineInTest.java | 33 +++++++++ .../transport/BasePackFetchConnection.java | 71 +++++++++++++------ .../eclipse/jgit/transport/PacketLineIn.java | 17 ++++- .../eclipse/jgit/transport/UploadPack.java | 48 ++++++++++--- 4 files changed, 134 insertions(+), 35 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PacketLineInTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PacketLineInTest.java index 1bcac9e37..851dcc07b 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PacketLineInTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PacketLineInTest.java @@ -225,6 +225,28 @@ public class PacketLineInTest extends TestCase { 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 class PacketLineInTest extends TestCase { } 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()); 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 5faf957b5..8e7d3bdb1 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java +++ b/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 * Copyright (C) 2008, Shawn O. Pearce * 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 @@ abstract class BasePackFetchConnection extends BasePackConnection implements 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 @@ abstract class BasePackFetchConnection extends BasePackConnection implements 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 @@ abstract class BasePackFetchConnection extends BasePackConnection implements 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; - } + break SEND_HAVES; - 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 @@ abstract class BasePackFetchConnection extends BasePackConnection implements receivedAck = true; receivedContinue = true; havesSinceLastContinue = 0; + break; } if (monitor.isCancelled()) @@ -450,23 +464,36 @@ abstract class BasePackFetchConnection extends BasePackConnection implements // 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) - break; // commit negotiation is finished. + switch (anr) { + case NAK: + // A NAK is a response to an end we queued earlier + // we eat it and look for another ACK/NAK message. + // + 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; - if (anr == PacketLineIn.AckNackResult.ACK_CONTINUE) { - // There must be a normal ACK following this. + case ACK_CONTINUE: + case ACK_COMMON: + case ACK_READY: + // We will expect a normal ACK to break out of the loop. // - multiAck = true; + multiAck = MultiAck.CONTINUE; + break; } if (monitor.isCancelled()) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java index bc1e48e4e..db6abef1a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java @@ -64,7 +64,11 @@ class PacketLineIn { /** ACK */ ACK, /** ACK + continue */ - ACK_CONTINUE + ACK_CONTINUE, + /** ACK + common */ + ACK_COMMON, + /** ACK + ready */ + ACK_READY; } private final InputStream in; @@ -88,9 +92,16 @@ class PacketLineIn { 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); } 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 7e534a39c..20d693406 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/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 @@ public class UploadPack { 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 @@ public class UploadPack { 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 @@ public class UploadPack { } 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 @@ public class UploadPack { 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) - pckOut.writeString("ACK " + id.name() + "\n"); - } else { + break; + case DETAILED: + 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;