changes to path iterator behavior

Summary:
This modifies the iterator behavior to so the behavior is a bit cleaner
with respect to empty paths.  It is valid to have an empty relative path,
and there are legitimate use cases where this is useful.  For instance,
calling dirname() on a RelativePath with a single component will result in
an empty path.  It is useful to use this empty path to refer to the parent
directory, to which the path is relative.  Therefore it is also useful to
be able to include the empty path when iterating through the parent
directories of a path.

This removes RelativePath::begin() and RelativePath::end(), and replaces
them with a RelativePath::paths() function.  paths() returns a struct with
a begin() and end() function, so it can be used in range-based for loops,
and has the same behavior that begin()/end() did.  This also adds a
RelativePath::allPaths() function, which also includes the empty relative
path in the results.

Reviewed By: bolinfest

Differential Revision: D3366877

fbshipit-source-id: 3d92b600f07b993925f88d4f1e619b6c1705fb82

eden/fs/overlay/Overlay.cpp

@@ -123,7 +123,7 @@ bool Overlay::isWhiteout(RelativePathPiece path) {
   }
 
   // Iterate the various path combinations in path.
-  for (auto candidatePath : path) {
+  for (auto candidatePath : path.paths()) {
     struct stat st;
 
     auto whiteoutPath = localDir_ + candidatePath.dirname() +

eden/fs/service/EdenServiceHandler.cpp

@@ -130,7 +130,7 @@ void EdenServiceHandler::getSHA1(
   auto inodeDispatcher = edenMount->getMountPoint()->getDispatcher();
   auto parent = inodeDispatcher->getDirInode(FUSE_ROOT_ID);
 
-  auto it = relativePath.begin();
+  auto it = relativePath.paths().begin();
   while (true) {
     shared_ptr<fusell::InodeBase> inodeBase;
     try {
@@ -152,7 +152,7 @@ void EdenServiceHandler::getSHA1(
     auto inodeNumber = inodeBase->getNodeId();
     auto currentPiece = it.piece();
     it++;
-    if (it == relativePath.end()) {
+    if (it == relativePath.paths().end()) {
       // inodeNumber must correspond to the last path component, which we expect
       // to correspond to a file.
       std::shared_ptr<fusell::FileInode> fileInode;

eden/utils/PathFuncs.h

@@ -432,6 +432,27 @@ class ComposedPathIterator
     return tmp;
   }
 
+  ComposedPathIterator& operator--() {
+    if (pos_ == nullptr) {
+      pos_ = path_.end();
+      return *this;
+    }
+
+    CHECK_NE(pos_, path_.begin());
+    --pos_;
+    while (pos_ > path_.begin() && *pos_ != '/') {
+      --pos_;
+    }
+
+    return *this;
+  }
+
+  ComposedPathIterator operator--(int) {
+    ComposedPathIterator tmp(*this);
+    --(*this); // invoke the --iter handler above.
+    return tmp;
+  }
+
   /// Returns the piece for the current iterator position.
   Piece piece() const {
     CHECK_NOTNULL(pos_);
@@ -480,16 +501,11 @@ class RelativePathReverseIterator : public ComposedPathIterator<Piece> {
    */
   RelativePathReverseIterator& operator++() {
     CHECK_NOTNULL(this->pos_);
-    CHECK_NE(this->pos_, this->path_.begin());
-
-    while (this->pos_ != this->path_.begin()) {
-      --this->pos_;
-      if (*this->pos_ == '/') {
-        return *this;
-      }
+    if (this->pos_ == this->path_.begin()) {
+      this->pos_ = nullptr;
+      return *this;
     }
-
-    CHECK_EQ(this->pos_, this->path_.begin()); // terminal position.
+    this->ComposedPathIterator<Piece>::operator--();
     return *this;
   }
 };
@@ -512,23 +528,28 @@ class AbsolutePathReverseIterator : public ComposedPathIterator<Piece> {
    */
   AbsolutePathReverseIterator& operator++() {
     CHECK_NOTNULL(this->pos_);
-    CHECK_NE(this->pos_, this->path_.begin());
-
-    --this->pos_;
-    if (this->pos_ == this->path_.begin()) {
+    // If we were previously pointing to the first character (just the "/"),
+    // we should now advance to the end.
+    if (this->pos_ <= this->path_.begin() + 1) {
       this->pos_ = nullptr;
       return *this;
     }
-
-    while (*this->pos_ != '/') {
-      --this->pos_;
-    }
-    if (this->pos_ == this->path_.begin()) {
-      ++this->pos_;
-    }
-
+    this->ComposedPathIterator<Piece>::operator--();
     return *this;
   }
+
+  Piece piece() const {
+    CHECK_NOTNULL(this->pos_);
+    // When pos_ is at the beginning of the string, return "/" rather than ""
+    if (this->pos_ == this->path_.begin()) {
+      return Piece(folly::StringPiece(this->path_.begin(), 1));
+    }
+    return this->ComposedPathIterator<Piece>::piece();
+  }
+
+  Piece operator*() const {
+    return piece();
+  }
 };
 
 /** Represents any number of PathComponents composed together.
@@ -578,10 +599,36 @@ struct RelativePathSanityCheck {
   }
 };
 
+/** A pair of path iterators.
+ * This is used to implement the paths() and allPaths() methods.
+ */
+template <typename Iterator>
+class PathIteratorRange {
+ public:
+  using iterator = Iterator;
+
+  PathIteratorRange(iterator b, iterator e)
+      : begin_(std::move(b)), end_(std::move(e)) {}
+
+  iterator begin() const {
+    return begin_;
+  }
+  iterator end() const {
+    return end_;
+  }
+
+ private:
+  iterator begin_;
+  iterator end_;
+};
+
 /** Represents any number of PathComponents composed together.
  * It is illegal for a RelativePath to begin with an absolute
  * path prefix (`/` on unix, more complex on windows, but we
- * haven't implemented that yet in any case) */
+ * haven't implemented that yet in any case)
+ *
+ * A RelativePath may be the empty string.
+ */
 template <typename Storage>
 class RelativePathBase : public ComposedPathBase<
                              Storage,
@@ -608,24 +655,60 @@ class RelativePathBase : public ComposedPathBase<
   // For iteration
   using iterator = ComposedPathIterator<RelativePathPiece>;
   using reverse_iterator = RelativePathReverseIterator<RelativePathPiece>;
+  using iterator_range = PathIteratorRange<iterator>;
+  using reverse_iterator_range = PathIteratorRange<reverse_iterator>;
 
-  iterator begin() const {
-    // A RelativePath iteration skips the empty initial element.
-    return ++iterator(this->piece());
+  /** Return an iterator range that will yield all parent directories of this
+   * path, and then the path itself.
+   *
+   * For example, iterating the path "foo/bar/baz" will yield
+   * this series of Piece elements:
+   *
+   * 1. "foo"
+   * 2. "foo/bar"
+   * 3. "foo/bar/baz"
+   */
+  iterator_range paths() const {
+    auto p = this->piece();
+    return iterator_range(++iterator{p}, iterator{p, nullptr});
   }
 
-  iterator end() const {
-    return iterator(this->piece(), nullptr);
+  /** Return an iterator range that will yield all parent directories of this
+   * path, and then the path itself.
+   *
+   * This is very similar to paths(), but also includes the empty string
+   * first, to represent the current directory that this path is relative to.
+   *
+   * For example, iterating the path "foo/bar/baz" will yield
+   * this series of Piece elements:
+   *
+   * 1. ""
+   * 2. "foo"
+   * 3. "foo/bar"
+   * 4. "foo/bar/baz"
+   */
+  iterator_range allPaths() const {
+    auto p = this->piece();
+    return iterator_range(iterator{p}, iterator{p, nullptr});
   }
 
-  reverse_iterator rbegin() const {
-    return reverse_iterator(this->piece(), this->stringPiece().end());
+  /** Return a reverse_iterator over all parent directories and this path.
+   */
+  reverse_iterator_range rpaths() const {
+    auto p = this->piece();
+    return reverse_iterator_range(
+        reverse_iterator{p, this->stringPiece().end()},
+        reverse_iterator{p, this->stringPiece().begin()});
   }
 
-  reverse_iterator rend() const {
-    // A RelativePath reverse iteration skips the final empty element,
-    // so arrange to stop when we hit the front of the string.
-    return reverse_iterator(this->piece(), this->stringPiece().begin());
+  /** Return a reverse_iterator over this path and all parent directories,
+   * including the empty path at the end.
+   */
+  reverse_iterator_range rallPaths() const {
+    auto p = this->piece();
+    return reverse_iterator_range(
+        reverse_iterator{p, this->stringPiece().end()},
+        reverse_iterator{p, nullptr});
   }
 
   /** Construct from an iterable set of PathComponents.
@@ -713,24 +796,23 @@ class AbsolutePathBase : public ComposedPathBase<
   // For iteration
   using iterator = ComposedPathIterator<AbsolutePathPiece>;
   using reverse_iterator = AbsolutePathReverseIterator<AbsolutePathPiece>;
+  using iterator_range = PathIteratorRange<iterator>;
+  using reverse_iterator_range = PathIteratorRange<reverse_iterator>;
 
-  iterator begin() const {
+  iterator_range paths() const {
+    auto p = this->piece();
     // The +1 allows us to deal with the case where we're iterating
     // over literally "/".  Without this +1, we would emit "/" twice
     // and we don't want that.
-    return iterator(this->piece(), this->stringPiece().begin() + 1);
+    return iterator_range(
+        iterator{p, this->stringPiece().begin() + 1}, iterator{p, nullptr});
   }
 
-  iterator end() const {
-    return iterator(this->piece(), nullptr);
-  }
-
-  reverse_iterator rbegin() const {
-    return reverse_iterator(this->piece(), this->stringPiece().end());
-  }
-
-  reverse_iterator rend() const {
-    return reverse_iterator(this->piece(), nullptr);
+  reverse_iterator_range rpaths() const {
+    auto p = this->piece();
+    return reverse_iterator_range(
+        reverse_iterator{p, this->stringPiece().end()},
+        reverse_iterator{p, nullptr});
   }
 
   /** Compose an AbsolutePath with a RelativePath */

eden/utils/PathFuncsTest.cpp

@@ -15,6 +15,8 @@
 using facebook::eden::dirname;
 using facebook::eden::basename;
 using folly::StringPiece;
+using std::string;
+using std::vector;
 using namespace facebook::eden;
 
 TEST(PathFuncs, StringCompare) {
@@ -27,38 +29,68 @@ TEST(PathFuncs, StringCompare) {
 TEST(PathFuncs, Iterate) {
   RelativePath rel("foo/bar/baz");
 
-  std::vector<RelativePathPiece> components(rel.begin(), rel.end());
-  EXPECT_EQ(3, components.size());
-  EXPECT_EQ(RelativePathPiece("foo"), components.at(0));
-  EXPECT_EQ(RelativePathPiece("foo/bar"), components.at(1));
-  EXPECT_EQ(RelativePathPiece("foo/bar/baz"), components.at(2));
+  std::vector<RelativePathPiece> parents(
+      rel.paths().begin(), rel.paths().end());
+  EXPECT_EQ(3, parents.size());
+  EXPECT_EQ(RelativePathPiece("foo"), parents.at(0));
+  EXPECT_EQ(RelativePathPiece("foo/bar"), parents.at(1));
+  EXPECT_EQ(RelativePathPiece("foo/bar/baz"), parents.at(2));
+
+  std::vector<RelativePathPiece> allPaths(
+      rel.allPaths().begin(), rel.allPaths().end());
+  EXPECT_EQ(4, allPaths.size());
+  EXPECT_EQ(RelativePathPiece(""), allPaths.at(0));
+  EXPECT_EQ(RelativePathPiece("foo"), allPaths.at(1));
+  EXPECT_EQ(RelativePathPiece("foo/bar"), allPaths.at(2));
+  EXPECT_EQ(RelativePathPiece("foo/bar/baz"), allPaths.at(3));
 
   // And in reverse.
-  std::vector<RelativePathPiece> rcomponents(rel.rbegin(), rel.rend());
-  EXPECT_EQ(3, rcomponents.size());
-  EXPECT_EQ(RelativePathPiece("foo/bar/baz"), rcomponents.at(0));
-  EXPECT_EQ(RelativePathPiece("foo/bar"), rcomponents.at(1));
-  EXPECT_EQ(RelativePathPiece("foo"), rcomponents.at(2));
+  std::vector<RelativePathPiece> rparents(
+      rel.rpaths().begin(), rel.rpaths().end());
+  EXPECT_EQ(3, rparents.size());
+  EXPECT_EQ(RelativePathPiece("foo/bar/baz"), rparents.at(0));
+  EXPECT_EQ(RelativePathPiece("foo/bar"), rparents.at(1));
+  EXPECT_EQ(RelativePathPiece("foo"), rparents.at(2));
+
+  std::vector<RelativePathPiece> rallPaths(
+      rel.rallPaths().begin(), rel.rallPaths().end());
+  EXPECT_EQ(4, rallPaths.size());
+  EXPECT_EQ(RelativePathPiece("foo/bar/baz"), rallPaths.at(0));
+  EXPECT_EQ(RelativePathPiece("foo/bar"), rallPaths.at(1));
+  EXPECT_EQ(RelativePathPiece("foo"), rallPaths.at(2));
+  EXPECT_EQ(RelativePathPiece(""), rallPaths.at(3));
 
   // An empty relative path yields no elements.
   RelativePath emptyRel;
-  std::vector<RelativePathPiece> emptyPieces(emptyRel.begin(), emptyRel.end());
-  EXPECT_EQ(0, emptyPieces.size());
+  std::vector<RelativePathPiece> emptyPaths(
+      emptyRel.paths().begin(), emptyRel.paths().end());
+  EXPECT_EQ(0, emptyPaths.size());
+
+  std::vector<RelativePathPiece> allEmptyPaths(
+      emptyRel.allPaths().begin(), emptyRel.allPaths().end());
+  EXPECT_EQ(1, allEmptyPaths.size());
+  EXPECT_EQ(RelativePathPiece(""), allEmptyPaths.at(0));
 
   // An empty relative path yields no elements in reverse either.
-  std::vector<RelativePathPiece> remptyPieces(
-      emptyRel.rbegin(), emptyRel.rend());
-  EXPECT_EQ(0, remptyPieces.size());
+  std::vector<RelativePathPiece> remptyPaths(
+      emptyRel.rpaths().begin(), emptyRel.rpaths().end());
+  EXPECT_EQ(0, remptyPaths.size());
+  std::vector<RelativePathPiece> rallEmptyPaths(
+      emptyRel.rallPaths().begin(), emptyRel.rallPaths().end());
+  EXPECT_EQ(1, rallEmptyPaths.size());
+  EXPECT_EQ(RelativePathPiece(""), rallEmptyPaths.at(0));
 
   AbsolutePath absPath("/foo/bar/baz");
-  std::vector<AbsolutePathPiece> acomps(absPath.begin(), absPath.end());
+  std::vector<AbsolutePathPiece> acomps(
+      absPath.paths().begin(), absPath.paths().end());
   EXPECT_EQ(4, acomps.size());
   EXPECT_EQ(AbsolutePathPiece("/"), acomps.at(0));
   EXPECT_EQ(AbsolutePathPiece("/foo"), acomps.at(1));
   EXPECT_EQ(AbsolutePathPiece("/foo/bar"), acomps.at(2));
   EXPECT_EQ(AbsolutePathPiece("/foo/bar/baz"), acomps.at(3));
 
-  std::vector<AbsolutePathPiece> racomps(absPath.rbegin(), absPath.rend());
+  std::vector<AbsolutePathPiece> racomps(
+      absPath.rpaths().begin(), absPath.rpaths().end());
   EXPECT_EQ(4, racomps.size());
   EXPECT_EQ(AbsolutePathPiece("/foo/bar/baz"), racomps.at(0));
   EXPECT_EQ(AbsolutePathPiece("/foo/bar"), racomps.at(1));
@@ -66,11 +98,13 @@ TEST(PathFuncs, Iterate) {
   EXPECT_EQ(AbsolutePathPiece("/"), racomps.at(3));
 
   AbsolutePath slashAbs("/");
-  std::vector<AbsolutePathPiece> slashPieces(slashAbs.begin(), slashAbs.end());
+  std::vector<AbsolutePathPiece> slashPieces(
+      slashAbs.paths().begin(), slashAbs.paths().end());
   EXPECT_EQ(1, slashPieces.size());
   EXPECT_EQ(AbsolutePathPiece("/"), slashPieces.at(0));
 
-  std::vector<AbsolutePathPiece> rslashPieces(slashAbs.begin(), slashAbs.end());
+  std::vector<AbsolutePathPiece> rslashPieces(
+      slashAbs.rpaths().begin(), slashAbs.rpaths().end());
   EXPECT_EQ(1, rslashPieces.size());
   EXPECT_EQ(AbsolutePathPiece("/"), rslashPieces.at(0));
 }