From 2ff0c0abaaa88f26eda4ae553cbbb954c74642df Mon Sep 17 00:00:00 2001
From: Ivan Frade <ifrade@google.com>
Date: Tue, 12 Nov 2019 14:33:29 -0800
Subject: [PATCH] UploadPack: Prioritize references for non-advertised wants
 checks

UploadPack needs to check if object ids that weren't advertised before
are reachable from the references visible to the user. In the
bitmap-based reachability check, this is done incrementally: checking
against one reference, if anything remaining adding a second and so on.
It is more efficient to check first more common references (e.g. refs/heads/*)

Sort the references for the reachability checker. This should solve the
connectivity earlier and require less bitmap creation and less memory.

Change-Id: I48ac10d71e29fab2d346479802401eaea4aacb5c
Signed-off-by: Ivan Frade <ifrade@google.com>
---
 .../eclipse/jgit/transport/UploadPack.java    | 51 ++++++++++++++++---
 1 file changed, 44 insertions(+), 7 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 5bee1ce9a..5d952ff49 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -85,6 +85,7 @@
 import java.util.Optional;
 import java.util.Set;
 import java.util.TreeMap;
+import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
 import org.eclipse.jgit.annotations.NonNull;
@@ -1873,8 +1874,7 @@ public static final class ReachableCommitRequestValidator
 		@Override
 		public void checkWants(UploadPack up, List<ObjectId> wants)
 				throws PackProtocolException, IOException {
-			checkNotAdvertisedWants(up, wants,
-					refIdSet(up.getAdvertisedRefs().values()));
+			checkNotAdvertisedWants(up, wants, up.getAdvertisedRefs().values());
 		}
 	}
 
@@ -1911,7 +1911,7 @@ public static final class ReachableCommitTipRequestValidator
 		public void checkWants(UploadPack up, List<ObjectId> wants)
 				throws PackProtocolException, IOException {
 			checkNotAdvertisedWants(up, wants,
-					refIdSet(up.getRepository().getRefDatabase().getRefs()));
+					up.getRepository().getRefDatabase().getRefs());
 		}
 	}
 
@@ -1971,12 +1971,14 @@ private static void checkReachabilityByWalkingObjects(ObjectWalk walk,
 	}
 
 	private static void checkNotAdvertisedWants(UploadPack up,
-			List<ObjectId> notAdvertisedWants, Set<ObjectId> reachableFrom)
+			List<ObjectId> notAdvertisedWants, Collection<Ref> visibleRefs)
 			throws IOException {
 
 		ObjectReader reader = up.getRevWalk().getObjectReader();
+
 		try (RevWalk walk = new RevWalk(reader)) {
 			walk.setRetainBody(false);
+			Set<ObjectId> reachableFrom = refIdSet(visibleRefs);
 			// Missing "wants" throw exception here
 			List<RevObject> wantsAsObjs = objectIdsToRevObjects(walk,
 					notAdvertisedWants);
@@ -2023,10 +2025,11 @@ private static void checkNotAdvertisedWants(UploadPack up,
 			ReachabilityChecker reachabilityChecker = walk
 					.createReachabilityChecker();
 
-			List<RevCommit> starters = objectIdsToRevCommits(walk,
-					reachableFrom);
+			List<Ref> sortedVisibleRefs = moreImportantRefsFirst(visibleRefs);
+			List<RevCommit> reachableCommits = refsToRevCommits(walk,
+					sortedVisibleRefs);
 			Optional<RevCommit> unreachable = reachabilityChecker
-					.areAllReachable(wantsAsCommits, starters);
+					.areAllReachable(wantsAsCommits, reachableCommits);
 			if (unreachable.isPresent()) {
 				throw new WantNotValidException(unreachable.get());
 			}
@@ -2036,6 +2039,40 @@ private static void checkNotAdvertisedWants(UploadPack up,
 		}
 	}
 
+	private static List<Ref> moreImportantRefsFirst(
+			Collection<Ref> visibleRefs) {
+		Predicate<Ref> startsWithRefsHeads = ref -> ref.getName()
+				.startsWith(Constants.R_HEADS);
+		Predicate<Ref> startsWithRefsTags = ref -> ref.getName()
+				.startsWith(Constants.R_TAGS);
+		Predicate<Ref> allOther = ref -> !startsWithRefsHeads.test(ref)
+				&& !startsWithRefsTags.test(ref);
+
+		List<Ref> sorted = new ArrayList<>(visibleRefs.size());
+		sorted.addAll(filterRefByPredicate(visibleRefs, startsWithRefsHeads));
+		sorted.addAll(filterRefByPredicate(visibleRefs, startsWithRefsTags));
+		sorted.addAll(filterRefByPredicate(visibleRefs, allOther));
+
+		return sorted;
+	}
+
+	private static List<Ref> filterRefByPredicate(Collection<Ref> refs,
+			Predicate<Ref> predicate) {
+		return refs.stream().filter(predicate).collect(Collectors.toList());
+	}
+
+	private static List<RevCommit> refsToRevCommits(RevWalk walk,
+			List<Ref> refs) throws MissingObjectException, IOException {
+		List<ObjectId> objIds = refs.stream().map(
+				ref -> firstNonNull(ref.getPeeledObjectId(), ref.getObjectId()))
+				.collect(Collectors.toList());
+		return objectIdsToRevCommits(walk, objIds);
+	}
+
+	private static ObjectId firstNonNull(ObjectId one, ObjectId two) {
+		return one != null ? one : two;
+	}
+
 	// Resolve the ObjectIds into RevObjects. Any missing object raises an
 	// exception
 	private static List<RevObject> objectIdsToRevObjects(RevWalk walk,