Merge pull request #284 from MarkEWaite/fix-submodule-url-regex

Fix submodule url regex [JENKINS-46054]
jenkinsci · Nov 18, 2017 · b8bad8a · b8bad8a
2 parents c2b1ff4 + 52f681e
commit b8bad8a
Show file tree

Hide file tree

Showing 3 changed files with 180 additions and 2 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java b/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java
@@ -141,6 +141,25 @@ public class CliGitAPIImpl extends LegacyCompatibleGitAPIImpl {
     private StandardCredentials defaultCredentials;
     private StandardCredentials lfsCredentials;
 
+    /* git config --get-regex applies the regex to match keys, and returns all matches (including substring matches).
+     * Thus, a config call:
+     *   git config -f .gitmodules --get-regexp "^submodule\.([^ ]+)\.url"
+     * will report two lines of output if the submodule URL includes ".url":
+     *   submodule.modules/JENKINS-46504.url.path modules/JENKINS-46504.url
+     *   submodule.modules/JENKINS-46504.url.url https://github.com/MarkEWaite/JENKINS-46054.url
+     * The code originally used the same pattern for get-regexp and for output parsing.
+     * By using the same pattern in both places, it incorrectly took the first line
+     * of output as the URL of a submodule (when it is instead the path of a submodule).
+    */
+    private final static String SUBMODULE_REMOTE_PATTERN_CONFIG_KEY = "^submodule\\.([^ ]+)\\.url";
+
+    /* See comments for SUBMODULE_REMOTE_PATTERN_CONFIG_KEY to explain why this
+     * regular expression string adds the trailing space character as part of its match.
+     * Without the trailing whitespace character in the pattern, too many matches are found.
+     */
+    /* Package protected for testing */
+    final static String SUBMODULE_REMOTE_PATTERN_STRING = SUBMODULE_REMOTE_PATTERN_CONFIG_KEY + "\\b\\s";
+
     private void warnIfWindowsTemporaryDirNameHasSpaces() {
         if (!isWindows()) {
             return;
@@ -1089,7 +1108,7 @@ else if (!referencePath.isDirectory())
                 try {
                     // We might fail if we have no modules, so catch this
                     // exception and just return.
-                    cfgOutput = launchCommand("config", "-f", ".gitmodules", "--get-regexp", "^submodule\\.(.*)\\.url");
+                    cfgOutput = launchCommand("config", "-f", ".gitmodules", "--get-regexp", SUBMODULE_REMOTE_PATTERN_CONFIG_KEY);
                 } catch (GitException e) {
                     listener.error("No submodules found.");
                     return;
@@ -1098,7 +1117,7 @@ else if (!referencePath.isDirectory())
                 // Use a matcher to find each configured submodule name, and
                 // then run the submodule update command with the provided
                 // path.
-                Pattern pattern = Pattern.compile("^submodule\\.(.*)\\.url", Pattern.MULTILINE);
+                Pattern pattern = Pattern.compile(SUBMODULE_REMOTE_PATTERN_STRING, Pattern.MULTILINE);
                 Matcher matcher = pattern.matcher(cfgOutput);
                 while (matcher.find()) {
                     ArgumentListBuilder perModuleArgs = args.clone();

diff --git a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientTest.java b/src/test/java/org/jenkinsci/plugins/gitclient/GitClientTest.java
@@ -16,6 +16,7 @@
 import java.io.UnsupportedEncodingException;
 import java.nio.charset.Charset;
 import java.nio.file.Files;
+import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -1437,6 +1438,54 @@ public void testSubmodulesUsedFromOtherBranches() throws Exception {
         assertSubmoduleStatus(gitClient, true, "firewall", "ntp", "sshkeys"); // newDirName module won't be there
     }
 
+    @Issue("JENKINS-46054")
+    @Test
+    public void testSubmoduleUrlEndsWithDotUrl() throws Exception {
+        // Create a new repository that includes ".url" in directory name
+        File baseDir = tempFolder.newFolder();
+        File urlRepoDir = new File(baseDir, "my-submodule.url");
+        assertTrue("Failed to create URL repo dir", urlRepoDir.mkdir());
+        GitClient urlRepoClient = Git.with(TaskListener.NULL, new EnvVars()).in(urlRepoDir).using(gitImplName).getClient();
+        urlRepoClient.init();
+        File readme = new File(urlRepoDir, "readme");
+        String readmeText = "This repo includes .url in its directory name (" + random.nextInt() + ")";
+        Files.write(Paths.get(readme.getAbsolutePath()), readmeText.getBytes());
+        urlRepoClient.add("readme");
+        urlRepoClient.commit("Added README to repo used as a submodule");
+
+        // Add new repository as submodule to repository that ends in .url
+        File repoHasSubmodule = new File(baseDir, "has-submodule.url");
+        assertTrue("Failed to create repo dir that will have submodule", repoHasSubmodule.mkdir());
+        GitClient repoHasSubmoduleClient = Git.with(TaskListener.NULL, new EnvVars()).in(repoHasSubmodule).using(gitImplName).getClient();
+        repoHasSubmoduleClient.init();
+        File hasSubmoduleReadme = new File(repoHasSubmodule, "readme");
+        String hasSubmoduleReadmeText = "Repo has a submodule that includes .url in its directory name (" + random.nextInt() + ")";
+        Files.write(Paths.get(hasSubmoduleReadme.getAbsolutePath()), hasSubmoduleReadmeText.getBytes());
+        repoHasSubmoduleClient.add("readme");
+        repoHasSubmoduleClient.commit("Added README to repo that will include a submodule whose URL ends in '.url'");
+        String moduleDirBaseName = "module.named.url";
+        File modulesDir = new File(repoHasSubmodule, "modules");
+        assertTrue("Failed to create modules dir in repoHasSubmodule", modulesDir.mkdir());
+        repoHasSubmoduleClient.addSubmodule(repoHasSubmodule.getAbsolutePath(), "modules/" + moduleDirBaseName);
+        repoHasSubmoduleClient.add(".");
+        repoHasSubmoduleClient.commit("Add modules/" + moduleDirBaseName + " as submodule");
+        repoHasSubmoduleClient.submoduleInit();
+        repoHasSubmoduleClient.submoduleUpdate(false);
+        assertSubmoduleStatus(repoHasSubmoduleClient, true, moduleDirBaseName);
+
+        // Clone repoHasSubmodule to new repository with submodule
+        File cloneDir = new File(baseDir, "cloned-submodule");
+        assertTrue("Failed to create clone dir", cloneDir.mkdir());
+        GitClient cloneGitClient = Git.with(TaskListener.NULL, new EnvVars()).in(cloneDir).using(gitImplName).getClient();
+        cloneGitClient.init();
+        cloneGitClient.clone_().url(repoHasSubmodule.getAbsolutePath()).execute();
+        String branch = "master";
+        cloneGitClient.checkoutBranch(branch, "origin/" + branch);
+        cloneGitClient.submoduleInit();
+        cloneGitClient.submoduleUpdate().recursive(false).execute();
+        assertSubmoduleStatus(cloneGitClient, true, moduleDirBaseName);
+    }
+
     @Test
     public void testGetSubmodules() throws Exception {
         assumeThat(gitImplName, is("git")); // JGit implementation doesn't handle renamed submodules

diff --git a/src/test/java/org/jenkinsci/plugins/gitclient/SubmodulePatternStringTest.java b/src/test/java/org/jenkinsci/plugins/gitclient/SubmodulePatternStringTest.java
@@ -0,0 +1,110 @@
+package org.jenkinsci.plugins.gitclient;
+
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import static org.hamcrest.Matchers.is;
+import static org.junit.Assert.*;
+import org.junit.Before;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+
+public class SubmodulePatternStringTest {
+
+    private Pattern submoduleConfigPattern;
+    private String remoteName = "simple-name";
+
+    @Before
+    public void configure() {
+        // String patternString = "^submodule\\.([^ ]+)\\.url ";
+        String patternString = CliGitAPIImpl.SUBMODULE_REMOTE_PATTERN_STRING;
+        submoduleConfigPattern = Pattern.compile(patternString, Pattern.MULTILINE);
+    }
+
+    @Issue("46054")
+    @Test
+    public void urlEmbeddedInRepoURL() {
+        String repoUrl = "file://gitroot/thirdparty.url.repo.git";
+        String submoduleConfigOutput = "submodule." + remoteName + ".url " + repoUrl;
+        Matcher matcher = submoduleConfigPattern.matcher(submoduleConfigOutput);
+        assertTrue("Match not found for '" + submoduleConfigOutput + "'", matcher.find());
+        assertThat(matcher.group(1), is(remoteName));
+    }
+
+    @Issue("46054")
+    @Test
+    public void urlEmbeddedInRepoURLsubmoduleEmbeddedDot() {
+        String repoUrl = "https://mark.url:some%20pass.urlify@gitroot/repo.git";
+        remoteName = "simple.name";
+        String submoduleConfigOutput = "submodule." + remoteName + ".url " + repoUrl;
+        Matcher matcher = submoduleConfigPattern.matcher(submoduleConfigOutput);
+        assertTrue("Match not found for '" + submoduleConfigOutput + "'", matcher.find());
+        assertThat(matcher.group(1), is(remoteName));
+    }
+
+    @Issue("46054")
+    @Test
+    public void urlEmbeddedInSubmoduleRepoNameEndsWithURL() {
+        String repoUrl = "https://gitroot/repo.url";
+        remoteName = "simple.name";
+        String submoduleConfigOutput = "submodule." + remoteName + ".url " + repoUrl;
+        Matcher matcher = submoduleConfigPattern.matcher(submoduleConfigOutput);
+        assertTrue("Match not found for '" + submoduleConfigOutput + "'", matcher.find());
+        assertThat(matcher.group(1), is(remoteName));
+    }
+
+    @Issue("46054")
+    @Test
+    public void urlEmbeddedInSubmoduleRepoNameEndsWithURLSpace() {
+        String repoUrl = "https://gitroot/repo.url ";
+        remoteName = "simple.name";
+        String submoduleConfigOutput = "submodule." + remoteName + ".url " + repoUrl;
+        Matcher matcher = submoduleConfigPattern.matcher(submoduleConfigOutput);
+        assertTrue("Match not found for '" + submoduleConfigOutput + "'", matcher.find());
+        assertThat(matcher.group(1), is(remoteName));
+    }
+
+    @Issue("46054")
+    @Test
+    public void urlEmbeddedInSubmoduleNameAndRepoNameEndsWithURL() {
+        String repoUrl = "https://gitroot/repo.url.git";
+        remoteName = "simple.name.url";
+        String submoduleConfigOutput = "submodule." + remoteName + ".url " + repoUrl;
+        Matcher matcher = submoduleConfigPattern.matcher(submoduleConfigOutput);
+        assertTrue("Match not found for '" + submoduleConfigOutput + "'", matcher.find());
+        assertThat(matcher.group(1), is(remoteName));
+    }
+
+    @Issue("46054")
+    @Test
+    public void urlExploratoryTestFailureCase() {
+        /* See https://github.com/MarkEWaite/JENKINS-46054.url/ */
+        String repoUrl = "https://github.com/MarkEWaite/JENKINS-46054.url";
+        remoteName = "modules/JENKINS-46504.url";
+        String submoduleConfigOutput = "submodule." + remoteName + ".url " + repoUrl;
+        Matcher matcher = submoduleConfigPattern.matcher(submoduleConfigOutput);
+        assertTrue("Match not found for '" + submoduleConfigOutput + "'", matcher.find());
+        assertThat(matcher.group(1), is(remoteName));
+    }
+
+    @Issue("46054")
+    @Test
+    public void remoteNameIncludesSubmodule() {
+        /* See https://github.com/MarkEWaite/JENKINS-46054.url/ */
+        String repoUrl = "https://github.com/MarkEWaite/JENKINS-46054.url";
+        remoteName = "submodule.JENKINS-46504.url";
+        String submoduleConfigOutput = "submodule." + remoteName + ".url " + repoUrl;
+        Matcher matcher = submoduleConfigPattern.matcher(submoduleConfigOutput);
+        assertTrue("Match not found for '" + submoduleConfigOutput + "'", matcher.find());
+        assertThat(matcher.group(1), is(remoteName));
+    }
+
+    @Issue("46054")
+    @Test
+    public void urlEmbeddedInRepoNameEndsWithURLEmptyRemoteName() {
+        String repoUrl = "https://github.com/MarkEWaite/JENKINS-46054.url.git";
+        remoteName = "";
+        String submoduleConfigOutput = "submodule." + remoteName + ".url " + repoUrl;
+        Matcher matcher = submoduleConfigPattern.matcher(submoduleConfigOutput);
+        assertFalse("Unexpected match found for '" + submoduleConfigOutput + "'", matcher.find());
+    }
+}