make all GetNode explicit, add DepsLog canonicalize test

11 files changed, 135 insertions, 67 deletions

diff --git a/src/build.cc b/src/build.cc
index a346028..3e74131 100644
--- a/src/build.cc
+++ b/src/build.cc
@@ -825,7 +825,11 @@ bool Builder::ExtractDeps(CommandRunner::Result* result,
     result->output = parser.Parse(result->output, deps_prefix);
     for (set<string>::iterator i = parser.includes_.begin();
          i != parser.includes_.end(); ++i) {
-      deps_nodes->push_back(state_->GetNode(*i));
+      // ~0 is assuming that with MSVC-parsed headers, it's ok to always make
+      // all backslashes (as some of the slashes will certainly be backslashes
+      // anyway). This could be fixed if necessary with some additional
+      // complexity in IncludesNormalize::Relativize.
+      deps_nodes->push_back(state_->GetNode(*i, ~0u));
     }
   } else
 #endif
diff --git a/src/build_test.cc b/src/build_test.cc
index 912e3ef..bd1cd30 100644
--- a/src/build_test.cc
+++ b/src/build_test.cc
@@ -1883,7 +1883,7 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) {
 
     Edge* edge = state.edges_.back();
 
-    state.GetNode("bar.h")->MarkDirty();  // Mark bar.h as missing.
+    state.GetNode("bar.h", 0)->MarkDirty();  // Mark bar.h as missing.
     EXPECT_TRUE(builder.AddTarget("fo o.o", &err));
     ASSERT_EQ("", err);
 
@@ -1901,6 +1901,72 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) {
   }
 }
 
+#ifdef _WIN32
+TEST_F(BuildWithDepsLogTest, DepFileDepsLogCanonicalize) {
+  string err;
+  const char* manifest =
+      "rule cc\n  command = cc $in\n  depfile = $out.d\n  deps = gcc\n"
+      "build a/b\\c\\d/e/fo$ o.o: cc x\\y/z\\foo.c\n";
+
+  fs_.Create("x/y/z/foo.c", "");
+
+  {
+    State state;
+    ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
+
+    // Run the build once, everything should be ok.
+    DepsLog deps_log;
+    ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
+    ASSERT_EQ("", err);
+
+    Builder builder(&state, config_, NULL, &deps_log, &fs_);
+    builder.command_runner_.reset(&command_runner_);
+    EXPECT_TRUE(builder.AddTarget("a/b/c/d/e/fo o.o", &err));
+    ASSERT_EQ("", err);
+    // Note, different slashes from manifest.
+    fs_.Create("a/b\\c\\d/e/fo o.o.d",
+               "a\\b\\c\\d\\e\\fo\\ o.o: blah.h bar.h\n");
+    EXPECT_TRUE(builder.Build(&err));
+    EXPECT_EQ("", err);
+
+    deps_log.Close();
+    builder.command_runner_.release();
+  }
+
+  {
+    State state;
+    ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest));
+
+    DepsLog deps_log;
+    ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err));
+    ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err));
+    ASSERT_EQ("", err);
+
+    Builder builder(&state, config_, NULL, &deps_log, &fs_);
+    builder.command_runner_.reset(&command_runner_);
+
+    Edge* edge = state.edges_.back();
+
+    state.GetNode("bar.h", 0)->MarkDirty();  // Mark bar.h as missing.
+    EXPECT_TRUE(builder.AddTarget("a/b/c/d/e/fo o.o", &err));
+    ASSERT_EQ("", err);
+
+    // Expect three new edges: one generating fo o.o, and two more from
+    // loading the depfile.
+    ASSERT_EQ(3u, state.edges_.size());
+    // Expect our edge to now have three inputs: foo.c and two headers.
+    ASSERT_EQ(3u, edge->inputs_.size());
+
+    // Expect the command line we generate to only use the original input.
+    // Note, slashes from manifest, not .d.
+    ASSERT_EQ("cc x\\y/z\\foo.c", edge->EvaluateCommand());
+
+    deps_log.Close();
+    builder.command_runner_.release();
+  }
+}
+#endif
+
 /// Check that a restat rule doesn't clear an edge if the depfile is missing.
 /// Follows from: https://github.com/martine/ninja/issues/603
 TEST_F(BuildTest, RestatMissingDepfile) {
diff --git a/src/deps_log.cc b/src/deps_log.cc
index 39de180..d889dfd 100644
--- a/src/deps_log.cc
+++ b/src/deps_log.cc
@@ -233,14 +233,20 @@ bool DepsLog::Load(const string& path, State* state, string* err) {
       if (!UpdateDeps(out_id, deps))
         ++unique_dep_record_count;
     } else {
-      int path_size = size - 4;
+      size_t path_size = size - 4;
       assert(path_size > 0);  // CanonicalizePath() rejects empty paths.
       // There can be up to 3 bytes of padding.
       if (buf[path_size - 1] == '\0') --path_size;
       if (buf[path_size - 1] == '\0') --path_size;
       if (buf[path_size - 1] == '\0') --path_size;
+      unsigned int slash_bits;
+      string err;
+      if (!CanonicalizePath(&buf[0], &path_size, &slash_bits, &err)) {
+        read_failed = true;
+        break;
+      }
       StringPiece path(buf, path_size);
-      Node* node = state->GetNode(path);
+      Node* node = state->GetNode(path, slash_bits);
 
       // Check that the expected index matches the actual index. This can only
       // happen if two ninja processes write to the same deps log concurrently.
diff --git a/src/deps_log_test.cc b/src/deps_log_test.cc
index 60ff48f..ac2b315 100644
--- a/src/deps_log_test.cc
+++ b/src/deps_log_test.cc
@@ -46,16 +46,16 @@ TEST_F(DepsLogTest, WriteRead) {
 
   {
     vector<Node*> deps;
-    deps.push_back(state1.GetNode("foo.h"));
-    deps.push_back(state1.GetNode("bar.h"));
-    log1.RecordDeps(state1.GetNode("out.o"), 1, deps);
+    deps.push_back(state1.GetNode("foo.h", 0));
+    deps.push_back(state1.GetNode("bar.h", 0));
+    log1.RecordDeps(state1.GetNode("out.o", 0), 1, deps);
 
     deps.clear();
-    deps.push_back(state1.GetNode("foo.h"));
-    deps.push_back(state1.GetNode("bar2.h"));
-    log1.RecordDeps(state1.GetNode("out2.o"), 2, deps);
+    deps.push_back(state1.GetNode("foo.h", 0));
+    deps.push_back(state1.GetNode("bar2.h", 0));
+    log1.RecordDeps(state1.GetNode("out2.o", 0), 2, deps);
 
-    DepsLog::Deps* log_deps = log1.GetDeps(state1.GetNode("out.o"));
+    DepsLog::Deps* log_deps = log1.GetDeps(state1.GetNode("out.o", 0));
     ASSERT_TRUE(log_deps);
     ASSERT_EQ(1, log_deps->mtime);
     ASSERT_EQ(2, log_deps->node_count);
@@ -79,7 +79,7 @@ TEST_F(DepsLogTest, WriteRead) {
   }
 
   // Spot-check the entries in log2.
-  DepsLog::Deps* log_deps = log2.GetDeps(state2.GetNode("out2.o"));
+  DepsLog::Deps* log_deps = log2.GetDeps(state2.GetNode("out2.o", 0));
   ASSERT_TRUE(log_deps);
   ASSERT_EQ(2, log_deps->mtime);
   ASSERT_EQ(2, log_deps->node_count);
@@ -101,11 +101,11 @@ TEST_F(DepsLogTest, LotsOfDeps) {
     for (int i = 0; i < kNumDeps; ++i) {
       char buf[32];
       sprintf(buf, "file%d.h", i);
-      deps.push_back(state1.GetNode(buf));
+      deps.push_back(state1.GetNode(buf, 0));
     }
-    log1.RecordDeps(state1.GetNode("out.o"), 1, deps);
+    log1.RecordDeps(state1.GetNode("out.o", 0), 1, deps);
 
-    DepsLog::Deps* log_deps = log1.GetDeps(state1.GetNode("out.o"));
+    DepsLog::Deps* log_deps = log1.GetDeps(state1.GetNode("out.o", 0));
     ASSERT_EQ(kNumDeps, log_deps->node_count);
   }
 
@@ -116,7 +116,7 @@ TEST_F(DepsLogTest, LotsOfDeps) {
   EXPECT_TRUE(log2.Load(kTestFilename, &state2, &err));
   ASSERT_EQ("", err);
 
-  DepsLog::Deps* log_deps = log2.GetDeps(state2.GetNode("out.o"));
+  DepsLog::Deps* log_deps = log2.GetDeps(state2.GetNode("out.o", 0));
   ASSERT_EQ(kNumDeps, log_deps->node_count);
 }
 
@@ -132,9 +132,9 @@ TEST_F(DepsLogTest, DoubleEntry) {
     ASSERT_EQ("", err);
 
     vector<Node*> deps;
-    deps.push_back(state.GetNode("foo.h"));
-    deps.push_back(state.GetNode("bar.h"));
-    log.RecordDeps(state.GetNode("out.o"), 1, deps);
+    deps.push_back(state.GetNode("foo.h", 0));
+    deps.push_back(state.GetNode("bar.h", 0));
+    log.RecordDeps(state.GetNode("out.o", 0), 1, deps);
     log.Close();
 
     struct stat st;
@@ -154,9 +154,9 @@ TEST_F(DepsLogTest, DoubleEntry) {
     ASSERT_EQ("", err);
 
     vector<Node*> deps;
-    deps.push_back(state.GetNode("foo.h"));
-    deps.push_back(state.GetNode("bar.h"));
-    log.RecordDeps(state.GetNode("out.o"), 1, deps);
+    deps.push_back(state.GetNode("foo.h", 0));
+    deps.push_back(state.GetNode("bar.h", 0));
+    log.RecordDeps(state.GetNode("out.o", 0), 1, deps);
     log.Close();
 
     struct stat st;
@@ -186,14 +186,14 @@ TEST_F(DepsLogTest, Recompact) {
     ASSERT_EQ("", err);
 
     vector<Node*> deps;
-    deps.push_back(state.GetNode("foo.h"));
-    deps.push_back(state.GetNode("bar.h"));
-    log.RecordDeps(state.GetNode("out.o"), 1, deps);
+    deps.push_back(state.GetNode("foo.h", 0));
+    deps.push_back(state.GetNode("bar.h", 0));
+    log.RecordDeps(state.GetNode("out.o", 0), 1, deps);
 
     deps.clear();
-    deps.push_back(state.GetNode("foo.h"));
-    deps.push_back(state.GetNode("baz.h"));
-    log.RecordDeps(state.GetNode("other_out.o"), 1, deps);
+    deps.push_back(state.GetNode("foo.h", 0));
+    deps.push_back(state.GetNode("baz.h", 0));
+    log.RecordDeps(state.GetNode("other_out.o", 0), 1, deps);
 
     log.Close();
 
@@ -216,8 +216,8 @@ TEST_F(DepsLogTest, Recompact) {
     ASSERT_EQ("", err);
 
     vector<Node*> deps;
-    deps.push_back(state.GetNode("foo.h"));
-    log.RecordDeps(state.GetNode("out.o"), 1, deps);
+    deps.push_back(state.GetNode("foo.h", 0));
+    log.RecordDeps(state.GetNode("out.o", 0), 1, deps);
     log.Close();
 
     struct stat st;
@@ -237,14 +237,14 @@ TEST_F(DepsLogTest, Recompact) {
     string err;
     ASSERT_TRUE(log.Load(kTestFilename, &state, &err));
 
-    Node* out = state.GetNode("out.o");
+    Node* out = state.GetNode("out.o", 0);
     DepsLog::Deps* deps = log.GetDeps(out);
     ASSERT_TRUE(deps);
     ASSERT_EQ(1, deps->mtime);
     ASSERT_EQ(1, deps->node_count);
     ASSERT_EQ("foo.h", deps->nodes[0]->path());
 
-    Node* other_out = state.GetNode("other_out.o");
+    Node* other_out = state.GetNode("other_out.o", 0);
     deps = log.GetDeps(other_out);
     ASSERT_TRUE(deps);
     ASSERT_EQ(1, deps->mtime);
@@ -287,14 +287,14 @@ TEST_F(DepsLogTest, Recompact) {
     string err;
     ASSERT_TRUE(log.Load(kTestFilename, &state, &err));
 
-    Node* out = state.GetNode("out.o");
+    Node* out = state.GetNode("out.o", 0);
     DepsLog::Deps* deps = log.GetDeps(out);
     ASSERT_TRUE(deps);
     ASSERT_EQ(1, deps->mtime);
     ASSERT_EQ(1, deps->node_count);
     ASSERT_EQ("foo.h", deps->nodes[0]->path());
 
-    Node* other_out = state.GetNode("other_out.o");
+    Node* other_out = state.GetNode("other_out.o", 0);
     deps = log.GetDeps(other_out);
     ASSERT_TRUE(deps);
     ASSERT_EQ(1, deps->mtime);
@@ -361,14 +361,14 @@ TEST_F(DepsLogTest, Truncated) {
     ASSERT_EQ("", err);
 
     vector<Node*> deps;
-    deps.push_back(state.GetNode("foo.h"));
-    deps.push_back(state.GetNode("bar.h"));
-    log.RecordDeps(state.GetNode("out.o"), 1, deps);
+    deps.push_back(state.GetNode("foo.h", 0));
+    deps.push_back(state.GetNode("bar.h", 0));
+    log.RecordDeps(state.GetNode("out.o", 0), 1, deps);
 
     deps.clear();
-    deps.push_back(state.GetNode("foo.h"));
-    deps.push_back(state.GetNode("bar2.h"));
-    log.RecordDeps(state.GetNode("out2.o"), 2, deps);
+    deps.push_back(state.GetNode("foo.h", 0));
+    deps.push_back(state.GetNode("bar2.h", 0));
+    log.RecordDeps(state.GetNode("out2.o", 0), 2, deps);
 
     log.Close();
   }
@@ -420,14 +420,14 @@ TEST_F(DepsLogTest, TruncatedRecovery) {
     ASSERT_EQ("", err);
 
     vector<Node*> deps;
-    deps.push_back(state.GetNode("foo.h"));
-    deps.push_back(state.GetNode("bar.h"));
-    log.RecordDeps(state.GetNode("out.o"), 1, deps);
+    deps.push_back(state.GetNode("foo.h", 0));
+    deps.push_back(state.GetNode("bar.h", 0));
+    log.RecordDeps(state.GetNode("out.o", 0), 1, deps);
 
     deps.clear();
-    deps.push_back(state.GetNode("foo.h"));
-    deps.push_back(state.GetNode("bar2.h"));
-    log.RecordDeps(state.GetNode("out2.o"), 2, deps);
+    deps.push_back(state.GetNode("foo.h", 0));
+    deps.push_back(state.GetNode("bar2.h", 0));
+    log.RecordDeps(state.GetNode("out2.o", 0), 2, deps);
 
     log.Close();
   }
@@ -448,16 +448,16 @@ TEST_F(DepsLogTest, TruncatedRecovery) {
     err.clear();
 
     // The truncated entry should've been discarded.
-    EXPECT_EQ(NULL, log.GetDeps(state.GetNode("out2.o")));
+    EXPECT_EQ(NULL, log.GetDeps(state.GetNode("out2.o", 0)));
 
     EXPECT_TRUE(log.OpenForWrite(kTestFilename, &err));
     ASSERT_EQ("", err);
 
     // Add a new entry.
     vector<Node*> deps;
-    deps.push_back(state.GetNode("foo.h"));
-    deps.push_back(state.GetNode("bar2.h"));
-    log.RecordDeps(state.GetNode("out2.o"), 3, deps);
+    deps.push_back(state.GetNode("foo.h", 0));
+    deps.push_back(state.GetNode("bar2.h", 0));
+    log.RecordDeps(state.GetNode("out2.o", 0), 3, deps);
 
     log.Close();
   }
@@ -471,7 +471,7 @@ TEST_F(DepsLogTest, TruncatedRecovery) {
     EXPECT_TRUE(log.Load(kTestFilename, &state, &err));
 
     // The truncated entry should exist.
-    DepsLog::Deps* deps = log.GetDeps(state.GetNode("out2.o"));
+    DepsLog::Deps* deps = log.GetDeps(state.GetNode("out2.o", 0));
     ASSERT_TRUE(deps);
   }
 }
diff --git a/src/graph.cc b/src/graph.cc
index f85a1f6..930c626 100644
--- a/src/graph.cc
+++ b/src/graph.cc
@@ -331,7 +331,7 @@ bool Edge::use_console() const {
 string Node::PathDecanonicalized() const {
   string result = path_;
   unsigned int mask = 1;
-  for (char* c = &result[0]; (c = strpbrk(c, "/\\")) != NULL;) {
+  for (char* c = &result[0]; (c = strchr(c, '/')) != NULL;) {
     if (slash_bits_ & mask)
       *c = '\\';
     c++;
diff --git a/src/includes_normalize.h b/src/includes_normalize.h
index 43527af..634fef3 100644
--- a/src/includes_normalize.h
+++ b/src/includes_normalize.h
@@ -29,7 +29,6 @@ struct IncludesNormalize {
   static string Relativize(StringPiece path, const string& start);
 
   /// Normalize by fixing slashes style, fixing redundant .. and . and makes the
-  /// path relative to |relative_to|. Case is normalized to lowercase on
-  /// Windows too.
+  /// path relative to |relative_to|.
   static string Normalize(const string& input, const char* relative_to);
 };
diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc
index 0668668..6909ea9 100644
--- a/src/manifest_parser_test.cc
+++ b/src/manifest_parser_test.cc
@@ -96,7 +96,7 @@ TEST_F(ParserTest, IgnoreIndentedComments) {
   ASSERT_EQ(2u, state.rules_.size());
   const Rule* rule = state.rules_.begin()->second;
   EXPECT_EQ("cat", rule->name());
-  Edge* edge = state.GetNode("result")->in_edge();
+  Edge* edge = state.GetNode("result", 0)->in_edge();
   EXPECT_TRUE(edge->GetBindingBool("restat"));
   EXPECT_FALSE(edge->GetBindingBool("generator"));
 }
diff --git a/src/state.cc b/src/state.cc
index 7d1d79d..6e3e10d 100644
--- a/src/state.cc
+++ b/src/state.cc
@@ -111,13 +111,6 @@ Edge* State::AddEdge(const Rule* rule) {
   return edge;
 }
 
-Node* State::GetNode(StringPiece path) {
-#if defined(_WIN32) && !defined(NDEBUG)
-  assert(strpbrk(path.AsString().c_str(), "/\\") == NULL);
-#endif
-  return GetNode(path, 0);
-}
-
 Node* State::GetNode(StringPiece path, unsigned int slash_bits) {
   Node* node = LookupNode(path);
   if (node)
diff --git a/src/state.h b/src/state.h
index 9da6d1b..8804532 100644
--- a/src/state.h
+++ b/src/state.h
@@ -95,7 +95,6 @@ struct State {
 
   Edge* AddEdge(const Rule* rule);
 
-  Node* GetNode(StringPiece path);
   Node* GetNode(StringPiece path, unsigned int slash_bits);
   Node* LookupNode(StringPiece path) const;
   Node* SpellcheckNode(const string& path);
diff --git a/src/state_test.cc b/src/state_test.cc
index 85c6513..bd133b6 100644
--- a/src/state_test.cc
+++ b/src/state_test.cc
@@ -38,9 +38,9 @@ TEST(State, Basic) {
 
   EXPECT_EQ("cat in1 in2 > out", edge->EvaluateCommand());
 
-  EXPECT_FALSE(state.GetNode("in1")->dirty());
-  EXPECT_FALSE(state.GetNode("in2")->dirty());
-  EXPECT_FALSE(state.GetNode("out")->dirty());
+  EXPECT_FALSE(state.GetNode("in1", 0)->dirty());
+  EXPECT_FALSE(state.GetNode("in2", 0)->dirty());
+  EXPECT_FALSE(state.GetNode("out", 0)->dirty());
 }
 
 }  // namespace
diff --git a/src/test.cc b/src/test.cc
index 560ef3a..f667fef 100644
--- a/src/test.cc
+++ b/src/test.cc
@@ -89,7 +89,8 @@ void StateTestWithBuiltinRules::AddCatRule(State* state) {
 }
 
 Node* StateTestWithBuiltinRules::GetNode(const string& path) {
-  return state_.GetNode(path);
+  EXPECT_FALSE(strpbrk(path.c_str(), "/\\"));
+  return state_.GetNode(path, 0);
 }
 
 void AssertParse(State* state, const char* input) {