From f9de9175477577c6c7d0a22ebe5a1c9a1c299c1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5vard=20Wall?= Date: Wed, 17 Oct 2018 15:34:51 +0200 Subject: [PATCH 1/2] Fix git-describe tie-breakers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correct behaviour as git 1.7.1.1 is to resolve tie-breakers to choose the most recent tag. https://github.com/git/git/blob/master/Documentation/RelNotes/1.7.1.1.txt: * "git describe" did not tie-break tags that point at the same commit correctly; newer ones are preferred by paying attention to the tagger date now. Bug: 538610 Change-Id: Ib0b2a301997bb7f75935baf7005473f4de952a64 Signed-off-by: Håvard Wall --- .../eclipse/jgit/api/DescribeCommandTest.java | 51 ++++++++++++++----- .../org/eclipse/jgit/api/DescribeCommand.java | 23 ++++++++- 2 files changed, 60 insertions(+), 14 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 79da2da7e..a2d8e8969 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 @@ -44,6 +44,7 @@ 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.File; @@ -134,24 +135,48 @@ public void testDescribe() throws Exception { 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"); - // 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.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 if we're only interested in one of multiple tags, we get the right match + assertEquals("v1.0.1", describe(c1, "v1.0*")); + assertEquals("v1.1.1", describe(c1, "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.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*")); + // Ensure that ordering of match precedence is preserved as per Git behaviour + assertEquals("v1.0.1", describe(c1, "v1.0*", "v1.1*")); + assertEquals("v1.1.1", describe(c1, "v1.1*", "v1.0*")); + assertEquals("v1.0.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*")); + + } } /** 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 01fe4aa9e..18e8a7f4f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/DescribeCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/DescribeCommand.java @@ -50,6 +50,7 @@ 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; @@ -71,6 +72,7 @@ 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; /** @@ -203,11 +205,29 @@ public DescribeCommand setMatch(String... patterns) throws InvalidPatternExcepti return this; } + private final Comparator TAG_TIE_BREAKER = new Comparator() { + + @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 getBestMatch(List 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 @@ -215,6 +235,7 @@ private Optional getBestMatch(List tags) { Optional match = tags.stream() .filter(tag -> matcher.matches(tag.getName(), false, false)) + .sorted(TAG_TIE_BREAKER) .findFirst(); if (match.isPresent()) { return match; From df6263644bb3387239cd77aa57074380610a0ec4 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Sun, 25 Nov 2018 23:15:11 +0100 Subject: [PATCH 2/2] Fix DescribeCommand with multiple match options when multiple match options are given in git describe the result must not depend on the order of the match options. JGit wrongly picked the first match using the match options in the order they were defined. Fix this by concatenating the streams of matching tags for all match options and then choosing the first match on the concatenated stream sorted in tie break order. See https://git-scm.com/docs/git-describe#git-describe---matchltpatterngt Change-Id: Id01433d35fa16fb4c30526605bee041ac1d954b2 Signed-off-by: Matthias Sohn --- .../eclipse/jgit/api/DescribeCommandTest.java | 4 ++-- .../org/eclipse/jgit/api/DescribeCommand.java | 18 ++++++++---------- 2 files changed, 10 insertions(+), 12 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 a2d8e8969..be67893cb 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 @@ -156,9 +156,9 @@ public void testDescribeMultiMatch() throws Exception { 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.1", 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.1-1-gb89dead", describe(c2, "v1.0*", "v1.1*")); + 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 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 18e8a7f4f..5bd932d1c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/DescribeCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/DescribeCommand.java @@ -55,6 +55,7 @@ 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; @@ -230,18 +231,15 @@ private Optional getBestMatch(List tags) { 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 matchingTags = Stream.empty(); for (IMatcher matcher : matchers) { - Optional match = tags.stream() - .filter(tag -> matcher.matches(tag.getName(), false, - false)) - .sorted(TAG_TIE_BREAKER) - .findFirst(); - if (match.isPresent()) { - return match; - } + Stream 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(); } }