From d7a63c337538bc6bc9c25affc7f5333c58588049 Mon Sep 17 00:00:00 2001 From: Michael Trimarchi Date: Wed, 27 May 2026 20:11:03 +0200 Subject: [PATCH 01/16] feat: Add support for gerrit changes for download Jenkins file We want to support the verification pipeline when the Jenkinsfile is part of the modification itself. This allow to test the pipeline on any project and test it. The change should address JENKINS-66599 Signed-off-by: Michael Trimarchi --- .../java/hudson/plugins/git/BranchSpec.java | 3 +++ .../jenkins/plugins/git/GitSCMFileSystem.java | 5 ++++ .../plugins/git/BranchSpec/help-name.html | 4 ++++ .../hudson/plugins/git/BranchSpecTest.java | 8 +++++++ .../plugins/git/GitSCMFileSystemTest.java | 23 +++++++++++++++++++ 5 files changed, 43 insertions(+) diff --git a/src/main/java/hudson/plugins/git/BranchSpec.java b/src/main/java/hudson/plugins/git/BranchSpec.java index 2c6ce09367..fba6efdaeb 100644 --- a/src/main/java/hudson/plugins/git/BranchSpec.java +++ b/src/main/java/hudson/plugins/git/BranchSpec.java @@ -159,6 +159,9 @@ private Pattern getPattern(EnvVars env, String repositoryName) { String regexSubstring = expandedName.substring(1, expandedName.length()); return Pattern.compile(regexSubstring); } + if (expandedName.startsWith("refs/changes/")) { + return Pattern.compile(Pattern.quote(expandedName)); + } if (repositoryName != null) { // remove the "refs/.../" stuff from the branch-spec if necessary String pattern = cutRefs(expandedName) diff --git a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java index 2165734450..ca610a65d3 100644 --- a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java +++ b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java @@ -267,6 +267,9 @@ public boolean supports(SCM source) { || gscm.getBranches().get(0).getName().matches( "^((\\Q" + Constants.R_TAGS + "\\E.*)|([^/]+)|(\\*/[^/*]+(/[^/*]+)*))$" ) + || gscm.getBranches().get(0).getName().matches( + "^((\\Qrefs/changes/\\E.*)|([^/]+)|(\\*/[^/*]+(/[^/*]+)*))$" + ) ); // we only support where the branch spec is obvious and not a wildcard } @@ -306,6 +309,8 @@ static HeadNameResult calculate(@NonNull BranchSpec branchSpec, String prefix = Constants.R_HEADS; if (branchSpecExpandedName.startsWith(Constants.R_TAGS)) { prefix = Constants.R_TAGS; + } else if (branchSpecExpandedName.startsWith("refs/changes")) { + prefix = "refs/changes/"; } String headName; diff --git a/src/main/resources/hudson/plugins/git/BranchSpec/help-name.html b/src/main/resources/hudson/plugins/git/BranchSpec/help-name.html index 7c39aee1fd..3548b4846e 100644 --- a/src/main/resources/hudson/plugins/git/BranchSpec/help-name.html +++ b/src/main/resources/hudson/plugins/git/BranchSpec/help-name.html @@ -47,6 +47,10 @@ Tracks/checks out the specified tag.
E.g. refs/tags/git-2.3.0 +
  • refs/changes/<gerritChange>
    + Tracks/checks out the specified gerrit change.
    + E.g. refs/changes/91/45391/1 +
  • <commitId>
    Checks out the specified commit.
    E.g. 5062ac843f2b947733e6a3b105977056821bd352, 5062ac84, ... diff --git a/src/test/java/hudson/plugins/git/BranchSpecTest.java b/src/test/java/hudson/plugins/git/BranchSpecTest.java index 835cf16a5b..27b82e6bac 100644 --- a/src/test/java/hudson/plugins/git/BranchSpecTest.java +++ b/src/test/java/hudson/plugins/git/BranchSpecTest.java @@ -149,6 +149,14 @@ void testUsesRefsHeads() { assertFalse(m.matches("remote/origin/jane")); } + @Test + public void testMatchesGerritChangeRef() { + BranchSpec spec = new BranchSpec("refs/changes/91/45391/1"); + + assertTrue(spec.matches("refs/changes/91/45391/1")); + assertFalse(spec.matches("refs/heads/refs/changes/91/45391/1")); + } + @Test void testUsesJavaPatternDirectlyIfPrefixedWithColon() { BranchSpec m = new BranchSpec(":^(?!(origin/prefix)).*"); diff --git a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java index ffee98ff89..41923f1d3e 100644 --- a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java +++ b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java @@ -435,6 +435,29 @@ void create_SCMFileSystem_from_tag() throws Exception { assertThat(file.contentAsString(), is("modified")); } + @Test + public void testSupportsGerritChangeRef() throws Exception { + // Create a dummy GitSCM configured with a Gerrit change refspec + GitSCM scm = new GitSCM( + GitSCM.createRepoList("https://example.com/repo.git", null), + Collections.singletonList(new BranchSpec("refs/changes/91/45391/1")), + null, null, Collections.emptyList()); + + GitSCMFileSystem.BuilderImpl builder = new GitSCMFileSystem.BuilderImpl(); + assertTrue(builder.supports(scm), "Builder should support refs/changes/ branch specs"); + } + + @Test + public void testHeadNameResultCalculateGerritChange() { + BranchSpec spec = new BranchSpec("refs/changes/91/45391/1"); + + GitSCMFileSystem.BuilderImpl.HeadNameResult result = + GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(spec, null, null); + + assertEquals("refs/changes/", result.prefix); + assertEquals("91/45391/1", result.headName); + } + @Issue("JENKINS-52964") @Test void filesystem_supports_descriptor() throws Exception { From 02ec983d94c63876af2e840410f2c332667e1e20 Mon Sep 17 00:00:00 2001 From: kuc2hi Date: Mon, 19 Jun 2023 09:06:22 +0200 Subject: [PATCH 02/16] feat: Support custom refspec in lightweight checkout Allow users to specify a custom refspec for fetching in lightweight checkout mode via GitSCMFileSystem. This enables SCMFileSystem to fetch specific revisions rather than always fetching the branch tip. Add support for: - Custom refspec from UserRemoteConfig.getRefspec() - FETCH_HEAD as a valid branch specification - Raw commit hashes as branch specification - refs/changes/* prefix for Gerrit change refs These changes enable lightweight checkout to be used for: - Gerrit changes using refs/changes/* refspecs with FETCH_HEAD - Checking out a specific commit instead of branch HEAD --- .../jenkins/plugins/git/GitSCMFileSystem.java | 36 +++++++++++++++---- 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java index ca610a65d3..0c8ba8e37c 100644 --- a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java +++ b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java @@ -59,6 +59,9 @@ import java.util.concurrent.locks.Lock; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + import jenkins.scm.api.SCMFile; import jenkins.scm.api.SCMFileSystem; import jenkins.scm.api.SCMHead; @@ -403,15 +406,36 @@ public SCMFileSystem build(@NonNull Item owner, @NonNull SCM scm, @CheckForNull } HeadNameResult headNameResult = HeadNameResult.calculate(branchSpec, rev, env); + String head = Constants.R_REMOTES + remoteName + "/" + headNameResult.headName; - client.fetch_().prune(true).from(remoteURI, Collections.singletonList(new RefSpec( - "+" + headNameResult.prefix + headNameResult.headName + ":" + Constants.R_REMOTES + remoteName + "/" - + headNameResult.headName))).execute(); + if (branchSpec.getName().equals("FETCH_HEAD")) { + head = "FETCH_HEAD"; + } else { + // check for a commit hash in branchSpec + final String regex = "^[a-fA-F0-9]{40}$"; + final Pattern pattern = Pattern.compile(regex, Pattern.MULTILINE); + final Matcher matcher = pattern.matcher(branchSpec.getName()); + + if (matcher.find()) { + head = branchSpec.getName(); + rev = new AbstractGitSCMSource.SCMRevisionImpl(new SCMHead(headNameResult.headName), head); + } + } + + String refspec = config.getRefspec(); + if (refspec != null) { + if (env != null) { + refspec = env.expand(refspec); + } + } else { + refspec = "+" + headNameResult.prefix + headNameResult.headName + ":" + Constants.R_REMOTES + remoteName + "/" + + headNameResult.headName; + } + + client.fetch_().prune(true).from(remoteURI, Collections.singletonList(new RefSpec(refspec))).execute(); listener.getLogger().println("Done."); - return new GitSCMFileSystem(client, remote, Constants.R_REMOTES + remoteName + "/" + headNameResult.headName, (AbstractGitSCMSource.SCMRevisionImpl) rev); - } catch (GitException x) { - throw new IOException(x); + return new GitSCMFileSystem(client, remote, head, (AbstractGitSCMSource.SCMRevisionImpl) rev); } finally { cacheLock.unlock(); } From a53959d152f302d7d4fa7356d49eed5cefea54fd Mon Sep 17 00:00:00 2001 From: kuc2hi Date: Wed, 21 Jun 2023 10:00:38 +0200 Subject: [PATCH 03/16] refactor: Move branch resolution logic into HeadNameResult.calculate() Centralize branch name resolution, prefix detection, commit hash matching, and refspec construction inside HeadNameResult.calculate(). Previously this logic was split between the BuilderImpl.build() method and HeadNameResult, making the flow hard to follow. - Add remoteName parameter for constructing remote-tracking refs - Add remoteHeadName, refspec, and rev fields to HeadNameResult - Move FETCH_HEAD detection from build() into calculate() - Move commit hash matching from build() into calculate() - Move refspec construction from build() into calculate() - Simplify build() to delegate entirely to HeadNameResult.calculate() - Add unit tests for head name calculation with refspecs, commits, and FETCH_HEAD --- .../jenkins/plugins/git/GitSCMFileSystem.java | 87 +++++---- .../plugins/git/GitSCMFileSystemTest.java | 183 ++++++++++++++---- 2 files changed, 190 insertions(+), 80 deletions(-) diff --git a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java index 0c8ba8e37c..8b85521085 100644 --- a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java +++ b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java @@ -294,41 +294,78 @@ public boolean supportsDescriptor(SCMSourceDescriptor descriptor) { static class HeadNameResult { final String headName; - final String prefix; + final String remoteHeadName; + final String refspec; + final SCMRevision rev; - private HeadNameResult(String headName, String prefix) { + private HeadNameResult(String headName, String remoteHeadName, String refspec, SCMRevision rev) { this.headName = headName; - this.prefix = prefix; + this.remoteHeadName = remoteHeadName; + this.refspec = refspec; + this.rev = rev; } static HeadNameResult calculate(@NonNull BranchSpec branchSpec, @CheckForNull SCMRevision rev, - @CheckForNull EnvVars env) { + @NonNull String refSpec, + @CheckForNull EnvVars env, + @CheckForNull String remoteName) { + String branchSpecExpandedName = branchSpec.getName(); if (env != null) { branchSpecExpandedName = env.expand(branchSpecExpandedName); } + String refspecExpandedName = refSpec; + if (env != null) { + refspecExpandedName = env.expand(refspecExpandedName); + } + // default to a branch (refs/heads) String prefix = Constants.R_HEADS; + // check for a tag if (branchSpecExpandedName.startsWith(Constants.R_TAGS)) { prefix = Constants.R_TAGS; } else if (branchSpecExpandedName.startsWith("refs/changes")) { prefix = "refs/changes/"; + } else { + // check for FETCH_HEAD + if (branchSpecExpandedName.equals("FETCH_HEAD") && !refspecExpandedName.equals("")) { + prefix = null; + } else { + // check for commit-id + final String regex = "^[a-fA-F0-9]{40}$"; + final Pattern pattern = Pattern.compile(regex, Pattern.MULTILINE); + final Matcher matcher = pattern.matcher(branchSpecExpandedName); + + if (matcher.find()) { + // commit-id + prefix = null; + rev = new AbstractGitSCMSource.SCMRevisionImpl(new SCMHead(branchSpecExpandedName), branchSpecExpandedName); + } + } } - String headName; + String headName = branchSpecExpandedName; if (rev != null && env != null) { headName = env.expand(rev.getHead().getName()); } else { - if (branchSpecExpandedName.startsWith(prefix)) { + if (prefix != null && branchSpecExpandedName.startsWith(prefix)) { headName = branchSpecExpandedName.substring(prefix.length()); } else if (branchSpecExpandedName.startsWith("*/")) { headName = branchSpecExpandedName.substring(2); - } else { - headName = branchSpecExpandedName; } } - return new HeadNameResult(headName, prefix); + + if (refspecExpandedName == null || refspecExpandedName.equals("")) { + refspecExpandedName = "+" + prefix + headName + ":" + Constants.R_REMOTES + remoteName + "/" + headName; + } + + String remoteHead = headName; + if (prefix == Constants.R_HEADS || prefix == Constants.R_TAGS) { + remoteHead = Constants.R_REMOTES + remoteName + "/" + headName; + } + + return new HeadNameResult(headName, remoteHead, refspecExpandedName, rev); } } @@ -405,37 +442,11 @@ public SCMFileSystem build(@NonNull Item owner, @NonNull SCM scm, @CheckForNull listener.getLogger().println("URI syntax exception for '" + remoteName + "' " + ex); } - HeadNameResult headNameResult = HeadNameResult.calculate(branchSpec, rev, env); - String head = Constants.R_REMOTES + remoteName + "/" + headNameResult.headName; - - if (branchSpec.getName().equals("FETCH_HEAD")) { - head = "FETCH_HEAD"; - } else { - // check for a commit hash in branchSpec - final String regex = "^[a-fA-F0-9]{40}$"; - final Pattern pattern = Pattern.compile(regex, Pattern.MULTILINE); - final Matcher matcher = pattern.matcher(branchSpec.getName()); - - if (matcher.find()) { - head = branchSpec.getName(); - rev = new AbstractGitSCMSource.SCMRevisionImpl(new SCMHead(headNameResult.headName), head); - } - } - - String refspec = config.getRefspec(); - if (refspec != null) { - if (env != null) { - refspec = env.expand(refspec); - } - } else { - refspec = "+" + headNameResult.prefix + headNameResult.headName + ":" + Constants.R_REMOTES + remoteName + "/" - + headNameResult.headName; - } - - client.fetch_().prune(true).from(remoteURI, Collections.singletonList(new RefSpec(refspec))).execute(); + HeadNameResult headNameResult = HeadNameResult.calculate(branchSpec, rev, config.getRefspec(), env, remoteName); + client.fetch_().prune(true).from(remoteURI, Collections.singletonList(new RefSpec(headNameResult.refspec))).execute(); listener.getLogger().println("Done."); - return new GitSCMFileSystem(client, remote, head, (AbstractGitSCMSource.SCMRevisionImpl) rev); + return new GitSCMFileSystem(client, remote, headNameResult.remoteHeadName, (AbstractGitSCMSource.SCMRevisionImpl) headNameResult.rev); } finally { cacheLock.unlock(); } diff --git a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java index 41923f1d3e..19d542b42f 100644 --- a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java +++ b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java @@ -32,11 +32,15 @@ import hudson.plugins.git.GitException; import java.io.ByteArrayOutputStream; import java.io.File; +import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; import java.util.Iterator; +import java.util.List; import java.util.Set; import java.util.TreeSet; +import hudson.plugins.git.UserRemoteConfig; import jenkins.plugins.git.junit.jupiter.WithGitSampleRepo; import jenkins.scm.api.SCMFile; import jenkins.scm.api.SCMFileSystem; @@ -410,29 +414,72 @@ void create_SCMFileSystem_from_tag() throws Exception { sampleRepo.git("commit", "--all", "--message=dev"); sampleRepo.git("tag", "v1.0"); SCMFileSystem fs = SCMFileSystem.of(r.createFreeStyleProject(), new GitSCM(GitSCM.createRepoList(sampleRepo.toString(), null), Collections.singletonList(new BranchSpec("refs/tags/v1.0")), null, null, Collections.emptyList())); + assertEquals("modified", getFileContent(fs, "dir/subdir/file", "modified")); + } + + public static List createRepoListWithRefspec(String url, String refspec) { + List repoList = new ArrayList<>(); + repoList.add(new UserRemoteConfig(url, null, refspec, null)); + return repoList; + } + + public String getFileContent(SCMFileSystem fs, String path, String expectedContent) throws IOException, InterruptedException { assertThat(fs, notNullValue()); assertThat(fs.getRoot(), notNullValue()); - Iterable children = fs.getRoot().children(); - Iterator iterator = children.iterator(); - assertThat(iterator.hasNext(), is(true)); - SCMFile dir = iterator.next(); - assertThat(iterator.hasNext(), is(false)); - assertThat(dir.getName(), is("dir")); - assertThat(dir.getType(), is(SCMFile.Type.DIRECTORY)); - children = dir.children(); - iterator = children.iterator(); - assertThat(iterator.hasNext(), is(true)); - SCMFile subdir = iterator.next(); - assertThat(iterator.hasNext(), is(false)); - assertThat(subdir.getName(), is("subdir")); - assertThat(subdir.getType(), is(SCMFile.Type.DIRECTORY)); - children = subdir.children(); - iterator = children.iterator(); - assertThat(iterator.hasNext(), is(true)); - SCMFile file = iterator.next(); - assertThat(iterator.hasNext(), is(false)); - assertThat(file.getName(), is("file")); - assertThat(file.contentAsString(), is("modified")); + String[] segments = path.split("/"); + + SCMFile parent = fs.getRoot(); + SCMFile file = null; + for (String segment: segments) { + Iterable children = parent.children(); + Iterator iterator = children.iterator(); + if (segment == segments[segments.length - 1]) { + // file + assertThat(iterator.hasNext(), is(true)); + SCMFile t = iterator.next(); + assertThat(iterator.hasNext(), is(false)); + assertThat(t.getName(), is(segment)); + assertThat(t.getType(), is(SCMFile.Type.REGULAR_FILE)); + file = t; + } else { + // dir + assertThat(iterator.hasNext(), is(true)); + SCMFile dir = iterator.next(); + assertThat(iterator.hasNext(), is(false)); + assertThat(dir.getName(), is(segment)); + assertThat(dir.getType(), is(SCMFile.Type.DIRECTORY)); + parent = dir; + } + } + return file.contentAsString(); + } + + @Test + public void create_SCMFileSystem_from_commit() throws Exception { + sampleRepo.init(); + sampleRepo.git("checkout", "-b", "dev"); + sampleRepo.mkdirs("dir/subdir"); + sampleRepo.git("mv", "file", "dir/subdir/file"); + sampleRepo.write("dir/subdir/file", "modified"); + sampleRepo.git("commit", "--all", "--message=dev"); + String modifiedCommit = sampleRepo.head(); + sampleRepo.write("dir/subdir/file", "modified again"); + sampleRepo.git("commit", "--all", "--message=dev"); + SCMFileSystem fs = SCMFileSystem.of(r.createFreeStyleProject(), new GitSCM(createRepoListWithRefspec(sampleRepo.toString(), "dev"), Collections.singletonList(new BranchSpec(modifiedCommit)), null, null, Collections.emptyList())); + assertEquals(modifiedCommit, fs.getRevision().toString()); + assertEquals("modified", getFileContent(fs, "dir/subdir/file", "modified")); + } + + @Test + public void create_SCMFileSystem_from_FETCH_HEAD() throws Exception { + sampleRepo.init(); + sampleRepo.git("checkout", "-b", "dev"); + sampleRepo.mkdirs("dir/subdir"); + sampleRepo.git("mv", "file", "dir/subdir/file"); + sampleRepo.write("dir/subdir/file", "modified"); + sampleRepo.git("commit", "--all", "--message=dev"); + SCMFileSystem fs = SCMFileSystem.of(r.createFreeStyleProject(), new GitSCM(createRepoListWithRefspec(sampleRepo.toString(), "dev"), Collections.singletonList(new BranchSpec("FETCH_HEAD")), null, null, Collections.emptyList())); + assertEquals("modified", getFileContent(fs, "dir/subdir/file", "modified")); } @Test @@ -467,36 +514,88 @@ void filesystem_supports_descriptor() throws Exception { @Issue("JENKINS-42971") @Test - void calculate_head_name_with_env() throws Exception { - GitSCMFileSystem.BuilderImpl.HeadNameResult result1 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, - new EnvVars("BRANCH", "master-a")); + public void calculate_head_name_with_env() throws Exception { + String remote = "origin"; + GitSCMFileSystem.BuilderImpl.HeadNameResult result1 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, null, + new EnvVars("BRANCH", "master-a"), remote); assertEquals("master-a", result1.headName); - assertEquals(Constants.R_HEADS, result1.prefix); + assertTrue(result1.refspec.startsWith("+" + Constants.R_HEADS)); - GitSCMFileSystem.BuilderImpl.HeadNameResult result2 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, - new EnvVars("BRANCH", "refs/heads/master-b")); + GitSCMFileSystem.BuilderImpl.HeadNameResult result2 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, null, + new EnvVars("BRANCH", "refs/heads/master-b"), remote); assertEquals("master-b", result2.headName); - assertEquals(Constants.R_HEADS, result2.prefix); + assertTrue(result2.refspec.startsWith("+" + Constants.R_HEADS)); - GitSCMFileSystem.BuilderImpl.HeadNameResult result3 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("refs/heads/${BRANCH}"), null, - new EnvVars("BRANCH", "master-c")); + GitSCMFileSystem.BuilderImpl.HeadNameResult result3 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("refs/heads/${BRANCH}"), null,null, + new EnvVars("BRANCH", "master-c"), remote); assertEquals("master-c", result3.headName); - assertEquals(Constants.R_HEADS, result3.prefix); + assertTrue(result3.refspec.startsWith("+" + Constants.R_HEADS)); - GitSCMFileSystem.BuilderImpl.HeadNameResult result4 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, - null); + GitSCMFileSystem.BuilderImpl.HeadNameResult result4 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, null, + null, remote); assertEquals("${BRANCH}", result4.headName); - assertEquals(Constants.R_HEADS, result4.prefix); + assertTrue(result4.refspec.startsWith("+" + Constants.R_HEADS)); - GitSCMFileSystem.BuilderImpl.HeadNameResult result5 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("*/${BRANCH}"), null, - new EnvVars("BRANCH", "master-d")); + GitSCMFileSystem.BuilderImpl.HeadNameResult result5 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("*/${BRANCH}"), null, null, + new EnvVars("BRANCH", "master-d"), remote); assertEquals("master-d", result5.headName); - assertEquals(Constants.R_HEADS, result5.prefix); + assertTrue(result5.refspec.startsWith("+" + Constants.R_HEADS)); - GitSCMFileSystem.BuilderImpl.HeadNameResult result6 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("*/master-e"), null, - new EnvVars("BRANCH", "dummy")); + GitSCMFileSystem.BuilderImpl.HeadNameResult result6 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("*/master-e"), null, null, + new EnvVars("BRANCH", "dummy"), remote); assertEquals("master-e", result6.headName); - assertEquals(Constants.R_HEADS, result6.prefix); + assertTrue(result6.refspec.startsWith("+" + Constants.R_HEADS)); + } + + @Test + public void calculate_head_name() throws Exception { + String remote = "origin"; + GitSCMFileSystem.BuilderImpl.HeadNameResult result1 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("branch"), null, null, null, remote); + assertEquals("branch", result1.headName); + assertEquals("refs/remotes/origin/branch", result1.remoteHeadName); + assertEquals("+refs/heads/branch:refs/remotes/origin/branch", result1.refspec); + + GitSCMFileSystem.BuilderImpl.HeadNameResult result2 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("refs/heads/branch"), null, null, null, remote); + assertEquals("branch", result2.headName); + assertEquals("refs/remotes/origin/branch", result2.remoteHeadName); + assertEquals("+refs/heads/branch:refs/remotes/origin/branch", result2.refspec); + + GitSCMFileSystem.BuilderImpl.HeadNameResult result3 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("refs/tags/my-tag"), null, null, null, remote); + assertEquals("my-tag", result3.headName); + assertEquals("refs/remotes/origin/my-tag", result3.remoteHeadName); + assertEquals("+refs/tags/my-tag:refs/remotes/origin/my-tag", result3.refspec); + } + + @Test + public void calculate_head_name_with_refspec_commit() throws Exception { + String remote = "origin"; + String commit = "0123456789" + "0123456789" + "0123456789" + "0123456789"; + String branch = "branch"; + GitSCMFileSystem.BuilderImpl.HeadNameResult result1 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec(commit), null, branch, null, remote); + assertEquals(commit, result1.headName); + assertEquals(commit, result1.remoteHeadName); + assertEquals(branch, result1.refspec); + + GitSCMFileSystem.BuilderImpl.HeadNameResult result2 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, "${REFSPEC}", + new EnvVars("BRANCH", commit, "REFSPEC", branch), remote); + assertEquals(commit, result2.headName); + assertEquals(commit, result2.remoteHeadName); + assertEquals(branch, result2.refspec); + } + + @Test + public void calculate_head_name_with_refspec_FETCH_HEAD() throws Exception { + String remote = "origin"; + GitSCMFileSystem.BuilderImpl.HeadNameResult result1 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("FETCH_HEAD"), null, "refs/changes/1/2/3", null, remote); + assertEquals("FETCH_HEAD", result1.headName); + assertEquals("FETCH_HEAD", result1.remoteHeadName); + assertEquals("refs/changes/1/2/3", result1.refspec); + + GitSCMFileSystem.BuilderImpl.HeadNameResult result2 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, "${REFSPEC}", + new EnvVars("BRANCH", "FETCH_HEAD", "REFSPEC", "refs/changes/1/2/3"), remote); + assertEquals("FETCH_HEAD", result2.headName); + assertEquals("FETCH_HEAD", result2.remoteHeadName); + assertEquals("refs/changes/1/2/3", result2.refspec); } /* GitSCMFileSystem in git plugin 4.14.0 reported a null pointer @@ -509,8 +608,8 @@ void null_pointer_exception() throws Exception { ObjectId git260 = client.revParse(GIT_2_6_0_TAG); AbstractGitSCMSource.SCMRevisionImpl rev260 = new AbstractGitSCMSource.SCMRevisionImpl(new SCMHead("origin"), git260.getName()); - GitSCMFileSystem.BuilderImpl.HeadNameResult result1 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("master-f"), rev260, null); + GitSCMFileSystem.BuilderImpl.HeadNameResult result1 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("master-f"), rev260, null, null, "origin"); assertEquals("master-f", result1.headName); - assertEquals(Constants.R_HEADS, result1.prefix); + assertTrue(result1.refspec.startsWith("+" + Constants.R_HEADS)); } } From 449bdb5930a78f90974b458600dd22b04cfb2d1b Mon Sep 17 00:00:00 2001 From: kuc2hi Date: Wed, 21 Jun 2023 13:03:55 +0200 Subject: [PATCH 04/16] fix: Use .equals() for prefix String comparison Replace reference equality (==) with value equality (.equals()) when comparing the prefix against Constants.R_HEADS and Constants.R_TAGS. The == operator may fail for string values that are equal but not the same object instance. --- src/main/java/jenkins/plugins/git/GitSCMFileSystem.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java index 8b85521085..7abf39de84 100644 --- a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java +++ b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java @@ -361,7 +361,7 @@ static HeadNameResult calculate(@NonNull BranchSpec branchSpec, } String remoteHead = headName; - if (prefix == Constants.R_HEADS || prefix == Constants.R_TAGS) { + if (prefix.equals(Constants.R_HEADS) || prefix.equals(Constants.R_TAGS)) { remoteHead = Constants.R_REMOTES + remoteName + "/" + headName; } From 4c4118adf544f2e6af45686e8911c9f22e767d2f Mon Sep 17 00:00:00 2001 From: kuc2hi Date: Wed, 21 Jun 2023 15:00:25 +0200 Subject: [PATCH 05/16] fix: Add null guard on prefix before .equals() call The prefix variable can be null when the branch specification is FETCH_HEAD or a raw commit hash. Add a null check before calling .equals() to prevent a NullPointerException. --- src/main/java/jenkins/plugins/git/GitSCMFileSystem.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java index 7abf39de84..ddf85d333b 100644 --- a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java +++ b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java @@ -361,7 +361,7 @@ static HeadNameResult calculate(@NonNull BranchSpec branchSpec, } String remoteHead = headName; - if (prefix.equals(Constants.R_HEADS) || prefix.equals(Constants.R_TAGS)) { + if (prefix != null && (prefix.equals(Constants.R_HEADS) || prefix.equals(Constants.R_TAGS))) { remoteHead = Constants.R_REMOTES + remoteName + "/" + headName; } From a393a8655e047f815cba108120a8d25f4ebf998e Mon Sep 17 00:00:00 2001 From: kuc2hi Date: Fri, 23 Jun 2023 16:15:28 +0200 Subject: [PATCH 06/16] refactor: Use Constants.FETCH_HEAD instead of hardcoded string Replace the hardcoded "FETCH_HEAD" string literal with the JGit constant Constants.FETCH_HEAD in both production code and tests. This improves maintainability and ensures consistency if the constant value ever changes. --- .../java/jenkins/plugins/git/GitSCMFileSystem.java | 2 +- .../jenkins/plugins/git/GitSCMFileSystemTest.java | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java index ddf85d333b..aab2a8b94f 100644 --- a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java +++ b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java @@ -329,7 +329,7 @@ static HeadNameResult calculate(@NonNull BranchSpec branchSpec, prefix = "refs/changes/"; } else { // check for FETCH_HEAD - if (branchSpecExpandedName.equals("FETCH_HEAD") && !refspecExpandedName.equals("")) { + if (branchSpecExpandedName.equals(Constants.FETCH_HEAD) && !refspecExpandedName.equals("")) { prefix = null; } else { // check for commit-id diff --git a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java index 19d542b42f..781114e308 100644 --- a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java +++ b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java @@ -478,7 +478,7 @@ public void create_SCMFileSystem_from_FETCH_HEAD() throws Exception { sampleRepo.git("mv", "file", "dir/subdir/file"); sampleRepo.write("dir/subdir/file", "modified"); sampleRepo.git("commit", "--all", "--message=dev"); - SCMFileSystem fs = SCMFileSystem.of(r.createFreeStyleProject(), new GitSCM(createRepoListWithRefspec(sampleRepo.toString(), "dev"), Collections.singletonList(new BranchSpec("FETCH_HEAD")), null, null, Collections.emptyList())); + SCMFileSystem fs = SCMFileSystem.of(r.createFreeStyleProject(), new GitSCM(createRepoListWithRefspec(sampleRepo.toString(), "dev"), Collections.singletonList(new BranchSpec(Constants.FETCH_HEAD)), null, null, Collections.emptyList())); assertEquals("modified", getFileContent(fs, "dir/subdir/file", "modified")); } @@ -586,15 +586,15 @@ public void calculate_head_name_with_refspec_commit() throws Exception { @Test public void calculate_head_name_with_refspec_FETCH_HEAD() throws Exception { String remote = "origin"; - GitSCMFileSystem.BuilderImpl.HeadNameResult result1 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("FETCH_HEAD"), null, "refs/changes/1/2/3", null, remote); - assertEquals("FETCH_HEAD", result1.headName); - assertEquals("FETCH_HEAD", result1.remoteHeadName); + GitSCMFileSystem.BuilderImpl.HeadNameResult result1 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec(Constants.FETCH_HEAD), null, "refs/changes/1/2/3", null, remote); + assertEquals(Constants.FETCH_HEAD, result1.headName); + assertEquals(Constants.FETCH_HEAD, result1.remoteHeadName); assertEquals("refs/changes/1/2/3", result1.refspec); GitSCMFileSystem.BuilderImpl.HeadNameResult result2 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, "${REFSPEC}", - new EnvVars("BRANCH", "FETCH_HEAD", "REFSPEC", "refs/changes/1/2/3"), remote); - assertEquals("FETCH_HEAD", result2.headName); - assertEquals("FETCH_HEAD", result2.remoteHeadName); + new EnvVars("BRANCH", Constants.FETCH_HEAD, "REFSPEC", "refs/changes/1/2/3"), remote); + assertEquals(Constants.FETCH_HEAD, result2.headName); + assertEquals(Constants.FETCH_HEAD, result2.remoteHeadName); assertEquals("refs/changes/1/2/3", result2.refspec); } From cf547a21335aeb7643449e44a9140299119ba480 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 1 Jul 2023 12:06:59 -0600 Subject: [PATCH 07/16] test: Refactor GitSCMFileSystemTest helpers for readability Extract a getFileContent() helper method to reduce duplication in the file tree traversal assertions across multiple test methods. Move createRepoListWithRefspec() helper adjacent to it. Add new test methods: - create_SCMFileSystem_from_commit: verify checkout at a specific commit revision - create_SCMFileSystem_from_FETCH_HEAD: verify checkout using a custom refspec with FETCH_HEAD --- .../plugins/git/GitSCMFileSystemTest.java | 57 +++++++++---------- 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java index 781114e308..a245fe8560 100644 --- a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java +++ b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java @@ -417,43 +417,38 @@ void create_SCMFileSystem_from_tag() throws Exception { assertEquals("modified", getFileContent(fs, "dir/subdir/file", "modified")); } - public static List createRepoListWithRefspec(String url, String refspec) { - List repoList = new ArrayList<>(); - repoList.add(new UserRemoteConfig(url, null, refspec, null)); - return repoList; - } - public String getFileContent(SCMFileSystem fs, String path, String expectedContent) throws IOException, InterruptedException { assertThat(fs, notNullValue()); assertThat(fs.getRoot(), notNullValue()); - String[] segments = path.split("/"); - - SCMFile parent = fs.getRoot(); - SCMFile file = null; - for (String segment: segments) { - Iterable children = parent.children(); - Iterator iterator = children.iterator(); - if (segment == segments[segments.length - 1]) { - // file - assertThat(iterator.hasNext(), is(true)); - SCMFile t = iterator.next(); - assertThat(iterator.hasNext(), is(false)); - assertThat(t.getName(), is(segment)); - assertThat(t.getType(), is(SCMFile.Type.REGULAR_FILE)); - file = t; - } else { - // dir - assertThat(iterator.hasNext(), is(true)); - SCMFile dir = iterator.next(); - assertThat(iterator.hasNext(), is(false)); - assertThat(dir.getName(), is(segment)); - assertThat(dir.getType(), is(SCMFile.Type.DIRECTORY)); - parent = dir; - } - } + Iterable children = fs.getRoot().children(); + Iterator iterator = children.iterator(); + assertThat(iterator.hasNext(), is(true)); + SCMFile dir = iterator.next(); + assertThat(iterator.hasNext(), is(false)); + assertThat(dir.getName(), is("dir")); + assertThat(dir.getType(), is(SCMFile.Type.DIRECTORY)); + children = dir.children(); + iterator = children.iterator(); + assertThat(iterator.hasNext(), is(true)); + SCMFile subdir = iterator.next(); + assertThat(iterator.hasNext(), is(false)); + assertThat(subdir.getName(), is("subdir")); + assertThat(subdir.getType(), is(SCMFile.Type.DIRECTORY)); + children = subdir.children(); + iterator = children.iterator(); + assertThat(iterator.hasNext(), is(true)); + SCMFile file = iterator.next(); + assertThat(iterator.hasNext(), is(false)); + assertThat(file.getName(), is("file")); return file.contentAsString(); } + public static List createRepoListWithRefspec(String url, String refspec) { + List repoList = new ArrayList<>(); + repoList.add(new UserRemoteConfig(url, null, refspec, null)); + return repoList; + } + @Test public void create_SCMFileSystem_from_commit() throws Exception { sampleRepo.init(); From 57dad31750e21a6c03012c059c2908a232ab9145 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 1 Jul 2023 16:07:01 -0600 Subject: [PATCH 08/16] test: Add static import for HeadNameResult in tests Add static import for GitSCMFileSystem.BuilderImpl.HeadNameResult and use the short name throughout calculate_head_name_with_env(). This reduces line length and improves readability of the test assertions. --- .../plugins/git/GitSCMFileSystemTest.java | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java index a245fe8560..f7de1e3eb7 100644 --- a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java +++ b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java @@ -72,6 +72,8 @@ import static org.junit.jupiter.api.Assertions.*; import static org.hamcrest.MatcherAssert.assertThat; +import jenkins.plugins.git.GitSCMFileSystem.BuilderImpl.HeadNameResult; + /** * Tests for {@link AbstractGitSCMSource} */ @@ -511,33 +513,27 @@ void filesystem_supports_descriptor() throws Exception { @Test public void calculate_head_name_with_env() throws Exception { String remote = "origin"; - GitSCMFileSystem.BuilderImpl.HeadNameResult result1 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, null, - new EnvVars("BRANCH", "master-a"), remote); + HeadNameResult result1 = HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, null, new EnvVars("BRANCH", "master-a"), remote); assertEquals("master-a", result1.headName); assertTrue(result1.refspec.startsWith("+" + Constants.R_HEADS)); - GitSCMFileSystem.BuilderImpl.HeadNameResult result2 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, null, - new EnvVars("BRANCH", "refs/heads/master-b"), remote); + HeadNameResult result2 = HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, null, new EnvVars("BRANCH", "refs/heads/master-b"), remote); assertEquals("master-b", result2.headName); assertTrue(result2.refspec.startsWith("+" + Constants.R_HEADS)); - GitSCMFileSystem.BuilderImpl.HeadNameResult result3 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("refs/heads/${BRANCH}"), null,null, - new EnvVars("BRANCH", "master-c"), remote); + HeadNameResult result3 = HeadNameResult.calculate(new BranchSpec("refs/heads/${BRANCH}"), null, null, new EnvVars("BRANCH", "master-c"), remote); assertEquals("master-c", result3.headName); assertTrue(result3.refspec.startsWith("+" + Constants.R_HEADS)); - GitSCMFileSystem.BuilderImpl.HeadNameResult result4 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, null, - null, remote); + HeadNameResult result4 = HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, null, null, remote); assertEquals("${BRANCH}", result4.headName); assertTrue(result4.refspec.startsWith("+" + Constants.R_HEADS)); - GitSCMFileSystem.BuilderImpl.HeadNameResult result5 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("*/${BRANCH}"), null, null, - new EnvVars("BRANCH", "master-d"), remote); + HeadNameResult result5 = HeadNameResult.calculate(new BranchSpec("*/${BRANCH}"), null, null, new EnvVars("BRANCH", "master-d"), remote); assertEquals("master-d", result5.headName); assertTrue(result5.refspec.startsWith("+" + Constants.R_HEADS)); - GitSCMFileSystem.BuilderImpl.HeadNameResult result6 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("*/master-e"), null, null, - new EnvVars("BRANCH", "dummy"), remote); + HeadNameResult result6 = HeadNameResult.calculate(new BranchSpec("*/master-e"), null, null, new EnvVars("BRANCH", "dummy"), remote); assertEquals("master-e", result6.headName); assertTrue(result6.refspec.startsWith("+" + Constants.R_HEADS)); } From a5d69f946afcfbe648625f5ad4e66451d712b903 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 1 Jul 2023 16:09:18 -0600 Subject: [PATCH 09/16] chore: Suppress spotbugs URF_UNREAD_FIELD for HeadNameResult The headName field in HeadNameResult is reported as unread by spotbugs (URF_UNREAD_FIELD). Suppress this warning temporarily while investigating the proper resolution. Note: This exclusion is removed in a subsequent commit once the field is properly used in logging output. --- src/spotbugs/excludesFilter.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/spotbugs/excludesFilter.xml b/src/spotbugs/excludesFilter.xml index bf1e7ef977..27bf5a1b76 100644 --- a/src/spotbugs/excludesFilter.xml +++ b/src/spotbugs/excludesFilter.xml @@ -74,4 +74,9 @@ + + + + + From 79c9477e680ad0b90b3ee09d0fd2baaef43d8db8 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 1 Jul 2023 16:16:58 -0600 Subject: [PATCH 10/16] style: Rename headName to calculatedHeadName for clarity Rename the local variable headName to calculatedHeadName inside HeadNameResult.calculate() to avoid confusion with the HeadNameResult.headName field. This makes the code easier to understand by distinguishing the temporary local value from the result object's field of the same name. --- .../jenkins/plugins/git/GitSCMFileSystem.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java index aab2a8b94f..235d180363 100644 --- a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java +++ b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java @@ -345,27 +345,27 @@ static HeadNameResult calculate(@NonNull BranchSpec branchSpec, } } - String headName = branchSpecExpandedName; + String calculatedHeadName = branchSpecExpandedName; if (rev != null && env != null) { - headName = env.expand(rev.getHead().getName()); + calculatedHeadName = env.expand(rev.getHead().getName()); } else { if (prefix != null && branchSpecExpandedName.startsWith(prefix)) { - headName = branchSpecExpandedName.substring(prefix.length()); + calculatedHeadName = branchSpecExpandedName.substring(prefix.length()); } else if (branchSpecExpandedName.startsWith("*/")) { - headName = branchSpecExpandedName.substring(2); + calculatedHeadName = branchSpecExpandedName.substring(2); } } if (refspecExpandedName == null || refspecExpandedName.equals("")) { - refspecExpandedName = "+" + prefix + headName + ":" + Constants.R_REMOTES + remoteName + "/" + headName; + refspecExpandedName = "+" + prefix + calculatedHeadName + ":" + Constants.R_REMOTES + remoteName + "/" + calculatedHeadName; } - String remoteHead = headName; + String remoteHead = calculatedHeadName; if (prefix != null && (prefix.equals(Constants.R_HEADS) || prefix.equals(Constants.R_TAGS))) { - remoteHead = Constants.R_REMOTES + remoteName + "/" + headName; + remoteHead = Constants.R_REMOTES + remoteName + "/" + calculatedHeadName; } - return new HeadNameResult(headName, remoteHead, refspecExpandedName, rev); + return new HeadNameResult(calculatedHeadName, remoteHead, refspecExpandedName, rev); } } From 882082b768b059fab05ffe261745d319ac14bf8b Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 1 Jul 2023 16:18:56 -0600 Subject: [PATCH 11/16] fix: Log resolved head name during lightweight checkout fetch Include the remote name and resolved head name in the "Done." log message so users can see which branch or commit was actually used during the fetch operation. Also remove the spotbugs URF_UNREAD_FIELD exclusion for HeadNameResult.headName, which is now properly accessed through the logging statement. --- src/main/java/jenkins/plugins/git/GitSCMFileSystem.java | 2 +- src/spotbugs/excludesFilter.xml | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java index 235d180363..3796c1a683 100644 --- a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java +++ b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java @@ -445,7 +445,7 @@ public SCMFileSystem build(@NonNull Item owner, @NonNull SCM scm, @CheckForNull HeadNameResult headNameResult = HeadNameResult.calculate(branchSpec, rev, config.getRefspec(), env, remoteName); client.fetch_().prune(true).from(remoteURI, Collections.singletonList(new RefSpec(headNameResult.refspec))).execute(); - listener.getLogger().println("Done."); + listener.getLogger().println("Done with " + remoteName + " using " + headNameResult.headName + "."); return new GitSCMFileSystem(client, remote, headNameResult.remoteHeadName, (AbstractGitSCMSource.SCMRevisionImpl) headNameResult.rev); } finally { cacheLock.unlock(); diff --git a/src/spotbugs/excludesFilter.xml b/src/spotbugs/excludesFilter.xml index 27bf5a1b76..bf1e7ef977 100644 --- a/src/spotbugs/excludesFilter.xml +++ b/src/spotbugs/excludesFilter.xml @@ -74,9 +74,4 @@ - - - - - From 05c78fbdb3546ad9536151bd666523ac8a6b97e3 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 1 Jul 2023 16:21:39 -0600 Subject: [PATCH 12/16] fix: Change refSpec parameter from @NonNull to @CheckForNull Update the refSpec parameter annotation in HeadNameResult.calculate() from @NonNull to @CheckForNull. The existing tests pass null for this parameter, and the implementation already handles null correctly by constructing a default refspec when none is provided. --- src/main/java/jenkins/plugins/git/GitSCMFileSystem.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java index 3796c1a683..b3b7227ffd 100644 --- a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java +++ b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java @@ -307,7 +307,7 @@ private HeadNameResult(String headName, String remoteHeadName, String refspec, S static HeadNameResult calculate(@NonNull BranchSpec branchSpec, @CheckForNull SCMRevision rev, - @NonNull String refSpec, + @CheckForNull String refSpec, @CheckForNull EnvVars env, @CheckForNull String remoteName) { From fefac72dc3e3b16787a488266d985e151cc6a024 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sun, 2 Jul 2023 07:02:53 -0600 Subject: [PATCH 13/16] test: Use HeadNameResult short name in remaining tests Extend the static import of HeadNameResult to cover calculate_head_name(), calculate_head_name_with_refspec_commit(), calculate_head_name_with_refspec_FETCH_HEAD(), and calculate_revision_head_name() test methods. This completes the cleanup started in an earlier commit. --- .../plugins/git/GitSCMFileSystemTest.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java index f7de1e3eb7..b36639027b 100644 --- a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java +++ b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java @@ -541,17 +541,17 @@ public void calculate_head_name_with_env() throws Exception { @Test public void calculate_head_name() throws Exception { String remote = "origin"; - GitSCMFileSystem.BuilderImpl.HeadNameResult result1 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("branch"), null, null, null, remote); + HeadNameResult result1 = HeadNameResult.calculate(new BranchSpec("branch"), null, null, null, remote); assertEquals("branch", result1.headName); assertEquals("refs/remotes/origin/branch", result1.remoteHeadName); assertEquals("+refs/heads/branch:refs/remotes/origin/branch", result1.refspec); - GitSCMFileSystem.BuilderImpl.HeadNameResult result2 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("refs/heads/branch"), null, null, null, remote); + HeadNameResult result2 = HeadNameResult.calculate(new BranchSpec("refs/heads/branch"), null, null, null, remote); assertEquals("branch", result2.headName); assertEquals("refs/remotes/origin/branch", result2.remoteHeadName); assertEquals("+refs/heads/branch:refs/remotes/origin/branch", result2.refspec); - GitSCMFileSystem.BuilderImpl.HeadNameResult result3 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("refs/tags/my-tag"), null, null, null, remote); + HeadNameResult result3 = HeadNameResult.calculate(new BranchSpec("refs/tags/my-tag"), null, null, null, remote); assertEquals("my-tag", result3.headName); assertEquals("refs/remotes/origin/my-tag", result3.remoteHeadName); assertEquals("+refs/tags/my-tag:refs/remotes/origin/my-tag", result3.refspec); @@ -562,12 +562,12 @@ public void calculate_head_name_with_refspec_commit() throws Exception { String remote = "origin"; String commit = "0123456789" + "0123456789" + "0123456789" + "0123456789"; String branch = "branch"; - GitSCMFileSystem.BuilderImpl.HeadNameResult result1 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec(commit), null, branch, null, remote); + HeadNameResult result1 = HeadNameResult.calculate(new BranchSpec(commit), null, branch, null, remote); assertEquals(commit, result1.headName); assertEquals(commit, result1.remoteHeadName); assertEquals(branch, result1.refspec); - GitSCMFileSystem.BuilderImpl.HeadNameResult result2 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, "${REFSPEC}", + HeadNameResult result2 = HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, "${REFSPEC}", new EnvVars("BRANCH", commit, "REFSPEC", branch), remote); assertEquals(commit, result2.headName); assertEquals(commit, result2.remoteHeadName); @@ -577,12 +577,12 @@ public void calculate_head_name_with_refspec_commit() throws Exception { @Test public void calculate_head_name_with_refspec_FETCH_HEAD() throws Exception { String remote = "origin"; - GitSCMFileSystem.BuilderImpl.HeadNameResult result1 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec(Constants.FETCH_HEAD), null, "refs/changes/1/2/3", null, remote); + HeadNameResult result1 = HeadNameResult.calculate(new BranchSpec(Constants.FETCH_HEAD), null, "refs/changes/1/2/3", null, remote); assertEquals(Constants.FETCH_HEAD, result1.headName); assertEquals(Constants.FETCH_HEAD, result1.remoteHeadName); assertEquals("refs/changes/1/2/3", result1.refspec); - GitSCMFileSystem.BuilderImpl.HeadNameResult result2 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, "${REFSPEC}", + HeadNameResult result2 = HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, "${REFSPEC}", new EnvVars("BRANCH", Constants.FETCH_HEAD, "REFSPEC", "refs/changes/1/2/3"), remote); assertEquals(Constants.FETCH_HEAD, result2.headName); assertEquals(Constants.FETCH_HEAD, result2.remoteHeadName); @@ -599,7 +599,7 @@ void null_pointer_exception() throws Exception { ObjectId git260 = client.revParse(GIT_2_6_0_TAG); AbstractGitSCMSource.SCMRevisionImpl rev260 = new AbstractGitSCMSource.SCMRevisionImpl(new SCMHead("origin"), git260.getName()); - GitSCMFileSystem.BuilderImpl.HeadNameResult result1 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("master-f"), rev260, null, null, "origin"); + HeadNameResult result1 = HeadNameResult.calculate(new BranchSpec("master-f"), rev260, null, null, "origin"); assertEquals("master-f", result1.headName); assertTrue(result1.refspec.startsWith("+" + Constants.R_HEADS)); } From 97a27817b7eaa5f1248b26511edb0661e3a36135 Mon Sep 17 00:00:00 2001 From: kuc2hi Date: Fri, 7 Jul 2023 16:40:04 +0200 Subject: [PATCH 14/16] fix: Correct tag refspec to fetch tags into refs/tags/* Fix the refspec calculation for tag branches. Previously a lightweight checkout of a tag would generate a refspec mapping the tag to refs/remotes/origin/, which places the tag reference in the wrong namespace. Now tag refspecs use +refs/tags/:refs/tags/, keeping tag references in the correct refs/tags/ namespace. The remoteHeadName for tags is also corrected to use just the tag name instead of the remote-tracking path. Additionally add a null guard when expanding refspec environment variables to prevent NPE when refspecExpandedName is null. --- .../java/jenkins/plugins/git/GitSCMFileSystem.java | 13 +++++++++---- .../jenkins/plugins/git/GitSCMFileSystemTest.java | 4 ++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java index b3b7227ffd..7107774f21 100644 --- a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java +++ b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java @@ -316,7 +316,7 @@ static HeadNameResult calculate(@NonNull BranchSpec branchSpec, branchSpecExpandedName = env.expand(branchSpecExpandedName); } String refspecExpandedName = refSpec; - if (env != null) { + if (env != null && refspecExpandedName != null) { refspecExpandedName = env.expand(refspecExpandedName); } @@ -329,7 +329,8 @@ static HeadNameResult calculate(@NonNull BranchSpec branchSpec, prefix = "refs/changes/"; } else { // check for FETCH_HEAD - if (branchSpecExpandedName.equals(Constants.FETCH_HEAD) && !refspecExpandedName.equals("")) { + if (branchSpecExpandedName.equals(Constants.FETCH_HEAD) && refspecExpandedName != null && + !refspecExpandedName.equals("")) { prefix = null; } else { // check for commit-id @@ -357,11 +358,15 @@ static HeadNameResult calculate(@NonNull BranchSpec branchSpec, } if (refspecExpandedName == null || refspecExpandedName.equals("")) { - refspecExpandedName = "+" + prefix + calculatedHeadName + ":" + Constants.R_REMOTES + remoteName + "/" + calculatedHeadName; + if (prefix.equals(Constants.R_TAGS)) { + refspecExpandedName = "+" + prefix + calculatedHeadName + ":" + prefix + calculatedHeadName; + } else { + refspecExpandedName = "+" + prefix + calculatedHeadName + ":" + Constants.R_REMOTES + remoteName + "/" + calculatedHeadName; + } } String remoteHead = calculatedHeadName; - if (prefix != null && (prefix.equals(Constants.R_HEADS) || prefix.equals(Constants.R_TAGS))) { + if (prefix != null && prefix.equals(Constants.R_HEADS)) { remoteHead = Constants.R_REMOTES + remoteName + "/" + calculatedHeadName; } diff --git a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java index b36639027b..f2879b5097 100644 --- a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java +++ b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java @@ -553,8 +553,8 @@ public void calculate_head_name() throws Exception { HeadNameResult result3 = HeadNameResult.calculate(new BranchSpec("refs/tags/my-tag"), null, null, null, remote); assertEquals("my-tag", result3.headName); - assertEquals("refs/remotes/origin/my-tag", result3.remoteHeadName); - assertEquals("+refs/tags/my-tag:refs/remotes/origin/my-tag", result3.refspec); + assertEquals("my-tag", result3.remoteHeadName); + assertEquals("+refs/tags/my-tag:refs/tags/my-tag", result3.refspec); } @Test From 66f2650c42eba57e6cf2210b7dbc6f59ae5b6034 Mon Sep 17 00:00:00 2001 From: kuc2hi Date: Fri, 7 Jul 2023 16:42:54 +0200 Subject: [PATCH 15/16] refactor: Remove redundant headName field from HeadNameResult The headName field in HeadNameResult was only used internally for constructing the remoteHeadName. Since remoteHeadName is now always a fully qualified reference (either refs/remotes// for branches or the raw name for tags/commits/FETCH_HEAD), the separate headName field is no longer needed. - Remove headName from HeadNameResult constructor and fields - Update callers to use remoteHeadName instead of headName - Update test assertions to verify remoteHeadName directly --- .../jenkins/plugins/git/GitSCMFileSystem.java | 8 +++---- .../plugins/git/GitSCMFileSystemTest.java | 21 +++++++------------ 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java index 7107774f21..071091a0fb 100644 --- a/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java +++ b/src/main/java/jenkins/plugins/git/GitSCMFileSystem.java @@ -293,13 +293,11 @@ public boolean supportsDescriptor(SCMSourceDescriptor descriptor) { } static class HeadNameResult { - final String headName; final String remoteHeadName; final String refspec; final SCMRevision rev; - private HeadNameResult(String headName, String remoteHeadName, String refspec, SCMRevision rev) { - this.headName = headName; + private HeadNameResult(String remoteHeadName, String refspec, SCMRevision rev) { this.remoteHeadName = remoteHeadName; this.refspec = refspec; this.rev = rev; @@ -370,7 +368,7 @@ static HeadNameResult calculate(@NonNull BranchSpec branchSpec, remoteHead = Constants.R_REMOTES + remoteName + "/" + calculatedHeadName; } - return new HeadNameResult(calculatedHeadName, remoteHead, refspecExpandedName, rev); + return new HeadNameResult(remoteHead, refspecExpandedName, rev); } } @@ -450,7 +448,7 @@ public SCMFileSystem build(@NonNull Item owner, @NonNull SCM scm, @CheckForNull HeadNameResult headNameResult = HeadNameResult.calculate(branchSpec, rev, config.getRefspec(), env, remoteName); client.fetch_().prune(true).from(remoteURI, Collections.singletonList(new RefSpec(headNameResult.refspec))).execute(); - listener.getLogger().println("Done with " + remoteName + " using " + headNameResult.headName + "."); + listener.getLogger().println("Done with " + remoteName + " using " + headNameResult.remoteHeadName + "."); return new GitSCMFileSystem(client, remote, headNameResult.remoteHeadName, (AbstractGitSCMSource.SCMRevisionImpl) headNameResult.rev); } finally { cacheLock.unlock(); diff --git a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java index f2879b5097..b062878e11 100644 --- a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java +++ b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java @@ -514,27 +514,27 @@ void filesystem_supports_descriptor() throws Exception { public void calculate_head_name_with_env() throws Exception { String remote = "origin"; HeadNameResult result1 = HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, null, new EnvVars("BRANCH", "master-a"), remote); - assertEquals("master-a", result1.headName); + assertEquals("refs/remotes/origin/master-a", result1.remoteHeadName); assertTrue(result1.refspec.startsWith("+" + Constants.R_HEADS)); HeadNameResult result2 = HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, null, new EnvVars("BRANCH", "refs/heads/master-b"), remote); - assertEquals("master-b", result2.headName); + assertEquals("refs/remotes/origin/master-b", result2.remoteHeadName); assertTrue(result2.refspec.startsWith("+" + Constants.R_HEADS)); HeadNameResult result3 = HeadNameResult.calculate(new BranchSpec("refs/heads/${BRANCH}"), null, null, new EnvVars("BRANCH", "master-c"), remote); - assertEquals("master-c", result3.headName); + assertEquals("refs/remotes/origin/master-c", result3.remoteHeadName); assertTrue(result3.refspec.startsWith("+" + Constants.R_HEADS)); HeadNameResult result4 = HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, null, null, remote); - assertEquals("${BRANCH}", result4.headName); + assertEquals("refs/remotes/origin/${BRANCH}", result4.remoteHeadName); assertTrue(result4.refspec.startsWith("+" + Constants.R_HEADS)); HeadNameResult result5 = HeadNameResult.calculate(new BranchSpec("*/${BRANCH}"), null, null, new EnvVars("BRANCH", "master-d"), remote); - assertEquals("master-d", result5.headName); + assertEquals("refs/remotes/origin/master-d", result5.remoteHeadName); assertTrue(result5.refspec.startsWith("+" + Constants.R_HEADS)); HeadNameResult result6 = HeadNameResult.calculate(new BranchSpec("*/master-e"), null, null, new EnvVars("BRANCH", "dummy"), remote); - assertEquals("master-e", result6.headName); + assertEquals("refs/remotes/origin/master-e", result6.remoteHeadName); assertTrue(result6.refspec.startsWith("+" + Constants.R_HEADS)); } @@ -542,17 +542,14 @@ public void calculate_head_name_with_env() throws Exception { public void calculate_head_name() throws Exception { String remote = "origin"; HeadNameResult result1 = HeadNameResult.calculate(new BranchSpec("branch"), null, null, null, remote); - assertEquals("branch", result1.headName); assertEquals("refs/remotes/origin/branch", result1.remoteHeadName); assertEquals("+refs/heads/branch:refs/remotes/origin/branch", result1.refspec); HeadNameResult result2 = HeadNameResult.calculate(new BranchSpec("refs/heads/branch"), null, null, null, remote); - assertEquals("branch", result2.headName); assertEquals("refs/remotes/origin/branch", result2.remoteHeadName); assertEquals("+refs/heads/branch:refs/remotes/origin/branch", result2.refspec); HeadNameResult result3 = HeadNameResult.calculate(new BranchSpec("refs/tags/my-tag"), null, null, null, remote); - assertEquals("my-tag", result3.headName); assertEquals("my-tag", result3.remoteHeadName); assertEquals("+refs/tags/my-tag:refs/tags/my-tag", result3.refspec); } @@ -563,13 +560,11 @@ public void calculate_head_name_with_refspec_commit() throws Exception { String commit = "0123456789" + "0123456789" + "0123456789" + "0123456789"; String branch = "branch"; HeadNameResult result1 = HeadNameResult.calculate(new BranchSpec(commit), null, branch, null, remote); - assertEquals(commit, result1.headName); assertEquals(commit, result1.remoteHeadName); assertEquals(branch, result1.refspec); HeadNameResult result2 = HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, "${REFSPEC}", new EnvVars("BRANCH", commit, "REFSPEC", branch), remote); - assertEquals(commit, result2.headName); assertEquals(commit, result2.remoteHeadName); assertEquals(branch, result2.refspec); } @@ -578,13 +573,11 @@ public void calculate_head_name_with_refspec_commit() throws Exception { public void calculate_head_name_with_refspec_FETCH_HEAD() throws Exception { String remote = "origin"; HeadNameResult result1 = HeadNameResult.calculate(new BranchSpec(Constants.FETCH_HEAD), null, "refs/changes/1/2/3", null, remote); - assertEquals(Constants.FETCH_HEAD, result1.headName); assertEquals(Constants.FETCH_HEAD, result1.remoteHeadName); assertEquals("refs/changes/1/2/3", result1.refspec); HeadNameResult result2 = HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, "${REFSPEC}", new EnvVars("BRANCH", Constants.FETCH_HEAD, "REFSPEC", "refs/changes/1/2/3"), remote); - assertEquals(Constants.FETCH_HEAD, result2.headName); assertEquals(Constants.FETCH_HEAD, result2.remoteHeadName); assertEquals("refs/changes/1/2/3", result2.refspec); } @@ -600,7 +593,7 @@ void null_pointer_exception() throws Exception { AbstractGitSCMSource.SCMRevisionImpl rev260 = new AbstractGitSCMSource.SCMRevisionImpl(new SCMHead("origin"), git260.getName()); HeadNameResult result1 = HeadNameResult.calculate(new BranchSpec("master-f"), rev260, null, null, "origin"); - assertEquals("master-f", result1.headName); + assertEquals("refs/remotes/origin/master-f", result1.remoteHeadName); assertTrue(result1.refspec.startsWith("+" + Constants.R_HEADS)); } } From 4c57de22ad3834c096e479ed0e973905680aa445 Mon Sep 17 00:00:00 2001 From: Michael Trimarchi Date: Sat, 27 Jun 2026 18:45:57 +0200 Subject: [PATCH 16/16] Fix GitSCMFileSystemTest due the api change Signed-off-by: Michael Trimarchi --- .../java/jenkins/plugins/git/GitSCMFileSystemTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java index b062878e11..6bef3b8ff7 100644 --- a/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java +++ b/src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java @@ -495,11 +495,11 @@ public void testSupportsGerritChangeRef() throws Exception { public void testHeadNameResultCalculateGerritChange() { BranchSpec spec = new BranchSpec("refs/changes/91/45391/1"); - GitSCMFileSystem.BuilderImpl.HeadNameResult result = - GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(spec, null, null); + HeadNameResult result = + HeadNameResult.calculate(spec, null, null, null, "origin"); - assertEquals("refs/changes/", result.prefix); - assertEquals("91/45391/1", result.headName); + assertEquals("91/45391/1", result.remoteHeadName); + assertTrue(result.refspec.startsWith("+refs/changes/")); } @Issue("JENKINS-52964")