[FIXED HUDSON-2180] revised the design of polling to fix the problem.

See:

  - SCM.compareRemoteRevisionWith
  - SCM.calcRevisionsFromBuild
  - SCMRevisionState

for details. This feature was originally developed in a branch,
whose merge records are as follows:

------------------

Merged revisions 24271,27060,27065,27083-27084 via svnmerge from 
https://www.dev.java.net/svn/hudson/branches/HUDSON-2180/main

........
  r24271 | kohsuke | 2009-11-30 19:41:30 -0800 (Mon, 30 Nov 2009) | 1 line
  
  committing my work in progress
........
  r27060 | kohsuke | 2010-02-05 18:11:05 -0800 (Fri, 05 Feb 2010) | 7 lines
  
  Made SVNRevisionState non-comparable, since doing so and distinguishing
  significant/insignificant changes is rather involving work.
  
  So that's why I originally opted for the compareRemoteRevisionWith method approach
  of telling SCM upfront about what it's comparing the state with.
........
  r27065 | kohsuke | 2010-02-06 08:49:50 -0800 (Sat, 06 Feb 2010) | 1 line
  
  crucial bug fix
........
  r27083 | kohsuke | 2010-02-06 10:37:46 -0800 (Sat, 06 Feb 2010) | 1 line
  
  doc improvement
........
  r27084 | kohsuke | 2010-02-06 10:39:50 -0800 (Sat, 06 Feb 2010) | 1 line
  
  doc improvement
........


git-svn-id: https://hudson.dev.java.net/svn/hudson/trunk/hudson/main@27103 71c3de6d-444a-0410-be80-ed276b4c234a

core/src/main/java/hudson/fsp/WorkspaceSnapshotSCM.java

@@ -23,9 +23,11 @@
  */
 package hudson.fsp;
 
+import hudson.scm.PollingResult;
 import hudson.scm.SCM;
 import hudson.scm.ChangeLogParser;
 import hudson.scm.SCMDescriptor;
+import hudson.scm.SCMRevisionState;
 import hudson.model.AbstractProject;
 import hudson.model.TaskListener;
 import hudson.model.AbstractBuild;
@@ -118,15 +120,12 @@ public class WorkspaceSnapshotSCM extends SCM {
         return new Snapshot(snapshot,b);
     }
 
-    public boolean pollChanges(AbstractProject project, Launcher launcher, FilePath workspace, TaskListener listener) throws IOException, InterruptedException {
-        AbstractBuild lastBuild = (AbstractBuild) project.getLastBuild();
-        if (lastBuild == null) {
-            listener.getLogger().println("No existing build. Starting a new one");
-            return true;
-        }
-
+    public SCMRevisionState calcRevisionsFromBuild(AbstractBuild<?, ?> build, Launcher launcher, TaskListener listener) throws IOException, InterruptedException {
+        return null;
+    }
 
-        return false;
+    protected PollingResult compareRemoteRevisionWith(AbstractProject project, Launcher launcher, FilePath workspace, TaskListener listener, SCMRevisionState baseline) throws IOException, InterruptedException {
+        return PollingResult.NO_CHANGES;
     }
 
     public boolean checkout(AbstractBuild build, Launcher launcher, FilePath workspace, BuildListener listener, File changelogFile) throws IOException, InterruptedException {

core/src/main/java/hudson/model/AbstractProject.java

@@ -44,6 +44,10 @@ import hudson.scm.ChangeLogSet.Entry;
 import hudson.scm.NullSCM;
 import hudson.scm.SCM;
 import hudson.scm.SCMS;
+import hudson.scm.PollingResult;
+import hudson.scm.SCMRevisionState;
+import static hudson.scm.PollingResult.NO_CHANGES;
+import static hudson.scm.PollingResult.BUILD_NOW;
 import hudson.search.SearchIndexBuilder;
 import hudson.security.Permission;
 import hudson.tasks.BuildStep;
@@ -107,6 +111,11 @@ public abstract class AbstractProject<P extends AbstractProject<P,R>,R extends A
      */
     private volatile SCM scm = new NullSCM();
 
+    /**
+     * State returned from {@link SCM#poll(AbstractProject, Launcher, FilePath, TaskListener, SCMRevisionState)}.
+     */
+    private volatile transient SCMRevisionState pollingBaseline = null;
+
     /**
      * All the builds keyed by their build number.
      */
@@ -1011,7 +1020,34 @@ public abstract class AbstractProject<P extends AbstractProject<P,R>,R extends A
 
         FilePath workspace = build.getWorkspace();
         workspace.mkdirs();
-        return scm.checkout(build, launcher, workspace, listener, changelogFile);
+        
+        boolean r = scm.checkout(build, launcher, workspace, listener, changelogFile);
+        calcPollingBaseline(build, launcher, listener);
+        return r;
+    }
+
+    /**
+     * Pushes the baseline up to the newly checked out revision.
+     */
+    private void calcPollingBaseline(AbstractBuild build, Launcher launcher, TaskListener listener) throws IOException, InterruptedException {
+        SCMRevisionState baseline = build.getAction(SCMRevisionState.class);
+        if (baseline==null) {
+            try {
+                baseline = safeCalcRevisionsFromBuild(build, launcher, listener);
+            } catch (AbstractMethodError e) {
+                baseline = SCMRevisionState.NONE; // pre-1.345 SCM implementations, which doesn't use the baseline in polling
+            }
+            if (baseline!=null)
+                build.addAction(baseline);
+        }
+        pollingBaseline = baseline;
+    }
+
+    /**
+     * For reasons I don't understand, if I inline this method, AbstractMethodError escapes try/catch block.
+     */
+    private SCMRevisionState safeCalcRevisionsFromBuild(AbstractBuild build, Launcher launcher, TaskListener listener) throws IOException, InterruptedException {
+        return getScm().calcRevisionsFromBuild(build, launcher, listener);
     }
 
     /**
@@ -1022,22 +1058,54 @@ public abstract class AbstractProject<P extends AbstractProject<P,R>,R extends A
      * a build and polling, as both touches the workspace.
      */
     public boolean pollSCMChanges( TaskListener listener ) {
+        return poll(listener).hasChanges();
+    }
+
+    /**
+     * Checks if there's any update in SCM, and returns true if any is found.
+     *
+     * <p>
+     * The caller is responsible for coordinating the mutual exclusion between
+     * a build and polling, as both touches the workspace.
+     *
+     * @since 1.345
+     */
+    public PollingResult poll( TaskListener listener ) {
         SCM scm = getScm();
         if(scm==null) {
             listener.getLogger().println(Messages.AbstractProject_NoSCM());
-            return false;
+            return NO_CHANGES;
         }
         if(isDisabled()) {
             listener.getLogger().println(Messages.AbstractProject_Disabled());
-            return false;
+            return NO_CHANGES;
+        }
+
+        R lb = getLastBuild();
+        if (lb==null) {
+            listener.getLogger().println("No builds have done yet. Scheduling a new one");
+            return BUILD_NOW;
+        }
+
+        if (pollingBaseline==null) {
+            R success = getLastSuccessfulBuild(); // if we have a persisted baseline, we'll find it by this
+            for (R r=lb; r!=null; r=r.getPreviousBuild()) {
+                SCMRevisionState s = r.getAction(SCMRevisionState.class);
+                if (s!=null) {
+                    pollingBaseline = s;
+                    break;
+                }
+                if (r==success) break;  // searched far enough
+            }
+            // NOTE-NO-BASELINE:
+            // if we don't have baseline yet, it means the data is built by old Hudson that doesn't set the baseline
+            // as action, so we need to compute it. This happens later.
         }
 
         try {
             if(scm.requiresWorkspaceForPolling()) {
                 // lock the workspace of the last build
-                FilePath ws=null;
-                R lb = getLastBuild();
-                if (lb!=null)   ws = lb.getWorkspace();
+                FilePath ws=lb.getWorkspace();
 
                 if (ws==null || !ws.exists()) {
                     // workspace offline. build now, or nothing will ever be built
@@ -1046,14 +1114,13 @@ public abstract class AbstractProject<P extends AbstractProject<P,R>,R extends A
                         // if the build is fixed on a node, then attempting a build will do us
                         // no good. We should just wait for the slave to come back.
                         listener.getLogger().println(Messages.AbstractProject_NoWorkspace());
-                        return false;
+                        return NO_CHANGES;
                     }
-                    if (ws == null)
-                        listener.getLogger().println(Messages.AbstractProject_WorkspaceOffline());
-                    else
-                        listener.getLogger().println(Messages.AbstractProject_NoWorkspace());
+                    listener.getLogger().println( ws==null
+                        ? Messages.AbstractProject_WorkspaceOffline()
+                        : Messages.AbstractProject_NoWorkspace());
                     listener.getLogger().println(Messages.AbstractProject_NewBuildForWorkspace());
-                    return true;
+                    return BUILD_NOW;
                 } else {
                     WorkspaceList l = lb.getBuiltOn().toComputer().getWorkspaceList();
                     // if doing non-concurrent build, acquite a workspace in a way that causes builds to block for this workspace.
@@ -1063,9 +1130,14 @@ public abstract class AbstractProject<P extends AbstractProject<P,R>,R extends A
                     // so better throughput is achieved over time (modulo the initial cost of creating that many workspaces)
                     // by having multiple workspaces
                     WorkspaceList.Lease lease = l.acquire(ws, !concurrentBuild);
+                    Launcher launcher = ws.createLauncher(listener);
                     try {
                         LOGGER.fine("Polling SCM changes of " + getName());
-                        return scm.pollChanges(this, ws.createLauncher(listener), ws, listener);
+                        if (pollingBaseline==null) // see NOTE-NO-BASELINE above
+                            calcPollingBaseline(lb,launcher,listener);
+                        PollingResult r = scm.poll(this, launcher, ws, listener, pollingBaseline);
+                        pollingBaseline = r.remote;
+                        return r;
                     } finally {
                         lease.release();
                     }
@@ -1073,18 +1145,23 @@ public abstract class AbstractProject<P extends AbstractProject<P,R>,R extends A
             } else {
                 // polling without workspace
                 LOGGER.fine("Polling SCM changes of " + getName());
-                return scm.pollChanges(this, null, null, listener);
+
+                if (pollingBaseline==null) // see NOTE-NO-BASELINE above
+                    calcPollingBaseline(lb,null,listener);
+                PollingResult r = scm.poll(this, null, null, listener, pollingBaseline);
+                pollingBaseline = r.remote;
+                return r;
             }
         } catch (AbortException e) {
             listener.fatalError(Messages.AbstractProject_Aborted());
             LOGGER.log(Level.FINE, "Polling "+this+" aborted",e);
-            return false;
+            return NO_CHANGES;
         } catch (IOException e) {
             e.printStackTrace(listener.fatalError(e.getMessage()));
-            return false;
+            return NO_CHANGES;
         } catch (InterruptedException e) {
             e.printStackTrace(listener.fatalError(Messages.AbstractProject_PollingABorted()));
-            return false;
+            return NO_CHANGES;
         }
     }
 
@@ -1338,7 +1415,6 @@ public abstract class AbstractProject<P extends AbstractProject<P,R>,R extends A
         ParametersDefinitionProperty pp = getProperty(ParametersDefinitionProperty.class);
         if (pp != null) {
             pp.buildWithParameters(req,rsp);
-            return;
         } else {
         	throw new IllegalStateException("This build is not parameterized!");
         }

core/src/main/java/hudson/scm/NullSCM.java

@@ -42,9 +42,12 @@ import java.io.IOException;
  * @author Kohsuke Kawaguchi
  */
 public class NullSCM extends SCM {
-    public boolean pollChanges(AbstractProject project, Launcher launcher, FilePath dir, TaskListener listener) throws IOException {
-        // no change
-        return false;
+    public SCMRevisionState calcRevisionsFromBuild(AbstractBuild<?, ?> build, Launcher launcher, TaskListener listener) throws IOException, InterruptedException {
+        return null;
+    }
+
+    protected PollingResult compareRemoteRevisionWith(AbstractProject project, Launcher launcher, FilePath workspace, TaskListener listener, SCMRevisionState baseline) throws IOException, InterruptedException {
+        return PollingResult.NO_CHANGES;
     }
 
     public boolean checkout(AbstractBuild build, Launcher launcher, FilePath remoteDir, BuildListener listener, File changeLogFile) throws IOException, InterruptedException {

core/src/main/java/hudson/scm/PollingResult.java

+package hudson.scm;
+
+import hudson.model.AbstractProject;
+import hudson.model.TaskListener;
+import hudson.Launcher;
+import hudson.FilePath;
+
+import java.io.Serializable;
+
+/**
+ * Immutable object that represents the result of {@linkplain SCM#poll(AbstractProject, Launcher, FilePath, TaskListener, SCMRevisionState) SCM polling}.
+ *
+ * <p>
+ * This object is marked serializable just to be remoting friendly &mdash; Hudson by itself
+ * doesn't persist this object.
+ *
+ * @author Kohsuke Kawaguchi
+ * @since 1.345
+ */
+public final class PollingResult implements Serializable {
+    /**
+     * Baseline of the comparison.
+     * (This comes from either the workspace, or from the remote repository as of the last polling.
+     * Can be null.
+     */
+    public final SCMRevisionState baseline;
+
+    /**
+     * Current state of the remote repository. To be passed to the next invocation of the polling method.
+     * Can be null.
+     */
+    public final SCMRevisionState remote;
+
+    /**
+     * Degree of the change between baseline and remote. Never null.
+     * <p>
+     * The fact that this field is independent from {@link #baseline} and {@link #remote} are
+     * used to (1) allow the {@linkplain Change#INCOMPARABLE incomparable} state which forces
+     * the immediate rebuild, and (2) allow SCM to ignore some changes in the repository to implement
+     * exclusion feature.
+     */
+    public final Change change;
+
+    /**
+     * Degree of changes between the previous state and this state.
+     */
+    public enum Change {
+        /**
+         * No change. Two {@link SCMRevisionState}s point to the same state of the same repository / the same commit.
+         */
+        NONE,
+        /**
+         * There are some changes between states, but those changes are not significant enough to consider
+         * a new build. For example, some SCMs allow certain commits to be marked as excluded, and this is how
+         * you can do it.
+         */
+        INSIGNIFICANT,
+        /**
+         * There are changes between states that warrant a new build. Hudson will eventually
+         * schedule a new build for this change, subject to other considerations
+         * such as the quiet period.
+         */
+        SIGNIFICANT,
+        /**
+         * The state as of baseline is so different from the current state that they are incomparable
+         * (for example, the workspace and the remote repository points to two unrelated repositories
+         * because the configuration has changed.) This forces Hudson to schedule a build right away.
+         * <p>
+         * This is primarily useful in SCM implementations that require a workspace to be able
+         * to perform a polling. SCMs that can always compare remote revisions regardless of the local
+         * state should do so, and never return this constant, to let Hudson maintain the quiet period
+         * behavior all the time.
+         * <p>
+         * This constant is not to be confused with the errors encountered during polling, which
+         * should result in an exception and eventual retry by Hudson.
+         */
+        INCOMPARABLE
+    }
+
+    public PollingResult(SCMRevisionState baseline, SCMRevisionState remote, Change change) {
+        if (change==null)   throw new IllegalArgumentException();
+        this.baseline = baseline;
+        this.remote = remote;
+        this.change = change;
+    }
+
+    public PollingResult(Change change) {
+        this(null,null,change);
+    }
+
+    public boolean hasChanges() {
+        return change.ordinal() > Change.INSIGNIFICANT.ordinal();
+    }
+
+    /**
+     * Constant to indicate no changes in the remote repository.
+     */
+    public static final PollingResult NO_CHANGES = new PollingResult(Change.NONE);
+
+    public static final PollingResult SIGNIFICANT = new PollingResult(Change.SIGNIFICANT);
+
+    /**
+     * Constant that uses {@link Change#INCOMPARABLE} which forces an immediate build.
+     */
+    public static final PollingResult BUILD_NOW = new PollingResult(Change.INCOMPARABLE);
+
+    private static final long serialVersionUID = 1L;
+}

core/src/main/java/hudson/scm/SCM.java

@@ -41,6 +41,7 @@ import hudson.model.WorkspaceCleanupThread;
 import hudson.model.Hudson;
 import hudson.model.Descriptor;
 import hudson.model.Api;
+import hudson.model.Action;
 import hudson.model.AbstractProject.AbstractProjectDescriptor;
 
 import java.io.File;
@@ -133,7 +134,7 @@ public abstract class SCM implements Describable<SCM>, ExtensionPoint {
 
     /**
      * Returns true if this SCM supports
-     * {@link #pollChanges(AbstractProject, Launcher, FilePath, TaskListener) polling}.
+     * {@link #poll(AbstractProject, Launcher, FilePath, TaskListener, SCMRevisionState) poling}.
      *
      * @since 1.105
      */
@@ -236,8 +237,113 @@ public abstract class SCM implements Describable<SCM>, ExtensionPoint {
      *      this exception should be simply propagated all the way up.
      *
      * @see #supportsPolling()
+     *
+     * @deprecated as of 1.345
+     *      Override {@link #calcRevisionsFromBuild(AbstractBuild, Launcher, TaskListener)} and
+     *      {@link #compareRemoteRevisionWith(AbstractProject, Launcher, FilePath, TaskListener, SCMRevisionState)} for implementation.
+     *
+     *      The implementation is now separated in two pieces, one that computes the revision of the current workspace,
+     *      and the other that computes the revision of the remote repository.
+     *
+     *      Call {@link #poll(AbstractProject, Launcher, FilePath, TaskListener, SCMRevisionState)} for use instead.
+     */
+    public boolean pollChanges(AbstractProject project, Launcher launcher, FilePath workspace, TaskListener listener) throws IOException, InterruptedException {
+        // up until 1.336, this method was abstract, so everyone should have overridden this method
+        // without calling super.pollChanges. So the compatibility implementation is purely for
+        // new implementations that doesn't override this method.
+
+        // not sure if this can be implemented any better
+        return false;
+    }
+
+    /**
+     * Calculates the {@link SCMRevisionState} that represents the state of the workspace of the given build.
+     *
+     * <p>
+     * The returned object is then fed into the
+     * {@link #compareRemoteRevisionWith(AbstractProject, Launcher, FilePath, TaskListener, SCMRevisionState)} method
+     * as the baseline {@link SCMRevisionState} to determine if the build is necessary.
+     *
+     * <p>
+     * This method is called after source code is checked out for the given build (that is, after
+     * {@link SCM#checkout(AbstractBuild, Launcher, FilePath, BuildListener, File)} has finished successfully.)
+     *
+     * <p>
+     * The obtained object is added to the build as an {@link Action} for later retrieval. As an optimization,
+     * {@link SCM} implementation can choose to compute {@link SCMRevisionState} and add it as an action
+     * during check out, in which case this method will not called. 
+     *
+     * @param build
+     *      The calculated {@link SCMRevisionState} is for the files checked out in this build. Never null.
+     *      If {@link #requiresWorkspaceForPolling()} returns true, Hudson makes sure that the workspace of this
+     *      build is available and accessible by the callee.
+     * @param launcher
+     *      Abstraction of the machine where the polling will take place. If SCM declares
+     *      that {@linkplain #requiresWorkspaceForPolling() the polling doesn't require a workspace},
+     *      this parameter is null. Otherwise never null.
+     * @param listener
+     *      Logs during the polling should be sent here.
+     *
+     * @return can be null.
+     *
+     * @throws InterruptedException
+     *      interruption is usually caused by the user aborting the computation.
+     *      this exception should be simply propagated all the way up. 
+     */
+    public abstract SCMRevisionState calcRevisionsFromBuild(AbstractBuild<?,?> build, Launcher launcher, TaskListener listener) throws IOException, InterruptedException;
+
+    /**
+     * Compares the current state of the remote repository against the given baseline {@link SCMRevisionState}.
+     *
+     * <p>
+     * Conceptually, the act of polling is to take two states of the repository and to compare them to see
+     * if there's any difference. In practice, however, comparing two arbitrary repository states is an expensive
+     * operation, so in this abstraction, we chose to mix (1) the act of building up a repository state and
+     * (2) the act of comparing it with the earlier state, so that SCM implementations can implement this
+     * more easily. 
+     *
+     * <p>
+     * Multiple invocations of this method may happen over time to make sure that the remote repository
+     * is "quiet" before Hudson schedules a new build.
+     *
+     * @param project
+     *      The project to check for updates
+     * @param launcher
+     *      Abstraction of the machine where the polling will take place. If SCM declares
+     *      that {@linkplain #requiresWorkspaceForPolling() the polling doesn't require a workspace}, this parameter is null.
+     * @param workspace
+     *      The workspace directory that contains baseline files. If SCM declares
+     *      that {@linkplain #requiresWorkspaceForPolling() the polling doesn't require a workspace}, this parameter is null.
+     * @param listener
+     *      Logs during the polling should be sent here.
+     * @param baseline
+     *      The baseline of the comparison. This object is the return value from earlier
+     *      {@link #compareRemoteRevisionWith(AbstractProject, Launcher, FilePath, TaskListener, SCMRevisionState)} or
+     *      {@link #calcRevisionsFromBuild(AbstractBuild, Launcher, TaskListener)}.
+     *
+     * @return
+     *      This method returns multiple values that are bundled together into the {@link PollingResult} value type.
+     *      {@link PollingResult#baseline} should be the value of the baseline parameter, {@link PollingResult#remote}
+     *      is the current state of the remote repository (this object only needs to be understandable to the future
+     *      invocations of this method),
+     *      and {@link PollingResult#change} that indicates the degree of changes found during the comparison.
+     *
+     * @throws InterruptedException
+     *      interruption is usually caused by the user aborting the computation.
+     *      this exception should be simply propagated all the way up.
      */
-    public abstract boolean pollChanges(AbstractProject project, Launcher launcher, FilePath workspace, TaskListener listener) throws IOException, InterruptedException;
+    protected abstract PollingResult compareRemoteRevisionWith(AbstractProject<?,?> project, Launcher launcher, FilePath workspace, TaskListener listener, SCMRevisionState baseline) throws IOException, InterruptedException;
+
+    /**
+     * Convenience method for the caller to handle the backward compatibility between pre 1.345 SCMs.
+     */
+    public final PollingResult poll(AbstractProject<?,?> project, Launcher launcher, FilePath workspace, TaskListener listener, SCMRevisionState baseline) throws IOException, InterruptedException {
+        try {
+            return compareRemoteRevisionWith(project,launcher,workspace,listener,baseline);
+        } catch (AbstractMethodError e) {// pre 1.345 SCM that doesn't implement new polling methods
+            return pollChanges(project,launcher,workspace,listener) ? PollingResult.SIGNIFICANT : PollingResult.NO_CHANGES;
+        }
+    }
 
     /**
      * Obtains a fresh workspace of the module(s) into the specified directory

core/src/main/java/hudson/scm/SCMRevisionState.java

+package hudson.scm;
+
+import hudson.model.AbstractProject;
+import hudson.model.InvisibleAction;
+import hudson.model.TaskListener;
+import hudson.model.AbstractBuild;
+import hudson.Launcher;
+import hudson.FilePath;
+
+/**
+ * Immutable object that represents revisions of the files in the repository,
+ * used to represent the result of
+ * {@linkplain SCM#poll(AbstractProject, Launcher, FilePath, TaskListener, SCMRevisionState) a SCM polling}.
+ *
+ * <p>
+ * This object is used so that the successive polling can compare the tip of the repository now vs
+ * what it was when it was last polled. (Before 1.345, Hudson was only able to compare the tip
+ * of the repository vs the state of the workspace, which resulted in a problem like HUDSON-2180.
+ *
+ * <p>
+ * {@link SCMRevisionState} is persisted as an action to {@link AbstractBuild}.
+ *
+ * @author Kohsuke Kawaguchi
+ * @since 1.345
+ */
+public abstract class SCMRevisionState extends InvisibleAction {
+    /*
+      I can't really make this comparable because comparing two revision states often requires
+      non-trivial computation and conversations with the repository (mainly to figure out
+      which changes are insignificant and which are not.)
+
+      So instead, here we opt to a design where we tell SCM upfront about what we are comparing
+      against (baseline), and have it give us the new state and degree of change in PollingResult.
+     */
+
+    public static SCMRevisionState NONE = new None();
+
+    private static final class None extends SCMRevisionState {}
+}

test/src/test/groovy/hudson/cli/BuildCommandTest.groovy

@@ -54,7 +54,7 @@ public class BuildCommandTest extends HudsonTestCase {
         }] as TestBuilder);
 
         // this should be asynchronous
-        new CLI(getURL()).execute(["build", p.name])
+        assertEquals(0,new CLI(getURL()).execute(["build", p.name]))
         started.block();
         assertTrue(p.getBuildByNumber(1).isBuilding())
         completed.signal();