Fix checkReferencedIsReachable to use correct base list

When checkReferencedIsReachable is set in ReceivePack we are trying
to prove that the push client is permitted to access an object that
it did not send to us, but that the received objects link to either
via a link inside of an object (e.g. commit parent pointer or tree
member) or by a delta base reference.

To do this check we are making a list of every potential delta base,
and then ensuring that every delta base used appears on this list.
If a delta base does not appear on this list, we abort with an error,
letting the client know we are missing a particular object.

Preventing spurious errors about missing delta base objects requires
us to use the exact same list of potential delta bases as the remote
push client used.  This means we must use TOPO ordering, and we
need to enable BOUNDARY sorting so that ObjectWalk will correctly
include any trees found during the enumeration back to the common
merge base between the interesting and uninteresting heads.

To ensure JGit's own push client matches this same potential delta
base list, we need to undo 60aae90d4d ("Disable topological
sorting in PackWriter") and switch back to using the conventional
TOPO ordering for commits in a pack file.  This ensures that our
own push client will use the same potential base object list as
checkReferencedIsReachable uses on the receiving side.

Change-Id: I14d0a326deb62a43f987b375cfe519711031e172
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackRefFilterTest.java

@@ -66,6 +66,7 @@ import org.eclipse.jgit.revwalk.RevBlob;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevTree;
 import org.eclipse.jgit.storage.file.ObjectDirectory;
+import org.eclipse.jgit.storage.pack.BinaryDelta;
 import org.eclipse.jgit.util.NB;
 import org.eclipse.jgit.util.TemporaryBuffer;
 
@@ -271,16 +272,24 @@ public class ReceivePackRefFilterTest extends LocalDiskRepositoryTestCase {
 	}
 
 	public void testUsingHiddenDeltaBaseFails() throws Exception {
+		byte[] delta = { 0x1, 0x1, 0x1, 'c' };
+		TestRepository<Repository> s = new TestRepository<Repository>(src);
+		RevCommit N = s.commit().parent(B).add("q",
+				s.blob(BinaryDelta.apply(dst.open(b).getCachedBytes(), delta)))
+				.create();
+
 		final TemporaryBuffer.Heap pack = new TemporaryBuffer.Heap(1024);
-		packHeader(pack, 1);
+		packHeader(pack, 3);
+		copy(pack, src.open(N));
+		copy(pack, src.open(s.parseBody(N).getTree()));
 		pack.write((Constants.OBJ_REF_DELTA) << 4 | 4);
 		b.copyRawTo(pack);
-		deflate(pack, new byte[] { 0x1, 0x1, 0x1, 'b' });
+		deflate(pack, delta);
 		digest(pack);
 
-		final TemporaryBuffer.Heap inBuf = new TemporaryBuffer.Heap(256);
+		final TemporaryBuffer.Heap inBuf = new TemporaryBuffer.Heap(1024);
 		final PacketLineOut inPckLine = new PacketLineOut(inBuf);
-		inPckLine.writeString(ObjectId.zeroId().name() + ' ' + P.name() + ' '
+		inPckLine.writeString(ObjectId.zeroId().name() + ' ' + N.name() + ' '
 				+ "refs/heads/s" + '\0'
 				+ BasePackPushConnection.CAPABILITY_REPORT_STATUS);
 		inPckLine.end();

org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackWriter.java

@@ -994,7 +994,7 @@ public class PackWriter {
 
 		final ObjectWalk walker = new ObjectWalk(reader);
 		walker.setRetainBody(false);
-		walker.sort(RevSort.COMMIT_TIME_DESC);
+		walker.sort(RevSort.TOPO);
 		if (thin && !not.isEmpty())
 			walker.sort(RevSort.BOUNDARY, true);
 

org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java

@@ -84,6 +84,7 @@ import org.eclipse.jgit.revwalk.RevBlob;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevFlag;
 import org.eclipse.jgit.revwalk.RevObject;
+import org.eclipse.jgit.revwalk.RevSort;
 import org.eclipse.jgit.revwalk.RevTree;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.storage.file.PackLock;
@@ -811,6 +812,11 @@ public class ReceivePack {
 
 		final ObjectWalk ow = new ObjectWalk(db);
 		ow.setRetainBody(false);
+		if (checkReferencedIsReachable) {
+			ow.sort(RevSort.TOPO);
+			if (!baseObjects.isEmpty())
+				ow.sort(RevSort.BOUNDARY, true);
+		}
 
 		for (final ReceiveCommand cmd : commands) {
 			if (cmd.getResult() != Result.NOT_ATTEMPTED)
@@ -832,22 +838,19 @@ public class ReceivePack {
 			}
 		}
 
-		if (checkReferencedIsReachable) {
-			for (ObjectId id : baseObjects) {
-				   RevObject b = ow.lookupAny(id, Constants.OBJ_BLOB);
-				   if (!b.has(RevFlag.UNINTERESTING))
-				     throw new MissingObjectException(b, b.getType());
-			}
-		}
-
 		RevCommit c;
 		while ((c = ow.next()) != null) {
-			if (checkReferencedIsReachable && !providedObjects.contains(c))
+			if (checkReferencedIsReachable //
+					&& !c.has(RevFlag.UNINTERESTING) //
+					&& !providedObjects.contains(c))
 				throw new MissingObjectException(c, Constants.TYPE_COMMIT);
 		}
 
 		RevObject o;
 		while ((o = ow.nextObject()) != null) {
+			if (o.has(RevFlag.UNINTERESTING))
+				continue;
+
 			if (checkReferencedIsReachable) {
 				if (providedObjects.contains(o))
 					continue;
@@ -858,6 +861,14 @@ public class ReceivePack {
 			if (o instanceof RevBlob && !db.hasObject(o))
 				throw new MissingObjectException(o, Constants.TYPE_BLOB);
 		}
+
+		if (checkReferencedIsReachable) {
+			for (ObjectId id : baseObjects) {
+				o = ow.parseAny(id);
+				if (!o.has(RevFlag.UNINTERESTING))
+					throw new MissingObjectException(o, o.getType());
+			}
+		}
 	}
 
 	private void validateCommands() {