Include the path in RevisionParser.Result instead of an offset This simplifies the logic in the various ViewFilter methods; clean those up a bit more as well. Change-Id: I5fb138b18e3f67b9036ce9bbed179d8cfa6197ee
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/RevisionParser.java b/gitiles-servlet/src/main/java/com/google/gitiles/RevisionParser.java index 743b909..d540647 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/RevisionParser.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/RevisionParser.java
@@ -39,18 +39,18 @@ static class Result { private final Revision revision; private final Revision oldRevision; - private final int pathStart; + private final String path; @VisibleForTesting Result(Revision revision) { - this(revision, null, revision.getName().length()); + this(revision, null, ""); } @VisibleForTesting - Result(Revision revision, Revision oldRevision, int pathStart) { + Result(Revision revision, Revision oldRevision, String path) { this.revision = revision; this.oldRevision = oldRevision; - this.pathStart = pathStart; + this.path = path; } public Revision getRevision() { @@ -61,35 +61,35 @@ return oldRevision; } + public String getPath() { + return path; + } + @Override public boolean equals(Object o) { if (o instanceof Result) { Result r = (Result) o; return Objects.equal(revision, r.revision) && Objects.equal(oldRevision, r.oldRevision) - && Objects.equal(pathStart, r.pathStart); + && Objects.equal(path, r.path); } return false; } @Override public int hashCode() { - return Objects.hashCode(revision, oldRevision, pathStart); + return Objects.hashCode(revision, oldRevision, path); } @Override public String toString() { return Objects.toStringHelper(this) .omitNullValues() - .add("revision", revision) - .add("oldRevision", oldRevision) - .add("pathStart", pathStart) + .add("revision", revision.getName()) + .add("oldRevision", oldRevision != null ? oldRevision.getName() : null) + .add("path", path) .toString(); } - - int getPathStart() { - return pathStart; - } } private final Repository repo; @@ -103,6 +103,9 @@ } Result parse(String path) throws IOException { + if (path.startsWith("/")) { + path = path.substring(1); + } RevWalk walk = new RevWalk(repo); try { Revision oldRevision = null; @@ -162,7 +165,7 @@ } else { oldRevision = Revision.NULL; } - Result result = new Result(Revision.peeled(name, c), oldRevision, name.length() + 2); + Result result = new Result(Revision.peeled(name, c), oldRevision, path.substring(name.length() + 2)); return isVisible(walk, result) ? result : null; } } @@ -181,7 +184,7 @@ // foo..bar (foo may be empty) pathStart = oldRevision.getName().length() + 2 + name.length(); } - Result result = new Result(Revision.peel(name, obj, walk), oldRevision, pathStart); + Result result = new Result(Revision.peel(name, obj, walk), oldRevision, path.substring(pathStart)); return isVisible(walk, result) ? result : null; } first = false;
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 896dff0..767b99c 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/ViewFilter.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/ViewFilter.java
@@ -60,8 +60,12 @@ } static String trimLeadingSlash(String str) { + return checkLeadingSlash(str).substring(1); + } + + private static String checkLeadingSlash(String str) { checkArgument(str.startsWith("/"), "expected string starting with a slash: %s", str); - return str.substring(1); + return str; } private static boolean isEmptyOrSlash(String path) { @@ -139,15 +143,14 @@ if (path.isEmpty()) { return null; } - path = trimLeadingSlash(path); RevisionParser.Result result = parseRevision(req, path); if (result == null) { return null; } if (result.getOldRevision() != null) { - return parseDiffCommand(repoName, result, path); + return parseDiffCommand(repoName, result); } else { - return parseShowCommand(repoName, result, path); + return parseShowCommand(repoName, result); } } @@ -163,21 +166,19 @@ private GitilesView.Builder parseDiffCommand( HttpServletRequest req, String repoName, String path) throws IOException { - path = trimLeadingSlash(path); - RevisionParser.Result result = parseRevision(req, path); - if (result == null) { - return null; - } - return parseDiffCommand(repoName, result, path); + return parseDiffCommand(repoName, parseRevision(req, path)); } private GitilesView.Builder parseDiffCommand( - String repoName, RevisionParser.Result result, String path) { + String repoName, RevisionParser.Result result) { + if (result == null) { + return null; + } return GitilesView.diff() .setRepositoryName(repoName) .setRevision(result.getRevision()) .setOldRevision(result.getOldRevision()) - .setPathPart(path.substring(result.getPathStart())); + .setPathPart(result.getPath()); } private GitilesView.Builder parseLogCommand( @@ -185,7 +186,6 @@ if (isEmptyOrSlash(path)) { return GitilesView.log().setRepositoryName(repoName); } - path = trimLeadingSlash(path); RevisionParser.Result result = parseRevision(req, path); if (result == null) { return null; @@ -194,7 +194,7 @@ .setRepositoryName(repoName) .setRevision(result.getRevision()) .setOldRevision(result.getOldRevision()) - .setPathPart(path.substring(result.getPathStart())); + .setPathPart(result.getPath()); } private GitilesView.Builder parseRefsCommand( @@ -206,21 +206,15 @@ private GitilesView.Builder parseShowCommand( HttpServletRequest req, String repoName, String path) throws IOException { - path = trimLeadingSlash(path); - RevisionParser.Result result = parseRevision(req, path); - if (result == null) { - return null; - } - return parseShowCommand(repoName, result, path); + return parseShowCommand(repoName, parseRevision(req, path)); } private GitilesView.Builder parseShowCommand( - String repoName, RevisionParser.Result result, String path) { - if (result.getOldRevision() != null) { + String repoName, RevisionParser.Result result) { + if (result == null || result.getOldRevision() != null) { return null; } - path = path.substring(result.getPathStart()); - if (path.isEmpty()) { + if (result.getPath().isEmpty()) { return GitilesView.revision() .setRepositoryName(repoName) .setRevision(result.getRevision()); @@ -228,7 +222,7 @@ return GitilesView.path() .setRepositoryName(repoName) .setRevision(result.getRevision()) - .setPathPart(path); + .setPathPart(result.getPath()); } } @@ -236,6 +230,6 @@ throws IOException { RevisionParser revParser = new RevisionParser( ServletUtils.getRepository(req), accessFactory.forRequest(req), visibilityCache); - return revParser.parse(path); + return revParser.parse(checkLeadingSlash(path)); } }
diff --git a/gitiles-servlet/src/test/java/com/google/gitiles/RevisionParserTest.java b/gitiles-servlet/src/test/java/com/google/gitiles/RevisionParserTest.java index 9d26504..6cba43d 100644 --- a/gitiles-servlet/src/test/java/com/google/gitiles/RevisionParserTest.java +++ b/gitiles-servlet/src/test/java/com/google/gitiles/RevisionParserTest.java
@@ -133,37 +133,37 @@ new Result( Revision.peeled("master", commit), Revision.peeled("master^", parent), - 15), + ""), parser.parse("master^..master")); assertEquals( new Result( Revision.peeled("master", commit), Revision.peeled("master^", parent), - 15), + "/"), parser.parse("master^..master/")); assertEquals( new Result( Revision.peeled("master", commit), Revision.peeled("master^", parent), - 15), + "/path/to/a/file"), parser.parse("master^..master/path/to/a/file")); assertEquals( new Result( Revision.peeled("master", commit), Revision.peeled("master^", parent), - 15), + "/path/to/a/..file"), parser.parse("master^..master/path/to/a/..file")); assertEquals( new Result( Revision.peeled("refs/heads/master", commit), Revision.peeled("refs/heads/master^", parent), - 37), + ""), parser.parse("refs/heads/master^..refs/heads/master")); assertEquals( new Result( Revision.peeled("master", commit), Revision.peeled("master~1", parent), - 16), + ""), parser.parse("master~1..master")); // TODO(dborowitz): 2a2362fbb in JGit causes master~2 to resolve to master // rather than null. Uncomment when upstream regression is fixed. @@ -172,7 +172,7 @@ new Result( Revision.peeled("master", commit), Revision.peeled("other", other), - 13), + ""), parser.parse("other..master")); } @@ -184,19 +184,19 @@ new Result( Revision.peeled("master", commit), Revision.peeled("master^", parent), - 8), + ""), parser.parse("master^!")); assertEquals( new Result( Revision.peeled("master^", parent), Revision.NULL, - 9), + ""), parser.parse("master^^!")); assertEquals( new Result( Revision.peeled(parent.name(), parent), Revision.NULL, - 42), + ""), parser.parse(parent.name() + "^!")); repo.update("refs/tags/tag", repo.tag("tag", commit)); @@ -204,13 +204,13 @@ new Result( Revision.peeled("tag", commit), Revision.peeled("tag^", parent), - 5), + ""), parser.parse("tag^!")); assertEquals( new Result( Revision.peeled("tag^", parent), Revision.NULL, - 6), + ""), parser.parse("tag^^!")); } @@ -225,13 +225,13 @@ new Result( Revision.peeled("master", master), Revision.peeled("other", other), - 13), + ""), parser.parse("other..master")); assertEquals( new Result( Revision.peeled("other", other), Revision.peeled("master", master), - 13), + ""), parser.parse("master..other")); }