UploadPack: Refactor to generalize the object reachability checks

ObjectWalk#createObjectReachabilityChecker() returns the best
implementation for the repo. UploadPack can use the interface and fold
the with/without commits cases in one code path.

Change-Id: I857c11735d1d8e36c3ed8185ff11de8a62e86540
Signed-off-by: Ivan Frade <ifrade@google.com>

4 files changed, 34 insertions, 33 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/BitmappedReachabilityChecker.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/BitmappedReachabilityChecker.java
index 02514526d0..0d9c4593bf 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/BitmappedReachabilityChecker.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/BitmappedReachabilityChecker.java
@@ -42,7 +42,7 @@ class BitmappedReachabilityChecker implements ReachabilityChecker {
 	 * @throws IOException
 	 *             if the index or the object reader cannot be opened.
 	 */
-	public BitmappedReachabilityChecker(RevWalk walk)
+	BitmappedReachabilityChecker(RevWalk walk)
 			throws IOException {
 		this.walk = walk;
 		if (walk.getObjectReader().getBitmapIndex() == null) {
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/ObjectWalk.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/ObjectWalk.java
index 04a4b4c631..4c7a6f556e 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/ObjectWalk.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/ObjectWalk.java
@@ -160,6 +160,29 @@ public class ObjectWalk extends RevWalk {
 	}
 
 	/**
+	 * Create an object reachability checker that will use bitmaps if possible.
+	 *
+	 * This reachability checker accepts any object as target. For checks
+	 * exclusively between commits, see
+	 * {@link RevWalk#createReachabilityChecker()}.
+	 *
+	 * @return an object reachability checker, using bitmaps if possible.
+	 *
+	 * @throws IOException
+	 *             when the index fails to load.
+	 *
+	 * @since 5.8
+	 */
+	public ObjectReachabilityChecker createObjectReachabilityChecker()
+			throws IOException {
+		if (reader.getBitmapIndex() != null) {
+			return new BitmappedObjectReachabilityChecker(this);
+		}
+
+		return new PedestrianObjectReachabilityChecker(this);
+	}
+
+	/**
 	 * Mark an object or commit to start graph traversal from.
 	 * <p>
 	 * Callers are encouraged to use
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/PedestrianObjectReachabilityChecker.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/PedestrianObjectReachabilityChecker.java
index 90cb2fac4d..55c5156db5 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/PedestrianObjectReachabilityChecker.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/PedestrianObjectReachabilityChecker.java
@@ -22,7 +22,7 @@ import org.eclipse.jgit.errors.MissingObjectException;
  * Checks if all objects are reachable from certain starting points doing a
  * walk.
  */
-public class PedestrianObjectReachabilityChecker implements ObjectReachabilityChecker {
+class PedestrianObjectReachabilityChecker implements ObjectReachabilityChecker {
 	private ObjectWalk walk;
 
 	/**
@@ -31,7 +31,7 @@ public class PedestrianObjectReachabilityChecker implements ObjectReachabilityCh
 	 * @param walk
 	 *            ObjectWalk instance to reuse. Caller retains ownership.
 	 */
-	public PedestrianObjectReachabilityChecker(ObjectWalk walk) {
+	PedestrianObjectReachabilityChecker(ObjectWalk walk) {
 		this.walk = walk;
 	}
 
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 95e2e5a145..cf504d25ad 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -75,11 +75,9 @@ import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefDatabase;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.AsyncRevObjectQueue;
-import org.eclipse.jgit.revwalk.BitmappedObjectReachabilityChecker;
 import org.eclipse.jgit.revwalk.DepthWalk;
 import org.eclipse.jgit.revwalk.ObjectReachabilityChecker;
 import org.eclipse.jgit.revwalk.ObjectWalk;
-import org.eclipse.jgit.revwalk.PedestrianObjectReachabilityChecker;
 import org.eclipse.jgit.revwalk.ReachabilityChecker;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevFlag;
@@ -1918,44 +1916,24 @@ public class UploadPack {
 			boolean repoHasBitmaps = reader.getBitmapIndex() != null;
 
 			if (!allWantsAreCommits) {
-				if (!repoHasBitmaps) {
-					if (up.transferConfig.isAllowFilter()) {
-						// Use allowFilter as an indication that the server
-						// operator is willing to pay the cost of these
-						// reachability checks.
-						try (ObjectWalk objWalk = walk.toObjectWalkWithSameObjects()) {
-							List<RevObject> havesAsObjs = objectIdsToRevObjects(
-									objWalk, reachableFrom);
-							ObjectReachabilityChecker reachabilityChecker = new PedestrianObjectReachabilityChecker(
-									objWalk);
-							Optional<RevObject> unreachable = reachabilityChecker
-									.areAllReachable(wantsAsObjs,
-											havesAsObjs.stream());
-							if (unreachable.isPresent()) {
-								throw new WantNotValidException(
-										unreachable.get());
-							}
-						}
-						return;
-					}
-
-					// If unadvertized non-commits are requested, use
-					// bitmaps. If there are no bitmaps, instead of
-					// incurring the expense of a manual walk, reject
-					// the request.
+				if (!repoHasBitmaps && !up.transferConfig.isAllowFilter()) {
+					// Checking unadvertised non-commits without bitmaps
+					// requires an expensive manual walk. Use allowFilter as an
+					// indication that the server operator is willing to pay
+					// this cost. Reject the request otherwise.
 					RevObject nonCommit = wantsAsObjs
 							.stream()
 							.filter(obj -> !(obj instanceof RevCommit))
 							.limit(1)
 							.collect(Collectors.toList()).get(0);
 					throw new WantNotValidException(nonCommit);
-
 				}
+
 				try (ObjectWalk objWalk = walk.toObjectWalkWithSameObjects()) {
 					List<RevObject> havesAsObjs = objectIdsToRevObjects(objWalk,
 							reachableFrom);
-					ObjectReachabilityChecker reachabilityChecker = new BitmappedObjectReachabilityChecker(
-							objWalk);
+					ObjectReachabilityChecker reachabilityChecker = objWalk
+							.createObjectReachabilityChecker();
 					Optional<RevObject> unreachable = reachabilityChecker
 							.areAllReachable(wantsAsObjs, havesAsObjs.stream());
 					if (unreachable.isPresent()) {