Implement -follow
#420
Implement -follow
#420
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #420 +/- ##
==========================================
+ Coverage 66.26% 66.62% +0.36%
==========================================
Files 34 34
Lines 4043 4054 +11
Branches 916 917 +1
==========================================
+ Hits 2679 2701 +22
+ Misses 1000 990 -10
+ Partials 364 363 -1 ☔ View full report in Codecov by Sentry. |
|
Commit 93b5ae2 has GNU testsuite comparison: |
|
Commit c1ebc0a has GNU testsuite comparison: |
|
Commit 5abaed4 has GNU testsuite comparison: |
|
Commit 1b4e25b has GNU testsuite comparison: |
|
Commit e3911be has GNU testsuite comparison: |
|
Commit 887d76c has GNU testsuite comparison: |
|
Sorry, the test coverage is still not up to target. :( |
|
Commit 1e1846a has GNU testsuite comparison: |
|
Commit a0d4e46 has GNU testsuite comparison: |
|
Commit eed1c1e has GNU testsuite comparison: |
src/find/matchers/mod.rs
Outdated
| FileAgeRangeMatcher::new( | ||
| file_time_type, | ||
| minutes, | ||
| config.today_start, |
There was a problem hiding this comment.
It might be nicer to just pass &config to the matcher constructors so we don't have to keep changing the parameters.
src/find/matchers/time.rs
Outdated
| } | ||
|
|
||
| impl NewerMatcher { | ||
| pub fn new(path_to_file: &str) -> Result<Self, Box<dyn Error>> { | ||
| pub fn new(path_to_file: &str, follow: bool) -> Result<Self, Box<dyn Error>> { | ||
| let metadata = fs::metadata(path_to_file)?; |
There was a problem hiding this comment.
This should be symlink_metadata() if follow is false. I'd recommend adding a Config::metadata() method or something to centralize the logic.
There was a problem hiding this comment.
Thanks, I opened an issue tracking this: #431, and I'd like to open some new PRs to address it. :)
src/find/matchers/time.rs
Outdated
| }) | ||
| } | ||
|
|
||
| /// Implementation of matches that returns a result, allowing use to use try! | ||
| /// to deal with the errors. | ||
| fn matches_impl(&self, file_info: &DirEntry) -> Result<bool, Box<dyn Error>> { | ||
| let this_time = file_info.metadata()?.modified()?; | ||
| let path = file_info.path(); | ||
| let this_time = if self.follow && path.is_symlink() { |
There was a problem hiding this comment.
Checking self.follow here is wrong for command lines like
$ find foo -newer bar -follow-follow should apply here, but because NewerMatcher::new() was called before -follow was parsed, self.follow will be false. I think the right implementation would pass the final -follow state to matches(), possibly as part of MatcherIO. There could be a convenience method like MatcherIO::metadata(file_info) that handles following symlinks correctly.
(Also, it would be a good idea to cache the Metadata so each matcher doesn't have to do its own stat() call, but that's probably a separate refactoring.)
There was a problem hiding this comment.
The corresponding logic for using symlink_metadata has been added in the current commit, and I have also opened an issue to track the problem of metadata caching: #430. Thanks!
|
Commit 23675ab has GNU testsuite comparison: |
|
Commit 4092697 has GNU testsuite comparison: |
|
Commit 579ae32 has GNU testsuite comparison: |
|
Commit 2cc61ec has GNU testsuite comparison: |
There was a problem hiding this comment.
Presumably you want to call WalkDir::follow_links(config.follow) at some point?
Also, you could implement -L at the same time.
Revert changes and use the existing walkdir API instead.
|
Commit a7bf906 has GNU testsuite comparison: |
|
Commit aabc8b4 has GNU testsuite comparison: |
|
Commit 8443618 has GNU testsuite comparison: |
|
@sylvestre #436 contains a squashed version of this PR if you'd rather avoid fixing the tests here |
|
This can be closed now that #436 was merged |
Closed #308
May conflict with #416, better merge #416 first. :)
There are changes as described in
man find:(The following changes are: When matching files, match the file pointed to by the file link.)