Don't assume name = path in .gitmodules

While parsing .gitmodules, the name of the submodule subsection is
purely arbitrary: it frequently is the path of the submodule, but
there's no requirement for it to be. By building a map of paths to
the section name in .gitmodules, we can more accurately return
the submodule URL.

Bug: 508801
Change-Id: I8399ccada1834d4cc5d023344b97dcf8d5869b16
Also-by: Doug Kelly <dougk.ff7@gmail.com>
Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/submodule/SubmoduleWalkTest.java

@@ -444,4 +444,44 @@ public class SubmoduleWalkTest extends RepositoryTestCase {
 		assertNull(gen.getRepository());
 		assertFalse(gen.next());
 	}
+
+	@Test
+	public void testTreeIteratorWithGitmodulesNameNotPath() throws Exception {
+		final ObjectId subId = ObjectId
+				.fromString("abcd1234abcd1234abcd1234abcd1234abcd1234");
+		final String path = "sub";
+		final String arbitraryName = "x";
+
+		final Config gitmodules = new Config();
+		gitmodules.setString(CONFIG_SUBMODULE_SECTION, arbitraryName,
+				CONFIG_KEY_PATH, "sub");
+		gitmodules.setString(CONFIG_SUBMODULE_SECTION, arbitraryName,
+				CONFIG_KEY_URL, "git://example.com/sub");
+
+		RevCommit commit = testDb.getRevWalk()
+				.parseCommit(testDb.commit().noParents()
+						.add(DOT_GIT_MODULES, gitmodules.toText())
+						.edit(new PathEdit(path) {
+
+							@Override
+							public void apply(DirCacheEntry ent) {
+								ent.setFileMode(FileMode.GITLINK);
+								ent.setObjectId(subId);
+							}
+						}).create());
+
+		final CanonicalTreeParser p = new CanonicalTreeParser();
+		p.reset(testDb.getRevWalk().getObjectReader(), commit.getTree());
+		SubmoduleWalk gen = SubmoduleWalk.forPath(db, p, "sub");
+		assertEquals(path, gen.getPath());
+		assertEquals(subId, gen.getObjectId());
+		assertEquals(new File(db.getWorkTree(), path), gen.getDirectory());
+		assertNull(gen.getConfigUpdate());
+		assertNull(gen.getConfigUrl());
+		assertEquals("sub", gen.getModulesPath());
+		assertNull(gen.getModulesUpdate());
+		assertEquals("git://example.com/sub", gen.getModulesUrl());
+		assertNull(gen.getRepository());
+		assertFalse(gen.next());
+	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/submodule/SubmoduleWalk.java

@@ -46,6 +46,8 @@ import java.io.File;
 import java.io.IOException;
 import java.text.MessageFormat;
 import java.util.Locale;
+import java.util.HashMap;
+import java.util.Map;
 
 import org.eclipse.jgit.dircache.DirCache;
 import org.eclipse.jgit.dircache.DirCacheIterator;
@@ -330,6 +332,8 @@ public class SubmoduleWalk implements AutoCloseable {
 
 	private String path;
 
+	private Map<String, String> pathToName;
+
 	/**
 	 * Create submodule generator
 	 *
@@ -355,6 +359,7 @@ public class SubmoduleWalk implements AutoCloseable {
 	 */
 	public SubmoduleWalk setModulesConfig(final Config config) {
 		modulesConfig = config;
+		loadPathNames();
 		return this;
 	}
 
@@ -374,6 +379,7 @@ public class SubmoduleWalk implements AutoCloseable {
 	public SubmoduleWalk setRootTree(final AbstractTreeIterator tree) {
 		rootTree = tree;
 		modulesConfig = null;
+		pathToName = null;
 		return this;
 	}
 
@@ -396,6 +402,7 @@ public class SubmoduleWalk implements AutoCloseable {
 		p.reset(walk.getObjectReader(), id);
 		rootTree = p;
 		modulesConfig = null;
+		pathToName = null;
 		return this;
 	}
 
@@ -419,6 +426,7 @@ public class SubmoduleWalk implements AutoCloseable {
 					repository.getFS());
 			config.load();
 			modulesConfig = config;
+			loadPathNames();
 		} else {
 			try (TreeWalk configWalk = new TreeWalk(repository)) {
 				configWalk.addTree(rootTree);
@@ -438,10 +446,12 @@ public class SubmoduleWalk implements AutoCloseable {
 						if (filter.isDone(configWalk)) {
 							modulesConfig = new BlobBasedConfig(null, repository,
 									configWalk.getObjectId(0));
+							loadPathNames();
 							return this;
 						}
 					}
 					modulesConfig = new Config();
+					pathToName = null;
 				} finally {
 					if (idx > 0)
 						rootTree.next(idx);
@@ -451,6 +461,20 @@ public class SubmoduleWalk implements AutoCloseable {
 		return this;
 	}
 
+	private void loadPathNames() {
+		pathToName = null;
+		if (modulesConfig != null) {
+			HashMap<String, String> pathNames = new HashMap<>();
+			for (String name : modulesConfig
+					.getSubsections(ConfigConstants.CONFIG_SUBMODULE_SECTION)) {
+				pathNames.put(modulesConfig.getString(
+						ConfigConstants.CONFIG_SUBMODULE_SECTION, name,
+						ConfigConstants.CONFIG_KEY_PATH), name);
+			}
+			pathToName = pathNames;
+		}
+	}
+
 	/**
 	 * Checks whether the working tree contains a .gitmodules file. That's a
 	 * hint that the repo contains submodules.
@@ -475,8 +499,14 @@ public class SubmoduleWalk implements AutoCloseable {
 	}
 
 	private void lazyLoadModulesConfig() throws IOException, ConfigInvalidException {
-		if (modulesConfig == null)
+		if (modulesConfig == null) {
 			loadModulesConfig();
+		}
+	}
+
+	private String getModuleName(String modulePath) {
+		String name = pathToName != null ? pathToName.get(modulePath) : null;
+		return name != null ? name : modulePath;
 	}
 
 	/**
@@ -525,6 +555,7 @@ public class SubmoduleWalk implements AutoCloseable {
 	public SubmoduleWalk reset() {
 		repoConfig = repository.getConfig();
 		modulesConfig = null;
+		pathToName = null;
 		walk.reset();
 		return this;
 	}
@@ -586,9 +617,8 @@ public class SubmoduleWalk implements AutoCloseable {
 	 */
 	public String getModulesPath() throws IOException, ConfigInvalidException {
 		lazyLoadModulesConfig();
-		return modulesConfig.getString(
+		return modulesConfig.getString(ConfigConstants.CONFIG_SUBMODULE_SECTION,
-				ConfigConstants.CONFIG_SUBMODULE_SECTION, path,
+				getModuleName(path), ConfigConstants.CONFIG_KEY_PATH);
-				ConfigConstants.CONFIG_KEY_PATH);
 	}
 
 	/**
@@ -600,6 +630,10 @@ public class SubmoduleWalk implements AutoCloseable {
 	 * @throws IOException
 	 */
 	public String getConfigUrl() throws IOException, ConfigInvalidException {
+		// SubmoduleInitCommand copies the submodules.*.url and
+		// submodules.*.update values from .gitmodules to the config, and
+		// does so using the path defined in .gitmodules as the subsection
+		// name. So no path-to-name translation is necessary here.
 		return repoConfig.getString(ConfigConstants.CONFIG_SUBMODULE_SECTION,
 				path, ConfigConstants.CONFIG_KEY_URL);
 	}
@@ -614,9 +648,8 @@ public class SubmoduleWalk implements AutoCloseable {
 	 */
 	public String getModulesUrl() throws IOException, ConfigInvalidException {
 		lazyLoadModulesConfig();
-		return modulesConfig.getString(
+		return modulesConfig.getString(ConfigConstants.CONFIG_SUBMODULE_SECTION,
-				ConfigConstants.CONFIG_SUBMODULE_SECTION, path,
+				getModuleName(path), ConfigConstants.CONFIG_KEY_URL);
-				ConfigConstants.CONFIG_KEY_URL);
 	}
 
 	/**
@@ -642,9 +675,8 @@ public class SubmoduleWalk implements AutoCloseable {
 	 */
 	public String getModulesUpdate() throws IOException, ConfigInvalidException {
 		lazyLoadModulesConfig();
-		return modulesConfig.getString(
+		return modulesConfig.getString(ConfigConstants.CONFIG_SUBMODULE_SECTION,
-				ConfigConstants.CONFIG_SUBMODULE_SECTION, path,
+				getModuleName(path), ConfigConstants.CONFIG_KEY_UPDATE);
-				ConfigConstants.CONFIG_KEY_UPDATE);
 	}
 
 	/**
@@ -660,7 +692,7 @@ public class SubmoduleWalk implements AutoCloseable {
 			ConfigInvalidException {
 		lazyLoadModulesConfig();
 		String name = modulesConfig.getString(
-				ConfigConstants.CONFIG_SUBMODULE_SECTION, path,
+				ConfigConstants.CONFIG_SUBMODULE_SECTION, getModuleName(path),
 				ConfigConstants.CONFIG_KEY_IGNORE);
 		if (name == null)
 			return null;