Simplify the URI/repoName handling
The methods in the GerritRemoteReader can receive URIs or repoNames
(a host-relative name of the project). The handling of the URIs is
inconsistent: sha1() takes the path before calling openRepository()
while readFileWithMode() passes through the whole URL. This is wrong
e.g. for external URLs. An outter host URL can open the wrong repo in
sha1() and nothing in readFileWithMode().
Make internal methods to open by URI or name. The public methods
must decide what to invoke. This makes clear what is coming and gives a
unique place for the URI -> repoName translation.
This also paves the way to plug a repository opener that supports
cross-host URLs.
Change-Id: Ia2a925764bb9c3305a434240d44873b31c6e0c75
diff --git a/java/com/googlesource/gerrit/plugins/supermanifest/SuperManifestRefUpdatedListener.java b/java/com/googlesource/gerrit/plugins/supermanifest/SuperManifestRefUpdatedListener.java
index c8f7895..11e1233 100644
--- a/java/com/googlesource/gerrit/plugins/supermanifest/SuperManifestRefUpdatedListener.java
+++ b/java/com/googlesource/gerrit/plugins/supermanifest/SuperManifestRefUpdatedListener.java
@@ -55,7 +55,6 @@
import java.util.Map;
import java.util.Set;
import org.eclipse.jgit.api.errors.GitAPIException;
-import org.eclipse.jgit.api.errors.InvalidRemoteException;
import org.eclipse.jgit.api.errors.RefNotFoundException;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
@@ -357,35 +356,18 @@
// GerritRemoteReader is for injecting Gerrit's Git implementation into JGit.
static class GerritRemoteReader implements RepoCommand.RemoteReader, AutoCloseable {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- private final Map<String, Repository> repos;
+ private final Map<Project.NameKey, Repository> repos;
private final GitRepositoryManager repoManager;
- private final URI canonicalWebUrl;
+ private final String canonicalWebUrl;
GerritRemoteReader(GitRepositoryManager repoManager, @CanonicalWebUrl String canonicalWebUrl) {
this.repos = new HashMap<>();
this.repoManager = repoManager;
- try {
- this.canonicalWebUrl = new URI(canonicalWebUrl);
- } catch (URISyntaxException e) {
- throw new IllegalArgumentException(e);
- }
+ this.canonicalWebUrl = canonicalWebUrl;
}
@Override
public ObjectId sha1(String uriStr, String refName) throws GitAPIException {
- URI url;
- try {
- url = new URI(uriStr);
- } catch (URISyntaxException e) {
- // TODO(hanwen): is there a better exception for this?
- throw new InvalidRemoteException(e.getMessage());
- }
-
- String repoName = url.getPath();
- while (repoName.startsWith("/")) {
- repoName = repoName.substring(1);
- }
-
// This is a (mis)feature of JGit, which ignores SHA1s but only if ignoreRemoteFailures
// is set.
if (ObjectId.isId(refName)) {
@@ -393,7 +375,10 @@
}
try {
- Repository repo = openRepository(repoName);
+ // When the remote is fetch="<relative path>" the manifest parser uses a repoName as URI.
+ // Do a poor man's guessing if we have a repoName or URI
+ Repository repo =
+ uriStr.contains("://") ? doOpenByUri(uriStr) : doOpenByName(Project.nameKey(uriStr));
Ref ref = repo.findRef(refName);
if (ref == null || ref.getObjectId() == null) {
logger.atWarning().log(
@@ -405,21 +390,23 @@
ObjectId id = ref.getPeeledObjectId();
return id != null ? id : ref.getObjectId();
} catch (RepositoryNotFoundException e) {
- logger.atWarning().log(
- "%s: failed to open repository %s: %s", canonicalWebUrl, repoName, e);
+ logger.atWarning().log("%s: failed to open repository %s: %s", canonicalWebUrl, uriStr, e);
return null;
} catch (IOException io) {
RefNotFoundException e =
- new RefNotFoundException(String.format("cannot open %s to read %s", repoName, refName));
+ new RefNotFoundException(String.format("cannot open %s to read %s", uriStr, refName));
e.initCause(io);
throw e;
}
}
@Override
- public RemoteFile readFileWithMode(String repoName, String ref, String path)
+ public RemoteFile readFileWithMode(String uriStr, String ref, String path)
throws GitAPIException, IOException {
- Repository repo = openRepository(repoName);
+ // When the remote is fetch="<relative path>" the manifest parser uses a repoName as URI.
+ // Do a poor man's guessing if we have a repoName or URI
+ Repository repo =
+ uriStr.contains("://") ? doOpenByUri(uriStr) : doOpenByName(Project.nameKey(uriStr));
Ref r = repo.findRef(ref);
ObjectId objectId = r == null ? repo.resolve(ref) : r.getObjectId();
if (objectId == null) {
@@ -434,16 +421,38 @@
}
public Repository openRepository(String name) throws IOException {
- name = urlToRepoKey(canonicalWebUrl, name);
+ return doOpenByName(Project.nameKey(name));
+ }
+
+ private Repository doOpenByName(Project.NameKey name) throws IOException {
if (repos.containsKey(name)) {
return repos.get(name);
}
- Repository repo = repoManager.openRepository(Project.nameKey(name));
+ Repository repo = repoManager.openRepository(name);
repos.put(name, repo);
return repo;
}
+ private Repository doOpenByUri(String uriStr) throws IOException {
+ // A URL in this host is <canonicalWebUrl>/<repoName>.
+ if (!uriStr.startsWith(canonicalWebUrl)) {
+ // Cannot open repos in another host.
+ // InvalidRemoteException would stop the whole manifest processing. We rather ignore these
+ // entries.
+ throw new RepositoryNotFoundException(
+ "Manifest refers to repo in different host: " + uriStr);
+ }
+
+ // canonicalWebUrl can contain also a path part
+ String repoName = uriStr.substring(canonicalWebUrl.length());
+ while (repoName.startsWith("/")) {
+ repoName = repoName.substring(1);
+ }
+
+ return doOpenByName(Project.nameKey(repoName));
+ }
+
@Override
public void close() {
for (Repository repo : repos.values()) {
@@ -452,17 +461,4 @@
repos.clear();
}
}
-
- @VisibleForTesting
- static String urlToRepoKey(URI baseUri, String name) {
- if (name.startsWith(baseUri.toString())) {
- // It would be nice to parse the URL and do relativize on the Path, but
- // I am lazy, and nio.Path considers the file system and symlinks.
- name = name.substring(baseUri.toString().length());
- while (name.startsWith("/")) {
- name = name.substring(1);
- }
- }
- return name;
- }
}
diff --git a/javatests/com/googlesource/gerrit/plugins/supermanifest/GerritRemoteReaderTest.java b/javatests/com/googlesource/gerrit/plugins/supermanifest/GerritRemoteReaderTest.java
new file mode 100644
index 0000000..539bdc4
--- /dev/null
+++ b/javatests/com/googlesource/gerrit/plugins/supermanifest/GerritRemoteReaderTest.java
@@ -0,0 +1,114 @@
+package com.googlesource.gerrit.plugins.supermanifest;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.testing.InMemoryRepositoryManager;
+import org.eclipse.jgit.api.errors.GitAPIException;
+import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.eclipse.jgit.gitrepo.RepoCommand;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.junit.Before;
+import org.junit.Test;
+
+public class GerritRemoteReaderTest {
+
+ private static final String MASTER = "refs/heads/master";
+ private static final String CANONICAL_WEB_URL = "https://example.com/gerrit";
+
+ private static final String PROJECT_REPONAME = "project/x";
+ private static final String PROJECT_URL = CANONICAL_WEB_URL + "/" + PROJECT_REPONAME;
+
+ private GitRepositoryManager repoManager = new InMemoryRepositoryManager();
+ private SuperManifestRefUpdatedListener.GerritRemoteReader reader =
+ new SuperManifestRefUpdatedListener.GerritRemoteReader(repoManager, CANONICAL_WEB_URL);
+ private Repository projectX;
+ private RevCommit projectXTip;
+
+ @Before
+ public void setUp() throws Exception {
+ repoManager.createRepository(Project.nameKey(PROJECT_REPONAME));
+ projectX = repoManager.openRepository(Project.nameKey(PROJECT_REPONAME));
+ try (TestRepository<Repository> git = new TestRepository<>(projectX)) {
+ projectXTip = git.branch(MASTER).commit().add("test_file", "test_contents").create();
+ }
+ }
+
+ @Test
+ public void sha1_inCanonical_opens() throws Exception {
+ ObjectId objectId = reader.sha1(PROJECT_URL, "refs/heads/master");
+ assertThat(objectId).isEqualTo(projectXTip);
+ }
+
+ @Test
+ public void sha1_outterHost_null() throws Exception {
+ ObjectId objectId = reader.sha1("https://other-host.com/gerrit/project/x", "refs/heads/master");
+ assertThat(objectId).isNull();
+ }
+
+ @Test
+ public void sha1_repoName_opens() throws GitAPIException {
+ // When remote has relative fetch (e.g. "..") supermanifest returns repoNames as uri
+ ObjectId objectId = reader.sha1(PROJECT_REPONAME, "refs/heads/master");
+ assertThat(objectId).isEqualTo(projectXTip);
+ }
+
+ @Test
+ public void sha1_repoName_nonExisting_null() throws GitAPIException {
+ ObjectId objectId = reader.sha1("unexisting/project", "refs/heads/master");
+ assertThat(objectId).isNull();
+ }
+
+ @Test
+ public void sha1_forObjectId_returnIt() throws Exception {
+ String sha1 = "91f2c8cb366e21c20544f531be710fdfa5eb3afb";
+ ObjectId oid = ObjectId.fromString(sha1);
+ assertThat(reader.sha1("http://this.is.not.read/", sha1)).isEqualTo(oid);
+ }
+
+ @Test
+ public void readWithFileMode_uriInCanonical_reads() throws Exception {
+ RepoCommand.RemoteFile remoteFile =
+ reader.readFileWithMode(PROJECT_URL, "refs/heads/master", "test_file");
+ assertThat(new String(remoteFile.getContents())).isEqualTo("test_contents");
+ }
+
+ @Test
+ public void readWithFileMode_repoName_reads() throws Exception {
+ RepoCommand.RemoteFile remoteFile =
+ reader.readFileWithMode(PROJECT_REPONAME, "refs/heads/master", "test_file");
+ assertThat(new String(remoteFile.getContents())).isEqualTo("test_contents");
+ }
+
+ @Test
+ public void readWithFileMode_outerHostUri_throws() {
+ assertThrows(
+ RepositoryNotFoundException.class,
+ () ->
+ reader.readFileWithMode(
+ "https://somwhere.com/project/x", "refs/heads/master", "test_file"));
+ }
+
+ @Test
+ public void readWithFileMode_repoName_nonexisting_throws() {
+ assertThrows(
+ RepositoryNotFoundException.class,
+ () -> reader.readFileWithMode("unexisting/project", "refs/heads/master", "test_file"));
+ }
+
+ @Test
+ public void openRepository_byRepoName_ok() throws Exception {
+ Repository repo = reader.openRepository(PROJECT_REPONAME);
+ assertThat(repo).isNotNull();
+ }
+
+ @Test
+ public void openRepository_byUri_doesNotOpen() throws Exception {
+ assertThrows(RepositoryNotFoundException.class, () -> reader.openRepository(PROJECT_URL));
+ }
+}
diff --git a/javatests/com/googlesource/gerrit/plugins/supermanifest/RepoSuperManifestIT.java b/javatests/com/googlesource/gerrit/plugins/supermanifest/RepoSuperManifestIT.java
index 6ba1a1f..aad0d4f 100644
--- a/javatests/com/googlesource/gerrit/plugins/supermanifest/RepoSuperManifestIT.java
+++ b/javatests/com/googlesource/gerrit/plugins/supermanifest/RepoSuperManifestIT.java
@@ -544,7 +544,9 @@
+ defaultXml
+ " <project name=\""
+ testRepoKeys[0].get()
- + "\" path=\"path1\" />\n"
+ + "\" path=\"path1\" >\n"
+ + "<copyfile src=\"file0\" dest=\"dest\" />\n"
+ + "</project>"
+ "</manifest>\n";
pushFactory
.create(admin.newIdent(), manifestRepo, "Subject", "default.xml", xml)
@@ -553,6 +555,7 @@
BranchApi branch = gApi.projects().name(superKey.get()).branch("refs/heads/destbranch");
assertThat(branch.file("path1").getContentType()).isEqualTo("x-git/gitlink; charset=UTF-8");
+ assertThat(branch.file("dest").asString()).isEqualTo("file");
Config base = new Config();
String gitmodule = branch.file(".gitmodules").asString();
@@ -571,21 +574,6 @@
}
@Test
- public void testToRepoKey() {
- URI base = URI.create("https://gerrit-review.googlesource.com");
- assertThat(
- SuperManifestRefUpdatedListener.urlToRepoKey(
- base, "https://gerrit-review.googlesource.com/repo"))
- .isEqualTo("repo");
- assertThat(SuperManifestRefUpdatedListener.urlToRepoKey(base, "repo")).isEqualTo("repo");
- assertThat(
- SuperManifestRefUpdatedListener.urlToRepoKey(
- URI.create("https://gerrit-review.googlesource.com/"),
- "https://gerrit-review.googlesource.com/repo"))
- .isEqualTo("repo");
- }
-
- @Test
public void wildcardDestBranchWithExclusion() throws Exception {
setupTestRepos("project");
