TreeWalkConnectivityChecker: Support non-commit objects in refs

Running in shadow mode in production turned up the case of the reference
database containing references to annotated tags and other non-commit
objects. Fixed that and added tests.

Change-Id: I7f1029e2512c96500658d9556d7c7d7c2753c817

org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/connectivity/TreeWalkConnectivityCheckerTest.java

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/connectivity/TreeWalkConnectivityCheckerTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/connectivity/TreeWalkConnectivityCheckerTest.java
index 284c56e..371f876 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/connectivity/TreeWalkConnectivityCheckerTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/connectivity/TreeWalkConnectivityCheckerTest.java
@@ -40,6 +40,7 @@
 import org.eclipse.jgit.lib.TreeFormatter;
 import org.eclipse.jgit.revwalk.RevBlob;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevTag;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.ConnectivityChecker.ConnectivityCheckInfo;
 import org.eclipse.jgit.transport.PackParser;
@@ -517,10 +518,74 @@ public void testSuccessFallbackToFullRefDatabase() throws Exception {
 		info.setCommands(Collections.singletonList(new ReceiveCommand(
 				ObjectId.zeroId(), newCommit.getId(), "refs/heads/master")));
 		mockNewPackObjects(newCommit, newCommit.getTree());
+
 		runCheckAndAssertCount(5);
 	}
 
 	@Test
+	public void testCheckReachabilityWithBlobInHaves() throws Exception {
+		RevCommit base = tr.commit().create();
+		haves.add(base.getId());
+
+		RevBlob blob = tr.blob("blob content");
+		haves.add(blob.getId());
+
+		RevCommit newCommit = tr.commit().parent(base).create();
+
+		setupSingleReceiveCommand(ObjectId.zeroId(), newCommit.getId());
+		mockNewPackObjects(newCommit, newCommit.getTree());
+
+		runCheckAndAssertCount(4);
+	}
+
+	@Test
+	public void testCheckReachabilityWithAnnotatedTagInHaves()
+			throws Exception {
+		RevCommit base = tr.commit().create();
+
+		RevTag tag = tr.tag("my-tag", base);
+		haves.add(tag.getId());
+
+		RevCommit newCommit = tr.commit().parent(base).create();
+
+		setupSingleReceiveCommand(ObjectId.zeroId(), newCommit.getId());
+		mockNewPackObjects(newCommit, newCommit.getTree());
+
+		runCheckAndAssertCount(5);
+	}
+
+	@Test
+	public void testCheckReachabilityWithBlobInRefs() throws Exception {
+		RevCommit base = tr.commit().create();
+		haves.add(base.getId());
+
+		RevBlob blob = tr.blob("blob content");
+		tr.update("refs/tags/my-blob", blob);
+
+		RevCommit newCommit = tr.commit().parent(base).create();
+
+		setupSingleReceiveCommand(ObjectId.zeroId(), newCommit.getId());
+		mockNewPackObjects(newCommit, newCommit.getTree());
+
+		runCheckAndAssertCount(4);
+	}
+
+	@Test
+	public void testCheckReachabilityWithAnnotatedTagInRefs() throws Exception {
+		RevCommit base = tr.commit().create();
+
+		RevTag tag = tr.tag("my-tag", base);
+		tr.update("refs/tags/my-tag", tag);
+
+		RevCommit newCommit = tr.commit().parent(base).create();
+
+		setupSingleReceiveCommand(ObjectId.zeroId(), newCommit.getId());
+		mockNewPackObjects(newCommit, newCommit.getTree());
+
+		runCheckAndAssertCount(4);
+	}
+
+	@Test
 	public void testSuccessWithDeletion() throws Exception {
 		RevBlob blob = tr.blob("hello");
 		RevCommit base = tr.commit().add("foo", blob).create();
@@ -598,6 +663,98 @@ public void testThinPackSuccess() throws Exception {
 	}
 
 	@Test
+	public void testThinPackWithBlobInHaves() throws Exception {
+		RevCommit base = tr.commit().create();
+		haves.add(base.getId());
+
+		RevBlob baseBlob = tr.blob("base blob content");
+
+		RevBlob unrelatedBlob = tr.blob("unrelated blob content");
+		haves.add(unrelatedBlob.getId());
+
+		RevCommit newCommit = tr.commit().parent(base).create();
+
+		setupSingleReceiveCommand(base.getId(), newCommit.getId());
+		mockNewPackObjects(newCommit, newCommit.getTree());
+
+		ObjectIdSubclassMap<ObjectId> baseObjectIds = new ObjectIdSubclassMap<>();
+		baseObjectIds.add(baseBlob.getId());
+		when(parser.getBaseObjectIds()).thenReturn(baseObjectIds);
+		info.setCheckObjects(true);
+
+		runCheckAndAssertCount(4);
+	}
+
+	@Test
+	public void testThinPackWithSignedTagInHaves() throws Exception {
+		RevCommit base = tr.commit().create();
+		haves.add(base.getId());
+
+		RevBlob baseBlob = tr.blob("base blob content");
+
+		RevTag tag = tr.tag("my-tag", base);
+		haves.add(tag.getId());
+
+		RevCommit newCommit = tr.commit().parent(base).create();
+
+		setupSingleReceiveCommand(base.getId(), newCommit.getId());
+		mockNewPackObjects(newCommit, newCommit.getTree());
+
+		ObjectIdSubclassMap<ObjectId> baseObjectIds = new ObjectIdSubclassMap<>();
+		baseObjectIds.add(baseBlob.getId());
+		when(parser.getBaseObjectIds()).thenReturn(baseObjectIds);
+		info.setCheckObjects(true);
+
+		runCheckAndAssertCount(4);
+	}
+
+	@Test
+	public void testThinPackWithBlobInRefs() throws Exception {
+		RevCommit base = tr.commit().create();
+		haves.add(base.getId());
+
+		RevBlob baseBlob = tr.blob("base blob content");
+
+		RevBlob unrelatedBlob = tr.blob("unrelated blob content");
+		tr.update("refs/tags/my-blob", unrelatedBlob);
+
+		RevCommit newCommit = tr.commit().parent(base).create();
+
+		setupSingleReceiveCommand(base.getId(), newCommit.getId());
+		mockNewPackObjects(newCommit, newCommit.getTree());
+
+		ObjectIdSubclassMap<ObjectId> baseObjectIds = new ObjectIdSubclassMap<>();
+		baseObjectIds.add(baseBlob.getId());
+		when(parser.getBaseObjectIds()).thenReturn(baseObjectIds);
+		info.setCheckObjects(true);
+
+		runCheckAndAssertCount(4);
+	}
+
+	@Test
+	public void testThinPackWithAnnotatedTagInRefs() throws Exception {
+		RevCommit base = tr.commit().create();
+		haves.add(base.getId());
+
+		RevBlob baseBlob = tr.blob("base blob content");
+
+		RevTag tag = tr.tag("my-tag", base);
+		tr.update("refs/tags/my-tag", tag);
+
+		RevCommit newCommit = tr.commit().parent(base).create();
+
+		setupSingleReceiveCommand(base.getId(), newCommit.getId());
+		mockNewPackObjects(newCommit, newCommit.getTree());
+
+		ObjectIdSubclassMap<ObjectId> baseObjectIds = new ObjectIdSubclassMap<>();
+		baseObjectIds.add(baseBlob.getId());
+		when(parser.getBaseObjectIds()).thenReturn(baseObjectIds);
+		info.setCheckObjects(true);
+
+		runCheckAndAssertCount(4);
+	}
+
+	@Test
 	public void testMultipleCommandsDifferentParents() throws Exception {
 		RevCommit base1 = tr.commit().create();
 		RevCommit base2 = tr.commit().create();

org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/connectivity/TreeWalkConnectivityChecker.java

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/connectivity/TreeWalkConnectivityChecker.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/connectivity/TreeWalkConnectivityChecker.java
index 8c667df..cfe6bcf 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/connectivity/TreeWalkConnectivityChecker.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/connectivity/TreeWalkConnectivityChecker.java
@@ -21,6 +21,7 @@
 import java.util.stream.Stream;
 import java.util.stream.StreamSupport;
 
+import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.FileMode;
@@ -215,7 +216,7 @@ private void checkReachability(RevWalk rw, Repository repo,
 			ReachabilityChecker checker = rw.getObjectReader()
 					.createReachabilityChecker(rw);
 			Stream<RevCommit> starterCommits = advertisedHaves.stream()
-					.map(id -> rw.lookupCommit(id));
+					.map(id -> parseCommitOrNull(rw, id)).filter(Objects::nonNull);
 
 			Optional<RevCommit> unreachable = checker.areAllReachable(
 					nonAdvertisedParentCommitsOutsidePack, starterCommits);
@@ -224,7 +225,8 @@ private void checkReachability(RevWalk rw, Repository repo,
 				// Fallback to check against full ref database
 				Stream<RevCommit> allRefCommits = repo.getRefDatabase()
 						.getRefs().stream()
-						.map(ref -> rw.lookupCommit(ref.getObjectId()));
+						.map(ref -> parseCommitOrNull(rw, ref.getObjectId()))
+						.filter(Objects::nonNull);
 				unreachable = checker.areAllReachable(
 						nonAdvertisedParentCommitsOutsidePack, allRefCommits);
 				if (unreachable.isPresent()) {
@@ -276,7 +278,8 @@ private void checkThinPackBases(RevWalk rw,
 				// Fallback to check against full ref database
 				Stream<RevObject> allRefCommits = connectivityCheckInfo
 						.getRepository().getRefDatabase().getRefs().stream()
-						.map(ref -> rw.lookupCommit(ref.getObjectId()));
+						.map(ref -> parseAnyUnchecked(ow, ref.getObjectId()))
+						.filter(Objects::nonNull);
 				unreachable = checker.areAllReachable(targetObjs,
 						allRefCommits);
 				if (unreachable.isPresent()) {
@@ -401,6 +404,27 @@ private static RevObject parseAnyUnchecked(RevWalk rw, ObjectId id) {
 	}
 
 	/**
+	 * Parses an object as a commit, peeling tags if necessary. Returns null if
+	 * the object is not a commit or cannot be peeled to a commit. Throws
+	 * UncheckedIOException on other I/O failures. For use with streams.
+	 *
+	 * @param rw
+	 *          the RevWalk to use
+	 * @param id
+	 *          the object ID
+	 * @return the parsed commit, or null
+	 */
+	private static RevCommit parseCommitOrNull(RevWalk rw, ObjectId id) {
+		try {
+			return rw.parseCommit(id);
+		} catch (IncorrectObjectTypeException e) {
+			return null;
+		} catch (IOException e) {
+			throw new UncheckedIOException(e);
+		}
+	}
+
+	/**
 	 * Checks if the new object ID matches any parent tree's object ID at the
 	 * current path.
 	 *