bug: fix contents_first with root symlink #165
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Sorry, that was kind of a lot of text. |
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: BurntSushi#163
danielparks
force-pushed
the
defer_root_symlink
branch
from
9fa3585
to
18d6c79
Compare
|
Ping @BurntSushi. Anything I can do on this or #164? I just rebased. Of course, I realize you probably have other priorities. :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes two incorrect behaviors when
contents_firstis on and the root path is a symlink:The issue was that root symlinks were returned immediately rather than being pushed onto the
deferred_dirsvec. That lead todeferred_dirsanddepthbeing out of sync, which lead to deferred directories being processed one ascent too late.This also changes the internal handling of the
same_file_systemoption 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 caseshandle_entry()would return tonext(), 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_systemwould involve either adding a FS indirection layer (oh boy), or… calling out tosudo, 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_linksin with thelet should_descend = if, but I felt that it was weird to havedent = itry!(self.follow(dent))inside anifblock that was nominally for determining the value ofshould_descend.