Merge branch 'stable-5.6' into stable-5.7

* stable-5.6:
  LockFile: create OutputStream only when needed

Change-Id: I7c0e37d2cee0923662a7e39df5a802a84c017e4f

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012, GitHub Inc. and others
+ * Copyright (C) 2012, 2021 GitHub Inc. and others
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Distribution License v. 1.0 which is available at
@@ -9,10 +9,16 @@
  */
 package org.eclipse.jgit.internal.storage.file;
 
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
+import java.io.File;
+import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
+
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.errors.JGitInternalException;
 import org.eclipse.jgit.errors.LockFailedException;
@@ -49,4 +55,149 @@ public class LockFileTest extends RepositoryTestCase {
 			}
 		}
 	}
+
+	@Test
+	public void testLockTwice() throws Exception {
+		File f = writeTrashFile("somefile", "content");
+		LockFile lock = new LockFile(f);
+		assertTrue(lock.lock());
+		lock.write("other".getBytes(StandardCharsets.US_ASCII));
+		lock.commit();
+		assertFalse(lock.isLocked());
+		checkFile(f, "other");
+		assertTrue(lock.lock());
+		assertTrue(lock.isLocked());
+		try (OutputStream out = lock.getOutputStream()) {
+			out.write("second".getBytes(StandardCharsets.US_ASCII));
+		}
+		lock.commit();
+		assertFalse(lock.isLocked());
+		checkFile(f, "second");
+	}
+
+	@Test
+	public void testLockTwiceUnlock() throws Exception {
+		File f = writeTrashFile("somefile", "content");
+		LockFile lock = new LockFile(f);
+		assertTrue(lock.lock());
+		assertTrue(lock.isLocked());
+		lock.write("other".getBytes(StandardCharsets.US_ASCII));
+		lock.unlock();
+		assertFalse(lock.isLocked());
+		checkFile(f, "content");
+		assertTrue(lock.lock());
+		assertTrue(lock.isLocked());
+		try (OutputStream out = lock.getOutputStream()) {
+			out.write("second".getBytes(StandardCharsets.US_ASCII));
+		}
+		lock.commit();
+		assertFalse(lock.isLocked());
+		checkFile(f, "second");
+	}
+
+	@Test
+	public void testLockWriteTwiceThrows1() throws Exception {
+		File f = writeTrashFile("somefile", "content");
+		LockFile lock = new LockFile(f);
+		assertTrue(lock.lock());
+		assertTrue(lock.isLocked());
+		lock.write("other".getBytes(StandardCharsets.US_ASCII));
+		assertThrows(Exception.class,
+				() -> lock.write("second".getBytes(StandardCharsets.US_ASCII)));
+		lock.unlock();
+	}
+
+	@Test
+	public void testLockWriteTwiceThrows2() throws Exception {
+		File f = writeTrashFile("somefile", "content");
+		LockFile lock = new LockFile(f);
+		assertTrue(lock.lock());
+		assertTrue(lock.isLocked());
+		try (OutputStream out = lock.getOutputStream()) {
+			out.write("other".getBytes(StandardCharsets.US_ASCII));
+		}
+		assertThrows(Exception.class,
+				() -> lock.write("second".getBytes(StandardCharsets.US_ASCII)));
+		lock.unlock();
+	}
+
+	@Test
+	public void testLockWriteTwiceThrows3() throws Exception {
+		File f = writeTrashFile("somefile", "content");
+		LockFile lock = new LockFile(f);
+		assertTrue(lock.lock());
+		assertTrue(lock.isLocked());
+		lock.write("other".getBytes(StandardCharsets.US_ASCII));
+		assertThrows(Exception.class, () -> {
+			try (OutputStream out = lock.getOutputStream()) {
+				out.write("second".getBytes(StandardCharsets.US_ASCII));
+			}
+		});
+		lock.unlock();
+	}
+
+	@Test
+	public void testLockWriteTwiceThrows4() throws Exception {
+		File f = writeTrashFile("somefile", "content");
+		LockFile lock = new LockFile(f);
+		assertTrue(lock.lock());
+		assertTrue(lock.isLocked());
+		try (OutputStream out = lock.getOutputStream()) {
+			out.write("other".getBytes(StandardCharsets.US_ASCII));
+		}
+		assertThrows(Exception.class, () -> {
+			try (OutputStream out = lock.getOutputStream()) {
+				out.write("second".getBytes(StandardCharsets.US_ASCII));
+			}
+		});
+		lock.unlock();
+	}
+
+	@Test
+	public void testLockUnclosedCommitThrows() throws Exception {
+		File f = writeTrashFile("somefile", "content");
+		LockFile lock = new LockFile(f);
+		assertTrue(lock.lock());
+		assertTrue(lock.isLocked());
+		try (OutputStream out = lock.getOutputStream()) {
+			out.write("other".getBytes(StandardCharsets.US_ASCII));
+			assertThrows(Exception.class, () -> lock.commit());
+		}
+	}
+
+	@Test
+	public void testLockNested() throws Exception {
+		File f = writeTrashFile("somefile", "content");
+		LockFile lock = new LockFile(f);
+		assertTrue(lock.lock());
+		assertTrue(lock.isLocked());
+		assertThrows(IllegalStateException.class, () -> lock.lock());
+		assertTrue(lock.isLocked());
+		lock.unlock();
+	}
+
+	@Test
+	public void testLockHeld() throws Exception {
+		File f = writeTrashFile("somefile", "content");
+		LockFile lock = new LockFile(f);
+		assertTrue(lock.lock());
+		assertTrue(lock.isLocked());
+		LockFile lock2 = new LockFile(f);
+		assertFalse(lock2.lock());
+		assertFalse(lock2.isLocked());
+		assertTrue(lock.isLocked());
+		lock.unlock();
+	}
+
+	@Test
+	public void testLockForAppend() throws Exception {
+		File f = writeTrashFile("somefile", "content");
+		LockFile lock = new LockFile(f);
+		assertTrue(lock.lockForAppend());
+		assertTrue(lock.isLocked());
+		lock.write("other".getBytes(StandardCharsets.US_ASCII));
+		lock.commit();
+		assertFalse(lock.isLocked());
+		checkFile(f, "contentother");
+	}
 }

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

@@ -410,11 +410,18 @@ listingPacks=Listing packs
 localObjectsIncomplete=Local objects incomplete.
 localRefIsMissingObjects=Local ref {0} is missing object(s).
 localRepository=local repository
+lockAlreadyHeld=Lock on {0} already held
 lockCountMustBeGreaterOrEqual1=lockCount must be >= 1
 lockError=lock error: {0}
 lockFailedRetry=locking {0} failed after {1} retries
 lockOnNotClosed=Lock on {0} not closed.
 lockOnNotHeld=Lock on {0} not held.
+lockStreamClosed=Output to lock on {0} already closed
+lockStreamMultiple=Output to lock on {0} already opened
+logInconsistentFiletimeDiff={}: inconsistent duration from file timestamps on {}, {}: {} > {}, but diff = {}. Aborting measurement at resolution {}.
+logLargerFiletimeDiff={}: inconsistent duration from file timestamps on {}, {}: diff = {} > {} (last good value). Aborting measurement.
+logSmallerFiletime={}: got smaller file timestamp on {}, {}: {} < {}. Aborting measurement at resolution {}.
+logXDGConfigHomeInvalid=Environment variable XDG_CONFIG_HOME contains an invalid path {}
 maxCountMustBeNonNegative=max count must be >= 0
 mergeConflictOnNonNoteEntries=Merge conflict on non-note entries: base = {0}, ours = {1}, theirs = {2}
 mergeConflictOnNotes=Merge conflict on note {0}. base = {1}, ours = {2}, theirs = {2}

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

@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2010, 2013 Sasa Zivkov <sasa.zivkov@sap.com>
- * Copyright (C) 2012, Research In Motion Limited and others
+ * Copyright (C) 2012, 2021 Research In Motion Limited and others
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Distribution License v. 1.0 which is available at
@@ -439,10 +439,17 @@ public class JGitText extends TranslationBundle {
 	/***/ public String localRefIsMissingObjects;
 	/***/ public String localRepository;
 	/***/ public String lockCountMustBeGreaterOrEqual1;
+	/***/ public String lockAlreadyHeld;
 	/***/ public String lockError;
 	/***/ public String lockFailedRetry;
 	/***/ public String lockOnNotClosed;
 	/***/ public String lockOnNotHeld;
+	/***/ public String lockStreamClosed;
+	/***/ public String lockStreamMultiple;
+	/***/ public String logInconsistentFiletimeDiff;
+	/***/ public String logLargerFiletimeDiff;
+	/***/ public String logSmallerFiletime;
+	/***/ public String logXDGConfigHomeInvalid;
 	/***/ public String maxCountMustBeNonNegative;
 	/***/ public String mergeConflictOnNonNoteEntries;
 	/***/ public String mergeConflictOnNotes;

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

@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2007, Robin Rosenberg <robin.rosenberg@dewire.com>
- * Copyright (C) 2006-2008, Shawn O. Pearce <spearce@spearce.org> and others
+ * Copyright (C) 2006-2021, Shawn O. Pearce <spearce@spearce.org> and others
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Distribution License v. 1.0 which is available at
@@ -96,11 +96,15 @@ public class LockFile {
 
 	private boolean haveLck;
 
-	FileOutputStream os;
+	private FileOutputStream os;
 
 	private boolean needSnapshot;
 
-	boolean fsync;
+	private boolean fsync;
+
+	private boolean isAppend;
+
+	private boolean written;
 
 	private FileSnapshot commitSnapshot;
 
@@ -127,6 +131,10 @@ public class LockFile {
 	 *             does not hold the lock.
 	 */
 	public boolean lock() throws IOException {
+		if (haveLck) {
+			throw new IllegalStateException(
+					MessageFormat.format(JGitText.get().lockAlreadyHeld, ref));
+		}
 		FileUtils.mkdirs(lck.getParentFile(), true);
 		try {
 			token = FS.DETECTED.createNewFileAtomic(lck);
@@ -134,18 +142,15 @@ public class LockFile {
 			LOG.error(JGitText.get().failedCreateLockFile, lck, e);
 			throw e;
 		}
-		if (token.isCreated()) {
+		boolean obtainedLock = token.isCreated();
+		if (obtainedLock) {
 			haveLck = true;
-			try {
-				os = new FileOutputStream(lck);
-			} catch (IOException ioe) {
-				unlock();
-				throw ioe;
-			}
+			isAppend = false;
+			written = false;
 		} else {
 			closeToken();
 		}
-		return haveLck;
+		return obtainedLock;
 	}
 
 	/**
@@ -158,12 +163,24 @@ public class LockFile {
 	 *             does not hold the lock.
 	 */
 	public boolean lockForAppend() throws IOException {
-		if (!lock())
+		if (!lock()) {
 			return false;
+		}
 		copyCurrentContent();
+		isAppend = true;
+		written = false;
 		return true;
 	}
 
+	// For tests only
+	boolean isLocked() {
+		return haveLck;
+	}
+
+	private FileOutputStream getStream() throws IOException {
+		return new FileOutputStream(lck, isAppend);
+	}
+
 	/**
 	 * Copy the current file content into the temporary file.
 	 * <p>
@@ -185,32 +202,31 @@ public class LockFile {
 	 */
 	public void copyCurrentContent() throws IOException {
 		requireLock();
-		try {
+		try (FileOutputStream out = getStream()) {
 			try (FileInputStream fis = new FileInputStream(ref)) {
 				if (fsync) {
 					FileChannel in = fis.getChannel();
 					long pos = 0;
 					long cnt = in.size();
 					while (0 < cnt) {
-						long r = os.getChannel().transferFrom(in, pos, cnt);
+						long r = out.getChannel().transferFrom(in, pos, cnt);
 						pos += r;
 						cnt -= r;
 					}
 				} else {
 					final byte[] buf = new byte[2048];
 					int r;
-					while ((r = fis.read(buf)) >= 0)
-						os.write(buf, 0, r);
+					while ((r = fis.read(buf)) >= 0) {
+						out.write(buf, 0, r);
 				}
 			}
 		} catch (FileNotFoundException fnfe) {
 			if (ref.exists()) {
-				unlock();
 				throw fnfe;
 			}
 			// Don't worry about a file that doesn't exist yet, it
 			// conceptually has no current content to copy.
-			//
+			}
 		} catch (IOException | RuntimeException | Error ioe) {
 			unlock();
 			throw ioe;
@@ -253,18 +269,22 @@ public class LockFile {
 	 */
 	public void write(byte[] content) throws IOException {
 		requireLock();
-		try {
+		try (FileOutputStream out = getStream()) {
+			if (written) {
+				throw new IOException(MessageFormat
+						.format(JGitText.get().lockStreamClosed, ref));
+			}
 			if (fsync) {
-				FileChannel fc = os.getChannel();
+				FileChannel fc = out.getChannel();
 				ByteBuffer buf = ByteBuffer.wrap(content);
-				while (0 < buf.remaining())
+				while (0 < buf.remaining()) {
 					fc.write(buf);
+				}
 				fc.force(true);
 			} else {
-				os.write(content);
+				out.write(content);
 			}
-			os.close();
-			os = null;
+			written = true;
 		} catch (IOException | RuntimeException | Error ioe) {
 			unlock();
 			throw ioe;
@@ -283,36 +303,68 @@ public class LockFile {
 	public OutputStream getOutputStream() {
 		requireLock();
 
-		final OutputStream out;
-		if (fsync)
+		if (written || os != null) {
+			throw new IllegalStateException(MessageFormat
+					.format(JGitText.get().lockStreamMultiple, ref));
+		}
+
+		return new OutputStream() {
+
+			private OutputStream out;
+
+			private boolean closed;
+
+			private OutputStream get() throws IOException {
+				if (written) {
+					throw new IOException(MessageFormat
+							.format(JGitText.get().lockStreamMultiple, ref));
+				}
+				if (out == null) {
+					os = getStream();
+					if (fsync) {
 			out = Channels.newOutputStream(os.getChannel());
-		else
+					} else {
 			out = os;
+					}
+				}
+				return out;
+			}
 
-		return new OutputStream() {
 			@Override
 			public void write(byte[] b, int o, int n)
 					throws IOException {
-				out.write(b, o, n);
+				get().write(b, o, n);
 			}
 
 			@Override
 			public void write(byte[] b) throws IOException {
-				out.write(b);
+				get().write(b);
 			}
 
 			@Override
 			public void write(int b) throws IOException {
-				out.write(b);
+				get().write(b);
 			}
 
 			@Override
 			public void close() throws IOException {
+				if (closed) {
+					return;
+				}
+				closed = true;
 				try {
-					if (fsync)
+					if (written) {
+						throw new IOException(MessageFormat
+								.format(JGitText.get().lockStreamClosed, ref));
+					}
+					if (out != null) {
+						if (fsync) {
 						os.getChannel().force(true);
+						}
 					out.close();
 					os = null;
+					}
+					written = true;
 				} catch (IOException | RuntimeException | Error ioe) {
 					unlock();
 					throw ioe;
@@ -322,7 +374,7 @@ public class LockFile {
 	}
 
 	void requireLock() {
-		if (os == null) {
+		if (!haveLck) {
 			unlock();
 			throw new IllegalStateException(MessageFormat.format(JGitText.get().lockOnNotHeld, ref));
 		}
@@ -411,6 +463,8 @@ public class LockFile {
 		try {
 			FileUtils.rename(lck, ref, StandardCopyOption.ATOMIC_MOVE);
 			haveLck = false;
+			isAppend = false;
+			written = false;
 			closeToken();
 			return true;
 		} catch (IOException e) {
@@ -497,6 +551,8 @@ public class LockFile {
 				closeToken();
 			}
 		}
+		isAppend = false;
+		written = false;
 	}
 
 	/** {@inheritDoc} */