Return non-zero status when errors occur.

Clean all was not returning non-zero when an error occur (like when
failing to remove a directory). This patch fix the problem and add a test
for it.

9 files changed, 78 insertions, 40 deletions

diff --git a/src/clean.cc b/src/clean.cc
index 5072484..e86793e 100644
--- a/src/clean.cc
+++ b/src/clean.cc
@@ -31,6 +31,7 @@ Cleaner::Cleaner(State* state, const BuildConfig& config)
   , removed_()
   , cleaned_files_count_(0)
   , disk_interface_(new RealDiskInterface)
+  , status_(0)
 {
 }
 
@@ -42,10 +43,11 @@ Cleaner::Cleaner(State* state,
   , removed_()
   , cleaned_files_count_(0)
   , disk_interface_(disk_interface)
+  , status_(0)
 {
 }
 
-bool Cleaner::RemoveFile(const string& path) {
+int Cleaner::RemoveFile(const string& path) {
   return disk_interface_->RemoveFile(path);
 }
 
@@ -66,8 +68,11 @@ void Cleaner::Remove(const string& path) {
       if (FileExists(path))
         Report(path);
     } else {
-      if (RemoveFile(path))
+      int ret = RemoveFile(path);
+      if (ret == 0)
         Report(path);
+      else if (ret == -1)
+        status_ = 1;
     }
   }
 }
@@ -93,7 +98,8 @@ void Cleaner::PrintFooter() {
   printf("%d files.\n", cleaned_files_count_);
 }
 
-void Cleaner::CleanAll() {
+int Cleaner::CleanAll() {
+  Reset();
   PrintHeader();
   for (vector<Edge*>::iterator e = state_->edges_.begin();
        e != state_->edges_.end();
@@ -103,6 +109,7 @@ void Cleaner::CleanAll() {
          ++out_node)
       Remove((*out_node)->file_->path_);
   PrintFooter();
+  return status_;
 }
 
 void Cleaner::DoCleanTarget(Node* target) {
@@ -116,28 +123,32 @@ void Cleaner::DoCleanTarget(Node* target) {
   }
 }
 
-void Cleaner::CleanTarget(Node* target) {
+int Cleaner::CleanTarget(Node* target) {
   assert(target);
 
+  Reset();
   PrintHeader();
   DoCleanTarget(target);
   PrintFooter();
+  return status_;
 }
 
 int Cleaner::CleanTarget(const char* target) {
   assert(target);
+
+  Reset();
   Node* node = state_->LookupNode(target);
   if (node) {
     CleanTarget(node);
-    return 0;
   } else {
     Error("unknown target '%s'", target);
-    return 1;
+    status_ = 1;
   }
+  return status_;
 }
 
 int Cleaner::CleanTargets(int target_count, char* targets[]) {
-  int status = 0;
+  Reset();
   PrintHeader();
   for (int i = 0; i < target_count; ++i) {
     const char* target_name = targets[i];
@@ -148,11 +159,11 @@ int Cleaner::CleanTargets(int target_count, char* targets[]) {
       DoCleanTarget(target);
     } else {
       Error("unknown target '%s'", target_name);
-      status = 1;
+      status_ = 1;
     }
   }
   PrintFooter();
-  return status;
+  return status_;
 }
 
 void Cleaner::DoCleanRule(const Rule* rule) {
@@ -168,31 +179,34 @@ void Cleaner::DoCleanRule(const Rule* rule) {
         Remove((*out_node)->file_->path_);
 }
 
-void Cleaner::CleanRule(const Rule* rule) {
+int Cleaner::CleanRule(const Rule* rule) {
   assert(rule);
 
+  Reset();
   PrintHeader();
   DoCleanRule(rule);
   PrintFooter();
+  return status_;
 }
 
 int Cleaner::CleanRule(const char* rule) {
   assert(rule);
 
+  Reset();
   const Rule* r = state_->LookupRule(rule);
   if (r) {
     CleanRule(r);
-    return 0;
   } else {
     Error("unknown rule '%s'", rule);
-    return 1;
+    status_ = 1;
   }
+  return status_;
 }
 
 int Cleaner::CleanRules(int rule_count, char* rules[]) {
   assert(rules);
 
-  int status = 0;
+  Reset();
   PrintHeader();
   for (int i = 0; i < rule_count; ++i) {
     const char* rule_name = rules[i];
@@ -203,9 +217,15 @@ int Cleaner::CleanRules(int rule_count, char* rules[]) {
       DoCleanRule(rule);
     } else {
       Error("unknown rule '%s'", rule_name);
-      status = 1;
+      status_ = 1;
     }
   }
   PrintFooter();
-  return status;
+  return status_;
+}
+
+void Cleaner::Reset() {
+  status_ = 0;
+  cleaned_files_count_ = 0;
+  removed_.clear();
 }
diff --git a/src/clean.h b/src/clean.h
index 6194aa0..8f0cfa4 100644
--- a/src/clean.h
+++ b/src/clean.h
@@ -40,7 +40,8 @@ public:
           DiskInterface* disk_interface);
 
   /// Clean the given @a target and all the file built for it.
-  void CleanTarget(Node* target);
+  /// @return non-zero if an error occurs.
+  int CleanTarget(Node* target);
   /// Clean the given target @a target.
   /// @return non-zero if an error occurs.
   int CleanTarget(const char* target);
@@ -49,10 +50,12 @@ public:
   int CleanTargets(int target_count, char* targets[]);
 
   /// Clean all built files.
-  void CleanAll();
+  /// @return non-zero if an error occurs.
+  int CleanAll();
 
   /// Clean all the file built with the given rule @a rule.
-  void CleanRule(const Rule* rule);
+  /// @return non-zero if an error occurs.
+  int CleanRule(const Rule* rule);
   /// Clean the file produced by the given @a rule.
   /// @return non-zero if an error occurs.
   int CleanRule(const char* rule);
@@ -74,7 +77,7 @@ public:
 private:
   /// Remove the file @a path.
   /// @return whether the file has been removed.
-  bool RemoveFile(const string& path);
+  int RemoveFile(const string& path);
   /// @returns whether the file @a path exists.
   bool FileExists(const string& path);
   void Report(const string& path);
@@ -87,6 +90,7 @@ private:
   void PrintHeader();
   void PrintFooter();
   void DoCleanRule(const Rule* rule);
+  void Reset();
 
 private:
   State* state_;
@@ -94,6 +98,7 @@ private:
   set<string> removed_;
   int cleaned_files_count_;
   DiskInterface* disk_interface_;
+  int status_;
 };
 
 #endif  // NINJA_CLEAN_H_
diff --git a/src/clean_test.cc b/src/clean_test.cc
index 0e2114e..d16941c 100644
--- a/src/clean_test.cc
+++ b/src/clean_test.cc
@@ -40,7 +40,7 @@ TEST_F(CleanTest, CleanAll) {
     Cleaner cleaner(&state_, config_, &fs_);
 
     ASSERT_EQ(0, cleaner.cleaned_files_count());
-    cleaner.CleanAll();
+    EXPECT_EQ(0, cleaner.CleanAll());
     EXPECT_EQ(4, cleaner.cleaned_files_count());
     EXPECT_EQ(4, fs_.files_removed_.size());
   }
@@ -56,7 +56,7 @@ TEST_F(CleanTest, CleanAll) {
     Cleaner cleaner(&state_, config_, &fs_);
 
     ASSERT_EQ(0, cleaner.cleaned_files_count());
-    cleaner.CleanAll();
+    EXPECT_EQ(0, cleaner.CleanAll());
     EXPECT_EQ(0, cleaner.cleaned_files_count());
     EXPECT_EQ(0, fs_.files_removed_.size());
   }
@@ -78,7 +78,7 @@ TEST_F(CleanTest, CleanAllDryRun) {
     Cleaner cleaner(&state_, config_, &fs_);
 
     ASSERT_EQ(0, cleaner.cleaned_files_count());
-    cleaner.CleanAll();
+    EXPECT_EQ(0, cleaner.CleanAll());
     EXPECT_EQ(4, cleaner.cleaned_files_count());
     EXPECT_EQ(0, fs_.files_removed_.size());
   }
@@ -94,7 +94,7 @@ TEST_F(CleanTest, CleanAllDryRun) {
     Cleaner cleaner(&state_, config_, &fs_);
 
     ASSERT_EQ(0, cleaner.cleaned_files_count());
-    cleaner.CleanAll();
+    EXPECT_EQ(0, cleaner.CleanAll());
     EXPECT_EQ(4, cleaner.cleaned_files_count());
     EXPECT_EQ(0, fs_.files_removed_.size());
   }
@@ -253,3 +253,11 @@ TEST_F(CleanTest, CleanRuleDryRun) {
     EXPECT_EQ(0, fs_.files_removed_.size());
   }
 }
+
+TEST_F(CleanTest, CleanFailure) {
+  ASSERT_NO_FATAL_FAILURE(AssertParse(&state_,
+                                      "build dir: cat src1\n"));
+  fs_.MakeDir("dir");
+  Cleaner cleaner(&state_, config_, &fs_);
+  EXPECT_NE(0, cleaner.CleanAll());
+}
diff --git a/src/ninja.cc b/src/ninja.cc
index fbae091..26b6e5e 100644
--- a/src/ninja.cc
+++ b/src/ninja.cc
@@ -335,8 +335,7 @@ int CmdClean(State* state,
     }
   }
   else {
-    cleaner.CleanAll();
-    return 0;
+    return cleaner.CleanAll();
   }
 }
 
diff --git a/src/ninja.h b/src/ninja.h
index 0f4a029..8927768 100644
--- a/src/ninja.h
+++ b/src/ninja.h
@@ -51,8 +51,10 @@ struct DiskInterface {
   virtual string ReadFile(const string& path, string* err) = 0;
   /// Remove the file named @a path. It behaves like 'rm -f path' so no errors
   /// are reported if it does not exists.
-  /// @returns whether the file has been removed.
-  virtual bool RemoveFile(const string& path) = 0;
+  /// @returns 0 if the file has been removed,
+  ///          1 if the file does not exists, and
+  ///          -1 if an error occurs.
+  virtual int RemoveFile(const string& path) = 0;
 
   /// Create all the parent directories for path; like mkdir -p
   /// `basename path`.
@@ -65,7 +67,7 @@ struct RealDiskInterface : public DiskInterface {
   virtual int Stat(const string& path);
   virtual bool MakeDir(const string& path);
   virtual string ReadFile(const string& path, string* err);
-  virtual bool RemoveFile(const string& path);
+  virtual int RemoveFile(const string& path);
 };
 
 /// Mapping of path -> FileStat.
diff --git a/src/ninja_jumble.cc b/src/ninja_jumble.cc
index 1230546..537f48e 100644
--- a/src/ninja_jumble.cc
+++ b/src/ninja_jumble.cc
@@ -107,17 +107,17 @@ bool RealDiskInterface::MakeDir(const string& path) {
   return true;
 }
 
-bool RealDiskInterface::RemoveFile(const string& path) {
+int RealDiskInterface::RemoveFile(const string& path) {
   if (remove(path.c_str()) < 0) {
     switch (errno) {
       case ENOENT:
-        return false;
+        return 1;
       default:
         Error("remove(%s): %s", path.c_str(), strerror(errno));
-        return false;
+        return -1;
     }
   } else {
-    return true;
+    return 0;
   }
 }
 
diff --git a/src/ninja_test.cc b/src/ninja_test.cc
index e8e7f08..8d53343 100644
--- a/src/ninja_test.cc
+++ b/src/ninja_test.cc
@@ -114,9 +114,9 @@ struct StatTest : public StateTestWithBuiltinRules,
     assert(false);
     return "";
   }
-  virtual bool RemoveFile(const string& path) {
+  virtual int RemoveFile(const string& path) {
     assert(false);
-    return false;
+    return 0;
   }
 
   map<string, time_t> mtimes_;
@@ -262,6 +262,7 @@ TEST_F(DiskInterfaceTest, RemoveFile) {
   string cmd = "touch ";
   cmd += kFileName;
   ASSERT_EQ(0, system(cmd.c_str()));
-  EXPECT_TRUE(disk_.RemoveFile(kFileName));
-  EXPECT_FALSE(disk_.RemoveFile(kFileName));
+  EXPECT_EQ(0, disk_.RemoveFile(kFileName));
+  EXPECT_EQ(1, disk_.RemoveFile(kFileName));
+  EXPECT_EQ(-1, disk_.RemoveFile("does not exist"));
 }
diff --git a/src/test.cc b/src/test.cc
index 4dc2f47..719cec3 100644
--- a/src/test.cc
+++ b/src/test.cc
@@ -59,13 +59,16 @@ string VirtualFileSystem::ReadFile(const string& path, string* err) {
   return "";
 }
 
-bool VirtualFileSystem::RemoveFile(const string& path) {
+int VirtualFileSystem::RemoveFile(const string& path) {
+  if (find(directories_made_.begin(), directories_made_.end(), path)
+      != directories_made_.end())
+    return -1;
   FileMap::iterator i = files_.find(path);
   if (i != files_.end()) {
     files_.erase(i);
     files_removed_.insert(path);
-    return true;
+    return 0;
   } else {
-    return false;
+    return 1;
   }
 }
diff --git a/src/test.h b/src/test.h
index 0af8874..c516508 100644
--- a/src/test.h
+++ b/src/test.h
@@ -45,7 +45,7 @@ struct VirtualFileSystem : public DiskInterface {
   virtual int Stat(const string& path);
   virtual bool MakeDir(const string& path);
   virtual string ReadFile(const string& path, string* err);
-  virtual bool RemoveFile(const string& path);
+  virtual int RemoveFile(const string& path);
 
   /// An entry for a single in-memory file.
   struct Entry {