[findbugs] Do not ignore exceptional return value

java.io.File.delete() reports failure as an exceptional
return value false. Fix the code which silently ignored
this exceptional return value. Also remove some duplicate
deletion helper methods.

This change depends on jgit change I430c77b5.

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

6 files changed, 29 insertions, 58 deletions

diff --git a/org.eclipse.egit.core.test/src/org/eclipse/egit/core/securestorage/EGitSecureStoreTest.java b/org.eclipse.egit.core.test/src/org/eclipse/egit/core/securestorage/EGitSecureStoreTest.java
index 5abaf3483a..0d0af585a1 100644
--- a/org.eclipse.egit.core.test/src/org/eclipse/egit/core/securestorage/EGitSecureStoreTest.java
+++ b/org.eclipse.egit.core.test/src/org/eclipse/egit/core/securestorage/EGitSecureStoreTest.java
@@ -28,6 +28,7 @@ import org.eclipse.equinox.security.storage.ISecurePreferences;
 import org.eclipse.equinox.security.storage.SecurePreferencesFactory;
 import org.eclipse.equinox.security.storage.provider.IProviderHints;
 import org.eclipse.jgit.transport.URIish;
+import org.eclipse.jgit.util.FileUtils;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -153,7 +154,8 @@ public class EGitSecureStoreTest {
 		String secureStorePath = ResourcesPlugin.getWorkspace().getRoot()
 				.getLocation().append("testSecureStore").toOSString();
 		File file = new File(secureStorePath);
-		testUtils.deleteRecursive(file);
+		if (file.exists())
+			FileUtils.delete(file, FileUtils.RECURSIVE | FileUtils.RETRY);
 		URL url = file.toURI().toURL();
 		secureStoreForTest = SecurePreferencesFactory.open(url, options);
 		secureStoreForTest.node("/GIT").removeNode();
diff --git a/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/GitTestCase.java b/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/GitTestCase.java
index da5920dd9b..4cd502ae34 100644
--- a/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/GitTestCase.java
+++ b/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/GitTestCase.java
@@ -21,6 +21,7 @@ import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.util.FileUtils;
 import org.eclipse.jgit.util.IO;
 import org.eclipse.jgit.util.SystemReader;
 import org.junit.After;
@@ -47,13 +48,15 @@ public abstract class GitTestCase {
 		project = new TestProject(true);
 		gitDir = new File(project.getProject().getWorkspace().getRoot()
 				.getRawLocation().toFile(), Constants.DOT_GIT);
-		testUtils.deleteRecursive(gitDir);
+		if (gitDir.exists())
+			FileUtils.delete(gitDir, FileUtils.RECURSIVE | FileUtils.RETRY);
 	}
 
 	@After
 	public void tearDown() throws Exception {
 		project.dispose();
-		testUtils.deleteRecursive(gitDir);
+		if (gitDir.exists())
+			FileUtils.delete(gitDir, FileUtils.RECURSIVE | FileUtils.RETRY);
 	}
 
 	protected ObjectId createFile(Repository repository, IProject actProject, String name, String content) throws IOException {
@@ -72,11 +75,14 @@ public abstract class GitTestCase {
 		}
 	}
 
-	protected ObjectId createFileCorruptShort(Repository repository, IProject actProject, String name, String content) throws IOException {
+	protected ObjectId createFileCorruptShort(Repository repository,
+			IProject actProject, String name, String content)
+			throws IOException {
 		ObjectId id = createFile(repository, actProject, name, content);
-		File file = new File(repository.getDirectory(), "objects/" + id.name().substring(0,2) + "/" + id.name().substring(2));
+		File file = new File(repository.getDirectory(), "objects/"
+				+ id.name().substring(0, 2) + "/" + id.name().substring(2));
 		byte[] readFully = IO.readFully(file);
-		file.delete();
+		FileUtils.delete(file);
 		FileOutputStream fileOutputStream = new FileOutputStream(file);
 		byte[] truncatedData = new byte[readFully.length - 1];
 		System.arraycopy(readFully, 0, truncatedData, 0, truncatedData.length);
diff --git a/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/TestProject.java b/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/TestProject.java
index 3ecd4fac22..b91adb709c 100644
--- a/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/TestProject.java
+++ b/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/TestProject.java
@@ -34,6 +34,7 @@ import org.eclipse.jdt.core.IType;
 import org.eclipse.jdt.core.JavaCore;
 import org.eclipse.jdt.core.JavaModelException;
 import org.eclipse.jdt.launching.JavaRuntime;
+import org.eclipse.jgit.util.FileUtils;
 import org.osgi.framework.Bundle;
 
 public class TestProject {
@@ -136,8 +137,11 @@ public class TestProject {
 		waitForIndexer();
 		if (project.exists())
 			project.delete(true, true, null);
-		else
-			testUtils.deleteRecursive(new File(location));
+		else {
+			File f = new File(location);
+			if (f.exists())
+				FileUtils.delete(f, FileUtils.RECURSIVE | FileUtils.RETRY);
+		}
 	}
 
 	private IFolder createBinFolder() throws CoreException {
diff --git a/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/TestUtils.java b/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/TestUtils.java
index 98bfd9b1a4..bc685849fb 100644
--- a/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/TestUtils.java
+++ b/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/TestUtils.java
@@ -23,6 +23,7 @@ import org.eclipse.core.resources.ResourcesPlugin;
 import org.eclipse.core.runtime.IPath;
 import org.eclipse.core.runtime.Path;
 import org.eclipse.jgit.util.FS;
+import org.eclipse.jgit.util.FileUtils;
 
 public class TestUtils {
 
@@ -31,49 +32,6 @@ public class TestUtils {
 	public final static String COMMITTER = "The Commiter <The.committer@some.com>";
 
 	/**
-	 * This method deletes a file / subtree
-	 *
-	 * @param d
-	 *            file / folder to delete
-	 * @throws IOException
-	 *             if file can not be deleted
-	 */
-	public void deleteRecursive(File d) throws IOException {
-		if (!d.exists())
-			return;
-
-		File[] files = d.listFiles();
-		if (files != null) {
-			for (int i = 0; i < files.length; ++i) {
-				if (files[i].isDirectory())
-					deleteRecursive(files[i]);
-				else
-					deleteFile(files[i]);
-			}
-		}
-		deleteFile(d);
-		assert !d.exists();
-	}
-
-	private void deleteFile(File file) throws IOException{
-		boolean deleted = false;
-		for (int i = 0; i < 10; i++) {
-			if (file.delete()) {
-				deleted = true;
-				break;
-			}
-			try {
-				Thread.sleep(100);
-			} catch (InterruptedException e) {
-				// ignore
-			}
-			System.out.println(">>> retried deleting " + file.getAbsolutePath());
-		}
-		if (!deleted)
-			throw new IOException("Retried 10 times. Could not delete " + file.getAbsolutePath());
-	}
-
-	/**
 	 * Create a "temporary" directory
 	 *
 	 * @param name
@@ -87,7 +45,7 @@ public class TestUtils {
 		File rootDir = new File(userHome, "EGitCoreTestTempDir");
 		File result = new File(rootDir, name);
 		if (result.exists())
-			deleteRecursive(result);
+			FileUtils.delete(result, FileUtils.RECURSIVE | FileUtils.RETRY);
 		return result;
 	}
 
@@ -100,7 +58,7 @@ public class TestUtils {
 		File userHome = FS.DETECTED.userHome();
 		File rootDir = new File(userHome, "EGitCoreTestTempDir");
 		if (rootDir.exists())
-			deleteRecursive(rootDir);
+			FileUtils.delete(rootDir, FileUtils.RECURSIVE | FileUtils.RETRY);
 	}
 
 	/**
@@ -190,7 +148,7 @@ public class TestUtils {
 		}
 		File testFile = new File(parentFile, projectName);
 		if (testFile.exists())
-			deleteRecursive(testFile);
+			FileUtils.delete(testFile, FileUtils.RECURSIVE | FileUtils.RETRY);
 
 		IProjectDescription desc = ResourcesPlugin.getWorkspace()
 				.newProjectDescription(projectName);
diff --git a/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/internal/mapping/HistoryTest.java b/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/internal/mapping/HistoryTest.java
index 7161708f58..172a812c2e 100644
--- a/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/internal/mapping/HistoryTest.java
+++ b/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/internal/mapping/HistoryTest.java
@@ -29,6 +29,7 @@ import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.storage.file.FileRepository;
+import org.eclipse.jgit.util.FileUtils;
 import org.eclipse.team.core.RepositoryProvider;
 import org.eclipse.team.core.history.IFileHistory;
 import org.eclipse.team.core.history.IFileHistoryProvider;
@@ -57,7 +58,8 @@ public class HistoryTest extends GitTestCase {
 		super.setUp();
 
 		// ensure we are working on an empty repository
-		testUtils.deleteRecursive(gitDir);
+		if (gitDir.exists())
+			FileUtils.delete(gitDir, FileUtils.RECURSIVE | FileUtils.RETRY);
 		thisGit = new FileRepository(gitDir);
 		workDir = thisGit.getWorkTree();
 		thisGit.create();
diff --git a/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/op/IgnoreOperationTest.java b/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/op/IgnoreOperationTest.java
index f6574c83a2..fac06bfc8a 100644
--- a/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/op/IgnoreOperationTest.java
+++ b/org.eclipse.egit.core.test/src/org/eclipse/egit/core/test/op/IgnoreOperationTest.java
@@ -14,7 +14,6 @@ import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
 import java.io.File;
-import java.io.IOException;
 
 import org.eclipse.core.resources.IFolder;
 import org.eclipse.core.resources.IResource;
@@ -25,6 +24,7 @@ import org.eclipse.egit.core.op.IgnoreOperation;
 import org.eclipse.egit.core.test.GitTestCase;
 import org.eclipse.egit.core.test.TestRepository;
 import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.util.FileUtils;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -48,8 +48,7 @@ public class IgnoreOperationTest extends GitTestCase {
 		File rootFile = root.getRawLocation().toFile();
 		File ignoreFile = new File(rootFile, Constants.GITIGNORE_FILENAME);
 		if (ignoreFile.exists()) {
-			if (!ignoreFile.delete())
-				throw new IOException(ignoreFile + " in use or undeletable");
+			FileUtils.delete(ignoreFile, FileUtils.RETRY);
 			assert !ignoreFile.exists();
 		}
 		super.tearDown();