From cd9da7e6588a554093648f701662d441f2a95ee0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Wed, 23 Sep 2020 14:18:49 +0200 Subject: [PATCH] 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 | 1 + .../repository/api/GitHookTagProvider.java | 42 ++++++++--- .../api/GitHookTagProviderTest.java | 70 +++++++++++++++---- 3 files changed, 90 insertions(+), 23 deletions(-) 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; + } + }; } }