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-29326] Restore lost BuildData.equals override #388

Merged
merged 1 commit into from Mar 24, 2016

Conversation

Projects
None yet
6 participants
@jglick
Copy link
Member

commented Mar 22, 2016

336c178 by @MarkEWaite purported to merely be reverting c1872d0 but in fact it also deleted an unrelated method introduced in #372. Thus the tested added in jenkinsci/pipeline-plugin#271 passes against 2.4.1, starts failing against 2.4.2, and is fixed by this patch.

@reviewbybees esp. @abayer

jglick referenced this pull request Mar 22, 2016

[Fix JENKINS-29482] Don't prune stale branches from BuildData
This reverts commit c1872d0.

Git plugin 2.4.1 included c1872d0 which regressed the plugin and
introduced JENKINS-32218
@KostyaSha

This comment has been minimized.

Copy link
Member

commented Mar 22, 2016

Maybe add simple test that will compare two objects?

@reviewbybees

This comment has been minimized.

Copy link

commented Mar 22, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@abayer

This comment has been minimized.

Copy link
Member

commented Mar 22, 2016

🐝

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Mar 23, 2016

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

@@ -243,4 +243,22 @@ public String toString() {
",buildsByBranchName="+buildsByBranchName+
",lastBuild="+lastBuild+"]";
}

@Override
public boolean equals(Object o) {

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Mar 23, 2016

So sorry that I dropped this from the earlier revert. I still don't understand how I made that mistake, but it clearly looks like it was my mistake. I thought I had used a pure "git revert", yet that's not what it shows.

Doesn't adding an equals method without a hashCode method mean that this object will violate the hashCode contract? I thought that it was a known findbugs warning if equals() is implemented in a class, but hashCode() is not implemented.

This comment has been minimized.

Copy link
@jglick

jglick Mar 23, 2016

Author Member

Doesn't adding an equals method without a hashCode method mean that this object will violate the hashCode contract?

It does. The current calling code is searching in a List so it does not matter.

For purposes of this PR I am simply trying to restore what was lost. If you think #372 was mistaken to begin with, then some further development is needed, either here or in a follow-up.

This comment has been minimized.

Copy link
@jglick

jglick Mar 23, 2016

Author Member

I thought I had used a pure "git revert", yet that's not what it shows.

Possibly you did just that, and it produced a “merge” conflict, and you then resolved it incorrectly before committing.

} else {
BuildData otherBuildData = (BuildData) o;

if (otherBuildData.remoteUrls.equals(this.remoteUrls)

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Mar 23, 2016

The tests I wrote for the equals method threw null pointer exceptions on these unguarded references to fields within the BuildData object. I don't see any annotation which hints that they won't be null, so I assume they can be null, and this could throw a null pointer exception.

Would you be willing to consider https://github.com/MarkEWaite/git-plugin/commits/master-PR388-BuildData-equals ? I changed the equals implementation to not throw a null pointer exception and added an implementation of hashCode. I added several tests trying to assure that the equals and hashCode implementations were both valid for the equals contract and the hashCode contract.

This comment has been minimized.

Copy link
@jglick

jglick Mar 23, 2016

Author Member

Would you be willing to consider

Comparison

Other than the verbose implementation (Java 7+ Objects makes this a lot cleaner), seems fine to me, but the question is really for @abayer the original author.

I don't see any annotation which hints that they won't be null, so I assume they can be null

I really do not know. From casual inspection it seems that remoteUrls and buildsByBranchName are intended to be @Nonnull while lastBuild is intended to be @CheckForNull. The fact the fields are public non-final, and set in some constructors but not others, is disturbing. Probably the class should be tightened up with @AdaptField.

This comment has been minimized.

Copy link
@MarkEWaite

MarkEWaite Mar 23, 2016

@abayer, I'd much prefer to use the equals implementation that is null pointer safe and includes the hashCode implementation just in case some portion of the code is placing BuildData items into hash buckets.

Could you review https://github.com/MarkEWaite/git-plugin/commits/master-PR388-BuildData-equals and see if that is acceptable to you?

If that's not acceptable, would you consider the tests which I wrote for the BuildData class? Those tests increase the statement coverage of the class significantly. I think there is still some hope to eventually remove or massively rework the BuildData references, but until that rework, it seems safer to have tests to confirm the class is still well behaved.

@MarkEWaite

This comment has been minimized.

Copy link

commented Mar 24, 2016

@jglick and @abayer, JENKINS-33695 and JENKINS-33564 are severe enough regressions (extensions are lost when job configuration is saved with git plugin 2.4.3) that I need to release a new version of the plugin within the next 24 hours.

Unless I hear differently @abayer, I'll merge this pull request before releasing that new version, then include my change to the equals() and hashCode() methods, and my additional tests of the BuildData object. With those changes, I'll build a prototype, perform a series of interactive tests, then release git plugin 2.4.4 to fix those two data loss problems and this bad revert.

@abayer

This comment has been minimized.

Copy link
Member

commented Mar 24, 2016

@jglick

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2016

👍. BTW

I'll merge this pull request […] then include my change

a simple

git merge master-PR388-BuildData-equals

will automatically mark this PR as merged—GitHub only cares whether the PR branch head is an ancestor of its base branch’s head, not how you got there.

@MarkEWaite MarkEWaite merged commit 7e501ab into jenkinsci:master Mar 24, 2016

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the jglick:JENKINS-29326-redux branch Mar 24, 2016

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.