Properly handle relative submodule URL paths
JGit's SubmoduleWalk tries to resolve relative submodule URLs relative
to the working tree of the repo, which fails on bare repos. JGit
should be able to handle bare file-based repos, but Gitiles is
designed to work even with non-file-based repos. So, do relative path
resolving ourselves.
While we're there, more gracefully handle the case where the submodule
URL can't be resolved for whatever reason.
Change-Id: I5e00b88e041246f8c9b0725817ca9d7345898feb
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/GitilesFilter.java b/gitiles-servlet/src/main/java/com/google/gitiles/GitilesFilter.java
index 57f9306..a69835c 100644
--- a/gitiles-servlet/src/main/java/com/google/gitiles/GitilesFilter.java
+++ b/gitiles-servlet/src/main/java/com/google/gitiles/GitilesFilter.java
@@ -235,7 +235,7 @@
case REVISION:
return new RevisionServlet(renderer, linkifier());
case PATH:
- return new PathServlet(renderer);
+ return new PathServlet(renderer, urls);
case DIFF:
return new DiffServlet(renderer, linkifier());
case LOG:
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 67040fe..ca6315e 100644
--- a/gitiles-servlet/src/main/java/com/google/gitiles/GitilesView.java
+++ b/gitiles-servlet/src/main/java/com/google/gitiles/GitilesView.java
@@ -17,7 +17,6 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.gitiles.GitilesUrls.NAME_ESCAPER;
-import static com.google.gitiles.RevisionParser.PATH_SPLITTER;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Charsets;
@@ -559,7 +558,7 @@
breadcrumbs.add(breadcrumb(".", copyWithPath().setTreePath("")));
}
StringBuilder cur = new StringBuilder();
- List<String> parts = ImmutableList.copyOf(PATH_SPLITTER.omitEmptyStrings().split(path));
+ List<String> parts = ImmutableList.copyOf(Paths.SPLITTER.omitEmptyStrings().split(path));
checkArgument(hasSingleTree == null
|| (parts.isEmpty() && hasSingleTree.isEmpty())
|| hasSingleTree.size() == parts.size() - 1,
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/PathServlet.java b/gitiles-servlet/src/main/java/com/google/gitiles/PathServlet.java
index fff423a..e52cdf6 100644
--- a/gitiles-servlet/src/main/java/com/google/gitiles/PathServlet.java
+++ b/gitiles-servlet/src/main/java/com/google/gitiles/PathServlet.java
@@ -14,9 +14,12 @@
package com.google.gitiles;
+import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.gitiles.TreeSoyData.resolveTargetUrl;
+
import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND;
+
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import static org.eclipse.jgit.lib.Constants.OBJ_COMMIT;
import static org.eclipse.jgit.lib.Constants.OBJ_TREE;
@@ -103,8 +106,11 @@
}
}
- public PathServlet(Renderer renderer) {
+ private final GitilesUrls urls;
+
+ public PathServlet(Renderer renderer, GitilesUrls urls) {
super(renderer);
+ this.urls = checkNotNull(urls, "urls");
}
@Override
@@ -376,9 +382,18 @@
SubmoduleWalk sw = SubmoduleWalk.forPath(ServletUtils.getRepository(req), root,
view.getTreePath());
- String remoteUrl;
+ String modulesUrl;
+ String remoteUrl = null;
try {
- remoteUrl = sw.getRemoteUrl();
+ modulesUrl = sw.getModulesUrl();
+ if (modulesUrl != null && (modulesUrl.startsWith("./") || modulesUrl.startsWith("../"))) {
+ String moduleRepo = Paths.simplifyPathUpToRoot(modulesUrl, view.getRepositoryName());
+ if (moduleRepo != null) {
+ modulesUrl = urls.getBaseGitUrl(req) + moduleRepo;
+ }
+ } else {
+ remoteUrl = sw.getRemoteUrl();
+ }
} catch (ConfigInvalidException e) {
throw new IOException(e);
} finally {
@@ -387,22 +402,25 @@
Map<String, Object> data = Maps.newHashMap();
data.put("sha", ObjectId.toString(tw.getObjectId(0)));
- data.put("remoteUrl", remoteUrl);
+ data.put("remoteUrl", remoteUrl != null ? remoteUrl : modulesUrl);
- // TODO(dborowitz): Guess when we can put commit SHAs in the URL.
- String httpUrl = resolveHttpUrl(remoteUrl);
- if (httpUrl != null) {
- data.put("httpUrl", httpUrl);
- }
+ // TODO(dborowitz): Guess when we can put commit SHAs in the URL.
+ String httpUrl = resolveHttpUrl(remoteUrl);
+ if (httpUrl != null) {
+ data.put("httpUrl", httpUrl);
+ }
- // TODO(sop): Allow caching links by SHA-1 when no S cookie is sent.
- renderHtml(req, res, "gitiles.pathDetail", ImmutableMap.of(
- "title", view.getTreePath(),
- "type", FileType.GITLINK.toString(),
- "data", data));
+ // TODO(sop): Allow caching links by SHA-1 when no S cookie is sent.
+ renderHtml(req, res, "gitiles.pathDetail", ImmutableMap.of(
+ "title", view.getTreePath(),
+ "type", FileType.GITLINK.toString(),
+ "data", data));
}
private static String resolveHttpUrl(String remoteUrl) {
+ if (remoteUrl == null) {
+ return null;
+ }
return VERBATIM_SUBMODULE_URL_PATTERN.matcher(remoteUrl).matches() ? remoteUrl : null;
}
}
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/Paths.java b/gitiles-servlet/src/main/java/com/google/gitiles/Paths.java
new file mode 100644
index 0000000..e4e66af
--- /dev/null
+++ b/gitiles-servlet/src/main/java/com/google/gitiles/Paths.java
@@ -0,0 +1,53 @@
+// Copyright 2012 Google Inc. All Rights Reserved.
+//
+// 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;
+
+import com.google.common.base.Splitter;
+import com.google.common.base.Strings;
+import com.google.common.io.Files;
+
+import java.util.StringTokenizer;
+
+/** Static utilities for dealing with pathnames. */
+class Paths {
+ static final Splitter SPLITTER = Splitter.on('/');
+
+ static String simplifyPathUpToRoot(String path, String root) {
+ if (path.startsWith("/")) {
+ return null;
+ }
+
+ root = Strings.nullToEmpty(root);
+ // simplifyPath() normalizes "a/../../" to "a", so manually check whether
+ // the path leads above the root.
+ int depth = new StringTokenizer(root, "/").countTokens();
+ for (String part : SPLITTER.split(path)) {
+ if (part.equals("..")) {
+ depth--;
+ if (depth < 0) {
+ return null;
+ }
+ } else if (!part.isEmpty() && !part.equals(".")) {
+ depth++;
+ }
+ }
+
+ String result = Files.simplifyPath(!root.isEmpty() ? root + "/" + path : path);
+ return !result.equals(".") ? result : "";
+ }
+
+ private Paths() {
+ }
+}
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 cabde7c..ffb59ab 100644
--- a/gitiles-servlet/src/main/java/com/google/gitiles/RevisionParser.java
+++ b/gitiles-servlet/src/main/java/com/google/gitiles/RevisionParser.java
@@ -32,7 +32,6 @@
/** Object to parse revisions out of Gitiles paths. */
class RevisionParser {
- static final Splitter PATH_SPLITTER = Splitter.on('/');
private static final Splitter OPERATOR_SPLITTER = Splitter.on(CharMatcher.anyOf("^~"));
static class Result {
@@ -108,7 +107,7 @@
StringBuilder b = new StringBuilder();
boolean first = true;
- for (String part : PATH_SPLITTER.split(path)) {
+ for (String part : Paths.SPLITTER.split(path)) {
if (part.isEmpty()) {
return null; // No valid revision contains empty segments.
}
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/TreeSoyData.java b/gitiles-servlet/src/main/java/com/google/gitiles/TreeSoyData.java
index 66aa207..4893208 100644
--- a/gitiles-servlet/src/main/java/com/google/gitiles/TreeSoyData.java
+++ b/gitiles-servlet/src/main/java/com/google/gitiles/TreeSoyData.java
@@ -14,15 +14,12 @@
package com.google.gitiles;
-import static com.google.gitiles.RevisionParser.PATH_SPLITTER;
import static org.eclipse.jgit.lib.Constants.OBJ_COMMIT;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Charsets;
-import com.google.common.base.Objects;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
-import com.google.common.io.Files;
import com.google.gitiles.PathServlet.FileType;
import org.eclipse.jgit.errors.MissingObjectException;
@@ -33,7 +30,6 @@
import java.io.IOException;
import java.util.List;
import java.util.Map;
-import java.util.StringTokenizer;
/** Soy data converter for git trees. */
public class TreeSoyData {
@@ -51,29 +47,13 @@
static final int MAX_SYMLINK_SIZE = 16 << 10;
static String resolveTargetUrl(GitilesView view, String target) {
- if (target.startsWith("/")) {
+ String resolved = Paths.simplifyPathUpToRoot(target, view.getTreePath());
+ if (resolved == null) {
return null;
}
-
- // simplifyPath() normalizes "a/../../" to "a", so manually check whether
- // the path leads above the git root.
- String path = Objects.firstNonNull(view.getTreePath(), "");
- int depth = new StringTokenizer(path, "/").countTokens();
- for (String part : PATH_SPLITTER.split(target)) {
- if (part.equals("..")) {
- depth--;
- if (depth < 0) {
- return null;
- }
- } else if (!part.isEmpty() && !part.equals(".")) {
- depth++;
- }
- }
-
- path = Files.simplifyPath(view.getTreePath() + "/" + target);
return GitilesView.path()
.copyFrom(view)
- .setTreePath(!path.equals(".") ? path : "")
+ .setTreePath(resolved)
.toUrl();
}
@@ -130,8 +110,6 @@
String target = new String(
rw.getObjectReader().open(tw.getObjectId(0)).getCachedBytes(),
Charsets.UTF_8);
- // TODO(dborowitz): Merge Shawn's changes before copying these methods
- // in.
entry.put("targetName", getTargetDisplayName(target));
String targetUrl = resolveTargetUrl(view, target);
if (targetUrl != null) {
diff --git a/gitiles-servlet/src/main/resources/com/google/gitiles/templates/PathDetail.soy b/gitiles-servlet/src/main/resources/com/google/gitiles/templates/PathDetail.soy
index 33d38a2..d954268 100644
--- a/gitiles-servlet/src/main/resources/com/google/gitiles/templates/PathDetail.soy
+++ b/gitiles-servlet/src/main/resources/com/google/gitiles/templates/PathDetail.soy
@@ -79,6 +79,10 @@
{template .gitlinkDetail}
<div class="gitlink-detail">
{msg desc="Lead-in text for the git link URL"}Submodule link to {$sha} of{/msg}
- {sp}{if $httpUrl}<a href="{$httpUrl}">{$remoteUrl}</a>{else}{$remoteUrl}{/if}
+ {if $remoteUrl}
+ {sp}{if $httpUrl}<a href="{$httpUrl}">{$remoteUrl}</a>{else}{$remoteUrl}{/if}
+ {else}
+ {sp}{msg desc="Text describing an unknown submodule URL"}a submodule with an unknown URL{/msg}
+ {/if}
</div>
{/template}
diff --git a/gitiles-servlet/src/test/java/com/google/gitiles/PathsTest.java b/gitiles-servlet/src/test/java/com/google/gitiles/PathsTest.java
new file mode 100644
index 0000000..e7cd492
--- /dev/null
+++ b/gitiles-servlet/src/test/java/com/google/gitiles/PathsTest.java
@@ -0,0 +1,45 @@
+// Copyright 2012 Google Inc. All Rights Reserved.
+//
+// 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;
+
+import static com.google.gitiles.Paths.simplifyPathUpToRoot;
+
+import junit.framework.TestCase;
+
+/** Tests for {@link Paths}. */
+public class PathsTest extends TestCase {
+ public void testSimplifyPathUpToRoot() throws Exception {
+ String root = "a/b/c";
+ assertNull(simplifyPathUpToRoot("/foo", root));
+ assertEquals("a", simplifyPathUpToRoot("../../", root));
+ assertEquals("a", simplifyPathUpToRoot(".././../", root));
+ assertEquals("a", simplifyPathUpToRoot("..//../", root));
+ assertEquals("a/d", simplifyPathUpToRoot("../../d", root));
+ assertEquals("", simplifyPathUpToRoot("../../..", root));
+ assertEquals("a/d/e", simplifyPathUpToRoot("../../d/e", root));
+ assertEquals("a/b", simplifyPathUpToRoot("../d/../e/../", root));
+ assertNull(simplifyPathUpToRoot("../../../../", root));
+ assertNull(simplifyPathUpToRoot("../../a/../../..", root));
+ }
+
+ public void testSimplifyPathUpToNullRoot() throws Exception {
+ assertNull(simplifyPathUpToRoot("/foo", null));
+ assertNull(simplifyPathUpToRoot("../", null));
+ assertNull(simplifyPathUpToRoot("../../", null));
+ assertNull(simplifyPathUpToRoot(".././../", null));
+ assertEquals("a/b", simplifyPathUpToRoot("a/b", null));
+ assertEquals("a/c", simplifyPathUpToRoot("a/b/../c", null));
+ }
+}
