[JENKINS-43507] Allow SCMSource and SCMNavigator subtypes to share common traits #494

stephenc · 2017-05-04T10:42:11Z

Downstream of jenkinsci/scm-api-plugin#36

@reviewbybees

Todo

Ensure that the trait implementations work correctly when GitHub and BitBucky branch sources pick up traits

stephenc · 2017-05-04T10:43:09Z

@MarkEWaite @rsandell this PR is still somewhat in progress until I complete the TODO tasks... should be mostly stable enough to review

stephenc · 2017-05-04T10:47:01Z

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

-        if (revision instanceof SCMRevisionImpl) {
-            extensions.add(new BuildChooserSetting(new SpecificRevisionBuildChooser((SCMRevisionImpl) revision)));
+        GitSCMBuilder<?> builder = newBuilder(head, revision);
+        if (Util.isOverridden(AbstractGitSCMSource.class, getClass(), "getExtensions")) {


@rsandell This method is the one that needs great care and attention as we discussed previously.

The assumption is that any new subclasses cannot override these methods as they are @Restricted thus there are either no traits (because old implementations cannot have overridden getTraits()) or there are only traits...

Hence these methods just apply the equivalent decoration to the builder to result in the same behaviour as before...

Now I have convinced myself of the equivalence, but I really need you to probe my self-convictions ;-)

Looks OK except for my worry about the DoNotUse.class restriction.

ghost · 2017-05-04T10:54:36Z

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.

rsandell

nothing major. mostly concerned with the suspicious hashCode()s.

rsandell · 2017-05-15T09:27:38Z

src/main/java/hudson/plugins/git/extensions/impl/CheckoutOption.java

+     */
+    @Override
+    public int hashCode() {
+        return 0;


timeout.hashCode()?

the fields are all mutable, thus the hashcode must be constant... should be CheckoutOption.class.hashCode() if I am being fair to any hashsets/maps

So CheckoutOption.class.hashCode() then to be consistent with all the others.

rsandell · 2017-05-15T09:36:11Z

src/main/java/hudson/plugins/git/extensions/impl/CloneOption.java

+     */
+    @Override
+    public int hashCode() {
+        return CloneOption.class.hashCode();


Not really following the equals/hashCode contract here either.

the fields are all mutable, thus the hashcode must be constant...

If they where all imutable I would understand that argument.

The hashcode must be consistent or the object cannot be found again. Thus you must exclude mutable fields from the hashCode calculation.

If all fields are mutable then no fields may be used to calculate the hashCode.

Since we expect these to end up in collections with their sibling class implementations and we only ever have at most one impl of a given type, we just give each impl a different constant hashCode and all is right with the world

It would even be acceptable if they had the same hashCode, wouldn't it? They would collide in the hash buckets, but that would (at most) have a performance impact, not a functional impact.

rsandell · 2017-05-15T09:37:17Z

src/main/java/hudson/plugins/git/extensions/impl/LocalBranch.java

+     */
+    @Override
+    public int hashCode() {
+        return LocalBranch.class.hashCode();


rsandell · 2017-05-15T09:40:48Z

src/main/java/hudson/plugins/git/extensions/impl/SubmoduleOption.java

+     */
+    @Override
+    public int hashCode() {
+        return SubmoduleOption.class.hashCode();


rsandell · 2017-05-15T09:58:17Z

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

+                                      try {
+                                          hash = client.revParse(context.remoteName() + "/" + revision).name();
+                                      } catch (GitException x2) {
+                                          listener.getLogger().println(x.getMessage());


So in case of the first exception x, but not the second x2, there won't be anything printed to the log?

rsandell · 2017-05-15T11:10:03Z

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

+            RefSpecsSCMSourceTrait trait = asRefSpecsSCMSourceTrait(rawRefSpecs, remoteName);
+            if (trait != null) {
+                traits.add(trait);
+            }


should probably set most fields to null after the conversion to potentially be able to collect some garbage.

rsandell · 2017-05-15T11:13:05Z

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

@@ -188,43 +344,55 @@ public String getRemote() {
        return remote;
    }

-    @Override
-    public String getRemoteName() {


hard to see in this diff if all old removed signatures are removed or just moved.

rsandell · 2017-05-15T11:25:31Z

src/main/java/jenkins/plugins/git/traits/AuthorInChangelogTrait.java

+         */
+        @Override
+        public String getDisplayName() {
+            return "Use commit author in changelog";


I18n? Some display names might be reusable from the extensions.

rsandell · 2017-05-15T11:33:35Z

src/main/java/jenkins/plugins/git/traits/GitSCMExtensionTraitDescriptor.java

+    public String getHelpFile() {
+        String primary = super.getHelpFile();
+        return primary == null ? getExtensionDescriptor().getHelpFile() : primary;
+    }


not the tooltip? :-p

rsandell · 2017-05-15T11:41:17Z

src/main/resources/jenkins/plugins/git/GitSCMSource/help-excludes.html

- ~ THE SOFTWARE.
- -->
-
-<div>


shouldn't this just be moved to the trait?

…nd toString implementation requirements

…lways `origin`

…scriptor

…aseline version of Jenkins core

… by GitSCMSource should honour refspecs on clone and not fetch tags

rsandell

🐝

rsandell · 2017-06-21T10:51:46Z

src/main/java/hudson/plugins/git/extensions/impl/CheckoutOption.java

+     */
+    @Override
+    public int hashCode() {
+        return 0;


So CheckoutOption.class.hashCode() then to be consistent with all the others.

rsandell · 2017-06-21T13:17:36Z

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

+        if (refSpecs.isEmpty()) {
+            return "";
+        }
+        if (refSpecs.size() == 1) {


StringUtils.join(refSpecs, ' ');

rsandell · 2017-06-21T13:43:58Z

src/main/java/jenkins/plugins/git/traits/CheckoutOptionTrait.java

+         */
+        @Override
+        public String getDisplayName() {
+            return "Advanced checkout behaviours";


rsandell · 2017-06-21T13:58:22Z

src/main/java/jenkins/plugins/git/traits/RefSpecsSCMSourceTrait.java

+             */
+            @Override
+            public String getDisplayName() {
+                return "Ref Spec";


rsandell · 2017-06-21T14:16:52Z

src/test/java/jenkins/plugins/git/GitSCMBuilderTest.java

+        assertThat(instance.gitTool(), is("git"));
+        GitSCM scm = instance.build();
+        assertThat(scm.getBrowser(), is(nullValue()));
+        assertThat(scm.getUserRemoteConfigs(), contains(allOf(


Seems very excessive to assert on these all the time, since if there is a problem with it it will get reported on "unrelated" tests.

MarkEWaite

Started, but not finished the review. No issues found to date

MarkEWaite · 2017-06-21T21:35:02Z

src/main/java/hudson/plugins/git/extensions/impl/AuthorInChangelog.java

+        if (o == null || getClass() != o.getClass()) {
+            return false;
+        }
+        return o instanceof AuthorInChangelog;


Doesn't the check:

getClass() != o.getClass()

guarantee that o is an instanceof AuthorInChangelog? If so, isn't this more readily expressed as "return true"? Or is this worrying about the risk of a class which might inherit from this class and fail to override the equals method?

I think it is correct that every instance of AuthorInChangelog is equal to every other instance of AuthorInChangelog, since the AuthorInChangelog class is being used as a boolean. If AuthorInChangelog is enabled, then the author should be used rather than the committer when displaying changelogs.

MarkEWaite · 2017-06-21T21:42:50Z

src/main/java/hudson/plugins/git/extensions/impl/CloneOption.java

    @Override
    public void decorateFetchCommand(GitSCM scm, GitClient git, TaskListener listener, FetchCommand cmd) throws IOException, InterruptedException, GitException {
        cmd.shallow(shallow);
        if (shallow && depth > 1) {
-	    cmd.depth(depth);
+            cmd.depth(depth);


Leading tab before a space?

MarkEWaite · 2017-06-21T21:44:25Z

src/main/java/hudson/plugins/git/extensions/impl/CloneOption.java

+     */
+    @Override
+    public int hashCode() {
+        return CloneOption.class.hashCode();


It would even be acceptable if they had the same hashCode, wouldn't it? They would collide in the hash buckets, but that would (at most) have a performance impact, not a functional impact.

…ethod names

…on for JENKINS-33445

stephenc · 2017-07-05T21:50:59Z

Ok, merging to master in order to cut beta... any required changes will be in follow-up PRs

stephenc · 2017-07-05T21:51:29Z

https://github.com/jenkinsci/git-plugin/tree/stable-3.3.x was cut from master pre-merge in case any changes are needed to be released on the 3.3.x line

stephenc requested review from MarkEWaite and rsandell May 4, 2017 10:42

stephenc commented May 4, 2017

View reviewed changes

stephenc force-pushed the jenkins-43507 branch from 7934691 to 498d1b6 Compare May 4, 2017 13:43

rsandell requested changes May 15, 2017

View reviewed changes

stephenc mentioned this pull request Jun 12, 2017

[JENKINS-43507] Migrate Bitbucket Plugin to Traits jenkinsci/bitbucket-branch-source-plugin#53

Merged

stephenc force-pushed the jenkins-43507 branch from 498d1b6 to b8620ae Compare June 12, 2017 20:13

stephenc added 20 commits June 12, 2017 21:38

[JENKINS-43507] Check-pointing work in progress

84913c7

[JENKINS-43507] Fix test case in AbstractGitSCMSourceTest

1bb8905

[JENKINS-43507] Fix test case in AbstractGitSCMSourceRetrieveHeadsTest

802e7ce

[JENKINS-43507] More documentation

3c3336e

[JENKINS-43507] Documenting GitSCMBuilder

7c2ba1b

[JENKINS-43507] More javadoc documentation

c5c4116

[JENKINS-43507] More javadoc documentation (and more compliance too)

72cab2a

[JENKINS-43507] Document the traits

f09e7a2

[JENKINS-43507] Add a test for the GitSCMExtension equals, hashCode a…

f056b80

…nd toString implementation requirements

[JENKINS-43507] Document deprecated methods

55e9a98

[JENKINS-43507] Fix invalid implicit assumption that remote name is a…

7fd9e9f

…lways `origin`

[JENKINS-43507] Fix invalid implicit assumption that remote name is a…

aaaa28b

…lways `origin`

[JENKINS-43507] Documenting SCMNavigatorTrait and SCMNavigatorTraitDe…

b70866b

…scriptor

[JENKINS-43507] Should be first round complete for scm-api changes

01465e3

[JENKINS-43507] Fix the <scm:traits> taglib

7f32a8b

[JENKINS-43507] Towards 3.4.0

0993e7b

[JENKINS-43507] Remove dead help files

09afe78

[JENKINS-43507] Pickup SCM-API timestamped snapshot

ee664b5

[JENKINS-43507] Allow adding additional remotes to the GitSCMBuilder

6396748

[JENKINS-43507] Lower restrictions on deprecated methods

4cfcc51

stephenc force-pushed the jenkins-43507 branch from d3a77d5 to 4cfcc51 Compare June 12, 2017 20:39

stephenc added 5 commits June 12, 2017 22:30

[JENKINS-43507] Add workaround for JENKINS-30002 until available in b…

c3a66f2

…aseline version of Jenkins core

[JENKINS-43507] Handle refspec aggregation correctly and GitSCM built…

a2afab2

… by GitSCMSource should honour refspecs on clone and not fetch tags

[JENKINS-43507] Future-proof for if we implement TagSCMHead discovery

1f0d0e1

[JENKINS-43507] Pick up changes for JENKINS-44891

07ba4b5

[JENKINS-43507] Bump structs to 1.8 and fix tests

dd3e427

stephenc mentioned this pull request Jun 19, 2017

[JENKINS-43507] Migrate GitHub plugin to Traits jenkinsci/github-branch-source-plugin#141

Merged

stephenc added 4 commits June 20, 2017 12:08

[JENKINS-43507] Pick up alpha-1 of scm-api

a894dfb

[maven-release-plugin] prepare release git-3.4.0-alpha-1

68f1518

[maven-release-plugin] prepare for next development iteration

beda43c

[JENKINS-43507] US people want to spell things incorrectly

f52fe82

rsandell approved these changes Jun 21, 2017

View reviewed changes

MarkEWaite reviewed Jun 21, 2017

View reviewed changes

stephenc added 8 commits June 23, 2017 14:55

[JENKINS-43507] Found some minor bugs in the form validation action m…

15f218a

…ethod names

[JENKINS-43507] Give GitSCMSource a BranchDiscoveryTrait in preparati…

1c1c664

…on for JENKINS-33445

[JENKINS-43507] Fix trait drop-down list population

1743eab

[maven-release-plugin] prepare release git-3.4.0-alpha-4

1dd1e14

[maven-release-plugin] prepare for next development iteration

acdb6a9

[JENKINS-43507] Bump structs to 1.9

3343484

Merge branch 'master' into jenkins-43507

fe99a79

[JENKINS-43507] Pick up new releases

d7c3ece

MarkEWaite added the work-in-progress label Jul 5, 2017

[JENKINS-43507] Fix tests

af3d6fd

stephenc removed the work-in-progress label Jul 5, 2017

stephenc merged commit dd2b842 into jenkinsci:master Jul 5, 2017

stephenc deleted the jenkins-43507 branch July 5, 2017 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-43507] Allow SCMSource and SCMNavigator subtypes to share common traits #494

[JENKINS-43507] Allow SCMSource and SCMNavigator subtypes to share common traits #494

stephenc commented May 4, 2017 •

edited

Loading

stephenc commented May 4, 2017

stephenc May 4, 2017

rsandell May 15, 2017

ghost commented May 4, 2017

rsandell left a comment

rsandell May 15, 2017

stephenc May 15, 2017

rsandell Jun 21, 2017

rsandell May 15, 2017

stephenc May 15, 2017

rsandell May 15, 2017

stephenc May 15, 2017

MarkEWaite Jun 21, 2017

rsandell May 15, 2017

rsandell May 15, 2017

rsandell May 15, 2017

rsandell May 15, 2017

rsandell May 15, 2017

rsandell May 15, 2017

rsandell May 15, 2017

rsandell May 15, 2017

rsandell left a comment

rsandell Jun 21, 2017

rsandell Jun 21, 2017

rsandell Jun 21, 2017

rsandell Jun 21, 2017

rsandell Jun 21, 2017

MarkEWaite left a comment

MarkEWaite Jun 21, 2017

MarkEWaite Jun 21, 2017

MarkEWaite Jun 21, 2017

stephenc commented Jul 5, 2017

stephenc commented Jul 5, 2017

[JENKINS-43507] Allow SCMSource and SCMNavigator subtypes to share common traits #494

[JENKINS-43507] Allow SCMSource and SCMNavigator subtypes to share common traits #494

Conversation

stephenc commented May 4, 2017 • edited Loading

stephenc commented May 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 4, 2017

rsandell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsandell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkEWaite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc commented Jul 5, 2017

stephenc commented Jul 5, 2017

stephenc commented May 4, 2017 •

edited

Loading