Redirect to normalized versions of parent revision expressions Repeatedly clicking the "parent" link in a commit page can result in amusing but unhelpful URLs like "/+/master^^^^^^^^^^^^". Avoid such URLs by detecting when a revision is a parent expression, and sending a 302 redirect to the absolute revision. Change-Id: I8aec460a99eba581052e0319069eacd8f3e2e743
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/GitilesView.java b/gitiles-servlet/src/main/java/com/google/gitiles/GitilesView.java index fc553c7..d3bf7cf 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/GitilesView.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/GitilesView.java
@@ -196,7 +196,8 @@ case LOG: break; default: - checkState(revision == null, "cannot set old revision on %s view", type); + revision = Objects.firstNonNull(revision, Revision.NULL); + checkState(revision == Revision.NULL, "cannot set old revision on %s view", type); break; } this.oldRevision = revision; @@ -212,7 +213,7 @@ } public Revision getOldRevision() { - return revision; + return oldRevision; } public Builder setPathPart(String path) {
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/Revision.java b/gitiles-servlet/src/main/java/com/google/gitiles/Revision.java index 3fdf777..a93d4b5 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/Revision.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/Revision.java
@@ -43,6 +43,14 @@ /** Common default branch given to clients. */ public static final Revision HEAD = named("HEAD"); + public static Revision normalizeParentExpressions(Revision rev) { + if (rev == null + || (rev.name.indexOf("~") < 0 && rev.name.indexOf("^") < 0)) { + return rev; + } + return new Revision(rev.id.name(), rev.id, rev.type, rev.peeledId, rev.peeledType); + } + private final String name; private final ObjectId id; private final int type;
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/ViewFilter.java b/gitiles-servlet/src/main/java/com/google/gitiles/ViewFilter.java index 13537dc..e8b1055 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/ViewFilter.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/ViewFilter.java
@@ -106,11 +106,16 @@ res.setStatus(SC_NOT_FOUND); return; } + @SuppressWarnings("unchecked") Map<String, String[]> params = req.getParameterMap(); view.setHostName(urls.getHostName(req)) .setServletPath(req.getContextPath() + req.getServletPath()) .putAllParams(params); + if (normalize(view, res)) { + return; + } + setView(req, view.build()); try { chain.doFilter(req, res); @@ -119,6 +124,20 @@ } } + private boolean normalize(GitilesView.Builder view, HttpServletResponse res) + throws IOException { + if (view.getOldRevision() != Revision.NULL) { + return false; + } + Revision r = view.getRevision(); + Revision nr = Revision.normalizeParentExpressions(r); + if (r != nr) { + res.sendRedirect(view.setRevision(nr).toUrl()); + return true; + } + return false; + } + private GitilesView.Builder parse(HttpServletRequest req) throws IOException { String repoName = trimLeadingSlash(getRegexGroup(req, 1)); if (repoName.isEmpty()) {
diff --git a/gitiles-servlet/src/test/java/com/google/gitiles/FakeHttpServletResponse.java b/gitiles-servlet/src/test/java/com/google/gitiles/FakeHttpServletResponse.java index 85da54f..558eb4c 100644 --- a/gitiles-servlet/src/test/java/com/google/gitiles/FakeHttpServletResponse.java +++ b/gitiles-servlet/src/test/java/com/google/gitiles/FakeHttpServletResponse.java
@@ -208,8 +208,9 @@ } @Override - public synchronized void sendRedirect(String msg) { + public synchronized void sendRedirect(String loc) { status = SC_FOUND; + setHeader(HttpHeaders.LOCATION, loc); committed = true; }
diff --git a/gitiles-servlet/src/test/java/com/google/gitiles/ViewFilterTest.java b/gitiles-servlet/src/test/java/com/google/gitiles/ViewFilterTest.java index 4e80c39..2660541 100644 --- a/gitiles-servlet/src/test/java/com/google/gitiles/ViewFilterTest.java +++ b/gitiles-servlet/src/test/java/com/google/gitiles/ViewFilterTest.java
@@ -20,6 +20,7 @@ import static com.google.gitiles.GitilesFilter.ROOT_REGEX; import com.google.common.collect.ImmutableList; +import com.google.common.net.HttpHeaders; import com.google.common.util.concurrent.Atomics; import com.google.gitiles.GitilesView.Type; @@ -440,8 +441,43 @@ assertEquals("foo/bar", view.getPathPart()); } + public void testNormalizeParents() throws Exception { + RevCommit parent = repo.commit().create(); + RevCommit master = repo.branch("refs/heads/master").commit().parent(parent).create(); + GitilesView view; + + assertEquals("/b/repo/+/master", getView("/repo/+/master").toUrl()); + assertEquals("/b/repo/+/" + master.name(), getView("/repo/+/" + master.name()).toUrl()); + assertEquals("/b/repo/+/" + parent.name(), getRedirectUrl("/repo/+/master~")); + assertEquals("/b/repo/+/" + parent.name(), getRedirectUrl("/repo/+/master^")); + + view = getView("/repo/+log/master~..master/"); + assertEquals("master", view.getRevision().getName()); + assertEquals("master~", view.getOldRevision().getName()); + + view = getView("/repo/+log/master^!/"); + assertEquals("master", view.getRevision().getName()); + assertEquals("master^", view.getOldRevision().getName()); + } + + private String getRedirectUrl(String pathAndQuery) throws ServletException, IOException { + FakeHttpServletResponse res = new FakeHttpServletResponse(); + service(pathAndQuery, Atomics.<GitilesView> newReference(), res); + assertEquals(302, res.getStatus()); + return res.getHeader(HttpHeaders.LOCATION); + } + private GitilesView getView(String pathAndQuery) throws ServletException, IOException { - final AtomicReference<GitilesView> view = Atomics.newReference(); + AtomicReference<GitilesView> view = Atomics.newReference(); + FakeHttpServletResponse res = new FakeHttpServletResponse(); + service(pathAndQuery, view, res); + assertTrue("expected non-redirect status, got " + res.getStatus(), + res.getStatus() < 300 || res.getStatus() >= 400); + return view.get(); + } + + private void service(String pathAndQuery, final AtomicReference<GitilesView> view, + FakeHttpServletResponse res) throws ServletException, IOException { HttpServlet testServlet = new HttpServlet() { private static final long serialVersionUID = 1L; @Override @@ -470,9 +506,7 @@ } else { req.setPathInfo(pathAndQuery); } - dummyServlet(mf).service(req, new FakeHttpServletResponse()); - - return view.get(); + dummyServlet(mf).service(req, res); } private MetaServlet dummyServlet(MetaFilter mf) {