Extract findLastCommit method from BlameServlet into BlameCache
On a slow DFS backend like the one used by googlesource.com, walking
commits may be much slower than on a local disk with a hot buffer
cache, and this step may require walking hundreds of thousands of
commits.
Optimizing this operation in the most common case is not likely to be
useful, so the default implementation does no caching. However,
extracting this into the BlameCache interface allows for more complex
implementations in alternate backends.
Change-Id: Id138dbae9fb8ecb82a6b6e486d5d162869cc04ed
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/blame/BlameCache.java b/gitiles-servlet/src/main/java/com/google/gitiles/blame/BlameCache.java
index 530227d..d0c6fe5 100644
--- a/gitiles-servlet/src/main/java/com/google/gitiles/blame/BlameCache.java
+++ b/gitiles-servlet/src/main/java/com/google/gitiles/blame/BlameCache.java
@@ -21,5 +21,13 @@
import java.util.List;
public interface BlameCache {
+ /** @return the blame of a path at a given commit. */
public List<Region> get(Repository repo, ObjectId commitId, String path) throws IOException;
+
+ /**
+ * @return the last commit that modified a path, starting at the given
+ * commit.
+ */
+ public ObjectId findLastCommit(Repository repo, ObjectId commitId, String path)
+ throws IOException;
}
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/blame/BlameCacheImpl.java b/gitiles-servlet/src/main/java/com/google/gitiles/blame/BlameCacheImpl.java
index 459bccd..50b2d8f 100644
--- a/gitiles-servlet/src/main/java/com/google/gitiles/blame/BlameCacheImpl.java
+++ b/gitiles-servlet/src/main/java/com/google/gitiles/blame/BlameCacheImpl.java
@@ -31,6 +31,10 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.treewalk.filter.AndTreeFilter;
+import org.eclipse.jgit.treewalk.filter.PathFilterGroup;
+import org.eclipse.jgit.treewalk.filter.TreeFilter;
import org.eclipse.jgit.util.QuotedString;
import java.io.IOException;
@@ -124,6 +128,26 @@
}
}
+ @Override
+ public ObjectId findLastCommit(Repository repo, ObjectId commitId, String path)
+ throws IOException {
+ // Default implementation does no caching.
+ RevWalk rw = new RevWalk(repo);
+ try {
+ rw.markStart(rw.parseCommit(commitId));
+ rw.setRewriteParents(false);
+ // Don't use rename detection, even though BlameGenerator does. It is not
+ // possible for a commit to modify a path when not doing rename detection
+ // but to not modify the same path when taking renames into account.
+ rw.setTreeFilter(AndTreeFilter.create(
+ PathFilterGroup.createFromStrings(path),
+ TreeFilter.ANY_DIFF));
+ return rw.next();
+ } finally {
+ rw.release();
+ }
+ }
+
public static List<Region> loadBlame(Key key) throws IOException {
try {
BlameGenerator gen = new BlameGenerator(key.repo, key.path);
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/blame/BlameServlet.java b/gitiles-servlet/src/main/java/com/google/gitiles/blame/BlameServlet.java
index 9c3fab7..04e86a8 100644
--- a/gitiles-servlet/src/main/java/com/google/gitiles/blame/BlameServlet.java
+++ b/gitiles-servlet/src/main/java/com/google/gitiles/blame/BlameServlet.java
@@ -41,11 +41,9 @@
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevTree;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.treewalk.TreeWalk;
-import org.eclipse.jgit.treewalk.filter.AndTreeFilter;
-import org.eclipse.jgit.treewalk.filter.PathFilterGroup;
-import org.eclipse.jgit.treewalk.filter.TreeFilter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -84,7 +82,7 @@
return;
}
- RevCommit lastCommit = findLastCommit(rw, currCommit, view.getPathPart());
+ ObjectId lastCommit = cache.findLastCommit(repo, currCommit, view.getPathPart());
ObjectId lastCommitBlobId = resolveBlob(view, rw, lastCommit);
if (!Objects.equal(currCommitBlobId, lastCommitBlobId)) {
@@ -125,30 +123,14 @@
}
}
- private static RevCommit findLastCommit(RevWalk rw, RevCommit curr, String path)
- throws IOException {
- rw.markStart(curr);
- rw.setRewriteParents(false);
- // Don't use rename detection, even though BlameGenerator does. It is not
- // possible for a commit to modify a path when not doing rename detection
- // but to not modify the same path when taking renames into account.
- rw.setTreeFilter(AndTreeFilter.create(
- PathFilterGroup.createFromStrings(path),
- TreeFilter.ANY_DIFF));
- try {
- return rw.next();
- } finally {
- rw.reset();
- }
- }
-
- private static ObjectId resolveBlob(GitilesView view, RevWalk rw, RevCommit commit)
+ private static ObjectId resolveBlob(GitilesView view, RevWalk rw, ObjectId commitId)
throws IOException {
try {
- if (commit == null || Strings.isNullOrEmpty(view.getPathPart())) {
+ if (commitId == null || Strings.isNullOrEmpty(view.getPathPart())) {
return null;
}
- TreeWalk tw = TreeWalk.forPath(rw.getObjectReader(), view.getPathPart(), commit.getTree());
+ RevTree tree = rw.parseTree(commitId);
+ TreeWalk tw = TreeWalk.forPath(rw.getObjectReader(), view.getPathPart(), tree);
if (tw == null || (tw.getRawMode(0) & FileMode.TYPE_FILE) == 0) {
return null;
}
