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

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 bc40454166021c7fc559c3683283a6a29fae6215..f0ba0cdc5b616b041cafd2abe1e983cdcf800e85 100644 (file)
--- 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));