Merge branch 'master' into next

* master:
  Prepare 5.3.8-SNAPSHOT builds
  JGit v5.3.7.202002110540-r
  Prepare 5.1.14-SNAPSHOT builds
  JGit v5.1.13.202002110435-r
  reftable: don't check deadline on the first try
  reftable: clarify comment
  reftable: clear cache on full compaction
  reftable: remove outdated comment
  reftable: clarify that LogCursor may return a null ReflogEntry
  Use lambdas where possible
  Upgrade maven-pmd-plugin to 3.13.0
  Restore behavior of CloneCommand
  Include org.apache.commons.codec 1.13 in the JGit http.apache.feature
  Update Maven plugins
  AmazonS3: Speed up fetch, try recent packs first

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

org.eclipse.jgit.packaging/org.eclipse.jgit.http.apache.feature/feature.xml

@@ -47,4 +47,11 @@
          version="0.0.0"
          unpack="false"/>
 
+   <plugin
+         id="org.apache.commons.codec"
+         download-size="0"
+         install-size="0"
+         version="0.0.0"
+         unpack="false"/>
+
 </feature>

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

@@ -408,6 +408,7 @@ public class CloneCommandTest extends RepositoryTestCase {
 		Git git2 = command.call();
 		addRepoToClose(git2.getRepository());
 		assertNotNull(git2);
+		assertTrue(git2.getRepository().isBare());
 		assertNotNull(git2.getRepository().resolve("tag-for-blob"));
 		assertNotNull(git2.getRepository().resolve("tag-initial"));
 		assertEquals(git2.getRepository().getFullBranch(), "refs/heads/master");
@@ -448,6 +449,60 @@ public class CloneCommandTest extends RepositoryTestCase {
 				specs.get(0));
 	}
 
+	@Test
+	public void testCloneRepositoryAllBranchesTakesPreference()
+			throws Exception {
+		File directory = createTempDirectory(
+				"testCloneRepositoryAllBranchesTakesPreference");
+		CloneCommand command = Git.cloneRepository();
+		command.setCloneAllBranches(true);
+		command.setBranchesToClone(
+				Collections.singletonList("refs/heads/test"));
+		command.setDirectory(directory);
+		command.setURI(fileUri());
+		Git git2 = command.call();
+		addRepoToClose(git2.getRepository());
+		assertNotNull(git2);
+		assertEquals(git2.getRepository().getFullBranch(), "refs/heads/test");
+		// Expect both remote branches to exist; setCloneAllBranches(true)
+		// should override any setBranchesToClone().
+		assertNotNull(
+				git2.getRepository().resolve("refs/remotes/origin/master"));
+		assertNotNull(git2.getRepository().resolve("refs/remotes/origin/test"));
+		RemoteConfig cfg = new RemoteConfig(git2.getRepository().getConfig(),
+				Constants.DEFAULT_REMOTE_NAME);
+		List<RefSpec> specs = cfg.getFetchRefSpecs();
+		assertEquals(1, specs.size());
+		assertEquals(new RefSpec("+refs/heads/*:refs/remotes/origin/*"),
+				specs.get(0));
+	}
+
+	@Test
+	public void testCloneRepositoryAllBranchesIndependent() throws Exception {
+		File directory = createTempDirectory(
+				"testCloneRepositoryAllBranchesIndependent");
+		CloneCommand command = Git.cloneRepository();
+		command.setCloneAllBranches(true);
+		command.setBranchesToClone(
+				Collections.singletonList("refs/heads/test"));
+		command.setCloneAllBranches(false);
+		command.setDirectory(directory);
+		command.setURI(fileUri());
+		Git git2 = command.call();
+		addRepoToClose(git2.getRepository());
+		assertNotNull(git2);
+		assertEquals(git2.getRepository().getFullBranch(), "refs/heads/test");
+		// Expect only the test branch; allBranches was re-set to false
+		assertNull(git2.getRepository().resolve("refs/remotes/origin/master"));
+		assertNotNull(git2.getRepository().resolve("refs/remotes/origin/test"));
+		RemoteConfig cfg = new RemoteConfig(git2.getRepository().getConfig(),
+				Constants.DEFAULT_REMOTE_NAME);
+		List<RefSpec> specs = cfg.getFetchRefSpecs();
+		assertEquals(1, specs.size());
+		assertEquals(new RefSpec("+refs/heads/test:refs/remotes/origin/test"),
+				specs.get(0));
+	}
+
 	public static String allRefNames(List<Ref> refs) {
 		StringBuilder sb = new StringBuilder();
 		for (Ref f : refs) {

org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java

@@ -26,6 +26,7 @@ import static org.junit.Assert.fail;
 
 import java.io.File;
 import java.io.IOException;
+import java.security.SecureRandom;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -509,6 +510,43 @@ public class FileReftableTest extends SampleDataRepositoryTestCase {
 		assertEquals(RefUpdate.Result.LOCK_FAILURE, rename.rename());
 	}
 
+	@Test
+	public void compactFully() throws Exception {
+		FileReftableDatabase refDb = (FileReftableDatabase) db.getRefDatabase();
+		PersonIdent person = new PersonIdent("jane", "jane@invalid");
+
+		ObjectId aId  = db.exactRef("refs/heads/a").getObjectId();
+		ObjectId bId  = db.exactRef("refs/heads/b").getObjectId();
+
+		SecureRandom random = new SecureRandom();
+		List<String> strs = new ArrayList<>();
+		for (int i = 0; i < 1024; i++) {
+			strs.add(String.format("%02x",
+					Integer.valueOf(random.nextInt(256))));
+		}
+
+		String randomStr = String.join("", strs);
+		String refName = "branch";
+		for (long i = 0; i < 2; i++) {
+			RefUpdate ru = refDb.newUpdate(refName, false);
+			ru.setNewObjectId(i % 2 == 0 ? aId : bId);
+			ru.setForceUpdate(true);
+			// Only write a large string in the first table, so it becomes much larger
+			// than the second, and the result is not autocompacted.
+			ru.setRefLogMessage(i == 0 ? randomStr : "short", false);
+			ru.setRefLogIdent(person);
+
+			RefUpdate.Result res = ru.update();
+			assertTrue(res == Result.NEW || res == FORCED);
+		}
+
+		assertEquals(refDb.exactRef(refName).getObjectId(), bId);
+		assertTrue(randomStr.equals(refDb.getReflogReader(refName).getReverseEntry(1).getComment()));
+		refDb.compactFully();
+		assertEquals(refDb.exactRef(refName).getObjectId(), bId);
+		assertTrue(randomStr.equals(refDb.getReflogReader(refName).getReverseEntry(1).getComment()));
+	}
+
 	@Test
 	public void reftableRefsStorageClass() throws IOException {
 		Ref b = db.exactRef("refs/heads/b");

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

@@ -71,7 +71,9 @@ public class CloneCommand extends TransportCommand<CloneCommand, Git> {
 
 	private ProgressMonitor monitor = NullProgressMonitor.INSTANCE;
 
-	private FETCH_TYPE fetchType = FETCH_TYPE.ALL_BRANCHES;
+	private boolean cloneAllBranches;
+
+	private boolean mirror;
 
 	private boolean cloneSubmodules;
 
@@ -85,6 +87,8 @@ public class CloneCommand extends TransportCommand<CloneCommand, Git> {
 
 	private boolean gitDirExistsInitially;
 
+	private FETCH_TYPE fetchType;
+
 	private enum FETCH_TYPE {
 		MULTIPLE_BRANCHES, ALL_BRANCHES, MIRROR
 	}
@@ -162,6 +166,7 @@ public class CloneCommand extends TransportCommand<CloneCommand, Git> {
 			throw new InvalidRemoteException(
 					MessageFormat.format(JGitText.get().invalidURL, uri), e);
 		}
+		setFetchType();
 		@SuppressWarnings("resource") // Closed by caller
 		Repository repository = init();
 		FetchResult fetchResult = null;
@@ -206,6 +211,20 @@ public class CloneCommand extends TransportCommand<CloneCommand, Git> {
 		return new Git(repository, true);
 	}
 
+	private void setFetchType() {
+		if (mirror) {
+			fetchType = FETCH_TYPE.MIRROR;
+			setBare(true);
+		} else if (cloneAllBranches) {
+			fetchType = FETCH_TYPE.ALL_BRANCHES;
+		} else if (branchesToClone != null && !branchesToClone.isEmpty()) {
+			fetchType = FETCH_TYPE.MULTIPLE_BRANCHES;
+		} else {
+			// Default: neither mirror nor all nor specific refs given
+			fetchType = FETCH_TYPE.ALL_BRANCHES;
+		}
+	}
+
 	private static boolean isNonEmptyDirectory(File dir) {
 		if (dir != null && dir.exists()) {
 			File[] files = dir.listFiles();
@@ -587,8 +606,7 @@ public class CloneCommand extends TransportCommand<CloneCommand, Git> {
 	 * @return {@code this}
 	 */
 	public CloneCommand setCloneAllBranches(boolean cloneAllBranches) {
-		this.fetchType = cloneAllBranches ? FETCH_TYPE.ALL_BRANCHES
-				: this.fetchType;
+		this.cloneAllBranches = cloneAllBranches;
 		return this;
 	}
 
@@ -608,10 +626,7 @@ public class CloneCommand extends TransportCommand<CloneCommand, Git> {
 	 * @since 5.6
 	 */
 	public CloneCommand setMirror(boolean mirror) {
-		if (mirror) {
-			this.fetchType = FETCH_TYPE.MIRROR;
-			setBare(true);
-		}
+		this.mirror = mirror;
 		return this;
 	}
 
@@ -632,8 +647,9 @@ public class CloneCommand extends TransportCommand<CloneCommand, Git> {
 	 * Set the branches or tags to clone.
 	 * <p>
 	 * This is ignored if {@link #setCloneAllBranches(boolean)
-	 * setCloneAllBranches(true)} is used. If {@code branchesToClone} is
-	 * {@code null} or empty, it's also ignored and all branches will be cloned.
+	 * setCloneAllBranches(true)} or {@link #setMirror(boolean) setMirror(true)}
+	 * is used. If {@code branchesToClone} is {@code null} or empty, it's also
+	 * ignored.
 	 * </p>
 	 *
 	 * @param branchesToClone
@@ -643,13 +659,7 @@ public class CloneCommand extends TransportCommand<CloneCommand, Git> {
 	 * @return {@code this}
 	 */
 	public CloneCommand setBranchesToClone(Collection<String> branchesToClone) {
-		if (branchesToClone == null || branchesToClone.isEmpty()) {
-			// fallback to default
-			fetchType = FETCH_TYPE.ALL_BRANCHES;
-		} else {
-			this.fetchType = FETCH_TYPE.MULTIPLE_BRANCHES;
-			this.branchesToClone = branchesToClone;
-		}
+		this.branchesToClone = branchesToClone;
 		return this;
 	}
 

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java

@@ -106,6 +106,7 @@ public class FileReftableDatabase extends RefDatabase {
 		reftableDatabase.getLock().lock();
 		try {
 			reftableStack.compactFully();
+			reftableDatabase.clearCache();
 		} finally {
 			reftableDatabase.getLock().unlock();
 		}

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableStack.java

@@ -237,8 +237,13 @@ public class FileReftableStack implements AutoCloseable {
 		long max = 1000;
 		long delay = 0;
 		boolean success = false;
-		while (System.currentTimeMillis() < deadline) {
+
+		// Don't check deadline for the first 3 retries, so we can step with a
+		// debugger without worrying about deadlines.
+		int tries = 0;
+		while (tries < 3 || System.currentTimeMillis() < deadline) {
 			List<String> names = readTableNames();
+			tries++;
 			try {
 				reloadOnce(names);
 				success = true;
@@ -260,9 +265,6 @@ public class FileReftableStack implements AutoCloseable {
 		}
 
 		if (!success) {
-			// TODO: should reexamine the 'refs' file to see if it was the same
-			// if it didn't change, then we must have corruption. If it did,
-			// retry.
 			throw new LockFailedException(stackPath);
 		}
 
@@ -374,7 +376,7 @@ public class FileReftableStack implements AutoCloseable {
 	 *
 	 * @param w
 	 *            writer to write data to a reftable under construction
-	 * @return true if the transaction.
+	 * @return true if the transaction was successful.
 	 * @throws IOException
 	 *             on I/O problems
 	 */

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java

@@ -71,13 +71,12 @@ import org.slf4j.LoggerFactory;
  */
 public class PackFile implements Iterable<PackIndex.MutableEntry> {
 	private final static Logger LOG = LoggerFactory.getLogger(PackFile.class);
-	/** Sorts PackFiles to be most recently created to least recently created. */
-	public static final Comparator<PackFile> SORT = new Comparator<PackFile>() {
-		@Override
-		public int compare(PackFile a, PackFile b) {
-			return b.packLastModified.compareTo(a.packLastModified);
-		}
-	};
+
+	/**
+	 * Sorts PackFiles to be most recently created to least recently created.
+	 */
+	public static final Comparator<PackFile> SORT = (a, b) -> b.packLastModified
+			.compareTo(a.packLastModified);
 
 	private final File packFile;
 

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java

@@ -142,43 +142,42 @@ public class PackWriter implements AutoCloseable {
 	private static final Map<WeakReference<PackWriter>, Boolean> instances =
 			new ConcurrentHashMap<>();
 
-	private static final Iterable<PackWriter> instancesIterable = new Iterable<PackWriter>() {
+	private static final Iterable<PackWriter> instancesIterable = () -> new Iterator<PackWriter>() {
+
+		private final Iterator<WeakReference<PackWriter>> it = instances
+				.keySet().iterator();
+
+		private PackWriter next;
+
 		@Override
-		public Iterator<PackWriter> iterator() {
-			return new Iterator<PackWriter>() {
-				private final Iterator<WeakReference<PackWriter>> it =
-						instances.keySet().iterator();
-				private PackWriter next;
-
-				@Override
-				public boolean hasNext() {
-					if (next != null)
-						return true;
-					while (it.hasNext()) {
-						WeakReference<PackWriter> ref = it.next();
-						next = ref.get();
-						if (next != null)
-							return true;
-						it.remove();
-					}
-					return false;
+		public boolean hasNext() {
+			if (next != null) {
+				return true;
+			}
+			while (it.hasNext()) {
+				WeakReference<PackWriter> ref = it.next();
+				next = ref.get();
+				if (next != null) {
+					return true;
 				}
+				it.remove();
+			}
+			return false;
+		}
 
-				@Override
-				public PackWriter next() {
-					if (hasNext()) {
-						PackWriter result = next;
-						next = null;
-						return result;
-					}
-					throw new NoSuchElementException();
-				}
+		@Override
+		public PackWriter next() {
+			if (hasNext()) {
+				PackWriter result = next;
+				next = null;
+				return result;
+			}
+			throw new NoSuchElementException();
+		}
 
-				@Override
-				public void remove() {
-					throw new UnsupportedOperationException();
-				}
-			};
+		@Override
+		public void remove() {
+			throw new UnsupportedOperationException();
 		}
 	};
 

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/LogCursor.java

@@ -12,6 +12,7 @@ package org.eclipse.jgit.internal.storage.reftable;
 
 import java.io.IOException;
 
+import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.lib.ReflogEntry;
 
 /**
@@ -45,8 +46,9 @@ public abstract class LogCursor implements AutoCloseable {
 	/**
 	 * Get current log entry.
 	 *
-	 * @return current log entry.
+	 * @return current log entry. Maybe null if we are producing deletions.
 	 */
+	@Nullable
 	public abstract ReflogEntry getReflogEntry();
 
 	/** {@inheritDoc} */

org.eclipse.jgit/src/org/eclipse/jgit/transport/AmazonS3.java

@@ -33,6 +33,7 @@ import java.text.MessageFormat;
 import java.text.SimpleDateFormat;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.Date;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -44,6 +45,8 @@ import java.util.Set;
 import java.util.SortedMap;
 import java.util.TimeZone;
 import java.util.TreeMap;
+import java.util.stream.Collectors;
+import java.time.Instant;
 
 import javax.crypto.Mac;
 import javax.crypto.spec.SecretKeySpec;
@@ -288,6 +291,8 @@ public class AmazonS3 {
 	 * <p>
 	 * This method is primarily meant for obtaining a "recursive directory
 	 * listing" rooted under the specified bucket and prefix location.
+	 * It returns the keys sorted in reverse order of LastModified time
+	 * (freshest keys first).
 	 *
 	 * @param bucket
 	 *            name of the bucket whose objects should be listed.
@@ -311,7 +316,10 @@ public class AmazonS3 {
 		do {
 			lp.list();
 		} while (lp.truncated);
-		return lp.entries;
+
+		Comparator<KeyInfo> comparator = Comparator.comparingLong(KeyInfo::getLastModifiedSecs);
+		return lp.entries.stream().sorted(comparator.reversed())
+			.map(KeyInfo::getName).collect(Collectors.toList());
 	}
 
 	/**
@@ -620,8 +628,26 @@ public class AmazonS3 {
 		return p;
 	}
 
+	/**
+	 * KeyInfo enables sorting of keys by lastModified time
+	 */
+	private static final class KeyInfo {
+		private final String name;
+		private final long lastModifiedSecs;
+		public KeyInfo(String aname, long lsecs) {
+			name = aname;
+			lastModifiedSecs = lsecs;
+		}
+		public String getName() {
+			return name;
+		}
+		public long getLastModifiedSecs() {
+			return lastModifiedSecs;
+		}
+	}
+
 	private final class ListParser extends DefaultHandler {
-		final List<String> entries = new ArrayList<>();
+		final List<KeyInfo> entries = new ArrayList<>();
 
 		private final String bucket;
 
@@ -630,6 +656,8 @@ public class AmazonS3 {
 		boolean truncated;
 
 		private StringBuilder data;
+		private String keyName;
+		private Instant keyLastModified;
 
 		ListParser(String bn, String p) {
 			bucket = bn;
@@ -641,7 +669,7 @@ public class AmazonS3 {
 			if (prefix.length() > 0)
 				args.put("prefix", prefix); //$NON-NLS-1$
 			if (!entries.isEmpty())
-				args.put("marker", prefix + entries.get(entries.size() - 1)); //$NON-NLS-1$
+				args.put("marker", prefix + entries.get(entries.size() - 1).getName()); //$NON-NLS-1$
 
 			for (int curAttempt = 0; curAttempt < maxAttempts; curAttempt++) {
 				final HttpURLConnection c = open("GET", bucket, "", args); //$NON-NLS-1$ //$NON-NLS-2$
@@ -650,6 +678,8 @@ public class AmazonS3 {
 				case HttpURLConnection.HTTP_OK:
 					truncated = false;
 					data = null;
+					keyName = null;
+					keyLastModified = null;
 
 					final XMLReader xr;
 					try {
@@ -683,8 +713,13 @@ public class AmazonS3 {
 		public void startElement(final String uri, final String name,
 				final String qName, final Attributes attributes)
 				throws SAXException {
-			if ("Key".equals(name) || "IsTruncated".equals(name)) //$NON-NLS-1$ //$NON-NLS-2$
+			if ("Key".equals(name) || "IsTruncated".equals(name) || "LastModified".equals(name)) { //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
 				data = new StringBuilder();
+			}
+			if ("Contents".equals(name)) { //$NON-NLS-1$
+				keyName = null;
+				keyLastModified = null;
+			}
 		}
 
 		@Override
@@ -704,10 +739,16 @@ public class AmazonS3 {
 		@Override
 		public void endElement(final String uri, final String name,
 				final String qName) throws SAXException {
-			if ("Key".equals(name)) //$NON-NLS-1$
-				entries.add(data.toString().substring(prefix.length()));
-			else if ("IsTruncated".equals(name)) //$NON-NLS-1$
+			if ("Key".equals(name))  { //$NON-NLS-1$
+				keyName = data.toString().substring(prefix.length());
+			} else if ("IsTruncated".equals(name)) { //$NON-NLS-1$
 				truncated = StringUtils.equalsIgnoreCase("true", data.toString()); //$NON-NLS-1$
+			} else if ("LastModified".equals(name)) { //$NON-NLS-1$
+				keyLastModified = Instant.parse(data.toString());
+			} else if ("Contents".equals(name)) { //$NON-NLS-1$
+				entries.add(new KeyInfo(keyName, keyLastModified.getEpochSecond()));
+			}
+
 			data = null;
 		}
 	}

org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportAmazonS3.java

@@ -23,6 +23,7 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Properties;
 import java.util.Set;
@@ -241,11 +242,14 @@ public class TransportAmazonS3 extends HttpTransport implements WalkTransport {
 
 		@Override
 		Collection<String> getPackNames() throws IOException {
+			// s3.list returns most recently modified packs first.
+			// These are the packs most likely to contain missing refs.
+			final List<String> packList = s3.list(bucket, resolveKey("pack")); //$NON-NLS-1$
 			final HashSet<String> have = new HashSet<>();
-			have.addAll(s3.list(bucket, resolveKey("pack"))); //$NON-NLS-1$
+			have.addAll(packList);
 
 			final Collection<String> packs = new ArrayList<>();
-			for (String n : have) {
+			for (String n : packList) {
 				if (!n.startsWith("pack-") || !n.endsWith(".pack")) //$NON-NLS-1$ //$NON-NLS-2$
 					continue;
 

pom.xml

@@ -163,7 +163,7 @@
     <osgi-core-version>4.3.1</osgi-core-version>
     <servlet-api-version>3.1.0</servlet-api-version>
     <jetty-version>9.4.25.v20191220</jetty-version>
-    <japicmp-version>0.14.1</japicmp-version>
+    <japicmp-version>0.14.3</japicmp-version>
     <httpclient-version>4.5.10</httpclient-version>
     <httpcore-version>4.4.12</httpcore-version>
     <slf4j-version>1.7.2</slf4j-version>
@@ -175,7 +175,7 @@
     <spotbugs-maven-plugin-version>3.1.12.2</spotbugs-maven-plugin-version>
     <maven-project-info-reports-plugin-version>3.0.0</maven-project-info-reports-plugin-version>
     <maven-jxr-plugin-version>3.0.0</maven-jxr-plugin-version>
-    <maven-surefire-plugin-version>3.0.0-M3</maven-surefire-plugin-version>
+    <maven-surefire-plugin-version>3.0.0-M4</maven-surefire-plugin-version>
     <maven-surefire-report-plugin-version>${maven-surefire-plugin-version}</maven-surefire-report-plugin-version>
     <maven-compiler-plugin-version>3.8.1</maven-compiler-plugin-version>
 
@@ -298,7 +298,7 @@
         <plugin>
           <groupId>org.apache.maven.plugins</groupId>
           <artifactId>maven-pmd-plugin</artifactId>
-          <version>3.12.0</version>
+          <version>3.13.0</version>
           <configuration>
             <sourceEncoding>utf-8</sourceEncoding>
             <minimumTokens>100</minimumTokens>