[JGIT PATCH 0/4] RepositoryTestCase cleanups

@@ -172,8 +172,6 @@ protected void tearDown() throws Exception {
 	protected Repository createNewEmptyRepo() throws IOException {
 		File newTestRepo = new File(trashParent, "new"+System.currentTimeMillis()+"/.git");
 		assertFalse(newTestRepo.exists());
-		File unusedDir = new File(trashParent, "tmp"+System.currentTimeMillis());
-		assertTrue(unusedDir.mkdirs());
 		final Repository newRepo = new Repository(newTestRepo);
 		newRepo.create();
 		return newRepo;

@@ -150,11 +150,13 @@ protected static void checkFile(File f, final String checkData)
 
 	protected Repository db;
 
+	private static int testcount;
+
 	public void setUp() throws Exception {
 		super.setUp();
 		configure();
 		recursiveDelete(trashParent);
-		trash = new File(trashParent,"trash"+System.currentTimeMillis());
+		trash = new File(trashParent,"trash"+System.currentTimeMillis()+"."+(testcount++));
 		trash_git = new File(trash, ".git");
 
 		Runtime.getRuntime().addShutdownHook(new Thread() {

@@ -200,7 +202,7 @@ protected void tearDown() throws Exception {
 	 * @throws IOException
 	 */
 	protected Repository createNewEmptyRepo() throws IOException {
-		File newTestRepo = new File(trashParent, "new"+System.currentTimeMillis()+"/.git");
+		File newTestRepo = new File(trashParent, "new"+System.currentTimeMillis()+"."+(testcount++)+"/.git");
 		assertFalse(newTestRepo.exists());
 		final Repository newRepo = new Repository(newTestRepo);
 		newRepo.create();

@@ -49,6 +49,19 @@
 import junit.framework.TestCase;
 import org.spearce.jgit.util.JGitTestUtil;
 
+/**
+ * Base class for most JGit unit tests.
+ *
+ * Sets up a predefined test repository and has support for creating additional
+ * repositories and destroying them when the tests are finished.
+ *
+ * A system property <em>jgit.junit.usemmmap</em> defines whether memory mapping
+ * is used. Memory mapping has an effect on the file system, in that memory
+ * mapped files in java cannot be deleted as long as they mapped arrays have not
+ * been reclaimed by the garbage collector. The programmer cannot control this
+ * with precision, though hinting using <em>{@link java.lang.System#gc}</em>
+ * often helps.
+ */
 public abstract class RepositoryTestCase extends TestCase {
 
 	protected final File trashParent = new File("trash");

@@ -66,6 +79,22 @@
 		jcommitter = new PersonIdent("J. Committer", "jcommitter@example.com");
 	}
 
+	protected boolean packedGitMMAP;
+
+	/**
+	 * Configure JGit before setting up test repositories.
+	 */
+	protected void configure() {
+		packedGitMMAP = "true".equals(System.getProperty("jgit.junit.usemmmap"));
+		WindowCache.reconfigure(128*1024, 8192, packedGitMMAP, 8192);
+	}
+
+	/**
+	 * Utility method to delete a directory recursively. It is
+	 * also used internally.
+	 *
+	 * @param dir
+	 */
 	protected static void recursiveDelete(final File dir) {
 		final File[] ls = dir.listFiles();
 		if (ls != null) {

@@ -123,6 +152,7 @@ protected static void checkFile(File f, final String checkData)
 
 	public void setUp() throws Exception {
 		super.setUp();
+		configure();
 		recursiveDelete(trashParent);
 		trash = new File(trashParent,"trash"+System.currentTimeMillis());
 		trash_git = new File(trash, ".git");

@@ -49,6 +49,7 @@
 import java.util.List;
 
 import junit.framework.TestCase;
+
 import org.spearce.jgit.util.JGitTestUtil;
 
 /**

@@ -93,16 +94,18 @@ protected void configure() {
 
 	/**
 	 * Utility method to delete a directory recursively. It is
-	 * also used internally.
+	 * also used internally. If a file or directory cannote be removed
+	 * it throws an AssertionFailure.
 	 *
 	 * @param dir
 	 */
 	protected void recursiveDelete(final File dir) {
-		recursiveDelete(dir, false, getClass().getName() + "." + getName());
+		recursiveDelete(dir, false, getClass().getName() + "." + getName(), true);
 	}
 
 	protected static boolean recursiveDelete(final File dir, boolean silent,
-			final String name) {
+			final String name, boolean failOnError) {
+		assert !(silent && failOnError);
 		if (!dir.exists())
 			return silent;
 		final File[] ls = dir.listFiles();

@@ -110,32 +113,42 @@ protected static boolean recursiveDelete(final File dir, boolean silent,
 			for (int k = 0; k < ls.length; k++) {
 				final File e = ls[k];
 				if (e.isDirectory()) {
-					silent = recursiveDelete(e, silent, name);
+					silent = recursiveDelete(e, silent, name, failOnError);
 				} else {
 					if (!e.delete()) {
 						if (!silent) {
-							String msg = "Warning: Failed to delete " + e;
-							if (name != null)
-								msg += " in " + name;
-							System.out.println(msg);
+							reportDeleteFailure(name, failOnError, e);
 						}
-						silent = true;
+						silent = !failOnError;
 					}
 				}
 			}
 		}
 		if (!dir.delete()) {
 			if (!silent) {
-				String msg = "Warning: Failed to delete " + dir;
-				if (name != null)
-					msg += " in " + name;
-				System.out.println(msg);
+				reportDeleteFailure(name, failOnError, dir);
 			}
-			silent = true;
+			silent = !failOnError;
 		}
 		return silent;
 	}
 
+	private static void reportDeleteFailure(final String name,
+			boolean failOnError, final File e) {
+		String severity;
+		if (failOnError)
+			severity = "Error";
+		else
+			severity = "Warning";
+		String msg = severity + ": Failed to delete " + e;
+		if (name != null)
+			msg += " in " + name;
+		if (failOnError)
+			fail(msg);
+		else
+			System.out.println(msg);
+	}
+
 	protected static void copyFile(final File src, final File dst)
 			throws IOException {
 		final FileInputStream fis = new FileInputStream(src);

@@ -186,7 +199,7 @@ public void setUp() throws Exception {
 		super.setUp();
 		configure();
 		final String name = getClass().getName() + "." + getName();
-		recursiveDelete(trashParent, true, name);
+		recursiveDelete(trashParent, true, name, false); // Cleanup old failed stuff
 		trash = new File(trashParent,"trash"+System.currentTimeMillis()+"."+(testcount++));
 		trash_git = new File(trash, ".git");
 		if (shutdownhook == null) {

@@ -201,7 +214,7 @@ public void run() {
 					System.gc();
 					for (Runnable r : shutDownCleanups)
 						r.run();
-					recursiveDelete(trashParent, false, null);
+					recursiveDelete(trashParent, false, null, false);
 				}
 			};
 			Runtime.getRuntime().addShutdownHook(shutdownhook);

@@ -241,10 +254,9 @@ protected void tearDown() throws Exception {
 			System.gc();
 
 		final String name = getClass().getName() + "." + getName();
-		recursiveDelete(trash, false, name);
+		recursiveDelete(trash, false, name, true);
 		for (Repository r : repositoriesToClose)
-			recursiveDelete(r.getWorkDir(), false, name);
-
+			recursiveDelete(r.getWorkDir(), false, name, true);
 		repositoriesToClose.clear();
 
 		super.tearDown();

@@ -266,7 +278,7 @@ protected Repository createNewEmptyRepo() throws IOException {
 		final String name = getClass().getName() + "." + getName();
 		shutDownCleanups.add(new Runnable() {
 			public void run() {
-				recursiveDelete(newTestRepo, false, name);
+				recursiveDelete(newTestRepo, false, name, false);
 			}
 		});
 		repositoriesToClose.add(newRepo);

@@ -165,10 +165,14 @@ protected File writeTrashFile(final String name, final String data)
 	protected static void checkFile(File f, final String checkData)
 			throws IOException {
 		Reader r = new InputStreamReader(new FileInputStream(f), "ISO-8859-1");
-		char[] data = new char[(int) f.length()];
-		if (f.length() !=  r.read(data))
-			throw new IOException("Internal error reading file data from "+f);
-		assertEquals(checkData, new String(data));
+		try {
+			char[] data = new char[(int) f.length()];
+			if (f.length() !=  r.read(data))
+				throw new IOException("Internal error reading file data from "+f);
+			assertEquals(checkData, new String(data));
+		} finally {
+			r.close();
+		}
 	}
 
 	protected Repository db;

@@ -424,9 +424,13 @@ public void test031_executeBit_coreModeFalse() throws IllegalArgumentException, 
 	private String content(File f) throws IOException {
 		byte[] buf = new byte[(int) f.length()];
 		FileInputStream is = new FileInputStream(f);
-		int read = is.read(buf);
-		assertEquals(f.length(), read);
-		return new String(buf, 0);
+		try {
+			int read = is.read(buf);
+			assertEquals(f.length(), read);
+			return new String(buf, 0);
+		} finally {
+			is.close();
+		}
 	}
 
 	private void delete(File f) throws IOException {

@@ -309,6 +309,7 @@ public void testWritePack4ThinPack() throws IOException {
 	public void testWritePack2SizeDeltasVsNoDeltas() throws Exception {
 		testWritePack2();
 		final int sizePack2NoDeltas = cos.getCount();
+		tearDown();
 		setUp();
 		testWritePack2DeltasReuseRefs();
 		final int sizePack2DeltasRefs = cos.getCount();

@@ -327,6 +328,7 @@ public void testWritePack2SizeDeltasVsNoDeltas() throws Exception {
 	public void testWritePack2SizeOffsetsVsRefs() throws Exception {
 		testWritePack2DeltasReuseRefs();
 		final int sizePack2DeltasRefs = cos.getCount();
+		tearDown();
 		setUp();
 		testWritePack2DeltasReuseOffsets();
 		final int sizePack2DeltasOffsets = cos.getCount();

@@ -344,6 +346,7 @@ public void testWritePack2SizeOffsetsVsRefs() throws Exception {
 	public void testWritePack4SizeThinVsNoThin() throws Exception {
 		testWritePack4();
 		final int sizePack4 = cos.getCount();
+		tearDown();
 		setUp();
 		testWritePack4ThinPack();
 		final int sizePack4Thin = cos.getCount();

@@ -96,21 +96,42 @@ protected void configure() {
 	 * @param dir
 	 */
 	protected static void recursiveDelete(final File dir) {
+		recursiveDelete(dir, false, null);
+	}
+
+	protected static boolean recursiveDelete(final File dir, boolean silent,
+			final String name) {
+		if (!dir.exists())
+			return silent;
 		final File[] ls = dir.listFiles();
 		if (ls != null) {
 			for (int k = 0; k < ls.length; k++) {
 				final File e = ls[k];
 				if (e.isDirectory()) {
-					recursiveDelete(e);
+					silent = recursiveDelete(e, silent, name);
 				} else {
-					e.delete();
+					if (!e.delete()) {
+						if (!silent) {
+							String msg = "Warning: Failed to delete " + e;
+							if (name != null)
+								msg += " in " + name;
+							System.out.println(msg);
+						}
+						silent = true;
+					}
 				}
 			}
 		}
-		dir.delete();
-		if (dir.exists()) {
-			System.out.println("Warning: Failed to delete " + dir);
+		if (!dir.delete()) {
+			if (!silent) {
+				String msg = "Warning: Failed to delete " + dir;
+				if (name != null)
+					msg += " in " + name;
+				System.out.println(msg);
+			}
+			silent = true;
 		}
+		return silent;
 	}
 
 	protected static void copyFile(final File src, final File dst)

@@ -155,14 +176,15 @@ protected static void checkFile(File f, final String checkData)
 	public void setUp() throws Exception {
 		super.setUp();
 		configure();
-		recursiveDelete(trashParent);
+		final String name = getClass().getName() + "." + getName();
+		recursiveDelete(trashParent, true, name);
 		trash = new File(trashParent,"trash"+System.currentTimeMillis()+"."+(testcount++));
 		trash_git = new File(trash, ".git");
 
 		Runtime.getRuntime().addShutdownHook(new Thread() {
 			@Override
 			public void run() {
-				recursiveDelete(trashParent);
+				recursiveDelete(trashParent, false, name);
 			}
 		});
 

@@ -45,6 +45,8 @@
 import java.io.InputStreamReader;
 import java.io.OutputStreamWriter;
 import java.io.Reader;
+import java.util.ArrayList;
+import java.util.List;
 
 import junit.framework.TestCase;
 import org.spearce.jgit.util.JGitTestUtil;

@@ -95,8 +97,8 @@ protected void configure() {
 	 *
 	 * @param dir
 	 */
-	protected static void recursiveDelete(final File dir) {
-		recursiveDelete(dir, false, null);
+	protected void recursiveDelete(final File dir) {
+		recursiveDelete(dir, false, getClass().getName() + "." + getName());
 	}
 
 	protected static boolean recursiveDelete(final File dir, boolean silent,

@@ -170,9 +172,12 @@ protected static void checkFile(File f, final String checkData)
 	}
 
 	protected Repository db;
-
+	private static Thread shutdownhook;
+	private static List<Runnable> shutDownCleanups = new ArrayList<Runnable>();
 	private static int testcount;
 
+	private ArrayList<Repository> repositoriesToClose = new ArrayList<Repository>();
+
 	public void setUp() throws Exception {
 		super.setUp();
 		configure();

@@ -180,14 +185,23 @@ public void setUp() throws Exception {
 		recursiveDelete(trashParent, true, name);
 		trash = new File(trashParent,"trash"+System.currentTimeMillis()+"."+(testcount++));
 		trash_git = new File(trash, ".git");
-
-		Runtime.getRuntime().addShutdownHook(new Thread() {
-			@Override
-			public void run() {
-				recursiveDelete(trashParent, false, name);
-			}
-		});
-
+		if (shutdownhook == null) {
+			shutdownhook = new Thread() {
+				@Override
+				public void run() {
+					// This may look superfluous, but is an extra attempt
+					// to clean up. First GC to release as many resources
+					// as possible and then try to clean up one test repo
+					// at a time (to record problems) and finally to drop
+					// the directory containing all test repositories.
+					System.gc();
+					for (Runnable r : shutDownCleanups)
+						r.run();
+					recursiveDelete(trashParent, false, null);
+				}
+			};
+			Runtime.getRuntime().addShutdownHook(shutdownhook);
+		}
 		db = new Repository(trash_git);
 		db.create();
 

@@ -213,6 +227,22 @@ copyFile(JGitTestUtil.getTestResourceFile(packs[k] + ".idx"), new File(packDir,
 
 	protected void tearDown() throws Exception {
 		db.close();
+		for (Repository r : repositoriesToClose)
+			r.close();
+
+		// Since memory mapping is controlled by the GC we need to
+		// tell it this is a good time to clean up and unlock
+		// memory mapped files.
+		if (packedGitMMAP)
+			System.gc();
+
+		final String name = getClass().getName() + "." + getName();
+		recursiveDelete(trash, false, name);
+		for (Repository r : repositoriesToClose)
+			recursiveDelete(r.getWorkDir(), false, name);
+
+		repositoriesToClose.clear();
+
 		super.tearDown();
 	}
 

@@ -224,10 +254,18 @@ protected void tearDown() throws Exception {
 	 * @throws IOException
 	 */
 	protected Repository createNewEmptyRepo() throws IOException {
-		File newTestRepo = new File(trashParent, "new"+System.currentTimeMillis()+"."+(testcount++)+"/.git");
+		final File newTestRepo = new File(trashParent, "new"
+				+ System.currentTimeMillis() + "." + (testcount++) + "/.git");
 		assertFalse(newTestRepo.exists());
 		final Repository newRepo = new Repository(newTestRepo);
 		newRepo.create();
+		final String name = getClass().getName() + "." + getName();
+		shutDownCleanups.add(new Runnable() {
+			public void run() {
+				recursiveDelete(newTestRepo, false, name);
+			}
+		});
+		repositoriesToClose.add(newRepo);
 		return newRepo;
 	}