walkdir: fix root symlink bug

This commit fixes a nasty bug where the root path given to walkdir was
always reported as a symlink, even when 'follow_links' was enabled. This
appears to be a regression introduced by commit 6f72fce as part of
fixing BurntSushi/ripgrep#984.

The central problem was that since root paths should always be followed,
we were creating a DirEntry whose internal file type was always resolved
by following a symlink, but whose 'metadata' method still returned the
metadata of the symlink and not the target. This was problematic and
inconsistent both with and without 'follow_links' enabled.

We also fix the documentation. In particular, we make the docs of 'new'
more unambiguous, where it previously could have been interpreted as
contradictory to the docs on 'DirEntry'. Specifically, 'WalkDir::new'
says:

    If root is a symlink, then it is always followed.

But the docs for 'DirEntry::metadata' say

    This always calls std::fs::symlink_metadata.

    If this entry is a symbolic link and follow_links is enabled, then
    std::fs::metadata is called instead.

Similarly, 'DirEntry::file_type' said

    If this is a symbolic link and follow_links is true, then this
    returns the type of the target.

That is, if 'root' is a symlink and 'follow_links' is NOT enabled,
then the previous incorrect behavior resulted in 'DirEntry::file_type'
behaving as if 'follow_links' was enabled. If 'follow_links'
was enabled, then the previous incorrect behavior resulted in
'DirEntry::metadata' reporting the metadata of the symlink itself.

We fix this by correctly constructing the DirEntry in the first place,
and then adding special case logic to path traversal that will always
attempt to follow the root path if it's a symlink and 'follow_links'
was not enabled. We also tweak the docs on 'WalkDir::new' to be more
precise.

Fixes #115

2 files changed, 99 insertions, 17 deletions

diff --git a/src/lib.rs b/src/lib.rs
index 0d04a5d..1c38f73 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -277,7 +277,9 @@ impl WalkDir {
     /// file path `root`. If `root` is a directory, then it is the first item
     /// yielded by the iterator. If `root` is a file, then it is the first
     /// and only item yielded by the iterator. If `root` is a symlink, then it
-    /// is always followed.
+    /// is always followed for the purposes of directory traversal. (A root
+    /// `DirEntry` still obeys its documentation with respect to symlinks and
+    /// the `follow_links` setting.)
     pub fn new<P: AsRef<Path>>(root: P) -> Self {
         WalkDir {
             opts: WalkDirOptions {
@@ -681,7 +683,7 @@ impl Iterator for IntoIter {
                     .map_err(|e| Error::from_path(0, start.clone(), e));
                 self.root_device = Some(itry!(result));
             }
-            let dent = itry!(DirEntry::from_path(0, start, false));
+            let mut dent = itry!(DirEntry::from_path(0, start, false));
             if let Some(result) = self.handle_entry(dent) {
                 return Some(result);
             }
@@ -844,6 +846,20 @@ impl IntoIter {
             } else {
                 itry!(self.push(&dent));
             }
+        } else if dent.depth() == 0 && dent.file_type().is_symlink() {
+            // As a special case, if we are processing a root entry, then we
+            // always follow it even if it's a symlink and follow_links is
+            // false. We are careful to not let this change the semantics of
+            // the DirEntry however. Namely, the DirEntry should still respect
+            // the follow_links setting. When it's disabled, it should report
+            // itself as a symlink. When it's enabled, it should always report
+            // itself as the target.
+            let md = itry!(fs::metadata(dent.path()).map_err(|err| {
+                Error::from_path(dent.depth(), dent.path().to_path_buf(), err)
+            }));
+            if md.file_type().is_dir() {
+                itry!(self.push(&dent));
+            }
         }
         if is_normal_dir && self.opts.contents_first {
             self.deferred_dirs.push(dent);
@@ -1181,44 +1197,65 @@ impl DirEntry {
     }
 
     #[cfg(windows)]
-    fn from_path(depth: usize, pb: PathBuf, link: bool) -> Result<DirEntry> {
-        let md = fs::metadata(&pb).map_err(|err| {
-            Error::from_path(depth, pb.clone(), err)
-        })?;
+    fn from_path(depth: usize, pb: PathBuf, follow: bool) -> Result<DirEntry> {
+        let md =
+            if follow {
+                fs::metadata(&pb).map_err(|err| {
+                    Error::from_path(depth, pb.clone(), err)
+                })?
+            } else {
+                fs::symlink_metadata(&pb).map_err(|err| {
+                    Error::from_path(depth, pb.clone(), err)
+                })?
+            };
         Ok(DirEntry {
             path: pb,
             ty: md.file_type(),
-            follow_link: link,
+            follow_link: follow,
             depth: depth,
             metadata: md,
         })
     }
 
     #[cfg(unix)]
-    fn from_path(depth: usize, pb: PathBuf, link: bool) -> Result<DirEntry> {
+    fn from_path(depth: usize, pb: PathBuf, follow: bool) -> Result<DirEntry> {
         use std::os::unix::fs::MetadataExt;
 
-        let md = fs::metadata(&pb).map_err(|err| {
-            Error::from_path(depth, pb.clone(), err)
-        })?;
+        let md =
+            if follow {
+                fs::metadata(&pb).map_err(|err| {
+                    Error::from_path(depth, pb.clone(), err)
+                })?
+            } else {
+                fs::symlink_metadata(&pb).map_err(|err| {
+                    Error::from_path(depth, pb.clone(), err)
+                })?
+            };
         Ok(DirEntry {
             path: pb,
             ty: md.file_type(),
-            follow_link: link,
+            follow_link: follow,
             depth: depth,
             ino: md.ino(),
         })
     }
 
     #[cfg(not(any(unix, windows)))]
-    fn from_path(depth: usize, pb: PathBuf, link: bool) -> Result<DirEntry> {
-        let md = fs::metadata(&pb).map_err(|err| {
-            Error::from_path(depth, pb.clone(), err)
-        })?;
+    fn from_path(depth: usize, pb: PathBuf, follow: bool) -> Result<DirEntry> {
+        let md =
+            if follow {
+                fs::metadata(&pb).map_err(|err| {
+                    Error::from_path(depth, pb.clone(), err)
+                })?
+            } else {
+                fs::symlink_metadata(&pb).map_err(|err| {
+                    Error::from_path(depth, pb.clone(), err)
+                })?
+            };
         Ok(DirEntry {
             path: pb,
             ty: md.file_type(),
-            follow_link: link,
+            follow_link: follow,
             depth: depth,
         })
     }
diff --git a/src/tests.rs b/src/tests.rs
index 1fb7682..ddce795 100644
--- a/src/tests.rs
+++ b/src/tests.rs
@@ -531,6 +531,51 @@ fn first_path_not_symlink() {
     assert!(!dents[0].path_is_symlink());
 }
 
+// Like first_path_not_symlink, but checks that the first path is not reported
+// as a symlink even when we are supposed to be following them.
+#[test]
+#[cfg(unix)]
+fn first_path_not_symlink_follow() {
+    let exp = td("foo", vec![]);
+    let (tmp, _got) = dir_setup(&exp);
+
+    let dents = WalkDir::new(tmp.path().join("foo"))
+        .follow_links(true)
+        .into_iter()
+        .collect::<Result<Vec<_>, _>>()
+        .unwrap();
+    assert_eq!(1, dents.len());
+    assert!(!dents[0].path_is_symlink());
+}
+
+// See: https://github.com/BurntSushi/walkdir/issues/115
+#[test]
+#[cfg(unix)]
+fn first_path_is_followed() {
+    let exp = td("foo", vec![
+        td("a", vec![tf("a1"), tf("a2")]),
+        td("b", vec![tlf("../a/a1", "alink")]),
+    ]);
+    let (tmp, _got) = dir_setup(&exp);
+
+    let dents = WalkDir::new(tmp.path().join("foo/b/alink"))
+        .into_iter()
+        .collect::<Result<Vec<_>, _>>()
+        .unwrap();
+    assert_eq!(1, dents.len());
+    assert!(dents[0].file_type().is_symlink());
+    assert!(dents[0].metadata().unwrap().file_type().is_symlink());
+
+    let dents = WalkDir::new(tmp.path().join("foo/b/alink"))
+        .follow_links(true)
+        .into_iter()
+        .collect::<Result<Vec<_>, _>>()
+        .unwrap();
+    assert_eq!(1, dents.len());
+    assert!(!dents[0].file_type().is_symlink());
+    assert!(!dents[0].metadata().unwrap().file_type().is_symlink());
+}
+
 #[test]
 #[cfg(unix)]
 fn walk_dir_sym_detect_no_follow_no_loop() {