Abort before delete in FileUtils.delete EMPTY_DIRECTORIES_ONLY|RECURSIVE

Depending on the order in which items are traversed for RECURSIVE, an
empty directory may come first before detecting that there is a file and
aborting.

This fixes it by traversing files first.

Bug: 405558
Change-Id: I638b7da58e33ffeb0fee172b96f4c823943d29e9
Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

2 files changed, 131 insertions, 2 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/FileUtilTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/FileUtilTest.java
index 3cd01453a1..b325d4be5e 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/FileUtilTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/FileUtilTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2010, Matthias Sohn <matthias.sohn@sap.com>
+ * Copyright (C) 2010, 2013 Matthias Sohn <matthias.sohn@sap.com>
  * and other copyright owners as documented in the project's IP log.
  *
  * This program and the accompanying materials are made available
@@ -119,6 +119,121 @@ public class FileUtilTest {
 	}
 
 	@Test
+
+	public void testDeleteRecursiveEmpty() throws IOException {
+		File f1 = new File(trash, "test/test/a");
+		File f2 = new File(trash, "test/a");
+		File d1 = new File(trash, "test");
+		File d2 = new File(trash, "test/test");
+		File d3 = new File(trash, "test/b");
+		FileUtils.mkdirs(f1.getParentFile());
+		FileUtils.createNewFile(f2);
+		FileUtils.createNewFile(f1);
+		FileUtils.mkdirs(d3);
+
+		// Cannot delete hierarchy since files exist
+		try {
+			FileUtils.delete(d1, FileUtils.EMPTY_DIRECTORIES_ONLY);
+			fail("delete should fail");
+		} catch (IOException e1) {
+			try {
+				FileUtils.delete(d1, FileUtils.EMPTY_DIRECTORIES_ONLY|FileUtils.RECURSIVE);
+				fail("delete should fail");
+			} catch (IOException e2) {
+				// Everything still there
+				assertTrue(f1.exists());
+				assertTrue(f2.exists());
+				assertTrue(d1.exists());
+				assertTrue(d2.exists());
+				assertTrue(d3.exists());
+			}
+		}
+
+		// setup: delete files, only directories left
+		assertTrue(f1.delete());
+		assertTrue(f2.delete());
+
+		// Shall not delete hierarchy without recursive
+		try {
+			FileUtils.delete(d1, FileUtils.EMPTY_DIRECTORIES_ONLY);
+			fail("delete should fail");
+		} catch (IOException e2) {
+			// Everything still there
+			assertTrue(d1.exists());
+			assertTrue(d2.exists());
+			assertTrue(d3.exists());
+		}
+
+		// Now delete the empty hierarchy
+		FileUtils.delete(d2, FileUtils.EMPTY_DIRECTORIES_ONLY
+				| FileUtils.RECURSIVE);
+		assertFalse(d2.exists());
+
+		// Will fail to delete non-existing without SKIP_MISSING
+		try {
+			FileUtils.delete(d2, FileUtils.EMPTY_DIRECTORIES_ONLY);
+			fail("Cannot delete non-existent entity");
+		} catch (IOException e) {
+			// ok
+		}
+
+		// ..with SKIP_MISSING there is no exception
+		FileUtils.delete(d2, FileUtils.EMPTY_DIRECTORIES_ONLY
+				| FileUtils.SKIP_MISSING);
+		FileUtils.delete(d2, FileUtils.EMPTY_DIRECTORIES_ONLY
+				| FileUtils.RECURSIVE | FileUtils.SKIP_MISSING);
+
+		// essentially the same, using IGNORE_ERRORS
+		FileUtils.delete(d2, FileUtils.EMPTY_DIRECTORIES_ONLY
+				| FileUtils.IGNORE_ERRORS);
+		FileUtils.delete(d2, FileUtils.EMPTY_DIRECTORIES_ONLY
+				| FileUtils.RECURSIVE | FileUtils.IGNORE_ERRORS);
+	}
+
+	@Test
+	public void testDeleteRecursiveEmptyNeedsToCheckFilesFirst()
+			throws IOException {
+		File d1 = new File(trash, "test");
+		File d2 = new File(trash, "test/a");
+		File d3 = new File(trash, "test/b");
+		File f1 = new File(trash, "test/c");
+		File d4 = new File(trash, "test/d");
+		FileUtils.mkdirs(d1);
+		FileUtils.mkdirs(d2);
+		FileUtils.mkdirs(d3);
+		FileUtils.mkdirs(d4);
+		FileUtils.createNewFile(f1);
+
+		// Cannot delete hierarchy since file exists
+		try {
+			FileUtils.delete(d1, FileUtils.EMPTY_DIRECTORIES_ONLY
+					| FileUtils.RECURSIVE);
+			fail("delete should fail");
+		} catch (IOException e) {
+			// Everything still there
+			assertTrue(f1.exists());
+			assertTrue(d1.exists());
+			assertTrue(d2.exists());
+			assertTrue(d3.exists());
+			assertTrue(d4.exists());
+		}
+	}
+
+	@Test
+	public void testDeleteRecursiveEmptyDirectoriesOnlyButIsFile()
+			throws IOException {
+		File f1 = new File(trash, "test/test/a");
+		FileUtils.mkdirs(f1.getParentFile());
+		FileUtils.createNewFile(f1);
+		try {
+			FileUtils.delete(f1, FileUtils.EMPTY_DIRECTORIES_ONLY);
+			fail("delete should fail");
+		} catch (IOException e) {
+			assertTrue(f1.exists());
+		}
+	}
+
+	@Test
 	public void testMkdir() throws IOException {
 		File d = new File(trash, "test");
 		FileUtils.mkdir(d);
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java
index 5fbbda7c50..c4fdd6fff9 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FileUtils.java
@@ -49,6 +49,8 @@ import java.io.File;
 import java.io.IOException;
 import java.nio.channels.FileLock;
 import java.text.MessageFormat;
+import java.util.ArrayList;
+import java.util.List;
 
 import org.eclipse.jgit.internal.JGitText;
 
@@ -130,8 +132,20 @@ public class FileUtils {
 		if ((options & RECURSIVE) != 0 && f.isDirectory()) {
 			final File[] items = f.listFiles();
 			if (items != null) {
+				List<File> files = new ArrayList<File>();
+				List<File> dirs = new ArrayList<File>();
 				for (File c : items)
-					delete(c, options);
+					if (c.isFile())
+						files.add(c);
+					else
+						dirs.add(c);
+				// Try to delete files first, otherwise options
+				// EMPTY_DIRECTORIES_ONLY|RECURSIVE will delete empty
+				// directories before aborting, depending on order.
+				for (File file : files)
+					delete(file, options);
+				for (File d : dirs)
+					delete(d, options);
 			}
 		}