Teach independent negotiation (no pack file) using an option "wait-for-done"

From Git commit 9c1e657a8f:
  Currently, the packfile negotiation step within a Git fetch cannot be
  done independent of sending the packfile, even though there is at
  least one application wherein this is useful - push negotiation.
  Therefore, make it possible for this negotiation step to be done
  independently.

  This feature is for protocol v2 only.

  In the protocol, the main hindrance towards independent negotiation is
  that the server can unilaterally decide to send the packfile. This is
  solved by a "wait-for-done" argument: the server will then wait for
  the client to say "done". In practice, the client will never say it;
  instead it will cease requests once it is satisfied.

Advertising the server capability option "wait-for-done" is behind the
transport config: uploadpack.advertisewaitfordone, which by default is
false.

Change-Id: I5ebd3e99ad76b8943597216e23ced2ed38eb5224

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

@@ -998,6 +998,108 @@ public class UploadPackTest {
 		assertTrue(client.getObjectDatabase().has(barChild.toObjectId()));
 	}
 
+	@Test
+	public void testV2FetchWithoutWaitForDoneReceivesPackfile()
+			throws Exception {
+		String commonInBlob = "abcdefghijklmnopqrstuvwxyz";
+
+		RevBlob parentBlob = remote.blob(commonInBlob + "a");
+		RevCommit parent = remote
+				.commit(remote.tree(remote.file("foo", parentBlob)));
+		remote.update("branch1", parent);
+
+		RevCommit localParent = null;
+		RevCommit localChild = null;
+		try (TestRepository<InMemoryRepository> local = new TestRepository<>(
+				client)) {
+			RevBlob localParentBlob = local.blob(commonInBlob + "a");
+			localParent = local
+					.commit(local.tree(local.file("foo", localParentBlob)));
+			RevBlob localChildBlob = local.blob(commonInBlob + "b");
+			localChild = local.commit(
+					local.tree(local.file("foo", localChildBlob)), localParent);
+			local.update("branch1", localChild);
+		}
+
+		ByteArrayInputStream recvStream = uploadPackV2("command=fetch\n",
+				PacketLineIn.delimiter(),
+				"have " + localParent.toObjectId().getName() + "\n",
+				"have " + localChild.toObjectId().getName() + "\n",
+				PacketLineIn.end());
+		PacketLineIn pckIn = new PacketLineIn(recvStream);
+		assertThat(pckIn.readString(), is("acknowledgments"));
+		assertThat(Arrays.asList(pckIn.readString()),
+				hasItems("ACK " + parent.toObjectId().getName()));
+		assertThat(pckIn.readString(), is("ready"));
+		assertTrue(PacketLineIn.isDelimiter(pckIn.readString()));
+		assertThat(pckIn.readString(), is("packfile"));
+		parsePack(recvStream);
+	}
+
+	@Test
+	public void testV2FetchWithWaitForDoneOnlyDoesNegotiation()
+			throws Exception {
+		String commonInBlob = "abcdefghijklmnopqrstuvwxyz";
+
+		RevBlob parentBlob = remote.blob(commonInBlob + "a");
+		RevCommit parent = remote
+				.commit(remote.tree(remote.file("foo", parentBlob)));
+		remote.update("branch1", parent);
+
+		RevCommit localParent = null;
+		RevCommit localChild = null;
+		try (TestRepository<InMemoryRepository> local = new TestRepository<>(
+				client)) {
+			RevBlob localParentBlob = local.blob(commonInBlob + "a");
+			localParent = local
+					.commit(local.tree(local.file("foo", localParentBlob)));
+			RevBlob localChildBlob = local.blob(commonInBlob + "b");
+			localChild = local.commit(
+					local.tree(local.file("foo", localChildBlob)), localParent);
+			local.update("branch1", localChild);
+		}
+
+		ByteArrayInputStream recvStream = uploadPackV2("command=fetch\n",
+				PacketLineIn.delimiter(), "wait-for-done\n",
+				"have " + localParent.toObjectId().getName() + "\n",
+				"have " + localChild.toObjectId().getName() + "\n",
+				PacketLineIn.end());
+		PacketLineIn pckIn = new PacketLineIn(recvStream);
+		assertThat(pckIn.readString(), is("acknowledgments"));
+		assertThat(Arrays.asList(pckIn.readString()),
+				hasItems("ACK " + parent.toObjectId().getName()));
+		assertTrue(PacketLineIn.isEnd(pckIn.readString()));
+	}
+
+	@Test
+	public void testV2FetchWithWaitForDoneOnlyDoesNegotiationAndNothingToAck()
+			throws Exception {
+		String commonInBlob = "abcdefghijklmnopqrstuvwxyz";
+
+		RevCommit localParent = null;
+		RevCommit localChild = null;
+		try (TestRepository<InMemoryRepository> local = new TestRepository<>(
+				client)) {
+			RevBlob localParentBlob = local.blob(commonInBlob + "a");
+			localParent = local
+					.commit(local.tree(local.file("foo", localParentBlob)));
+			RevBlob localChildBlob = local.blob(commonInBlob + "b");
+			localChild = local.commit(
+					local.tree(local.file("foo", localChildBlob)), localParent);
+			local.update("branch1", localChild);
+		}
+
+		ByteArrayInputStream recvStream = uploadPackV2("command=fetch\n",
+				PacketLineIn.delimiter(), "wait-for-done\n",
+				"have " + localParent.toObjectId().getName() + "\n",
+				"have " + localChild.toObjectId().getName() + "\n",
+				PacketLineIn.end());
+		PacketLineIn pckIn = new PacketLineIn(recvStream);
+		assertThat(pckIn.readString(), is("acknowledgments"));
+		assertThat(pckIn.readString(), is("NAK"));
+		assertTrue(PacketLineIn.isEnd(pckIn.readString()));
+	}
+
 	@Test
 	public void testV2FetchThinPack() throws Exception {
 		String commonInBlob = "abcdefghijklmnopqrstuvwxyz";

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

@@ -36,6 +36,8 @@ public final class FetchV2Request extends FetchRequest {
 
 	private final boolean doneReceived;
 
+	private final boolean waitForDone;
+
 	@NonNull
 	private final List<String> serverOptions;
 
@@ -50,7 +52,8 @@ public final class FetchV2Request extends FetchRequest {
 			@NonNull Set<ObjectId> clientShallowCommits, int deepenSince,
 			@NonNull List<String> deepenNotRefs, int depth,
 			@NonNull FilterSpec filterSpec,
-			boolean doneReceived, @NonNull Set<String> clientCapabilities,
+			boolean doneReceived, boolean waitForDone,
+			@NonNull Set<String> clientCapabilities,
 			@Nullable String agent, @NonNull List<String> serverOptions,
 			boolean sidebandAll, @NonNull List<String> packfileUriProtocols) {
 		super(wantIds, depth, clientShallowCommits, filterSpec,
@@ -59,6 +62,7 @@ public final class FetchV2Request extends FetchRequest {
 		this.peerHas = requireNonNull(peerHas);
 		this.wantedRefs = requireNonNull(wantedRefs);
 		this.doneReceived = doneReceived;
+		this.waitForDone = waitForDone;
 		this.serverOptions = requireNonNull(serverOptions);
 		this.sidebandAll = sidebandAll;
 		this.packfileUriProtocols = packfileUriProtocols;
@@ -90,7 +94,14 @@ public final class FetchV2Request extends FetchRequest {
 	}
 
 	/**
-	 * Options received in server-option lines. The caller can choose to act on
+	 * @return true if the request had a "wait-for-done" line
+	 */
+	boolean wasWaitForDoneReceived() {
+		return waitForDone;
+	}
+
+	/**
+	 * Options received in server-option lines. The caller can choose to	 act on
 	 * these in an application-specific way
 	 *
 	 * @return Immutable list of server options received in the request
@@ -141,6 +152,8 @@ public final class FetchV2Request extends FetchRequest {
 
 		boolean doneReceived;
 
+		boolean waitForDone;
+
 		@Nullable
 		String agent;
 
@@ -279,6 +292,16 @@ public final class FetchV2Request extends FetchRequest {
 			return this;
 		}
 
+		/**
+		 * Mark that the "wait-for-done" line has been received.
+		 *
+		 * @return this builder
+		 */
+		Builder setWaitForDone() {
+			waitForDone = true;
+			return this;
+		}
+
 		/**
 		 * Value of an agent line received after the command and before the
 		 * arguments. E.g. "agent=a.b.c/1.0" should set "a.b.c/1.0".
@@ -328,7 +351,7 @@ public final class FetchV2Request extends FetchRequest {
 		FetchV2Request build() {
 			return new FetchV2Request(peerHas, wantedRefs, wantIds,
 					clientShallowCommits, deepenSince, deepenNotRefs,
-					depth, filterSpec, doneReceived, clientCapabilities,
+					depth, filterSpec, doneReceived, waitForDone, clientCapabilities,
 					agent, Collections.unmodifiableList(serverOptions),
 					sidebandAll,
 					Collections.unmodifiableList(packfileUriProtocols));

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

@@ -148,6 +148,13 @@ public final class GitProtocolConstants {
 	 */
 	public static final String OPTION_SIDEBAND_ALL = "sideband-all"; //$NON-NLS-1$
 
+	/**
+	 * The server waits for client to send "done" before sending any packs back.
+	 *
+	 * @since 5.12
+	 */
+	public static final String OPTION_WAIT_FOR_DONE = "wait-for-done"; //$NON-NLS-1$
+
 	/**
 	 * The client supports atomic pushes. If this option is used, the server
 	 * will update all refs within one atomic transaction.

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

@@ -19,6 +19,7 @@ import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_SERVER_OPTI
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_SIDEBAND_ALL;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_SIDE_BAND_64K;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_THIN_PACK;
+import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_WAIT_FOR_DONE;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_WANT_REF;
 
 import java.io.IOException;
@@ -123,6 +124,8 @@ final class ProtocolV2Parser {
 				reqBuilder.addPeerHas(ObjectId.fromString(line2.substring(5)));
 			} else if (line2.equals("done")) { //$NON-NLS-1$
 				reqBuilder.setDoneReceived();
+			} else if (line2.equals(OPTION_WAIT_FOR_DONE)) {
+				reqBuilder.setWaitForDone();
 			} else if (line2.equals(OPTION_THIN_PACK)) {
 				reqBuilder.addClientCapability(OPTION_THIN_PACK);
 			} else if (line2.equals(OPTION_NO_PROGRESS)) {

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

@@ -120,7 +120,10 @@ public class TransferConfig {
 	private final boolean allowReachableSha1InWant;
 	private final boolean allowFilter;
 	private final boolean allowSidebandAll;
+
 	private final boolean advertiseSidebandAll;
+	private final boolean advertiseWaitForDone;
+
 	final @Nullable ProtocolVersion protocolVersion;
 	final String[] hideRefs;
 
@@ -206,6 +209,8 @@ public class TransferConfig {
 				"uploadpack", "allowsidebandall", false);
 		advertiseSidebandAll = rc.getBoolean("uploadpack",
 				"advertisesidebandall", false);
+		advertiseWaitForDone = rc.getBoolean("uploadpack",
+				"advertisewaitfordone", false);
 	}
 
 	/**
@@ -304,6 +309,14 @@ public class TransferConfig {
 		return advertiseSidebandAll && allowSidebandAll;
 	}
 
+	/**
+	 * @return true to advertise wait-for-done all to the clients
+	 * @since 5.12
+	 */
+	public boolean isAdvertiseWaitForDone() {
+		return advertiseWaitForDone;
+	}
+
 	/**
 	 * Get {@link org.eclipse.jgit.transport.RefFilter} respecting configured
 	 * hidden refs.

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

@@ -33,6 +33,7 @@ import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_SIDEBAND_AL
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_SIDE_BAND;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_SIDE_BAND_64K;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_THIN_PACK;
+import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_WAIT_FOR_DONE;
 import static org.eclipse.jgit.transport.GitProtocolConstants.VERSION_2_REQUEST;
 import static org.eclipse.jgit.util.RefMap.toRefMap;
 
@@ -1192,9 +1193,10 @@ public class UploadPack {
 			walk.assumeShallow(req.getClientShallowCommits());
 
 		if (req.wasDoneReceived()) {
-			processHaveLines(req.getPeerHas(), ObjectId.zeroId(),
+			processHaveLines(
+					req.getPeerHas(), ObjectId.zeroId(),
 					new PacketLineOut(NullOutputStream.INSTANCE, false),
-					accumulator);
+					accumulator, req.wasWaitForDoneReceived() ? Option.WAIT_FOR_DONE : Option.NONE);
 		} else {
 			pckOut.writeString(
 					GitProtocolConstants.SECTION_ACKNOWLEDGMENTS + '\n');
@@ -1205,8 +1207,8 @@ public class UploadPack {
 			}
 			processHaveLines(req.getPeerHas(), ObjectId.zeroId(),
 					new PacketLineOut(NullOutputStream.INSTANCE, false),
-					accumulator);
-			if (okToGiveUp()) {
+					accumulator, Option.NONE);
+			if (!req.wasWaitForDoneReceived() && okToGiveUp()) {
 				pckOut.writeString("ready\n"); //$NON-NLS-1$
 			} else if (commonBase.isEmpty()) {
 				pckOut.writeString("NAK\n"); //$NON-NLS-1$
@@ -1214,7 +1216,7 @@ public class UploadPack {
 			sectionSent = true;
 		}
 
-		if (req.wasDoneReceived() || okToGiveUp()) {
+		if (req.wasDoneReceived() || (!req.wasWaitForDoneReceived() && okToGiveUp())) {
 			if (mayHaveShallow) {
 				if (sectionSent)
 					pckOut.writeDelim();
@@ -1312,6 +1314,9 @@ public class UploadPack {
 						? OPTION_SIDEBAND_ALL + ' '
 						: "")
 				+ (cachedPackUriProvider != null ? "packfile-uris " : "")
+				+ (transferConfig.isAdvertiseWaitForDone()
+						? OPTION_WAIT_FOR_DONE + ' '
+						: "")
 				+ OPTION_SHALLOW);
 		caps.add(CAPABILITY_SERVER_OPTION);
 		return caps;
@@ -1656,7 +1661,7 @@ public class UploadPack {
 			}
 
 			if (PacketLineIn.isEnd(line)) {
-				last = processHaveLines(peerHas, last, pckOut, accumulator);
+				last = processHaveLines(peerHas, last, pckOut, accumulator, Option.NONE);
 				if (commonBase.isEmpty() || multiAck != MultiAck.OFF)
 					pckOut.writeString("NAK\n"); //$NON-NLS-1$
 				if (noDone && sentReady) {
@@ -1671,7 +1676,7 @@ public class UploadPack {
 				peerHas.add(ObjectId.fromString(line.substring(5)));
 				accumulator.haves++;
 			} else if (line.equals("done")) { //$NON-NLS-1$
-				last = processHaveLines(peerHas, last, pckOut, accumulator);
+				last = processHaveLines(peerHas, last, pckOut, accumulator, Option.NONE);
 
 				if (commonBase.isEmpty())
 					pckOut.writeString("NAK\n"); //$NON-NLS-1$
@@ -1687,8 +1692,14 @@ public class UploadPack {
 		}
 	}
 
+	private enum Option {
+		WAIT_FOR_DONE,
+		NONE;
+	}
+
 	private ObjectId processHaveLines(List<ObjectId> peerHas, ObjectId last,
-			PacketLineOut out, PackStatistics.Accumulator accumulator)
+			PacketLineOut out, PackStatistics.Accumulator accumulator,
+			Option option)
 			throws IOException {
 		preUploadHook.onBeginNegotiateRound(this, wantIds, peerHas.size());
 		if (wantAll.isEmpty() && !wantIds.isEmpty())
@@ -1754,6 +1765,18 @@ public class UploadPack {
 		// create a pack at this point, let the client know so it stops
 		// telling us about its history.
 		//
+		if (option != Option.WAIT_FOR_DONE) {
+			sentReady = shouldGiveUp(peerHas, out, missCnt);
+		}
+
+		preUploadHook.onEndNegotiateRound(this, wantAll, haveCnt, missCnt, sentReady);
+		peerHas.clear();
+		return last;
+	}
+
+	private boolean shouldGiveUp(List<ObjectId> peerHas, PacketLineOut out, int missCnt)
+			throws IOException {
+		boolean sentReady = false;
 		boolean didOkToGiveUp = false;
 		if (0 < missCnt) {
 			for (int i = peerHas.size() - 1; i >= 0; i--) {
@@ -1765,10 +1788,12 @@ public class UploadPack {
 						case OFF:
 							break;
 						case CONTINUE:
-							out.writeString("ACK " + id.name() + " continue\n"); //$NON-NLS-1$ //$NON-NLS-2$
+							out.writeString(
+									"ACK " + id.name() + " continue\n"); //$NON-NLS-1$ //$NON-NLS-2$
 							break;
 						case DETAILED:
-							out.writeString("ACK " + id.name() + " ready\n"); //$NON-NLS-1$ //$NON-NLS-2$
+							out.writeString(
+									"ACK " + id.name() + " ready\n"); //$NON-NLS-1$ //$NON-NLS-2$
 							sentReady = true;
 							break;
 						}
@@ -1778,15 +1803,14 @@ public class UploadPack {
 			}
 		}
 
-		if (multiAck == MultiAck.DETAILED && !didOkToGiveUp && okToGiveUp()) {
+		if (multiAck == MultiAck.DETAILED && !didOkToGiveUp
+				&& okToGiveUp()) {
 			ObjectId id = peerHas.get(peerHas.size() - 1);
 			out.writeString("ACK " + id.name() + " ready\n"); //$NON-NLS-1$ //$NON-NLS-2$
 			sentReady = true;
 		}
 
-		preUploadHook.onEndNegotiateRound(this, wantAll, haveCnt, missCnt, sentReady);
-		peerHas.clear();
-		return last;
+		return sentReady;
 	}
 
 	private void parseWants(PackStatistics.Accumulator accumulator) throws IOException {