VisibilityCache: Split the visibility checks from the actual caching
VisibilityCache cache does two things: it calculates if something is
visible for the user AND caches the result.
Move the checks to the VisibilityChecker class. Besides better
separation of concerns, this allows us to plug for example an
instrumented checker.
Change-Id: I90701db964f454474d058ede834a24a25fac8f86
Signed-off-by: Ivan Frade <ifrade@google.com>
diff --git a/java/com/google/gitiles/VisibilityCache.java b/java/com/google/gitiles/VisibilityCache.java
index fbb3a45..fe1c07e 100644
--- a/java/com/google/gitiles/VisibilityCache.java
+++ b/java/com/google/gitiles/VisibilityCache.java
@@ -22,6 +22,7 @@
import static org.eclipse.jgit.lib.Constants.R_HEADS;
import static org.eclipse.jgit.lib.Constants.R_TAGS;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Throwables;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
@@ -34,17 +35,16 @@
import java.util.concurrent.TimeUnit;
import java.util.stream.Stream;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
-import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefDatabase;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
-import org.eclipse.jgit.revwalk.RevSort;
import org.eclipse.jgit.revwalk.RevWalk;
/** Cache of per-user object visibility. */
public class VisibilityCache {
+
private static class Key {
private final Object user;
private final String repositoryName;
@@ -83,7 +83,7 @@
}
private final Cache<Key, Boolean> cache;
- private final boolean topoSort;
+ private final VisibilityChecker checker;
public static CacheBuilder<Object, Object> defaultBuilder() {
return CacheBuilder.newBuilder().maximumSize(1 << 10).expireAfterWrite(30, TimeUnit.MINUTES);
@@ -94,14 +94,37 @@
}
public VisibilityCache(boolean topoSort, CacheBuilder<Object, Object> builder) {
+ this(new VisibilityChecker(topoSort), builder);
+ }
+
+ /**
+ * Use the constructors with a boolean parameter (e.g. {@link #VisibilityCache(boolean)}). The
+ * default visibility checker should cover all common use cases.
+ *
+ * <p>This constructor is useful to use a checker with additional logging or metrics collection,
+ * for example.
+ */
+ public VisibilityCache(VisibilityChecker checker) {
+ this(checker, defaultBuilder());
+ }
+
+ /**
+ * Use the constructors with a boolean parameter (e.g. {@link #VisibilityCache(boolean)}). The
+ * default visibility checker should cover all common use cases.
+ *
+ * <p>This constructor is useful to use a checker with additional logging or metrics collection,
+ * for example.
+ */
+ public VisibilityCache(VisibilityChecker checker, CacheBuilder<Object, Object> builder) {
this.cache = builder.build();
- this.topoSort = topoSort;
+ this.checker = checker;
}
public Cache<?, Boolean> getCache() {
return cache;
}
+ @VisibleForTesting
boolean isVisible(
final Repository repo,
final RevWalk walk,
@@ -126,8 +149,7 @@
}
}
- private boolean isVisible(
- Repository repo, RevWalk walk, ObjectId id, Collection<ObjectId> knownReachable)
+ boolean isVisible(Repository repo, RevWalk walk, ObjectId id, Collection<ObjectId> knownReachable)
throws IOException {
RevCommit commit;
try {
@@ -137,23 +159,18 @@
}
RefDatabase refDb = repo.getRefDatabase();
-
- // If any reference directly points at the requested object, permit display. Common for displays
- // of pending patch sets in Gerrit Code Review, or bookmarks to the commit a tag points at.
- for (Ref ref : repo.getRefDatabase().getRefs()) {
- ref = repo.getRefDatabase().peel(ref);
- if (id.equals(ref.getObjectId()) || id.equals(ref.getPeeledObjectId())) {
- return true;
- }
+ if (checker.isTipOfBranch(refDb, id)) {
+ return true;
}
// Check heads first under the assumption that most requests are for refs close to a head. Tags
// tend to be much further back in history and just clutter up the priority queue in the common
// case.
- return isReachableFrom(walk, commit, knownReachable)
- || isReachableFromRefs(walk, commit, refDb.getRefsByPrefix(R_HEADS).stream())
- || isReachableFromRefs(walk, commit, refDb.getRefsByPrefix(R_TAGS).stream())
- || isReachableFromRefs(walk, commit, refDb.getRefs().stream().filter(r -> otherRefs(r)));
+ return checker.isReachableFrom("knownReachable", walk, commit, knownReachable)
+ || isReachableFromRefs("heads", walk, commit, refDb.getRefsByPrefix(R_HEADS).stream())
+ || isReachableFromRefs("tags", walk, commit, refDb.getRefsByPrefix(R_TAGS).stream())
+ || isReachableFromRefs(
+ "other", walk, commit, refDb.getRefs().stream().filter(r -> otherRefs(r)));
}
private static boolean refStartsWith(Ref ref, String prefix) {
@@ -166,43 +183,14 @@
|| refStartsWith(r, "refs/changes/"));
}
- private boolean isReachableFromRefs(RevWalk walk, RevCommit commit, Stream<Ref> refs)
+ private boolean isReachableFromRefs(String desc, RevWalk walk, RevCommit commit, Stream<Ref> refs)
throws IOException {
return isReachableFrom(
- walk, commit, refs.map(r -> firstNonNull(r.getPeeledObjectId(), r.getObjectId())));
+ desc, walk, commit, refs.map(r -> firstNonNull(r.getPeeledObjectId(), r.getObjectId())));
}
- private boolean isReachableFrom(RevWalk walk, RevCommit commit, Stream<ObjectId> ids)
+ private boolean isReachableFrom(String desc, RevWalk walk, RevCommit commit, Stream<ObjectId> ids)
throws IOException {
- return isReachableFrom(walk, commit, ids.collect(toList()));
- }
-
- private boolean isReachableFrom(RevWalk walk, RevCommit commit, Collection<ObjectId> ids)
- throws IOException {
- if (ids.isEmpty()) {
- return false;
- }
- walk.reset();
- if (topoSort) {
- walk.sort(RevSort.TOPO);
- }
- walk.markStart(commit);
- for (ObjectId id : ids) {
- markUninteresting(walk, id);
- }
- // If the commit is reachable from any given tip, it will appear to be
- // uninteresting to the RevWalk and no output will be produced.
- return walk.next() == null;
- }
-
- private static void markUninteresting(RevWalk walk, ObjectId id) throws IOException {
- if (id == null) {
- return;
- }
- try {
- walk.markUninteresting(walk.parseCommit(id));
- } catch (IncorrectObjectTypeException | MissingObjectException e) {
- // Do nothing, doesn't affect reachability.
- }
+ return checker.isReachableFrom(desc, walk, commit, ids.collect(toList()));
}
}
diff --git a/java/com/google/gitiles/VisibilityChecker.java b/java/com/google/gitiles/VisibilityChecker.java
new file mode 100644
index 0000000..ad6c88b
--- /dev/null
+++ b/java/com/google/gitiles/VisibilityChecker.java
@@ -0,0 +1,139 @@
+/*
+ * Copyright (C) 2019, Google LLC.
+ * 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 com.google.gitiles;
+
+import java.io.IOException;
+import java.util.Collection;
+import org.eclipse.jgit.errors.IncorrectObjectTypeException;
+import org.eclipse.jgit.errors.MissingObjectException;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.RefDatabase;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevSort;
+import org.eclipse.jgit.revwalk.RevWalk;
+
+/**
+ * Checks for object visibility
+ *
+ * <p>Objects are visible if they are reachable from any of the references visible to the user.
+ */
+public class VisibilityChecker {
+
+ private boolean topoSort;
+
+ /**
+ * @param topoSort whether to use a more thorough reachability check by sorting in topological
+ * order
+ */
+ public VisibilityChecker(boolean topoSort) {
+ this.topoSort = topoSort;
+ }
+
+ /**
+ * Check if any of the refs in {@code refDb} points to the object {@code id}.
+ *
+ * @param refDb a reference database
+ * @param id object we are looking for
+ * @return true if the any of the references in the db points directly to the id
+ * @throws IOException the reference space cannot be accessed
+ */
+ protected boolean isTipOfBranch(RefDatabase refDb, ObjectId id) throws IOException {
+ // If any reference directly points at the requested object, permit display. Common for displays
+ // of pending patch sets in Gerrit Code Review, or bookmarks to the commit a tag points at.
+ for (Ref ref : refDb.getRefs()) {
+ ref = refDb.peel(ref);
+ if (id.equals(ref.getObjectId()) || id.equals(ref.getPeeledObjectId())) {
+ return true;
+ }
+ }
+
+ return false;
+ }
+
+ /**
+ * Check if {@code commit} is reachable starting from {@code starters}.
+ *
+ * @param description Description of the ids (e.g. "heads"). Mainly for tracing.
+ * @param walk The walk to use for the reachability check
+ * @param commit The starting commit. It *MUST* come from the walk in use
+ * @param starters visible commits. Anything reachable from these commits is visible. Missing ids
+ * or ids pointing to wrong kind of objects are ignored.
+ * @return true if we can get to {@code commit} from the {@code starters}
+ * @throws IOException a pack file or loose object could not be read
+ */
+ protected boolean isReachableFrom(
+ @SuppressWarnings("unused") String description,
+ RevWalk walk,
+ RevCommit commit,
+ Collection<ObjectId> starters)
+ throws IOException {
+ if (starters.isEmpty()) {
+ return false;
+ }
+
+ walk.reset();
+ if (topoSort) {
+ walk.sort(RevSort.TOPO);
+ }
+
+ walk.markStart(commit);
+ for (ObjectId id : starters) {
+ markUninteresting(walk, id);
+ }
+ // If the commit is reachable from any given tip, it will appear to be
+ // uninteresting to the RevWalk and no output will be produced.
+ return walk.next() == null;
+ }
+
+ private static void markUninteresting(RevWalk walk, ObjectId id) throws IOException {
+ if (id == null) {
+ return;
+ }
+ try {
+ walk.markUninteresting(walk.parseCommit(id));
+ } catch (IncorrectObjectTypeException | MissingObjectException e) {
+ // Do nothing, doesn't affect reachability.
+ }
+ }
+}
diff --git a/javatests/com/google/gitiles/VisibilityCheckerTest.java b/javatests/com/google/gitiles/VisibilityCheckerTest.java
new file mode 100644
index 0000000..857ecca
--- /dev/null
+++ b/javatests/com/google/gitiles/VisibilityCheckerTest.java
@@ -0,0 +1,119 @@
+/*
+ * Copyright (C) 2019, Google LLC.
+ * 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 com.google.gitiles;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.List;
+import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public class VisibilityCheckerTest {
+ private InMemoryRepository repo;
+
+ private RevCommit baseCommit;
+ private RevCommit commit1;
+ private RevCommit commit2;
+ private RevCommit commitA;
+ private RevCommit commitB;
+ private RevCommit commitC;
+
+ private VisibilityChecker visibilityChecker;
+ private RevWalk walk;
+
+ @Before
+ public void setUp() throws Exception {
+ repo = new InMemoryRepository(new DfsRepositoryDescription());
+ TestRepository<InMemoryRepository> git = new TestRepository<>(repo);
+ baseCommit = git.commit().message("baseCommit").create();
+ commit1 = git.commit().parent(baseCommit).message("commit1").create();
+ commit2 = git.commit().parent(commit1).message("commit2").create();
+
+ commitA = git.commit().parent(baseCommit).message("commitA").create();
+ commitB = git.commit().parent(commitA).message("commitB").create();
+ commitC = git.commit().parent(commitB).message("commitC").create();
+
+ git.update("master", commit2);
+ git.update("refs/tags/v0.1", commitA);
+
+ visibilityChecker = new VisibilityChecker(true);
+ walk = new RevWalk(repo);
+ walk.setRetainBody(false);
+ }
+
+ @Test
+ public void isTip() throws IOException {
+ assertTrue(visibilityChecker.isTipOfBranch(repo.getRefDatabase(), commit2.getId()));
+ }
+
+ @Test
+ public void isNotTip() throws IOException {
+ assertFalse(visibilityChecker.isTipOfBranch(repo.getRefDatabase(), commit1.getId()));
+ }
+
+ @Test
+ public void reachableFromRef() throws IOException {
+ List<ObjectId> starters = Arrays.asList(commitC.getId());
+ assertTrue(
+ visibilityChecker.isReachableFrom("test", walk, walk.parseCommit(commitB), starters));
+ }
+
+ @Test
+ public void unreachableFromRef() throws IOException {
+ List<ObjectId> starters = Arrays.asList(commit2.getId(), commitA.getId());
+ assertFalse(
+ visibilityChecker.isReachableFrom("test", walk, walk.parseCommit(commitC), starters));
+ }
+}
