From b760e06a9db62e6f1f2ce7d564b642959411a32f Mon Sep 17 00:00:00 2001 From: Thomas Keller Date: Fri, 28 Oct 2016 17:47:13 +0200 Subject: [PATCH 1/2] JENKINS-36703: Fix polling behaviour Properly return SCMRevisionState.NONE and handle the cases when we get this back from the API. --- .../java/hudson/plugins/repo/RepoScm.java | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/main/java/hudson/plugins/repo/RepoScm.java b/src/main/java/hudson/plugins/repo/RepoScm.java index 527a774..8fa29c6 100644 --- a/src/main/java/hudson/plugins/repo/RepoScm.java +++ b/src/main/java/hudson/plugins/repo/RepoScm.java @@ -596,7 +596,7 @@ public SCMRevisionState calcRevisionsFromBuild( // be called often. However it will be called if this is the first // build, if a build was aborted before it reported the repository // state, etc. - return null; + return SCMRevisionState.NONE; } private boolean shouldIgnoreChanges(final RevisionState current, final RevisionState baseline) { @@ -630,10 +630,10 @@ public PollingResult compareRemoteRevisionWith( final String expandedManifestBranch = env.expand(manifestBranch); final Run lastRun = job.getLastBuild(); - if (myBaseline == null) { + if (myBaseline == SCMRevisionState.NONE) { // Probably the first build, or possibly an aborted build. myBaseline = getLastState(lastRun, expandedManifestBranch); - if (myBaseline == null) { + if (myBaseline == SCMRevisionState.NONE) { return PollingResult.BUILD_NOW; } } @@ -664,7 +664,8 @@ public PollingResult compareRemoteRevisionWith( if (currentState.equals(myBaseline)) { change = Change.NONE; } else { - if (shouldIgnoreChanges(currentState, (RevisionState) myBaseline)) { + if (shouldIgnoreChanges(currentState, + myBaseline instanceof RevisionState ? (RevisionState) myBaseline : null)) { change = Change.NONE; } else { change = Change.SIGNIFICANT; @@ -708,12 +709,17 @@ public void checkout( build.addAction(currentState); final Run previousBuild = build.getPreviousBuild(); - final RevisionState previousState = + final SCMRevisionState previousState = getLastState(previousBuild, expandedBranch); if (changelogFile != null) { - ChangeLog.saveChangeLog(currentState, previousState, changelogFile, - launcher, repoDir, showAllChanges); + ChangeLog.saveChangeLog( + currentState, + previousState == SCMRevisionState.NONE ? null : (RevisionState) previousState, + changelogFile, + launcher, + repoDir, + showAllChanges); } build.addAction(new TagAction(build)); } @@ -876,10 +882,11 @@ private String getManifestRevision(final Launcher launcher, return manifestText; } - private RevisionState getLastState(final Run lastBuild, + @Nonnull + private SCMRevisionState getLastState(final Run lastBuild, final String expandedManifestBranch) { if (lastBuild == null) { - return null; + return RevisionState.NONE; } final RevisionState lastState = lastBuild.getAction(RevisionState.class); From ae08c754dd451a16f735a9173178b84932769c1d Mon Sep 17 00:00:00 2001 From: Thomas Keller Date: Fri, 28 Oct 2016 17:49:07 +0200 Subject: [PATCH 2/2] Some stylistic / NPE / safeguard changes. --- .../java/hudson/plugins/repo/ChangeLog.java | 23 +++++++++++-------- .../hudson/plugins/repo/ChangeLogEntry.java | 20 ++++++++-------- .../hudson/plugins/repo/ProjectState.java | 8 +++---- .../hudson/plugins/repo/RepoChangeLogSet.java | 6 ++--- .../java/hudson/plugins/repo/RepoScm.java | 6 ++--- .../hudson/plugins/repo/RevisionState.java | 18 +++++++++------ .../java/hudson/plugins/repo/TagAction.java | 3 +-- 7 files changed, 44 insertions(+), 40 deletions(-) diff --git a/src/main/java/hudson/plugins/repo/ChangeLog.java b/src/main/java/hudson/plugins/repo/ChangeLog.java index 7406cb1..259c2ee 100644 --- a/src/main/java/hudson/plugins/repo/ChangeLog.java +++ b/src/main/java/hudson/plugins/repo/ChangeLog.java @@ -30,7 +30,6 @@ import hudson.scm.ChangeLogParser; import hudson.scm.RepositoryBrowser; import hudson.util.AtomicFileWriter; -import hudson.util.IOException2; import hudson.util.XStream2; import java.io.BufferedReader; @@ -50,11 +49,14 @@ import com.thoughtworks.xstream.io.StreamException; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; + /** * Utility functions to generate and parse a file listing the differences * between builds. Differences are saved as a list of ChangeLogEntry. */ -public class ChangeLog extends ChangeLogParser { +class ChangeLog extends ChangeLogParser { private static Logger debug = Logger.getLogger("hudson.plugins.repo.ChangeLog"); @@ -64,6 +66,7 @@ public class ChangeLog extends ChangeLogParser { // for some possibilities. @Override + @SuppressWarnings("unchecked") public RepoChangeLogSet parse( final Run build, final RepositoryBrowser browser, final File changelogFile) throws IOException, SAXException { @@ -103,9 +106,9 @@ public RepoChangeLogSet parse( * is thrown if we are interrupted while waiting on the git * commands to run in a forked process. */ - public static List generateChangeLog( - final RevisionState currentState, - final RevisionState previousState, final Launcher launcher, + private static List generateChangeLog( + @Nonnull final RevisionState currentState, + @Nullable final RevisionState previousState, final Launcher launcher, final FilePath workspace, final boolean showAllChanges) throws IOException, InterruptedException { @@ -238,10 +241,10 @@ public static List generateChangeLog( * is thrown if we are interrupted while waiting on the git * commands to run in a forked process. */ - public static void saveChangeLog(final RevisionState currentState, - final RevisionState previousState, final File changelogFile, - final Launcher launcher, final FilePath workspace, - final boolean showAllChanges) + static void saveChangeLog(@Nonnull final RevisionState currentState, + @Nullable final RevisionState previousState, final File changelogFile, + final Launcher launcher, final FilePath workspace, + final boolean showAllChanges) throws IOException, InterruptedException { List logs = generateChangeLog(currentState, previousState, launcher, @@ -259,7 +262,7 @@ public static void saveChangeLog(final RevisionState currentState, xs.toXML(logs, w); w.commit(); } catch (final StreamException e) { - throw new IOException2(e); + throw new IOException("Could not save changelog", e); } finally { w.close(); } diff --git a/src/main/java/hudson/plugins/repo/ChangeLogEntry.java b/src/main/java/hudson/plugins/repo/ChangeLogEntry.java index b5580e8..12aa8f1 100644 --- a/src/main/java/hudson/plugins/repo/ChangeLogEntry.java +++ b/src/main/java/hudson/plugins/repo/ChangeLogEntry.java @@ -36,21 +36,21 @@ * A POJO containing information about a single change (git commit) in a git * repository. These objects are used to build the change log page. */ -public class ChangeLogEntry extends ChangeLogSet.Entry { +class ChangeLogEntry extends ChangeLogSet.Entry { /** * A POJO containing information about a modified file. A RepoChangeLogEntry * contains a list of ModifiedFiles. We track the file path and how it was * modified (added, edited, removed, etc). */ - public static class ModifiedFile implements AffectedFile { + static class ModifiedFile implements AffectedFile { /** * An EditType for a Renamed file. Most version control systems don't * support file renames, so this EditType isn't in the default set * provided by Hudson. */ - public static final EditType RENAME = new EditType("rename", + static final EditType RENAME = new EditType("rename", "The file was renamed"); private final String path; @@ -65,7 +65,7 @@ public static class ModifiedFile implements AffectedFile { * the action performed on the file, as reported by Git (A * for add, D for delete, M for modified, etc) */ - public ModifiedFile(final String path, final char action) { + ModifiedFile(final String path, final char action) { this.path = path; this.action = action; } @@ -145,12 +145,12 @@ public EditType getEditType() { // CS IGNORE ParameterNumber FOR NEXT 16 LINES. REASON: I've got no // better ideas. Passing in all the variables here makes sense to me, even // if it is ugly. - public ChangeLogEntry(final String path, final String serverPath, - final String revision, final String authorName, - final String authorEmail, final String authorDate, - final String committerName, final String committerEmail, - final String committerDate, final String commitText, - final List modifiedFiles) { + ChangeLogEntry(final String path, final String serverPath, + final String revision, final String authorName, + final String authorEmail, final String authorDate, + final String committerName, final String committerEmail, + final String committerDate, final String commitText, + final List modifiedFiles) { this.path = path; this.serverPath = serverPath; this.revision = revision; diff --git a/src/main/java/hudson/plugins/repo/ProjectState.java b/src/main/java/hudson/plugins/repo/ProjectState.java index d41fb9c..8557bd7 100644 --- a/src/main/java/hudson/plugins/repo/ProjectState.java +++ b/src/main/java/hudson/plugins/repo/ProjectState.java @@ -33,7 +33,7 @@ * when projects have changed. A repo manifest contains a list of projects, and * a build in Hudson has a list of ProjectStates. */ -public final class ProjectState { +final class ProjectState { private final String path; private final String serverPath; @@ -57,7 +57,7 @@ public final class ProjectState { * @param revision * The SHA-1 revision of the project */ - public static synchronized ProjectState constructCachedInstance( + static synchronized ProjectState constructCachedInstance( final String path, final String serverPath, final String revision) { ProjectState projectState = projectStateCache.get( @@ -156,8 +156,8 @@ public int hashCode() { * @param revision * The SHA-1 revision of the project */ - public static int calculateHashCode(final String path, - final String serverPath, final String revision) { + private static int calculateHashCode(final String path, + final String serverPath, final String revision) { return 23 + (path == null ? 37 : path.hashCode()) + (serverPath == null ? 97 : serverPath.hashCode()) + (revision == null ? 389 : revision.hashCode()); diff --git a/src/main/java/hudson/plugins/repo/RepoChangeLogSet.java b/src/main/java/hudson/plugins/repo/RepoChangeLogSet.java index 1fd00b4..9e92122 100644 --- a/src/main/java/hudson/plugins/repo/RepoChangeLogSet.java +++ b/src/main/java/hudson/plugins/repo/RepoChangeLogSet.java @@ -34,7 +34,7 @@ * A ChangeLogSet, which is used when generating the list of changes from one * build to the next. */ -public class RepoChangeLogSet extends ChangeLogSet { +class RepoChangeLogSet extends ChangeLogSet { private final List logs; /** @@ -49,8 +49,8 @@ public class RepoChangeLogSet extends ChangeLogSet { * a list of RepoChangeLogEntry, containing every change (commit) * which has occurred since the last build. */ - protected RepoChangeLogSet(final Run build, - final RepositoryBrowser browser, final List logs) { + RepoChangeLogSet(final Run build, + final RepositoryBrowser browser, final List logs) { super(build, browser); this.logs = logs; for (final ChangeLogEntry log : logs) { diff --git a/src/main/java/hudson/plugins/repo/RepoScm.java b/src/main/java/hudson/plugins/repo/RepoScm.java index 8fa29c6..1116618 100644 --- a/src/main/java/hudson/plugins/repo/RepoScm.java +++ b/src/main/java/hudson/plugins/repo/RepoScm.java @@ -765,10 +765,8 @@ private int doSync(final Launcher launcher, final FilePath workspace, commands.add("--no-tags"); } - int returnCode = - launcher.launch().stdout(logger).pwd(workspace) - .cmds(commands).envs(env).join(); - return returnCode; + return launcher.launch().stdout(logger).pwd(workspace) + .cmds(commands).envs(env).join(); } private boolean checkoutCode(final Launcher launcher, diff --git a/src/main/java/hudson/plugins/repo/RevisionState.java b/src/main/java/hudson/plugins/repo/RevisionState.java index c96cb49..928558e 100644 --- a/src/main/java/hudson/plugins/repo/RevisionState.java +++ b/src/main/java/hudson/plugins/repo/RevisionState.java @@ -38,6 +38,7 @@ import java.util.logging.Level; import java.util.logging.Logger; +import javax.annotation.Nullable; import javax.xml.parsers.DocumentBuilderFactory; import org.w3c.dom.Document; @@ -50,7 +51,7 @@ * It is used to see what changed from build to build. */ @SuppressWarnings("serial") -public class RevisionState extends SCMRevisionState implements Serializable { +class RevisionState extends SCMRevisionState implements Serializable { private final String manifest; private final Map projects = @@ -72,8 +73,8 @@ public class RevisionState extends SCMRevisionState implements Serializable { * @param logger * A PrintStream for logging errors */ - public RevisionState(final String manifest, final String manifestRevision, - final String branch, final PrintStream logger) { + RevisionState(final String manifest, final String manifestRevision, + final String branch, @Nullable final PrintStream logger) { this.manifest = manifest; this.branch = branch; try { @@ -84,7 +85,9 @@ public RevisionState(final String manifest, final String manifestRevision, .parse(xmlSource); if (!doc.getDocumentElement().getNodeName().equals("manifest")) { - logger.println("Error - malformed manifest"); + if (logger != null) { + logger.println("Error - malformed manifest"); + } return; } final NodeList projectNodes = doc.getElementsByTagName("project"); @@ -124,8 +127,9 @@ public RevisionState(final String manifest, final String manifestRevision, } catch (final Exception e) { - logger.println(e); - return; + if (logger != null) { + logger.println(e); + } } } @@ -187,7 +191,7 @@ public String getRevision(final String path) { * @return A List of ProjectStates from the previous repo state which have * since been updated. */ - public List whatChanged(final RevisionState previousState) { + List whatChanged(@Nullable final RevisionState previousState) { final List changes = new ArrayList(); if (previousState == null) { // Everything is new. The change log would include every change, diff --git a/src/main/java/hudson/plugins/repo/TagAction.java b/src/main/java/hudson/plugins/repo/TagAction.java index 13c8110..27dab29 100644 --- a/src/main/java/hudson/plugins/repo/TagAction.java +++ b/src/main/java/hudson/plugins/repo/TagAction.java @@ -85,7 +85,6 @@ public boolean isTagged() { public String getManifest() { final RevisionState revisionState = getRun().getAction(RevisionState.class); - final String manifest = revisionState.getManifest(); - return manifest; + return revisionState.getManifest(); } }