diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackRefSortingForReachabilityTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackRefSortingForReachabilityTest.java new file mode 100644 index 000000000..c9632aefa --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackRefSortingForReachabilityTest.java @@ -0,0 +1,33 @@ +package org.eclipse.jgit.transport; + +import static org.hamcrest.Matchers.contains; +import static org.junit.Assert.assertThat; + +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectIdRef.Unpeeled; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.Ref.Storage; +import org.junit.Test; + +public class UploadPackRefSortingForReachabilityTest { + + @Test + public void sortReferences() { + List refs = Stream.of("refs/changes/12/12", "refs/changes/12/1", + "refs/heads/master", "refs/heads/something", + "refs/changes/55/1", "refs/tags/v1.1") + .map(s -> new Unpeeled(Storage.LOOSE, s, ObjectId.zeroId())) + .collect(Collectors.toList()); + Stream sorted = UploadPack.importantRefsFirst(refs); + List collected = sorted.map(Ref::getName) + .collect(Collectors.toList()); + assertThat(collected, + contains("refs/heads/master", "refs/heads/something", + "refs/tags/v1.1", "refs/changes/12/12", + "refs/changes/12/1", "refs/changes/55/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 a26d23d6d..6ee280b81 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -87,6 +87,7 @@ import java.util.Set; import java.util.TreeMap; import java.util.function.Predicate; import java.util.stream.Collectors; +import java.util.stream.Stream; import org.eclipse.jgit.annotations.NonNull; import org.eclipse.jgit.annotations.Nullable; @@ -2025,11 +2026,13 @@ public class UploadPack { ReachabilityChecker reachabilityChecker = walk .createReachabilityChecker(); - List sortedVisibleRefs = moreImportantRefsFirst(visibleRefs); - List reachableCommits = refsToRevCommits(walk, - sortedVisibleRefs); + Stream reachableCommits = importantRefsFirst(visibleRefs) + .map(UploadPack::refToObjectId) + .map(objId -> objectIdToRevCommit(walk, objId)) + .filter(Objects::nonNull); // Ignore missing tips + Optional unreachable = reachabilityChecker - .areAllReachable(wantsAsCommits, reachableCommits.stream()); + .areAllReachable(wantsAsCommits, reachableCommits); if (unreachable.isPresent()) { throw new WantNotValidException(unreachable.get()); } @@ -2039,7 +2042,7 @@ public class UploadPack { } } - private static List moreImportantRefsFirst( + static Stream importantRefsFirst( Collection visibleRefs) { Predicate startsWithRefsHeads = ref -> ref.getName() .startsWith(Constants.R_HEADS); @@ -2048,29 +2051,39 @@ public class UploadPack { Predicate allOther = ref -> !startsWithRefsHeads.test(ref) && !startsWithRefsTags.test(ref); - List sorted = new ArrayList<>(visibleRefs.size()); - sorted.addAll(filterRefByPredicate(visibleRefs, startsWithRefsHeads)); - sorted.addAll(filterRefByPredicate(visibleRefs, startsWithRefsTags)); - sorted.addAll(filterRefByPredicate(visibleRefs, allOther)); - - return sorted; + return Stream.concat( + visibleRefs.stream().filter(startsWithRefsHeads), + Stream.concat( + visibleRefs.stream().filter(startsWithRefsTags), + visibleRefs.stream().filter(allOther))); } - private static List filterRefByPredicate(Collection refs, - Predicate predicate) { - return refs.stream().filter(predicate).collect(Collectors.toList()); + private static ObjectId refToObjectId(Ref ref) { + return ref.getObjectId() != null ? ref.getObjectId() + : ref.getPeeledObjectId(); } - private static List refsToRevCommits(RevWalk walk, - List refs) throws MissingObjectException, IOException { - List objIds = refs.stream().map( - ref -> firstNonNull(ref.getPeeledObjectId(), ref.getObjectId())) - .collect(Collectors.toList()); - return objectIdsToRevCommits(walk, objIds); - } + /** + * Translate an object id to a RevCommit. + * + * @param walk + * walk on the relevant object storae + * @param objectId + * Object Id + * @return RevCommit instance or null if the object is missing + */ + @Nullable + private static RevCommit objectIdToRevCommit(RevWalk walk, + ObjectId objectId) { + if (objectId == null) { + return null; + } - private static ObjectId firstNonNull(ObjectId one, ObjectId two) { - return one != null ? one : two; + try { + return walk.parseCommit(objectId); + } catch (IOException e) { + return null; + } } // Resolve the ObjectIds into RevObjects. Any missing object raises an @@ -2085,23 +2098,6 @@ public class UploadPack { return result; } - // Get commits from object ids. If the id is not a commit, ignore it. If the - // id doesn't exist, report the missing object in a exception. - private static List objectIdsToRevCommits(RevWalk walk, - Iterable objectIds) - throws MissingObjectException, IOException { - List result = new ArrayList<>(); - for (ObjectId objectId : objectIds) { - try { - result.add(walk.parseCommit(objectId)); - } catch (IncorrectObjectTypeException e) { - continue; - } - } - return result; - } - - private void addCommonBase(RevObject o) { if (!o.has(COMMON)) { o.add(COMMON);