Fix hardcoded use of target/trash in LocalDiskRepositoryTestCase

`pwd`/target is only valid in Maven Reactor builds where Maven
has moved into the project directory and created a target for
the build output. Most other build systems do not use "target"
and may not even perform a directory change between test suites.

Rewrite LocalDiskRepositoryTestCase's temporary directory code
to use the system specified location and create new unique names.
This prevents fixes between concurrently running tests and allows
the caller to specify the root using java.io.tmpdir.

Update the surefire command lines to use target within each project as
the system temporary directory during unit testing, preventing JGit's
own test suite from writing to /tmp or somewhere like C:\tmp.

Change-Id: I9e8431f6ddfc16fee89f677bcce67c99cfb56782

org.eclipse.jgit.ant.test/pom.xml

@@ -94,7 +94,7 @@
       <plugin>
         <artifactId>maven-surefire-plugin</artifactId>
         <configuration>
-          <argLine>-Xmx256m -Dfile.encoding=UTF-8</argLine>
+          <argLine>-Xmx256m -Dfile.encoding=UTF-8 -Djava.io.tmpdir=${project.build.directory}</argLine>
         </configuration>
       </plugin>
     </plugins>

org.eclipse.jgit.java7.test/pom.xml

@@ -107,7 +107,7 @@
       <plugin>
 	<artifactId>maven-surefire-plugin</artifactId>
 	<configuration>
-	  <argLine>-Xmx256m -Dfile.encoding=UTF-8</argLine>
+	  <argLine>-Xmx256m -Dfile.encoding=UTF-8 -Djava.io.tmpdir=${project.build.directory}</argLine>
 	</configuration>
       </plugin>
     </plugins>

org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/LocalDiskRepositoryTestCase.java

@@ -89,10 +89,6 @@ import org.junit.Before;
  * descriptors or address space for the test process.
  */
 public abstract class LocalDiskRepositoryTestCase {
-	private static Thread shutdownHook;
-
-	private static int testCount;
-
 	private static final boolean useMMAP = "true".equals(System
 			.getProperty("jgit.junit.usemmap"));
 
@@ -102,36 +98,20 @@ public abstract class LocalDiskRepositoryTestCase {
 	/** A fake (but stable) identity for committer fields in the test. */
 	protected PersonIdent committer;
 
-	private final File trash = new File(new File("target"), "trash");
-
 	private final List<Repository> toClose = new ArrayList<Repository>();
+	private File tmp;
 
 	private MockSystemReader mockSystemReader;
 
 	@Before
 	public void setUp() throws Exception {
-
+		tmp = File.createTempFile("jgit_test_", "_tmp");
-		synchronized(this) {
+		CleanupThread.deleteOnShutdown(tmp);
-			if (shutdownHook == null) {
+		if (!tmp.delete() || !tmp.mkdir())
-				shutdownHook = new Thread() {
+			throw new IOException("Cannot create " + tmp);
-					@Override
-					public void run() {
-						// On windows accidentally open files or memory
-						// mapped regions may prevent files from being deleted.
-						// Suggesting a GC increases the likelihood that our
-						// test repositories actually get removed after the
-						// tests, even in the case of failure.
-						System.gc();
-						recursiveDelete("SHUTDOWN", trash, false, false);
-					}
-				};
-				Runtime.getRuntime().addShutdownHook(shutdownHook);
-			}
-		}
-		recursiveDelete(testId(), trash, true, false);
 
 		mockSystemReader = new MockSystemReader();
-		mockSystemReader.userGitConfig = new FileBasedConfig(new File(trash,
+		mockSystemReader.userGitConfig = new FileBasedConfig(new File(tmp,
 				"usergitconfig"), FS.DETECTED);
 		ceilTestDirectories(getCeilings());
 		SystemReader.setInstance(mockSystemReader);
@@ -152,9 +132,12 @@ public abstract class LocalDiskRepositoryTestCase {
 		c.install();
 	}
 
+	protected File getTemporaryDirectory() {
+		return tmp.getAbsoluteFile();
+	}
 
 	protected List<File> getCeilings() {
-		return Collections.singletonList(trash.getParentFile().getAbsoluteFile());
+		return Collections.singletonList(getTemporaryDirectory());
 	}
 
 	private void ceilTestDirectories(List<File> ceilings) {
@@ -184,8 +167,10 @@ public abstract class LocalDiskRepositoryTestCase {
 		//
 		if (useMMAP)
 			System.gc();
-
+		if (tmp != null)
-		recursiveDelete(testId(), trash, false, true);
+			recursiveDelete(tmp, false, true);
+		if (tmp != null && !tmp.exists())
+			CleanupThread.removed(tmp);
 	}
 
 	/** Increment the {@link #author} and {@link #committer} times. */
@@ -206,11 +191,11 @@ public abstract class LocalDiskRepositoryTestCase {
 	 *            the recursively directory to delete, if present.
 	 */
 	protected void recursiveDelete(final File dir) {
-		recursiveDelete(testId(), dir, false, true);
+		recursiveDelete(dir, false, true);
 	}
 
-	private static boolean recursiveDelete(final String testName,
+	private static boolean recursiveDelete(final File dir,
-			final File dir, boolean silent, boolean failOnError) {
+			boolean silent, boolean failOnError) {
 		assert !(silent && failOnError);
 		if (!dir.exists())
 			return silent;
@@ -219,31 +204,24 @@ public abstract class LocalDiskRepositoryTestCase {
 			for (int k = 0; k < ls.length; k++) {
 				final File e = ls[k];
 				if (e.isDirectory())
-					silent = recursiveDelete(testName, e, silent, failOnError);
+					silent = recursiveDelete(e, silent, failOnError);
 				else if (!e.delete()) {
 					if (!silent)
-						reportDeleteFailure(testName, failOnError, e);
+						reportDeleteFailure(failOnError, e);
 					silent = !failOnError;
 				}
 			}
 		if (!dir.delete()) {
 			if (!silent)
-				reportDeleteFailure(testName, failOnError, dir);
+				reportDeleteFailure(failOnError, dir);
 			silent = !failOnError;
 		}
 		return silent;
 	}
 
-	private static void reportDeleteFailure(final String testName,
+	private static void reportDeleteFailure(boolean failOnError, File e) {
-			final boolean failOnError, final File e) {
+		String severity = failOnError ? "ERROR" : "WARNING";
-		final String severity;
+		String msg = severity + ": Failed to delete " + e;
-		if (failOnError)
-			severity = "ERROR";
-		else
-			severity = "WARNING";
-
-		final String msg = severity + ": Failed to delete " + e + " in "
-				+ testName;
 		if (failOnError)
 			fail(msg);
 		else
@@ -302,10 +280,6 @@ public abstract class LocalDiskRepositoryTestCase {
 		toClose.add(r);
 	}
 
-	private static String createUniqueTestFolderPrefix() {
-		return "test" + (System.currentTimeMillis() + "_" + (testCount++));
-	}
-
 	/**
 	 * Creates a unique directory for a test
 	 *
@@ -315,9 +289,7 @@ public abstract class LocalDiskRepositoryTestCase {
 	 * @throws IOException
 	 */
 	protected File createTempDirectory(String name) throws IOException {
-		String gitdirName = createUniqueTestFolderPrefix();
+		File directory = new File(createTempFile(), name);
-		File parent = new File(trash, gitdirName);
-		File directory = new File(parent, name);
 		FileUtils.mkdirs(directory);
 		return directory.getCanonicalFile();
 	}
@@ -332,16 +304,31 @@ public abstract class LocalDiskRepositoryTestCase {
 	 * @throws IOException
 	 */
 	protected File createUniqueTestGitDir(boolean bare) throws IOException {
-		String gitdirName = createUniqueTestFolderPrefix();
+		String gitdirName = createTempFile().getPath();
 		if (!bare)
 			gitdirName += "/";
-		gitdirName += Constants.DOT_GIT;
+		return new File(gitdirName + Constants.DOT_GIT);
-		File gitdir = new File(trash, gitdirName);
-		return gitdir.getCanonicalFile();
 	}
 
+	/**
+	 * Allocates a new unique file path that does not exist.
+	 * <p>
+	 * Unlike the standard {@code File.createTempFile} the returned path does
+	 * not exist, but may be created by another thread in a race with the
+	 * caller. Good luck.
+	 * <p>
+	 * This method is inherently unsafe due to a race condition between creating
+	 * the name and the first use that reserves it.
+	 *
+	 * @return a unique path that does not exist.
+	 * @throws IOException
+	 */
 	protected File createTempFile() throws IOException {
-		return new File(trash, "tmp-" + UUID.randomUUID()).getCanonicalFile();
+		File p = File.createTempFile("tmp_", "", tmp);
+		if (!p.delete()) {
+			throw new IOException("Cannot obtain unique path " + tmp);
+		}
+		return p;
 	}
 
 	/**
@@ -399,7 +386,7 @@ public abstract class LocalDiskRepositoryTestCase {
 	 *             the file could not be written.
 	 */
 	protected File write(final String body) throws IOException {
-		final File f = File.createTempFile("temp", "txt", trash);
+		final File f = File.createTempFile("temp", "txt", tmp);
 		try {
 			write(f, body);
 			return f;
@@ -449,8 +436,41 @@ public abstract class LocalDiskRepositoryTestCase {
 		return new HashMap<String, String>(System.getenv());
 	}
 
-	private String testId() {
+	private static final class CleanupThread extends Thread {
-		return getClass().getName() + "." + testCount;
+		private static final CleanupThread me;
-	}
+		static {
+			me = new CleanupThread();
+			Runtime.getRuntime().addShutdownHook(me);
+		}
 
+		static void deleteOnShutdown(File tmp) {
+			synchronized (me) {
+				me.toDelete.add(tmp);
+			}
+		}
+
+		static void removed(File tmp) {
+			synchronized (me) {
+				me.toDelete.remove(tmp);
+			}
+		}
+
+		private final List<File> toDelete = new ArrayList<File>();
+
+		@Override
+		public void run() {
+			// On windows accidentally open files or memory
+			// mapped regions may prevent files from being deleted.
+			// Suggesting a GC increases the likelihood that our
+			// test repositories actually get removed after the
+			// tests, even in the case of failure.
+			System.gc();
+			synchronized (this) {
+				boolean silent = false;
+				boolean failOnError = false;
+				for (File tmp : toDelete)
+					recursiveDelete(tmp, silent, failOnError);
+			}
+		}
+	}
 }

org.eclipse.jgit.test/pom.xml

@@ -136,7 +136,7 @@
       <plugin>
         <artifactId>maven-surefire-plugin</artifactId>
         <configuration>
-          <argLine>-Xmx256m -Dfile.encoding=UTF-8</argLine>
+          <argLine>-Xmx256m -Dfile.encoding=UTF-8 -Djava.io.tmpdir=${project.build.directory}</argLine>
         </configuration>
       </plugin>
     </plugins>

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

@@ -187,16 +187,15 @@ public class RepositorySetupWorkDirTest extends LocalDiskRepositoryTestCase {
 		}
 	}
 
-	private static File getFile(String... pathComponents) throws IOException {
+	private File getFile(String... pathComponents) throws IOException {
-		String rootPath = new File(new File("target"), "trash").getPath();
+		File dir = getTemporaryDirectory();
 		for (String pathComponent : pathComponents)
-			rootPath = rootPath + File.separatorChar + pathComponent;
+			dir = new File(dir, pathComponent);
-		File result = new File(rootPath);
+		FileUtils.mkdirs(dir, true);
-		FileUtils.mkdirs(result, true);
+		return dir;
-		return result;
 	}
 
-	private static void setBare(File gitDir, boolean bare) throws IOException,
+	private void setBare(File gitDir, boolean bare) throws IOException,
 			ConfigInvalidException {
 		FileBasedConfig cfg = configFor(gitDir);
 		cfg.setBoolean(ConfigConstants.CONFIG_CORE_SECTION, null,
@@ -204,7 +203,7 @@ public class RepositorySetupWorkDirTest extends LocalDiskRepositoryTestCase {
 		cfg.save();
 	}
 
-	private static void setWorkTree(File gitDir, File workTree)
+	private void setWorkTree(File gitDir, File workTree)
 			throws IOException,
 			ConfigInvalidException {
 		String path = workTree.getAbsolutePath();
@@ -214,7 +213,7 @@ public class RepositorySetupWorkDirTest extends LocalDiskRepositoryTestCase {
 		cfg.save();
 	}
 
-	private static FileBasedConfig configFor(File gitDir) throws IOException,
+	private FileBasedConfig configFor(File gitDir) throws IOException,
 			ConfigInvalidException {
 		File configPath = new File(gitDir, Constants.CONFIG);
 		FileBasedConfig cfg = new FileBasedConfig(configPath, FS.DETECTED);
@@ -222,14 +221,14 @@ public class RepositorySetupWorkDirTest extends LocalDiskRepositoryTestCase {
 		return cfg;
 	}
 
-	private static void assertGitdirPath(Repository repo, String... expected)
+	private void assertGitdirPath(Repository repo, String... expected)
 			throws IOException {
 		File exp = getFile(expected).getCanonicalFile();
 		File act = repo.getDirectory().getCanonicalFile();
 		assertEquals("Wrong Git Directory", exp, act);
 	}
 
-	private static void assertWorkdirPath(Repository repo, String... expected)
+	private void assertWorkdirPath(Repository repo, String... expected)
 			throws IOException {
 		File exp = getFile(expected).getCanonicalFile();
 		File act = repo.getWorkTree().getCanonicalFile();