diff --git a/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java b/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java index ff07100eed..d0afca0bff 100644 --- a/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java +++ b/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 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); diff --git a/src/main/java/org/jenkinsci/plugins/gitclient/JGitAPIImpl.java b/src/main/java/org/jenkinsci/plugins/gitclient/JGitAPIImpl.java index 94e63ed330..eb3c84b546 100644 --- a/src/main/java/org/jenkinsci/plugins/gitclient/JGitAPIImpl.java +++ b/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 refspecs) { - this.refspecs = new ArrayList<>(refspecs); + this.refspecs = trimRefSpecs(refspecs); return this; } diff --git a/src/main/java/org/jenkinsci/plugins/gitclient/LegacyCompatibleGitAPIImpl.java b/src/main/java/org/jenkinsci/plugins/gitclient/LegacyCompatibleGitAPIImpl.java index 45b17e614f..e1d667507e 100644 --- a/src/main/java/org/jenkinsci/plugins/gitclient/LegacyCompatibleGitAPIImpl.java +++ b/src/main/java/org/jenkinsci/plugins/gitclient/LegacyCompatibleGitAPIImpl.java @@ -262,6 +262,67 @@ public List showRevision(ObjectId r) throws GitException, InterruptedExc 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 " +refs/heads/*" 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 trimRefSpecs(List refSpecs) { + List 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/"
diff --git a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientCloneTest.java b/src/test/java/org/jenkinsci/plugins/gitclient/GitClientCloneTest.java index b1a25c23ae..25cf01b127 100644 --- a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientCloneTest.java +++ b/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 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 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()); diff --git a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientFetchTest.java b/src/test/java/org/jenkinsci/plugins/gitclient/GitClientFetchTest.java index f2764ff8ea..dee3b50332 100644 --- a/src/test/java/org/jenkinsci/plugins/gitclient/GitClientFetchTest.java +++ b/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 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()); diff --git a/src/test/java/org/jenkinsci/plugins/gitclient/RefSpecTrimTest.java b/src/test/java/org/jenkinsci/plugins/gitclient/RefSpecTrimTest.java new file mode 100644 index 0000000000..d8a2d2ea53 --- /dev/null +++ b/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 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 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 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")); + } +}