JGit checkout fails if refspec has leading spaces

src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java

@@ -615,7 +615,7 @@ public void execute() throws GitException, InterruptedException {
                 if (refspecs != null) {
                     for (RefSpec rs : refspecs) {
                         if (rs != null) {
-                            args.add(rs.toString());
+                            args.add(trimRefSpec(rs).toString());
                         }
                     }
                 }
@@ -672,7 +672,7 @@ public void fetch(String remoteName, RefSpec... refspec) throws GitException, In
         if (refspec != null && refspec.length > 0) {
             for (RefSpec rs : refspec) {
                 if (rs != null) {
-                    args.add(rs.toString());
+                    args.add(trimRefSpec(rs).toString());
                 }
             }
         }
@@ -798,7 +798,7 @@ public CloneCommand depth(Integer depth) {
 
             @Override
             public CloneCommand refspecs(List<RefSpec> refspecs) {
-                this.refspecs = new ArrayList<>(refspecs);
+                this.refspecs = trimRefSpecs(refspecs);
                 return this;
             }
 
@@ -2928,7 +2928,7 @@ public void execute() throws GitException, InterruptedException {
                 args.add("push", remote.toPrivateASCIIString());
 
                 if (refspec != null) {
-                    args.add(refspec);
+                    args.add(trimRefSpec(refspec));
                 }
 
                 if (force) {
@@ -3897,7 +3897,7 @@ public void push(RemoteConfig repository, String refspec) throws GitException, I
         addCheckedRemoteUrl(args, url);
 
         if (refspec != null) {
-            args.add(refspec);
+            args.add(trimRefSpec(refspec));
         }
 
         launchCommandWithCredentials(args, workspace, cred, uri);

src/main/java/org/jenkinsci/plugins/gitclient/JGitAPIImpl.java

@@ -795,7 +795,7 @@ public void execute() throws GitException {
                     if (refspecs != null) {
                         for (RefSpec rs : refspecs) {
                             if (rs != null) {
-                                allRefSpecs.add(rs);
+                                allRefSpecs.add(trimRefSpec(rs));
                             }
                         }
                     }
@@ -866,7 +866,7 @@ public void fetch(String remoteName, RefSpec... refspec) throws GitException {
             if (refspec != null && refspec.length > 0) {
                 for (RefSpec rs : refspec) {
                     if (rs != null) {
-                        refSpecs.add(rs);
+                        refSpecs.add(trimRefSpec(rs));
                     }
                 }
             }
@@ -1609,7 +1609,7 @@ public CloneCommand reference(String reference) {
 
             @Override
             public CloneCommand refspecs(List<RefSpec> refspecs) {
-                this.refspecs = new ArrayList<>(refspecs);
+                this.refspecs = trimRefSpecs(refspecs);
                 return this;
             }
 

src/main/java/org/jenkinsci/plugins/gitclient/LegacyCompatibleGitAPIImpl.java

@@ -262,6 +262,67 @@
         return showRevision(null, r);
     }
 
+    /**
+     * Removes the whitespace which surrounds the source and the destination of a refspec.
+     * Command line git discards that whitespace when it reads a refspec from a configuration
+     * file, JGit keeps it and then reads <code>" +refs/heads/*"</code> as the source ref.
+     * See JENKINS-70303.
+     *
+     * @param refSpec refspec which may include surrounding whitespace, may be null
+     * @return trimmed refspec, null if refSpec is null
+     */
+    static String trimRefSpec(String refSpec) {
+        if (refSpec == null) {
+            return null;
+        }
+        /* JGit splits the refspec on its last colon, do the same */
+        int colon = refSpec.lastIndexOf(':');
+        if (colon < 0) {
+            return refSpec.trim();
+        }
+        return refSpec.substring(0, colon).trim() + ":"
+                + refSpec.substring(colon + 1).trim();
+    }
+
+    /**
+     * @param refSpec refspec which may include surrounding whitespace, may be null
+     * @return trimmed refspec, null if refSpec is null
+     * @see #trimRefSpec(String)
+     */
+    static RefSpec trimRefSpec(RefSpec refSpec) {
+        if (refSpec == null) {
+            return null;
+        }
+        String original = refSpec.toString();
+        String trimmed = trimRefSpec(original);
+        if (original.equals(trimmed)) {
+            return refSpec;
+        }
+        try {
+            return new RefSpec(trimmed);
+        } catch (IllegalArgumentException mismatchedWildcards) {
+            try {
+                /* JGit rejects a wildcard on only one side unless mismatches are allowed */
+                return new RefSpec(trimmed, RefSpec.WildcardMode.ALLOW_MISMATCH);
+            } catch (IllegalArgumentException invalidRefSpec) {
+                return refSpec;
+            }
+        }
+    }
+
+    /**
+     * @param refSpecs refspecs which may include surrounding whitespace, must not be null
+     * @return trimmed refspecs
+     * @see #trimRefSpec(RefSpec)
+     */
+    static List<RefSpec> trimRefSpecs(List<RefSpec> refSpecs) {
+        List<RefSpec> trimmed = new ArrayList<>(refSpecs.size());
+        for (RefSpec refSpec : refSpecs) {
+            trimmed.add(trimRefSpec(refSpec));
+        }
+        return trimmed;
+    }
+
     /**
      * This method takes a branch specification and normalizes it get unambiguous results.
      * This is the case when using "refs/heads/"<br>

src/test/java/org/jenkinsci/plugins/gitclient/GitClientCloneTest.java

@@ -40,6 +40,7 @@
 import org.junit.jupiter.params.ParameterizedClass;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.jvnet.hudson.test.Issue;
 
 @ParameterizedClass(name = "{0}")
 @MethodSource("gitObjects")
@@ -351,6 +352,32 @@ void test_clone_refspecs() throws Exception {
         assertThat(remoteBranches.size(), is(2));
     }
 
+    @Test
+    @Issue("JENKINS-70303")
+    void test_clone_refspecs_with_surrounding_whitespace() throws Exception {
+        List<RefSpec> refspecs = Arrays.asList(
+                new RefSpec(" +refs/heads/master:refs/remotes/origin/master"),
+                new RefSpec("+refs/heads/1.4.x:refs/remotes/origin/1.4.x "));
+        testGitClient
+                .clone_()
+                .url(workspace.localMirror())
+                .refspecs(refspecs)
+                .repositoryName("origin")
+                .execute();
+        testGitClient.withRepository((Repository workRepo, VirtualChannel channel) -> {
+            String[] fetchRefSpecs = workRepo.getConfig()
+                    .getStringList(ConfigConstants.CONFIG_REMOTE_SECTION, Constants.DEFAULT_REMOTE_NAME, "fetch");
+            assertThat(fetchRefSpecs.length, is(2));
+            assertThat(fetchRefSpecs[0], is("+refs/heads/master:refs/remotes/origin/master"));
+            assertThat(fetchRefSpecs[1], is("+refs/heads/1.4.x:refs/remotes/origin/1.4.x"));
+            return null;
+        });
+        Set<Branch> remoteBranches = testGitClient.getRemoteBranches();
+        assertBranchesExist(remoteBranches, "origin/master");
+        assertBranchesExist(remoteBranches, "origin/1.4.x");
+        assertThat(remoteBranches.size(), is(2));
+    }
+
     @Test
     void test_getRemoteURL_local_clone() throws Exception {
         workspace.cloneRepo(workspace, workspace.localMirror());

src/test/java/org/jenkinsci/plugins/gitclient/GitClientFetchTest.java

@@ -531,6 +531,41 @@ void test_fetch_from_url() throws Exception {
                 is(true));
     }
 
+    @Test
+    @Issue("JENKINS-70303")
+    void test_fetch_refspec_with_surrounding_whitespace() throws Exception {
+        /* Push a commit from the working repo to a bare repo */
+        bareWorkspace = new WorkspaceWithRepo(secondRepo.getRoot(), gitImplName, TaskListener.NULL);
+        bareWorkspace.initBareRepo(bareWorkspace.getGitClient(), true);
+        testGitClient.setRemoteUrl("origin", bareWorkspace.getGitFileDir().getAbsolutePath());
+        workspace.touch(testGitDir, "file-whitespace", "file whitespace content " + UUID.randomUUID());
+        testGitClient.add("file-whitespace");
+        testGitClient.commit("whitespace-refspec-commit");
+        testGitClient.push("origin", defaultBranchName);
+        ObjectId bareCommit = bareWorkspace
+                .getGitClient()
+                .getHeadRev(bareWorkspace.getGitFileDir().getAbsolutePath(), defaultBranchName);
+
+        /* Fetch from the bare repo with a refspec which is surrounded by whitespace */
+        newAreaWorkspace = new WorkspaceWithRepo(thirdRepo.getRoot(), gitImplName, TaskListener.NULL);
+        newAreaWorkspace.initializeWorkspace(
+                "Vojtěch whitespace refspec Zweibrücken-Šafařík", "email.by.git.fetch.test@example.com");
+        List<RefSpec> refSpecs = Collections.singletonList(new RefSpec("  +refs/heads/*:refs/remotes/origin/*  "));
+        newAreaWorkspace
+                .getGitClient()
+                .fetch_()
+                .from(new URIish(bareWorkspace.getGitFileDir().toString()), refSpecs)
+                .execute();
+
+        assertThat(
+                getBranchNames(newAreaWorkspace.getGitClient().getRemoteBranches()),
+                hasItem("origin/" + defaultBranchName));
+        assertThat(
+                "fetched commit does not match the bare repository commit",
+                newAreaWorkspace.getGitClient().revParse("refs/remotes/origin/" + defaultBranchName),
+                is(bareCommit));
+    }
+
     @Test
     void test_fetch_shallow() throws Exception {
         testGitClient.setRemoteUrl("origin", workspace.localMirror());

src/test/java/org/jenkinsci/plugins/gitclient/RefSpecTrimTest.java

@@ -0,0 +1,142 @@
+package org.jenkinsci.plugins.gitclient;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.nullValue;
+import static org.hamcrest.Matchers.sameInstance;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import org.eclipse.jgit.transport.RefSpec;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.jvnet.hudson.test.Issue;
+
+/**
+ * Tests the removal of whitespace which surrounds a refspec.
+ *
+ * @author Akash Manna
+ * @see LegacyCompatibleGitAPIImpl#trimRefSpec(RefSpec)
+ */
+@Issue("JENKINS-70303")
+class RefSpecTrimTest {
+
+    private static final String DEFAULT_REFSPEC = "+refs/heads/*:refs/remotes/origin/*";
+
+    @ParameterizedTest
+    @ValueSource(
+            strings = {
+                " +refs/heads/*:refs/remotes/origin/*",
+                "+refs/heads/*:refs/remotes/origin/* ",
+                "  +refs/heads/*:refs/remotes/origin/*  ",
+                "\t+refs/heads/*:refs/remotes/origin/*\t",
+                "\n+refs/heads/*:refs/remotes/origin/*\n",
+                "+refs/heads/* : refs/remotes/origin/*",
+            })
+    void surroundingWhitespaceIsRemovedFromString(String refSpec) {
+        assertThat(LegacyCompatibleGitAPIImpl.trimRefSpec(refSpec), is(DEFAULT_REFSPEC));
+    }
+
+    @ParameterizedTest
+    @ValueSource(
+            strings = {
+                " +refs/heads/*:refs/remotes/origin/*",
+                "+refs/heads/*:refs/remotes/origin/* ",
+                "  +refs/heads/*:refs/remotes/origin/*  ",
+                "\t+refs/heads/*:refs/remotes/origin/*\t",
+                "+refs/heads/* : refs/remotes/origin/*",
+            })
+    void surroundingWhitespaceIsRemovedFromRefSpec(String refSpec) {
+        RefSpec trimmed = LegacyCompatibleGitAPIImpl.trimRefSpec(new RefSpec(refSpec));
+        assertThat(trimmed.toString(), is(DEFAULT_REFSPEC));
+        assertThat(trimmed.getSource(), is("refs/heads/*"));
+        assertThat(trimmed.getDestination(), is("refs/remotes/origin/*"));
+        assertThat("refspec is not forced", trimmed.isForceUpdate(), is(true));
+    }
+
+    /** Leading whitespace hides the '+' from JGit, so the source ref does not exist. */
+    @Test
+    void untrimmedRefSpecIsMisparsedByJGit() {
+        RefSpec untrimmed = new RefSpec(" " + DEFAULT_REFSPEC);
+        assertThat(untrimmed.getSource(), is(" +refs/heads/*"));
+        assertThat(untrimmed.isForceUpdate(), is(false));
+    }
+
+    /** A trimmed wildcard refspec must still expand, JGit fetch relies on it. */
+    @Test
+    void trimmedWildcardRefSpecExpands() {
+        RefSpec trimmed = LegacyCompatibleGitAPIImpl.trimRefSpec(new RefSpec(" " + DEFAULT_REFSPEC + " "));
+        assertThat(trimmed.isWildcard(), is(true));
+        RefSpec expanded = trimmed.expandFromSource("refs/heads/main");
+        assertThat(expanded.getDestination(), is("refs/remotes/origin/main"));
+    }
+
+    @Test
+    void surroundingWhitespaceIsRemovedFromNegativeRefSpec() {
+        RefSpec trimmed = LegacyCompatibleGitAPIImpl.trimRefSpec(new RefSpec(" ^refs/heads/dev/private "));
+        assertThat(trimmed.toString(), is("^refs/heads/dev/private"));
+        assertThat("refspec is not negative", trimmed.isNegative(), is(true));
+    }
+
+    @Test
+    void refSpecWithoutDestinationIsTrimmed() {
+        assertThat(LegacyCompatibleGitAPIImpl.trimRefSpec(" refs/heads/main "), is("refs/heads/main"));
+        RefSpec trimmed = LegacyCompatibleGitAPIImpl.trimRefSpec(new RefSpec(" refs/heads/main "));
+        assertThat(trimmed.getSource(), is("refs/heads/main"));
+        assertThat(trimmed.getDestination(), is(nullValue()));
+    }
+
+    /** Only the last colon separates source from destination, as in JGit. */
+    @Test
+    void onlyLastColonSeparatesSourceFromDestination() {
+        assertThat(LegacyCompatibleGitAPIImpl.trimRefSpec(" a:b : c "), is("a:b:c"));
+    }
+
+    @Test
+    void refSpecWithoutSurroundingWhitespaceIsUnchanged() {
+        assertThat(LegacyCompatibleGitAPIImpl.trimRefSpec(DEFAULT_REFSPEC), is(DEFAULT_REFSPEC));
+        RefSpec refSpec = new RefSpec(DEFAULT_REFSPEC);
+        assertThat(LegacyCompatibleGitAPIImpl.trimRefSpec(refSpec), is(sameInstance(refSpec)));
+    }
+
+    @Test
+    void nullRefSpecIsNull() {
+        assertThat(LegacyCompatibleGitAPIImpl.trimRefSpec((String) null), is(nullValue()));
+        assertThat(LegacyCompatibleGitAPIImpl.trimRefSpec((RefSpec) null), is(nullValue()));
+    }
+
+    @Test
+    void refSpecListIsTrimmed() {
+        List<RefSpec> refSpecs = Arrays.asList(
+                new RefSpec(" +refs/heads/*:refs/remotes/origin/* "),
+                new RefSpec("+refs/tags/*:refs/tags/*"),
+                new RefSpec(" refs/heads/main:refs/remotes/origin/main"));
+        List<String> trimmed = new ArrayList<>();
+        for (RefSpec refSpec : LegacyCompatibleGitAPIImpl.trimRefSpecs(refSpecs)) {
+            trimmed.add(refSpec.toString());
+        }
+        assertThat(
+                trimmed,
+                contains(DEFAULT_REFSPEC, "+refs/tags/*:refs/tags/*", "refs/heads/main:refs/remotes/origin/main"));
+    }
+
+    @Test
+    void nullEntriesOfRefSpecListArePreserved() {
+        List<RefSpec> trimmed = LegacyCompatibleGitAPIImpl.trimRefSpecs(Collections.singletonList((RefSpec) null));
+        assertThat(trimmed.size(), is(1));
+        assertThat(trimmed.get(0), is(nullValue()));
+    }
+
+    /** Trimming must not turn a mismatched wildcard refspec into an IllegalArgumentException. */
+    @Test
+    void mismatchedWildcardRefSpecIsTrimmed() {
+        RefSpec mismatched =
+                new RefSpec(" +refs/heads/*:refs/remotes/origin/main ", RefSpec.WildcardMode.ALLOW_MISMATCH);
+        RefSpec trimmed = LegacyCompatibleGitAPIImpl.trimRefSpec(mismatched);
+        assertThat(trimmed.toString(), is("+refs/heads/*:refs/remotes/origin/main"));
+    }
+}