Merge pull request #1996 from digit-google/cleandead-preserves-inputs

cleandead: Fix the logic to preserve inputs.

2 files changed, 71 insertions, 1 deletions

diff --git a/src/clean.cc b/src/clean.cc
index 72dee1f..575bf6b 100644
--- a/src/clean.cc
+++ b/src/clean.cc
@@ -129,7 +129,16 @@ int Cleaner::CleanDead(const BuildLog::Entries& entries) {
   PrintHeader();
   for (BuildLog::Entries::const_iterator i = entries.begin(); i != entries.end(); ++i) {
     Node* n = state_->LookupNode(i->first);
-    if (!n || !n->in_edge()) {
+    // Detecting stale outputs works as follows:
+    //
+    // - If it has no Node, it is not in the build graph, or the deps log
+    //   anymore, hence is stale.
+    //
+    // - If it isn't an output or input for any edge, it comes from a stale
+    //   entry in the deps log, but no longer referenced from the build
+    //   graph.
+    //
+    if (!n || (!n->in_edge() && n->out_edges().empty())) {
       Remove(i->first.AsString());
     }
   }
diff --git a/src/clean_test.cc b/src/clean_test.cc
index 1b843a2..e99909c 100644
--- a/src/clean_test.cc
+++ b/src/clean_test.cc
@@ -537,4 +537,65 @@ TEST_F(CleanDeadTest, CleanDead) {
   EXPECT_NE(0, fs_.Stat("out2", &err));
   log2.Close();
 }
+
+TEST_F(CleanDeadTest, CleanDeadPreservesInputs) {
+  State state;
+  ASSERT_NO_FATAL_FAILURE(AssertParse(&state,
+"rule cat\n"
+"  command = cat $in > $out\n"
+"build out1: cat in\n"
+"build out2: cat in\n"
+));
+  // This manifest does not build out1 anymore, but makes
+  // it an implicit input. CleanDead should detect this
+  // and preserve it.
+  ASSERT_NO_FATAL_FAILURE(AssertParse(&state_,
+"build out2: cat in | out1\n"
+));
+  fs_.Create("in", "");
+  fs_.Create("out1", "");
+  fs_.Create("out2", "");
+
+  BuildLog log1;
+  string err;
+  EXPECT_TRUE(log1.OpenForWrite(kTestFilename, *this, &err));
+  ASSERT_EQ("", err);
+  log1.RecordCommand(state.edges_[0], 15, 18);
+  log1.RecordCommand(state.edges_[1], 20, 25);
+  log1.Close();
+
+  BuildLog log2;
+  EXPECT_TRUE(log2.Load(kTestFilename, &err));
+  ASSERT_EQ("", err);
+  ASSERT_EQ(2u, log2.entries().size());
+  ASSERT_TRUE(log2.LookupByOutput("out1"));
+  ASSERT_TRUE(log2.LookupByOutput("out2"));
+
+  // First use the manifest that describe how to build out1.
+  Cleaner cleaner1(&state, config_, &fs_);
+  EXPECT_EQ(0, cleaner1.CleanDead(log2.entries()));
+  EXPECT_EQ(0, cleaner1.cleaned_files_count());
+  EXPECT_EQ(0u, fs_.files_removed_.size());
+  EXPECT_NE(0, fs_.Stat("in", &err));
+  EXPECT_NE(0, fs_.Stat("out1", &err));
+  EXPECT_NE(0, fs_.Stat("out2", &err));
+
+  // Then use the manifest that does not build out1 anymore.
+  Cleaner cleaner2(&state_, config_, &fs_);
+  EXPECT_EQ(0, cleaner2.CleanDead(log2.entries()));
+  EXPECT_EQ(0, cleaner2.cleaned_files_count());
+  EXPECT_EQ(0u, fs_.files_removed_.size());
+  EXPECT_NE(0, fs_.Stat("in", &err));
+  EXPECT_NE(0, fs_.Stat("out1", &err));
+  EXPECT_NE(0, fs_.Stat("out2", &err));
+
+  // Nothing to do now.
+  EXPECT_EQ(0, cleaner2.CleanDead(log2.entries()));
+  EXPECT_EQ(0, cleaner2.cleaned_files_count());
+  EXPECT_EQ(0u, fs_.files_removed_.size());
+  EXPECT_NE(0, fs_.Stat("in", &err));
+  EXPECT_NE(0, fs_.Stat("out1", &err));
+  EXPECT_NE(0, fs_.Stat("out2", &err));
+  log2.Close();
+}
 }  // anonymous namespace