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

Add a --quote option to put quotation marks around output files/dirs #1366

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,14 @@ pub struct Opts {
#[arg(long, aliases(&["mount", "xdev"]), hide_short_help = true, long_help)]
pub one_file_system: bool,

/// By default we output matched files/dirs raw. When the user specifies
/// --quote we output the files wrapped in quotes per the rules laid out
/// in coreutils: https://www.gnu.org/software/coreutils/quotes.html
/// This should mimic the `ls -lsa` output style
#[cfg(any(unix, windows))]
#[arg(long, aliases(&["quote"]), hide_short_help = true, long_help)]
pub use_quoting: bool,

#[cfg(feature = "completions")]
#[arg(long, hide = true, exclusive = true)]
gen_completions: Option<Option<Shell>>,
Expand Down
3 changes: 3 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ pub struct Config {

/// Whether or not to strip the './' prefix for search results
pub strip_cwd_prefix: bool,

/// Whether to use quoting on the output file names
pub use_quoting: bool,
}

impl Config {
Expand Down
Empty file added src/exec/foo'bar'baz'exit.txt
Empty file.
Empty file added src/exec/foo'bar'baz_exit.txt
Empty file.
1 change: 1 addition & 0 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ fn construct_config(mut opts: Opts, pattern_regexps: &[String]) -> Result<Config
one_file_system: opts.one_file_system,
null_separator: opts.null_separator,
quiet: opts.quiet,
use_quoting: opts.use_quoting,
max_depth: opts.max_depth(),
min_depth: opts.min_depth(),
prune: opts.prune,
Expand Down
61 changes: 58 additions & 3 deletions src/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,43 @@ fn print_trailing_slash<W: Write>(
Ok(())
}

// Trying to copy: https://www.gnu.org/software/coreutils/quotes.html
fn path_needs_quoting(path: &str) -> i8 {
// If it contains any special chars we return single quote
if path.contains(" ") || path.contains("$") || path.contains("\"") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

return 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better to use an enumeration instead of an i8, where it isn't obvious what the different numbers mean.

// If it ONLY contains a ' we return double quote
} else if path.contains("'") {
return 2;
}

return 0;
}

// Quote a path with coreutils style quoting to make copy/paste
// more friendly for shells
fn quote_path(path_str: &str) -> String {
let quote_type = path_needs_quoting(path_str);
let mut tmp_str:String = path_str.into();

// Quote with single quotes
if quote_type == 1 {
// Escape single quotes in path
tmp_str = str::replace(&tmp_str, "'", "'\\''");

format!("'{}'", tmp_str)
// Quote with double quotes
} else if quote_type == 2 {
// Escape double quotes in path
tmp_str = str::replace(&tmp_str, "\"", "\\\"");

format!("\"{}\"", tmp_str)
// No quoting required
} else {
path_str.to_string()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This unnecessarily copies the string again even if there is no change

}
}

// TODO: this function is performance critical and can probably be optimized
fn print_entry_colorized<W: Write>(
stdout: &mut W,
Expand All @@ -62,9 +99,22 @@ fn print_entry_colorized<W: Write>(
ls_colors: &LsColors,
) -> io::Result<()> {
// Split the path between the parent and the last component
let mut offset = 0;
let path = entry.stripped_path(config);
let path_str = path.to_string_lossy();
let mut offset = 0;
let path = entry.stripped_path(config);
let mut path_str = path.to_string_lossy();
let mut needs_quoting = false;

// Wrap the path in quotes
if config.use_quoting {
let tmp_str = quote_path(&path_str);

// If the quoted string is new, then we flag that to tweak the offset
// so the colors line up
if tmp_str != path_str {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has to compare every character of the string. It would be more efficient if you did something like have return quote_path return an Option that is only Some if quoting happened, and then do:

if let Some(quoted) = quote_path(&path_str) {
    path_str = quoted.into();
    needs_quoting = true;
}

path_str = tmp_str.into();
needs_quoting = true;
}
}

if let Some(parent) = path.parent() {
offset = parent.to_string_lossy().len();
Expand All @@ -78,6 +128,11 @@ fn print_entry_colorized<W: Write>(
}

if offset > 0 {

if needs_quoting {
offset += 2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is correct. If it is only quoted shouldn't the offset be just 1 more?

And if it is double quoted, and has escaped characters, then the proper offset will depend on how many characters in the parent ended up getting escaped.

}

let mut parent_str = Cow::from(&path_str[..offset]);
if let Some(ref separator) = config.path_separator {
*parent_str.to_mut() = replace_path_separator(&parent_str, separator);
Expand Down
22 changes: 22 additions & 0 deletions tests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,14 @@ fn test_simple() {
);
}

static AND_QUOTE_FILES: &[&str] = &[
"one'two.quo",
"one two.quo",
"one\"two.quo",
"one$two.quo",
"one'two$.quo",
];

static AND_EXTRA_FILES: &[&str] = &[
"a.foo",
"one/b.foo",
Expand Down Expand Up @@ -2527,6 +2535,20 @@ fn test_strip_cwd_prefix() {
);
}

#[test]
fn test_quoting() {
let te = TestEnv::new(DEFAULT_DIRS, AND_QUOTE_FILES);

te.assert_output(
&["--quote", ".quo"],
"'one two.quo'
\"one'two.quo\"
\"one'two\\$.quo\"
'one\"two.quo'
'one$two.quo'",
);
}

/// When fd is ran from a non-existent working directory, but an existent
/// directory is passed in the arguments, it should still run fine
#[test]
Expand Down