[JENKINS-71506] Support custom refspec in lightweight checkout #1468
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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; | ||||||||||||||
|
|
@@ -287,39 +290,76 @@ 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; | ||||||||||||||
| this.remoteHeadName = remoteHeadName; | ||||||||||||||
| this.refspec = refspec; | ||||||||||||||
| this.rev = rev; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| static HeadNameResult calculate(@NonNull BranchSpec branchSpec, | ||||||||||||||
| @CheckForNull SCMRevision rev, | ||||||||||||||
| @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 { | ||||||||||||||
| // check for FETCH_HEAD | ||||||||||||||
| if (branchSpecExpandedName.equals(Constants.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()) { | ||||||||||||||
|
Comment on lines
+329
to
+333
There was a problem hiding this comment. There is an existing pattern for this here
Suggested change
|
||||||||||||||
| // commit-id | ||||||||||||||
| prefix = null; | ||||||||||||||
| rev = new AbstractGitSCMSource.SCMRevisionImpl(new SCMHead(branchSpecExpandedName), branchSpecExpandedName); | ||||||||||||||
|
There was a problem hiding this comment. Overriding arguments like that deserves at least a comment. |
||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| String headName = branchSpecExpandedName; | ||||||||||||||
| if (rev != null && env != null) { | ||||||||||||||
| headName = env.expand(rev.getHead().getName()); | ||||||||||||||
| } else { | ||||||||||||||
| if (prefix != null && branchSpecExpandedName.startsWith(prefix)) { | ||||||||||||||
| headName = branchSpecExpandedName.substring(prefix.length()); | ||||||||||||||
| } else if (branchSpecExpandedName.startsWith("*/")) { | ||||||||||||||
| headName = branchSpecExpandedName.substring(2); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if (refspecExpandedName == null || refspecExpandedName.equals("")) { | ||||||||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||||||||
| refspecExpandedName = "+" + prefix + headName + ":" + Constants.R_REMOTES + remoteName + "/" + headName; | ||||||||||||||
|
There was a problem hiding this comment. This line seems to generate an incorrect refspec expanded name when the In that same test case, the remoteHead value is I think that it would be desirable to handle tags, since the change is already adding the ability to handle other refspecs. There was a problem hiding this comment. Thanks for pointing out, fixed now |
||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| String remoteHead = headName; | ||||||||||||||
| if (prefix != null && (prefix.equals(Constants.R_HEADS) || prefix.equals(Constants.R_TAGS))) { | ||||||||||||||
| remoteHead = Constants.R_REMOTES + remoteName + "/" + headName; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| return new HeadNameResult(headName, remoteHead, refspecExpandedName, rev); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -399,14 +439,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, 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, headNameResult.remoteHeadName, (AbstractGitSCMSource.SCMRevisionImpl) headNameResult.rev); | ||||||||||||||
| } finally { | ||||||||||||||
| cacheLock.unlock(); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either the annotation is incorrect here or the tests are incorrect. The tests are passing a null
refSpecbut the annotation declares thatrefSpecis NonNull.I'm surprised that spotbugs did not detect that issue. I'll need to investigate further to understand if there is a suppression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refSpec should have the @checkfornull annotation, fixed now