Skip to content

[JENKINS-29796] Multiple refspecs should be matched using OR, not AND#343

Merged
MarkEWaite merged 3 commits intojenkinsci:masterfrom
hannes-ucsc:JENKINS-29796
Aug 6, 2015
Merged

[JENKINS-29796] Multiple refspecs should be matched using OR, not AND#343
MarkEWaite merged 3 commits intojenkinsci:masterfrom
hannes-ucsc:JENKINS-29796

Conversation

@hannes-ucsc
Copy link
Contributor

@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@MarkEWaite
Copy link
Contributor

Could you provide a test which shows the problem? A bug fix without a test is too easy to lose when someone else submits a different change.

Copy link
Member

Choose a reason for hiding this comment

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

if (!match) {

@hannes-ucsc
Copy link
Contributor Author

@MarkEWaite Sure, I'll write a test. Do you agree with the general idea, though? I was a little perplexed as to why the original had AND semantics. It seemed like a deliberate decision and I'm worried I'm wasting my time because I don't see the big picture.

@hannes-ucsc hannes-ucsc changed the title [JENKINS-29796] Multiple refspecs should be matched using OR not AND [JENKINS-29796] Multiple refspecs should be matched using OR, not AND Aug 5, 2015
@MarkEWaite
Copy link
Contributor

I think you're on the right track. I don't know why multiple refspecs would all be required to match. That seems counter to my expectation of the cases where I might use multiple refspecs.

@hannes-ucsc
Copy link
Contributor Author

Added a test. The test fails on the master branch and succeeds on this one.

The tests in SCMTriggerTest are a bit wonky IMHO. For example, the commits parameter to SCMTriggerTest.check() is never used. I was also surprised to see the poll.hasChanges() assertion in check() is always false.

@MarkEWaite
Copy link
Contributor

It is a complicated enough plugin that I often discover unexpected twists and turns while writing tests. That discovery process has been part of the fun of trying to maintain the plugin. Thanks for writing the test.

MarkEWaite added a commit that referenced this pull request Aug 6, 2015
[JENKINS-29796] Multiple refspecs should be matched using OR, not AND
@MarkEWaite MarkEWaite merged commit 9abd397 into jenkinsci:master Aug 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants