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 <oliver.lockwood@cantab.net>

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 ff9ae596c9d29feca4b1bdbe69df956ee9404dd0..6a667830e80a1ee359bd51b69110355358400f8d 100644 (file)
--- 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 1353bf11a4099e3c7654dabf38bb956a713bd9c8..d365171ed1cdd31b4678aa5c5972756fe944715a 100644 (file)
--- 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<String> {
                return this;
        }
 
-       private boolean tagMatches(Ref tag) {
-               if (tag == null) {
-                       return false;
+       private Optional<Ref> getBestMatch(List<Ref> 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<Ref> 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<String> {
                        if (target == null)
                                setTarget(Constants.HEAD);
 
-                       Map<ObjectId, Ref> 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<Ref> tagList = repo.getRefDatabase().getRefs(R_TAGS).values();
+                       Map<ObjectId, List<Ref>> 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<String> {
                        }
                        List<Candidate> 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<Ref> 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<String> {
                                        // 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;
                                        }