Merge changes from topics "jgit-reachability", "reachability-2" * changes: VisibilityChecker: Remove topoSort argument VisibilityCache: Use jgit's ReachabilityChecker
diff --git a/java/com/google/gitiles/GitilesFilter.java b/java/com/google/gitiles/GitilesFilter.java index 254fe22..79dcc4c 100644 --- a/java/com/google/gitiles/GitilesFilter.java +++ b/java/com/google/gitiles/GitilesFilter.java
@@ -383,9 +383,9 @@ if (visibilityCache == null) { if (config.getSubsections("cache").contains("visibility")) { visibilityCache = - new VisibilityCache(false, ConfigUtil.getCacheBuilder(config, "visibility")); + new VisibilityCache(ConfigUtil.getCacheBuilder(config, "visibility")); } else { - visibilityCache = new VisibilityCache(false); + visibilityCache = new VisibilityCache(); } } }
diff --git a/java/com/google/gitiles/VisibilityCache.java b/java/com/google/gitiles/VisibilityCache.java index fe1c07e..1c6b785 100644 --- a/java/com/google/gitiles/VisibilityCache.java +++ b/java/com/google/gitiles/VisibilityCache.java
@@ -89,16 +89,16 @@ return CacheBuilder.newBuilder().maximumSize(1 << 10).expireAfterWrite(30, TimeUnit.MINUTES); } - public VisibilityCache(boolean topoSort) { - this(topoSort, defaultBuilder()); + public VisibilityCache() { + this(new VisibilityChecker(), defaultBuilder()); } - public VisibilityCache(boolean topoSort, CacheBuilder<Object, Object> builder) { - this(new VisibilityChecker(topoSort), builder); + public VisibilityCache(CacheBuilder<Object, Object> builder) { + this(new VisibilityChecker(), builder); } /** - * Use the constructors with a boolean parameter (e.g. {@link #VisibilityCache(boolean)}). The + * Use the constructors with a boolean parameter (e.g. {@link #VisibilityCache()}). 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, @@ -109,7 +109,7 @@ } /** - * Use the constructors with a boolean parameter (e.g. {@link #VisibilityCache(boolean)}). The + * Use the constructors with a boolean parameter (e.g. {@link #VisibilityCache()}). 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,
diff --git a/java/com/google/gitiles/VisibilityChecker.java b/java/com/google/gitiles/VisibilityChecker.java index ccd5cf1..899d1f1 100644 --- a/java/com/google/gitiles/VisibilityChecker.java +++ b/java/com/google/gitiles/VisibilityChecker.java
@@ -42,6 +42,7 @@ */ package com.google.gitiles; +import com.google.common.collect.ImmutableList; import java.io.IOException; import java.util.Collection; import org.eclipse.jgit.errors.IncorrectObjectTypeException; @@ -49,7 +50,6 @@ import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.RefDatabase; import org.eclipse.jgit.revwalk.RevCommit; -import org.eclipse.jgit.revwalk.RevSort; import org.eclipse.jgit.revwalk.RevWalk; /** @@ -59,16 +59,6 @@ */ public class VisibilityChecker { - private final 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}. * @@ -90,7 +80,7 @@ * @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. + * or ids referring to other kinds 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 */ @@ -101,28 +91,29 @@ return false; } - walk.reset(); - if (topoSort) { - walk.sort(RevSort.TOPO); + ImmutableList<RevCommit> startCommits = objectIdsToCommits(walk, starters); + if (startCommits.isEmpty()) { + return false; } - 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; + return !walk.createReachabilityChecker() + .areAllReachable(ImmutableList.of(commit), startCommits) + .isPresent(); } - private static void markUninteresting(RevWalk walk, ObjectId id) throws IOException { - if (id == null) { - return; + private static ImmutableList<RevCommit> objectIdsToCommits(RevWalk walk, Collection<ObjectId> ids) + throws IOException { + ImmutableList.Builder<RevCommit> commits = ImmutableList.builder(); + for (ObjectId id : ids) { + try { + commits.add(walk.parseCommit(id)); + } catch (MissingObjectException e) { + // TODO(ifrade): ResolveParser has already checked that the object exists in the repo. + // Report as AssertionError. + } catch (IncorrectObjectTypeException e) { + // Ignore, doesn't affect commit reachability + } } - try { - walk.markUninteresting(walk.parseCommit(id)); - } catch (IncorrectObjectTypeException | MissingObjectException e) { - // Do nothing, doesn't affect reachability. - } + return commits.build(); } }
diff --git a/javatests/com/google/gitiles/RevisionParserTest.java b/javatests/com/google/gitiles/RevisionParserTest.java index b2c88ea..16e76a7 100644 --- a/javatests/com/google/gitiles/RevisionParserTest.java +++ b/javatests/com/google/gitiles/RevisionParserTest.java
@@ -46,7 +46,7 @@ new RevisionParser( repo.getRepository(), new TestGitilesAccess(repo.getRepository()).forRequest(null), - new VisibilityCache(false, CacheBuilder.newBuilder().maximumSize(0))); + new VisibilityCache(CacheBuilder.newBuilder().maximumSize(0))); } @Test
diff --git a/javatests/com/google/gitiles/TestViewFilter.java b/javatests/com/google/gitiles/TestViewFilter.java index 68acffb..4f67efc 100644 --- a/javatests/com/google/gitiles/TestViewFilter.java +++ b/javatests/com/google/gitiles/TestViewFilter.java
@@ -65,7 +65,7 @@ new ViewFilter( new TestGitilesAccess(repo.getRepository()), TestGitilesUrls.URLS, - new VisibilityCache(false)); + new VisibilityCache()); MetaFilter mf = new MetaFilter(); for (Pattern p : ImmutableList.of(ROOT_REGEX, REPO_REGEX, REPO_PATH_REGEX)) {
diff --git a/javatests/com/google/gitiles/VisibilityCacheTest.java b/javatests/com/google/gitiles/VisibilityCacheTest.java index bcc2b40..92ea04b 100644 --- a/javatests/com/google/gitiles/VisibilityCacheTest.java +++ b/javatests/com/google/gitiles/VisibilityCacheTest.java
@@ -108,7 +108,7 @@ git.update("refs/tags/v0.1", commitA); } - visibilityCache = new VisibilityCache(true); + visibilityCache = new VisibilityCache(); walk = new RevWalk(repo); walk.setRetainBody(false); }
diff --git a/javatests/com/google/gitiles/VisibilityCheckerTest.java b/javatests/com/google/gitiles/VisibilityCheckerTest.java index 3171459..5470e52 100644 --- a/javatests/com/google/gitiles/VisibilityCheckerTest.java +++ b/javatests/com/google/gitiles/VisibilityCheckerTest.java
@@ -89,7 +89,7 @@ git.update("refs/tags/v0.1", commitA); } - visibilityChecker = new VisibilityChecker(true); + visibilityChecker = new VisibilityChecker(); walk = new RevWalk(repo); walk.setRetainBody(false); }