Merge branch 'stable-5.1'

* stable-5.1:
  Fix DescribeCommand with multiple match options
  Fix git-describe tie-breakers

Change-Id: I64d9b0099fc0d8449ec08f0b5653572b5c2f840f
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/api/DescribeCommandTest.java

@@ -45,6 +45,7 @@ package org.eclipse.jgit.api;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
 import java.io.BufferedWriter;
@@ -157,6 +158,11 @@ public class DescribeCommandTest extends RepositoryTestCase {
 	public void testDescribeMultiMatch() throws Exception {
 		ObjectId c1 = modify("aaa");
 		tag("v1.0.0");
+		tick();
+		tag("v1.0.1");
+		tick();
+		tag("v1.1.0");
+		tick();
 		tag("v1.1.1");
 		ObjectId c2 = modify("bbb");
 
@@ -166,22 +172,39 @@ public class DescribeCommandTest extends RepositoryTestCase {
 			return;
 		}
 
-		// 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 interested in any tags, we get the most recent tag
+		// as per Git behaviour since 1.7.1.1
+		if (useAnnotatedTags) {
+			assertEquals("v1.1.1", describe(c1));
+			assertEquals("v1.1.1-1-gb89dead", 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.0.1", 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*"));
+			assertEquals("v1.0.1-1-gb89dead", describe(c2, "v1.0*"));
+			assertEquals("v1.1.1-1-gb89dead", 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.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*"));
+			assertEquals("v1.1.1-1-gb89dead", describe(c2, "v1.0*", "v1.1*"));
+			assertEquals("v1.1.1-1-gb89dead", describe(c2, "v1.1*", "v1.0*"));
+		} else {
+			// no timestamps so no guarantees on which tag is chosen
+			assertNotNull(describe(c1));
+			assertNotNull(describe(c2));
+
+			assertNotNull(describe(c1, "v1.0*"));
+			assertNotNull(describe(c1, "v1.1*"));
+			assertNotNull(describe(c2, "v1.0*"));
+			assertNotNull(describe(c2, "v1.1*"));
+
+			// Ensure that ordering of match precedence is preserved as per Git behaviour
+			assertNotNull(describe(c1, "v1.0*", "v1.1*"));
+			assertNotNull(describe(c1, "v1.1*", "v1.0*"));
+			assertNotNull(describe(c2, "v1.0*", "v1.1*"));
+			assertNotNull(describe(c2, "v1.1*", "v1.0*"));
+
+		}
 	}
 
 	/**

org.eclipse.jgit/src/org/eclipse/jgit/api/DescribeCommand.java

@@ -50,10 +50,12 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
+import java.util.Date;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import org.eclipse.jgit.api.errors.GitAPIException;
 import org.eclipse.jgit.api.errors.JGitInternalException;
@@ -71,6 +73,7 @@ import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevFlag;
 import org.eclipse.jgit.revwalk.RevFlagSet;
+import org.eclipse.jgit.revwalk.RevTag;
 import org.eclipse.jgit.revwalk.RevWalk;
 
 /**
@@ -224,24 +227,40 @@ public class DescribeCommand extends GitCommand<String> {
 		return this;
 	}
 
+	private final Comparator<Ref> TAG_TIE_BREAKER = new Comparator<Ref>() {
+
+		@Override
+		public int compare(Ref o1, Ref o2) {
+			try {
+				return tagDate(o2).compareTo(tagDate(o1));
+			} catch (IOException e) {
+				return 0;
+			}
+		}
+
+		private Date tagDate(Ref tag) throws IOException {
+			RevTag t = w.parseTag(tag.getObjectId());
+			w.parseBody(t);
+			return t.getTaggerIdent().getWhen();
+		}
+	};
+
 	private Optional<Ref> getBestMatch(List<Ref> tags) {
 		if (tags == null || tags.size() == 0) {
 			return Optional.empty();
 		} else if (matchers.size() == 0) {
-			// No matchers, simply return the first tag entry
+			Collections.sort(tags, TAG_TIE_BREAKER);
 			return Optional.of(tags.get(0));
 		} else {
-			// Find the first tag that matches one of the matchers; precedence according to matcher definition order
+			// Find the first tag that matches in the stream of all tags
+			// filtered by matchers ordered by tie break order
+			Stream<Ref> matchingTags = Stream.empty();
 			for (IMatcher matcher : matchers) {
-				Optional<Ref> match = tags.stream()
-						.filter(tag -> matcher.matches(tag.getName(), false,
-								false))
-						.findFirst();
-				if (match.isPresent()) {
-					return match;
+				Stream<Ref> m = tags.stream().filter(
+						tag -> matcher.matches(tag.getName(), false, false));
+				matchingTags = Stream.of(matchingTags, m).flatMap(i -> i);
 			}
-			}
-			return Optional.empty();
+			return matchingTags.sorted(TAG_TIE_BREAKER).findFirst();
 		}
 	}