Merge branch 'receive-pack-filter'

* receive-pack-filter:
  ReceivePack: Clarify the check reachable option
  ReceivePack: Micro-optimize object lookup when checking connectivity
  ReceivePack: Correct type of not provided object
  IndexPack: Tighten up new and base object bookkeeping
  ReceivePack: Remove need new,base object id properties
  ReceivePack: Discard IndexPack as soon as possible
  ReceivePack: fix ensureProvidedObjectsVisible on thin packs

Change-Id: I4ef2fcb931f3219872e0519abfcee220191d5133

3 files changed, 106 insertions, 92 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java
index 29f200c52d..6eeccea841 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/IndexPack.java
@@ -54,10 +54,7 @@ import java.io.RandomAccessFile;
 import java.security.MessageDigest;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collections;
-import java.util.HashSet;
 import java.util.List;
-import java.util.Set;
 import java.util.zip.CRC32;
 import java.util.zip.DataFormatException;
 import java.util.zip.Deflater;
@@ -173,7 +170,14 @@ public class IndexPack {
 
 	private PackedObjectInfo[] entries;
 
-	private Set<ObjectId> newObjectIds;
+	/**
+	 * Every object contained within the incoming pack.
+	 * <p>
+	 * This is a subset of {@link #entries}, as thin packs can add additional
+	 * objects to {@code entries} by copying already existing objects from the
+	 * repository onto the end of the thin pack to make it self-contained.
+	 */
+	private ObjectIdSubclassMap<ObjectId> newObjectIds;
 
 	private int deltaCount;
 
@@ -183,7 +187,14 @@ public class IndexPack {
 
 	private ObjectIdSubclassMap<DeltaChain> baseById;
 
-	private Set<ObjectId> baseIds;
+	/**
+	 * Objects referenced by their name from deltas, that aren't in this pack.
+	 * <p>
+	 * This is the set of objects that were copied onto the end of this pack to
+	 * make it complete. These objects were not transmitted by the remote peer,
+	 * but instead were assumed to already exist in the local repository.
+	 */
+	private ObjectIdSubclassMap<ObjectId> baseObjectIds;
 
 	private LongMap<UnresolvedDelta> baseByPos;
 
@@ -287,7 +298,7 @@ public class IndexPack {
 	 */
 	public void setNeedNewObjectIds(boolean b) {
 		if (b)
-			newObjectIds = new HashSet<ObjectId>();
+			newObjectIds = new ObjectIdSubclassMap<ObjectId>();
 		else
 			newObjectIds = null;
 	}
@@ -311,17 +322,17 @@ public class IndexPack {
 	}
 
 	/** @return the new objects that were sent by the user */
-	public Set<ObjectId> getNewObjectIds() {
-		return newObjectIds == null ?
-				Collections.<ObjectId>emptySet() : newObjectIds;
+	public ObjectIdSubclassMap<ObjectId> getNewObjectIds() {
+		if (newObjectIds != null)
+			return newObjectIds;
+		return new ObjectIdSubclassMap<ObjectId>();
 	}
 
-	/**
-	 *  @return the set of objects the incoming pack assumed for delta purposes
-	 */
-	public Set<ObjectId> getBaseObjectIds() {
-		return baseIds == null ?
-				Collections.<ObjectId>emptySet() : baseIds;
+	/** @return set of objects the incoming pack assumed for delta purposes */
+	public ObjectIdSubclassMap<ObjectId> getBaseObjectIds() {
+		if (baseObjectIds != null)
+			return baseObjectIds;
+		return new ObjectIdSubclassMap<ObjectId>();
 	}
 
 	/**
@@ -390,12 +401,6 @@ public class IndexPack {
 					if (packOut == null)
 						throw new IOException("need packOut");
 					resolveDeltas(progress);
-					if (needBaseObjectIds) {
-						baseIds = new HashSet<ObjectId>();
-						for (DeltaChain c : baseById) {
-							baseIds.add(c);
-						}
-					}
 					if (entryCount < objectCount) {
 						if (!fixThin) {
 							throw new IOException("pack has "
@@ -566,6 +571,9 @@ public class IndexPack {
 	private void fixThinPack(final ProgressMonitor progress) throws IOException {
 		growEntries();
 
+		if (needBaseObjectIds)
+			baseObjectIds = new ObjectIdSubclassMap<ObjectId>();
+
 		packDigest.reset();
 		originalEOF = packOut.length() - 20;
 		final Deflater def = new Deflater(Deflater.DEFAULT_COMPRESSION, false);
@@ -574,6 +582,8 @@ public class IndexPack {
 		for (final DeltaChain baseId : baseById) {
 			if (baseId.head == null)
 				continue;
+			if (needBaseObjectIds)
+				baseObjectIds.add(baseId);
 			final ObjectLoader ldr = repo.openObject(readCurs, baseId);
 			if (ldr == null) {
 				missing.add(baseId);
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 6ba326c00b..4e62d7427f 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java
@@ -82,6 +82,8 @@ 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.RevTag;
+import org.eclipse.jgit.revwalk.RevTree;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.ReceiveCommand.Result;
 import org.eclipse.jgit.transport.RefAdvertiser.PacketLineOutRefAdvertiser;
@@ -182,11 +184,7 @@ public class ReceivePack {
 	/** Lock around the received pack file, while updating refs. */
 	private PackLock packLock;
 
-	private boolean needNewObjectIds;
-
-	private boolean needBaseObjectIds;
-
-	private boolean ensureObjectsProvidedVisible;
+	private boolean checkReferencedIsReachable;
 
 	/**
 	 * Create a new pack receive for an open repository.
@@ -254,62 +252,36 @@ public class ReceivePack {
 	}
 
 	/**
-	 * Configure this receive pack instance to keep track of the objects assumed
-	 * for delta bases.
-	 * <p>
-	 * By default a receive pack doesn't save the objects that were used as
-	 * delta bases. Setting this flag to {@code true} will allow the caller to
-	 * use {@link #getBaseObjectIds()} to retrieve that list.
-	 *
-	 * @param b {@code true} to enable keeping track of delta bases.
-	 */
-	public void setNeedBaseObjectIds(boolean b) {
-		this.needBaseObjectIds = b;
-	}
-
-	/**
-	 *  @return the set of objects the incoming pack assumed for delta purposes
+	 * @return true if this instance will validate all referenced, but not
+	 *         supplied by the client, objects are reachable from another
+	 *         reference.
 	 */
-	public final Set<ObjectId> getBaseObjectIds() {
-		return ip.getBaseObjectIds();
+	public boolean isCheckReferencedObjectsAreReachable() {
+		return checkReferencedIsReachable;
 	}
 
 	/**
-	 * Configure this receive pack instance to keep track of new objects.
+	 * Validate all referenced but not supplied objects are reachable.
 	 * <p>
-	 * By default a receive pack doesn't save the new objects that were created
-	 * when it was instantiated. Setting this flag to {@code true} allows the
-	 * caller to use {@link #getNewObjectIds()} to retrieve that list.
-	 *
-	 * @param b {@code true} to enable keeping track of new objects.
-	 */
-	public void setNeedNewObjectIds(boolean b) {
-		this.needNewObjectIds = b;
-	}
-
-	/** @return the new objects that were sent by the user */
-	public final Set<ObjectId> getNewObjectIds() {
-		return ip.getNewObjectIds();
-	}
-
-	/**
-	 * Configure this receive pack instance to ensure that the provided
-	 * objects are visible to the user.
+	 * If enabled, this instance will verify that references to objects not
+	 * contained within the received pack are already reachable through at least
+	 * one other reference selected by the {@link #getRefFilter()} and displayed
+	 * as part of {@link #getAdvertisedRefs()}.
 	 * <p>
-	 * By default, a receive pack assumes that its user will only provide
-	 * references to objects that it can see. Setting this flag to {@code true}
-	 * will add an additional check that verifies that the objects that were
-	 * provided are reachable by a tree or a commit that the user can see.
+	 * This feature is useful when the application doesn't trust the client to
+	 * not provide a forged SHA-1 reference to an object, in an attempt to
+	 * access parts of the DAG that they aren't allowed to see and which have
+	 * been hidden from them via the configured {@link RefFilter}.
 	 * <p>
-	 * This option is useful when the code doesn't trust the client not to
-	 * provide a forged SHA-1 reference to an object in an attempt to access
-	 * parts of the DAG that they aren't allowed to see, via the configured
-	 * {@link RefFilter}.
+	 * Enabling this feature may imply at least some, if not all, of the same
+	 * functionality performed by {@link #setCheckReceivedObjects(boolean)}.
+	 * Applications are encouraged to enable both features, if desired.
 	 *
-	 * @param b {@code true} to enable the additional check.
+	 * @param b
+	 *            {@code true} to enable the additional check.
 	 */
-	public void setEnsureProvidedObjectsVisible(boolean b) {
-		this.ensureObjectsProvidedVisible = b;
+	public void setCheckReferencedObjectsAreReachable(boolean b) {
+		this.checkReferencedIsReachable = b;
 	}
 
 	/**
@@ -652,8 +624,9 @@ public class ReceivePack {
 			if (needPack()) {
 				try {
 					receivePack();
-					if (isCheckReceivedObjects())
+					if (needCheckConnectivity())
 						checkConnectivity();
+					ip = null;
 					unpackError = null;
 				} catch (IOException err) {
 					unpackError = err;
@@ -801,9 +774,8 @@ public class ReceivePack {
 
 		ip = IndexPack.create(db, rawIn);
 		ip.setFixThin(true);
-		ip.setNeedNewObjectIds(needNewObjectIds || ensureObjectsProvidedVisible);
-		ip.setNeedBaseObjectIds(needBaseObjectIds
-				|| ensureObjectsProvidedVisible);
+		ip.setNeedNewObjectIds(checkReferencedIsReachable);
+		ip.setNeedBaseObjectIds(checkReferencedIsReachable);
 		ip.setObjectChecking(isCheckReceivedObjects());
 		ip.index(NullProgressMonitor.INSTANCE);
 
@@ -816,7 +788,21 @@ public class ReceivePack {
 			timeoutIn.setTimeout(timeout * 1000);
 	}
 
+	private boolean needCheckConnectivity() {
+		return isCheckReceivedObjects()
+				|| isCheckReferencedObjectsAreReachable();
+	}
+
 	private void checkConnectivity() throws IOException {
+		ObjectIdSubclassMap<ObjectId> baseObjects = null;
+		ObjectIdSubclassMap<ObjectId> providedObjects = null;
+
+		if (checkReferencedIsReachable) {
+			baseObjects = ip.getBaseObjectIds();
+			providedObjects = ip.getNewObjectIds();
+		}
+		ip = null;
+
 		final ObjectWalk ow = new ObjectWalk(db);
 		for (final ReceiveCommand cmd : commands) {
 			if (cmd.getResult() != Result.NOT_ATTEMPTED)
@@ -825,34 +811,44 @@ public class ReceivePack {
 				continue;
 			ow.markStart(ow.parseAny(cmd.getNewId()));
 		}
-		for (final Ref ref : refs.values())
-			ow.markUninteresting(ow.parseAny(ref.getObjectId()));
+		for (final Ref ref : refs.values()) {
+			RevObject o = ow.parseAny(ref.getObjectId());
+			ow.markUninteresting(o);
+
+			if (checkReferencedIsReachable && !baseObjects.isEmpty()) {
+				while (o instanceof RevTag)
+					o = ((RevTag) o).getObject();
+				if (o instanceof RevCommit)
+					o = ((RevCommit) o).getTree();
+				if (o instanceof RevTree)
+					ow.markUninteresting(o);
+			}
+		}
 
-		ObjectIdSubclassMap<ObjectId> provided =
-			new ObjectIdSubclassMap<ObjectId>();
-		if (ensureObjectsProvidedVisible) {
-			for (ObjectId id : getBaseObjectIds()) {
+		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());
 			}
-			for (ObjectId id : getNewObjectIds()) {
-				provided.add(id);
-			}
 		}
 
 		RevCommit c;
 		while ((c = ow.next()) != null) {
-			if (ensureObjectsProvidedVisible && !provided.contains(c))
+			if (checkReferencedIsReachable && !providedObjects.contains(c))
 				throw new MissingObjectException(c, Constants.TYPE_COMMIT);
 		}
 
 		RevObject o;
 		while ((o = ow.nextObject()) != null) {
-			if (o instanceof RevBlob && !db.hasObject(o))
-				throw new MissingObjectException(o, Constants.TYPE_BLOB);
+			if (checkReferencedIsReachable) {
+				if (providedObjects.contains(o))
+					continue;
+				else
+					throw new MissingObjectException(o, o.getType());
+			}
 
-			if (ensureObjectsProvidedVisible && !provided.contains(o))
+			if (o instanceof RevBlob && !db.hasObject(o))
 				throw new MissingObjectException(o, Constants.TYPE_BLOB);
 		}
 	}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportLocal.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportLocal.java
index b9b9dbd001..22c436de3a 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportLocal.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportLocal.java
@@ -111,6 +111,14 @@ class TransportLocal extends Transport implements PackTransport {
 		remoteGitDir = d;
 	}
 
+	UploadPack createUploadPack(final Repository dst) {
+		return new UploadPack(dst);
+	}
+
+	ReceivePack createReceivePack(final Repository dst) {
+		return new ReceivePack(dst);
+	}
+
 	@Override
 	public FetchConnection openFetch() throws TransportException {
 		final String up = getOptionUploadPack();
@@ -197,7 +205,7 @@ class TransportLocal extends Transport implements PackTransport {
 			worker = new Thread("JGit-Upload-Pack") {
 				public void run() {
 					try {
-						final UploadPack rp = new UploadPack(dst);
+						final UploadPack rp = createUploadPack(dst);
 						rp.upload(out_r, in_w, null);
 					} catch (IOException err) {
 						// Client side of the pipes should report the problem.
@@ -329,7 +337,7 @@ class TransportLocal extends Transport implements PackTransport {
 			worker = new Thread("JGit-Receive-Pack") {
 				public void run() {
 					try {
-						final ReceivePack rp = new ReceivePack(dst);
+						final ReceivePack rp = createReceivePack(dst);
 						rp.receive(out_r, in_w, System.err);
 					} catch (IOException err) {
 						// Client side of the pipes should report the problem.