Determine correct revision for tags in hooks

The former code only resolved the object id of the tags, when those were
annotated tags (for "simple" tags the code worked fine). Now we even can
handle nested tags (tags that reference other tags).

CHANGELOG.md

@@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Set default branch in branch selector if nothing is selected ([#1338](https://github.com/scm-manager/scm-manager/pull/1338))
 - Handling of branch with slashes in source view ([#1340](https://github.com/scm-manager/scm-manager/pull/1340))
 - Detect not existing paths correctly in Mercurial ([#1343](https://github.com/scm-manager/scm-manager/pull/1343))
+- Return correct revisions for tags in hooks for git repositories ([#1344](https://github.com/scm-manager/scm-manager/pull/1344))
 
 ## [2.5.0] - 2020-09-10
 ### Added

scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/api/GitHookTagProvider.java

@@ -24,9 +24,14 @@
 
 package sonia.scm.repository.api;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevObject;
+import org.eclipse.jgit.revwalk.RevTag;
+import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.ReceiveCommand;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -36,6 +41,8 @@ import sonia.scm.repository.Tag;
 import java.io.IOException;
 import java.util.List;
 
+import static sonia.scm.repository.GitUtil.getId;
+
 /**
  * Git provider implementation of {@link HookTagProvider}.
  *
@@ -65,14 +72,14 @@ public class GitHookTagProvider implements HookTagProvider {
       if (Strings.isNullOrEmpty(tag)) {
         LOG.debug("received ref name {} is not a tag", refName);
       } else {
-        try {
+        try (RevWalk revWalk = createRevWalk(repository)) {
           if (isCreate(rc)) {
-            createdTagBuilder.add(createTagFromNewId(rc, tag, GitUtil.getTagTime(repository, rc.getNewId())));
+            createdTagBuilder.add(createTagFromNewId(revWalk, rc, tag));
           } else if (isDelete(rc)) {
-            deletedTagBuilder.add(createTagFromOldId(rc, tag, GitUtil.getTagTime(repository, rc.getOldId())));
+            deletedTagBuilder.add(createTagFromOldId(revWalk, rc, tag));
           } else if (isUpdate(rc)) {
-            createdTagBuilder.add(createTagFromNewId(rc, tag, GitUtil.getTagTime(repository, rc.getNewId())));
+            createdTagBuilder.add(createTagFromNewId(revWalk, rc, tag));
-            deletedTagBuilder.add(createTagFromOldId(rc, tag, GitUtil.getTagTime(repository, rc.getOldId())));
+            deletedTagBuilder.add(createTagFromOldId(revWalk, rc, tag));
           }
         } catch (IOException e) {
           LOG.error("Could not read tag time", e);
@@ -84,12 +91,25 @@ public class GitHookTagProvider implements HookTagProvider {
     deletedTags = deletedTagBuilder.build();
   }
 
-  private Tag createTagFromNewId(ReceiveCommand rc, String tag, Long tagTime) {
+  private Tag createTagFromNewId(RevWalk revWalk, ReceiveCommand rc, String tag) throws IOException {
-    return new Tag(tag, GitUtil.getId(rc.getNewId()), tagTime);
+    final ObjectId newId = rc.getNewId();
+    return new Tag(tag, getId(unpeelTag(revWalk, newId)), GitUtil.getTagTime(revWalk, newId));
   }
 
-  private Tag createTagFromOldId(ReceiveCommand rc, String tag, Long tagTime) {
+  private Tag createTagFromOldId(RevWalk revWalk, ReceiveCommand rc, String tag) throws IOException {
-    return new Tag(tag, GitUtil.getId(rc.getOldId()), tagTime);
+    final ObjectId oldId = rc.getOldId();
+    return new Tag(tag, getId(unpeelTag(revWalk, oldId)), GitUtil.getTagTime(revWalk, oldId));
+  }
+
+  public ObjectId unpeelTag(RevWalk revWalk, ObjectId oldId) throws IOException {
+    final RevObject revObject = revWalk.parseAny(oldId);
+    if (revObject instanceof RevTag) {
+      return unpeelTag(revWalk, ((RevTag) revObject).getObject());
+    } else if (revObject == null) {
+      return oldId;
+    } else {
+      return revObject;
+    }
   }
 
   private boolean isUpdate(ReceiveCommand rc) {
@@ -114,4 +134,8 @@ public class GitHookTagProvider implements HookTagProvider {
     return deletedTags;
   }
 
+  @VisibleForTesting
+  RevWalk createRevWalk(Repository repository) {
+    return new RevWalk(repository);
+  }
 }

scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/api/GitHookTagProviderTest.java

@@ -27,6 +27,9 @@ package sonia.scm.repository.api;
 import com.google.common.collect.Lists;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevObject;
+import org.eclipse.jgit.revwalk.RevTag;
+import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.ReceiveCommand;
 import org.junit.Before;
 import org.junit.Test;
@@ -38,6 +41,7 @@ import org.mockito.junit.MockitoJUnitRunner;
 import sonia.scm.repository.GitUtil;
 import sonia.scm.repository.Tag;
 
+import java.io.IOException;
 import java.util.List;
 
 import static org.hamcrest.Matchers.empty;
@@ -45,6 +49,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThat;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 /**
@@ -56,12 +61,16 @@ import static org.mockito.Mockito.when;
 public class GitHookTagProviderTest {
 
   private static final String ZERO = ObjectId.zeroId().getName();
+  public static final String REVISION_1 = "b2002b64013e54b78eac251df0672bd5d6a83aa7";
+  public static final String REVISION_2 = "e0f2be968b147ff7043684a7715d2fe852553db4";
 
   @Mock
   private ReceiveCommand command;
 
   @Mock
   private Repository repository;
+  @Mock
+  private RevWalk revWalk;
 
   private List<ReceiveCommand> commands;
 
@@ -79,12 +88,12 @@ public class GitHookTagProviderTest {
   @Test
   public void testGetCreatedTags() {
     try (MockedStatic<GitUtil> dummy = Mockito.mockStatic(GitUtil.class)) {
-      String revision = "86a6645eceefe8b9a247db5eb16e3d89a7e6e6d1";
+      String revision = REVISION_1;
       Long timestamp = 1339416344000L;
       String tagName = "1.0.0";
       String ref = "refs/tags/" + tagName;
 
-      dummy.when(() -> GitUtil.getTagTime(repository, ObjectId.fromString(revision))).thenReturn(timestamp);
+      dummy.when(() -> GitUtil.getTagTime(revWalk, ObjectId.fromString(revision))).thenReturn(timestamp);
       dummy.when(() -> GitUtil.getTagName(ref)).thenReturn(tagName);
       dummy.when(() -> GitUtil.getId(ObjectId.fromString(revision))).thenReturn(revision);
 
@@ -101,12 +110,12 @@ public class GitHookTagProviderTest {
   @Test
   public void testGetDeletedTags() {
     try (MockedStatic<GitUtil> dummy = Mockito.mockStatic(GitUtil.class)) {
-      String revision = "b2002b64013e54b78eac251df0672bd5d6a83aa7";
+      String revision = REVISION_1;
       Long timestamp = 1339416344000L;
       String tagName = "1.0.0";
       String ref = "refs/tags/" + tagName;
 
-      dummy.when(() -> GitUtil.getTagTime(repository, ObjectId.fromString(revision))).thenReturn(timestamp);
+      dummy.when(() -> GitUtil.getTagTime(revWalk, ObjectId.fromString(revision))).thenReturn(timestamp);
       dummy.when(() -> GitUtil.getTagName(ref)).thenReturn(tagName);
       dummy.when(() -> GitUtil.getId(ObjectId.fromString(revision))).thenReturn(revision);
 
@@ -122,7 +131,7 @@ public class GitHookTagProviderTest {
    */
   @Test
   public void testWithBranch() {
-    String revision = "b2002b64013e54b78eac251df0672bd5d6a83aa7";
+    String revision = REVISION_1;
     GitHookTagProvider provider = createProvider(ReceiveCommand.Type.CREATE, "refs/heads/1.0.0", revision, revision);
 
     assertThat(provider.getCreatedTags(), empty());
@@ -135,15 +144,15 @@ public class GitHookTagProviderTest {
   @Test
   public void testUpdateTagsPreReceive() {
     try (MockedStatic<GitUtil> dummy = Mockito.mockStatic(GitUtil.class)) {
-      String oldRevision = "e0f2be968b147ff7043684a7715d2fe852553db4";
+      String oldRevision = REVISION_2;
-      String newRevision = "b2002b64013e54b78eac251df0672bd5d6a83aa7";
+      String newRevision = REVISION_1;
 
       Long timestamp = 1339416344000L;
       String tagName = "1.0.0";
       String ref = "refs/tags/" + tagName;
 
-      dummy.when(() -> GitUtil.getTagTime(repository, ObjectId.fromString(oldRevision))).thenReturn(timestamp);
+      dummy.when(() -> GitUtil.getTagTime(revWalk, ObjectId.fromString(oldRevision))).thenReturn(timestamp);
-      dummy.when(() -> GitUtil.getTagTime(repository, ObjectId.fromString(newRevision))).thenReturn(null);
+      dummy.when(() -> GitUtil.getTagTime(revWalk, ObjectId.fromString(newRevision))).thenReturn(null);
       dummy.when(() -> GitUtil.getTagName(ref)).thenReturn(tagName);
       dummy.when(() -> GitUtil.getId(ObjectId.fromString(oldRevision))).thenReturn(oldRevision);
       dummy.when(() -> GitUtil.getId(ObjectId.fromString(newRevision))).thenReturn(newRevision);
@@ -161,15 +170,15 @@ public class GitHookTagProviderTest {
   @Test
   public void testUpdateTagsPostReceive() {
     try (MockedStatic<GitUtil> dummy = Mockito.mockStatic(GitUtil.class)) {
-      String oldRevision = "e0f2be968b147ff7043684a7715d2fe852553db4";
+      String oldRevision = REVISION_2;
-      String newRevision = "b2002b64013e54b78eac251df0672bd5d6a83aa7";
+      String newRevision = REVISION_1;
 
       Long timestamp = 1339416344000L;
       String tagName = "1.0.0";
       String ref = "refs/tags/" + tagName;
 
-      dummy.when(() -> GitUtil.getTagTime(repository, ObjectId.fromString(newRevision))).thenReturn(timestamp);
+      dummy.when(() -> GitUtil.getTagTime(revWalk, ObjectId.fromString(newRevision))).thenReturn(timestamp);
-      dummy.when(() -> GitUtil.getTagTime(repository, ObjectId.fromString(oldRevision))).thenReturn(null);
+      dummy.when(() -> GitUtil.getTagTime(revWalk, ObjectId.fromString(oldRevision))).thenReturn(null);
       dummy.when(() -> GitUtil.getTagName(ref)).thenReturn(tagName);
       dummy.when(() -> GitUtil.getId(ObjectId.fromString(oldRevision))).thenReturn(oldRevision);
       dummy.when(() -> GitUtil.getId(ObjectId.fromString(newRevision))).thenReturn(newRevision);
@@ -181,6 +190,34 @@ public class GitHookTagProviderTest {
     }
   }
 
+  @Test
+  public void shouldUnpeelAnnotatedTag() throws IOException {
+    try (MockedStatic<GitUtil> dummy = Mockito.mockStatic(GitUtil.class)) {
+      String revisionOfTag = REVISION_1;
+      String revisionOfCommit = REVISION_2;
+      Long timestampOfTag = 6666666000L;
+      Long timestampOfCommit = 1339416344000L;
+      String tagName = "1.0.0";
+      String ref = "refs/tags/" + tagName;
+
+      final RevTag mockedTag = mock(RevTag.class);
+      when(revWalk.parseAny(ObjectId.fromString(REVISION_1))).thenReturn(mockedTag);
+      final RevObject commitForTag = mock(RevObject.class);
+      when(mockedTag.getObject()).thenReturn(commitForTag);
+      dummy.when(() -> GitUtil.getTagTime(revWalk, ObjectId.fromString(REVISION_1))).thenReturn(timestampOfTag);
+      dummy.when(() -> GitUtil.getTagTime(revWalk, commitForTag)).thenReturn(timestampOfCommit);
+
+      dummy.when(() -> GitUtil.getTagTime(revWalk, ObjectId.fromString(revisionOfTag))).thenReturn(timestampOfCommit);
+      dummy.when(() -> GitUtil.getTagName(ref)).thenReturn(tagName);
+      dummy.when(() -> GitUtil.getId(commitForTag)).thenReturn(revisionOfCommit);
+
+      GitHookTagProvider provider = createProvider(ReceiveCommand.Type.CREATE, ref, revisionOfTag, ZERO);
+
+      assertTag(tagName, revisionOfCommit, timestampOfCommit, provider.getCreatedTags());
+      assertThat(provider.getDeletedTags(), empty());
+    }
+  }
+
   private void assertTag(String name, String revision, Long date, List<Tag> tags) {
     assertNotNull(tags);
     assertFalse(tags.isEmpty());
@@ -196,7 +233,12 @@ public class GitHookTagProviderTest {
     when(command.getOldId()).thenReturn(ObjectId.fromString(oldId));
     when(command.getType()).thenReturn(type);
     when(command.getRefName()).thenReturn(ref);
-    return new GitHookTagProvider(commands, repository);
+    return new GitHookTagProvider(commands, repository) {
+      @Override
+      RevWalk createRevWalk(Repository repository) {
+        return revWalk;
+      }
+    };
   }
 
 }