Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-71506] Support custom refspec in lightweight checkout #1468

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
2a91c95
Support custom refspec in lightweight checkout
ckullabosch Jun 19, 2023
d20c716
Move logic inside HeadNameResult.calculate()
ckullabosch Jun 21, 2023
1fde396
[Jenkins-71506] Remove spotbugs finding
ckullabosch Jun 21, 2023
efed3af
Fix NPE
ckullabosch Jun 21, 2023
c64fd02
Make use of Constants.FETCH_HEAD
ckullabosch Jun 23, 2023
e3269b0
Merge branch 'master' into ckullabosch/master
MarkEWaite Jul 1, 2023
080cfb8
Accept 64 character SHA-256 as well
MarkEWaite Jul 1, 2023
fd64d80
Reduce diff for easier code review
MarkEWaite Jul 1, 2023
e6e957f
Revert "Accept 64 character SHA-256 as well"
MarkEWaite Jul 1, 2023
6d0daaf
Merge branch 'master' into ckullabosch/master
MarkEWaite Jul 1, 2023
fb0afc9
Use an import to simplify reading the test
MarkEWaite Jul 1, 2023
d21149d
Suppress a spotbugs warning as technical debt
MarkEWaite Jul 1, 2023
343d3f1
Rename local headName as calculatedHeadName for clarity
MarkEWaite Jul 1, 2023
244ba1f
Report value of headNameResult.headName
MarkEWaite Jul 1, 2023
1b881a6
Declare refSpec can be null in calculate
MarkEWaite Jul 1, 2023
ac489cf
Merge branch 'master' into ckullabosch/master
MarkEWaite Jul 2, 2023
e66e798
Simplify GitSCMFileSystemTest with import
MarkEWaite Jul 2, 2023
c40b747
Fix refspec calculation for tags
ckullabosch Jul 7, 2023
8878bbb
Remove headName field in HeadNameResult
ckullabosch Jul 7, 2023
6673ed0
Merge branch 'master' into ckullabosch/master
MarkEWaite Jul 8, 2023
78797e8
Merge branch 'master' into ckullabosch/master
MarkEWaite Jul 18, 2023
4c04395
Merge branch 'master' into ckullabosch/master
MarkEWaite Dec 29, 2023
7080cd6
Merge branch 'master' into ckullabosch/master
MarkEWaite Jul 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 61 additions & 21 deletions src/main/java/jenkins/plugins/git/GitSCMFileSystem.java
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,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;
Expand Down Expand Up @@ -286,40 +289,80 @@
}

static class HeadNameResult {
final String headName;
final String prefix;

private HeadNameResult(String headName, String prefix) {
this.headName = headName;
this.prefix = prefix;
final String remoteHeadName;
final String refspec;
final SCMRevision rev;

private HeadNameResult(String remoteHeadName, String refspec, SCMRevision rev) {
this.remoteHeadName = remoteHeadName;
this.refspec = refspec;
this.rev = rev;
}

static HeadNameResult calculate(@NonNull BranchSpec branchSpec,
@CheckForNull SCMRevision rev,
@CheckForNull EnvVars env) {
@CheckForNull 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 != null) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No harm calling env.expand on a null, this check is unnecessary

Suggested change
if (env != null && refspecExpandedName != null) {
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 {
// check for FETCH_HEAD
if (branchSpecExpandedName.equals(Constants.FETCH_HEAD) && refspecExpandedName != null &&

Check warning on line 324 in src/main/java/jenkins/plugins/git/GitSCMFileSystem.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 324 is only partially covered, one branch is missing
!refspecExpandedName.equals("")) {

Check warning on line 325 in src/main/java/jenkins/plugins/git/GitSCMFileSystem.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 325 is only partially covered, one branch is missing
Comment on lines +324 to +325

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have StringUtils imported, might as well use it

Suggested change
if (branchSpecExpandedName.equals(Constants.FETCH_HEAD) && refspecExpandedName != null &&
!refspecExpandedName.equals("")) {
if (branchSpecExpandedName.equals(Constants.FETCH_HEAD) && StringUtils.isNotBlank(refSpecExpandedName)) {

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()) {
Comment on lines +329 to +333

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an existing pattern for this here

Suggested change
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()) {
if (branchSpecExpandedName.matches("[0-9a-f]{6,40}")) {

// commit-id
prefix = null;
rev = new AbstractGitSCMSource.SCMRevisionImpl(new SCMHead(branchSpecExpandedName), branchSpecExpandedName);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overriding arguments like that deserves at least a comment.

}
}
}

String headName;
String calculatedHeadName = branchSpecExpandedName;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to rename this variable.

if (rev != null && env != null) {
headName = env.expand(rev.getHead().getName());
calculatedHeadName = env.expand(rev.getHead().getName());
} else {
if (branchSpecExpandedName.startsWith(prefix)) {
headName = branchSpecExpandedName.substring(prefix.length());
if (prefix != null && branchSpecExpandedName.startsWith(prefix)) {
calculatedHeadName = branchSpecExpandedName.substring(prefix.length());
} else if (branchSpecExpandedName.startsWith("*/")) {
headName = branchSpecExpandedName.substring(2);
calculatedHeadName = branchSpecExpandedName.substring(2);
}
}

if (refspecExpandedName == null || refspecExpandedName.equals("")) {

Check warning on line 352 in src/main/java/jenkins/plugins/git/GitSCMFileSystem.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 352 is only partially covered, one branch is missing

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringUtils again

Suggested change
if (refspecExpandedName == null || refspecExpandedName.equals("")) {
if (StringUtils.isBlank(refspecExpandedName)) {

if (prefix.equals(Constants.R_TAGS)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest adding a test with a sha1 and a null refspec. This if would need to be fixed

Suggested change
if (prefix.equals(Constants.R_TAGS)) {
if (prefix != null && prefix.equals(Constants.R_TAGS)) {

refspecExpandedName = "+" + prefix + calculatedHeadName + ":" + prefix + calculatedHeadName;
} else {
headName = branchSpecExpandedName;
refspecExpandedName = "+" + prefix + calculatedHeadName + ":" + Constants.R_REMOTES + remoteName + "/" + calculatedHeadName;
}
}
return new HeadNameResult(headName, prefix);

String remoteHead = calculatedHeadName;
if (prefix != null && prefix.equals(Constants.R_HEADS)) {
remoteHead = Constants.R_REMOTES + remoteName + "/" + calculatedHeadName;
}

return new HeadNameResult(remoteHead, refspecExpandedName, rev);
}
}

Expand Down Expand Up @@ -399,14 +442,11 @@
listener.getLogger().println("URI syntax exception for '" + remoteName + "' " + ex);
}

HeadNameResult headNameResult = HeadNameResult.calculate(branchSpec, rev, env);

client.fetch_().prune(true).from(remoteURI, Collections.singletonList(new RefSpec(
"+" + headNameResult.prefix + headNameResult.headName + ":" + Constants.R_REMOTES + remoteName + "/"
+ headNameResult.headName))).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, Constants.R_REMOTES + remoteName + "/" + headNameResult.headName, (AbstractGitSCMSource.SCMRevisionImpl) rev);
listener.getLogger().println("Done with " + remoteName + " using " + headNameResult.remoteHeadName + ".");
return new GitSCMFileSystem(client, remote, headNameResult.remoteHeadName, (AbstractGitSCMSource.SCMRevisionImpl) headNameResult.rev);
} finally {
cacheLock.unlock();
}
Expand Down
150 changes: 117 additions & 33 deletions src/test/java/jenkins/plugins/git/GitSCMFileSystemTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +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.scm.api.SCMFile;
import jenkins.scm.api.SCMFileSystem;
import jenkins.scm.api.SCMHead;
Expand Down Expand Up @@ -67,6 +72,8 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertTrue;

import jenkins.plugins.git.GitSCMFileSystem.BuilderImpl.HeadNameResult;

/**
* Tests for {@link AbstractGitSCMSource}
*/
Expand Down Expand Up @@ -396,6 +403,10 @@ public 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 String getFileContent(SCMFileSystem fs, String path, String expectedContent) throws IOException, InterruptedException {
assertThat(fs, notNullValue());
assertThat(fs.getRoot(), notNullValue());
Iterable<SCMFile> children = fs.getRoot().children();
Expand All @@ -418,7 +429,41 @@ public void create_SCMFileSystem_from_tag() throws Exception {
SCMFile file = iterator.next();
assertThat(iterator.hasNext(), is(false));
assertThat(file.getName(), is("file"));
assertThat(file.contentAsString(), is("modified"));
return file.contentAsString();
}

public static List<UserRemoteConfig> createRepoListWithRefspec(String url, String refspec) {
List<UserRemoteConfig> repoList = new ArrayList<>();
repoList.add(new UserRemoteConfig(url, null, refspec, null));
return repoList;
}

@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(Constants.FETCH_HEAD)), null, null, Collections.emptyList()));
assertEquals("modified", getFileContent(fs, "dir/subdir/file", "modified"));
}

@Issue("JENKINS-52964")
Expand All @@ -431,35 +476,74 @@ public void filesystem_supports_descriptor() throws Exception {
@Issue("JENKINS-42971")
@Test
public 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"));
assertEquals("master-a", result1.headName);
assertEquals(Constants.R_HEADS, result1.prefix);

GitSCMFileSystem.BuilderImpl.HeadNameResult result2 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null,
new EnvVars("BRANCH", "refs/heads/master-b"));
assertEquals("master-b", result2.headName);
assertEquals(Constants.R_HEADS, result2.prefix);

GitSCMFileSystem.BuilderImpl.HeadNameResult result3 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("refs/heads/${BRANCH}"), null,
new EnvVars("BRANCH", "master-c"));
assertEquals("master-c", result3.headName);
assertEquals(Constants.R_HEADS, result3.prefix);

GitSCMFileSystem.BuilderImpl.HeadNameResult result4 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null,
null);
assertEquals("${BRANCH}", result4.headName);
assertEquals(Constants.R_HEADS, result4.prefix);

GitSCMFileSystem.BuilderImpl.HeadNameResult result5 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("*/${BRANCH}"), null,
new EnvVars("BRANCH", "master-d"));
assertEquals("master-d", result5.headName);
assertEquals(Constants.R_HEADS, result5.prefix);

GitSCMFileSystem.BuilderImpl.HeadNameResult result6 = GitSCMFileSystem.BuilderImpl.HeadNameResult.calculate(new BranchSpec("*/master-e"), null,
new EnvVars("BRANCH", "dummy"));
assertEquals("master-e", result6.headName);
assertEquals(Constants.R_HEADS, result6.prefix);
String remote = "origin";
HeadNameResult result1 = HeadNameResult.calculate(new BranchSpec("${BRANCH}"), null, null, new EnvVars("BRANCH", "master-a"), remote);
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("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("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("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("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("refs/remotes/origin/master-e", result6.remoteHeadName);
assertTrue(result6.refspec.startsWith("+" + Constants.R_HEADS));
}

@Test
public void calculate_head_name() throws Exception {
String remote = "origin";
HeadNameResult result1 = HeadNameResult.calculate(new BranchSpec("branch"), null, null, null, remote);
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("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.remoteHeadName);
assertEquals("+refs/tags/my-tag:refs/tags/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";
HeadNameResult result1 = HeadNameResult.calculate(new BranchSpec(commit), null, branch, null, remote);
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.remoteHeadName);
assertEquals(branch, result2.refspec);
}

@Test
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.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.remoteHeadName);
assertEquals("refs/changes/1/2/3", result2.refspec);
}

/* GitSCMFileSystem in git plugin 4.14.0 reported a null pointer
Expand All @@ -472,9 +556,9 @@ public 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);
assertEquals("master-f", result1.headName);
assertEquals(Constants.R_HEADS, result1.prefix);
HeadNameResult result1 = HeadNameResult.calculate(new BranchSpec("master-f"), rev260, null, null, "origin");
assertEquals("refs/remotes/origin/master-f", result1.remoteHeadName);
assertTrue(result1.refspec.startsWith("+" + Constants.R_HEADS));
}

/** inline ${@link hudson.Functions#isWindows()} to prevent a transient remote classloader issue */
Expand Down
Loading