Clean up

pom.xml

@@ -52,7 +52,7 @@
         <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
         <workflow.version>1.4</workflow.version>
         <jenkins.version>2.164.3</jenkins.version>
-        <powermock.version>1.6.2</powermock.version>
+        <powermock.version>1.7.4</powermock.version>
         <java.level>8</java.level>
         <findbugs.failOnError>false</findbugs.failOnError>
         <checkstyle.version>8.33</checkstyle.version>

src/main/java/org/jenkinsci/plugins/ghprb/Ghprb.java

@@ -132,7 +132,6 @@ public String checkBlackListCommitAuthor(String author) {
         Set<String> authors = getBlacklistedCommitAuthors();
         authors.remove("");
 
-        Map<Pattern, String> skipPatterns = new HashMap<Pattern, String>();
         for (String s : authors) {
             s = s.trim();
             if (compilePattern(s).matcher(author).matches()) {

src/main/java/org/jenkinsci/plugins/ghprb/GhprbBuilds.java

@@ -7,6 +7,7 @@
 import hudson.model.Run;
 import hudson.model.TaskListener;
 import hudson.model.queue.QueueTaskFuture;
+import hudson.plugins.git.Revision;
 import hudson.plugins.git.util.BuildData;
 import org.apache.commons.lang.StringUtils;
 import org.jenkinsci.plugins.ghprb.extensions.GhprbBuildStep;
@@ -186,7 +187,8 @@ public void onCompleted(Run<?, ?> build, TaskListener listener) {
         // having two of them, and because the one we added isn't correct
         // @see GhprbTrigger
         for (BuildData data : build.getActions(BuildData.class)) {
-            if (data.getLastBuiltRevision() != null && !data.getLastBuiltRevision().getSha1String().equals(c.getCommit())) {
+            final Revision revision = data.getLastBuiltRevision();
+            if (revision != null && !revision.getSha1String().equals(c.getCommit())) {
                 build.getActions().remove(data);
                 break;
             }

src/main/java/org/jenkinsci/plugins/ghprb/GhprbGitHubAuth.java

@@ -152,7 +152,7 @@ public boolean checkSignature(String body, String signature) {
                             new Object[] {localSignature, expected});
                     return false;
                 }
-            } catch (Exception e) {
+            } catch (Throwable e) {
                 LOGGER.log(Level.SEVERE, "Couldn't match both signatures");
                 return false;
             }

src/main/java/org/jenkinsci/plugins/ghprb/GhprbPullRequest.java

@@ -235,7 +235,9 @@ private void checkBlackListLabels() {
                         LOGGER.log(Level.INFO,
                                 "Found label {0} in ignore list, pull request will be ignored.",
                                 label.getName());
-                        shouldRun = false;
+                        synchronized (this) {
+                            shouldRun = false;
+                        }
                     }
                 }
             } catch (Error e) {
@@ -262,7 +264,9 @@ private void checkWhiteListLabels() {
 
                 if (!containsWhiteListLabel) {
                     LOGGER.log(Level.INFO, "Can't find any of whitelist label.");
-                    shouldRun = false;
+                    synchronized (this) {
+                        shouldRun = false;
+                    }
                 }
             } catch (Error e) {
                 LOGGER.log(Level.SEVERE, "Failed to read whitelist labels", e);
@@ -330,20 +334,20 @@ public void check(GHIssueComment comment) {
     private void updatePR(GHPullRequest ghpr, GHIssueComment comment, boolean isWebhook) {
         // Get the updated time
         try {
-            Date lastUpdateTime = updated;
-            Date updatedDate = comment != null ? comment.getUpdatedAt() : ghpr.getUpdatedAt();
-            // Don't log unless it was actually updated
-            if (updated == null || updated.compareTo(updatedDate) < 0) {
-                String user = comment != null ? comment.getUser().getName() : ghpr.getUser().getName();
-                LOGGER.log(
-                        Level.INFO,
-                        "Pull request #{0} was updated/initialized on {1} at {2} by {3} ({4})",
-                        new Object[] {this.id, this.repo.getName(), updatedDate, user,
-                                comment != null ? "comment" : "PR update"}
-                );
-            }
-
             synchronized (this) {
+                Date lastUpdateTime = updated;
+                Date updatedDate = comment != null ? comment.getUpdatedAt() : ghpr.getUpdatedAt();
+                // Don't log unless it was actually updated
+                if (updated == null || updated.compareTo(updatedDate) < 0) {
+                    String user = comment != null ? comment.getUser().getName() : ghpr.getUser().getName();
+                    LOGGER.log(
+                            Level.INFO,
+                            "Pull request #{0} was updated/initialized on {1} at {2} by {3} ({4})",
+                            new Object[] {this.id, this.repo.getName(), updatedDate, user,
+                                    comment != null ? "comment" : "PR update"}
+                    );
+                }
+
                 boolean wasUpdated = setUpdated(updatedDate);
 
                 // Update the PR object with the new pull request object if

src/main/java/org/jenkinsci/plugins/ghprb/GhprbPullRequestMerge.java

@@ -110,7 +110,8 @@ public void perform(
     ) throws InterruptedException, IOException {
         listener = taskListener;
         Job<?, ?> project = run.getParent();
-        if (run.getResult().isWorseThan(Result.SUCCESS)) {
+        Result result = run.getResult();
+        if (result != null && result.isWorseThan(Result.SUCCESS)) {
             listener.getLogger().println("Build did not succeed, merge will not be run");
             return;
         }

src/main/java/org/jenkinsci/plugins/ghprb/GhprbTrigger.java

@@ -51,6 +51,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.WeakHashMap;
 import java.util.concurrent.ConcurrentHashMap;
@@ -248,7 +249,9 @@ public static DescriptorImpl getDscp() {
     @SuppressWarnings("deprecation")
     private void initState() throws IOException {
 
-        final GithubProjectProperty ghpp = super.job.getProperty(GithubProjectProperty.class);
+        final Job<?, ?> job = super.job;
+        Objects.requireNonNull(job);
+        final GithubProjectProperty ghpp = job.getProperty(GithubProjectProperty.class);
         if (ghpp == null || ghpp.getProjectUrl() == null) {
             throw new IllegalStateException("A GitHub project url is required.");
         }
@@ -266,7 +269,7 @@ private void initState() throws IOException {
         this.pullRequests = null;
 
         try {
-            Map<Integer, GhprbPullRequest> prs = getDescriptor().getPullRequests(super.job.getFullName());
+            Map<Integer, GhprbPullRequest> prs = getDescriptor().getPullRequests(job.getFullName());
             if (prs != null) {
                 prs = new ConcurrentHashMap<Integer, GhprbPullRequest>(prs);
                 if (pulls == null) {
@@ -366,7 +369,8 @@ public void run() {
             return;
         }
 
-        LOGGER.log(Level.FINE, "Running trigger for {0}", super.job.getFullName());
+        Job<?, ?> job = this.job;
+        LOGGER.log(Level.FINE, "Running trigger for {0}", job == null ? "unknown Job" : job.getFullName());
 
         this.repository.check();
     }
@@ -480,10 +484,13 @@ private String escapeText(String text) {
 
     private ArrayList<ParameterValue> getDefaultParameters() {
         ArrayList<ParameterValue> values = new ArrayList<ParameterValue>();
-        ParametersDefinitionProperty pdp = this.job.getProperty(ParametersDefinitionProperty.class);
-        if (pdp != null) {
-            for (ParameterDefinition pd : pdp.getParameterDefinitions()) {
-                values.add(pd.getDefaultParameterValue());
+        Job<?, ?> job = this.job;
+        if (job != null) {
+            ParametersDefinitionProperty pdp = job.getProperty(ParametersDefinitionProperty.class);
+            if (pdp != null) {
+                for (ParameterDefinition pd : pdp.getParameterDefinitions()) {
+                    values.add(pd.getDefaultParameterValue());
+                }
             }
         }
         return values;
@@ -518,10 +525,11 @@ public GitHub getGitHub() throws IOException {
     }
 
     public void addWhitelist(String author) {
-        whitelist = whitelist + " " + author;
         try {
-            this.job.save();
-        } catch (IOException ex) {
+            Job<?, ?> job = Objects.requireNonNull(this.job);
+            whitelist = whitelist + " " + author;
+            job.save();
+        } catch (Exception ex) {
             LOGGER.log(Level.SEVERE, "Failed to save new whitelist", ex);
         }
     }

src/main/java/org/jenkinsci/plugins/ghprb/extensions/build/GhprbCancelBuildsOnUpdate.java

@@ -2,6 +2,7 @@
 
 import hudson.Extension;
 import hudson.model.Cause;
+import hudson.model.Executor;
 import hudson.model.Job;
 import hudson.model.Queue;
 import hudson.model.Result;
@@ -98,7 +99,10 @@ protected void cancelCurrentBuilds(Job<?, ?> project,
                                     + project.getName() + " for PR # " + cause.getPullID()
                     );
                     run.addAction(this);
-                    run.getExecutor().interrupt(Result.ABORTED);
+                    Executor executor = run.getExecutor();
+                    if (executor != null) {
+                        executor.interrupt(Result.ABORTED);
+                    }
                 } catch (Exception e) {
                     LOGGER.log(Level.SEVERE, "Error while trying to interrupt build!", e);
                 }

src/main/java/org/jenkinsci/plugins/ghprb/extensions/comments/GhprbCommentFile.java

@@ -15,6 +15,7 @@
 
 import java.io.File;
 import java.io.IOException;
+import java.util.Objects;
 
 public class GhprbCommentFile extends GhprbExtension implements GhprbCommentAppender, GhprbProjectExtension {
     @Extension
@@ -35,15 +36,15 @@ public boolean ignorePublishedUrl() {
         return false;
     }
 
-    public String postBuildComment(Run<?, ?> build, TaskListener listener) {
+    public String postBuildComment(final Run<?, ?> build, TaskListener listener) {
         StringBuilder msg = new StringBuilder();
         if (commentFilePath != null && !commentFilePath.isEmpty()) {
-            String scriptFilePathResolved = Ghprb.replaceMacros(build, listener, commentFilePath);
-
+            final String scriptFilePathResolved = Objects.requireNonNull(Ghprb.replaceMacros(build, listener, commentFilePath));
             try {
                 String content = null;
                 if (build instanceof Build<?, ?>) {
                     final FilePath workspace = ((Build<?, ?>) build).getWorkspace();
+                    assert workspace != null;
                     final FilePath path = workspace.child(scriptFilePathResolved);
 
                     if (path.exists()) {

src/main/java/org/jenkinsci/plugins/ghprb/extensions/status/GhprbSimpleStatus.java

@@ -124,7 +124,7 @@ public void onBuildTriggered(Job<?, ?> project,
         }
 
         String statusUrl = getDescriptor().getStatusUrlDefault(this);
-        if (commitStatusContext == "") {
+        if ("".equals(commitStatusContext)) {
             commitStatusContext = getDescriptor().getCommitStatusContextDefault(this);
         }
 

src/main/java/org/jenkinsci/plugins/ghprb/jobdsl/GhprbTriggerContext.java

@@ -300,19 +300,19 @@ public void excludedRegions(String regions) {
     }
 
     public void includedRegions(Iterable<String> regions) {
-        String includedRegionsStr = "";
+        StringBuilder builder = new StringBuilder();
         for (String region : regions) {
-            includedRegionsStr += (region + "\n");
+            builder.append(region).append("\n");
         }
-        includedRegions(includedRegionsStr);
+        includedRegions(builder.toString());
     }
 
     public void excludedRegions(Iterable<String> regions) {
-        String excludedRegionsStr = "";
+        StringBuilder builder = new StringBuilder();
         for (String region : regions) {
-            excludedRegionsStr += (region + "\n");
+            builder.append(region).append("\n");
         }
-        excludedRegions(excludedRegionsStr);
+        excludedRegions(builder.toString());
     }
 
 }

src/main/java/org/jenkinsci/plugins/ghprb/manager/impl/GhprbBaseBuildManager.java

@@ -88,7 +88,8 @@ protected String getAggregatedTestResults(Run<?, ?> build) {
 
         if (build.getResult() != Result.UNSTABLE) {
             sb.append("<h2>Build result: ");
-            sb.append(build.getResult().toString());
+            Result result = build.getResult();
+            sb.append(result == null ? "None" : result.toString());
             sb.append("</span></h2>");
 
             try {