[Fix JENKINS-23299] tag based builds run at every poll

When a build is defined to use a tag, the "check for changes" uses ls-remote to compare the SHA1 of the remote tag to the SHA1 of the HEAD of the working directory. The tag SHA1 is not always the same as the commit SHA1 (annotated tags), so when that happens, the ls-remote based "check for changes" can never be satisfied. This change adapts the tag specific check for changes to use a syntax which returns the SHA1 of the commit referenced by the tag rather than the SHA1 of the tag. JGit always returns the SHA1 of the commit referenced by a tag (a "peeled" object), while CliGit returns a "peeled" object for tags only if the refs/tags/tag_name syntax is used. That difference is maintained for behavioral compatibility for users of CliGit. Cases where returning the SHA1 of the tag would be better than returning the SHA1 of the commit referenced by the tag seem rare, but better to retain CliGit compatibility than risk surprising users in some as yet undetected use case.
jenkinsci · Sep 12, 2014 · 4362cf6 · 4362cf6
1 parent 96a49dd
commit 4362cf6
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 1 deletion.
diff --git a/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java b/src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java
@@ -2051,7 +2051,11 @@ public ObjectId getHeadRev(String url, String branchSpec) throws GitException, I
         if (cred == null) cred = defaultCredentials;
 
         args.add(url);
-        args.add(branchName);
+        if (branchName.startsWith("refs/tags/")) {
+            args.add(branchName+"^{}"); // JENKINS-23299 - tag SHA1 needs to be converted to commit SHA1
+        } else {
+            args.add(branchName);
+        }
         String result = launchCommandWithCredentials(args, null, cred, url);
         return result.length()>=40 ? ObjectId.fromString(result.substring(0, 40)) : null;
     }

diff --git a/src/test/java/org/jenkinsci/plugins/gitclient/GitAPITestCase.java b/src/test/java/org/jenkinsci/plugins/gitclient/GitAPITestCase.java
@@ -24,6 +24,7 @@
 import hudson.remoting.VirtualChannel;
 import hudson.util.IOUtils;
 import junit.framework.TestCase;
+import static org.junit.Assert.assertNotEquals;
 
 import org.apache.commons.io.FileUtils;
 import org.eclipse.jgit.internal.storage.file.FileRepository;
@@ -1187,13 +1188,43 @@ public void test_delete_branch() throws Exception {
         }
     }
 
+    @Bug(23299)
     public void test_create_tag() throws Exception {
         w.init();
+        String gitDir = w.repoPath() + File.separator + ".git";
         w.commitEmpty("init");
+        ObjectId init = w.git.revParse("HEAD"); // Remember SHA1 of init commit
         w.git.tag("test", "this is a tag");
+
+        /* JGit seems to have the better behavior in this case, always
+         * returning the SHA1 of the commit. Most users are using
+         * command line git, so the difference is retained in command
+         * line git for compatibility with any legacy command line git
+         * use cases which depend on returning the SHA-1 of the
+         * annotated tag rather than the SHA-1 of the commit to which
+         * the annotated tag points.
+         */
+        ObjectId testTag = w.git.getHeadRev(gitDir, "test"); // Remember SHA1 of annotated test tag
+        if (w.git instanceof JGitAPIImpl) {
+            assertEquals("Annotated tag does not match SHA1", init, testTag);
+        } else {
+            assertNotEquals("Annotated tag unexpectedly equals SHA1", init, testTag);
+        }
+
+        /* Because refs/tags/test syntax is more specific than "test",
+         * and because the more specific syntax was only introduced in
+         * more recent git client plugin versions (like 1.10.0 and
+         * later), the CliGit and JGit behavior are kept the same here
+         * in order to fix JENKINS-23299.
+         */
+        ObjectId testTagCommit = w.git.getHeadRev(gitDir, "refs/tags/test"); // SHA1 of commit identified by test tag
+        assertEquals("Annotated tag doesn't match queried commit SHA1", init, testTagCommit);
+        assertEquals(init, w.git.revParse("test")); // SHA1 of commit identified by test tag
+        assertEquals(init, w.git.revParse("refs/tags/test")); // SHA1 of commit identified by test tag
         assertTrue("test tag not created", w.cmd("git tag").contains("test"));
         String message = w.cmd("git tag -l -n1");
         assertTrue("unexpected test tag message : " + message, message.contains("this is a tag"));
+        assertNull(w.git.getHeadRev(gitDir, "not-a-valid-tag")); // Confirm invalid tag returns null
     }
 
     public void test_delete_tag() throws Exception {
@@ -2088,10 +2119,16 @@ public void test_changelog_abort() throws InterruptedException, IOException
         assertTrue("No SHA1 in " + writer.toString(), writer.toString().contains(sha1));
     }
 
+    @Bug(23299)
     public void test_getHeadRev() throws Exception {
         Map<String, ObjectId> heads = w.git.getHeadRev(remoteMirrorURL);
         ObjectId master = w.git.getHeadRev(remoteMirrorURL, "refs/heads/master");
         assertEquals("URL is " + remoteMirrorURL + ", heads is " + heads, master, heads.get("refs/heads/master"));
+
+        /* Test with a specific tag reference - JENKINS-23299 */
+        ObjectId knownTag = w.git.getHeadRev(remoteMirrorURL, "refs/tags/git-client-1.10.0");
+        ObjectId expectedTag = ObjectId.fromString("1fb23708d6b639c22383c8073d6e75051b2a63aa"); // commit SHA1
+        assertEquals("Wrong SHA1 for git-client-1.10.0 tag", expectedTag, knownTag);
     }
 
     /**