diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a7d445ea5..f23524c666 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/api/GitHookTagProvider.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/api/GitHookTagProvider.java index 7889badc30..9c01bb40c5 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/api/GitHookTagProvider.java +++ b/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()))); - deletedTagBuilder.add(createTagFromOldId(rc, tag, GitUtil.getTagTime(repository, rc.getOldId()))); + createdTagBuilder.add(createTagFromNewId(revWalk, rc, tag)); + 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) { - return new Tag(tag, GitUtil.getId(rc.getNewId()), tagTime); + private Tag createTagFromNewId(RevWalk revWalk, ReceiveCommand rc, String tag) throws IOException { + 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) { - return new Tag(tag, GitUtil.getId(rc.getOldId()), tagTime); + private Tag createTagFromOldId(RevWalk revWalk, ReceiveCommand rc, String tag) throws IOException { + 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); + } } diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/api/GitHookTagProviderTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/api/GitHookTagProviderTest.java index ccf21b83c9..524998ee39 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/api/GitHookTagProviderTest.java +++ b/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 commands; @@ -79,12 +88,12 @@ public class GitHookTagProviderTest { @Test public void testGetCreatedTags() { try (MockedStatic 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 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 dummy = Mockito.mockStatic(GitUtil.class)) { - String oldRevision = "e0f2be968b147ff7043684a7715d2fe852553db4"; - String newRevision = "b2002b64013e54b78eac251df0672bd5d6a83aa7"; + String oldRevision = REVISION_2; + 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(repository, ObjectId.fromString(newRevision))).thenReturn(null); + dummy.when(() -> GitUtil.getTagTime(revWalk, ObjectId.fromString(oldRevision))).thenReturn(timestamp); + 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 dummy = Mockito.mockStatic(GitUtil.class)) { - String oldRevision = "e0f2be968b147ff7043684a7715d2fe852553db4"; - String newRevision = "b2002b64013e54b78eac251df0672bd5d6a83aa7"; + String oldRevision = REVISION_2; + 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(repository, ObjectId.fromString(oldRevision))).thenReturn(null); + dummy.when(() -> GitUtil.getTagTime(revWalk, ObjectId.fromString(newRevision))).thenReturn(timestamp); + 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 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 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; + } + }; } }