From 29aa444760ea729dd10cdb0468055282a59096e5 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Tue, 29 Dec 2015 15:11:21 -0800 Subject: [PATCH] PackWriter: use lib.ObjectIdSet to avoid wrapper Hoist ObjectIdSet up to lib as part of the public API and add the interface to some common types like PackIndex and JGit custom ObjectId map types. This cleans up wrapper code in a number of places by allowing direct use of the types as an ObjectIdSet. Future commits can now rely on ObjectIdSet as a simple read-only type to check a set of objects from a number of storage options. Change-Id: Ib62b062421d475bd52abd6c84a73916ef36e084b --- .../internal/storage/file/PackWriterTest.java | 15 +---- .../storage/dfs/DfsGarbageCollector.java | 19 ++---- .../storage/dfs/DfsPackCompactor.java | 15 ++--- .../jgit/internal/storage/file/GC.java | 15 +---- .../jgit/internal/storage/file/PackIndex.java | 9 ++- .../internal/storage/pack/PackWriter.java | 13 +--- .../eclipse/jgit/lib/ObjectIdOwnerMap.java | 4 +- .../src/org/eclipse/jgit/lib/ObjectIdSet.java | 64 +++++++++++++++++++ .../eclipse/jgit/lib/ObjectIdSubclassMap.java | 3 +- 9 files changed, 95 insertions(+), 62 deletions(-) create mode 100644 org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdSet.java diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java index 80efe199f..3c44799b6 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java @@ -43,12 +43,12 @@ package org.eclipse.jgit.internal.storage.file; +import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; -import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; @@ -67,13 +67,12 @@ import java.util.Set; import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.internal.storage.file.PackIndex.MutableEntry; import org.eclipse.jgit.internal.storage.pack.PackWriter; -import org.eclipse.jgit.internal.storage.pack.PackWriter.ObjectIdSet; import org.eclipse.jgit.junit.JGitTestUtil; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.junit.TestRepository.BranchBuilder; -import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectIdSet; import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.revwalk.RevBlob; import org.eclipse.jgit.revwalk.RevCommit; @@ -528,7 +527,7 @@ public class PackWriterTest extends SampleDataRepositoryTestCase { RevCommit c2 = bb.commit().add("f", contentB).create(); testRepo.getRevWalk().parseHeaders(c2); PackIndex pf2 = writePack(repo, Collections.singleton(c2), - Collections.singleton(objectIdSet(pf1))); + Collections. singleton(pf1)); assertContent( pf2, Arrays.asList(c2.getId(), c2.getTree().getId(), @@ -733,12 +732,4 @@ public class PackWriterTest extends SampleDataRepositoryTestCase { assertEquals(objectsOrder[i++].toObjectId(), me.toObjectId()); } } - - private static ObjectIdSet objectIdSet(final PackIndex idx) { - return new ObjectIdSet() { - public boolean contains(AnyObjectId objectId) { - return idx.hasObject(objectId); - } - }; - } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsGarbageCollector.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsGarbageCollector.java index faf27e32b..bcc46c355 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsGarbageCollector.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsGarbageCollector.java @@ -67,7 +67,7 @@ import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.ObjectIdOwnerMap; +import org.eclipse.jgit.lib.ObjectIdSet; import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.revwalk.RevWalk; @@ -87,7 +87,7 @@ public class DfsGarbageCollector { private final List newPackStats; - private final List newPackObj; + private final List newPackObj; private DfsReader ctx; @@ -117,7 +117,7 @@ public class DfsGarbageCollector { objdb = repo.getObjectDatabase(); newPackDesc = new ArrayList(4); newPackStats = new ArrayList(4); - newPackObj = new ArrayList(4); + newPackObj = new ArrayList(4); packConfig = new PackConfig(repo); packConfig.setIndexVersion(2); @@ -288,7 +288,7 @@ public class DfsGarbageCollector { return; try (PackWriter pw = newPackWriter()) { - for (PackWriter.ObjectIdSet packedObjs : newPackObj) + for (ObjectIdSet packedObjs : newPackObj) pw.excludeObjects(packedObjs); pw.preparePack(pm, nonHeads, allHeads); if (0 < pw.getObjectCount()) @@ -328,7 +328,7 @@ public class DfsGarbageCollector { } private boolean anyPackHas(AnyObjectId id) { - for (PackWriter.ObjectIdSet packedObjs : newPackObj) + for (ObjectIdSet packedObjs : newPackObj) if (packedObjs.contains(id)) return true; return false; @@ -389,17 +389,10 @@ public class DfsGarbageCollector { } } - final ObjectIdOwnerMap packedObjs = pw - .getObjectSet(); - newPackObj.add(new PackWriter.ObjectIdSet() { - public boolean contains(AnyObjectId objectId) { - return packedObjs.contains(objectId); - } - }); - PackStatistics stats = pw.getStatistics(); pack.setPackStats(stats); newPackStats.add(stats); + newPackObj.add(pw.getObjectSet()); DfsBlockCache.getInstance().getOrCreate(pack, null); return pack; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackCompactor.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackCompactor.java index 7073763a7..11aef7fea 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackCompactor.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackCompactor.java @@ -62,6 +62,7 @@ import org.eclipse.jgit.internal.storage.pack.PackWriter; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectIdSet; import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.revwalk.RevFlag; import org.eclipse.jgit.revwalk.RevObject; @@ -91,7 +92,7 @@ public class DfsPackCompactor { private final List srcPacks; - private final List exclude; + private final List exclude; private final List newPacks; @@ -113,7 +114,7 @@ public class DfsPackCompactor { repo = repository; autoAddSize = 5 * 1024 * 1024; // 5 MiB srcPacks = new ArrayList(); - exclude = new ArrayList(4); + exclude = new ArrayList(4); newPacks = new ArrayList(1); newStats = new ArrayList(1); } @@ -164,7 +165,7 @@ public class DfsPackCompactor { * objects to not include. * @return {@code this}. */ - public DfsPackCompactor exclude(PackWriter.ObjectIdSet set) { + public DfsPackCompactor exclude(ObjectIdSet set) { exclude.add(set); return this; } @@ -183,11 +184,7 @@ public class DfsPackCompactor { try (DfsReader ctx = (DfsReader) repo.newObjectReader()) { idx = pack.getPackIndex(ctx); } - return exclude(new PackWriter.ObjectIdSet() { - public boolean contains(AnyObjectId id) { - return idx.hasObject(id); - } - }); + return exclude(idx); } /** @@ -343,7 +340,7 @@ public class DfsPackCompactor { RevObject obj = rw.lookupOrNull(id); if (obj != null && (obj.has(added) || obj.has(isBase))) continue; - for (PackWriter.ObjectIdSet e : exclude) + for (ObjectIdSet e : exclude) if (e.contains(id)) continue SCAN; want.add(new ObjectIdWithOffset(id, ent.getOffset())); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java index c0f56f448..7b3966de1 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/GC.java @@ -79,13 +79,12 @@ import org.eclipse.jgit.errors.NoWorkTreeException; import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.internal.storage.pack.PackExt; import org.eclipse.jgit.internal.storage.pack.PackWriter; -import org.eclipse.jgit.internal.storage.pack.PackWriter.ObjectIdSet; -import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.ConfigConstants; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.FileMode; import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectIdSet; import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Ref.Storage; @@ -552,7 +551,7 @@ public class GC { List excluded = new LinkedList(); for (final PackFile f : repo.getObjectDatabase().getPacks()) if (f.shouldBeKept()) - excluded.add(objectIdSet(f.getIndex())); + excluded.add(f.getIndex()); tagTargets.addAll(allHeads); nonHeads.addAll(indexObjects); @@ -564,7 +563,7 @@ public class GC { tagTargets, excluded); if (heads != null) { ret.add(heads); - excluded.add(0, objectIdSet(heads.getIndex())); + excluded.add(0, heads.getIndex()); } } if (!nonHeads.isEmpty()) { @@ -1000,12 +999,4 @@ public class GC { this.expire = expire; expireAgeMillis = -1; } - - private static ObjectIdSet objectIdSet(final PackIndex idx) { - return new ObjectIdSet() { - public boolean contains(AnyObjectId objectId) { - return idx.hasObject(objectId); - } - }; - } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackIndex.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackIndex.java index 0040aea71..f36bd4d70 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackIndex.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackIndex.java @@ -60,6 +60,7 @@ import org.eclipse.jgit.lib.AbbreviatedObjectId; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.MutableObjectId; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectIdSet; import org.eclipse.jgit.util.IO; import org.eclipse.jgit.util.NB; @@ -72,7 +73,8 @@ import org.eclipse.jgit.util.NB; * by ObjectId. *

*/ -public abstract class PackIndex implements Iterable { +public abstract class PackIndex + implements Iterable, ObjectIdSet { /** * Open an existing pack .idx file for reading. *

@@ -166,6 +168,11 @@ public abstract class PackIndex implements Iterable { return findOffset(id) != -1; } + @Override + public boolean contains(AnyObjectId id) { + return findOffset(id) != -1; + } + /** * Provide iterator that gives access to index entries. Note, that iterator * returns reference to mutable object, the same reference in each call - 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 f3f77c449..763f35c21 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 @@ -99,6 +99,7 @@ import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectIdOwnerMap; +import org.eclipse.jgit.lib.ObjectIdSet; import org.eclipse.jgit.lib.ObjectLoader; import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.ProgressMonitor; @@ -161,18 +162,6 @@ import org.eclipse.jgit.util.TemporaryBuffer; public class PackWriter implements AutoCloseable { private static final int PACK_VERSION_GENERATED = 2; - /** A collection of object ids. */ - public interface ObjectIdSet { - /** - * Returns true if the objectId is contained within the collection. - * - * @param objectId - * the objectId to find - * @return whether the collection contains the objectId. - */ - boolean contains(AnyObjectId objectId); - } - private static final Map, Boolean> instances = new ConcurrentHashMap, Boolean>(); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdOwnerMap.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdOwnerMap.java index 95b16d917..442261cbd 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdOwnerMap.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdOwnerMap.java @@ -67,8 +67,8 @@ import java.util.NoSuchElementException; * @param * type of subclass of ObjectId that will be stored in the map. */ -public class ObjectIdOwnerMap implements - Iterable { +public class ObjectIdOwnerMap + implements Iterable, ObjectIdSet { /** Size of the initial directory, will grow as necessary. */ private static final int INITIAL_DIRECTORY = 1024; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdSet.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdSet.java new file mode 100644 index 000000000..0b5848463 --- /dev/null +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdSet.java @@ -0,0 +1,64 @@ +/* + * Copyright (C) 2015, Google Inc. + * and other copyright owners as documented in the project's IP log. + * + * This program and the accompanying materials are made available + * under the terms of the Eclipse Distribution License v1.0 which + * accompanies this distribution, is reproduced below, and is + * available at http://www.eclipse.org/org/documents/edl-v10.php + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * + * - Neither the name of the Eclipse Foundation, Inc. nor the + * names of its contributors may be used to endorse or promote + * products derived from this software without specific prior + * written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +package org.eclipse.jgit.lib; + +/** + * Simple set of ObjectIds. + *

+ * Usually backed by a read-only data structure such as + * {@link org.eclipse.jgit.internal.storage.file.PackIndex}. Mutable types like + * {@link ObjectIdOwnerMap} also implement the interface by checking keys. + * + * @since 4.2 + */ +public interface ObjectIdSet { + /** + * Returns true if the objectId is contained within the collection. + * + * @param objectId + * the objectId to find + * @return whether the collection contains the objectId. + */ + boolean contains(AnyObjectId objectId); +} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdSubclassMap.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdSubclassMap.java index 48aa109e7..faed64bfe 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdSubclassMap.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdSubclassMap.java @@ -60,7 +60,8 @@ import java.util.NoSuchElementException; * @param * type of subclass of ObjectId that will be stored in the map. */ -public class ObjectIdSubclassMap implements Iterable { +public class ObjectIdSubclassMap + implements Iterable, ObjectIdSet { private static final int INITIAL_TABLE_SIZE = 2048; int size;