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-48061] Add new "Discover other refs" trait, GitBranchSCMHead and GitRefSCMHead #577

Merged
merged 31 commits into from May 4, 2018

Conversation

Projects
None yet
7 participants
@rsandell
Copy link
Member

rsandell commented Mar 15, 2018

JENKINS-48061 - Add GitBranchSCMHead and GitRefSCMHead

To be able to fetch commits not on any branch or tag

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 findbugs warnings were introduced with my changes
  • I have interactively tested my changes
  • Any dependent changes have been merged and published in upstream modules (like git-client-plugin)

Types of changes

What types of changes does your code introduce? Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
[JENKINS-48061] Introduce GitBranchSCMHead
And migrate old usage of plain SCMHead
@stephenc

This comment has been minimized.

|| !head instanceof GitSCMHeadMixin so that plugins extending AbstractGitSCMSource that have not updated to reflect the new GitSCMHeadMixin will continue to behave as if their branches are branches

This comment has been minimized.

Copy link
Owner Author

rsandell replied Mar 16, 2018

But if it's not a GitBranchSCMHead I can't return a GitBranchSCMRevision?

This comment has been minimized.

Copy link

stephenc replied Mar 16, 2018

Well you can assume it’s a RevisionImpl and the ref has to be refs/heads/ if it’s a branch

@stephenc

This comment has been minimized.

So SCMHeadMixin is a marker interface, you need

public interface GitSCMHead extends SCMHeadMixin) {
  default String getRef() {
    return "refs/heads/" + getName();
  }
}

and then have this class extend that

This comment has been minimized.

Copy link
Owner Author

rsandell replied Mar 16, 2018

master is still on java 7, so no default methods yet :'( but I think we can do without them.

@stephenc

This comment has been minimized.

Copy link

stephenc commented on 50e4690 Mar 9, 2018

You seem to be missing an SCMHeadMigration also

@rsandell

This comment has been minimized.

Copy link
Member Author

rsandell commented Mar 15, 2018

SCMHeadMigrationImpl is an internal class in GitBranchSCMHead

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Mar 16, 2018

@rsandell is this pull request far enough along that I should evaluate it? I'm certainly not as qualified as @stephenc, but I'm willing to code review it if you think my weekend code review will help.

@rsandell

This comment has been minimized.

Copy link
Member Author

rsandell commented Mar 16, 2018

@MarkEWaite any help is good help :)
I believe the basics are there, otoh I just got an idea that might change some things ;) Because all is not working yet and I know I've broken at least one test.

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Mar 16, 2018

I've been asked to help this weekend with reviewing and improving the Jenkins-X documentation and initial experience. I will place the review of this pull request as second priority behind the Jenkins-X work. They're both open source, and they are competing for my weekend time...

@MarkEWaite

This comment has been minimized.

Copy link

MarkEWaite commented Mar 17, 2018

@rsandell while working through the pull request with debugging breakpoints set in the changes, I found a section of code that seemed to be untouched by the existing tests.

Would you consider using 13882c9 from my fork as ideas for additional tests? Several items are in a comment which I believe should work but do not work. I may be wrong thinking they should work, but at least the tests provide some additional coverage.

[JENKINS-48061] GitSCMHeadMixin and discover all published refs
Since it seems like the git protocol doesn't allow discovering unpublished refs
anyway, we can do ahead and find the sha among all published refs instead
@stephenc
Copy link
Member

stephenc left a comment

Getting there

@@ -43,6 +42,11 @@ public GitBranchSCMHead(@NonNull String name) {
super(name);
}

@Override
public String getRef() {
return "refs/heads/" + getName();

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 19, 2018

Member

Constants.R_HEADS


@Override
public String getRef() {
return "refs/heads/" + getName();

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 19, 2018

Member

Constants.R_HEADS

@rsandell

This comment has been minimized.

Copy link
Member Author

rsandell commented Mar 20, 2018

I'm fiddling with a test for build storm

return new GitBranchSCMRevision((GitBranchSCMHead)head, b.getSHA1String());
}
}
} else { //TODO change to something

This comment has been minimized.

Copy link
@rsandell

rsandell Mar 21, 2018

Author Member

I'm still a bit unsure what to do here.

rsandell added some commits Mar 23, 2018

[JENKINS-48061] Add a test for buildstorm check
This required a newer ish shared libraries plugin in the test scope
which required a slightly newer jenkins core

@rsandell rsandell changed the title [WiP][JENKINS-48061] Add GitBranchSCMHead and GitRefSCMHead [JENKINS-48061] Add GitBranchSCMHead and GitRefSCMHead Mar 23, 2018

@rsandell

This comment has been minimized.

Copy link
Member Author

rsandell commented Mar 23, 2018

@stephenc I might be done now, still a bit unsure about what I did in the last commit (ac97524), but I figured I needed to do something there :)

@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented Mar 23, 2018

I'll try to review on monday... ping me by midday in case i forget ;-)

@rsandell

This comment has been minimized.

Copy link
Member Author

rsandell commented Mar 23, 2018

OK, I've been trying to come up with additional tests to write but haven't found anything yet.

MarkEWaite added some commits Mar 23, 2018

Temporary findbugs workaround
Allow more testing without the distraction of a findbugs warning
shortHashMatch);
} else if (name.startsWith(Constants.R_HEADS)) {

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Mar 24, 2018

I believe this is unreachable code. I'm surprised the Java compiler didn't detect this.

Line 804 is the same condition and when that condition is satisfied, the method returns.

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Mar 24, 2018

See my pull request which proposes to change that to Constants.R_TAGS and extends a test to reach that code.

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Mar 25, 2018

I haven't yet performed any interactive testing of the code change, just code review and running tests with breakpoints set in a debugger. Several breakpoints in changes were not reached during my session, but I've not yet investigated further.

This comment has been minimized.

Copy link
@rsandell

rsandell Mar 26, 2018

Author Member

Yea, this must be a copy paste error :/

public void before() throws Throwable {
if (!Functions.isWindows() && "testMigrationNoBuildStorm".equals(this.getTestDescription().getMethodName())) {
URL res = getClass().getResource("/jenkins/plugins/git/GitBranchSCMHeadTest/testMigrationNoBuildStorm_repositories.zip");
final File path = new File("/tmp/JENKINS-48061");

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Mar 26, 2018

This hardcoded path ("/tmp/JENKINS-48061") embedded in the test creates the same types of problems as the JenkinsRule hardcoded path.

If different users are running Jenkins unit tests on the same Unix based computer, only one of them will be allowed to run this test successfully because other users won't have permission to remove the test directory.

See my pull request for a multi-platform proposal that worked in my simple tests.

Could we add an "after" which removes the directory with the hard coded name?

This comment has been minimized.

Copy link
@rsandell

rsandell Mar 26, 2018

Author Member

Yea, I usually don't like this way of doing it, but I couldn't figure out how to have a dynamic path without doing a configuration change and thereby voiding the test.

This comment has been minimized.

Copy link
@kshultzCB

kshultzCB Mar 26, 2018

Would a relative path (tmp/JENKINS-48061), instead of an absolute path (/tmp/JENKINS-48061), be of any help? Just thinking out loud.

An @After like @MarkEWaite suggested would be cool, too.

This comment has been minimized.

Copy link
@stephenc

stephenc Apr 3, 2018

Member

ideally I'd add a "lock file" that contains a time-stamp and if the time-stamp is less than 5 minutes old then sleep until the "lock-file" is deleted... if more than 5 minutes old, wipe the temp dir.

But there should be some way to template in the path without needing to hardcode

This comment has been minimized.

Copy link
@stephenc

stephenc May 2, 2018

Member

Did we get a solution to this? If not, file a JIRA, leave a TODO comment and we can move on

This comment has been minimized.

Copy link
@rsandell

rsandell May 3, 2018

Author Member

I added an @After to clean up after itself.

@rsandell

This comment has been minimized.

Copy link
Member Author

rsandell commented Mar 26, 2018

@MarkEWaite I merged all commits from your pr but the build storm test change into this PR. I need to have a think on how/if I can approve it somehow.

@stephenc
Copy link
Member

stephenc left a comment

Looks not bad... some questions and minor niggles

GitSCMTelescope telescope = GitSCMTelescope.of(this);
if (telescope != null) {
String remote = getRemote();
StandardUsernameCredentials credentials = getCredentials();
telescope.validate(remote, credentials);
return telescope.getRevision(remote, credentials, head);
}
if (head instanceof GitSCMHeadMixin) {
context = context.withoutRefSpecs().withRefSpec(((GitSCMHeadMixin) head).getRef());

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 26, 2018

Member

What about somebody who has added additional ref specs in a trait... will those refspecs be removed now?

This comment has been minimized.

Copy link
@rsandell

rsandell Mar 27, 2018

Author Member

Yes I was a bit hesitant on that myself, context.withoutRefSpecs() does clear all refspecs from the traits.
But from what I can tell, in this context you are after a specific SCMHead so wouldn't all other refspecs just be a waste of time and bandwidth?

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 27, 2018

Member

A comment explaining the logic as to why it is ok to strip it out here would be ideal then

}

@Override
public String getRef() {

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 26, 2018

Member

final let's not let anyone hijack branch for a non-branch

* The ref, e.g. /refs/heads/master
* @return the ref
*/
String getRef();

This comment has been minimized.

Copy link
@stephenc

stephenc Mar 26, 2018

Member

// TODO provide a default implementation once Java 8 baseline

@robincsmith

This comment has been minimized.

Copy link

robincsmith commented Apr 4, 2018

I tried installing the updated plugin directly from the Jenkins build for this PR. I still get the following issue when attempting to load a pipeline shared library from a PR:

> git fetch --tags --progress origin +refs/heads/*:refs/remotes/origin/* +refs/pull-requests/*/from:refs/remotes/origin/pr/*
> git rev-parse pr/33^{commit} # timeout=10
ERROR: Could not resolve pr/33
hudson.plugins.git.GitException: Command "git rev-parse pr/33^{commit}" returned status code 128:
stdout: pr/33^{commit}

stderr: fatal: ambiguous argument 'pr/33^{commit}': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1996)
at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1964)
at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1960)
at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommand(CliGitAPIImpl.java:1597)
at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommand(CliGitAPIImpl.java:1609)
at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.revParse(CliGitAPIImpl.java:794)
at hudson.plugins.git.GitAPI.revParse(GitAPI.java:316)
at jenkins.plugins.git.AbstractGitSCMSource$10.run(AbstractGitSCMSource.java:859)
at jenkins.plugins.git.AbstractGitSCMSource$10.run(AbstractGitSCMSource.java:853)
at jenkins.plugins.git.AbstractGitSCMSource.doRetrieve(AbstractGitSCMSource.java:352)
at jenkins.plugins.git.AbstractGitSCMSource.retrieve(AbstractGitSCMSource.java:853)
at jenkins.scm.api.SCMSource.fetch(SCMSource.java:598)
at org.jenkinsci.plugins.workflow.libs.SCMSourceRetriever.retrieve(SCMSourceRetriever.java:80)
at org.jenkinsci.plugins.workflow.libs.LibraryAdder.retrieve(LibraryAdder.java:153)
at org.jenkinsci.plugins.workflow.libs.LibraryAdder.add(LibraryAdder.java:134)
at org.jenkinsci.plugins.workflow.libs.LibraryDecorator$1.call(LibraryDecorator.java:125)
at org.codehaus.groovy.control.CompilationUnit.applyToPrimaryClassNodes(CompilationUnit.java:1065)
at org.codehaus.groovy.control.CompilationUnit.doPhaseOperation(CompilationUnit.java:603)
at org.codehaus.groovy.control.CompilationUnit.processPhaseOperations(CompilationUnit.java:581)
at org.codehaus.groovy.control.CompilationUnit.compile(CompilationUnit.java:558)
at groovy.lang.GroovyClassLoader.doParseClass(GroovyClassLoader.java:298)
at groovy.lang.GroovyClassLoader.parseClass(GroovyClassLoader.java:268)
at groovy.lang.GroovyShell.parseClass(GroovyShell.java:688)
at groovy.lang.GroovyShell.parse(GroovyShell.java:700)
at org.jenkinsci.plugins.workflow.cps.CpsGroovyShell.doParse(CpsGroovyShell.java:129)
at org.jenkinsci.plugins.workflow.cps.CpsGroovyShell.reparse(CpsGroovyShell.java:123)
at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.parseScript(CpsFlowExecution.java:517)
at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.start(CpsFlowExecution.java:480)
at org.jenkinsci.plugins.workflow.job.WorkflowRun.run(WorkflowRun.java:269)
at hudson.model.ResourceController.execute(ResourceController.java:97)
at hudson.model.Executor.run(Executor.java:421)
ERROR: No version pr/33 found for library forgerock-pipeline-libs
org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed:
WorkflowScript: Loading libraries failed

1 error

at org.codehaus.groovy.control.ErrorCollector.failIfErrors(ErrorCollector.java:310)
at org.codehaus.groovy.control.CompilationUnit.applyToPrimaryClassNodes(CompilationUnit.java:1085)
at org.codehaus.groovy.control.CompilationUnit.doPhaseOperation(CompilationUnit.java:603)
at org.codehaus.groovy.control.CompilationUnit.processPhaseOperations(CompilationUnit.java:581)
at org.codehaus.groovy.control.CompilationUnit.compile(CompilationUnit.java:558)
at groovy.lang.GroovyClassLoader.doParseClass(GroovyClassLoader.java:298)
at groovy.lang.GroovyClassLoader.parseClass(GroovyClassLoader.java:268)
at groovy.lang.GroovyShell.parseClass(GroovyShell.java:688)
at groovy.lang.GroovyShell.parse(GroovyShell.java:700)
at org.jenkinsci.plugins.workflow.cps.CpsGroovyShell.doParse(CpsGroovyShell.java:129)
at org.jenkinsci.plugins.workflow.cps.CpsGroovyShell.reparse(CpsGroovyShell.java:123)
at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.parseScript(CpsFlowExecution.java:517)
at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.start(CpsFlowExecution.java:480)
at org.jenkinsci.plugins.workflow.job.WorkflowRun.run(WorkflowRun.java:269)
at hudson.model.ResourceController.execute(ResourceController.java:97)
at hudson.model.Executor.run(Executor.java:421)
Finished: FAILURE

I'm using the refspec +refs/pull-requests/*/from:refs/remotes/@{remote}/pr/* in the global pipeline library configuration, then accessing that library in my pipeline like this: @Library('my-pipeline-library@pr/<pull request ID>') - that was broken by version 3.6.4 of the plugin

@MarkEWaite MarkEWaite referenced this pull request Apr 9, 2018

Merged

Use Jenkins 2.60.1, JDK 8, and git client 3.0.0 #565

4 of 5 tasks complete
@jacob-keller

This comment has been minimized.

Copy link

jacob-keller commented Apr 13, 2018

I tried installing the updated plugin directly from the Jenkins build for this PR. I still get the following issue when attempting to load a pipeline shared library from a PR

It looks like an issue that can occur if your shared pipeline doesn't fetch the refs properly, since git is trying to resolve pr/33^{commit} and pr/33 doesn't exist...

Can you run "git rev-parse pr/33^{commit} in the work tree manually and see what happens? What about if you add the "--" after to force git to treat it like a revision (that should at least clarify the error message).

rsandell added some commits Apr 13, 2018

@rsandell

This comment has been minimized.

Copy link
Member Author

rsandell commented Apr 17, 2018

@robincsmith you should not use the additional refspecs trait but instead the new "Discover other refs".

@robincsmith

This comment has been minimized.

Copy link

robincsmith commented Apr 17, 2018

Thanks @rsandell - I'll try that, using the latest build

@robincsmith

This comment has been minimized.

Copy link

robincsmith commented Apr 18, 2018

@rsandell that looks awesome. To confirm, under the new "Discover other refs" behaviour, I used pull-requests/*/from (Bitbucket Server 5.8.0)

Then, in my pipeline:

@Library('some-pipeline-library@pull-requests/33/from') _

And the correct commit was found.

@rsandell

This comment has been minimized.

Copy link
Member Author

rsandell commented Apr 18, 2018

@robincsmith yes, but with the new trait

@Library('some-pipeline-library@pull-requests-33') _

should also work, and you can change the name mapping behind the advanced button to enable

@Library('some-pipeline-library@pr-33') _

as well.

@robincsmith

This comment has been minimized.

Copy link

robincsmith commented Apr 18, 2018

Yep, I confirm those work as well (with the necessary values in the 'advanced' section)

@hughsaunders
Copy link

hughsaunders left a comment

Works for me checking out libraries from pull requests :D
screen shot 2018-04-23 at 11 20 22

Fixing findbugs issues after Jenkins core bump
The methods that had the issues should probably all just be removed
since almost all just have the default implementation.
But I wanted to disturb existing code as little as possible.
@stephenc

This comment has been minimized.

Copy link
Member

stephenc commented May 2, 2018

@rsandell So I have not done final code review, one question for yourself and @MarkEWaite

This PR adds a new "Discover other refs" trait...

In my mind, the majority of users probably do not want this. If we have it there then the likelihood is that we will have lots of users using it to try and discover PRs from GitHub... something that the GitHub Branch Source is better at... similarly, we'll have users using this to integrate with Gerrit instead of driving a full Gerrit Branch Source integration, etc

Would it be better to make this trait abstract and move the implementation into a separate extension plugin. This would IMHO save the majority of users from wondering what "Discover other refs" is all about while still enabling the power users who need it (for e.g. shared libraries) to get the feature?

WDYT?

P.S. I don't mind which you opt for, as long as you two agree and document your logic

@rsandell

This comment has been minimized.

Copy link
Member Author

rsandell commented May 2, 2018

Putting it in a separate plugin seems a bit excessive to me, though I understand your argument for doing so. Maybe some harsh words in the description/help could suffice? Though what those harsh words would be I don't know :)

@rsandell rsandell changed the title [JENKINS-48061] Add GitBranchSCMHead and GitRefSCMHead [JENKINS-48061] Add new "Discover other refs" trait, GitBranchSCMHead and GitRefSCMHead May 2, 2018

@stephenc
Copy link
Member

stephenc left a comment

Hopefully from here on, I can just review the diffs ;-)

Looking good... I'm just being careful given the importance of this plugin

pom.xml Outdated
<java.level>7</java.level>
<no-test-jar>false</no-test-jar>
<concurrency>1C</concurrency>
<findbugs.failOnError>false</findbugs.failOnError>
<workflow.version>1.14.2</workflow.version>
<!--<workflow.version>1.14.2</workflow.version>-->

This comment has been minimized.

Copy link
@stephenc

stephenc May 2, 2018

Member

rainy day code?

<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-step-api</artifactId>
<version>2.10</version>

This comment has been minimized.

Copy link
@stephenc

stephenc May 2, 2018

Member

I thought the workflow 2.x series required upgrading code to 2.7+ (fine if it doesn't... but we should confirm)

This comment has been minimized.

Copy link
@rsandell

rsandell May 3, 2018

Author Member

Nope, workflow 2.x is just the mono repo extraction of all the sub-plugins. All versions that I picked has a max core version of 1.642.3, that's why I bumped core dependency to that.

@@ -14,6 +14,7 @@
import org.kohsuke.stapler.QueryParameter;
import org.kohsuke.stapler.StaplerRequest;

import javax.annotation.Nonnull;

This comment has been minimized.

Copy link
@stephenc

stephenc May 2, 2018

Member

Can we stick to the findbugs ones?

This comment has been minimized.

Copy link
@rsandell

rsandell May 3, 2018

Author Member

I used whatever annotation that the methods extended does in an effort to try and not make too much of a mess :)

public String getDisplayName() {
return "AssemblaWeb";
}

@Override
public AssemblaWeb newInstance(StaplerRequest req, JSONObject jsonObject) throws FormException {
public AssemblaWeb newInstance(StaplerRequest req, @Nonnull JSONObject jsonObject) throws FormException {

This comment has been minimized.

Copy link
@stephenc

stephenc May 2, 2018

Member

these seem like unrelated changes... ideally the core version bump would be in a separate PR... not saying to move it out now... just noting for future code archeologists

This comment has been minimized.

Copy link
@rsandell

rsandell May 3, 2018

Author Member

Yepp, I kept all those changes in it's own commit so that it can be reverted if it was too much in the PR, but I can't get a green build without it. I consulted with @MarkEWaite on what to do about it :)

The core and plugin bumps is needed for the built storm fix and something else I can't remember at the moment.

@@ -272,6 +317,9 @@ public final C withRemoteName(String remoteName) {
@NonNull
public final List<RefSpec> asRefSpecs() {
List<RefSpec> result = new ArrayList<>(Math.max(refSpecs.size(), 1));
if (wantOtherRefs() && wantBranches()) {

This comment has been minimized.

Copy link
@stephenc

stephenc May 2, 2018

Member

what happens if only one of these is true?

This comment has been minimized.

Copy link
@rsandell

rsandell May 3, 2018

Author Member

Well if it want other refs, then that refspec is already added to the list retrieved below. Which means that if you also want branches that refspec needs to be added, otherwise you will only fetch the other refs and not the branches.

So if we don't want other refs (and no other explicit refspec is set) then the list of refspecs is empty and so the branches will be fetched as by default.

This comment has been minimized.

Copy link
@rsandell

rsandell May 3, 2018

Author Member

I.e. this is an ugly hack to get it to work when you both want other refs and branches, if any of those are false we can go on with our normal flow. :)

This comment has been minimized.

Copy link
@stephenc

stephenc May 4, 2018

Member

thanks for the clarifying comment

@@ -420,6 +440,7 @@ protected void retrieve(@CheckForNull SCMSourceCriteria criteria,
if (context.wantTags()) {
referenceTypes.add(GitSCMTelescope.ReferenceType.TAG);
}
//TODO DiscoverOtherRefsTrait?

This comment has been minimized.

Copy link
@stephenc

stephenc May 2, 2018

Member

rainy day comment? (either delete it or if important but not blocking file a JIRA and update the comment with the JIRA)

This comment has been minimized.

Copy link
@rsandell

rsandell May 3, 2018

Author Member

this is a comment for you I think, I sprinkled some of those comments around where they might be needed and then implemented in those places I definitely knew they where needed and I knew what to do.
I might have missed removing some that I implemented.

@@ -537,10 +560,84 @@ public Void run(GitClient client, String remoteName) throws IOException, Interru
if (context.wantTags()) {
discoverTags(repository, walk, request, remoteReferences);
}
if (context.wantOtherRefs()) {
discoverOtherRefs(repository, walk, request, remoteReferences,
(Collection<GitSCMSourceContext.WantedOtherRef>)context.getOtherWantedRefs());

This comment has been minimized.

Copy link
@stephenc

stephenc May 2, 2018

Member

WantedOtherRef is not a name I like, especially as it leaks into the public API... any suggested alternatives? (if it is the least bad, well we'll have to live with it)

This comment has been minimized.

Copy link
@stephenc

stephenc May 2, 2018

Member

AdditionalRefSpec or something like that might be better... or worse... suggestions please... and then we pick the least ugly

This comment has been minimized.

Copy link
@stephenc

stephenc May 2, 2018

Member

RefNameMapping aligns better with the usage that gets logged

This comment has been minimized.

Copy link
@rsandell

rsandell May 3, 2018

Author Member

AdditionalRefSpec is already used in a method somewhere.
The use of wanted was to reflect that it is about the same as wantBranches and wantTags. And I guess my brain got stuck on that train :)

RefNameMapping sounds better, I'll chew on it for a bit.

// 1. A branch name (if we have that we can return quickly)
// 2. A tag name (if we have that we will need to fetch the tag to resolve the tag date)
// 3. A short/full revision hash that is the head revision of a branch (if we have that we can return quickly)
// 3.2 A remote refspec for example pull-requests/1/from

This comment has been minimized.

Copy link
@stephenc

stephenc May 2, 2018

Member

just renumber, no sub-numbering

tagName = StringUtils.removeStart(name, Constants.R_TAGS);
context.wantBranches(false);
context.wantTags(true);
context.withoutRefSpecs();
}

This comment has been minimized.

Copy link
@stephenc

stephenc May 2, 2018

Member

If it is a ref, we save effort by returning the GitRefSCMRevision itself, so we should set a boolean here to say "found a ref match" and then return the GitRefSCMRevision at the end of the loop if the ref match was found. the current code will prefer a tag to a GitRefSCMRevision despite a tag needing date resolution (i.e. more work)

This comment has been minimized.

Copy link
@rsandell

rsandell May 3, 2018

Author Member

Yes that's the way I designed it after how I interpreted it from our earlier discussions, that it's better if we could return a tag or a branch than a ref.
I guess I misunderstood?

This comment has been minimized.

Copy link
@stephenc

stephenc May 4, 2018

Member

a branch is the best
a ref should be preferred to a tag (because we do not need to resolve the time)

@@ -831,7 +977,7 @@ public SCMRevision run(GitClient client, String remoteName) throws IOException,
}
listener.getLogger()
.printf("Selected match: %s revision %s%n", name, hash);
return new SCMRevisionImpl(new SCMHead(name), hash);
return new GitBranchSCMRevision(new GitBranchSCMHead(name), hash);

This comment has been minimized.

Copy link
@stephenc

stephenc May 2, 2018

Member

We are missing the case where the revision does not exist on any branch but does exist in the history of an other ref. Now if that is because client.getBranchesContaining(hash, true) doesn't allow us to query other refs, then we need to file a JIRA, add a comment and move on... otherwise we probably need to revisit the candidatePrefix search to include other refs

This comment has been minimized.

Copy link
@rsandell

rsandell May 3, 2018

Author Member

Well, the way I think the refspec for the trait is constructed the commits will be fetched as branches on the remote, I don't like it, but it should work. I should probably add a test for that though to verify.

This comment has been minimized.

Copy link
@rsandell

rsandell May 4, 2018

Author Member

See AbstractGitSCMSourceTest#retrieveRevision_customRef_descendant

@stephenc
Copy link
Member

stephenc left a comment

just the order of resolution is retrieve(hash) needs fixing

}
} else if (head instanceof GitRefSCMHead) {
try {
ObjectId objectId = client.revParse(((GitRefSCMHead) head).getRef());

This comment has been minimized.

Copy link
@stephenc

stephenc May 4, 2018

Member

yes covered by the else... i think I was just wondering if we could merge one of the paths... no worries. fine for now

tagName = StringUtils.removeStart(name, Constants.R_TAGS);
context.wantBranches(false);
context.wantTags(true);
context.withoutRefSpecs();
}

This comment has been minimized.

Copy link
@stephenc

stephenc May 4, 2018

Member

a branch is the best
a ref should be preferred to a tag (because we do not need to resolve the time)

@@ -272,6 +317,9 @@ public final C withRemoteName(String remoteName) {
@NonNull
public final List<RefSpec> asRefSpecs() {
List<RefSpec> result = new ArrayList<>(Math.max(refSpecs.size(), 1));
if (wantOtherRefs() && wantBranches()) {

This comment has been minimized.

Copy link
@stephenc

stephenc May 4, 2018

Member

thanks for the clarifying comment

[JENKINS-48061] Resolve other ref before tag name
So we don't have to check timestamps on the tag
@stephenc
Copy link
Member

stephenc left a comment

Probably will need refinement after it hits customers... probably good enough for now

MarkEWaite added some commits May 4, 2018

Use 3.9.0-SNAPSHOT as version
Increment the minor version number because it requires 
Jenkins 1.642.3 LTS instead of the older 1.625.3 LTS.

@MarkEWaite MarkEWaite merged commit 927c829 into jenkinsci:master May 4, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@rsandell rsandell deleted the rsandell:JENKINS-48061 branch Jan 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.