Refactor CommitSoyData to use more of a builder pattern These are created one-off on every request thread, so making them mutable is safe enough, and it allows us to get rid of the messy constructors. Be more explicit about which keys require which optional arguments. Change-Id: Ic175369898c206278bd8bc862bef0744b27ee79a
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/CommitSoyData.java b/gitiles-servlet/src/main/java/com/google/gitiles/CommitSoyData.java index 425a1a2..ff879ba 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/CommitSoyData.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/CommitSoyData.java
@@ -16,7 +16,6 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; - import static org.eclipse.jgit.diff.DiffEntry.ChangeType.COPY; import static org.eclipse.jgit.diff.DiffEntry.ChangeType.DELETE; import static org.eclipse.jgit.diff.DiffEntry.ChangeType.RENAME; @@ -31,6 +30,7 @@ import org.eclipse.jgit.diff.DiffEntry; import org.eclipse.jgit.diff.DiffEntry.ChangeType; import org.eclipse.jgit.diff.DiffFormatter; +import org.eclipse.jgit.http.server.ServletUtils; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; @@ -55,7 +55,6 @@ import java.util.Map; import java.util.Set; -import javax.annotation.Nullable; import javax.servlet.http.HttpServletRequest; /** Soy data converter for git commits. */ @@ -77,45 +76,47 @@ private KeySet(KeySet other, String... keys) { this.keys = ImmutableSet.<String> builder().addAll(other.keys).add(keys).build(); } + + private boolean contains(String key) { + return keys.contains(key); + } } - private final Linkifier linkifier; - private final HttpServletRequest req; - private final Repository repo; - private final RevWalk walk; - private final GitilesView view; - private final Map<AnyObjectId, Set<Ref>> refsById; - private final GitDateFormatter dateFormatter; + private final GitDateFormatter dateFormatter = new GitDateFormatter(Format.DEFAULT); + private Linkifier linkifier; + private Repository repo; + private RevWalk walk; + private GitilesView view; + private Map<AnyObjectId, Set<Ref>> refsById; - // TODO(dborowitz): This constructor is getting a bit ridiculous. - public CommitSoyData(@Nullable Linkifier linkifier, HttpServletRequest req, Repository repo, - RevWalk walk, GitilesView view) { - this(linkifier, req, repo, walk, view, null); + public CommitSoyData setLinkifier(Linkifier linkifier) { + this.linkifier = checkNotNull(linkifier, "linkifier"); + return this; } - public CommitSoyData(@Nullable Linkifier linkifier, HttpServletRequest req, Repository repo, - RevWalk walk, GitilesView view, @Nullable Map<AnyObjectId, Set<Ref>> refsById) { - this.linkifier = linkifier; - this.req = req; - this.repo = repo; - this.walk = walk; - this.view = view; - this.refsById = refsById; - this.dateFormatter = new GitDateFormatter(Format.DEFAULT); + public CommitSoyData setRevWalk(RevWalk walk) { + this.walk = checkNotNull(walk, "walk"); + return this; } - public Map<String, Object> toSoyData(RevCommit commit, KeySet keys) throws IOException { + public Map<String, Object> toSoyData(HttpServletRequest req, RevCommit commit, KeySet ks) + throws IOException { + checkKeys(ks); + req = checkNotNull(req, "request"); + repo = ServletUtils.getRepository(req); + view = ViewFilter.getView(req); + Map<String, Object> data = Maps.newHashMapWithExpectedSize(KeySet.DEFAULT.keys.size()); - if (keys.keys.contains("author")) { + if (ks.contains("author")) { data.put("author", toSoyData(commit.getAuthorIdent(), dateFormatter)); } - if (keys.keys.contains("committer")) { + if (ks.contains("committer")) { data.put("committer", toSoyData(commit.getCommitterIdent(), dateFormatter)); } - if (keys.keys.contains("sha")) { + if (ks.contains("sha")) { data.put("sha", ObjectId.toString(commit)); } - if (keys.keys.contains("abbrevSha")) { + if (ks.contains("abbrevSha")) { ObjectReader reader = repo.getObjectDatabase().newReader(); try { data.put("abbrevSha", reader.abbreviate(commit).name()); @@ -123,53 +124,57 @@ reader.release(); } } - if (keys.keys.contains("url")) { + if (ks.contains("url")) { data.put("url", GitilesView.revision() .copyFrom(view) .setRevision(commit) .toUrl()); } - if (keys.keys.contains("logUrl")) { + if (ks.contains("logUrl")) { data.put("logUrl", urlFromView(view, commit, GitilesView.log())); } - if (keys.keys.contains("tgzUrl")) { + if (ks.contains("tgzUrl")) { data.put("tgzUrl", urlFromView(view, commit, GitilesView.archive().setExtension(".tar.gz"))); } - if (keys.keys.contains("tree")) { + if (ks.contains("tree")) { data.put("tree", ObjectId.toString(commit.getTree())); } - if (keys.keys.contains("treeUrl")) { + if (ks.contains("treeUrl")) { data.put("treeUrl", GitilesView.path().copyFrom(view).setPathPart("/").toUrl()); } - if (keys.keys.contains("parents")) { + if (ks.contains("parents")) { data.put("parents", toSoyData(view, commit.getParents())); } - if (keys.keys.contains("shortMessage")) { + if (ks.contains("shortMessage")) { data.put("shortMessage", commit.getShortMessage()); } - if (keys.keys.contains("branches")) { + if (ks.contains("branches")) { data.put("branches", getRefsById(commit, Constants.R_HEADS)); } - if (keys.keys.contains("tags")) { + if (ks.contains("tags")) { data.put("tags", getRefsById(commit, Constants.R_TAGS)); } - if (keys.keys.contains("message")) { + if (ks.contains("message")) { if (linkifier != null) { data.put("message", linkifier.linkify(req, commit.getFullMessage())); } else { data.put("message", commit.getFullMessage()); } } - if (keys.keys.contains("diffTree")) { + if (ks.contains("diffTree")) { data.put("diffTree", computeDiffTree(commit)); } - checkState(keys.keys.size() == data.size(), "bad commit data keys: %s != %s", keys.keys, + checkState(ks.keys.size() == data.size(), "bad commit data keys: %s != %s", ks.keys, data.keySet()); return ImmutableMap.copyOf(data); } - public Map<String, Object> toSoyData(RevCommit commit) throws IOException { - return toSoyData(commit, KeySet.DEFAULT); + public Map<String, Object> toSoyData(HttpServletRequest req, RevCommit commit) throws IOException { + return toSoyData(req, commit, KeySet.DEFAULT); + } + + private void checkKeys(KeySet ks) { + checkState(!ks.contains("diffTree") || walk != null, "RevWalk required for diffTree"); } // TODO(dborowitz): Extract this. @@ -279,7 +284,9 @@ }; private List<Map<String, String>> getRefsById(ObjectId id, String prefix) { - checkNotNull(refsById, "must pass in ID to ref map to look up refs by ID"); + if (refsById == null) { + refsById = repo.getAllRefsByPeeledObjectId(); + } Set<Ref> refs = refsById.get(id); if (refs == null) { return ImmutableList.of();
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/DiffServlet.java b/gitiles-servlet/src/main/java/com/google/gitiles/DiffServlet.java index a8fdeec..bd97f6f 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/DiffServlet.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/DiffServlet.java
@@ -79,8 +79,9 @@ Map<String, Object> data = getData(req); data.put("title", "Diff - " + view.getRevisionRange()); if (showCommit) { - data.put("commit", new CommitSoyData(linkifier, req, repo, walk, view) - .toSoyData(walk.parseCommit(view.getRevision().getId()))); + data.put("commit", new CommitSoyData() + .setLinkifier(linkifier) + .toSoyData(req, walk.parseCommit(view.getRevision().getId()))); } if (!data.containsKey("repositoryName") && (view.getRepositoryName() != null)) { data.put("repositoryName", view.getRepositoryName());
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/LogServlet.java b/gitiles-servlet/src/main/java/com/google/gitiles/LogServlet.java index 9e16697..8ad30fc 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/LogServlet.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/LogServlet.java
@@ -31,7 +31,6 @@ import org.eclipse.jgit.errors.RevWalkException; import org.eclipse.jgit.http.server.ServletUtils; import org.eclipse.jgit.lib.AbbreviatedObjectId; -import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectReader; @@ -49,7 +48,6 @@ import java.util.Collection; import java.util.List; import java.util.Map; -import java.util.Set; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -93,7 +91,8 @@ return; } - Map<String, Object> data = new LogSoyData(req, repo, view) + Map<String, Object> data = + new LogSoyData(req, view) .toSoyData(walk, LIMIT, null, start.orNull()); if (!view.getRevision().nameIsId()) { @@ -109,11 +108,9 @@ } Paginator paginator = new Paginator(walk, LIMIT, start.orNull()); - Map<AnyObjectId, Set<Ref>> refsById = repo.getAllRefsByPeeledObjectId(); List<Map<String, Object>> entries = Lists.newArrayListWithCapacity(LIMIT); for (RevCommit c : paginator) { - entries.add(new CommitSoyData(null, req, repo, walk, view, refsById) - .toSoyData(c, KeySet.SHORTLOG)); + entries.add(new CommitSoyData().toSoyData(req, c, KeySet.SHORTLOG)); } String title = "Log - ";
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/LogSoyData.java b/gitiles-servlet/src/main/java/com/google/gitiles/LogSoyData.java index 77bd36a..f4be0a4 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/LogSoyData.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/LogSoyData.java
@@ -18,29 +18,23 @@ import com.google.common.collect.Maps; import com.google.gitiles.CommitSoyData.KeySet; -import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.Ref; -import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; import java.io.IOException; import java.util.List; import java.util.Map; -import java.util.Set; import javax.annotation.Nullable; import javax.servlet.http.HttpServletRequest; public class LogSoyData { private final HttpServletRequest req; - private final Repository repo; private final GitilesView view; - public LogSoyData(HttpServletRequest req, Repository repo, GitilesView view) { + public LogSoyData(HttpServletRequest req, GitilesView view) { this.req = req; - this.repo = repo; this.view = view; } @@ -49,11 +43,9 @@ Map<String, Object> data = Maps.newHashMapWithExpectedSize(3); Paginator paginator = new Paginator(walk, limit, start); - Map<AnyObjectId, Set<Ref>> refsById = repo.getAllRefsByPeeledObjectId(); List<Map<String, Object>> entries = Lists.newArrayListWithCapacity(limit); for (RevCommit c : paginator) { - entries.add(new CommitSoyData(null, req, repo, walk, view, refsById) - .toSoyData(c, KeySet.SHORTLOG)); + entries.add(new CommitSoyData().toSoyData(req, c, KeySet.SHORTLOG)); } data.put("entries", entries);
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/RepositoryIndexServlet.java b/gitiles-servlet/src/main/java/com/google/gitiles/RepositoryIndexServlet.java index 5a92dea..a07a649 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/RepositoryIndexServlet.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/RepositoryIndexServlet.java
@@ -74,7 +74,8 @@ if (head.getType() == Constants.OBJ_COMMIT) { walk.reset(); walk.markStart((RevCommit) head); - data = new LogSoyData(req, repo, view).toSoyData(walk, LOG_LIMIT, "HEAD", null); + data = + new LogSoyData(req, view).toSoyData(walk, LOG_LIMIT, "HEAD", null); } else { // TODO(dborowitz): Handle non-commit or missing HEAD? data = Maps.newHashMapWithExpectedSize(6);
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/RevisionServlet.java b/gitiles-servlet/src/main/java/com/google/gitiles/RevisionServlet.java index e4258c0..bf9de35 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/RevisionServlet.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/RevisionServlet.java
@@ -75,8 +75,10 @@ case OBJ_COMMIT: soyObjects.add(ImmutableMap.of( "type", Constants.TYPE_COMMIT, - "data", new CommitSoyData(linkifier, req, repo, walk, view) - .toSoyData((RevCommit) obj, KeySet.DETAIL_DIFF_TREE))); + "data", new CommitSoyData() + .setLinkifier(linkifier) + .setRevWalk(walk) + .toSoyData(req, (RevCommit) obj, KeySet.DETAIL_DIFF_TREE))); break; case OBJ_TREE: soyObjects.add(ImmutableMap.of(