From eba7a50456dfbbba8a3b0db7cc94214e7c105783 Mon Sep 17 00:00:00 2001
From: David 'Digit' Turner <digit@google.com>
Date: Wed, 21 Jul 2021 17:38:49 +0200
Subject: cleandead: Fix the logic to preserve inputs.

In the rare case when the build log contains an entry for
a file path that used to be an output in a previous build,
but was moved as an input in the new one, the cleandead tool
would incorrectly consider the input file stale and remove it.

This patch fixes the logic used in Cleaner::CleanDead to not
remove any path that is an input in the build graph.
---
 src/clean.cc      | 11 +++++++++-
 src/clean_test.cc | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletion(-)

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
-- 
cgit v0.12