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.

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

Implement -fprint #421

Merged
merged 23 commits into from
Aug 11, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 49 additions & 3 deletions src/find/matchers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
use ::regex::Regex;
use chrono::{DateTime, Datelike, NaiveDateTime, Utc};
use fs::FileSystemMatcher;
use std::fs::File;
use std::path::Path;
use std::time::SystemTime;
use std::{error::Error, str::FromStr};
Expand Down Expand Up @@ -202,7 +203,7 @@
if !top_level_matcher.has_side_effects() {
let mut new_and_matcher = AndMatcherBuilder::new();
new_and_matcher.new_and_condition(top_level_matcher);
new_and_matcher.new_and_condition(Printer::new(PrintDelimiter::Newline));
new_and_matcher.new_and_condition(Printer::new(PrintDelimiter::Newline, None));
return Ok(new_and_matcher.build());
}
Ok(top_level_matcher)
Expand Down Expand Up @@ -339,6 +340,23 @@
}
}

/// 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>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fn get_or_create_file(path: &str) -> Result<File, Box> {
let file = File::create(path)?;
Ok(file)
}
isn't good ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let file = match File::open(path) {
Ok(file) => file,
Err(err) => {
if err.kind() == std::io::ErrorKind::NotFound {
File::create(path)?
} else {
return Err(From::from(err));

Check warning on line 352 in src/find/matchers/mod.rs

View check run for this annotation

Codecov / codecov/patch

src/find/matchers/mod.rs#L352

Added line #L352 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think it would be possible to add a test to trigger this case ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added it at 823949c I think this is the most stable trigger we can get besides nightly features.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops... I didn't notice that create includes overwriting existing files. Corrected the code in the new commit. Thanks.

}
}
};

Ok(file)
}

/// The main "translate command-line args into a matcher" function. Will call
/// itself recursively if it encounters an opening bracket. A successful return
/// consists of a tuple containing the new index into the args array to use (if
Expand All @@ -361,15 +379,24 @@
let mut invert_next_matcher = false;
while i < args.len() {
let possible_submatcher = match args[i] {
"-print" => Some(Printer::new(PrintDelimiter::Newline).into_box()),
"-print0" => Some(Printer::new(PrintDelimiter::Null).into_box()),
"-print" => Some(Printer::new(PrintDelimiter::Newline, None).into_box()),
"-print0" => Some(Printer::new(PrintDelimiter::Null, None).into_box()),
"-printf" => {
if i >= args.len() - 1 {
return Err(From::from(format!("missing argument to {}", args[i])));
}
i += 1;
Some(Printf::new(args[i])?.into_box())
}
"-fprint" => {
if i >= args.len() - 1 {
return Err(From::from(format!("missing argument to {}", args[i])));
}
i += 1;

let file = get_or_create_file(args[i])?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Some(Printer::new(PrintDelimiter::Newline, Some(file)).into_box())
}
"-true" => Some(TrueMatcher.into_box()),
"-false" => Some(FalseMatcher.into_box()),
"-lname" | "-ilname" => {
Expand Down Expand Up @@ -1518,4 +1545,23 @@
.expect("-version should stop parsing");
assert!(config.version_requested);
}

#[test]
fn get_or_create_file_test() {
use std::fs;

// remove file if hard link file exist.
// But you can't delete a file that doesn't exist,
// so ignore the error returned here.
let _ = fs::remove_file("test_data/get_or_create_file_test");

// test create file
let file = get_or_create_file("test_data/get_or_create_file_test");
assert!(file.is_ok());

let file = get_or_create_file("test_data/get_or_create_file_test");
assert!(file.is_ok());

let _ = fs::remove_file("test_data/get_or_create_file_test");
}
}
47 changes: 34 additions & 13 deletions src/find/matchers/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
// license that can be found in the LICENSE file or at
// https://opensource.org/licenses/MIT.

use std::{fs::File, io::Write};

use walkdir::DirEntry;

use super::{Matcher, MatcherIO};
Expand All @@ -25,25 +27,44 @@ impl std::fmt::Display for PrintDelimiter {
/// This matcher just prints the name of the file to stdout.
pub struct Printer {
delimiter: PrintDelimiter,
output_file: Option<File>,
}

impl Printer {
pub fn new(delimiter: PrintDelimiter) -> Self {
Self { delimiter }
pub fn new(delimiter: PrintDelimiter, output_file: Option<File>) -> Self {
Self {
delimiter,
output_file,
}
}
}

impl Matcher for Printer {
fn matches(&self, file_info: &DirEntry, matcher_io: &mut MatcherIO) -> bool {
let mut out = matcher_io.deps.get_output().borrow_mut();
write!(
out,
"{}{}",
file_info.path().to_string_lossy(),
self.delimiter
)
.unwrap();
out.flush().unwrap();
match &self.output_file {
hanbings marked this conversation as resolved.
Show resolved Hide resolved
Some(output_file) => {
let mut out = output_file;
write!(
out,
"{}{}",
file_info.path().to_string_lossy(),
self.delimiter
)
.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GNU find doesn't treat this as a fatal error. You can try -fprint /dev/full on Linux for example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
2950167

I 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, changed in new commit.

out.flush().unwrap();
hanbings marked this conversation as resolved.
Show resolved Hide resolved
}
None => {
let mut out = matcher_io.deps.get_output().borrow_mut();
write!(
out,
"{}{}",
file_info.path().to_string_lossy(),
self.delimiter
)
.unwrap();
out.flush().unwrap();
}
}
true
}

Expand All @@ -64,7 +85,7 @@ mod tests {
fn prints_newline() {
let abbbc = get_dir_entry_for("./test_data/simple", "abbbc");

let matcher = Printer::new(PrintDelimiter::Newline);
let matcher = Printer::new(PrintDelimiter::Newline, None);
let deps = FakeDependencies::new();
assert!(matcher.matches(&abbbc, &mut deps.new_matcher_io()));
assert_eq!(
Expand All @@ -77,7 +98,7 @@ mod tests {
fn prints_null() {
let abbbc = get_dir_entry_for("./test_data/simple", "abbbc");

let matcher = Printer::new(PrintDelimiter::Null);
let matcher = Printer::new(PrintDelimiter::Null, None);
let deps = FakeDependencies::new();
assert!(matcher.matches(&abbbc, &mut deps.new_matcher_io()));
assert_eq!(
Expand Down
17 changes: 17 additions & 0 deletions src/find/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1300,4 +1300,21 @@ mod tests {

assert_eq!(rc, 0);
}

#[test]
fn find_fprint() {
let deps = FakeDependencies::new();
let rc = find_main(
&[
"find",
"./test_data/simple",
"-fprint",
"test_data/find_fprint",
],
&deps,
);
assert_eq!(rc, 0);

let _ = fs::remove_file("test_data/find_fprint");
}
}
26 changes: 24 additions & 2 deletions tests/find_cmd_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
use assert_cmd::Command;
use predicates::prelude::*;
use serial_test::serial;
use std::fs::File;
use std::io::Write;
use std::fs::{self, File};
use std::io::{Read, Write};
use std::{env, io::ErrorKind};
use tempfile::Builder;

Expand Down Expand Up @@ -946,3 +946,25 @@ fn find_daystart() {
.success()
.stderr(predicate::str::is_empty());
}

#[test]
#[serial(working_dir)]
fn find_nogroup() {
hanbings marked this conversation as resolved.
Show resolved Hide resolved
let _ = fs::remove_file("test_data/find_fprint");

Command::cargo_bin("find")
.expect("found binary")
.args(["test_data/simple", "-fprint", "test_data/find_fprint"])
.assert()
.success()
.stdout(predicate::str::is_empty())
.stderr(predicate::str::is_empty());

// read test_data/find_fprint
let mut f = File::open("test_data/find_fprint").unwrap();
let mut contents = String::new();
f.read_to_string(&mut contents).unwrap();
assert!(contents.contains("test_data/simple"));

let _ = fs::remove_file("test_data/find_fprint");
}
Loading