[JENKINS-70303] Remove leading and trailing spaces from refspec

[Jagrutiti]

JENKINS-70303 - Remove leading and trailing spaces from refspec

If a refspec is included in the checkout scm definition of a multibranch Pipeline using JGit for checkout and the refspec has a leading space character, the checkout fails with the JGit message

Remote does not have  available for fetch.

The same refspec with leading spaces does not affect command line git checkout. It works in either case with command line git. Leading space handling by command line git is preferred rather than the way that the git client plugin honors the leading spaces and passes them to JGit.

Command line git does not see the issue because extra spaces between arguments of a command line program are generally not an issue. Lucky circumstances that allow the CLI git checkout to be better behaved than the JGit checkout.

Checklist

• I have read the CONTRIBUTING doc
• I have referenced the Jira issue related to my changes in one or more commit messages
• I have added tests that verify my changes
• Unit tests pass locally with my changes
• I have added documentation as necessary
• No Javadoc warnings were introduced with my changes
• No spotbugs warnings were introduced with my changes
• I have interactively tested my changes

Types of changes

What types of changes does your code introduce?

• Bug fix (non-breaking change which fixes an issue)

[Jagrutiti]

I have tested this PR using mvn clean install. All the test cases in CliGitAPIImplTest.java pass.

How shall I write a test case of my own? I mean how do I test refspecs?

[Jagrutiti]

As this hard-coded we can remove trim(). I was not sure if I should.

[MarkEWaite]

The existing trim should be removed. It won't change the RefSpec definition in any way because "/*".trim() is the same as "/*"

[MarkEWaite]

Thanks very much for the pull request. The two cases seem reasonable at first glance. The automated tests that you'll create should help confirm that they are sufficient to resolve the issue.

Have you tested this interactively based on the description in the issue report?

I think that you can add an automated test in the GitClientCloneTest. It already has the test_clone_refspecs() test that defines a new RefSpec. You can modify that test to sometimes include leading or trailing spaces on the RefSpec or you can copy that test and test for spaces in the RefSpec in the copied test.

I've provided a sample of the way I would write the test and pushed it to your branch as 2b04ed4 . The test is currently failing for JGit. I think that means there are more places that will need to be changed.

I also added the Issue annotation to the modified test with a comment explaining the change.

I found that location by using git grep RefSpec -- src/test to search for existing uses of RefSpec in the tests. That test seemed like the easiest one to extend for this case. You should do the same search and confirm that my choice was reasonable. There may be other tests that assert RefSpec contents that could be extended in a similar way.

[Jagrutiti]

Thanks very much for the pull request. The two cases seem reasonable at first glance. The automated tests that you'll create should help confirm that they are sufficient to resolve the issue.

Have you tested this interactively based on the description in the issue report?

I think that you can add an automated test in the GitClientCloneTest. It already has the test_clone_refspecs() test that defines a new RefSpec. You can modify that test to sometimes include leading or trailing spaces on the RefSpec or you can copy that test and test for spaces in the RefSpec in the copied test.

I've provided a sample of the way I would write the test and pushed it to your branch as 2b04ed4 . The test is currently failing for JGit. I think that means there are more places that will need to be changed.

I also added the Issue annotation to the modified test with a comment explaining the change.

I found that location by using git grep RefSpec -- src/test to search for existing uses of RefSpec in the tests. That test seemed like the easiest one to extend for this case. You should do the same search and confirm that my choice was reasonable. There may be other tests that assert RefSpec contents that could be extended in a similar way.

I will work on this. Thanks.

[MarkEWaite]

I've included the issue description in the pull request description because it helps reviewers to have the issue described clearly in the pull request, without requiring that they open the bug report. That's my personal preference rather than a standard across the entire Jenkins organization, but since I'm one of the maintainers of this plugin, I applied my preference.

[Jagrutiti]

I've included the issue description in the pull request description because it helps reviewers to have the issue described clearly in the pull request, without requiring that they open the bug report. That's my personal preference rather than a standard across the entire Jenkins organization, but since I'm one of the maintainers of this plugin, I applied my preference.

I noticed. I had added the description in the PR title itself.

[Jagrutiti]

I was not able to fix the test case for jgit and jgitapache. I know the test executes this part of the code.

git-client-plugin/src/main/java/org/jenkinsci/plugins/gitclient/JGitAPIImpl.java

Lines 1547 to 1565 in 31ecb75

	if (refspecs == null) {
	refspecs =
	Collections.singletonList(new RefSpec("+refs/heads/*:refs/remotes/" + remote + "/*"));
	}
	FetchCommand fetch = new Git(repository)
	.fetch()
	.setProgressMonitor(new JGitProgressMonitor(listener))
	.setRemote(url.trim())
	.setCredentialsProvider(getProvider())
	.setTagOpt(tags ? TagOpt.FETCH_TAGS : TagOpt.NO_TAGS)
	.setRefSpecs(refspecs);
	setTransportTimeout(fetch, "fetch", timeout);
	if (shallow) {
	if (depth == null) {
	depth = 1;
	}
	fetch.setDepth(depth);
	}
	fetch.call();

[MarkEWaite]

I was not able to fix the test case for jgit and jgitapache. I know the test executes this part of the code.

At the location where you're looking, the RefSpec object already includes the leading space. It seems like it would be better to look at the locations where RefSpec objects are constructed. Trim the spaces before passing the string to the RefSpec constructor.

I'm making a guess by that idea, but I think it is a guess that is worth exploring.

[MarkEWaite]

This may be exactly the wrong place for this type of test. We generally prefer tests that are nearer to what a user would do. This is changing an argument to the RefSpec constructor when the real end user provides a string into a data entry field on a Jenkins form or they provide a string to an option of a Pipeline checkout step.

[Jagrutiti]

The test case did pass in my local.

[Jagrutiti]

Which test case tests the Pipeline checkout step?

[MarkEWaite]

I don't think that we'll be able to test Pipeline checkout from this plugin because it would require a dependency on the git plugin. That would result in a circular dependency, since the git plugin depends on this plugin. We'll need to test interactively.

[MarkEWaite]

Interactive testing of the pull request shows that a leading space in the refspec from the UI still fails to clone. The job definition is:

<project>
  <actions/>
  <description/>
  <keepDependencies>false</keepDependencies>
  <properties/>
  <scm class="hudson.plugins.git.GitSCM" plugin="git@5.2.1">
    <configVersion>2</configVersion>
    <userRemoteConfigs>
      <hudson.plugins.git.UserRemoteConfig>
        <name>origin</name>
        <refspec> +refs/heads/3.11.x:refs/remotes/origin/3.11.x</refspec>
        <url>https://github.com/jenkinsci/git-client-plugin</url>
      </hudson.plugins.git.UserRemoteConfig>
    </userRemoteConfigs>
    <branches>
      <hudson.plugins.git.BranchSpec>
        <name>3.11.x</name>
      </hudson.plugins.git.BranchSpec>
    </branches>
    <doGenerateSubmoduleConfigurations>false</doGenerateSubmoduleConfigurations>
    <gitTool>jgit</gitTool>
    <submoduleCfg class="empty-list"/>
    <extensions/>
  </scm>
  <canRoam>true</canRoam>
  <disabled>false</disabled>
  <blockBuildWhenDownstreamBuilding>false</blockBuildWhenDownstreamBuilding>
  <blockBuildWhenUpstreamBuilding>false</blockBuildWhenUpstreamBuilding>
  <triggers/>
  <concurrentBuild>false</concurrentBuild>
  <builders/>
  <publishers/>
  <buildWrappers/>
</project>

[Jagrutiti]

hudson.plugins.git.UserRemoteConfig

I have found this link for UserRemoteConfig: https://javadoc.jenkins.io/plugin/git/hudson/plugins/git/UserRemoteConfig.html

But not able to find where it is called in the code.

[Jagrutiti]

Hi @MarkEWaite

I have updated almost all the references I found for refspecs.

One of the test cases is failing because of:

ERROR]   GitClientFetchTest.test_fetch:256 » JGitInternal Exception caught during execution of merge command. org.eclipse.jgit.errors.MissingObjectException: Missing unknown dadd5f9e276467580ddb00c2403eec0e9501a6e3
[ERROR]   GitClientFetchTest.test_fetch:256 » JGitInternal Exception caught during execution of merge command. org.eclipse.jgit.errors.MissingObjectException: Missing unknown cb6a175eac031a7acf8270842a6eeda2ef1f27ad

I used debugger. It fails at this line:

newAreaWorkspace.getGitClient().merge().setRevisionToMerge(bareCommit3).execute();

but then the next line:

assertThat("bare3 != newArea3", newAreaWorkspace.getGitClient().revParse("HEAD"), is(bareCommit3));
has the following id as shown by the debugger.

AnyObjectId[224664d7fc3196baca45c60b06a6b269cea76932]

What should I fix here?