Merge branch 'stable-5.0'

* stable-5.0:
  Prepare 5.0.1-SNAPSHOT builds
  JGit v5.0.0.201806131550-r
  JGit v5.0.0.201806131210-r
  Downgrade Apache httpclient to 4.5.2.v20170210-0925
  RefUpdateTest: Refactor to not use deprecated Repository#getAllRefs
  Propagate failure of ssh command to caller of SshSupport
  Make JGit describe behaves same as c-git for lightweight tags
  Fix issues with LFS on GitHub (SSH)

Change-Id: I0471440919adfdbfc72996711d9e0bbd1f3cf477
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

2 files changed, 172 insertions, 53 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.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java
index b2fae316c3..e1adeedc54 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java
@@ -62,6 +62,7 @@ import java.io.IOException;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.Optional;
 
 import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.lib.Constants;
@@ -110,11 +111,21 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase {
 		delete(db, ref, expected, exists, removed);
 	}
 
-	private void delete(Repository repo, final RefUpdate ref, final Result expected,
-			final boolean exists, final boolean removed) throws IOException {
-		assertEquals(exists, repo.getAllRefs().containsKey(ref.getName()));
+	private void delete(Repository repo, final RefUpdate ref,
+			final Result expected, final boolean exists, final boolean removed)
+			throws IOException {
+		assertEquals(exists, getRef(repo, ref.getName()).isPresent());
 		assertEquals(expected, ref.delete());
-		assertEquals(!removed, repo.getAllRefs().containsKey(ref.getName()));
+		assertEquals(!removed, getRef(repo, ref.getName()).isPresent());
+	}
+
+	private Optional<Ref> getRef(Repository repo, String name)
+			throws IOException {
+		return getRef(repo.getRefDatabase().getRefs(), name);
+	}
+
+	private Optional<Ref> getRef(List<Ref> refs, String name) {
+		return refs.stream().filter(r -> r.getName().equals(name)).findAny();
 	}
 
 	@Test
@@ -125,8 +136,7 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase {
 		ru.setNewObjectId(newid);
 		Result update = ru.update();
 		assertEquals(Result.NEW, update);
-		final Ref r = db.getAllRefs().get(newRef);
-		assertNotNull(r);
+		final Ref r = getRef(db, newRef).get();
 		assertEquals(newRef, r.getName());
 		assertNotNull(r.getObjectId());
 		assertNotSame(newid, r.getObjectId());
@@ -378,10 +388,10 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase {
 
 	@Test
 	public void testRefKeySameAsName() {
+		@SuppressWarnings("deprecation")
 		Map<String, Ref> allRefs = db.getAllRefs();
 		for (Entry<String, Ref> e : allRefs.entrySet()) {
 			assertEquals(e.getKey(), e.getValue().getName());
-
 		}
 	}
 
@@ -520,8 +530,8 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase {
 	 */
 	@Test
 	public void testRefsCacheAfterUpdate() throws Exception {
-		// Do not use the defalt repo for this case.
-		Map<String, Ref> allRefs = db.getAllRefs();
+		// Do not use the default repo for this case.
+		List<Ref> allRefs = db.getRefDatabase().getRefs();
 		ObjectId oldValue = db.resolve("HEAD");
 		ObjectId newValue = db.resolve("HEAD^");
 		// first make HEAD refer to loose ref
@@ -537,9 +547,9 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase {
 		update = updateRef.update();
 		assertEquals(Result.FAST_FORWARD, update);
 
-		allRefs = db.getAllRefs();
-		Ref master = allRefs.get("refs/heads/master");
-		Ref head = allRefs.get("HEAD");
+		allRefs = db.getRefDatabase().getRefs();
+		Ref master = getRef(allRefs, "refs/heads/master").get();
+		Ref head = getRef(allRefs, "HEAD").get();
 		assertEquals("refs/heads/master", master.getName());
 		assertEquals("HEAD", head.getName());
 		assertTrue("is symbolic reference", head.isSymbolic());
@@ -557,8 +567,8 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase {
 	 */
 	@Test
 	public void testRefsCacheAfterUpdateLooseOnly() throws Exception {
-		// Do not use the defalt repo for this case.
-		Map<String, Ref> allRefs = db.getAllRefs();
+		// Do not use the default repo for this case.
+		List<Ref> allRefs = db.getRefDatabase().getRefs();
 		ObjectId oldValue = db.resolve("HEAD");
 		writeSymref(Constants.HEAD, "refs/heads/newref");
 		RefUpdate updateRef = db.updateRef(Constants.HEAD);
@@ -567,9 +577,9 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase {
 		Result update = updateRef.update();
 		assertEquals(Result.NEW, update);
 
-		allRefs = db.getAllRefs();
-		Ref head = allRefs.get("HEAD");
-		Ref newref = allRefs.get("refs/heads/newref");
+		allRefs = db.getRefDatabase().getRefs();
+		Ref head = getRef(allRefs, "HEAD").get();
+		Ref newref = getRef(allRefs, "refs/heads/newref").get();
 		assertEquals("refs/heads/newref", newref.getName());
 		assertEquals("HEAD", head.getName());
 		assertTrue("is symbolic reference", head.isSymbolic());