Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
# Unreleased

## Bugfixes
- Handle invalid working directories gracefully when using `--full-path`, see #1900 (@Xavrir).

# 10.4.2

## Bugfixes
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.into_os_string())
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 this is the same as what we had before, and doesn't need to be changed in this PR, but this could actually be a Borrowed, if the original path was absolute.

It would be a little simpler if we changed the new make_absolute function to return a Cow<'a, OsStr> directly instead of a PathBuf or Cow

} 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