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 60aae90d4d15 ("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>

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackRefFilterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackRefFilterTest.java
index edced2fbb791f0ff7c9586d8b1848c1df97633ac..3483b9d165e86c6d7d020d9a4e3815ed2a51a26f 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackRefFilterTest.java
+++ b/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();

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackWriter.java
index 2fb342dad675a483d16ce404418732da73a5b948..20c4bb0f9735216dbd038858c716fcf51b435470 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackWriter.java
+++ b/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);
 

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java
index 36ee2fedf61fb5894af2095560e742bbb2fc16c3..4cc9ea58b474669e28ccea9b6f909573ed3399b7 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java
+++ b/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() {