Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## Bugfixes
- Fixed performance regression due to `--ignore-contain`; see #1913 and #1914
- Handle invalid working directories gracefully when using `--full-path`, see #1900 (@Xavrir).
Comment thread
tmccombs marked this conversation as resolved.
Outdated

# 10.4.1

Expand Down
6 changes: 3 additions & 3 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ pub struct Config {
/// Whether the search is case-sensitive or case-insensitive.
pub case_sensitive: bool,

/// Whether to search within the full file path or just the base name (filename or directory
/// name).
pub search_full_path: bool,
/// Cached current working directory for absolute path construction.
/// Populated when `--full-path` is set; `None` means search by filename only.
pub cwd: Option<PathBuf>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: perhaps full_path_base would be more indicative of how this is used?


/// Whether to ignore hidden files and directories (or not).
pub ignore_hidden: bool,
Expand Down
47 changes: 47 additions & 0 deletions src/filesystem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,17 @@ pub fn path_absolute_form(path: &Path) -> io::Result<PathBuf> {
env::current_dir().map(|path_buf| path_buf.join(path))
}

/// Construct an absolute path from a potentially relative path and a
/// pre-resolved working directory. Unlike `path_absolute_form`, this
/// does not call `env::current_dir()` and cannot fail.
pub fn make_absolute(path: &Path, cwd: &Path) -> PathBuf {
if path.is_absolute() {
return path.to_path_buf();
}
let path = path.strip_prefix(".").unwrap_or(path);
cwd.join(path)
}
Comment on lines +26 to +32
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pub fn make_absolute(path: &Path, cwd: &Path) -> PathBuf {
if path.is_absolute() {
return path.to_path_buf();
}
let path = path.strip_prefix(".").unwrap_or(path);
cwd.join(path)
}
pub fn make_absolute<'a, 'b>(path: &'a Path, cwd: &'b Path) -> Cow<'a, Path> {
if path.is_absolute() {
return Cow::Borrowed(path);
}
let path = path.strip_prefix(".").unwrap_or(path);
Cow::Owned(cwd.join(path))
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: it seems slightly more intuitive to me for the cwd to come first, but that might just be me.


pub fn absolute_path(path: &Path) -> io::Result<PathBuf> {
let path_buf = path_absolute_form(path)?;

Expand Down Expand Up @@ -153,4 +164,40 @@ mod tests {
Path::new("foo/bar/baz")
);
}

#[test]
fn make_absolute_with_relative_path() {
use super::make_absolute;
use std::path::PathBuf;

let cwd = Path::new("/home/user");
assert_eq!(
make_absolute(Path::new("foo/bar"), cwd),
PathBuf::from("/home/user/foo/bar")
);
}

#[test]
fn make_absolute_strips_dot_prefix() {
use super::make_absolute;
use std::path::PathBuf;

let cwd = Path::new("/home/user");
assert_eq!(
make_absolute(Path::new("./foo/bar"), cwd),
PathBuf::from("/home/user/foo/bar")
);
}

#[test]
fn make_absolute_with_absolute_path() {
use super::make_absolute;
use std::path::PathBuf;

let cwd = Path::new("/home/user");
assert_eq!(
make_absolute(Path::new("/absolute/path"), cwd),
PathBuf::from("/absolute/path")
);
}
}
11 changes: 10 additions & 1 deletion src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,18 @@ fn construct_config(mut opts: Opts, pattern_regexps: &[String]) -> Result<Config
let command = extract_command(&mut opts, colored_output)?;
let has_command = command.is_some();

let cwd = if opts.full_path {
Some(env::current_dir().context(
"Could not determine current directory. \
This is required for --full-path.",
)?)
} else {
None
};

Ok(Config {
case_sensitive,
search_full_path: opts.full_path,
cwd,
ignore_hidden: !(opts.hidden || opts.rg_alias_ignore()),
read_fdignore: !(opts.no_ignore || opts.rg_alias_ignore()),
read_vcsignore: !(opts.no_ignore || opts.rg_alias_ignore() || opts.no_ignore_vcs),
Expand Down
34 changes: 20 additions & 14 deletions src/walk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -524,20 +524,7 @@ impl WorkerState {
// Check the name first, since it doesn't require metadata
let entry_path = entry.path();

let search_str: Cow<OsStr> = if config.search_full_path {
let path_abs_buf = filesystem::path_absolute_form(entry_path)
.expect("Retrieving absolute path succeeds");
Cow::Owned(path_abs_buf.as_os_str().to_os_string())
} else {
match entry_path.file_name() {
Some(filename) => Cow::Borrowed(filename),
None => unreachable!(
"Encountered file system entry without a file name. This should only \
happen for paths like 'foo/bar/..' or '/' which are not supposed to \
appear in a file system traversal."
),
}
};
let search_str = search_str_for_entry(entry_path, config.cwd.as_deref());

if !patterns
.iter()
Expand Down Expand Up @@ -676,6 +663,25 @@ impl WorkerState {
}
}

fn search_str_for_entry<'a>(
entry_path: &'a std::path::Path,
cwd: Option<&std::path::Path>,
) -> Cow<'a, OsStr> {
if let Some(cwd) = cwd {
let abs_path = filesystem::make_absolute(entry_path, cwd);
Cow::Owned(abs_path.as_os_str().to_os_string())
Comment thread
Xavrir marked this conversation as resolved.
Outdated
} else {
match entry_path.file_name() {
Some(filename) => Cow::Borrowed(filename),
None => unreachable!(
"Encountered file system entry without a file name. This should only \
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I know you cut and pasted this code so no need to fix this in this PR, but we already have a better approach here:

fd/src/dir_entry.rs

Lines 132 to 146 in 7027d45

fn file_name(&self) -> OsString {
let name = match &self.inner {
DirEntryInner::Normal(e) => e.file_name(),
DirEntryInner::BrokenSymlink(path) => {
// Path::file_name() only works if the last component is Normal,
// but we want it for all component types, so we open code it.
// Copied from LsColors::style_for_path_with_metadata().
path.components()
.next_back()
.map(|c| c.as_os_str())
.unwrap_or_else(|| path.as_os_str())
}
};
name.to_owned()
}

I think we should expose DirEntry::file_name() and then just use that rather than doing this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Noted, will keep in mind for a follow-up.

happen for paths like 'foo/bar/..' or '/' which are not supposed to \
appear in a file system traversal."
),
}
}
}

/// Recursively scan the given search path for files / pathnames matching the patterns.
///
/// If the `--exec` argument was supplied, this will create a thread pool for executing
Expand Down