Subninja chdir: clean up logspam

Warnings would be spammed if a subninja (not a chdir) was used:
ninja: warning: Node "%s" not in a child of target %p ""

This fixes ResolveChdir() by first walking the target up the
tree to the next chdir or root node. This prevents the child
being searched for from flying past the target and hitting
the root node unexpectedly.

Change-Id: I1136d8432a2172e432023c48626270f21da51049

3 files changed, 56 insertions, 18 deletions

diff --git a/src/eval_env.cc b/src/eval_env.cc
index 8fc25f5..38517ef 100644
--- a/src/eval_env.cc
+++ b/src/eval_env.cc
@@ -201,17 +201,38 @@ void Scope::resetGlobalPath(const std::string& path) {
 }
 
 std::string Scope::ResolveChdir(Scope* child, std::string path) {
+  // Calculate a relative path from this -> child by walking up the
+  // list of scopes to the root. Any chdir between child and this is
+  // added to the path.
+  //
+  // Step 1: follow this to its parents until a chdir is found. A chdir has
+  // target->parent_.scope == NULL && target->chdirParent_ != NULL:
+  Scope* target = this;
+  while (target->parent_.scope != NULL) {
+    target = target->parent_.scope;
+  }
+
+  // Step 2: follow child to its parents, adding any chdir found.
   Scope* it = child;
-  while (it != NULL && it != this) {
+  // Stop when it == target. The base case where it == NULL is an error.
+  while (it != NULL && it != target) {
     if (!it->chdir().empty())
       // path was relative to 'child'. Now make it relative to 'it'.
       path = it->chdir() + path;
 
+    // Either it->parent_.scope has a non-chdir parent, or...
     Scope* parent = it->parent_.scope;
+    // if parent is still NULL, it->chdirParent_ has a chdir parent.
     parent = (!parent) ? it->chdirParent_ : parent;
+
     if (!parent && it->chdirParent_ == NULL) {
-      Warning("Node \"%s\" not in a child of target %p \"%s\"\n", path.c_str(),
-              this, chdir().c_str());
+      // parent == NULL, and it = parent (below), so end the loop now.
+      //
+      // A Node cannot be a reference inside a sibling subninja chdir, only
+      // children of the current scope. At this point path has all dirs up to
+      // the root.
+      Warning("Node \"%s\" (%p) not in a child of target \"%s\" (%p)\n",
+              path.c_str(), child, target->chdir().c_str(), target);
       break;
     }
     it = parent;
diff --git a/src/eval_env.h b/src/eval_env.h
index 379212a..82e15f7 100644
--- a/src/eval_env.h
+++ b/src/eval_env.h
@@ -232,6 +232,9 @@ struct Scope {
   /// a relative path to where the build product can be found.
   std::string ResolveChdir(Scope* child, std::string path);
 
+  // Used for testing.
+  Scope* parent() const { return parent_.scope; }
+
 private:
   /// The position of this scope within its parent scope.
   /// (ScopePosition::parent will be nullptr for the root scope.)
diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc
index 61eafa7..308cc11 100644
--- a/src/manifest_parser_test.cc
+++ b/src/manifest_parser_test.cc
@@ -944,24 +944,30 @@ TEST_F(ParserTest, SubNinja) {
   ASSERT_EQ(1u, fs_.files_read_.size());
 
   EXPECT_EQ("test.ninja", fs_.files_read_[0]);
-  EXPECT_TRUE(LookupNode("some_dir/outer"));
+  Node* outer = LookupNode("some_dir/outer");
+  EXPECT_TRUE(outer);
   // Verify our builddir setting is inherited.
-  EXPECT_TRUE(LookupNode("some_dir/inner"));
+  Node* inner = LookupNode("some_dir/inner");
+  EXPECT_TRUE(inner);
 
   ASSERT_EQ(3u, state.edges_.size());
   EXPECT_EQ("varref outer", state.edges_[0]->EvaluateCommand());
   EXPECT_EQ("varref inner", state.edges_[1]->EvaluateCommand());
   EXPECT_EQ("varref outer", state.edges_[2]->EvaluateCommand());
+
+  // Verify inner scope links back to parent.
+  ASSERT_TRUE(state.edges_[1]->pos_.scope()->parent());
+  ASSERT_EQ(state.edges_[1]->pos_.scope()->parent(), outer->scope());
 }
 
 TEST_F(ParserTest, SubNinjaChdir) {
-  EXPECT_TRUE(fs_.MakeDir("a"));
-  fs_.Create("a/foo.ninja",
+  EXPECT_TRUE(fs_.MakeDir("b"));
+  fs_.Create("b/foo.ninja",
     "var = inner\n"
     "rule innerrule\n"
-    "  command = foo $var\n"
-    "build $builddir/inner: innerrule\n"
-    "build inner2: innerrule\n");
+    "  command = foo \"$var\" \"$in\" \"$out\"\n"
+    "build $builddir/inner: innerrule abc\n"
+    "build inner2: innerrule def\n");
 
   ASSERT_NO_FATAL_FAILURE(AssertParse(
 "builddir = a/\n"
@@ -969,15 +975,16 @@ TEST_F(ParserTest, SubNinjaChdir) {
 "  command = varref $var\n"
 "var = outer\n"
 "build $builddir/outer: varref\n"
-"subninja a/foo.ninja\n"
-"  chdir = a\n"
+"subninja b/foo.ninja\n"
+"  chdir = b\n"
 "build $builddir/outer2: varref\n"));
   ASSERT_EQ(1u, fs_.files_read_.size());
   ASSERT_EQ("/", VerifyCwd(fs_));
 
-  EXPECT_EQ("a/foo.ninja", fs_.files_read_[0]);
+  EXPECT_EQ("b/foo.ninja", fs_.files_read_[0]);
   HashedStrView outer("a/outer");
-  EXPECT_TRUE(state.LookupNode(GlobalPathStr{outer}));
+  Node* outerNode = state.LookupNode(GlobalPathStr{outer});
+  EXPECT_TRUE(outerNode);
   HashedStrView outer2("a/outer2");
   EXPECT_TRUE(state.LookupNode(GlobalPathStr{outer2}));
   // Verify builddir setting is *not* inherited.
@@ -987,9 +994,9 @@ TEST_F(ParserTest, SubNinjaChdir) {
   // (in this case, the filename is /inner because a/foo.ninja says
   // "build $bulddir/inner" and this validates that builddir from
   // the unnamed top-level .ninja file is not visible in a/foo.ninja)
-  HashedStrView innerslash("a/" "/inner");
+  HashedStrView innerslash("b/" "/inner");
   EXPECT_TRUE(state.LookupNode(GlobalPathStr{innerslash}));
-  HashedStrView inner2("a/inner2");
+  HashedStrView inner2("b/inner2");
   Node* inner2node = state.LookupNode(GlobalPathStr{inner2});
   EXPECT_TRUE(inner2node);
   HashedStrView slashinner("/inner");
@@ -999,10 +1006,17 @@ TEST_F(ParserTest, SubNinjaChdir) {
   Edge* edge = inner2node->in_edge();
   EXPECT_TRUE(edge);
 #ifdef _WIN32
-  EXPECT_EQ(NINJA_WIN32_CD_DELIM "a/" NINJA_WIN32_CD_DELIM "foo inner", edge->EvaluateCommand());
+  EXPECT_EQ(NINJA_WIN32_CD_DELIM "b/" NINJA_WIN32_CD_DELIM
+            "foo \"inner\" \"def\" \"inner2\"", edge->EvaluateCommand());
 #else /* _WIN32 */
-  EXPECT_EQ("cd \"a/\" && foo inner", edge->EvaluateCommand());
+  EXPECT_EQ("cd \"b/\" && foo \"inner\" \"def\" \"inner2\"", edge->EvaluateCommand());
 #endif /* _WIN32 */
+
+  // Verify chdir scope does *not* allow variables from a parent scope to
+  // leak into the chdir.
+  ASSERT_FALSE(state.edges_[1]->pos_.scope()->parent());
+  // Verify that the scope does know it is a chdir.
+  ASSERT_EQ(state.edges_[1]->pos_.scope()->chdir(), "b/");
 }
 
 TEST_F(ParserTest, SubNinjaChdirNoSuchFile) {