From cac933e3fa214dc24bbea735bbbe3cd542d22af6 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 20 Jun 2020 07:54:42 -0600 Subject: [PATCH 1/2] Add JENKINS-57587 test case --- .../plugins/git/GitSCMFileSystemTest.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java index 1a0e38828c..a4a45e40d0 100644 --- a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java +++ b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java @@ -63,6 +63,7 @@ import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.hamcrest.MatcherAssert.assertThat; @@ -197,6 +198,29 @@ public void slashyBranches() throws Exception { assertThat(file.contentAsString(), is("modified")); } + @Issue("JENKINS-57587") + @Test + public void wildcardBranchNameCausesNPE() throws Exception { + sampleRepo.init(); + sampleRepo.write("file", "contents-for-npe-when-branch-name-is-asterisk"); + sampleRepo.git("commit", "--all", "--message=npe-when-branch-name-is-asterisk"); + /* Non-existent branch names like 'not-a-branch', will fail + * the build early with a message that the remote ref cannot + * be found. Branch names that are valid portions of a + * refspec like '*' do not fail the build early but generate a + * null pointer exception when trying to resolve the branch + * name in the GitSCMFileSystem constructor. + */ + SCMFileSystem fs = SCMFileSystem.of(r.createFreeStyleProject(), + new GitSCM(GitSCM.createRepoList(sampleRepo.toString(), null), + Collections.singletonList(new BranchSpec("*")), // JENKINS-57587 issue here + false, + Collections.emptyList(), + null, null, + Collections.emptyList())); + assertThat("Wildcard branch name '*' resolved to a specific checkout unexpectedly", fs, is(nullValue())); + } + @Test @Deprecated // Testing deprecated GitSCMSource constructor public void lastModified_Smokes() throws Exception { From 39e2962999034298a6e96d73bd781508891c26d1 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 20 Jun 2020 08:12:56 -0600 Subject: [PATCH 2/2] [JENKINS-57587] Prevent GitSCMFileSystem NPE on '*' branch name This has the questionable result that by avoiding the null pointer exception it now allows a Pipeline job that is not a multibranch Pipeline to switch from one branch to another inside the same job. The technique of using a single job to validate multiple branches was commonly used with Freestyle jobs because they did not have the concept of multibranch. It is generally much better to allow Jenkins to manage the Pipeline jobs with multibranch rather than having a single job that switches between branches. However, a null pointer exception is not a friendly way to tell users that they should not use a specific technique. --- src/main/java/jenkins/plugins/git/GitSCMFileSystem.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java index 893587bcaf..c3c5a16e04 100644 --- a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java +++ b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java @@ -255,6 +255,7 @@ public boolean supports(SCM source) { return source instanceof GitSCM && ((GitSCM) source).getUserRemoteConfigs().size() == 1 && ((GitSCM) source).getBranches().size() == 1 + && !((GitSCM) source).getBranches().get(0).getName().equals("*") // JENKINS-57587 && ( ((GitSCM) source).getBranches().get(0).getName().matches( "^((\\Q" + Constants.R_HEADS + "\\E.*)|([^/]+)|(\\*/[^/*]+(/[^/*]+)*))$" @@ -263,7 +264,7 @@ public boolean supports(SCM source) { "^((\\Q" + Constants.R_TAGS + "\\E.*)|([^/]+)|(\\*/[^/*]+(/[^/*]+)*))$" ) ); - // we only support where the branch spec is obvious + // we only support where the branch spec is obvious and not a wildcard } @Override