fix: handle invalid working directories gracefully with --full-path - #1917
Conversation
tmccombs
left a comment
There was a problem hiding this comment.
This is a decent attempt, although there are some minor issues.
however, I'm not sure this is the best approach. One thing I notice is that we are calling current_dir() every time we construct an absolute path.
I wonder if maybe it would be better to re-architect this so that we only call current_dir() once, at the beginning, and if that succeeds, then we can create absolute paths from that infallibly afterwords, and if it fails we can fail early, without needing to check for every entry. And it would probably be more performant as well.
It would mean that if the current directory disappears while fd is running you will continue to get results that may no longer be valid, which may be good or bad depending on the situation.
|
You're right, caching cwd once at startup is the better approach. I reworked the PR: Config now gets a |
…harkdp#1900) - Add FatalError variant to WorkerResult for unrecoverable errors - Extract search_str_for_entry() helper to propagate IO errors from path_absolute_form - Propagate fatal errors through job and batch execution paths - Add regression test for invalid cwd scenario - Update CHANGELOG for sharkdp#1900
Instead of calling current_dir() for every entry when --full-path is set, cache it once in Config at startup. This eliminates the need for FatalError handling and makes path resolution infallible during the walk. If the cwd can't be retrieved, fd now fails early with a clear error message.
| match entry_path.file_name() { | ||
| Some(filename) => Cow::Borrowed(filename), | ||
| None => unreachable!( | ||
| "Encountered file system entry without a file name. This should only \ |
There was a problem hiding this comment.
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:
Lines 132 to 146 in 7027d45
I think we should expose DirEntry::file_name() and then just use that rather than doing this.
There was a problem hiding this comment.
Noted, will keep in mind for a follow-up.
| 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) | ||
| } |
There was a problem hiding this comment.
| 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)) | |
| } |
There was a problem hiding this comment.
nit: it seems slightly more intuitive to me for the cwd to come first, but that might just be me.
| 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>, |
There was a problem hiding this comment.
nit: perhaps full_path_base would be more indicative of how this is used?
| ) -> Cow<'a, OsStr> { | ||
| if let Some(cwd) = cwd { | ||
| let abs_path = filesystem::make_absolute(entry_path, cwd); | ||
| Cow::Owned(abs_path.into_os_string()) |
There was a problem hiding this comment.
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
fd 10.5.0 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>## Features - Add `--ignore-parent` option to override `--no-ignore-parent`, see #1958 (@tmchow) - Add `--exact` option to match the entire filename exactly (literal, non-substring). ## Bugfixes - Sanitize control characters and bidirectional override characters in filenames when output goes to a terminal, to prevent terminal escape-sequence injection. Also reject a placeholder as the executable for `--exec-batch`, while still allowing it for `--exec`. - Handle invalid working directories gracefully when using `--full-path`, see #1900 (@Xavrir). - Fire the "search pattern contains a path separator" diagnostic for any pattern containing `/`, not just patterns that happen to name an existing directory. Preserves the legacy Windows behaviour that also flags native `\` separators when the pattern resolves to a real directory. See #1873. - Also fire the "search pattern contains a path separator" diagnostic for `--and` patterns, not only the primary positional pattern. `--and` patterns are matched against the file name just like the primary pattern, so a path separator in them silently returned zero results. See #1873. - Fix bug where passing "-" as a directory argument didn't actually search that directory, see #849 (@Sean-Kenneth-Doherty). - Fix panic when `--changed-before`/`--changed-within` is given an out-of-range `@` Unix timestamp; the value is now rejected gracefully, see #2081 (@nikolauspschuetz). ## New Contributors * @liruiluo made their first contribution in sharkdp/fd#1924 * @Xavrir made their first contribution in sharkdp/fd#1917 * @rexkirshner made their first contribution in sharkdp/fd#1921 * @Rohan5commit made their first contribution in sharkdp/fd#1934 * @tmchow made their first contribution in sharkdp/fd#1959 * @YoshKoz made their first contribution in sharkdp/fd#1979 * @SAY-5 made their first contribution in sharkdp/fd#1975 * @skane-lukas made their first contribution in sharkdp/fd#1889 * @Sean-Kenneth-Doherty made their first contribution in sharkdp/fd#2006 * @stevenwalton made their first contribution in sharkdp/fd#2011 * @DeflateAwning made their first contribution in sharkdp/fd#1983 * @wyf027 made their first contribution in sharkdp/fd#2001 * @bonanza127 made their first contribution in sharkdp/fd#2040 * @mahirhir made their first contribution in sharkdp/fd#2036 * @parneetsingh022 made their first contribution in sharkdp/fd#2037 * @curious-rabbit made their first contribution in sharkdp/fd#1976 * @WilliamLeony made their first contribution in sharkdp/fd#2064 * @kobihikri made their first contribution in sharkdp/fd#2063 * @cmelaro made their first contribution in sharkdp/fd#2075 * @nikolauspschuetz made their first contribution in sharkdp/fd#2082 **Full Changelog**: https://github.com/sharkdp/fd/compare/v10.4.2...v10.5.0</pre> <p>View the full release notes at <a href="https://github.com/sharkdp/fd/releases/tag/v10.5.0">https://github.com/sharkdp/fd/releases/tag/v10.5.0</a>.</p> </details> <hr> See merge request: Harmonybrew/homebrew-core!17790
Fixes #1900
When using
--full-path, fd panics if the current working directory becomes invalid (e.g., deleted or inaccessible) during execution. This occurs because the code attempts to resolve relative paths after the cwd is no longer accessible.This fix replaces the panic with a graceful error message, allowing fd to exit cleanly instead of crashing.
Changes:
cargo testandcargo clippylocally