Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: filter_entry misbehaves when contents_first is enabled #198

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix: filter_entry misbehaves when contents_first is enabled #198

wants to merge 2 commits into from

Conversation

kikuomax
Copy link

Proposed changes

  • Introduce a new private method IntoIter::skip_current_dir_unless_contents_first
  • Make FilterEntry::next call skip_current_dir_unless_contents_first instead of skip_current_dir if the skipped entry is a directory
  • Add a test case that tests the combination of filter_entry and contents_first

Details

The documentation of IntoIter::filter_entry states that:

Note that if the iterator has contents_first enabled, then this method is no different than calling the standard Iterator::filter method (because directory entries are yielded after they’ve been descended into).

However, if contents_first was enabled and some directories were filtered out, unexpected entries may have been skipped regardless of the filter_entry results. Please refer to #171 for an example.

FilterEntry::next used to call skip_current_dir when the filtered out entry was a directory to stop reading the filtered out directory. skip_current_dir realizes this by popping the last DirList from stack_list. Unfortunately, if contents_first was enabled, skip_current_dir popped a wrong DirList from stack_list, because the DirList of the filtered out directory had already been removed from stack_list.

We should not update stack_list if contents_first is enabled. So I introduced skip_current_dir_unless_contents_first which does nothing if contents_first is enabled, otherwise calls skip_current_dir.

@kikuomax
bug: filter_entry misbehaved when contents_first is set
d4ac8c5 
The behavior of `IntoIter::filter_entry` contradicted its documentation
when the `contents_first` was enabled and the predicate returned `false`
for directories.

> Note that if the iterator has contents_first enabled, then this method
> is no different than calling the standard Iterator::filter method

According to the documentation, it must list all the entries in the
skipped directories anyway, but listed only few of them.

This commit fixes the issue by making `FilterEntry::next` call
a newly introduced `IntoIter::skip_current_dir_unless_contents_first`
method instead of the `IntoIter::skip_current_dir` method when the
predicate returns `false` for a directory. `FilterEntry::next` used to
skip a wrong directory when the predicate returned `false`.
@kikuomax
test: test filter_entry and contents_first
5977b2e 
A new test case `filter_entry_contents_first` makes sure that
`filter_entry` enumerates all the contents except for entries filtered
out, if `contents_first` is enabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

filter_entry misbehaves when contents_first is enabled
1 participant
@kikuomax