Merge branch 'stable-4.7' into stable-4.8

* stable-4.7:
  Fix atomic lock file creation on NFS
  Use constant for ".lock"
  Fix handling of option core.supportsAtomicCreateNewFile
  GC: Avoid logging errors when deleting non-empty folders

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

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

@@ -46,6 +46,7 @@
 package org.eclipse.jgit.internal.storage.file;
 
 import static org.eclipse.jgit.junit.Assert.assertEquals;
+import static org.eclipse.jgit.lib.Constants.LOCK_SUFFIX;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -758,11 +759,11 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase {
 			// Check that the involved refs are the same despite the failure
 			assertExists(false, toName);
 			if (!toLock.equals(toName))
-				assertExists(false, toName + ".lock");
+				assertExists(false, toName + LOCK_SUFFIX);
-			assertExists(true, toLock + ".lock");
+			assertExists(true, toLock + LOCK_SUFFIX);
 			if (!toLock.equals(fromName))
-				assertExists(false, "logs/" + fromName + ".lock");
+				assertExists(false, "logs/" + fromName + LOCK_SUFFIX);
-			assertExists(false, "logs/" + toName + ".lock");
+			assertExists(false, "logs/" + toName + LOCK_SUFFIX);
 			assertEquals(oldHeadId, db.resolve(Constants.HEAD));
 			assertEquals(oldfromId, db.resolve(fromName));
 			assertNull(db.resolve(toName));

org.eclipse.jgit/.settings/.api_filters

@@ -38,5 +38,21 @@
                 <message_argument value="supportsAtomicCreateNewFile()"/>
             </message_arguments>
         </filter>
+        <filter id="1141899266">
+            <message_arguments>
+                <message_argument value="4.7"/>
+                <message_argument value="4.8"/>
+                <message_argument value="createNewFileAtomic(File)"/>
+            </message_arguments>
+        </filter>
+    </resource>
+    <resource path="src/org/eclipse/jgit/util/FS.java" type="org.eclipse.jgit.util.FS$LockToken">
+        <filter id="1141899266">
+            <message_arguments>
+                <message_argument value="4.7"/>
+                <message_argument value="4.8"/>
+                <message_argument value="LockToken"/>
+            </message_arguments>
+        </filter>
     </resource>
 </component>

org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties

@@ -119,6 +119,7 @@ checkoutUnexpectedResult=Checkout returned unexpected result {0}
 classCastNotA=Not a {0}
 cloneNonEmptyDirectory=Destination path "{0}" already exists and is not an empty directory
 closed=closed
+closeLockTokenFailed=Closing LockToken ''{0}'' failed
 collisionOn=Collision on {0}
 commandRejectedByHook=Rejected by "{0}" hook.\n{1}
 commandWasCalledInTheWrongState=Command {0} was called in the wrong state
@@ -284,6 +285,7 @@ expectedLessThanGot=expected less than ''{0}'', got ''{1}''
 expectedPktLineWithService=expected pkt-line with ''# service=-'', got ''{0}''
 expectedReceivedContentType=expected Content-Type {0}; received Content-Type {1}
 expectedReportForRefNotReceived={0}: expected report for ref {1} not received
+failedAtomicFileCreation=Atomic file creation failed, number of hard links to file {0} was not 2 but {1}"
 failedToDetermineFilterDefinition=An exception occured while determining filter definitions
 failedUpdatingRefs=failed updating refs
 failureDueToOneOfTheFollowing=Failure due to one of the following:
@@ -669,6 +671,7 @@ unknownObjectType2=unknown
 unknownRepositoryFormat=Unknown repository format
 unknownRepositoryFormat2=Unknown repository format "{0}"; expected "0".
 unknownZlibError=Unknown zlib error.
+unlockLockFileFailed=Unlocking LockFile ''{0}'' failed
 unmergedPath=Unmerged path: {0}
 unmergedPaths=Repository contains unmerged paths
 unpackException=Exception while parsing pack stream

org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java

@@ -177,6 +177,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String checkoutUnexpectedResult;
 	/***/ public String classCastNotA;
 	/***/ public String cloneNonEmptyDirectory;
+	/***/ public String closeLockTokenFailed;
 	/***/ public String closed;
 	/***/ public String collisionOn;
 	/***/ public String commandRejectedByHook;
@@ -343,6 +344,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String expectedPktLineWithService;
 	/***/ public String expectedReceivedContentType;
 	/***/ public String expectedReportForRefNotReceived;
+	/***/ public String failedAtomicFileCreation;
 	/***/ public String failedToDetermineFilterDefinition;
 	/***/ public String failedUpdatingRefs;
 	/***/ public String failureDueToOneOfTheFollowing;
@@ -728,6 +730,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String unknownRepositoryFormat;
 	/***/ public String unknownRepositoryFormat2;
 	/***/ public String unknownZlibError;
+	/***/ public String unlockLockFileFailed;
 	/***/ public String unmergedPath;
 	/***/ public String unmergedPaths;
 	/***/ public String unpackException;

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

@@ -54,6 +54,7 @@ import java.io.PrintWriter;
 import java.io.StringWriter;
 import java.nio.channels.Channels;
 import java.nio.channels.FileChannel;
+import java.nio.file.DirectoryNotEmptyException;
 import java.nio.file.DirectoryStream;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -940,6 +941,8 @@ public class GC {
 	private void delete(Path d) {
 		try {
 			Files.delete(d);
+		} catch (DirectoryNotEmptyException e) {
+			// Don't log
 		} catch (IOException e) {
 			LOG.error(MessageFormat.format(JGitText.get().cannotDeleteFile, d),
 					e);

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

@@ -44,6 +44,8 @@
 
 package org.eclipse.jgit.internal.storage.file;
 
+import static org.eclipse.jgit.lib.Constants.LOCK_SUFFIX;
+
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileNotFoundException;
@@ -62,7 +64,10 @@ import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.util.FS;
+import org.eclipse.jgit.util.FS.LockToken;
 import org.eclipse.jgit.util.FileUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Git style file locking and replacement.
@@ -75,7 +80,7 @@ import org.eclipse.jgit.util.FileUtils;
  * name.
  */
 public class LockFile {
-	static final String SUFFIX = ".lock"; //$NON-NLS-1$
+	private final static Logger LOG = LoggerFactory.getLogger(LockFile.class);
 
 	/**
 	 * Unlock the given file.
@@ -105,14 +110,15 @@ public class LockFile {
 	 * @return lock file
 	 */
 	static File getLockFile(File file) {
-		return new File(file.getParentFile(), file.getName() + SUFFIX);
+		return new File(file.getParentFile(),
+				file.getName() + LOCK_SUFFIX);
 	}
 
 	/** Filter to skip over active lock files when listing a directory. */
 	static final FilenameFilter FILTER = new FilenameFilter() {
 		@Override
 		public boolean accept(File dir, String name) {
-			return !name.endsWith(SUFFIX);
+			return !name.endsWith(LOCK_SUFFIX);
 		}
 	};
 
@@ -130,6 +136,8 @@ public class LockFile {
 
 	private FileSnapshot commitSnapshot;
 
+	private LockToken token;
+
 	/**
 	 * Create a new lock for any file.
 	 *
@@ -168,7 +176,8 @@ public class LockFile {
 	 */
 	public boolean lock() throws IOException {
 		FileUtils.mkdirs(lck.getParentFile(), true);
-		if (FS.DETECTED.createNewFile(lck)) {
+		token = FS.DETECTED.createNewFileAtomic(lck);
+		if (token.isCreated()) {
 			haveLck = true;
 			try {
 				os = new FileOutputStream(lck);
@@ -176,6 +185,8 @@ public class LockFile {
 				unlock();
 				throw ioe;
 			}
+		} else {
+			closeToken();
 		}
 		return haveLck;
 	}
@@ -456,6 +467,7 @@ public class LockFile {
 		try {
 			FileUtils.rename(lck, ref, StandardCopyOption.ATOMIC_MOVE);
 			haveLck = false;
+			closeToken();
 			return true;
 		} catch (IOException e) {
 			unlock();
@@ -463,6 +475,13 @@ public class LockFile {
 		}
 	}
 
+	private void closeToken() {
+		if (token != null) {
+			token.close();
+			token = null;
+		}
+	}
+
 	private void saveStatInformation() {
 		if (needSnapshot)
 			commitSnapshot = FileSnapshot.save(lck);
@@ -501,8 +520,9 @@ public class LockFile {
 		if (os != null) {
 			try {
 				os.close();
-			} catch (IOException ioe) {
+			} catch (IOException e) {
-				// Ignore this
+				LOG.error(MessageFormat
+						.format(JGitText.get().unlockLockFileFailed, lck), e);
 			}
 			os = null;
 		}
@@ -512,7 +532,10 @@ public class LockFile {
 			try {
 				FileUtils.delete(lck, FileUtils.RETRY);
 			} catch (IOException e) {
-				// couldn't delete the file even after retry.
+				LOG.error(MessageFormat
+						.format(JGitText.get().unlockLockFileFailed, lck), e);
+			} finally {
+				closeToken();
 			}
 		}
 	}

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

@@ -47,6 +47,7 @@ package org.eclipse.jgit.internal.storage.file;
 
 import static org.eclipse.jgit.lib.Constants.HEAD;
 import static org.eclipse.jgit.lib.Constants.LOGS;
+import static org.eclipse.jgit.lib.Constants.LOCK_SUFFIX;
 import static org.eclipse.jgit.lib.Constants.R_HEADS;
 import static org.eclipse.jgit.lib.Constants.R_REFS;
 import static org.eclipse.jgit.lib.Constants.R_REMOTES;
@@ -86,7 +87,7 @@ public class ReflogWriter {
 	 * @return the name of the ref's lock ref
 	 */
 	public static String refLockFor(final String name) {
-		return name + LockFile.SUFFIX;
+		return name + LOCK_SUFFIX;
 	}
 
 	private final Repository parent;
@@ -287,4 +288,4 @@ public class ReflogWriter {
 				|| refName.startsWith(R_REMOTES) //
 				|| refName.equals(R_STASH);
 	}
-}
+}

org.eclipse.jgit/src/org/eclipse/jgit/lib/Constants.java

@@ -665,6 +665,13 @@ public final class Constants {
 	public static final ObjectId EMPTY_BLOB_ID = ObjectId
 			.fromString("e69de29bb2d1d6434b8b29ae775ad8c2e48c5391");
 
+	/**
+	 * Suffix of lock file name
+	 *
+	 * @since 4.8
+	 */
+	public static final String LOCK_SUFFIX = ".lock"; //$NON-NLS-1$
+
 	private Constants() {
 		// Hide the default constructor
 	}

org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java

@@ -48,6 +48,8 @@
 
 package org.eclipse.jgit.lib;
 
+import static org.eclipse.jgit.lib.Constants.LOCK_SUFFIX;
+
 import java.io.BufferedOutputStream;
 import java.io.File;
 import java.io.FileNotFoundException;
@@ -1320,10 +1322,12 @@ public abstract class Repository implements AutoCloseable {
 	 */
 	public static boolean isValidRefName(final String refName) {
 		final int len = refName.length();
-		if (len == 0)
+		if (len == 0) {
 			return false;
-		if (refName.endsWith(".lock")) //$NON-NLS-1$
+		}
+		if (refName.endsWith(LOCK_SUFFIX)) {
 			return false;
+		}
 
 		// Refs may be stored as loose files so invalid paths
 		// on the local system must also be invalid refs.

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

@@ -43,6 +43,8 @@
 
 package org.eclipse.jgit.transport;
 
+import static org.eclipse.jgit.lib.Constants.LOCK_SUFFIX;
+
 import java.io.BufferedReader;
 import java.io.FileNotFoundException;
 import java.io.IOException;
@@ -341,7 +343,7 @@ public class TransportSftp extends SshTransport implements WalkTransport {
 
 		@Override
 		void writeFile(final String path, final byte[] data) throws IOException {
-			final String lock = path + ".lock"; //$NON-NLS-1$
+			final String lock = path + LOCK_SUFFIX;
 			try {
 				super.writeFile(lock, data);
 				try {

org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java

@@ -45,6 +45,7 @@ package org.eclipse.jgit.util;
 
 import java.io.BufferedReader;
 import java.io.ByteArrayInputStream;
+import java.io.Closeable;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
@@ -52,6 +53,8 @@ import java.io.InputStreamReader;
 import java.io.OutputStream;
 import java.io.PrintStream;
 import java.nio.charset.Charset;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.text.MessageFormat;
@@ -59,6 +62,7 @@ import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
@@ -806,13 +810,78 @@ public abstract class FS {
 	 *            the file to be created
 	 * @return <code>true</code> if the file was created, <code>false</code> if
 	 *         the file already existed
-	 * @throws IOException
+	 * @throws java.io.IOException
+	 * @deprecated use {@link #createNewFileAtomic(File)} instead
 	 * @since 4.5
 	 */
+	@Deprecated
 	public boolean createNewFile(File path) throws IOException {
 		return path.createNewFile();
 	}
 
+	/**
+	 * A token representing a file created by
+	 * {@link #createNewFileAtomic(File)}. The token must be retained until the
+	 * file has been deleted in order to guarantee that the unique file was
+	 * created atomically. As soon as the file is no longer needed the lock
+	 * token must be closed.
+	 *
+	 * @since 4.7
+	 */
+	public static class LockToken implements Closeable {
+		private boolean isCreated;
+
+		private Optional<Path> link;
+
+		LockToken(boolean isCreated, Optional<Path> link) {
+			this.isCreated = isCreated;
+			this.link = link;
+		}
+
+		/**
+		 * @return {@code true} if the file was created successfully
+		 */
+		public boolean isCreated() {
+			return isCreated;
+		}
+
+		@Override
+		public void close() {
+			if (link.isPresent()) {
+				try {
+					Files.delete(link.get());
+				} catch (IOException e) {
+					LOG.error(MessageFormat.format(JGitText.get().closeLockTokenFailed,
+							this), e);
+				}
+			}
+		}
+
+		@Override
+		public String toString() {
+			return "LockToken [lockCreated=" + isCreated + //$NON-NLS-1$
+					", link=" //$NON-NLS-1$
+					+ (link.isPresent() ? link.get().getFileName() + "]" //$NON-NLS-1$
+							: "<null>]"); //$NON-NLS-1$
+		}
+	}
+
+	/**
+	 * Create a new file. See {@link java.io.File#createNewFile()}. Subclasses
+	 * of this class may take care to provide a safe implementation for this
+	 * even if {@link #supportsAtomicCreateNewFile()} is <code>false</code>
+	 *
+	 * @param path
+	 *            the file to be created
+	 * @return LockToken this token must be closed after the created file was
+	 *         deleted
+	 * @throws IOException
+	 * @since 4.7
+	 */
+	public LockToken createNewFileAtomic(File path) throws IOException {
+		return new LockToken(path.createNewFile(), Optional.empty());
+	}
+
 	/**
 	 * See {@link FileUtils#relativizePath(String, String, String, boolean)}.
 	 *

org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java

@@ -52,14 +52,19 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.nio.file.attribute.PosixFilePermission;
+import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
+import java.util.UUID;
 
+import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.api.errors.JGitInternalException;
 import org.eclipse.jgit.errors.CommandFailedException;
 import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.lib.ConfigConstants;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.Repository;
@@ -360,9 +365,12 @@ public class FS_POSIX extends FS {
 	 * multiple clients manage to create the same lock file nlink would be
 	 * greater than 2 showing the error.
 	 *
-	 * @see https://www.time-travellers.org/shane/papers/NFS_considered_harmful.html
+	 * @see "https://www.time-travellers.org/shane/papers/NFS_considered_harmful.html"
+	 *
+	 * @deprecated use {@link FS_POSIX#createNewFileAtomic(File)} instead
 	 * @since 4.5
 	 */
+	@Deprecated
 	public boolean createNewFile(File lock) throws IOException {
 		if (!lock.createNewFile()) {
 			return false;
@@ -371,22 +379,94 @@ public class FS_POSIX extends FS {
 			return true;
 		}
 		Path lockPath = lock.toPath();
-		Path link = Files.createLink(Paths.get(lock.getAbsolutePath() + ".lnk"), //$NON-NLS-1$
+		Path link = null;
-				lockPath);
 		try {
+			link = Files.createLink(
+					Paths.get(lock.getAbsolutePath() + ".lnk"), //$NON-NLS-1$
+					lockPath);
 			Integer nlink = (Integer) (Files.getAttribute(lockPath,
 					"unix:nlink")); //$NON-NLS-1$
-			if (nlink != 2) {
+			if (nlink > 2) {
 				LOG.warn("nlink of link to lock file {0} was not 2 but {1}", //$NON-NLS-1$
 						lock.getPath(), nlink);
 				return false;
+			} else if (nlink < 2) {
+				supportsUnixNLink = false;
 			}
 			return true;
 		} catch (UnsupportedOperationException | IllegalArgumentException e) {
 			supportsUnixNLink = false;
 			return true;
 		} finally {
-			Files.delete(link);
+			if (link != null) {
+				Files.delete(link);
+			}
 		}
 	}
+
+	/**
+	 * {@inheritDoc}
+	 * <p>
+	 * An implementation of the File#createNewFile() semantics which can create
+	 * a unique file atomically also on NFS. If the config option
+	 * {@code core.supportsAtomicCreateNewFile = true} (which is the default)
+	 * then simply File#createNewFile() is called.
+	 *
+	 * But if {@code core.supportsAtomicCreateNewFile = false} then after
+	 * successful creation of the lock file a hard link to that lock file is
+	 * created and the attribute nlink of the lock file is checked to be 2. If
+	 * multiple clients manage to create the same lock file nlink would be
+	 * greater than 2 showing the error. The hard link needs to be retained
+	 * until the corresponding file is no longer needed in order to prevent that
+	 * another process can create the same file concurrently using another NFS
+	 * client which might not yet see the file due to caching.
+	 *
+	 * @see "https://www.time-travellers.org/shane/papers/NFS_considered_harmful.html"
+	 * @param file
+	 *            the unique file to be created atomically
+	 * @return LockToken this lock token must be held until the file is no
+	 *         longer needed
+	 * @throws IOException
+	 * @since 5.0
+	 */
+	@Override
+	public LockToken createNewFileAtomic(File file) throws IOException {
+		if (!file.createNewFile()) {
+			return token(false, null);
+		}
+		if (supportsAtomicCreateNewFile() || !supportsUnixNLink) {
+			return token(true, null);
+		}
+		Path link = null;
+		Path path = file.toPath();
+		try {
+			link = Files.createLink(Paths.get(uniqueLinkPath(file)), path);
+			Integer nlink = (Integer) (Files.getAttribute(path,
+					"unix:nlink")); //$NON-NLS-1$
+			if (nlink.intValue() > 2) {
+				LOG.warn(MessageFormat.format(
+						JGitText.get().failedAtomicFileCreation, path, nlink));
+				return token(false, link);
+			} else if (nlink.intValue() < 2) {
+				supportsUnixNLink = false;
+			}
+			return token(true, link);
+		} catch (UnsupportedOperationException | IllegalArgumentException e) {
+			supportsUnixNLink = false;
+			return token(true, link);
+		}
+	}
+
+	private static LockToken token(boolean created, @Nullable Path p) {
+		return ((p != null) && Files.exists(p))
+				? new LockToken(created, Optional.of(p))
+				: new LockToken(created, Optional.empty());
+	}
+
+	private static String uniqueLinkPath(File file) {
+		UUID id = UUID.randomUUID();
+		return file.getAbsolutePath() + "." //$NON-NLS-1$
+				+ Long.toHexString(id.getMostSignificantBits())
+				+ Long.toHexString(id.getLeastSignificantBits());
+	}
 }