[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.

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

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

@@ -97,6 +97,7 @@ import org.eclipse.jgit.storage.file.PackIndex.MutableEntry;
 import org.eclipse.jgit.storage.pack.PackWriter;
 import org.eclipse.jgit.treewalk.TreeWalk;
 import org.eclipse.jgit.treewalk.filter.PathFilterGroup;
+import org.eclipse.jgit.util.FileUtils;
 
 /**
  * Wrapper to make creating test data easier.
@@ -651,10 +652,10 @@ public class TestRepository<R extends Repository> {
 		}
 	}
 
-	private void prunePacked(ObjectDirectory odb) {
+	private void prunePacked(ObjectDirectory odb) throws IOException {
 		for (PackFile p : odb.getPacks()) {
 			for (MutableEntry e : p)
-				odb.fileFor(e.toObjectId()).delete();
+				FileUtils.delete(odb.fileFor(e.toObjectId()));
 		}
 	}
 

org.eclipse.jgit.test/tst/org/eclipse/jgit/api/AddCommandTest.java

@@ -57,6 +57,7 @@ import org.eclipse.jgit.lib.FileMode;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.RepositoryTestCase;
+import org.eclipse.jgit.util.FileUtils;
 
 public class AddCommandTest extends RepositoryTestCase {
 
@@ -172,7 +173,7 @@ public class AddCommandTest extends RepositoryTestCase {
 		DirCache dc = git.add().addFilepattern("a.txt").call();
 
 		dc.getEntry(0).getObjectId();
-		file.delete();
+		FileUtils.delete(file);
 
 		// is supposed to do nothing
 		dc = git.add().addFilepattern("a.txt").call();
@@ -195,7 +196,7 @@ public class AddCommandTest extends RepositoryTestCase {
 		git.commit().setMessage("commit a.txt").call();
 
 		dc.getEntry(0).getObjectId();
-		file.delete();
+		FileUtils.delete(file);
 
 		// is supposed to do nothing
 		dc = git.add().addFilepattern("a.txt").call();
@@ -392,7 +393,7 @@ public class AddCommandTest extends RepositoryTestCase {
 		writer.close();
 
 		// file sub/b.txt is deleted
-		file2.delete();
+		FileUtils.delete(file2);
 
 		git.add().addFilepattern("sub").call();
 		// change in sub/a.txt is staged
@@ -444,7 +445,7 @@ public class AddCommandTest extends RepositoryTestCase {
 		writer.print("modified content");
 		writer.close();
 
-		file2.delete();
+		FileUtils.delete(file2);
 
 		// change in sub/a.txt is staged
 		// deletion of sub/b.txt is staged

org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/IgnoreNodeTest.java

@@ -51,6 +51,7 @@ import org.eclipse.jgit.lib.RepositoryTestCase;
 import org.eclipse.jgit.treewalk.FileTreeIterator;
 import org.eclipse.jgit.treewalk.TreeWalk;
 import org.eclipse.jgit.treewalk.WorkingTreeIterator;
+import org.eclipse.jgit.util.FileUtils;
 
 /**
  * Tests ignore node behavior on the local filesystem.
@@ -128,7 +129,7 @@ public class IgnoreNodeTest extends RepositoryTestCase {
 		assertEntry(F, tracked, ".gitignore");
 		assertEntry(F, tracked, "out");
 
-		new File(trash, "out").delete();
+		FileUtils.delete(new File(trash, "out"));
 		writeTrashFile("out/foo", "");
 
 		beginWalk();

org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ReadTreeTest.java

@@ -200,7 +200,7 @@ public abstract class ReadTreeTest extends RepositoryTestCase {
 
 		// rule 11
 		setupCase(headMap, null, idxMap);
-		new File(trash, "foo").delete();
+		assertTrue(new File(trash, "foo").delete());
 		writeTrashFile("foo", "bar");
 		db.getIndex().getMembers()[0].forceRecheck();
 		go();
@@ -238,7 +238,7 @@ public abstract class ReadTreeTest extends RepositoryTestCase {
 
 		// rules 21
 		setupCase(idxMap, mergeMap, idxMap);
-		new File(trash, "foo").delete();
+		assertTrue(new File(trash, "foo").delete());
 		writeTrashFile("foo", "bar");
 		db.getIndex().getMembers()[0].forceRecheck();
 		go();

org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryTestCase.java

@@ -62,6 +62,7 @@ import org.eclipse.jgit.dircache.DirCacheEntry;
 import org.eclipse.jgit.junit.LocalDiskRepositoryTestCase;
 import org.eclipse.jgit.storage.file.FileRepository;
 import org.eclipse.jgit.treewalk.FileTreeIterator;
+import org.eclipse.jgit.util.FileUtils;
 
 /**
  * Base class for most JGit unit tests.
@@ -98,8 +99,7 @@ public abstract class RepositoryTestCase extends LocalDiskRepositoryTestCase {
 
 	protected void deleteTrashFile(final String name) throws IOException {
 		File path = new File(db.getWorkTree(), name);
-		if (!path.delete())
-			throw new IOException("Could not delete file " + path.getPath());
+		FileUtils.delete(path);
 	}
 
 	protected static void checkFile(File f, final String checkData)
@@ -323,7 +323,7 @@ public abstract class RepositoryTestCase extends LocalDiskRepositoryTestCase {
 			}
 			return actTime;
 		} finally {
-			tmp.delete();
+			FileUtils.delete(tmp);
 		}
 	}
 }

org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/ConcurrentRepackTest.java

@@ -64,6 +64,7 @@ import org.eclipse.jgit.lib.RepositoryTestCase;
 import org.eclipse.jgit.revwalk.RevObject;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.storage.pack.PackWriter;
+import org.eclipse.jgit.util.FileUtils;
 
 public class ConcurrentRepackTest extends RepositoryTestCase {
 	public void setUp() throws Exception {
@@ -236,10 +237,10 @@ public class ConcurrentRepackTest extends RepositoryTestCase {
 		touch(begin, files[0].getParentFile());
 	}
 
-	private static void delete(final File[] list) {
+	private static void delete(final File[] list) throws IOException {
 		final long begin = list[0].getParentFile().lastModified();
 		for (final File f : list) {
-			f.delete();
+			FileUtils.delete(f);
 			assertFalse(f + " was removed", f.exists());
 		}
 		touch(begin, list[0].getParentFile());

org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/FileTreeIteratorTest.java

@@ -51,6 +51,7 @@ import org.eclipse.jgit.lib.FileMode;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.RepositoryTestCase;
+import org.eclipse.jgit.util.FileUtils;
 import org.eclipse.jgit.util.RawParseUtils;
 
 public class FileTreeIteratorTest extends RepositoryTestCase {
@@ -170,7 +171,7 @@ public class FileTreeIteratorTest extends RepositoryTestCase {
 
 		// Verify it was cached by removing the file and getting it again.
 		//
-		new File(trash, paths[0]).delete();
+		FileUtils.delete(new File(trash, paths[0]));
 		assertEquals(expect, top.getEntryObjectId());
 	}
 

org.eclipse.jgit/src/org/eclipse/jgit/api/RebaseCommand.java

@@ -78,6 +78,7 @@ import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.lib.RefUpdate.Result;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.util.FileUtils;
 import org.eclipse.jgit.util.IO;
 import org.eclipse.jgit.util.RawParseUtils;
 
@@ -220,7 +221,7 @@ public class RebaseCommand extends GitCommand<RebaseResult> {
 					rup = repo.updateRef(Constants.HEAD);
 					rup.link(headName);
 				}
-				deleteRecursive(rebaseDir);
+				FileUtils.delete(rebaseDir, FileUtils.RECURSIVE);
 				return new RebaseResult(Status.OK);
 			}
 			return new RebaseResult(Status.UP_TO_DATE);
@@ -482,7 +483,7 @@ public class RebaseCommand extends GitCommand<RebaseResult> {
 				}
 			}
 			// cleanup the files
-			deleteRecursive(rebaseDir);
+			FileUtils.delete(rebaseDir, FileUtils.RECURSIVE);
 			return new RebaseResult(Status.ABORTED);
 
 		} finally {
@@ -490,16 +491,6 @@ public class RebaseCommand extends GitCommand<RebaseResult> {
 		}
 	}
 
-	private void deleteRecursive(File fileOrFolder) throws IOException {
-		File[] children = fileOrFolder.listFiles();
-		if (children != null) {
-			for (File child : children)
-				deleteRecursive(child);
-		}
-		if (!fileOrFolder.delete())
-			throw new IOException("Could not delete " + fileOrFolder.getPath());
-	}
-
 	private String readFile(File directory, String fileName) throws IOException {
 		byte[] content = IO.readFully(new File(directory, fileName));
 		// strip off the last LF

org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheCheckout.java

@@ -72,6 +72,7 @@ import org.eclipse.jgit.treewalk.WorkingTreeIterator;
 import org.eclipse.jgit.treewalk.WorkingTreeOptions;
 import org.eclipse.jgit.treewalk.filter.PathFilter;
 import org.eclipse.jgit.util.FS;
+import org.eclipse.jgit.util.FileUtils;
 
 /**
  * This class handles checking out one or two trees merging with the index. This
@@ -784,7 +785,10 @@ public class DirCacheCheckout {
 		}
 		for (String r : removed) {
 			File file = new File(repo.getWorkTree(), r);
-			file.delete();
+			if (!file.delete())
+				throw new CheckoutConflictException(
+						MessageFormat.format(JGitText.get().cannotDeleteFile,
+								file.getAbsolutePath()));
 			removeEmptyParents(file);
 		}
 	}
@@ -854,7 +858,7 @@ public class DirCacheCheckout {
 		if (!tmpFile.renameTo(f)) {
 			// tried to rename which failed. Let' delete the target file and try
 			// again
-			f.delete();
+			FileUtils.delete(f);
 			if (!tmpFile.renameTo(f)) {
 				throw new IOException(MessageFormat.format(
 						JGitText.get().couldNotWriteFile, tmpFile.getPath(),

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

@@ -79,6 +79,7 @@ import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.storage.file.ReflogReader;
 import org.eclipse.jgit.treewalk.TreeWalk;
 import org.eclipse.jgit.util.FS;
+import org.eclipse.jgit.util.FileUtils;
 import org.eclipse.jgit.util.IO;
 import org.eclipse.jgit.util.RawParseUtils;
 
@@ -1186,7 +1187,7 @@ public abstract class Repository {
 				fos.close();
 			}
 		} else {
-			mergeMsgFile.delete();
+			FileUtils.delete(mergeMsgFile);
 		}
 	}
 
@@ -1252,7 +1253,7 @@ public abstract class Repository {
 				bos.close();
 			}
 		} else {
-			mergeHeadFile.delete();
+			FileUtils.delete(mergeHeadFile);
 		}
 	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java

@@ -83,6 +83,7 @@ import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.treewalk.CanonicalTreeParser;
 import org.eclipse.jgit.treewalk.NameConflictTreeWalk;
 import org.eclipse.jgit.treewalk.WorkingTreeIterator;
+import org.eclipse.jgit.util.FileUtils;
 
 /**
  * A three-way merger performing a content-merge if necessary
@@ -261,7 +262,7 @@ public class ResolveMerger extends ThreeWayMerger {
 				p = p.getParentFile();
 			if (p == null || p.isDirectory())
 				throw new IOException(JGitText.get().cannotCreateDirectory);
-			p.delete();
+			FileUtils.delete(p);
 			if (!f.mkdirs())
 				throw new IOException(JGitText.get().cannotCreateDirectory);
 		}
@@ -530,7 +531,7 @@ public class ResolveMerger extends ThreeWayMerger {
 			} finally {
 				is.close();
 				if (inCore)
-					of.delete();
+					FileUtils.delete(of);
 			}
 			builder.add(dce);
 			return true;

org.eclipse.jgit/src/org/eclipse/jgit/storage/file/CachedObjectDirectory.java

@@ -216,7 +216,7 @@ class CachedObjectDirectory extends FileObjectDatabase {
 
 	@Override
 	InsertLooseObjectResult insertUnpackedObject(File tmp, ObjectId objectId,
-			boolean createDuplicate) {
+			boolean createDuplicate) throws IOException {
 		InsertLooseObjectResult result = wrapped.insertUnpackedObject(tmp,
 				objectId, createDuplicate);
 		switch (result) {

org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileObjectDatabase.java

@@ -276,7 +276,7 @@ abstract class FileObjectDatabase extends ObjectDatabase {
 			AnyObjectId objectId) throws IOException;
 
 	abstract InsertLooseObjectResult insertUnpackedObject(File tmp,
-			ObjectId id, boolean createDuplicate);
+			ObjectId id, boolean createDuplicate) throws IOException;
 
 	abstract FileObjectDatabase newCachedFileObjectDatabase();
 

org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileRepository.java

@@ -66,6 +66,7 @@ import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.storage.file.FileObjectDatabase.AlternateHandle;
 import org.eclipse.jgit.storage.file.FileObjectDatabase.AlternateRepository;
+import org.eclipse.jgit.util.FileUtils;
 import org.eclipse.jgit.util.SystemReader;
 
 /**
@@ -240,7 +241,7 @@ public class FileRepository extends Repository {
 
 			getFS().setExecute(tmp, false);
 			final boolean off = getFS().canExecute(tmp);
-			tmp.delete();
+			FileUtils.delete(tmp);
 
 			fileMode = on && !off;
 		} else {

org.eclipse.jgit/src/org/eclipse/jgit/storage/file/ObjectDirectory.java

@@ -75,6 +75,7 @@ import org.eclipse.jgit.lib.RepositoryCache.FileKey;
 import org.eclipse.jgit.storage.pack.ObjectToPack;
 import org.eclipse.jgit.storage.pack.PackWriter;
 import org.eclipse.jgit.util.FS;
+import org.eclipse.jgit.util.FileUtils;
 
 /**
  * Traditional file system based {@link ObjectDatabase}.
@@ -454,15 +455,15 @@ public class ObjectDirectory extends FileObjectDatabase {
 
 	@Override
 	InsertLooseObjectResult insertUnpackedObject(File tmp, ObjectId id,
-			boolean createDuplicate) {
+			boolean createDuplicate) throws IOException {
 		// If the object is already in the repository, remove temporary file.
 		//
 		if (unpackedObjectCache.isUnpacked(id)) {
-			tmp.delete();
+			FileUtils.delete(tmp);
 			return InsertLooseObjectResult.EXISTS_LOOSE;
 		}
 		if (!createDuplicate && has(id)) {
-			tmp.delete();
+			FileUtils.delete(tmp);
 			return InsertLooseObjectResult.EXISTS_PACKED;
 		}
 
@@ -474,7 +475,7 @@ public class ObjectDirectory extends FileObjectDatabase {
 			// that already exists. We can't be sure renameTo() would
 			// fail on all platforms if dst exists, so we check first.
 			//
-			tmp.delete();
+			FileUtils.delete(tmp);
 			return InsertLooseObjectResult.EXISTS_LOOSE;
 		}
 		if (tmp.renameTo(dst)) {
@@ -493,7 +494,7 @@ public class ObjectDirectory extends FileObjectDatabase {
 		}
 
 		if (!createDuplicate && has(id)) {
-			tmp.delete();
+			FileUtils.delete(tmp);
 			return InsertLooseObjectResult.EXISTS_PACKED;
 		}
 
@@ -502,7 +503,7 @@ public class ObjectDirectory extends FileObjectDatabase {
 		// either. We really don't know what went wrong, so
 		// fail.
 		//
-		tmp.delete();
+		FileUtils.delete(tmp);
 		return InsertLooseObjectResult.FAILURE;
 	}
 

org.eclipse.jgit/src/org/eclipse/jgit/storage/file/ObjectDirectoryInserter.java

@@ -63,6 +63,7 @@ import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.util.FileUtils;
 
 /** Creates loose objects in a {@link ObjectDirectory}. */
 class ObjectDirectoryInserter extends ObjectInserter {
@@ -150,7 +151,7 @@ class ObjectDirectoryInserter extends ObjectInserter {
 			return tmp;
 		} finally {
 			if (delete)
-				tmp.delete();
+				FileUtils.delete(tmp);
 		}
 	}
 

org.eclipse.jgit/src/org/eclipse/jgit/storage/file/PackLock.java

@@ -48,6 +48,7 @@ import java.io.IOException;
 
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.util.FS;
+import org.eclipse.jgit.util.FileUtils;
 
 /** Keeps track of a {@link PackFile}'s associated <code>.keep</code> file. */
 public class PackLock {
@@ -90,8 +91,13 @@ public class PackLock {
 		return lf.commit();
 	}
 
-	/** Remove the <code>.keep</code> file that holds this pack in place. */
-	public void unlock() {
-		keepFile.delete();
+	/**
+	 * Remove the <code>.keep</code> file that holds this pack in place.
+	 *
+	 * @throws IOException
+	 *             if deletion of .keep file failed
+	 */
+	public void unlock() throws IOException {
+		FileUtils.delete(keepFile);
 	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectoryRename.java

@@ -53,6 +53,7 @@ import org.eclipse.jgit.lib.RefRename;
 import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.lib.RefUpdate.Result;
 import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.util.FileUtils;
 
 /**
  * Rename any reference stored by {@link RefDirectory}.
@@ -175,7 +176,7 @@ class RefDirectoryRename extends RefRename {
 			try {
 				refdb.delete(tmp);
 			} catch (IOException err) {
-				refdb.fileFor(tmp.getName()).delete();
+				FileUtils.delete(refdb.fileFor(tmp.getName()));
 			}
 			rw.release();
 		}

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

@@ -110,8 +110,12 @@ class FetchProcess {
 		try {
 			executeImp(monitor, result);
 		} finally {
+			try {
 			for (final PackLock lock : packLocks)
 				lock.unlock();
+			} catch (IOException e) {
+				throw new TransportException(e.getMessage(), e);
+			}
 		}
 	}
 

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

@@ -80,6 +80,7 @@ import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.storage.file.PackIndexWriter;
 import org.eclipse.jgit.storage.file.PackLock;
 import org.eclipse.jgit.storage.pack.BinaryDelta;
+import org.eclipse.jgit.util.FileUtils;
 import org.eclipse.jgit.util.NB;
 
 /** Indexes Git pack files for local use. */
@@ -459,9 +460,9 @@ public class IndexPack {
 			}
 		} catch (IOException err) {
 			if (dstPack != null)
-				dstPack.delete();
+				FileUtils.delete(dstPack);
 			if (dstIdx != null)
-				dstIdx.delete();
+				FileUtils.delete(dstIdx);
 			throw err;
 		}
 	}
@@ -1144,8 +1145,8 @@ public class IndexPack {
 			repo.openPack(finalPack, finalIdx);
 		} catch (IOException err) {
 			keep.unlock();
-			finalPack.delete();
-			finalIdx.delete();
+			FileUtils.delete(finalPack);
+			FileUtils.delete(finalIdx);
 			throw err;
 		}
 

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

@@ -669,7 +669,7 @@ public class ReceivePack {
 		}
 	}
 
-	private void unlockPack() {
+	private void unlockPack() throws IOException {
 		if (packLock != null) {
 			packLock.unlock();
 			packLock = null;

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

@@ -87,6 +87,7 @@ import org.eclipse.jgit.storage.file.PackIndex;
 import org.eclipse.jgit.storage.file.PackLock;
 import org.eclipse.jgit.storage.file.UnpackedObject;
 import org.eclipse.jgit.treewalk.TreeWalk;
+import org.eclipse.jgit.util.FileUtils;
 
 /**
  * Generic fetch support for dumb transport protocols.
@@ -540,8 +541,12 @@ class WalkFetchConnection extends BaseFetchConnection {
 				// it failed the index and pack are unusable and we
 				// shouldn't consult them again.
 				//
-				if (pack.tmpIdx != null)
-					pack.tmpIdx.delete();
+				try {
+					if (pack.tmpIdx != null)
+						FileUtils.delete(pack.tmpIdx);
+				} catch (IOException e) {
+					throw new TransportException(e.getMessage(), e);
+				}
 				packItr.remove();
 			}
 
@@ -830,7 +835,7 @@ class WalkFetchConnection extends BaseFetchConnection {
 					fos.close();
 				}
 			} catch (IOException err) {
-				tmpIdx.delete();
+				FileUtils.delete(tmpIdx);
 				throw err;
 			} finally {
 				s.in.close();
@@ -838,14 +843,14 @@ class WalkFetchConnection extends BaseFetchConnection {
 			pm.endTask();
 
 			if (pm.isCancelled()) {
-				tmpIdx.delete();
+				FileUtils.delete(tmpIdx);
 				return;
 			}
 
 			try {
 				index = PackIndex.open(tmpIdx);
 			} catch (IOException e) {
-				tmpIdx.delete();
+				FileUtils.delete(tmpIdx);
 				throw e;
 			}
 		}