UploadPack: Defer want-ref resolution to after parsing

ProtocolV2Parser explains:

  // TODO(ifrade): This validation should be done after the
  // protocol parsing. It is not a protocol problem asking for an
  // unexisting ref and we wouldn't need the ref database here.

Do so.  This way all ref database accesses are in one place, in the
UploadPack class.

No user-visible change intended --- this is just to make the code
easier to manipulate.

Change-Id: I68e87dff7b9a63ccc169bd0836e8e8baaf5d1048
Signed-off-by: Jonathan Nieder <jrn@google.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

4 files changed, 50 insertions, 77 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV2ParserTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV2ParserTest.java
index 4d1150844c..bf67d46d51 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV2ParserTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV2ParserTest.java
@@ -42,7 +42,6 @@
  */
 package org.eclipse.jgit.transport;
 
-import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.hasItems;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThat;
@@ -160,8 +159,7 @@ public class ProtocolV2ParserTest {
 				PacketLineIn.END);
 		ProtocolV2Parser parser = new ProtocolV2Parser(
 				ConfigBuilder.getDefault());
-		FetchV2Request request = parser.parseFetchRequest(pckIn,
-				testRepo.getRepository().getRefDatabase());
+		FetchV2Request request = parser.parseFetchRequest(pckIn);
 		assertTrue(request.getOptions()
 				.contains(GitProtocolConstants.OPTION_THIN_PACK));
 		assertTrue(request.getOptions()
@@ -191,8 +189,7 @@ public class ProtocolV2ParserTest {
 				PacketLineIn.END);
 		ProtocolV2Parser parser = new ProtocolV2Parser(
 				ConfigBuilder.getDefault());
-		FetchV2Request request = parser.parseFetchRequest(pckIn,
-				testRepo.getRepository().getRefDatabase());
+		FetchV2Request request = parser.parseFetchRequest(pckIn);
 		assertThat(objIdsAsStrings(request.getClientShallowCommits()),
 				hasItems("28274d02c489f4c7e68153056e9061a46f62d7a0",
 						"145e683b229dcab9d0e2ccb01b386f9ecc17d29d"));
@@ -211,8 +208,7 @@ public class ProtocolV2ParserTest {
 				PacketLineIn.END);
 		ProtocolV2Parser parser = new ProtocolV2Parser(
 				ConfigBuilder.getDefault());
-		FetchV2Request request = parser.parseFetchRequest(pckIn,
-				testRepo.getRepository().getRefDatabase());
+		FetchV2Request request = parser.parseFetchRequest(pckIn);
 		assertThat(objIdsAsStrings(request.getClientShallowCommits()),
 				hasItems("28274d02c489f4c7e68153056e9061a46f62d7a0",
 						"145e683b229dcab9d0e2ccb01b386f9ecc17d29d"));
@@ -229,8 +225,7 @@ public class ProtocolV2ParserTest {
 				PacketLineIn.END);
 		ProtocolV2Parser parser = new ProtocolV2Parser(
 				ConfigBuilder.getDefault());
-		FetchV2Request request = parser.parseFetchRequest(pckIn,
-				testRepo.getRepository().getRefDatabase());
+		FetchV2Request request = parser.parseFetchRequest(pckIn);
 		assertThat(objIdsAsStrings(request.getClientShallowCommits()),
 				hasItems("28274d02c489f4c7e68153056e9061a46f62d7a0",
 						"145e683b229dcab9d0e2ccb01b386f9ecc17d29d"));
@@ -244,8 +239,7 @@ public class ProtocolV2ParserTest {
 				PacketLineIn.END);
 		ProtocolV2Parser parser = new ProtocolV2Parser(
 				ConfigBuilder.start().allowFilter().done());
-		FetchV2Request request = parser.parseFetchRequest(pckIn,
-				testRepo.getRepository().getRefDatabase());
+		FetchV2Request request = parser.parseFetchRequest(pckIn);
 		assertEquals(0, request.getFilterBlobLimit());
 	}
 
@@ -256,8 +250,7 @@ public class ProtocolV2ParserTest {
 				PacketLineIn.END);
 		ProtocolV2Parser parser = new ProtocolV2Parser(
 				ConfigBuilder.start().allowFilter().done());
-		FetchV2Request request = parser.parseFetchRequest(pckIn,
-				testRepo.getRepository().getRefDatabase());
+		FetchV2Request request = parser.parseFetchRequest(pckIn);
 		assertEquals(15, request.getFilterBlobLimit());
 	}
 
@@ -270,8 +263,7 @@ public class ProtocolV2ParserTest {
 				PacketLineIn.END);
 		ProtocolV2Parser parser = new ProtocolV2Parser(
 				ConfigBuilder.start().allowFilter().done());
-		FetchV2Request request = parser.parseFetchRequest(pckIn,
-				testRepo.getRepository().getRefDatabase());
+		FetchV2Request request = parser.parseFetchRequest(pckIn);
 		assertEquals(0, request.getFilterBlobLimit());
 	}
 
@@ -282,8 +274,7 @@ public class ProtocolV2ParserTest {
 				"filter blob:limit=12", PacketLineIn.END);
 		ProtocolV2Parser parser = new ProtocolV2Parser(
 				ConfigBuilder.getDefault());
-		parser.parseFetchRequest(pckIn,
-				testRepo.getRepository().getRefDatabase());
+		parser.parseFetchRequest(pckIn);
 	}
 
 	@Test
@@ -300,23 +291,16 @@ public class ProtocolV2ParserTest {
 		ProtocolV2Parser parser = new ProtocolV2Parser(
 				ConfigBuilder.start().allowRefInWant().done());
 
-
-		FetchV2Request request = parser.parseFetchRequest(pckIn,
-				testRepo.getRepository().getRefDatabase());
+		FetchV2Request request = parser.parseFetchRequest(pckIn);
 		assertEquals(1, request.getWantedRefs().size());
-		assertThat(request.getWantedRefs().keySet(),
-				hasItems("refs/heads/branchA"));
-		assertEquals(2, request.getWantsIds().size());
+		assertThat(request.getWantedRefs(), hasItems("refs/heads/branchA"));
+		assertEquals(1, request.getWantsIds().size());
 		assertThat(objIdsAsStrings(request.getWantsIds()),
-				hasItems("e4980cdc48cfa1301493ca94eb70523f6788b819",
-						one.getName()));
+				hasItems("e4980cdc48cfa1301493ca94eb70523f6788b819"));
 	}
 
 	@Test
 	public void testFetchWithRefInWantUnknownRef() throws Exception {
-		thrown.expect(PackProtocolException.class);
-		thrown.expectMessage(containsString("refs/heads/branchC"));
-
 		PacketLineIn pckIn = formatAsPacketLine(PacketLineIn.DELIM,
 				"want e4980cdc48cfa1301493ca94eb70523f6788b819",
 				"want-ref refs/heads/branchC",
@@ -329,8 +313,9 @@ public class ProtocolV2ParserTest {
 		testRepo.update("branchA", one);
 		testRepo.update("branchB", two);
 
-		parser.parseFetchRequest(pckIn,
-				testRepo.getRepository().getRefDatabase());
+		FetchV2Request request = parser.parseFetchRequest(pckIn);
+		assertEquals(1, request.getWantedRefs().size());
+		assertThat(request.getWantedRefs(), hasItems("refs/heads/branchC"));
 	}
 
 }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java
index 853d96905c..34f3484951 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java
@@ -45,9 +45,7 @@ package org.eclipse.jgit.transport;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
-import java.util.TreeMap;
 
 import org.eclipse.jgit.annotations.NonNull;
 import org.eclipse.jgit.lib.ObjectId;
@@ -63,7 +61,7 @@ import org.eclipse.jgit.lib.ObjectId;
 public final class FetchV2Request {
 	private final List<ObjectId> peerHas;
 
-	private final TreeMap<String, ObjectId> wantedRefs;
+	private final List<String> wantedRefs;
 
 	private final Set<ObjectId> wantsIds;
 
@@ -82,7 +80,7 @@ public final class FetchV2Request {
 	private final boolean doneReceived;
 
 	private FetchV2Request(List<ObjectId> peerHas,
-			TreeMap<String, ObjectId> wantedRefs, Set<ObjectId> wantsIds,
+			List<String> wantedRefs, Set<ObjectId> wantsIds,
 			Set<ObjectId> clientShallowCommits, int deepenSince,
 			List<String> deepenNotRefs, int depth, long filterBlobLimit,
 			boolean doneReceived, Set<String> options) {
@@ -110,12 +108,12 @@ public final class FetchV2Request {
 	 * @return list of references in the "want-ref" lines of the request
 	 */
 	@NonNull
-	Map<String, ObjectId> getWantedRefs() {
-		return this.wantedRefs;
+	List<String> getWantedRefs() {
+		return wantedRefs;
 	}
 
 	/**
-	 * @return object ids in the "want" (and "want-ref") lines of the request
+	 * @return object ids in the "want" (but not "want-ref") lines of the request
 	 */
 	@NonNull
 	Set<ObjectId> getWantsIds() {
@@ -198,7 +196,7 @@ public final class FetchV2Request {
 	static final class Builder {
 		List<ObjectId> peerHas = new ArrayList<>();
 
-		TreeMap<String, ObjectId> wantedRefs = new TreeMap<>();
+		List<String> wantedRefs = new ArrayList<>();
 
 		Set<ObjectId> wantsIds = new HashSet<>();
 
@@ -234,12 +232,10 @@ public final class FetchV2Request {
 		 *
 		 * @param refName
 		 *            reference name
-		 * @param oid
-		 *            object id
 		 * @return the builder
 		 */
-		Builder addWantedRef(String refName, ObjectId oid) {
-			wantedRefs.put(refName, oid);
+		Builder addWantedRef(String refName) {
+			wantedRefs.add(refName);
 			return this;
 		}
 
@@ -258,7 +254,7 @@ public final class FetchV2Request {
 		 *            from a "want" line in a fetch request
 		 * @return the builder
 		 */
-		Builder addWantsIds(ObjectId objectId) {
+		Builder addWantsId(ObjectId objectId) {
 			wantsIds.add(objectId);
 			return this;
 		}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java
index eae2c6edb9..2cc50a7f38 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java
@@ -57,8 +57,6 @@ import java.text.MessageFormat;
 import org.eclipse.jgit.errors.PackProtocolException;
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.Ref;
-import org.eclipse.jgit.lib.RefDatabase;
 
 /**
  * Parse the incoming git protocol lines from the wire and translate them into a
@@ -79,24 +77,17 @@ final class ProtocolV2Parser {
 	 * Parse the incoming fetch request arguments from the wire. The caller must
 	 * be sure that what is comings is a fetch request before coming here.
 	 *
-	 * This operation requires the reference database to validate incoming
-	 * references.
-	 *
 	 * @param pckIn
 	 *            incoming lines
-	 * @param refdb
-	 *            reference database (to validate that received references exist
-	 *            and point to valid objects)
 	 * @return A FetchV2Request populated with information received from the
 	 *         wire.
 	 * @throws PackProtocolException
 	 *             incompatible options, wrong type of arguments or other issues
 	 *             where the request breaks the protocol.
 	 * @throws IOException
-	 *             an IO error prevented reading the incoming message or
-	 *             accessing the ref database.
+	 *             an IO error prevented reading the incoming message.
 	 */
-	FetchV2Request parseFetchRequest(PacketLineIn pckIn, RefDatabase refdb)
+	FetchV2Request parseFetchRequest(PacketLineIn pckIn)
 			throws PackProtocolException, IOException {
 		FetchV2Request.Builder reqBuilder = FetchV2Request.builder();
 
@@ -116,25 +107,10 @@ final class ProtocolV2Parser {
 		boolean filterReceived = false;
 		while ((line = pckIn.readString()) != PacketLineIn.END) {
 			if (line.startsWith("want ")) { //$NON-NLS-1$
-				reqBuilder.addWantsIds(ObjectId.fromString(line.substring(5)));
+				reqBuilder.addWantsId(ObjectId.fromString(line.substring(5)));
 			} else if (transferConfig.isAllowRefInWant()
 					&& line.startsWith(OPTION_WANT_REF + " ")) { //$NON-NLS-1$
-				String refName = line.substring(OPTION_WANT_REF.length() + 1);
-				// TODO(ifrade): This validation should be done after the
-				// protocol parsing. It is not a protocol problem asking for an
-				// unexisting ref and we wouldn't need the ref database here
-				Ref ref = refdb.exactRef(refName);
-				if (ref == null) {
-					throw new PackProtocolException(MessageFormat
-							.format(JGitText.get().invalidRefName, refName));
-				}
-				ObjectId oid = ref.getObjectId();
-				if (oid == null) {
-					throw new PackProtocolException(MessageFormat
-							.format(JGitText.get().invalidRefName, refName));
-				}
-				reqBuilder.addWantedRef(refName, oid);
-				reqBuilder.addWantsIds(oid);
+				reqBuilder.addWantedRef(line.substring(OPTION_WANT_REF.length() + 1));
 			} else if (line.startsWith("have ")) { //$NON-NLS-1$
 				reqBuilder.addPeerHas(ObjectId.fromString(line.substring(5)));
 			} else if (line.equals("done")) { //$NON-NLS-1$
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 c753bcdc7a..7e29c9b7a2 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -79,6 +79,7 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.TreeMap;
 
 import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.errors.CorruptObjectException;
@@ -994,8 +995,7 @@ public class UploadPack {
 		}
 
 		ProtocolV2Parser parser = new ProtocolV2Parser(transferConfig);
-		FetchV2Request req = parser.parseFetchRequest(pckIn,
-				db.getRefDatabase());
+		FetchV2Request req = parser.parseFetchRequest(pckIn);
 		rawOut.stopBuffering();
 
 		protocolV2Hook.onFetch(req);
@@ -1003,13 +1003,29 @@ public class UploadPack {
 		// TODO(ifrade): Refactor to pass around the Request object, instead of
 		// copying data back to class fields
 		options = req.getOptions();
-		wantIds.addAll(req.getWantsIds());
 		clientShallowCommits = req.getClientShallowCommits();
 		depth = req.getDepth();
 		shallowSince = req.getDeepenSince();
 		filterBlobLimit = req.getFilterBlobLimit();
 		deepenNotRefs = req.getDeepenNotRefs();
 
+		wantIds.addAll(req.getWantsIds());
+		Map<String, ObjectId> wantedRefs = new TreeMap<>();
+		for (String refName : req.getWantedRefs()) {
+			Ref ref = db.getRefDatabase().exactRef(refName);
+			if (ref == null) {
+				throw new PackProtocolException(MessageFormat
+						.format(JGitText.get().invalidRefName, refName));
+			}
+			ObjectId oid = ref.getObjectId();
+			if (oid == null) {
+				throw new PackProtocolException(MessageFormat
+						.format(JGitText.get().invalidRefName, refName));
+			}
+			wantIds.add(oid);
+			wantedRefs.put(refName, oid);
+		}
+
 		boolean sectionSent = false;
 		@Nullable List<ObjectId> shallowCommits = null;
 		List<ObjectId> unshallowCommits = new ArrayList<>();
@@ -1059,13 +1075,13 @@ public class UploadPack {
 				sectionSent = true;
 			}
 
-			if (!req.getWantedRefs().isEmpty()) {
+			if (!wantedRefs.isEmpty()) {
 				if (sectionSent) {
 					pckOut.writeDelim();
 				}
 				pckOut.writeString("wanted-refs\n"); //$NON-NLS-1$
-				for (Map.Entry<String, ObjectId> entry : req.getWantedRefs()
-						.entrySet()) {
+				for (Map.Entry<String, ObjectId> entry :
+						wantedRefs.entrySet()) {
 					pckOut.writeString(entry.getValue().getName() + ' ' +
 							entry.getKey() + '\n');
 				}