From 84d855cda7fc3f061778ea54b4f7ea569a75d7c3 Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Thu, 6 Apr 2017 13:44:10 +0900 Subject: [PATCH 1/2] ManifestParser: Throw exception if remote does not have fetch attribute In the repo manifest documentation [1] the fetch attribute is marked as "#REQUIRED". If the fetch attribute is not specified, this would previously result in NullPointerException. Throw a SAXException instead. [1] https://gerrit.googlesource.com/git-repo/+/master/docs/manifest-format.txt Change-Id: Ib8ed8cee6074fe6bf8f9ac6fc7a1664a547d2d49 Signed-off-by: Han-Wen Nienhuys Signed-off-by: David Pursehouse --- .../jgit/gitrepo/ManifestParserTest.java | 33 +++++++++++++++++++ .../jgit/gitrepo/internal/RepoText.properties | 1 + .../eclipse/jgit/gitrepo/ManifestParser.java | 7 +++- .../jgit/gitrepo/internal/RepoText.java | 1 + 4 files changed, 41 insertions(+), 1 deletion(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/ManifestParserTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/ManifestParserTest.java index c97b31880..4cd89e0aa 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/ManifestParserTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/ManifestParserTest.java @@ -44,12 +44,15 @@ package org.eclipse.jgit.gitrepo; import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import java.io.ByteArrayInputStream; +import java.io.IOException; import java.util.HashSet; import java.util.Set; import org.junit.Test; +import org.xml.sax.SAXException; public class ManifestParserTest { @@ -110,4 +113,34 @@ public class ManifestParserTest { "Filtered projects shouldn't contain any unexpected results", results.isEmpty()); } + + @Test + public void testManifestParserWithMissingFetchOnRemote() throws Exception { + String baseUrl = "https://git.google.com/"; + StringBuilder xmlContent = new StringBuilder(); + xmlContent.append("\n") + .append("") + .append("") + .append("") + .append("") + .append("") + .append("") + .append("").append(""); + + ManifestParser parser = new ManifestParser(null, null, "master", + baseUrl, null, null); + try { + parser.read(new ByteArrayInputStream( + xmlContent.toString().getBytes(UTF_8))); + fail("ManifestParser did not throw exception for missing fetch"); + } catch (IOException e) { + assertTrue(e.getCause() instanceof SAXException); + assertTrue(e.getCause().getMessage() + .contains("is missing fetch attribute")); + } + } } diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/gitrepo/internal/RepoText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/gitrepo/internal/RepoText.properties index 7443ad32f..e942038a3 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/gitrepo/internal/RepoText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/gitrepo/internal/RepoText.properties @@ -3,6 +3,7 @@ errorIncludeFile=Error: unable to read include file {0} errorIncludeNotImplemented=Error: tag not supported as no callback defined. errorNoDefault=Error: no default remote in manifest file. errorNoDefaultFilename=Error: no default remote in manifest file {0}. +errorNoFetch=Error: remote {0} is missing fetch attribute. errorParsingManifestFile=Error occurred during parsing manifest file. errorRemoteUnavailable=Error remote {0} is unavailable. invalidManifest=Invalid manifest. diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/ManifestParser.java b/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/ManifestParser.java index 26210ecbb..73b2e6dcf 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/ManifestParser.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/ManifestParser.java @@ -289,7 +289,12 @@ public class ManifestParser extends DefaultHandler { } String remoteUrl = remoteUrls.get(remote); if (remoteUrl == null) { - remoteUrl = baseUrl.resolve(remotes.get(remote).fetch).toString(); + String fetch = remotes.get(remote).fetch; + if (fetch == null) { + throw new SAXException(MessageFormat + .format(RepoText.get().errorNoFetch, remote)); + } + remoteUrl = baseUrl.resolve(fetch).toString(); if (!remoteUrl.endsWith("/")) //$NON-NLS-1$ remoteUrl = remoteUrl + "/"; //$NON-NLS-1$ remoteUrls.put(remote, remoteUrl); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/internal/RepoText.java b/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/internal/RepoText.java index 36b6e3aac..02a2565bd 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/internal/RepoText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/internal/RepoText.java @@ -64,6 +64,7 @@ public class RepoText extends TranslationBundle { /***/ public String errorIncludeNotImplemented; /***/ public String errorNoDefault; /***/ public String errorNoDefaultFilename; + /***/ public String errorNoFetch; /***/ public String errorParsingManifestFile; /***/ public String errorRemoteUnavailable; /***/ public String invalidManifest; From f17ec3928c45d7f5170e92b4e0e1f7390de5fff2 Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Tue, 28 Mar 2017 14:00:38 +0200 Subject: [PATCH 2/2] Cleanup and test trailing slash handling in ManifestParser This is a workaround for https://bugs.openjdk.java.net/browse/JDK-4666701. Change-Id: Idd04657e8d95a841d72230f8881b6b899daadbc2 Signed-off-by: Han-Wen Nienhuys Signed-off-by: David Pursehouse Signed-off-by: Matthias Sohn --- .../jgit/gitrepo/ManifestParserTest.java | 16 +++ .../eclipse/jgit/gitrepo/RepoCommandTest.java | 115 ++++++++++++++++++ .../eclipse/jgit/gitrepo/ManifestParser.java | 37 ++++-- 3 files changed, 155 insertions(+), 13 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/ManifestParserTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/ManifestParserTest.java index 4cd89e0aa..c9673a688 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/ManifestParserTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/ManifestParserTest.java @@ -48,6 +48,7 @@ import static org.junit.Assert.fail; import java.io.ByteArrayInputStream; import java.io.IOException; +import java.net.URI; import java.util.HashSet; import java.util.Set; @@ -143,4 +144,19 @@ public class ManifestParserTest { .contains("is missing fetch attribute")); } } + + void testNormalize(String in, String want) { + URI got = ManifestParser.normalizeEmptyPath(URI.create(in)); + if (!got.toString().equals(want)) { + fail(String.format("normalize(%s) = %s want %s", in, got, want)); + } + } + + @Test + public void testNormalizeEmptyPath() { + testNormalize("http://a.b", "http://a.b/"); + testNormalize("http://a.b/", "http://a.b/"); + testNormalize("", ""); + testNormalize("a/b", "a/b"); + } } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/RepoCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/RepoCommandTest.java index 03ed82443..9cf4569d6 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/RepoCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/RepoCommandTest.java @@ -48,15 +48,28 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import java.io.BufferedReader; +import java.io.ByteArrayInputStream; import java.io.File; import java.io.FileReader; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.HashMap; +import java.util.Map; import org.eclipse.jgit.api.Git; +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.junit.JGitTestUtil; import org.eclipse.jgit.junit.RepositoryTestCase; +import org.eclipse.jgit.lib.BlobBasedConfig; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectReader; +import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.storage.file.FileBasedConfig; import org.eclipse.jgit.util.FS; import org.junit.Test; @@ -124,6 +137,108 @@ public class RepoCommandTest extends RepositoryTestCase { resolveRelativeUris(); } + class IndexedRepos implements RepoCommand.RemoteReader { + Map uriRepoMap; + IndexedRepos() { + uriRepoMap = new HashMap<>(); + } + + void put(String u, Repository r) { + uriRepoMap.put(u, r); + } + + @Override + public ObjectId sha1(String uri, String refname) throws GitAPIException { + if (!uriRepoMap.containsKey(uri)) { + return null; + } + + Repository r = uriRepoMap.get(uri); + try { + Ref ref = r.findRef(refname); + if (ref == null) return null; + + ref = r.peel(ref); + ObjectId id = ref.getObjectId(); + return id; + } catch (IOException e) { + throw new InvalidRemoteException("", e); + } + } + + @Override + public byte[] readFile(String uri, String refName, String path) + throws GitAPIException, IOException { + Repository repo = uriRepoMap.get(uri); + + String idStr = refName + ":" + path; + ObjectId id = repo.resolve(idStr); + if (id == null) { + throw new RefNotFoundException( + String.format("repo %s does not have %s", repo.toString(), idStr)); + } + try (ObjectReader reader = repo.newObjectReader()) { + return reader.open(id).getCachedBytes(Integer.MAX_VALUE); + } + } + } + + @Test + public void absoluteRemoteURL() throws Exception { + Repository child = + Git.cloneRepository().setURI(groupADb.getDirectory().toURI().toString()) + .setDirectory(createUniqueTestGitDir(true)) + .setBare(true).call().getRepository(); + Repository dest = Git.cloneRepository() + .setURI(db.getDirectory().toURI().toString()).setDirectory(createUniqueTestGitDir(true)) + .setBare(true).call().getRepository(); + String abs = "https://chromium.googlesource.com"; + String repoUrl = "https://chromium.googlesource.com/chromium/src"; + boolean fetchSlash = false; + boolean baseSlash = false; + do { + do { + String fetchUrl = fetchSlash ? abs + "/" : abs; + String baseUrl = baseSlash ? abs + "/" : abs; + + StringBuilder xmlContent = new StringBuilder(); + xmlContent.append("\n") + .append("") + .append("") + .append("") + .append("") + .append(""); + RepoCommand cmd = new RepoCommand(dest); + + IndexedRepos repos = new IndexedRepos(); + repos.put(repoUrl, child); + + RevCommit commit = cmd + .setInputStream(new ByteArrayInputStream(xmlContent.toString().getBytes(StandardCharsets.UTF_8))) + .setRemoteReader(repos) + .setURI(baseUrl) + .setRecordRemoteBranch(true) + .setRecordSubmoduleLabels(true) + .call(); + + String idStr = commit.getId().name() + ":" + ".gitmodules"; + ObjectId modId = dest.resolve(idStr); + + try (ObjectReader reader = dest.newObjectReader()) { + byte[] bytes = reader.open(modId).getCachedBytes(Integer.MAX_VALUE); + Config base = new Config(); + BlobBasedConfig cfg = new BlobBasedConfig(base, bytes); + String subUrl = cfg.getString("submodule", "src", "url"); + assertEquals("https://chromium.googlesource.com/chromium/src", subUrl); + } + fetchSlash = !fetchSlash; + } while (fetchSlash); + baseSlash = !baseSlash; + } while (baseSlash); + child.close(); + dest.close(); + } + @Test public void testAddRepoManifest() throws Exception { StringBuilder xmlContent = new StringBuilder(); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/ManifestParser.java b/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/ManifestParser.java index 73b2e6dcf..94c8e437c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/ManifestParser.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/ManifestParser.java @@ -46,6 +46,7 @@ import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; import java.net.URI; +import java.net.URISyntaxException; import java.text.MessageFormat; import java.util.ArrayList; import java.util.Collections; @@ -124,12 +125,7 @@ public class ManifestParser extends DefaultHandler { this.filename = filename; this.defaultBranch = defaultBranch; this.rootRepo = rootRepo; - - // Strip trailing '/' to match repo behavior. - while (baseUrl.endsWith("/")) { //$NON-NLS-1$ - baseUrl = baseUrl.substring(0, baseUrl.length()-1); - } - this.baseUrl = URI.create(baseUrl); + this.baseUrl = normalizeEmptyPath(URI.create(baseUrl)); plusGroups = new HashSet<>(); minusGroups = new HashSet<>(); @@ -257,7 +253,7 @@ public class ManifestParser extends DefaultHandler { return; // Only do the following after we finished reading everything. - Map remoteUrls = new HashMap<>(); + Map remoteUrls = new HashMap<>(); if (defaultRevision == null && defaultRemote != null) { Remote remote = remotes.get(defaultRemote); if (remote != null) { @@ -287,20 +283,18 @@ public class ManifestParser extends DefaultHandler { revision = r.revision; } } - String remoteUrl = remoteUrls.get(remote); + URI remoteUrl = remoteUrls.get(remote); if (remoteUrl == null) { String fetch = remotes.get(remote).fetch; if (fetch == null) { throw new SAXException(MessageFormat .format(RepoText.get().errorNoFetch, remote)); } - remoteUrl = baseUrl.resolve(fetch).toString(); - if (!remoteUrl.endsWith("/")) //$NON-NLS-1$ - remoteUrl = remoteUrl + "/"; //$NON-NLS-1$ + remoteUrl = normalizeEmptyPath(baseUrl.resolve(fetch)); remoteUrls.put(remote, remoteUrl); } - proj.setUrl(remoteUrl + proj.getName()) - .setDefaultRevision(revision); + proj.setUrl(remoteUrl.resolve(proj.getName()).toString()) + .setDefaultRevision(revision); } filteredProjects.addAll(projects); @@ -308,6 +302,23 @@ public class ManifestParser extends DefaultHandler { removeOverlaps(); } + static URI normalizeEmptyPath(URI u) { + // URI.create("scheme://host").resolve("a/b") => "scheme://hosta/b" + // That seems like bug https://bugs.openjdk.java.net/browse/JDK-4666701. + // We workaround this by special casing the empty path case. + if (u.getHost() != null && !u.getHost().isEmpty() && + (u.getPath() == null || u.getPath().isEmpty())) { + try { + return new URI(u.getScheme(), + u.getUserInfo(), u.getHost(), u.getPort(), + "/", u.getQuery(), u.getFragment()); //$NON-NLS-1$ + } catch (URISyntaxException x) { + throw new IllegalArgumentException(x.getMessage(), x); + } + } + return u; + } + /** * Getter for projects. *