Fix relative hyperlinks in Markdown Generate links using GitilesView class, which always produces URLs starting with '/', including the servlet context, repository, revision and path. This avoids ambiguity when rendering README.md in the repository index or tree views. Downside is slightly more verbose HTML, but gzip may find the pattern common enough to include the prefix in its dictionary. Add a new unit test for the MarkdownToHtml href resolver to cover more of these cases. Issue: 106 Issue: 109 Change-Id: Icf4d0259f28e561cf98a9ce8b34b8c906cead23b
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/doc/MarkdownToHtml.java b/gitiles-servlet/src/main/java/com/google/gitiles/doc/MarkdownToHtml.java index 7d0d4e1..1b5df78 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/doc/MarkdownToHtml.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/doc/MarkdownToHtml.java
@@ -17,6 +17,8 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.gitiles.doc.MarkdownUtil.getInnerText; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.CharMatcher; import com.google.common.base.Strings; import com.google.gitiles.GitilesView; import com.google.gitiles.ThreadSafePrettifyParser; @@ -361,49 +363,52 @@ } } - private String href(String url) { - if (HtmlBuilder.isValidHttpUri(url)) { - return url; - } else if (MarkdownUtil.isAbsolutePathToMarkdown(url)) { - return GitilesView.doc().copyFrom(view).setPathPart(url).build().toUrl(); - } else if (url.startsWith("/")) { - return GitilesView.show().copyFrom(view).setPathPart(url).build().toUrl(); - } else if (!url.startsWith("../") && !url.startsWith("./")) { - return url; + @VisibleForTesting + String href(String target) { + if (HtmlBuilder.isValidHttpUri(target)) { + return target; } - String dir = Strings.nullToEmpty(view.getPathPart()); - if (!readme) { - int slash = dir.lastIndexOf('/'); - dir = slash < 0 ? "" : dir.substring(0, slash); + if (target.startsWith("/")) { + return toPath(target); } - while (!url.isEmpty()) { - if (url.startsWith("./")) { - url = url.substring(2); - } else if (url.startsWith("../")) { + + String viewPath = Strings.nullToEmpty(view.getPathPart()); + String dir; + if (readme) { + dir = CharMatcher.is('/').trimTrailingFrom(viewPath); + } else { + // When readme is false this instance is rendering a file, whose name + // appears as the last component of viewPath. Other links are relative + // to the file's container directory, so trim the file. + dir = trimLastComponent(viewPath); + } + + while (!target.isEmpty()) { + if (target.startsWith("../") || target.equals("..")) { if (dir.isEmpty()) { return FilterNormalizeUri.INSTANCE.getInnocuousOutput(); } - url = url.substring(3); - - int slash = dir.lastIndexOf('/'); - if (slash >= 0) { - dir = dir.substring(0, slash); - } else { - dir = ""; - break; - } + dir = trimLastComponent(dir); + target = target.equals("..") ? "" : target.substring(3); + } else if (target.startsWith("./")) { + target = target.substring(2); + } else if (target.equals(".")) { + target = ""; } else { break; } } + return toPath(dir + '/' + target); + } - if (url.isEmpty()) { - return FilterNormalizeUri.INSTANCE.getInnocuousOutput(); - } + private static String trimLastComponent(String path) { + int slash = path.lastIndexOf('/'); + return slash < 0 ? "" : path.substring(0, slash); + } - GitilesView.Builder dest = url.endsWith(".md") ? GitilesView.doc() : GitilesView.show(); - return dest.copyFrom(view).setPathPart(dir + url).build().toUrl(); + private String toPath(String path) { + return GitilesView.path().copyFrom(view).setPathPart(path).build().toUrl(); } @Override
diff --git a/gitiles-servlet/src/test/java/com/google/gitiles/doc/DocServletTest.java b/gitiles-servlet/src/test/java/com/google/gitiles/doc/DocServletTest.java index ad0f4e2..f73c331 100644 --- a/gitiles-servlet/src/test/java/com/google/gitiles/doc/DocServletTest.java +++ b/gitiles-servlet/src/test/java/com/google/gitiles/doc/DocServletTest.java
@@ -52,8 +52,8 @@ assertThat(html).contains("<title>Site Title - page</title>"); assertThat(html).contains("<span class=\"Header-anchorTitle\">Site Title</span>"); - assertThat(html).contains("<li><a href=\"index.md\">Home</a></li>"); - assertThat(html).contains("<li><a href=\"README.md\">README</a></li>"); + assertThat(html).contains("<li><a href=\"/b/repo/+/master/index.md\">Home</a></li>"); + assertThat(html).contains("<li><a href=\"/b/repo/+/master/README.md\">README</a></li>"); assertThat(html) .contains( "<h1>" + "<a class=\"h\" name=\"page\" href=\"#page\"><span></span></a>" + "page</h1>"); @@ -108,7 +108,7 @@ repo.branch("master").commit().add("A/B/README.md", "[c](../../C)").create(); String html = buildHtml("/repo/+doc/master/A/B/README.md"); - assertThat(html).contains("<a href=\"/b/repo/+show/master/C\">c</a>"); + assertThat(html).contains("<a href=\"/b/repo/+/master/C\">c</a>"); } @Test @@ -116,6 +116,6 @@ repo.branch("master").commit().add("README.md", "[c](/x)").create(); String html = buildHtml("/repo/+doc/master/README.md"); - assertThat(html).contains("<a href=\"/b/repo/+show/master/x\">c</a>"); + assertThat(html).contains("<a href=\"/b/repo/+/master/x\">c</a>"); } }
diff --git a/gitiles-servlet/src/test/java/com/google/gitiles/doc/LinkTest.java b/gitiles-servlet/src/test/java/com/google/gitiles/doc/LinkTest.java new file mode 100644 index 0000000..7b3eced --- /dev/null +++ b/gitiles-servlet/src/test/java/com/google/gitiles/doc/LinkTest.java
@@ -0,0 +1,146 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gitiles.doc; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.gitiles.GitilesView; + +import org.eclipse.jgit.lib.Config; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class LinkTest { + private GitilesView view; + private Config config; + + @Before + public void setup() { + view = GitilesView.revision() + .setHostName("127.0.0.1") + .setServletPath("/g") + .setRepositoryName("repo") + .setRevision("HEAD") + .build(); + config = new Config(); + } + + @Test + public void httpLink() { + MarkdownToHtml md = new MarkdownToHtml(view, config); + String url; + + url = "http://example.com/foo.html"; + assertThat(md.href(url)).isEqualTo(url); + + url = "https://example.com/foo.html"; + assertThat(md.href(url)).isEqualTo(url); + + url = "//example.com/foo.html"; + assertThat(md.href(url)).isEqualTo(url); + } + + @Test + public void absolutePath() { + MarkdownToHtml md = new MarkdownToHtml(view, config); + + assertThat(md.href("/")).isEqualTo("/g/repo/+/HEAD/"); + assertThat(md.href("/index.md")).isEqualTo("/g/repo/+/HEAD/index.md"); + assertThat(md.href("/doc/index.md")).isEqualTo("/g/repo/+/HEAD/doc/index.md"); + + // GitilesView trims trailing '/' from path expressions. + assertThat(md.href("/doc/")).isEqualTo("/g/repo/+/HEAD/doc"); + } + + @Test + public void relativePathInRootFile() { + testMarkdownInRoot(file("/index.md")); + } + + @Test + public void relativePathInTreeFile() { + testMarkdownInTree(file("/doc/index.md")); + } + + @Test + public void relativePathInRepositoryIndexReadme() { + testMarkdownInRoot(repoIndexReadme()); + } + + @Test + public void relativePathInCommitReadme() { + testMarkdownInRoot(revisionReadme()); + } + + @Test + public void relativePathInTreeReadme() { + testMarkdownInTree(treeReadme("/doc")); + testMarkdownInTree(treeReadme("/doc/")); + } + + private static void testMarkdownInRoot(MarkdownToHtml md) { + assertThat(md.href("setup.md")).isEqualTo("/g/repo/+/HEAD/setup.md"); + assertThat(md.href("./setup.md")).isEqualTo("/g/repo/+/HEAD/setup.md"); + assertThat(md.href("./")).isEqualTo("/g/repo/+/HEAD/"); + assertThat(md.href(".")).isEqualTo("/g/repo/+/HEAD/"); + + assertThat(md.href("../")).isEqualTo("#zSoyz"); + assertThat(md.href("../../")).isEqualTo("#zSoyz"); + assertThat(md.href("../..")).isEqualTo("#zSoyz"); + assertThat(md.href("..")).isEqualTo("#zSoyz"); + } + + private static void testMarkdownInTree(MarkdownToHtml md) { + assertThat(md.href("setup.md")).isEqualTo("/g/repo/+/HEAD/doc/setup.md"); + assertThat(md.href("./setup.md")).isEqualTo("/g/repo/+/HEAD/doc/setup.md"); + assertThat(md.href("../setup.md")).isEqualTo("/g/repo/+/HEAD/setup.md"); + assertThat(md.href("../tech/setup.md")).isEqualTo("/g/repo/+/HEAD/tech/setup.md"); + + assertThat(md.href("./")).isEqualTo("/g/repo/+/HEAD/doc"); + assertThat(md.href(".")).isEqualTo("/g/repo/+/HEAD/doc"); + assertThat(md.href("../")).isEqualTo("/g/repo/+/HEAD/"); + assertThat(md.href("..")).isEqualTo("/g/repo/+/HEAD/"); + + assertThat(md.href("../../")).isEqualTo("#zSoyz"); + assertThat(md.href("../..")).isEqualTo("#zSoyz"); + assertThat(md.href("../../../")).isEqualTo("#zSoyz"); + assertThat(md.href("../../..")).isEqualTo("#zSoyz"); + } + + private MarkdownToHtml file(String path) { + return new MarkdownToHtml( + GitilesView.doc().copyFrom(view).setPathPart(path).build(), + config); + } + + private MarkdownToHtml repoIndexReadme() { + return readme(view); + } + + private MarkdownToHtml revisionReadme() { + return readme(GitilesView.revision().copyFrom(view).build()); + } + + private MarkdownToHtml treeReadme(String path) { + return readme(GitilesView.path().copyFrom(view).setPathPart(path).build()); + } + + private MarkdownToHtml readme(GitilesView v) { + return new MarkdownToHtml(v, config).setReadme(true); + } +}