Use project names instead of paths for the submodule name

Two submodules at the same path on different branches need not represent
the same repository, and two submodules at different paths can represent
the same one.

The C Git implementation uses the submodule name to internally manage
the submodule repositories under .git/modules. When a submodule
represents different repositories in different branches, it makes a
conflict inside .git/modules.

The current RepoCommand implementation uses submodule paths as the
submodule names. When the manifest file mounts different repositories to
the same path in different branches, this makes a situation described
above. To solve this issue, we can use the project name instead of
the path as the submodule name.

On the other hand, since repo v1.12.8~3^2 (repo: Support multiple
branches for the same project., 2013-10-11), a manifest file can mount
the same project to different paths. If we naively use the project
name as the submodule name, it makes a conflict in .git/modules, too.

This patch uses the project name as the submodule name basically, but
when the same project is mounted to different paths, it uses the project
name and path as the submodule name.

Change-Id: I09dc7d62ba59016fe28852d3139a56ef7ef49b8f
Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
Reported-by: JP Sugarbroad <jpsugar@google.com>

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

@@ -268,7 +268,8 @@ public class RepoCommandTest extends RepositoryTestCase {
 						.getCachedBytes(Integer.MAX_VALUE);
 				Config base = new Config();
 				BlobBasedConfig cfg = new BlobBasedConfig(base, bytes);
-				String subUrl = cfg.getString("submodule", "base", "url");
+				String subUrl = cfg.getString("submodule", "platform/base",
+						"url");
 				assertEquals(subUrl, "../base");
 			}
 		}
@@ -301,7 +302,8 @@ public class RepoCommandTest extends RepositoryTestCase {
 						.getCachedBytes(Integer.MAX_VALUE);
 				Config base = new Config();
 				BlobBasedConfig cfg = new BlobBasedConfig(base, bytes);
-				String subUrl = cfg.getString("submodule", "base", "url");
+				String subUrl = cfg.getString("submodule", "platform/base",
+						"url");
 				assertEquals(subUrl, "https://host.com/platform/base");
 			}
 		}
@@ -387,8 +389,8 @@ public class RepoCommandTest extends RepositoryTestCase {
 								.getCachedBytes(Integer.MAX_VALUE);
 						Config base = new Config();
 						BlobBasedConfig cfg = new BlobBasedConfig(base, bytes);
-						String subUrl = cfg.getString("submodule", "src",
-								"url");
+						String subUrl = cfg.getString("submodule",
+								"chromium/src", "url");
 						assertEquals(
 								"https://chromium.googlesource.com/chromium/src",
 								subUrl);
@@ -443,8 +445,8 @@ public class RepoCommandTest extends RepositoryTestCase {
 								.getCachedBytes(Integer.MAX_VALUE);
 						Config base = new Config();
 						BlobBasedConfig cfg = new BlobBasedConfig(base, bytes);
-						String subUrl = cfg.getString("submodule", "src",
-								"url");
+						String subUrl = cfg.getString("submodule",
+								"chromium/src", "url");
 						assertEquals("../chromium/src", subUrl);
 					}
 					fetchSlash = !fetchSlash;
@@ -613,7 +615,7 @@ public class RepoCommandTest extends RepositoryTestCase {
 				String content = reader.readLine();
 				assertEquals(
 						"The first line of .gitmodules file should be as expected",
-						"[submodule \"foo\"]", content);
+						"[submodule \"" + defaultUri + "\"]", content);
 			}
 			// The gitlink should be the same as remote head sha1
 			String gitlink = localDb.resolve(Constants.HEAD + ":foo").name();
@@ -801,9 +803,9 @@ public class RepoCommandTest extends RepositoryTestCase {
 				.append("<manifest>")
 				.append("<remote name=\"remote1\" fetch=\".\" />")
 				.append("<default revision=\"master\" remote=\"remote1\" />")
-				.append("<project path=\"bar\" name=\"").append(defaultUri)
-				.append("\" revision=\"").append(BRANCH).append("\" >")
-				.append("<copyfile src=\"hello.txt\" dest=\"Hello.txt\" />")
+				.append("<project path=\"bar\" name=\"").append(notDefaultUri)
+				.append("\" >")
+				.append("<copyfile src=\"world.txt\" dest=\"World.txt\" />")
 				.append("</project>").append("</manifest>");
 		JGitTestUtil.writeTrashFile(tempDb, "new.xml", xmlContent.toString());
 		command = new RepoCommand(remoteDb);
@@ -819,8 +821,8 @@ public class RepoCommandTest extends RepositoryTestCase {
 			File hello = new File(localDb.getWorkTree(), "Hello");
 			assertFalse("The Hello file shouldn't exist", hello.exists());
 			// The Hello.txt file should exist
-			File hellotxt = new File(localDb.getWorkTree(), "Hello.txt");
-			assertTrue("The Hello.txt file should exist", hellotxt.exists());
+			File hellotxt = new File(localDb.getWorkTree(), "World.txt");
+			assertTrue("The World.txt file should exist", hellotxt.exists());
 			dotmodules = new File(localDb.getWorkTree(),
 					Constants.DOT_GIT_MODULES);
 		}
@@ -835,9 +837,9 @@ public class RepoCommandTest extends RepositoryTestCase {
 				String line = reader.readLine();
 				if (line == null)
 					break;
-				if (line.contains("submodule \"foo\""))
+				if (line.contains("submodule \"" + defaultUri + "\""))
 					foo = true;
-				if (line.contains("submodule \"bar\""))
+				if (line.contains("submodule \"" + notDefaultUri + "\""))
 					bar = true;
 			}
 			assertTrue("The bar submodule should exist", bar);
@@ -876,9 +878,7 @@ public class RepoCommandTest extends RepositoryTestCase {
 				Constants.DOT_GIT_MODULES);
 		}
 
-		// The .gitmodules file should have 'submodule "foo"' and shouldn't
-		// have
-		// 'submodule "foo/bar"' lines.
+		// Check .gitmodules file
 		try (BufferedReader reader = new BufferedReader(
 				new FileReader(dotmodules))) {
 			boolean foo = false;
@@ -888,16 +888,17 @@ public class RepoCommandTest extends RepositoryTestCase {
 				String line = reader.readLine();
 				if (line == null)
 					break;
-				if (line.contains("submodule \"foo\""))
+				if (line.contains("submodule \"" + defaultUri + "\""))
 					foo = true;
-				if (line.contains("submodule \"foo/bar\""))
+				if (line.contains("submodule \"" + groupBUri + "\""))
 					foobar = true;
-				if (line.contains("submodule \"a\""))
+				if (line.contains("submodule \"" + groupAUri + "\""))
 					a = true;
 			}
-			assertTrue("The foo submodule should exist", foo);
-			assertFalse("The foo/bar submodule shouldn't exist", foobar);
-			assertTrue("The a submodule should exist", a);
+			assertTrue("The " + defaultUri + " submodule should exist", foo);
+			assertFalse("The " + groupBUri + " submodule shouldn't exist",
+					foobar);
+			assertTrue("The " + groupAUri + " submodule should exist", a);
 		}
 	}
 
@@ -1033,11 +1034,11 @@ public class RepoCommandTest extends RepositoryTestCase {
 			assertEquals(
 					"Recording remote branches should work for short branch descriptions",
 					"master",
-					c.getString("submodule", "with-branch", "branch"));
+					c.getString("submodule", notDefaultUri, "branch"));
 			assertEquals(
 					"Recording remote branches should work for full ref specs",
 					"refs/heads/master",
-					c.getString("submodule", "with-long-branch", "branch"));
+					c.getString("submodule", defaultUri, "branch"));
 		}
 	}
 
@@ -1095,7 +1096,7 @@ public class RepoCommandTest extends RepositoryTestCase {
 				.append("<project path=\"shallow-please\" ").append("name=\"")
 				.append(defaultUri).append("\" ").append("clone-depth=\"1\" />")
 				.append("<project path=\"non-shallow\" ").append("name=\"")
-				.append(defaultUri).append("\" />").append("</manifest>");
+				.append(notDefaultUri).append("\" />").append("</manifest>");
 		JGitTestUtil.writeTrashFile(tempDb, "manifest.xml",
 				xmlContent.toString());
 
@@ -1115,9 +1116,9 @@ public class RepoCommandTest extends RepositoryTestCase {
 			FileBasedConfig c = new FileBasedConfig(gitmodules, FS.DETECTED);
 			c.load();
 			assertEquals("Recording shallow configuration should work", "true",
-					c.getString("submodule", "shallow-please", "shallow"));
+					c.getString("submodule", defaultUri, "shallow"));
 			assertNull("Recording non shallow configuration should work",
-					c.getString("submodule", "non-shallow", "shallow"));
+					c.getString("submodule", notDefaultUri, "shallow"));
 		}
 	}
 
@@ -1176,6 +1177,43 @@ public class RepoCommandTest extends RepositoryTestCase {
 		}
 	}
 
+	@Test
+	public void testTwoPathUseTheSameName() throws Exception {
+		Repository remoteDb = createBareRepository();
+		Repository tempDb = createWorkRepository();
+
+		StringBuilder xmlContent = new StringBuilder();
+		xmlContent.append("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n")
+				.append("<manifest>")
+				.append("<remote name=\"remote1\" fetch=\".\" />")
+				.append("<default revision=\"master\" remote=\"remote1\" />")
+				.append("<project path=\"path1\" ").append("name=\"")
+				.append(defaultUri).append("\" />")
+				.append("<project path=\"path2\" ").append("name=\"")
+				.append(defaultUri).append("\" />").append("</manifest>");
+		JGitTestUtil.writeTrashFile(tempDb, "manifest.xml",
+				xmlContent.toString());
+
+		RepoCommand command = new RepoCommand(remoteDb);
+		command.setPath(
+				tempDb.getWorkTree().getAbsolutePath() + "/manifest.xml")
+				.setURI(rootUri).setRecommendShallow(true).call();
+		File directory = createTempDirectory("testBareRepo");
+		try (Repository localDb = Git.cloneRepository().setDirectory(directory)
+				.setURI(remoteDb.getDirectory().toURI().toString()).call()
+				.getRepository();) {
+			File gitmodules = new File(localDb.getWorkTree(), ".gitmodules");
+			assertTrue("The .gitmodules file should exist",
+					gitmodules.exists());
+			FileBasedConfig c = new FileBasedConfig(gitmodules, FS.DETECTED);
+			c.load();
+			assertEquals("A module should exist for path1", "path1",
+					c.getString("submodule", defaultUri + "/path1", "path"));
+			assertEquals("A module should exist for path2", "path2",
+					c.getString("submodule", defaultUri + "/path2", "path"));
+		}
+	}
+
 	private void resolveRelativeUris() {
 		// Find the longest common prefix ends with "/" as rootUri.
 		defaultUri = defaultDb.getDirectory().toURI().toString();

org.eclipse.jgit/src/org/eclipse/jgit/gitrepo/RepoCommand.java

@@ -52,10 +52,10 @@ import java.io.InputStream;
 import java.net.URI;
 import java.text.MessageFormat;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
-import java.util.Set;
 import java.util.StringJoiner;
 
 import org.eclipse.jgit.annotations.Nullable;
@@ -124,7 +124,6 @@ public class RepoCommand extends GitCommand<RevCommit> {
 	private IncludedFileReader includedReader;
 	private boolean ignoreRemoteFailures = false;
 
-	private List<RepoProject> bareProjects;
 	private ProgressMonitor monitor;
 
 	/**
@@ -519,37 +518,33 @@ public class RepoCommand extends GitCommand<RevCommit> {
 		}
 
 		if (repo.isBare()) {
-			bareProjects = new ArrayList<>();
 			if (author == null)
 				author = new PersonIdent(repo);
 			if (callback == null)
 				callback = new DefaultRemoteReader();
-			for (RepoProject proj : filteredProjects) {
-				addSubmoduleBare(proj.getUrl(), proj.getPath(),
-						proj.getRevision(), proj.getCopyFiles(),
-						proj.getLinkFiles(), proj.getGroups(),
-						proj.getRecommendShallow());
-			}
+			List<RepoProject> renamedProjects = renameProjects(filteredProjects);
+
 			DirCache index = DirCache.newInCore();
 			DirCacheBuilder builder = index.builder();
 			ObjectInserter inserter = repo.newObjectInserter();
 			try (RevWalk rw = new RevWalk(repo)) {
 				Config cfg = new Config();
 				StringBuilder attributes = new StringBuilder();
-				for (RepoProject proj : bareProjects) {
+				for (RepoProject proj : renamedProjects) {
+					String name = proj.getName();
 					String path = proj.getPath();
-					String nameUri = proj.getName();
+					String url = proj.getUrl();
 					ObjectId objectId;
 					if (ObjectId.isId(proj.getRevision())) {
 						objectId = ObjectId.fromString(proj.getRevision());
 					} else {
-						objectId = callback.sha1(nameUri, proj.getRevision());
+						objectId = callback.sha1(url, proj.getRevision());
 						if (objectId == null && !ignoreRemoteFailures) {
-							throw new RemoteUnavailableException(nameUri);
+							throw new RemoteUnavailableException(url);
 						}
 						if (recordRemoteBranch) {
 							// can be branch or tag
-							cfg.setString("submodule", path, "branch", //$NON-NLS-1$ //$NON-NLS-2$
+							cfg.setString("submodule", name, "branch", //$NON-NLS-1$ //$NON-NLS-2$
 									proj.getRevision());
 						}
 
@@ -559,7 +554,7 @@ public class RepoCommand extends GitCommand<RevCommit> {
 							// depth in the 'clone-depth' field, while
 							// git core only uses a binary 'shallow = true/false'
 							// hint, we'll map any depth to 'shallow = true'
-							cfg.setBoolean("submodule", path, "shallow", //$NON-NLS-1$ //$NON-NLS-2$
+							cfg.setBoolean("submodule", name, "shallow", //$NON-NLS-1$ //$NON-NLS-2$
 									true);
 						}
 					}
@@ -575,12 +570,13 @@ public class RepoCommand extends GitCommand<RevCommit> {
 						attributes.append(rec.toString());
 					}
 
-					URI submodUrl = URI.create(nameUri);
+					URI submodUrl = URI.create(url);
 					if (targetUri != null) {
 						submodUrl = relativize(targetUri, submodUrl);
 					}
-					cfg.setString("submodule", path, "path", path); //$NON-NLS-1$ //$NON-NLS-2$
-					cfg.setString("submodule", path, "url", submodUrl.toString()); //$NON-NLS-1$ //$NON-NLS-2$
+					cfg.setString("submodule", name, "path", path); //$NON-NLS-1$ //$NON-NLS-2$
+					cfg.setString("submodule", name, "url", //$NON-NLS-1$ //$NON-NLS-2$
+							submodUrl.toString());
 
 					// create gitlink
 					if (objectId != null) {
@@ -591,7 +587,7 @@ public class RepoCommand extends GitCommand<RevCommit> {
 
 						for (CopyFile copyfile : proj.getCopyFiles()) {
 							byte[] src = callback.readFile(
-								nameUri, proj.getRevision(), copyfile.src);
+								url, proj.getRevision(), copyfile.src);
 							objectId = inserter.insert(Constants.OBJ_BLOB, src);
 							dcEntry = new DirCacheEntry(copyfile.dest);
 							dcEntry.setObjectId(objectId);
@@ -691,7 +687,7 @@ public class RepoCommand extends GitCommand<RevCommit> {
 		} else {
 			try (Git git = new Git(repo)) {
 				for (RepoProject proj : filteredProjects) {
-					addSubmodule(proj.getUrl(), proj.getPath(),
+					addSubmodule(proj.getName(), proj.getUrl(), proj.getPath(),
 							proj.getRevision(), proj.getCopyFiles(),
 							proj.getLinkFiles(), git);
 				}
@@ -703,9 +699,9 @@ public class RepoCommand extends GitCommand<RevCommit> {
 		}
 	}
 
-	private void addSubmodule(String url, String path, String revision,
-			List<CopyFile> copyfiles, List<LinkFile> linkfiles, Git git)
-			throws GitAPIException, IOException {
+	private void addSubmodule(String name, String url, String path,
+			String revision, List<CopyFile> copyfiles, List<LinkFile> linkfiles,
+			Git git) throws GitAPIException, IOException {
 		assert (!repo.isBare());
 		assert (git != null);
 		if (!linkfiles.isEmpty()) {
@@ -713,7 +709,8 @@ public class RepoCommand extends GitCommand<RevCommit> {
 					JGitText.get().nonBareLinkFilesNotSupported);
 		}
 
-		SubmoduleAddCommand add = git.submoduleAdd().setPath(path).setURI(url);
+		SubmoduleAddCommand add = git.submoduleAdd().setName(name).setPath(path)
+				.setURI(url);
 		if (monitor != null)
 			add.setProgressMonitor(monitor);
 
@@ -731,16 +728,42 @@ public class RepoCommand extends GitCommand<RevCommit> {
 		}
 	}
 
-	private void addSubmoduleBare(String url, String path, String revision,
-			List<CopyFile> copyfiles, List<LinkFile> linkfiles,
-			Set<String> groups, String recommendShallow) {
-		assert (repo.isBare());
-		assert (bareProjects != null);
-		RepoProject proj = new RepoProject(url, path, revision, null, groups,
-				recommendShallow);
-		proj.addCopyFiles(copyfiles);
-		proj.addLinkFiles(linkfiles);
-		bareProjects.add(proj);
+	/**
+	 * Rename the projects if there's a conflict when converted to submodules.
+	 *
+	 * @param projects
+	 *            parsed projects
+	 * @return projects that are renamed if necessary
+	 */
+	private List<RepoProject> renameProjects(List<RepoProject> projects) {
+		Map<String, List<RepoProject>> m = new HashMap<>();
+		for (RepoProject proj : projects) {
+			List<RepoProject> l = m.get(proj.getName());
+			if (l == null) {
+				l = new ArrayList<>();
+				m.put(proj.getName(), l);
+			}
+			l.add(proj);
+		}
+
+		List<RepoProject> ret = new ArrayList<>();
+		for (List<RepoProject> ps : m.values()) {
+			boolean nameConflict = ps.size() != 1;
+			for (RepoProject proj : ps) {
+				String name = proj.getName();
+				if (nameConflict) {
+					name += SLASH + proj.getPath();
+				}
+				RepoProject p = new RepoProject(name,
+						proj.getPath(), proj.getRevision(), null,
+						proj.getGroups(), proj.getRecommendShallow());
+				p.setUrl(proj.getUrl());
+				p.addCopyFiles(proj.getCopyFiles());
+				p.addLinkFiles(proj.getLinkFiles());
+				ret.add(p);
+			}
+		}
+		return ret;
 	}
 
 	/*