From 060f3699d473a977b86b2ad9d653df4c83e8b681 Mon Sep 17 00:00:00 2001 From: Oliver Lockwood Date: Wed, 21 Jun 2017 17:25:19 +0100 Subject: [PATCH] Fix bug in multiple tag handling on DescribeCommand In the case of multiple tags on the same commit, jgit previously only ever looked at the last of those tags; git behaviour is to return the first tag (or first matching one if --match is specified). Bug: 518377 Change-Id: I3b6b58ad9f8aa3879ae35b84542b7bddc74a27d6 Signed-off-by: Oliver Lockwood --- .../eclipse/jgit/api/DescribeCommandTest.java | 24 ++++++++ .../org/eclipse/jgit/api/DescribeCommand.java | 60 ++++++++++++------- 2 files changed, 61 insertions(+), 23 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/DescribeCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/DescribeCommandTest.java index ff9ae596c9..6a667830e8 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/DescribeCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/DescribeCommandTest.java @@ -130,6 +130,30 @@ public class DescribeCommandTest extends RepositoryTestCase { assertEquals("bob-t2-1-g3e563c5", git.describe().call()); } + @Test + public void testDescribeMultiMatch() throws Exception { + ObjectId c1 = modify("aaa"); + tag("v1.0.0"); + tag("v1.1.1"); + ObjectId c2 = modify("bbb"); + + // Ensure that if we're interested in any tags, we get the first match as per Git behaviour + assertEquals("v1.0.0", describe(c1)); + assertEquals("v1.0.0-1-g3747db3", describe(c2)); + + // Ensure that if we're only interested in one of multiple tags, we get the right match + assertEquals("v1.0.0", describe(c1, "v1.0*")); + assertEquals("v1.1.1", describe(c1, "v1.1*")); + assertEquals("v1.0.0-1-g3747db3", describe(c2, "v1.0*")); + assertEquals("v1.1.1-1-g3747db3", describe(c2, "v1.1*")); + + // Ensure that ordering of match precedence is preserved as per Git behaviour + assertEquals("v1.0.0", describe(c1, "v1.0*", "v1.1*")); + assertEquals("v1.1.1", describe(c1, "v1.1*", "v1.0*")); + assertEquals("v1.0.0-1-g3747db3", describe(c2, "v1.0*", "v1.1*")); + assertEquals("v1.1.1-1-g3747db3", describe(c2, "v1.1*", "v1.0*")); + } + /** * Make sure it finds a tag when not all ancestries include a tag. * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/DescribeCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/DescribeCommand.java index 1353bf11a4..d365171ed1 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/DescribeCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/DescribeCommand.java @@ -47,11 +47,13 @@ import static org.eclipse.jgit.lib.Constants.R_TAGS; import java.io.IOException; import java.text.MessageFormat; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; -import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; +import java.util.stream.Collectors; import org.eclipse.jgit.api.errors.GitAPIException; import org.eclipse.jgit.api.errors.JGitInternalException; @@ -197,17 +199,34 @@ public class DescribeCommand extends GitCommand { return this; } - private boolean tagMatches(Ref tag) { - if (tag == null) { - return false; + private Optional getBestMatch(List tags) { + if (tags == null || tags.size() == 0) { + return Optional.empty(); } else if (matchers.size() == 0) { - return true; + // No matchers, simply return the first tag entry + return Optional.of(tags.get(0)); } else { - return matchers.stream() - .anyMatch(m -> m.matches(tag.getName(), false)); + // Find the first tag that matches one of the matchers; precedence according to matcher definition order + for (IMatcher matcher : matchers) { + Optional match = tags.stream() + .filter(tag -> matcher.matches(tag.getName(), false)) + .findFirst(); + if (match.isPresent()) { + return match; + } + } + return Optional.empty(); } } + private ObjectId getObjectIdFromRef(Ref r) { + ObjectId key = repo.peel(r).getPeeledObjectId(); + if (key == null) { + key = r.getObjectId(); + } + return key; + } + /** * Describes the specified commit. Target defaults to HEAD if no commit was * set explicitly. @@ -228,14 +247,9 @@ public class DescribeCommand extends GitCommand { if (target == null) setTarget(Constants.HEAD); - Map tags = new HashMap<>(); - - for (Ref r : repo.getRefDatabase().getRefs(R_TAGS).values()) { - ObjectId key = repo.peel(r).getPeeledObjectId(); - if (key == null) - key = r.getObjectId(); - tags.put(key, r); - } + Collection tagList = repo.getRefDatabase().getRefs(R_TAGS).values(); + Map> tags = tagList.stream() + .collect(Collectors.groupingBy(this::getObjectIdFromRef)); // combined flags of all the candidate instances final RevFlagSet allFlags = new RevFlagSet(); @@ -281,11 +295,11 @@ public class DescribeCommand extends GitCommand { } List candidates = new ArrayList<>(); // all the candidates we find - // is the target already pointing to a tag? if so, we are done! - Ref tagOnTarget = tags.get(target); - if (tagMatches(tagOnTarget)) { - return longDesc ? longDescription(tagOnTarget, 0, target) : tagOnTarget - .getName().substring(R_TAGS.length()); + // is the target already pointing to a suitable tag? if so, we are done! + Optional bestMatch = getBestMatch(tags.get(target)); + if (bestMatch.isPresent()) { + return longDesc ? longDescription(bestMatch.get(), 0, target) : + bestMatch.get().getName().substring(R_TAGS.length()); } w.markStart(target); @@ -297,9 +311,9 @@ public class DescribeCommand extends GitCommand { // if a tag already dominates this commit, // then there's no point in picking a tag on this commit // since the one that dominates it is always more preferable - Ref t = tags.get(c); - if (tagMatches(t)) { - Candidate cd = new Candidate(c, t); + bestMatch = getBestMatch(tags.get(c)); + if (bestMatch.isPresent()) { + Candidate cd = new Candidate(c, bestMatch.get()); candidates.add(cd); cd.depth = seen; } -- 2.39.5