search

BurntSushi / walkdir

Rust library for walking directories recursively.
The Unlicense
1.3k stars 109 forks source link

bug: fix contents_first with root symlink #165

Open danielparks opened 2 years ago

danielparks commented 2 years ago

This fixes two incorrect behaviors when contents_first is on and the root path is a symlink:

  1. The root path was returned first in the iterator instead of last.
  2. Some subdirectories were returned out of order.

The issue was that root symlinks were returned immediately rather than being pushed onto the deferred_dirs vec. That lead to deferred_dirs and depth being out of sync, which lead to deferred directories being processed one ascent too late.

This also changes the internal handling of the same_file_system option slightly. Previously, if a directory was found to be on a different file system it would not be pushed to the stack, but it would be pushed to the deferred list. This did not matter because in those cases handle_entry() would return to next(), which would immediately pop the directory off the deferred list and return it. This ensures that the directory is returned immediately, and is not pushed to either the stack or the deferred list. (I think this makes the code clearer.)

Fixes: #163

Unfortunately, writing tests for same_file_system would involve either adding a FS indirection layer (oh boy), or… calling out to sudo, I guess? Or maybe having a separate script to set things up? I did some testing manually and I’m convinced that it still works the same way it previously did, and that it is correct.

See #164 for more tests related to this.

I haven’t tested this on Windows, but I have maintained the same checks for symlinks and dirs, so I’m pretty sure it should be fine. Sorry, I just don’t have a good way to test it right now.

There are a couple of other versions of this fix in defer_root_symlink_separate_commits. In particular, the original version combined the if self.opts.follow_links in with the let should_descend = if, but I felt that it was weird to have dent = itry!(self.follow(dent)) inside an if block that was nominally for determining the value of should_descend.

danielparks commented 2 years ago

Sorry, that was kind of a lot of text.

danielparks commented 1 year ago

Ping @BurntSushi. Anything I can do on this or #164? I just rebased. Of course, I realize you probably have other priorities. :)