Revert "Client-side protocol V2 support for fetching"

This reverts commit f802f06e7fd5a98f256b7b7727598491f563bf2f.

I had misunderstood how protocol V2 works. This implementation only
works if the negotiation during fetch is done in one round.

Fixing this is substantial work in BasePackFetchConnection. Basically
I think I'd have to change back negotiate to the V0 version, and have
a doFetch() that does

  if protocol V2
    doFetchV2()
  else
    doFetchV0()

with doFetchV0 the old code, and doFetchV2 completely new.

Plus there would need to be a HTTP test case requiring several
negotiation rounds.

This is a couple of days work at least, and I don't know when I will
have the time to revisit this. So although the rest of the code is
fine I prefer to back this out completely and not leave a only half
working implementation in the code for an indeterminate time.

Bug: 553083
Change-Id: Icbbbb09882b3b83f9897deac4a06d5f8dc99d84e
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

1 files changed, 16 insertions, 171 deletions

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 2432641ede..a2fb51f46d 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java
@@ -16,12 +16,9 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.text.MessageFormat;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Date;
-import java.util.HashSet;
-import java.util.LinkedHashSet;
 import java.util.Set;
 
 import org.eclipse.jgit.errors.PackProtocolException;
@@ -215,8 +212,6 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 
 	private PacketLineOut pckState;
 
-	private Set<String> fetchCapabilities = new HashSet<>();
-
 	/**
 	 * Either FilterSpec.NO_FILTER for a filter that doesn't filter
 	 * anything, or a filter that indicates what and what not to send to the
@@ -359,28 +354,6 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 				pckState = new PacketLineOut(state);
 			}
 
-			if (TransferConfig.ProtocolVersion.V2
-					.equals(getProtocolVersion())) {
-				sideband = true;
-				noDone = true;
-				multiAck = MultiAck.DETAILED;
-				setFetchOptions();
-				PacketLineOut output = statelessRPC ? pckState : pckOut;
-				output.writeString(
-						"command=" + GitProtocolConstants.COMMAND_FETCH); //$NON-NLS-1$
-				// Capabilities are sent as command arguments in protocol V2
-				String agent = UserAgent.get();
-				if (agent != null
-						&& isCapableOf(GitProtocolConstants.OPTION_AGENT)) {
-					output.writeString(
-							GitProtocolConstants.OPTION_AGENT + '=' + agent);
-				}
-				output.writeDelim();
-				// Arguments
-				for (String capability : getCapabilitiesV2()) {
-					output.writeString(capability);
-				}
-			}
 			if (sendWants(want)) {
 				negotiate(monitor);
 
@@ -389,25 +362,6 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 				state = null;
 				pckState = null;
 
-				if (TransferConfig.ProtocolVersion.V2
-						.equals(getProtocolVersion())) {
-					String header = pckIn.readString();
-					if (PacketLineIn.isEnd(header)) {
-						// No packfile following.
-						return;
-					}
-
-					if (header.startsWith("ERR ")) { //$NON-NLS-1$
-						// Protocol V2 may give us an error here (for instance,
-						// invalid want)
-						throw new PackProtocolException(header.substring(4));
-					}
-					if (!GitProtocolConstants.SECTION_PACKFILE.equals(header)) {
-						throw new PackProtocolException(MessageFormat.format(
-								JGitText.get().expectedGot,
-								GitProtocolConstants.SECTION_PACKFILE, header));
-					}
-				}
 				receivePack(monitor, outputStream);
 			}
 		} catch (CancelledException ce) {
@@ -427,14 +381,6 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 		super.close();
 	}
 
-	private void setFetchOptions() {
-		String advertised = getCapability(GitProtocolConstants.COMMAND_FETCH);
-		if (advertised == null) {
-			return;
-		}
-		fetchCapabilities.addAll(Arrays.asList(advertised.split(" "))); //$NON-NLS-1$
-	}
-
 	FetchConfig getFetchConfig() {
 		return local.getConfig().get(FetchConfig::new);
 	}
@@ -533,11 +479,10 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 			final StringBuilder line = new StringBuilder(46);
 			line.append("want "); //$NON-NLS-1$
 			line.append(objectId.name());
-			if (first && TransferConfig.ProtocolVersion.V0
-					.equals(getProtocolVersion())) {
+			if (first) {
 				line.append(enableCapabilities());
+				first = false;
 			}
-			first = false;
 			line.append('\n');
 			p.writeString(line.toString());
 		}
@@ -547,35 +492,11 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 		if (!filterSpec.isNoOp()) {
 			p.writeString(filterSpec.filterLine());
 		}
-		if (TransferConfig.ProtocolVersion.V0.equals(getProtocolVersion())) {
-			p.end();
-			outNeedsEnd = false;
-		}
+		p.end();
+		outNeedsEnd = false;
 		return true;
 	}
 
-	private Set<String> getCapabilitiesV2() throws TransportException {
-		Set<String> capabilities = new LinkedHashSet<>();
-		if (noProgress) {
-			capabilities.add(OPTION_NO_PROGRESS);
-		}
-		if (includeTags) {
-			capabilities.add(OPTION_INCLUDE_TAG);
-		}
-		if (allowOfsDelta) {
-			capabilities.add(OPTION_OFS_DELTA);
-		}
-		if (thinPack) {
-			capabilities.add(OPTION_THIN_PACK);
-		}
-		if (!filterSpec.isNoOp()
-				&& !fetchCapabilities.contains(OPTION_FILTER)) {
-			throw new PackProtocolException(uri,
-					JGitText.get().filterRequiresCapability);
-		}
-		return capabilities;
-	}
-
 	private String enableCapabilities() throws TransportException {
 		final StringBuilder line = new StringBuilder();
 		if (noProgress)
@@ -629,7 +550,6 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 		boolean receivedContinue = false;
 		boolean receivedAck = false;
 		boolean receivedReady = false;
-		boolean needsAcknowledgementV2 = true;
 
 		if (statelessRPC) {
 			state.writeTo(out, null);
@@ -643,7 +563,7 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 			}
 
 			ObjectId o = c.getId();
-			pckOut.writeString("have " + o.name() + '\n'); //$NON-NLS-1$
+			pckOut.writeString("have " + o.name() + "\n"); //$NON-NLS-1$ //$NON-NLS-2$
 			havesSent++;
 			havesSinceLastContinue++;
 
@@ -660,7 +580,6 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 			}
 
 			pckOut.end();
-			outNeedsEnd = false;
 			resultsPending++; // Each end will cause a result to come back.
 
 			if (havesSent == 32 && !statelessRPC) {
@@ -672,32 +591,9 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 				continue;
 			}
 
-			// Read the section header
-			if (needsAcknowledgementV2 && TransferConfig.ProtocolVersion.V2
-					.equals(getProtocolVersion())) {
-				String header = pckIn.readString();
-				if (!GitProtocolConstants.SECTION_ACKNOWLEDGMENTS
-						.equals(header)) {
-					throw new PackProtocolException(MessageFormat.format(
-							JGitText.get().expectedGot,
-							GitProtocolConstants.SECTION_ACKNOWLEDGMENTS,
-							header));
-				}
-				needsAcknowledgementV2 = false;
-			}
 			READ_RESULT: for (;;) {
-				AckNackResult anr = pckIn.readACKorEOF(ackId);
+				final AckNackResult anr = pckIn.readACK(ackId);
 				switch (anr) {
-				case ACK_EOF:
-					if (TransferConfig.ProtocolVersion.V0
-							.equals(getProtocolVersion())) {
-						throw new PackProtocolException(
-								JGitText.get().expectedACKNAKFoundEOF);
-					}
-					// More lines needed
-					resultsPending--;
-					break READ_RESULT;
-
 				case NAK:
 					// More have lines are necessary to compute the
 					// pack on the remote side. Keep doing that.
@@ -706,24 +602,17 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 					break READ_RESULT;
 
 				case ACK:
-					if (TransferConfig.ProtocolVersion.V0
-							.equals(getProtocolVersion())) {
-						// The remote side is happy and knows exactly what
-						// to send us. There is no further negotiation and
-						// we can break out immediately.
-						//
-						multiAck = MultiAck.OFF;
-						resultsPending = 0;
-						receivedAck = true;
-						if (statelessRPC) {
-							state.writeTo(out, null);
-						}
-						break SEND_HAVES;
-					}
-					// Keep on reading ACKs until we get the ACK_READY
-					markCommon(walk.parseAny(ackId), AckNackResult.ACK_COMMON);
+					// The remote side is happy and knows exactly what
+					// to send us. There is no further negotiation and
+					// we can break out immediately.
+					//
+					multiAck = MultiAck.OFF;
+					resultsPending = 0;
 					receivedAck = true;
-					break;
+					if (statelessRPC) {
+						state.writeTo(out, null);
+					}
+					break SEND_HAVES;
 
 				case ACK_CONTINUE:
 				case ACK_COMMON:
@@ -739,12 +628,6 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 					havesSinceLastContinue = 0;
 					if (anr == AckNackResult.ACK_READY) {
 						receivedReady = true;
-						if (TransferConfig.ProtocolVersion.V2
-								.equals(getProtocolVersion())) {
-							multiAck = MultiAck.OFF;
-							resultsPending = 0;
-							break SEND_HAVES;
-						}
 					}
 					break;
 				}
@@ -778,36 +661,15 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 			throw new CancelledException();
 		}
 
-		if (receivedReady && TransferConfig.ProtocolVersion.V2
-				.equals(getProtocolVersion())) {
-			// We'll get the packfile right away. Skip the delimiter.
-			String delim = pckIn.readString();
-			if (!PacketLineIn.isDelimiter(delim)) {
-				throw new PackProtocolException(MessageFormat
-						.format(JGitText.get().expectedGot, "0001", delim)); //$NON-NLS-1$
-			}
-			return;
-		}
 		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"); //$NON-NLS-1$
-			if (TransferConfig.ProtocolVersion.V2
-					.equals(getProtocolVersion())) {
-				pckOut.end();
-				outNeedsEnd = false;
-				pckOut.flush();
-				// Protocol V2 will skip acknowledgments completely if we send
-				// a done.
-				return;
-			}
 			pckOut.flush();
 		}
 
-		// Below is protocol V0 only.
-
 		if (!receivedAck) {
 			// Apparently if we have never received an ACK earlier
 			// there is one more result expected from the done we
@@ -827,14 +689,6 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 				//
 				break;
 
-			case ACK_EOF:
-				if (TransferConfig.ProtocolVersion.V0
-						.equals(getProtocolVersion())) {
-					throw new PackProtocolException(
-							JGitText.get().expectedACKNAKFoundEOF);
-				}
-				break READ_RESULT;
-
 			case ACK:
 				// A solitary ACK at this point means the remote won't
 				// speak anymore, but is going to send us a pack now.
@@ -843,16 +697,7 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 
 			case ACK_CONTINUE:
 			case ACK_COMMON:
-				// We will expect a normal ACK to break out of the loop.
-				//
-				multiAck = MultiAck.CONTINUE;
-				break;
-
 			case ACK_READY:
-				if (TransferConfig.ProtocolVersion.V2
-						.equals(getProtocolVersion())) {
-					break READ_RESULT;
-				}
 				// We will expect a normal ACK to break out of the loop.
 				//
 				multiAck = MultiAck.CONTINUE;