Make JGit describe behaves same as c-git for lightweight tags

JGit now considers lightweight tags only if the --tags option is set
i.e. `git.describe().setAllTags(true)` has to be set, else the default
is now as in c git:

Only annotated tags are evaluated unless you pass true
equivalent to --tags (or --all) by the option setAllTags.

Hint: This (still) doesn't address any difference between c-git
`--all` and `!--all --tags` behavior;
perhaps this might be a follow up request

Bug: 423206
Change-Id: I9a3699756df0b9c6a7c74a7e8887dea0df17c8e7
Signed-off-by: Marcel Trautwein <me+eclipse@childno.de>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

2 files changed, 187 insertions, 38 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 79da2da7ea..a422ef91cb 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
@@ -54,7 +54,6 @@ import java.util.Collection;
 
 import org.eclipse.jgit.api.errors.GitAPIException;
 import org.eclipse.jgit.api.errors.RefNotFoundException;
-import org.eclipse.jgit.errors.InvalidPatternException;
 import org.eclipse.jgit.junit.RepositoryTestCase;
 import org.eclipse.jgit.lib.ObjectId;
 import org.junit.Test;
@@ -68,13 +67,18 @@ public class DescribeCommandTest extends RepositoryTestCase {
 
 	private Git git;
 
-	@Parameter
+	@Parameter(0)
 	public boolean useAnnotatedTags;
 
-	@Parameters
+	@Parameter(1)
+	public boolean describeUseAllTags;
+
+	@Parameters(name = "git tag -a {0}?-a: with git describe {1}?--tags:")
 	public static Collection<Boolean[]> getUseAnnotatedTagsValues() {
-		return Arrays.asList(new Boolean[][] { { Boolean.TRUE },
-				{ Boolean.FALSE } });
+		return Arrays.asList(new Boolean[][] { { Boolean.TRUE, Boolean.FALSE },
+				{ Boolean.FALSE, Boolean.FALSE },
+				{ Boolean.TRUE, Boolean.TRUE },
+				{ Boolean.FALSE, Boolean.TRUE } });
 	}
 
 	@Override
@@ -99,35 +103,52 @@ public class DescribeCommandTest extends RepositoryTestCase {
 		tag("bob-t2");
 
 		ObjectId c4 = modify("ddd");
+		assertNameStartsWith(c4, "3e563c5");
 
 		assertNull(describe(c1));
 		assertNull(describe(c1, true));
 		assertNull(describe(c1, "a*", "b*", "c*"));
-
-		assertEquals("alice-t1", describe(c2));
-		assertEquals("alice-t1", describe(c2, "alice*"));
 		assertNull(describe(c2, "bob*"));
 		assertNull(describe(c2, "?ob*"));
-		assertEquals("alice-t1", describe(c2, "a*", "b*", "c*"));
 
-		assertEquals("bob-t2", describe(c3));
-		assertEquals("bob-t2-0-g44579eb", describe(c3, true));
-		assertEquals("alice-t1-1-g44579eb", describe(c3, "alice*"));
-		assertEquals("alice-t1-1-g44579eb", describe(c3, "a??c?-t*"));
-		assertEquals("bob-t2", describe(c3, "bob*"));
-		assertEquals("bob-t2", describe(c3, "?ob*"));
-		assertEquals("bob-t2", describe(c3, "a*", "b*", "c*"));
-
-		assertNameStartsWith(c4, "3e563c5");
-		// the value verified with git-describe(1)
-		assertEquals("bob-t2-1-g3e563c5", describe(c4));
-		assertEquals("bob-t2-1-g3e563c5", describe(c4, true));
-		assertEquals("alice-t1-2-g3e563c5", describe(c4, "alice*"));
-		assertEquals("bob-t2-1-g3e563c5", describe(c4, "bob*"));
-		assertEquals("bob-t2-1-g3e563c5", describe(c4, "a*", "b*", "c*"));
+		if (useAnnotatedTags || describeUseAllTags) {
+			assertEquals("alice-t1", describe(c2));
+			assertEquals("alice-t1", describe(c2, "alice*"));
+			assertEquals("alice-t1", describe(c2, "a*", "b*", "c*"));
+
+			assertEquals("bob-t2", describe(c3));
+			assertEquals("bob-t2-0-g44579eb", describe(c3, true));
+			assertEquals("alice-t1-1-g44579eb", describe(c3, "alice*"));
+			assertEquals("alice-t1-1-g44579eb", describe(c3, "a??c?-t*"));
+			assertEquals("bob-t2", describe(c3, "bob*"));
+			assertEquals("bob-t2", describe(c3, "?ob*"));
+			assertEquals("bob-t2", describe(c3, "a*", "b*", "c*"));
+
+			// the value verified with git-describe(1)
+			assertEquals("bob-t2-1-g3e563c5", describe(c4));
+			assertEquals("bob-t2-1-g3e563c5", describe(c4, true));
+			assertEquals("alice-t1-2-g3e563c5", describe(c4, "alice*"));
+			assertEquals("bob-t2-1-g3e563c5", describe(c4, "bob*"));
+			assertEquals("bob-t2-1-g3e563c5", describe(c4, "a*", "b*", "c*"));
+		} else {
+			assertEquals(null, describe(c2));
+			assertEquals(null, describe(c3));
+			assertEquals(null, describe(c4));
+		}
 
 		// test default target
-		assertEquals("bob-t2-1-g3e563c5", git.describe().call());
+		if (useAnnotatedTags) {
+			assertEquals("bob-t2-1-g3e563c5", git.describe().call());
+			assertEquals("bob-t2-1-g3e563c5",
+					git.describe().setTags(false).call());
+			assertEquals("bob-t2-1-g3e563c5",
+					git.describe().setTags(true).call());
+		} else {
+			assertEquals(null, git.describe().call());
+			assertEquals(null, git.describe().setTags(false).call());
+			assertEquals("bob-t2-1-g3e563c5",
+					git.describe().setTags(true).call());
+		}
 	}
 
 	@Test
@@ -137,7 +158,14 @@ public class DescribeCommandTest extends RepositoryTestCase {
 		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
+		if (!useAnnotatedTags && !describeUseAllTags) {
+			assertEquals(null, describe(c1));
+			assertEquals(null, describe(c2));
+			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));
 
@@ -179,7 +207,11 @@ public class DescribeCommandTest extends RepositoryTestCase {
 		ObjectId c4 = merge(c2);
 
 		assertNameStartsWith(c4, "119892b");
-		assertEquals("t-2-g119892b", describe(c4)); // 2 commits: c4 and c3
+		if (useAnnotatedTags || describeUseAllTags) {
+			assertEquals("2 commits: c4 and c3", "t-2-g119892b", describe(c4));
+		} else {
+			assertEquals(null, describe(c4));
+		}
 		assertNull(describe(c3));
 		assertNull(describe(c3, true));
 	}
@@ -211,14 +243,76 @@ public class DescribeCommandTest extends RepositoryTestCase {
 		branch("b", c1);
 
 		ObjectId c3 = modify("ccc");
+		assertNameStartsWith(c3, "0244e7f");
 
 		ObjectId c4 = merge(c2);
 
 		assertNameStartsWith(c4, "119892b");
-		assertEquals("t2-2-g119892b", describe(c4)); // 2 commits: c4 and c3
+
+		if (useAnnotatedTags || describeUseAllTags) {
+			assertEquals("t2-2-g119892b", describe(c4)); // 2 commits: c4 and c3
+			assertEquals("t1-1-g0244e7f", describe(c3));
+		} else {
+			assertEquals(null, describe(c4));
+			assertEquals(null, describe(c3));
+		}
+	}
+
+	/**
+	 * When t1 annotated dominates t2 lightweight tag
+	 *
+	 * <pre>
+	 * t1 -+-> t2  -
+	 *     |       |
+	 *     +-> c3 -+-> c4
+	 * </pre>
+	 *
+	 * @throws Exception
+	 */
+	@Test
+	public void t1AnnotatedDominatesT2lightweight() throws Exception {
+		ObjectId c1 = modify("aaa");
+		tag("t1", useAnnotatedTags);
+
+		ObjectId c2 = modify("bbb");
+		tag("t2", false);
+
+		assertNameStartsWith(c2, "3747db3");
+		if (useAnnotatedTags && !describeUseAllTags) {
+			assertEquals(
+					"only annotated tag t1 expected to be used for describe",
+					"t1-1-g3747db3", describe(c2)); // 1 commits: t2 overridden
+													// by t1
+		} else if (!useAnnotatedTags && !describeUseAllTags) {
+			assertEquals("no commits to describe expected", null, describe(c2));
+		} else {
+			assertEquals("lightweight tag t2 expected in describe", "t2",
+					describe(c2));
+		}
+
+		branch("b", c1);
+
+		ObjectId c3 = modify("ccc");
 
 		assertNameStartsWith(c3, "0244e7f");
-		assertEquals("t1-1-g0244e7f", describe(c3));
+		if (useAnnotatedTags || describeUseAllTags) {
+			assertEquals("t1-1-g0244e7f", describe(c3));
+		}
+
+		ObjectId c4 = merge(c2);
+
+		assertNameStartsWith(c4, "119892b");
+		if (describeUseAllTags) {
+			assertEquals(
+					"2 commits for describe commit increment expected since lightweight tag: c4 and c3",
+					"t2-2-g119892b", describe(c4)); // 2 commits: c4 and c3
+		} else if (!useAnnotatedTags && !describeUseAllTags) {
+			assertEquals("no matching commits expected", null, describe(c4));
+		} else {
+			assertEquals(
+					"3 commits for describe commit increment expected since annotated tag: c4 and c3 and c2",
+					"t1-3-g119892b", describe(c4)); //
+		}
 	}
 
 	/**
@@ -246,7 +340,11 @@ public class DescribeCommandTest extends RepositoryTestCase {
 		ObjectId c4 = merge(t1);
 
 		assertNameStartsWith(c4, "bb389a4");
-		assertEquals("t1-3-gbb389a4", describe(c4));
+		if (useAnnotatedTags || describeUseAllTags) {
+			assertEquals("t1-3-gbb389a4", describe(c4));
+		} else {
+			assertEquals(null, describe(c4));
+		}
 	}
 
 	/**
@@ -275,7 +373,11 @@ public class DescribeCommandTest extends RepositoryTestCase {
 		ObjectId c4 = merge(c2);
 
 		assertNameStartsWith(c4, "bb389a4");
-		assertEquals("t2-4-gbb389a4", describe(c4));
+		if (useAnnotatedTags || describeUseAllTags) {
+			assertEquals("t2-4-gbb389a4", describe(c4));
+		} else {
+			assertEquals(null, describe(c4));
+		}
 	}
 
 	private ObjectId merge(ObjectId c2) throws GitAPIException {
@@ -289,10 +391,15 @@ public class DescribeCommandTest extends RepositoryTestCase {
 	}
 
 	private void tag(String tag) throws GitAPIException {
+		tag(tag, this.useAnnotatedTags);
+	}
+
+	private void tag(String tag, boolean annotatedTag) throws GitAPIException {
 		TagCommand tagCommand = git.tag().setName(tag)
-				.setAnnotated(useAnnotatedTags);
-		if (useAnnotatedTags)
+				.setAnnotated(annotatedTag);
+		if (annotatedTag) {
 			tagCommand.setMessage(tag);
+		}
 		tagCommand.call();
 	}
 
@@ -304,15 +411,17 @@ public class DescribeCommandTest extends RepositoryTestCase {
 
 	private String describe(ObjectId c1, boolean longDesc)
 			throws GitAPIException, IOException {
-		return git.describe().setTarget(c1).setLong(longDesc).call();
+		return git.describe().setTarget(c1).setTags(describeUseAllTags)
+				.setLong(longDesc).call();
 	}
 
 	private String describe(ObjectId c1) throws GitAPIException, IOException {
 		return describe(c1, false);
 	}
 
-	private String describe(ObjectId c1, String... patterns) throws GitAPIException, IOException, InvalidPatternException {
-		return git.describe().setTarget(c1).setMatch(patterns).call();
+	private String describe(ObjectId c1, String... patterns) throws Exception {
+		return git.describe().setTarget(c1).setTags(describeUseAllTags)
+				.setMatch(patterns).call();
 	}
 
 	private static void assertNameStartsWith(ObjectId c4, String prefix) {
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 dc605a91e5..4d5e499571 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/api/DescribeCommand.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/DescribeCommand.java
@@ -104,6 +104,11 @@ public class DescribeCommand extends GitCommand<String> {
 	private List<IMatcher> matchers = new ArrayList<>();
 
 	/**
+	 * Whether to use all tags (incl. lightweight) or not
+	 */
+	private boolean useTags = false;
+
+	/**
 	 * Constructor for DescribeCommand.
 	 *
 	 * @param repo
@@ -173,6 +178,22 @@ public class DescribeCommand extends GitCommand<String> {
 		return this;
 	}
 
+	/**
+	 * Instead of using only the annotated tags, use any tag found in refs/tags
+	 * namespace. This option enables matching lightweight (non-annotated) tags
+	 * or not.
+	 *
+	 * @param tags
+	 *            <code>true</code> enables matching lightweight (non-annotated)
+	 *            tags like setting option --tags in c git
+	 * @return {@code this}
+	 * @since 5.0
+	 */
+	public DescribeCommand setTags(boolean tags) {
+		this.useTags = tags;
+		return this;
+	}
+
 	private String longDescription(Ref tag, int depth, ObjectId tip)
 			throws IOException {
 		return String.format(
@@ -246,13 +267,14 @@ public class DescribeCommand extends GitCommand<String> {
 	public String call() throws GitAPIException {
 		try {
 			checkCallable();
-
-			if (target == null)
+			if (target == null) {
 				setTarget(Constants.HEAD);
+			}
 
 			Collection<Ref> tagList = repo.getRefDatabase()
 					.getRefsByPrefix(R_TAGS);
 			Map<ObjectId, List<Ref>> tags = tagList.stream()
+					.filter(this::filterLightweightTags)
 					.collect(Collectors.groupingBy(this::getObjectIdFromRef));
 
 			// combined flags of all the candidate instances
@@ -376,4 +398,22 @@ public class DescribeCommand extends GitCommand<String> {
 			w.close();
 		}
 	}
+
+	/**
+	 * Whether we use lightweight tags or not for describe Candidates
+	 *
+	 * @param ref
+	 *            reference under inspection
+	 * @return true if it should be used for describe or not regarding
+	 *         {@link org.eclipse.jgit.api.DescribeCommand#useTags}
+	 */
+	@SuppressWarnings("null")
+	private boolean filterLightweightTags(Ref ref) {
+		ObjectId id = ref.getObjectId();
+		try {
+			return this.useTags || (id != null && (w.parseTag(id) != null));
+		} catch (IOException e) {
+			return false;
+		}
+	}
 }