[FIXED JENKINS-28506] - Git fails to trigger from polling for refs/heads/ style branch name

src/main/java/hudson/plugins/git/BranchSpec.java

@@ -60,8 +60,21 @@ public boolean matches(String item) {
         return matches(item, env);
     }
 
-    public boolean matches(String item, EnvVars env) {
-        return getPattern(env).matcher(item).matches();
+    /**
+     * Compare a git branch reference to configured pattern.
+     * <p>
+     * reference uses normalized format `ref/(heads|tags)/xx`
+     * pattern do support
+     * <ul>
+     *     <li>ref/heads/branch</li>
+     *     <li>(remote-name)?/branch</li>
+     *     <li>ref/remotes/branch</li>
+     *     <li>tag</li>
+     *     <li>(commit sha1)</li>
+     * </ul>
+     */
+    public boolean matches(String ref, EnvVars env) {
+        return getPattern(env).matcher(ref).matches();
     }
 
     /**
@@ -110,26 +123,28 @@ private Pattern getPattern(EnvVars env) {
             String regexSubstring = expandedName.substring(1, expandedName.length());
             return Pattern.compile(regexSubstring);
         }
-
-        // if an unqualified branch was given add a "*/" so it will match branches
-        // from remote repositories as the user probably intended
-        String qualifiedName;
-        if (!expandedName.contains("**") && !expandedName.contains("/"))
-            qualifiedName = "*/" + expandedName;
-        else
-            qualifiedName = expandedName;
-
+
         // build a pattern into this builder
         StringBuilder builder = new StringBuilder();
 
         // for legacy reasons (sic) we do support various branch spec format to declare remotes / branches
-        builder.append("(refs/heads/|refs/remotes/|remotes/)?");
+        builder.append("(refs/heads/");
+
+
+        // if an unqualified branch was given, consider all remotes (with various possible syntaxes)
+        // so it will match branches from  any remote repositories as the user probably intended
+        if (!expandedName.contains("**") && !expandedName.contains("/")) {
+            builder.append("|refs/remotes/[^/]+/|remotes/[^/]+/|[^/]+/");
+        } else {
+            builder.append("|refs/remotes/|remotes/");
+        }
+        builder.append(")?");
 
         // was the last token a wildcard?
         boolean foundWildcard = false;
 
         // split the string at the wildcards
-        StringTokenizer tokenizer = new StringTokenizer(qualifiedName, "*", true);
+        StringTokenizer tokenizer = new StringTokenizer(expandedName, "*", true);
         while (tokenizer.hasMoreTokens()) {
             String token = tokenizer.nextToken();
 

src/main/java/hudson/plugins/git/GitSCM.java

@@ -545,6 +545,8 @@ private static Node workspaceToNode(FilePath workspace) { // TODO https://trello
         return j;
     }
 
+    public static final Pattern GIT_REF = Pattern.compile("(refs/[^/]+)/.*");
+
     private PollingResult compareRemoteRevisionWithImpl(Job<?, ?> project, Launcher launcher, FilePath workspace, final TaskListener listener) throws IOException, InterruptedException {
         // Poll for changes. Are there any unbuilt revisions that Hudson ought to build ?
 
@@ -583,22 +585,28 @@ private PollingResult compareRemoteRevisionWithImpl(Job<?, ?> project, Launcher
                     for (BranchSpec branchSpec : getBranches()) {
                         for (Entry<String, ObjectId> entry : heads.entrySet()) {
                             final String head = entry.getKey();
-                            String name;
                             // head is "refs/(heads|tags)/branchName
-                            if (head.startsWith("refs/heads/")) name = remote + "/" + head.substring(11);
-                            else if (head.startsWith("refs/tags/")) name = remote + "/" + head.substring(10);
-                            else name = remote + "/" + head;
 
-                            if (!branchSpec.matches(name, environment)) continue;
+                            // first, check the a canonical git reference is configured
+                            if (!branchSpec.matches(head, environment)) {
+
+                                // convert head `refs/(heads|tags|whatever)/branch` into shortcut notation `remote/branch`
+                                String name = head;
+                                Matcher matcher = GIT_REF.matcher(head);
+                                if (matcher.matches()) name = remote + head.substring(matcher.group(1).length());
+                                else name = remote + "/" + head;
+
+                                if (!branchSpec.matches(name, environment)) continue;
+                            }
 
                             final ObjectId sha1 = entry.getValue();
                             Build built = buildData.getLastBuild(sha1);
                             if (built != null) {
-                                listener.getLogger().println("[poll] Latest remote head revision on " + name + " is: " + sha1.getName() + " - already built by " + built.getBuildNumber());
+                                listener.getLogger().println("[poll] Latest remote head revision on " + head + " is: " + sha1.getName() + " - already built by " + built.getBuildNumber());
                                 continue;
                             }
 
-                            listener.getLogger().println("[poll] Latest remote head revision on " + name + " is: " + sha1.getName());
+                            listener.getLogger().println("[poll] Latest remote head revision on " + head + " is: " + sha1.getName());
                             return BUILD_NOW;
                         }
                     }

src/test/java/hudson/plugins/git/CliGitSCMTriggerRemotePollTest.java

@@ -3,16 +3,14 @@
 import hudson.plugins.git.extensions.GitClientType;
 import hudson.plugins.git.extensions.impl.EnforceGitClient;
 
-public class CliGitSCMTriggerRemotePollTest extends SCMTriggerTest
-{
+/**
+ * Remote polling and local polling behave differently due to bugs in productive
+ * code which probably cannot be fixed without serious compatibility problems.
+ * The isChangeExpected() method adjusts the tests to the difference between
+ * local and remote polling.
+ */
+public class CliGitSCMTriggerRemotePollTest extends SCMTriggerTest {
 
-    /**
-     * Currently some tests still fail due to bugs in productive code.
-     * TODO: Fix bugs and enable tests.
-     */
-    private boolean SKIP_FAILING_TESTS = true;
-
-
     @Override
     protected EnforceGitClient getGitClient()
     {
@@ -24,52 +22,5 @@ protected boolean isDisableRemotePoll()
     {
         return false;
     }
-
-    @Override
-    public void testNamespaces_with_master() throws Exception {
-        if(SKIP_FAILING_TESTS) return; //TODO Fix productive code
-        super.testNamespaces_with_master();
-    }
-
-    @Override
-    public void testNamespaces_with_namespace1Master() throws Exception {
-        //This one works by accident! ls-remote lists this as first entry
-        super.testNamespaces_with_namespace1Master();
-    }
-
-    @Override
-    public void testNamespaces_with_namespace2Master() throws Exception {
-        if(SKIP_FAILING_TESTS) return; //TODO Fix productive code
-        super.testNamespaces_with_namespace2Master();
-    }
-
-    @Override
-    public void testCommitAsBranchSpec() throws Exception {
-        if(SKIP_FAILING_TESTS) return; //TODO Fix productive code
-        super.testCommitAsBranchSpec();
-    }
 
-    @Override
-    public void testTags_with_TagA() throws Exception {
-        if(SKIP_FAILING_TESTS) return; //TODO Fix productive code
-        super.testTags_with_TagA();
-    }
-
-    @Override
-    public void testTags_with_TagBAnnotated() throws Exception {
-        if(SKIP_FAILING_TESTS) return; //TODO Fix productive code
-        super.testTags_with_TagBAnnotated();
-    }
-
-    /* Skip this test because git client 1.10.2 and later include a fix for
-     * JENKINS-23299.  The fix resolves refs/tags/tag_name as the commit to
-     * which tag_name points.  Prior to that change, the ref pointed to the
-     * SHA-1 of the tag, instead of the SHA-1 of the commit to which the tag
-     * points.  Because of that bug fix, the git plugin correctly detects
-     * refs/tags/TagA as needing to build.
-     */
-    @Override
-    public void testTags_with_refsTagsTagA() throws Exception {
-        return;
-    }
 }

src/test/java/hudson/plugins/git/JGitSCMTriggerRemotePollTest.java

@@ -3,16 +3,14 @@
 import hudson.plugins.git.extensions.GitClientType;
 import hudson.plugins.git.extensions.impl.EnforceGitClient;
 
+/**
+ * Remote polling and local polling behave differently due to bugs in productive
+ * code which probably cannot be fixed without serious compatibility problems.
+ * The isChangeExpected() method adjusts the tests to the difference between
+ * local and remote polling.
+ */
 public class JGitSCMTriggerRemotePollTest extends SCMTriggerTest
 {
-
-    /**
-     * Currently some tests still fail due to bugs in productive code.
-     * TODO: Fix bugs and enable tests.
-     */
-    private boolean SKIP_FAILING_TESTS = true;
-
-
     @Override
     protected EnforceGitClient getGitClient()
     {
@@ -25,22 +23,4 @@ protected boolean isDisableRemotePoll()
         return false;
     }
 
-    @Override
-    public void testNamespaces_with_master() throws Exception {
-        if(SKIP_FAILING_TESTS) return; //TODO Fix productive code
-        super.testNamespaces_with_master();
-    }
-
-    @Override
-    public void testNamespaces_with_namespace2Master() throws Exception {
-        if(SKIP_FAILING_TESTS) return; //TODO Fix productive code
-        super.testNamespaces_with_namespace2Master();
-    }
-
-    @Override
-    public void testCommitAsBranchSpec() throws Exception {
-        if(SKIP_FAILING_TESTS) return; //TODO Fix productive code
-        super.testCommitAsBranchSpec();
-    }
-
-}
+}

src/test/java/hudson/plugins/git/SCMTriggerTest.java

@@ -22,8 +22,6 @@
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
-import java.util.concurrent.FutureTask;
-import java.util.concurrent.TimeUnit;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.zip.ZipEntry;
@@ -39,6 +37,7 @@ public abstract class SCMTriggerTest extends AbstractGitTestCase
     private ZipFile namespaceRepoZip;
     private Properties namespaceRepoCommits;
     private ExecutorService singleThreadExecutor;
+    protected boolean expectChanges = false;
 
     @Override
     protected void tearDown() throws Exception
@@ -59,6 +58,7 @@ protected void tearDown() throws Exception
     @Override
     public void setUp() throws Exception {
         super.setUp();
+        expectChanges = false;
         namespaceRepoZip = new ZipFile("src/test/resources/namespaceBranchRepo.zip");
         namespaceRepoCommits = parseLsRemote(new File("src/test/resources/namespaceBranchRepo.ls-remote"));
         tempAllocator = new TemporaryDirectoryAllocator();
@@ -68,7 +68,7 @@ public void setUp() throws Exception {
     protected abstract EnforceGitClient getGitClient();
 
     protected abstract boolean isDisableRemotePoll();
-    
+
     public void testNamespaces_with_refsHeadsMaster() throws Exception {
         check(namespaceRepoZip, namespaceRepoCommits,
             "refs/heads/master",
@@ -124,7 +124,35 @@ public void testNamespaces_with_refsHeadsNamespace2Master() throws Exception {
             namespaceRepoCommits.getProperty("refs/heads/a_tests/b_namespace2/master"),
             "origin/a_tests/b_namespace2/master");
     }
-
+
+    public void testNamespaces_with_namespace3_feature3_sha1() throws Exception {
+        check(namespaceRepoZip, namespaceRepoCommits,
+                namespaceRepoCommits.getProperty("refs/heads/a_tests/b_namespace3/feature3"),
+                namespaceRepoCommits.getProperty("refs/heads/a_tests/b_namespace3/feature3"),
+                "detached");
+    }
+
+    public void testNamespaces_with_namespace3_feature3_branchName() throws Exception {
+        check(namespaceRepoZip, namespaceRepoCommits,
+                "a_tests/b_namespace3/feature3",
+                namespaceRepoCommits.getProperty("refs/heads/a_tests/b_namespace3/feature3"),
+                "origin/a_tests/b_namespace3/feature3");
+    }
+
+    public void testNamespaces_with_refsHeadsNamespace3_feature3_sha1() throws Exception {
+        check(namespaceRepoZip, namespaceRepoCommits,
+                namespaceRepoCommits.getProperty("refs/heads/a_tests/b_namespace3/feature3"),
+                namespaceRepoCommits.getProperty("refs/heads/a_tests/b_namespace3/feature3"),
+                "detached");
+    }
+
+    public void testNamespaces_with_refsHeadsNamespace3_feature3_branchName() throws Exception {
+        check(namespaceRepoZip, namespaceRepoCommits,
+                "refs/heads/a_tests/b_namespace3/feature3",
+                namespaceRepoCommits.getProperty("refs/heads/a_tests/b_namespace3/feature3"),
+                "origin/a_tests/b_namespace3/feature3");
+    }
+
     public void testTags_with_TagA() throws Exception {
         check(namespaceRepoZip, namespaceRepoCommits,
             "TagA",
@@ -153,6 +181,20 @@ public void testTags_with_refsTagsTagBAnnotated() throws Exception {
             "refs/tags/TagBAnnotated");
     }
 
+    public void testCommitAsBranchSpec_feature4_sha1() throws Exception {
+        check(namespaceRepoZip, namespaceRepoCommits,
+                namespaceRepoCommits.getProperty("refs/heads/b_namespace3/feature4"),
+                namespaceRepoCommits.getProperty("refs/heads/b_namespace3/feature4"),
+                "detached");
+    }
+
+    public void testCommitAsBranchSpec_feature4_branchName() throws Exception {
+        check(namespaceRepoZip, namespaceRepoCommits,
+                "refs/heads/b_namespace3/feature4",
+                namespaceRepoCommits.getProperty("refs/heads/b_namespace3/feature4"),
+                "origin/b_namespace3/feature4");
+    }
+
     public void testCommitAsBranchSpec() throws Exception {
         check(namespaceRepoZip, namespaceRepoCommits,
             namespaceRepoCommits.getProperty("refs/heads/b_namespace3/master"), 
@@ -179,7 +221,7 @@ public void check(ZipFile repoZip, Properties commits, String branchSpec,
         assertNotNull("Job has not been triggered", build1);
 
         PollingResult poll = project.poll(listener);
-        assertFalse("Polling found new changes although nothing new", poll.hasChanges());
+        assertEquals("Expected and actual polling results disagree", false, poll.hasChanges());
 
         //Speedup test - avoid waiting 1 minute
         triggerSCMTrigger(project.getTrigger(SCMTrigger.class)).get(20, SECONDS);

src/test/java/hudson/plugins/git/TestBranchSpec.java

@@ -12,7 +12,7 @@ public void testMatch() {
         BranchSpec l = new BranchSpec("master");
         Assert.assertTrue(l.matches("origin/master"));
         Assert.assertFalse(l.matches("origin/something/master"));
-        Assert.assertFalse(l.matches("master"));
+        Assert.assertTrue(l.matches("master"));
         Assert.assertFalse(l.matches("dev"));
 
 
@@ -67,7 +67,7 @@ public void testMatchEnv() {
         BranchSpec l = new BranchSpec("${master}");
         Assert.assertTrue(l.matches("origin/master", env));
         Assert.assertFalse(l.matches("origin/something/master", env));
-        Assert.assertFalse(l.matches("master", env));
+        Assert.assertTrue(l.matches("master", env));
         Assert.assertFalse(l.matches("dev", env));
 
 

src/test/resources/namespaceBranchRepo.ls-remote

@@ -1,7 +1,9 @@
 86e6eeccc0b3a5e1c8034ff51718b8843a755789	HEAD
 1aeaf8635d2e419a6e9587fd1ed1ddcd445845d9	refs/heads/a_tests/b_namespace1/master
 3e73b26f220d8ad3e517858ffbe83b837d7f04c5	refs/heads/a_tests/b_namespace2/master
+73d4779eae7b6aed8191635f5debc2b37ad083d0	refs/heads/a_tests/b_namespace3/feature3
 d940b841b7a5488ab42772f263b107a58eba2621	refs/heads/a_tests/b_namespace3/master
+74ae8c24eb2783794d969e7f1df7260a8aa06d6b	refs/heads/b_namespace3/feature4
 0b97e49ddbf699cf7b6deb31982d9d568d5e30f4	refs/heads/b_namespace3/master
 b00d0c59cf79da494640788cb23453a0315b9c41	refs/heads/branchForTagA
 dde47cb9b577cb6b50597931cdead4255669f322	refs/heads/branchForTagBAnnotated