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

ckullabosch · 2023-06-19T12:48:26Z

[JENKINS-71506] Support custom refspec in lightweight checkout

Let user specify a custom refspec to fetch.
Add support for branch names FETCH_HEAD or a commit hash.

This allows to use lightweight checkout for:

Fetch the pipeline from a Gerrit change (refspec:
refs/changes/*/change/patchset and branch: FETCH_HEAD).
Fetch the pipeline from a fixed commit of a branch, instead of head (refspec: branch and branch: commit hash).

Checklist

Types of changes

New feature (non-breaking change which adds functionality)

Let user specify a custom refspec to fetch. Add support for branch names FETCH_HEAD or a commit hash. This allows to use lightweight checkout for: * Fetch the pipeline from a Gerrit change (refspec: refs/changes/*/change/patchset and branch: FETCH_HEAD). * Fetch the pipeline from a fixed commit of a branch, instead of head (refspec: branch and branch: commit hash).

ckullabosch · 2023-06-27T17:37:54Z

@MarkEWaite I think this PR is ready for review. Please let me know if I can do anything to drive the topic forward.

MarkEWaite · 2023-06-27T18:28:48Z

@MarkEWaite I think this PR is ready for review. Please let me know if I can do anything to drive the topic forward.

Thanks @ckullabosch . It will likely be a week before I'm able to review it due to other priorities. Weekend is coming that may make some time available.

MarkEWaite · 2023-06-27T18:38:05Z

It would be a great help if you could provide one or more examples in the README.adoc file that show how the custom refespec can be used. The most frequent request from Jenkins documentation readers is a request for more examples. The README file of the git plugin provides many examples in hopes that readers will find them useful. The documentation that you create will also be used as part of my testing of the pull request, so it will help me evaluate the change more effectively.

No need to apply that technique in one location when there are many others that have the same issue. This reverts commit 080cfb8.

MarkEWaite

I'd really like to have documentation included that provides an example using the new capabilities, either through a Pipeline example (if that applies) or through a configuration as code example.

I still need to perform interactive testing. My review of the tests and the code has not found any blocking issues, just questions that I need to answer when testing.

MarkEWaite · 2023-07-01T15:32:45Z

src/main/java/jenkins/plugins/git/GitSCMFileSystem.java

-                return new HeadNameResult(headName, prefix);
+
+                if (refspecExpandedName == null || refspecExpandedName.equals("")) {
+                    refspecExpandedName = "+" + prefix + headName + ":" + Constants.R_REMOTES + remoteName + "/" + headName;


This line seems to generate an incorrect refspec expanded name when the branchSpecExpandedName starts with refs/tags. In one of the tests, it generates a refspec +refs/tags/my-tag:refs/remotes/origin/my-tag when I believe the correct refspec for a tag would be +refs/tags/my-tag:refs/tags/my-tag

In that same test case, the remoteHead value is refs/remotes/origin/my-tag. That remoteHead seems like the correct reference as far as I can tell. At least, when I use a refspec +refs/tags/git-0.3:refs/tags/git-0.3 to fetch from the git plugin repository (after cloning with the --no-tags option), the git-0.3 tag is created in my local repository.

I think that it would be desirable to handle tags, since the change is already adding the ability to handle other refspecs.

Thanks for pointing out, fixed now

MarkEWaite · 2023-07-01T18:19:50Z

src/main/java/jenkins/plugins/git/GitSCMFileSystem.java

            }

            static HeadNameResult calculate(@NonNull BranchSpec branchSpec,
                                            @CheckForNull SCMRevision rev,
-                                            @CheckForNull EnvVars env) {
+                                            @NonNull String refSpec,


Either the annotation is incorrect here or the tests are incorrect. The tests are passing a null refSpec but the annotation declares that refSpec is NonNull.

I'm surprised that spotbugs did not detect that issue. I'll need to investigate further to understand if there is a suppression

Suggested change

@NonNull String refSpec,

@CheckForNull String refSpec,

refSpec should have the @checkfornull annotation, fixed now

MarkEWaite · 2023-07-01T21:52:11Z

With the additional spotbugs checks that were added in #1475 , there is now a spotbugs warning reported for the unread field HeadNameResult.headName. The report seems to be correct because the value of headName that is passed to the HeadNameResult constructor is ignored and replaced with a value computed from the branchSpec.

I suppressed the spotbugs warning in d21149d, but that should be temporary until the root issue can be identified and resolved.

Not clear to me how to resolve the spotbugs warning

Using the same string for the local as is used in the HeadNameResult class was confusing me. Rather than risk being confused when I read it again later, let's rename it now.

Silences a spotbugs warning and helps my testing

The tests are passing null and have encountered no error with the null argument

MarkEWaite · 2023-07-01T22:50:13Z

@ckullabosch I'll need more detailed instructions on the steps that I need to take in order to verify this works as expected. I have been unable to reach the modified code from my job definitions. I created a Pipeline, a multibranch Pipeline, and an organization folder and was unable in any of those cases to see the "Done with " message logged.

Can you provide more details of a way that I can verify the code using GitHub or Bitbucket or GitLab or Gitea? I don't have access to a Gerrit server.

The field was only needed to calculte remoteHeadName, which is now done inside the calculate() method.

ckullabosch · 2023-07-07T15:06:46Z

@MarkEWaite In order to try out the change you have to create a Pipeline job which loads the pipeline from SCM (git). You have to enable lightweight checkout (set the checkmark at the end of the job configuration page). Then you may want to experiment with different settings for refspec and branch. There is no need for a folder or multibranch project.

Settings which are possible now, but weren't before:

Have a refspec pointing to a branch, point the branch setting to a commit hash (not necessarily the tip of the branch). This lets you use a fixed commit which is not a ref, but contained in the history of a branch.
Have a refspec other than refs/tags and refs/heads and make use of "FETCH_HEAD" in the branch setting. The available refs depends on your git system (a branch like refs/heads/my-branch should do as well). In Gerrit you have the refs/changes/, probably other git systems have also special refs (I remember Bitbucket having refs for pull requests).

We need to clarify about improving the README. The lightweight checkout is not mentioned at all at the moment.

MarkEWaite · 2023-07-31T11:29:30Z

It will be 1-2 weeks before I can review and test this. We're in the last few weeks of Google Summer of Code and I put my efforts there.

ckullabosch · 2023-10-05T07:43:59Z

@MarkEWaite It's been a while, any chance we can conclude this PR? Please see latest comment of mine.

We need to clarify about improving the README. The lightweight checkout is not mentioned at all at the moment.

SinOverCos · 2023-11-16T04:12:53Z

@ckullabosch thank you for the PR, I've been looking for something like this.

@MarkEWaite any chance you could give this another pass?

MarkEWaite · 2023-11-17T19:05:42Z

@MarkEWaite any chance you could give this another pass?

As time allows, I'll do another review. @SinOverCos have you deployed the build of this change into your environment and confirmed that it works for you? It is much more persuasive for a maintainer like me when others users declare that they are using a pull request in production.

balakine

I really want to see this in merged. Perhaps reducing the line count would make it easier to review.

balakine · 2024-02-13T09:32:56Z

src/main/java/jenkins/plugins/git/GitSCMFileSystem.java

                String branchSpecExpandedName = branchSpec.getName();
                if (env != null) {
                    branchSpecExpandedName = env.expand(branchSpecExpandedName);
                }
+                String refspecExpandedName = refSpec;
+                if (env != null && refspecExpandedName != null) {


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

Suggested change

if (env != null && refspecExpandedName != null) {

if (env != null) {

balakine · 2024-02-13T15:12:32Z

src/main/java/jenkins/plugins/git/GitSCMFileSystem.java

+                    if (branchSpecExpandedName.equals(Constants.FETCH_HEAD) && refspecExpandedName != null &&
+                            !refspecExpandedName.equals("")) {


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)) {

balakine · 2024-02-13T15:14:28Z

src/main/java/jenkins/plugins/git/GitSCMFileSystem.java

+                        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()) {


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}")) {

balakine · 2024-02-13T15:16:15Z

src/main/java/jenkins/plugins/git/GitSCMFileSystem.java

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


StringUtils again

Suggested change

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

if (StringUtils.isBlank(refspecExpandedName)) {

balakine · 2024-02-13T15:18:50Z

src/main/java/jenkins/plugins/git/GitSCMFileSystem.java

+                }
+
+                if (refspecExpandedName == null || refspecExpandedName.equals("")) {
+                    if (prefix.equals(Constants.R_TAGS)) {


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)) {

balakine · 2024-02-13T15:28:00Z

src/main/java/jenkins/plugins/git/GitSCMFileSystem.java

+                        if (matcher.find()) {
+                            // commit-id
+                            prefix = null;
+                            rev = new AbstractGitSCMSource.SCMRevisionImpl(new SCMHead(branchSpecExpandedName), branchSpecExpandedName);


Overriding arguments like that deserves at least a comment.

balakine · 2024-02-13T15:36:46Z

src/main/java/jenkins/plugins/git/GitSCMFileSystem.java

                }

-                String headName;
+                String calculatedHeadName = branchSpecExpandedName;


We don't need to rename this variable.

ckullabosch added 3 commits June 19, 2023 09:06

Move logic inside HeadNameResult.calculate()

d20c716

[Jenkins-71506] Remove spotbugs finding

1fde396

ckullabosch changed the title ~~Support custom refspec in lightweight checkout~~ [Jenkins-71506] Support custom refspec in lightweight checkout Jun 21, 2023

ckullabosch added 2 commits June 21, 2023 15:00

Fix NPE

efed3af

Make use of Constants.FETCH_HEAD

c64fd02

MarkEWaite changed the title ~~[Jenkins-71506] Support custom refspec in lightweight checkout~~ [JENKINS-71506] Support custom refspec in lightweight checkout Jun 27, 2023

MarkEWaite added 4 commits July 1, 2023 08:39

Merge branch 'master' into ckullabosch/master

e3269b0

Accept 64 character SHA-256 as well

080cfb8

Reduce diff for easier code review

fd64d80

Revert "Accept 64 character SHA-256 as well"

e6e957f

No need to apply that technique in one location when there are many others that have the same issue. This reverts commit 080cfb8.

MarkEWaite mentioned this pull request Jul 1, 2023

Increase spotbugs checks #1475

Merged

12 tasks

Merge branch 'master' into ckullabosch/master

6d0daaf

MarkEWaite added the enhancement Improvement or new feature label Jul 1, 2023

MarkEWaite requested changes Jul 1, 2023

View reviewed changes

MarkEWaite added 5 commits July 1, 2023 16:07

Use an import to simplify reading the test

fb0afc9

Suppress a spotbugs warning as technical debt

d21149d

Not clear to me how to resolve the spotbugs warning

Rename local headName as calculatedHeadName for clarity

343d3f1

Using the same string for the local as is used in the HeadNameResult class was confusing me. Rather than risk being confused when I read it again later, let's rename it now.

Report value of headNameResult.headName

244ba1f

Silences a spotbugs warning and helps my testing

Declare refSpec can be null in calculate

1b881a6

The tests are passing null and have encountered no error with the null argument

MarkEWaite and others added 4 commits July 2, 2023 06:58

Merge branch 'master' into ckullabosch/master

ac489cf

Simplify GitSCMFileSystemTest with import

e66e798

Fix refspec calculation for tags

c40b747

Remove headName field in HeadNameResult

8878bbb

The field was only needed to calculte remoteHeadName, which is now done inside the calculate() method.

MarkEWaite added 2 commits July 8, 2023 06:19

Merge branch 'master' into ckullabosch/master

6673ed0

Merge branch 'master' into ckullabosch/master

78797e8

ckullabosch requested a review from MarkEWaite July 31, 2023 11:03

Merge branch 'master' into ckullabosch/master

4c04395

ckullabosch requested a review from a team as a code owner December 29, 2023 22:31

github-actions bot added the tests Automated test addition or improvement label Dec 29, 2023

balakine reviewed Feb 13, 2024

View reviewed changes

MarkEWaite removed the tests Automated test addition or improvement label Apr 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ckullabosch commented Jun 19, 2023 •

edited by MarkEWaite

ckullabosch commented Jun 27, 2023 •

edited

MarkEWaite commented Jun 27, 2023

MarkEWaite commented Jun 27, 2023 •

edited

MarkEWaite left a comment

MarkEWaite Jul 1, 2023

ckullabosch Jul 7, 2023

MarkEWaite Jul 1, 2023

ckullabosch Jul 7, 2023

MarkEWaite commented Jul 1, 2023 •

edited

MarkEWaite commented Jul 1, 2023

ckullabosch commented Jul 7, 2023

MarkEWaite commented Jul 31, 2023

ckullabosch commented Oct 5, 2023

SinOverCos commented Nov 16, 2023

MarkEWaite commented Nov 17, 2023

balakine left a comment

balakine Feb 13, 2024

balakine Feb 13, 2024

balakine Feb 13, 2024

balakine Feb 13, 2024

balakine Feb 13, 2024

balakine Feb 13, 2024

balakine Feb 13, 2024

	if (env != null && refspecExpandedName != null) {
	if (env != null) {

		if (branchSpecExpandedName.equals(Constants.FETCH_HEAD) && refspecExpandedName != null &&
		!refspecExpandedName.equals("")) {

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

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

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

Are you sure you want to change the base?

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

Conversation

ckullabosch commented Jun 19, 2023 • edited by MarkEWaite