Merge branch 'stable-4.7'

* stable-4.7:
  Cleanup and test trailing slash handling in ManifestParser
  ManifestParser: Throw exception if remote does not have fetch attribute

Change-Id: Ia9dc3110bcbdae05175851ce647ffd11c542f4c0
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/gitrepo/ManifestParserTest.java

@@ -44,12 +44,16 @@ 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.net.URI;
 import java.util.HashSet;
 import java.util.Set;
 
 import org.junit.Test;
+import org.xml.sax.SAXException;
 
 public class ManifestParserTest {
 
@@ -110,4 +114,49 @@ 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("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n")
+				.append("<manifest>")
+				.append("<remote name=\"remote1\" />")
+				.append("<default revision=\"master\" remote=\"remote1\" />")
+				.append("<project path=\"foo\" name=\"").append("foo")
+				.append("\" groups=\"a,test\" />")
+				.append("<project path=\"bar\" name=\"").append("bar")
+				.append("\" groups=\"notdefault\" />")
+				.append("<project path=\"foo/a\" name=\"").append("a")
+				.append("\" groups=\"a\" />")
+				.append("<project path=\"b\" name=\"").append("b")
+				.append("\" groups=\"b\" />").append("</manifest>");
+
+		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"));
+		}
+	}
+
+	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");
+	}
 }

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<String, Repository> 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("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n")
+					.append("<manifest>")
+					.append("<remote name=\"origin\" fetch=\"" + fetchUrl + "\" />")
+					.append("<default revision=\"master\" remote=\"origin\" />")
+					.append("<project path=\"src\" name=\"chromium/src\" />")
+					.append("</manifest>");
+				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();

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: <include> 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.

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<String, String> remoteUrls = new HashMap<>();
+		Map<String, URI> remoteUrls = new HashMap<>();
 		if (defaultRevision == null && defaultRemote != null) {
 			Remote remote = remotes.get(defaultRemote);
 			if (remote != null) {
@@ -287,14 +283,17 @@ public class ManifestParser extends DefaultHandler {
 					revision = r.revision;
 				}
 			}
-			String remoteUrl = remoteUrls.get(remote);
+			URI remoteUrl = remoteUrls.get(remote);
 			if (remoteUrl == null) {
-				remoteUrl = baseUrl.resolve(remotes.get(remote).fetch).toString();
-				if (!remoteUrl.endsWith("/")) //$NON-NLS-1$
-					remoteUrl = remoteUrl + "/"; //$NON-NLS-1$
+				String fetch = remotes.get(remote).fetch;
+				if (fetch == null) {
+					throw new SAXException(MessageFormat
+							.format(RepoText.get().errorNoFetch, remote));
+				}
+				remoteUrl = normalizeEmptyPath(baseUrl.resolve(fetch));
 				remoteUrls.put(remote, remoteUrl);
 			}
-			proj.setUrl(remoteUrl + proj.getName())
+			proj.setUrl(remoteUrl.resolve(proj.getName()).toString())
 				.setDefaultRevision(revision);
 		}
 
@@ -303,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.
 	 *

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;