From 3c807e01580680106b7db0b1aab60e3b5d010ea4 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Wed, 29 Jul 2020 19:18:51 -0700 Subject: [PATCH] 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 --- .../jgit/transport/ProtocolV0ParserTest.java | 8 ++ .../jgit/transport/ProtocolV2ParserTest.java | 18 +++- .../jgit/transport/UploadPackTest.java | 2 +- .../internal/storage/pack/PackWriter.java | 10 +- .../eclipse/jgit/transport/FilterSpec.java | 93 +++++++++++++++++-- 5 files changed, 115 insertions(+), 16 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV0ParserTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV0ParserTest.java index 965a2faf8..c0db83a82 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV0ParserTest.java +++ b/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()); } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV2ParserTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV2ParserTest.java index 038ce717b..837bdce91 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ProtocolV2ParserTest.java +++ b/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()); } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java index bb33eefce..e9b4af932 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java +++ b/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 remote2 = new TestRepository<>( server2)) { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java index 824c62ad9..2d574887b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java +++ b/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); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FilterSpec.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FilterSpec.java index 51e5a8f20..4353c1524 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FilterSpec.java +++ b/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();