Implement -fprint
#421
Implement -fprint
#421
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #421 +/- ##
==========================================
- Coverage 66.13% 66.08% -0.06%
==========================================
Files 34 34
Lines 4004 4042 +38
Branches 906 915 +9
==========================================
+ Hits 2648 2671 +23
- Misses 997 998 +1
- Partials 359 373 +14 ☔ View full report in Codecov by Sentry. |
|
Commit e40ae10 has GNU testsuite comparison: |
|
needs rebasing please :) |
|
Commit 0a6a9e9 has GNU testsuite comparison: |
|
Commit 9303508 has GNU testsuite comparison: |
src/find/matchers/mod.rs
Outdated
| if err.kind() == std::io::ErrorKind::NotFound { | ||
| File::create(path)? | ||
| } else { | ||
| return Err(From::from(err)); |
There was a problem hiding this comment.
do you think it would be possible to add a test to trigger this case ?
There was a problem hiding this comment.
I checked the ErrorKind page, how about we select PermissionDenied as a test case and use the following code to trigger it under Unix system?
#[cfg(unix)]
{
let result = get_or_create_file("/etc/shadow");
assert!(result.is_err());
}
There was a problem hiding this comment.
I've added it at 823949c I think this is the most stable trigger we can get besides nightly features.
There was a problem hiding this comment.
Why don't you just use File::create()? That truncates the file, but so does -fprint:
tavianator@tachyon $ echo Hello world >foo
tavianator@tachyon $ find -fprint foo -prune
tavianator@tachyon $ cat foo
.There was a problem hiding this comment.
Oops... I didn't notice that create includes overwriting existing files. Corrected the code in the new commit. Thanks.
|
Commit 823949c has GNU testsuite comparison: |
|
Commit b255b4e has GNU testsuite comparison: |
|
Commit 3e1b180 has GNU testsuite comparison: |
|
Commit 2823662 has GNU testsuite comparison: |
| @@ -339,6 +340,19 @@ fn parse_str_to_newer_args(input: &str) -> Option<(String, String)> { | |||
| } | |||
| } | |||
|
|
|||
| /// Creates a file if it doesn't exist. | |||
| /// If it does exist, it will be overwritten. | |||
| fn get_or_create_file(path: &str) -> Result<File, Box<dyn Error>> { | |||
There was a problem hiding this comment.
fn get_or_create_file(path: &str) -> Result<File, Box> {
let file = File::create(path)?;
Ok(file)
}
isn't good ?
There was a problem hiding this comment.
Would it be better to give the user a more friendly prompt here? But yes, it is better to use "?" for now.
I would like to keep get_or_create_file function first, so that it is convenient to modify these unexpected behaviors in a unified way later. :)
Changes have been committed at d02b2c4. thanks.
|
Commit d02b2c4 has GNU testsuite comparison: |
src/find/matchers/printer.rs
Outdated
| file_info.path().to_string_lossy(), | ||
| self.delimiter | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
GNU find doesn't treat this as a fatal error. You can try -fprint /dev/full on Linux for example.
There was a problem hiding this comment.
Thanks, the other comment in the current review have been revised.
and GNU find seems to also interrupt the traversal of the remaining files. Should we also interrupt the traversal and give a suitable message?
There was a problem hiding this comment.
GNU find doesn't interrupt traversal, but it does delay the error message until the end of traversal. You can see that with something like
tavianator@graphene $ find -print | wc -l
2950167
tavianator@graphene $ find -print -fprint /dev/full | wc -l
find: ‘/dev/full’: No space left on device
2950167I don't think we need to delay the error message like they do. It's fine to print the error when it happens, but we should continue traversal afterwards.
There was a problem hiding this comment.
Thanks, changed in new commit.
|
Commit bc6b06f has GNU testsuite comparison: |
|
Commit 65d23fb has GNU testsuite comparison: |
|
Commit 267db8a has GNU testsuite comparison: |
|
Commit da394ad has GNU testsuite comparison: |
|
Commit b38a963 has GNU testsuite comparison: |
| e | ||
| ) | ||
| .unwrap(); | ||
| uucore::error::set_exit_code(1); |
There was a problem hiding this comment.
This could be a method on MatcherIO, and then you wouldn't need to unset it in the tests
| } | ||
| i += 1; | ||
|
|
||
| let file = get_or_create_file(args[i])?; |
There was a problem hiding this comment.
For future reference: GNU find de-duplicates files, so if you do find -fprint foo -fprint foo it will use the same FILE * for both and you'll get each line twice. Right now uutils will use two different Files and the writes will overlap with each other.
|
@hanbings could you please address the two last comments in new PR ? |
Closed #381