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>

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.
 	 *

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) {
+	private Optional<Ref> getBestMatch(List<Ref> tags) {
-		if (tag == null) {
+		if (tags == null || tags.size() == 0) {
-			return false;
+			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()
+			// Find the first tag that matches one of the matchers; precedence according to matcher definition order
-					.anyMatch(m -> m.matches(tag.getName(), false));
+			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<>();
+			Collection<Ref> tagList = repo.getRefDatabase().getRefs(R_TAGS).values();
-
+			Map<ObjectId, List<Ref>> tags = tagList.stream()
-			for (Ref r : repo.getRefDatabase().getRefs(R_TAGS).values()) {
+					.collect(Collectors.groupingBy(this::getObjectIdFromRef));
-				ObjectId key = repo.peel(r).getPeeledObjectId();
-				if (key == null)
-					key = r.getObjectId();
-				tags.put(key, r);
-			}
 
 			// 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!
+			// is the target already pointing to a suitable tag? if so, we are done!
-			Ref tagOnTarget = tags.get(target);
+			Optional<Ref> bestMatch = getBestMatch(tags.get(target));
-			if (tagMatches(tagOnTarget)) {
+			if (bestMatch.isPresent()) {
-				return longDesc ? longDescription(tagOnTarget, 0, target) : tagOnTarget
+				return longDesc ? longDescription(bestMatch.get(), 0, target) :
-						.getName().substring(R_TAGS.length());
+						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);
+					bestMatch = getBestMatch(tags.get(c));
-					if (tagMatches(t)) {
+					if (bestMatch.isPresent()) {
-						Candidate cd = new Candidate(c, t);
+						Candidate cd = new Candidate(c, bestMatch.get());
 						candidates.add(cd);
 						cd.depth = seen;
 					}