Simplify UploadPack by parsing wants separately from haves

The DHT backend was very slow at parsing objects. To work around
that performance limitation I obfuscated UploadPack by folding both
the want and have sets together in a single parse queue. Since DHT
was removed the complexity is no longer constructive to JGit.

Doing this refactoring prepares the code for a slightly future
change where the have lines need to be handled specially from the
want lines. Splitting the parsing up into two phases makes such
a modification trivial.

Change-Id: If7aad533b82448bbb688278e21f709282e5ccf4b

1 files changed, 60 insertions, 83 deletions

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 bc40454166..f0ba0cdc5b 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -787,74 +787,23 @@ public class UploadPack {
 		preUploadHook.onBeginNegotiateRound(this, wantIds, peerHas.size());
 		if (peerHas.isEmpty())
 			return last;
+		if (wantAll.isEmpty() && !wantIds.isEmpty())
+			parseWants();
 
-		List<ObjectId> toParse = peerHas;
-		HashSet<ObjectId> peerHasSet = null;
-		boolean needMissing = false;
 		sentReady = false;
-
-		if (wantAll.isEmpty() && !wantIds.isEmpty()) {
-			// We have not yet parsed the want list. Parse it now.
-			peerHasSet = new HashSet<ObjectId>(peerHas);
-			int cnt = wantIds.size() + peerHasSet.size();
-			toParse = new ArrayList<ObjectId>(cnt);
-			toParse.addAll(wantIds);
-			toParse.addAll(peerHasSet);
-			needMissing = true;
-		}
-
-		Set<RevObject> notAdvertisedWants = null;
 		int haveCnt = 0;
-		AsyncRevObjectQueue q = walk.parseAny(toParse, needMissing);
+		AsyncRevObjectQueue q = walk.parseAny(peerHas, false);
 		try {
 			for (;;) {
 				RevObject obj;
 				try {
 					obj = q.next();
 				} catch (MissingObjectException notFound) {
-					ObjectId id = notFound.getObjectId();
-					if (wantIds.contains(id)) {
-						String msg = MessageFormat.format(
-								JGitText.get().wantNotValid, id.name());
-						throw new PackProtocolException(msg, notFound);
-					}
 					continue;
 				}
 				if (obj == null)
 					break;
 
-				// If the object is still found in wantIds, the want
-				// list wasn't parsed earlier, and was done in this batch.
-				//
-				if (wantIds.remove(obj)) {
-					if (!advertised.contains(obj) && requestPolicy != RequestPolicy.ANY) {
-						if (notAdvertisedWants == null)
-							notAdvertisedWants = new HashSet<RevObject>();
-						notAdvertisedWants.add(obj);
-					}
-
-					if (!obj.has(WANT)) {
-						obj.add(WANT);
-						wantAll.add(obj);
-					}
-
-					if (!(obj instanceof RevCommit))
-						obj.add(SATISFIED);
-
-					if (obj instanceof RevTag) {
-						RevObject target = walk.peel(obj);
-						if (target instanceof RevCommit) {
-							if (!target.has(WANT)) {
-								target.add(WANT);
-								wantAll.add(target);
-							}
-						}
-					}
-
-					if (!peerHasSet.contains(obj))
-						continue;
-				}
-
 				last = obj;
 				haveCnt++;
 
@@ -891,25 +840,6 @@ public class UploadPack {
 			q.release();
 		}
 
-		// If the client asked for non advertised object, check our policy.
-		if (notAdvertisedWants != null && !notAdvertisedWants.isEmpty()) {
-			switch (requestPolicy) {
-			case ADVERTISED:
-			default:
-				throw new PackProtocolException(MessageFormat.format(
-						JGitText.get().wantNotValid,
-						notAdvertisedWants.iterator().next().name()));
-
-			case REACHABLE_COMMIT:
-				checkNotAdvertisedWants(notAdvertisedWants);
-				break;
-
-			case ANY:
-				// Allow whatever was asked for.
-				break;
-			}
-		}
-
 		int missCnt = peerHas.size() - haveCnt;
 
 		// If we don't have one of the objects but we're also willing to
@@ -952,7 +882,61 @@ public class UploadPack {
 		return last;
 	}
 
-	private void checkNotAdvertisedWants(Set<RevObject> notAdvertisedWants)
+	private void parseWants() throws IOException {
+		AsyncRevObjectQueue q = walk.parseAny(wantIds, true);
+		try {
+			List<RevCommit> checkReachable = null;
+			RevObject obj;
+			while ((obj = q.next()) != null) {
+				if (!advertised.contains(obj)) {
+					switch (requestPolicy) {
+					case ADVERTISED:
+					default:
+						throw new PackProtocolException(MessageFormat.format(
+								JGitText.get().wantNotValid, obj));
+					case REACHABLE_COMMIT:
+						if (!(obj instanceof RevCommit)) {
+							throw new PackProtocolException(MessageFormat.format(
+								JGitText.get().wantNotValid, obj));
+						}
+						if (checkReachable == null)
+							checkReachable = new ArrayList<RevCommit>();
+						checkReachable.add((RevCommit) obj);
+						break;
+					case ANY:
+						break;
+					}
+				}
+				want(obj);
+
+				if (!(obj instanceof RevCommit))
+					obj.add(SATISFIED);
+				if (obj instanceof RevTag) {
+					obj = walk.peel(obj);
+					if (obj instanceof RevCommit)
+						want(obj);
+				}
+			}
+			if (checkReachable != null)
+				checkNotAdvertisedWants(checkReachable);
+			wantIds.clear();
+		} catch (MissingObjectException notFound) {
+			ObjectId id = notFound.getObjectId();
+			throw new PackProtocolException(MessageFormat.format(
+					JGitText.get().wantNotValid, id.name()), notFound);
+		} finally {
+			q.release();
+		}
+	}
+
+	private void want(RevObject obj) {
+		if (!obj.has(WANT)) {
+			obj.add(WANT);
+			wantAll.add(obj);
+		}
+	}
+
+	private void checkNotAdvertisedWants(List<RevCommit> notAdvertisedWants)
 			throws MissingObjectException, IncorrectObjectTypeException, IOException {
 		// Walk the requested commits back to the advertised commits.
 		// If any commit exists, a branch was deleted or rewound and
@@ -960,15 +944,8 @@ public class UploadPack {
 		// If the requested commit is merged into an advertised branch
 		// it will be marked UNINTERESTING and no commits return.
 
-		for (RevObject o : notAdvertisedWants) {
-			if (!(o instanceof RevCommit)) {
-				throw new PackProtocolException(MessageFormat.format(
-						JGitText.get().wantNotValid,
-						notAdvertisedWants.iterator().next().name()));
-			}
-			walk.markStart((RevCommit) o);
-		}
-
+		for (RevCommit c : notAdvertisedWants)
+			walk.markStart(c);
 		for (ObjectId id : advertised) {
 			try {
 				walk.markUninteresting(walk.parseCommit(id));