Merge pull request #2025 from mischajonker/mjonker_828

Fix ninja -t clean for directories on Windows

2 files changed, 32 insertions, 7 deletions

diff --git a/src/disk_interface.cc b/src/disk_interface.cc
index a9497cb..a37c570 100644
--- a/src/disk_interface.cc
+++ b/src/disk_interface.cc
@@ -279,14 +279,31 @@ int RealDiskInterface::RemoveFile(const string& path) {
     // Skip error checking.  If this fails, accept whatever happens below.
     SetFileAttributes(path.c_str(), attributes & ~FILE_ATTRIBUTE_READONLY);
   }
-  if (!DeleteFile(path.c_str())) {
-    DWORD win_err = GetLastError();
-    if (win_err == ERROR_FILE_NOT_FOUND || win_err == ERROR_PATH_NOT_FOUND) {
-      return 1;
+  if (attributes & FILE_ATTRIBUTE_DIRECTORY) {
+    // remove() deletes both files and directories. On Windows we have to 
+    // select the correct function (DeleteFile will yield Permission Denied when
+    // used on a directory)
+    // This fixes the behavior of ninja -t clean in some cases
+    // https://github.com/ninja-build/ninja/issues/828
+    if (!RemoveDirectory(path.c_str())) {
+      DWORD win_err = GetLastError();
+      if (win_err == ERROR_FILE_NOT_FOUND || win_err == ERROR_PATH_NOT_FOUND) {
+        return 1;
+      }
+      // Report remove(), not RemoveDirectory(), for cross-platform consistency.
+      Error("remove(%s): %s", path.c_str(), GetLastErrorString().c_str());
+      return -1;
+    }
+  } else {
+    if (!DeleteFile(path.c_str())) {
+      DWORD win_err = GetLastError();
+      if (win_err == ERROR_FILE_NOT_FOUND || win_err == ERROR_PATH_NOT_FOUND) {
+        return 1;
+      }
+      // Report as remove(), not DeleteFile(), for cross-platform consistency.
+      Error("remove(%s): %s", path.c_str(), GetLastErrorString().c_str());
+      return -1;
     }
-    // Report as remove(), not DeleteFile(), for cross-platform consistency.
-    Error("remove(%s): %s", path.c_str(), GetLastErrorString().c_str());
-    return -1;
   }
 #else
   if (remove(path.c_str()) < 0) {
diff --git a/src/disk_interface_test.cc b/src/disk_interface_test.cc
index b424243..339aea1 100644
--- a/src/disk_interface_test.cc
+++ b/src/disk_interface_test.cc
@@ -219,6 +219,14 @@ TEST_F(DiskInterfaceTest, RemoveFile) {
 #endif
 }
 
+TEST_F(DiskInterfaceTest, RemoveDirectory) {
+  const char* kDirectoryName = "directory-to-remove";
+  EXPECT_TRUE(disk_.MakeDir(kDirectoryName));
+  EXPECT_EQ(0, disk_.RemoveFile(kDirectoryName));
+  EXPECT_EQ(1, disk_.RemoveFile(kDirectoryName));
+  EXPECT_EQ(1, disk_.RemoveFile("does not exist"));
+}
+
 struct StatTest : public StateTestWithBuiltinRules,
                   public DiskInterface {
   StatTest() : scan_(&state_, NULL, NULL, this, NULL) {}