Do not send empty blob in response to blob:none filter

If I create a repository containing an empty file and clone it
with

	git clone --no-checkout --filter=blob:none \
		https://url/of/repository

then I would expect no blobs to be transferred over the wire.  Alas,
JGit rewrites filter=blob:none to filter=blob:limit=0, so if the
repository contains an empty file then the empty blob gets
transferred.

Fix it by teaching JGit about filters based on object type to
complement the existing filters based on object size.  This prepares
us for other future filters such as object:none.

In particular, this means we do not need to look up the size of the
filtered blobs, which should speed up clones.  Noticed by Anna
Pologova and Terry Parker.

Change-Id: Id4b234921a190c108d8be2c87f54dcbfa811602a
Signed-off-by: Jonathan Nieder <jrn@google.com>

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

@@ -9,6 +9,10 @@
  */
 package org.eclipse.jgit.transport;
 
+import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
+import static org.eclipse.jgit.lib.Constants.OBJ_COMMIT;
+import static org.eclipse.jgit.lib.Constants.OBJ_TAG;
+import static org.eclipse.jgit.lib.Constants.OBJ_TREE;
 import static org.eclipse.jgit.transport.ObjectIdMatcher.hasOnlyObjectIds;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertEquals;
@@ -160,6 +164,10 @@ public class ProtocolV0ParserTest {
 		assertThat(request.getWantIds(),
 				hasOnlyObjectIds("4624442d68ee402a94364191085b77137618633e",
 						"f900c8326a43303685c46b279b9f70411bff1a4b"));
+		assertTrue(request.getFilterSpec().allowsType(OBJ_BLOB));
+		assertTrue(request.getFilterSpec().allowsType(OBJ_TREE));
+		assertTrue(request.getFilterSpec().allowsType(OBJ_COMMIT));
+		assertTrue(request.getFilterSpec().allowsType(OBJ_TAG));
 		assertEquals(13000, request.getFilterSpec().getBlobLimit());
 		assertEquals(-1, request.getFilterSpec().getTreeDepthLimit());
 	}

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

@@ -9,6 +9,10 @@
  */
 package org.eclipse.jgit.transport;
 
+import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
+import static org.eclipse.jgit.lib.Constants.OBJ_COMMIT;
+import static org.eclipse.jgit.lib.Constants.OBJ_TAG;
+import static org.eclipse.jgit.lib.Constants.OBJ_TREE;
 import static org.eclipse.jgit.transport.ObjectIdMatcher.hasOnlyObjectIds;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.hasItems;
@@ -195,7 +199,11 @@ public class ProtocolV2ParserTest {
 		ProtocolV2Parser parser = new ProtocolV2Parser(
 				ConfigBuilder.start().allowFilter().done());
 		FetchV2Request request = parser.parseFetchRequest(pckIn);
-		assertEquals(0, request.getFilterSpec().getBlobLimit());
+		assertFalse(request.getFilterSpec().allowsType(OBJ_BLOB));
+		assertTrue(request.getFilterSpec().allowsType(OBJ_TREE));
+		assertTrue(request.getFilterSpec().allowsType(OBJ_COMMIT));
+		assertTrue(request.getFilterSpec().allowsType(OBJ_TAG));
+		assertEquals(-1, request.getFilterSpec().getBlobLimit());
 		assertEquals(-1, request.getFilterSpec().getTreeDepthLimit());
 	}
 
@@ -207,6 +215,10 @@ public class ProtocolV2ParserTest {
 		ProtocolV2Parser parser = new ProtocolV2Parser(
 				ConfigBuilder.start().allowFilter().done());
 		FetchV2Request request = parser.parseFetchRequest(pckIn);
+		assertTrue(request.getFilterSpec().allowsType(OBJ_BLOB));
+		assertTrue(request.getFilterSpec().allowsType(OBJ_TREE));
+		assertTrue(request.getFilterSpec().allowsType(OBJ_COMMIT));
+		assertTrue(request.getFilterSpec().allowsType(OBJ_TAG));
 		assertEquals(15, request.getFilterSpec().getBlobLimit());
 		assertEquals(-1, request.getFilterSpec().getTreeDepthLimit());
 	}
@@ -219,6 +231,10 @@ public class ProtocolV2ParserTest {
 		ProtocolV2Parser parser = new ProtocolV2Parser(
 				ConfigBuilder.start().allowFilter().done());
 		FetchV2Request request = parser.parseFetchRequest(pckIn);
+		assertTrue(request.getFilterSpec().allowsType(OBJ_BLOB));
+		assertTrue(request.getFilterSpec().allowsType(OBJ_TREE));
+		assertTrue(request.getFilterSpec().allowsType(OBJ_COMMIT));
+		assertTrue(request.getFilterSpec().allowsType(OBJ_TAG));
 		assertEquals(-1, request.getFilterSpec().getBlobLimit());
 		assertEquals(3, request.getFilterSpec().getTreeDepthLimit());
 	}

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

@@ -127,7 +127,7 @@ public class UploadPackTest {
 	}
 
 	@Test
-	public void testFetchWithBlobNoneFilter() throws Exception {
+	public void testFetchWithBlobZeroFilter() throws Exception {
 		InMemoryRepository server2 = newRepo("server2");
 		try (TestRepository<InMemoryRepository> remote2 = new TestRepository<>(
 				server2)) {

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

@@ -2182,10 +2182,12 @@ public class PackWriter implements AutoCloseable {
 
 		// Check if this object needs to be rejected, doing the cheaper
 		// checks first.
-		boolean reject = filterSpec.getBlobLimit() >= 0 &&
-			type == OBJ_BLOB &&
-			!want.contains(src) &&
-			reader.getObjectSize(src, OBJ_BLOB) > filterSpec.getBlobLimit();
+		boolean reject =
+			(!filterSpec.allowsType(type) && !want.contains(src)) ||
+			(filterSpec.getBlobLimit() >= 0 &&
+				type == OBJ_BLOB &&
+				!want.contains(src) &&
+				reader.getObjectSize(src, OBJ_BLOB) > filterSpec.getBlobLimit());
 		if (!reject) {
 			addObject(src, type, pathHashCode);
 		}

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

@@ -10,8 +10,14 @@
 
 package org.eclipse.jgit.transport;
 
+import static java.util.Objects.requireNonNull;
+import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
+import static org.eclipse.jgit.lib.Constants.OBJ_COMMIT;
+import static org.eclipse.jgit.lib.Constants.OBJ_TAG;
+import static org.eclipse.jgit.lib.Constants.OBJ_TREE;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_FILTER;
 
+import java.math.BigInteger;
 import java.text.MessageFormat;
 
 import org.eclipse.jgit.annotations.Nullable;
@@ -26,11 +32,54 @@ import org.eclipse.jgit.internal.JGitText;
  */
 public final class FilterSpec {
 
+	/** Immutable bit-set representation of a set of Git object types. */
+	static class ObjectTypes {
+		static ObjectTypes ALL = allow(OBJ_BLOB, OBJ_TREE, OBJ_COMMIT, OBJ_TAG);
+
+		private final BigInteger val;
+
+		private ObjectTypes(BigInteger val) {
+			this.val = requireNonNull(val);
+		}
+
+		static ObjectTypes allow(int... types) {
+			BigInteger bits = BigInteger.valueOf(0);
+			for (int type : types) {
+				bits = bits.setBit(type);
+			}
+			return new ObjectTypes(bits);
+		}
+
+		boolean contains(int type) {
+			return val.testBit(type);
+		}
+
+		/** {@inheritDoc} */
+		@Override
+		public boolean equals(Object obj) {
+			if (!(obj instanceof ObjectTypes)) {
+				return false;
+			}
+
+			ObjectTypes other = (ObjectTypes) obj;
+			return other.val.equals(val);
+		}
+
+		/** {@inheritDoc} */
+		@Override
+		public int hashCode() {
+			return val.hashCode();
+		}
+	}
+
+	private final ObjectTypes types;
+
 	private final long blobLimit;
 
 	private final long treeDepthLimit;
 
-	private FilterSpec(long blobLimit, long treeDepthLimit) {
+	private FilterSpec(ObjectTypes types, long blobLimit, long treeDepthLimit) {
+		this.types = requireNonNull(types);
 		this.blobLimit = blobLimit;
 		this.treeDepthLimit = treeDepthLimit;
 	}
@@ -55,7 +104,8 @@ public final class FilterSpec {
 	public static FilterSpec fromFilterLine(String filterLine)
 			throws PackProtocolException {
 		if (filterLine.equals("blob:none")) { //$NON-NLS-1$
-			return FilterSpec.withBlobLimit(0);
+			return FilterSpec.withObjectTypes(
+					ObjectTypes.allow(OBJ_TREE, OBJ_COMMIT, OBJ_TAG));
 		} else if (filterLine.startsWith("blob:limit=")) { //$NON-NLS-1$
 			long blobLimit = -1;
 			try {
@@ -87,9 +137,19 @@ public final class FilterSpec {
 				MessageFormat.format(JGitText.get().invalidFilter, filterLine));
 	}
 
+	/**
+	 * @param types
+	 *            set of permitted object types, for use in "blob:none" and
+	 *            "object:none" filters
+	 * @return a filter spec which restricts to objects of the specified types
+	 */
+	static FilterSpec withObjectTypes(ObjectTypes types) {
+		return new FilterSpec(types, -1, -1);
+	}
+
 	/**
 	 * @param blobLimit
-	 *            the blob limit in a "blob:[limit]" or "blob:none" filter line
+	 *            the blob limit in a "blob:[limit]" filter line
 	 * @return a filter spec which filters blobs above a certain size
 	 */
 	static FilterSpec withBlobLimit(long blobLimit) {
@@ -97,7 +157,7 @@ public final class FilterSpec {
 			throw new IllegalArgumentException(
 					"blobLimit cannot be negative: " + blobLimit); //$NON-NLS-1$
 		}
-		return new FilterSpec(blobLimit, -1);
+		return new FilterSpec(ObjectTypes.ALL, blobLimit, -1);
 	}
 
 	/**
@@ -111,13 +171,25 @@ public final class FilterSpec {
 			throw new IllegalArgumentException(
 					"treeDepthLimit cannot be negative: " + treeDepthLimit); //$NON-NLS-1$
 		}
-		return new FilterSpec(-1, treeDepthLimit);
+		return new FilterSpec(ObjectTypes.ALL, -1, treeDepthLimit);
 	}
 
 	/**
 	 * A placeholder that indicates no filtering.
 	 */
-	public static final FilterSpec NO_FILTER = new FilterSpec(-1, -1);
+	public static final FilterSpec NO_FILTER = new FilterSpec(ObjectTypes.ALL, -1, -1);
+
+	/**
+	 * @param type
+	 *            a Git object type, such as
+	 *            {@link org.eclipse.jgit.lib.Constants#OBJ_BLOB}
+	 * @return whether this filter allows objects of the specified type
+	 *
+	 * @since 5.9
+	 */
+	public boolean allowsType(int type) {
+		return types.contains(type);
+	}
 
 	/**
 	 * @return -1 if this filter does not filter blobs based on size, or a
@@ -140,7 +212,7 @@ public final class FilterSpec {
 	 * @return true if this filter doesn't filter out anything
 	 */
 	public boolean isNoOp() {
-		return blobLimit == -1 && treeDepthLimit == -1;
+		return types.equals(ObjectTypes.ALL) && blobLimit == -1 && treeDepthLimit == -1;
 	}
 
 	/**
@@ -150,11 +222,12 @@ public final class FilterSpec {
 	public String filterLine() {
 		if (isNoOp()) {
 			return null;
-		} else if (blobLimit == 0 && treeDepthLimit == -1) {
+		} else if (types.equals(ObjectTypes.allow(OBJ_TREE, OBJ_COMMIT, OBJ_TAG)) &&
+					blobLimit == -1 && treeDepthLimit == -1) {
 			return OPTION_FILTER + " blob:none"; //$NON-NLS-1$
-		} else if (blobLimit > 0 && treeDepthLimit == -1) {
+		} else if (types.equals(ObjectTypes.ALL) && blobLimit >= 0 && treeDepthLimit == -1) {
 			return OPTION_FILTER + " blob:limit=" + blobLimit; //$NON-NLS-1$
-		} else if (blobLimit == -1 && treeDepthLimit >= 0) {
+		} else if (types.equals(ObjectTypes.ALL) && blobLimit == -1 && treeDepthLimit >= 0) {
 			return OPTION_FILTER + " tree:" + treeDepthLimit; //$NON-NLS-1$
 		} else {
 			throw new IllegalStateException();