Fix 23 file ops, search, batch, and user menu bugs#50
Conversation
file_ops.rs: no-op perms (7.1), Windows overwrite race (7.2), case-rename docs (7.3), TOCTOU copy (7.8), string prefix critical-dir check (7.9), symlink descendant race (7.10) batch.rs: uncanonicalized dest dedup (7.4), symlink-aware dedup_paths (7.5) helpers.rs: silent entry errors (7.6), rename path_starts_with → lexical_path_starts_with (7.7) search.rs: glob overlap false positive (8.1), silent non-UTF-8 skip (8.2), missing pub API (8.3), İ limitation doc (8.4), per-match alloc (8.7), DP→greedy wildcard matcher (8.8) user_menu.rs: fail-open conditions (9.1), whitespace stripping (9.2), %f non-UTF-8 inconsistency (9.3), option injection (9.4) types.rs/README: CompareMode docs (8.5), dirs caveat (8.6) sorting.rs: ASCII-only natural sort doc (8.9)
There was a problem hiding this comment.
Sorry @leszek3737, you have reached your weekly rate limit of 1500000 diff characters.
Please try again later or upgrade to continue using Sourcery
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements to file operations, path handling, and search functionality. Key changes include a more robust dedup_paths implementation using device/inode IDs, enhanced safety in file and directory copying through temporary files and symlink race detection, and a more efficient greedy wildcard matching algorithm. Feedback from the review highlights potential issues with the option injection prevention strategy in shell commands, a collision risk in the Windows file backup naming scheme, the need for better error reporting when backup restoration fails, and potential memory overhead from excessive error logging during file searches.
Greptile Summary
This PR addresses 23 reported bugs across file operations, batch processing, search, and the user menu. The fixes cover a broad surface area including TOCTOU hardening, symlink-aware deduplication, wildcard matcher replacement, option-injection protection, and fail-closed condition semantics.
file_ops.rs: Copy operations now use a temp-then-rename pattern with post-copy symlink-race revalidation;delete_dir_recursiveusesPath-based critical-dir comparison instead of string-prefix matching;publish_temp_dirnow takes source permissions rather than reading them from the temp dir.batch.rs:dedup_pathsswitches fromcanonicalize-based dedup to inode identity (dev+ino) so symlinks and their targets are kept as distinct entries; the destination dedup key is canonicalized before insertion.search.rs: DP wildcard matcher replaced with a greedy algorithm;needle_strstored to avoid per-match allocation;WildcardSimplegains a combined prefix+suffix length guard; several functions madepub.user_menu.rs: Invalid/unsupported conditions now compile toCompiledCondition::Never(fail-closed);safe_file_argprepends--for dash-leading filenames;current_filefield widened to&Pathto handle non-UTF-8 names consistently.Confidence Score: 3/5
Two copy paths leave temporary files/directories on disk when specific error conditions are hit, and the new cross-platform dedup code will fail to compile on Windows due to unconditional declarations only consumed inside unix-only cfg blocks.
The temp-file leak in copy_file (apply_metadata failure path) and the temp-dir leak in both copy_dir_recursive variants (canonicalize failure inside the Ok arm) are real, reachable error paths that leave filesystem garbage behind. The unused seen_ids and meta bindings on non-Unix will produce compiler warnings that become hard errors under RUSTFLAGS=-D warnings, breaking Windows builds. These three issues are independent and affect different parts of the change.
src/ops/file_ops.rs (temp-resource cleanup paths) and src/ops/batch.rs (non-Unix cfg hygiene) need the most attention before merging.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[copy_file / copy_dir_recursive] --> B[reserve_temp_file/dir] B --> C[copy content to temp] C --> D{copy succeeded?} D -- No --> E[cleanup_file/dir_all\nreturn Err] D -- Yes --> F[apply_metadata to temp] F --> G{metadata applied?} G -- No --> H[temp LEAKED - missing cleanup_file] G -- Yes --> I{dest.exists? / revalidate dest} I -- dest changed --> J[cleanup_file/dir_all\nreturn Err] I -- dest OK --> K{canonicalize_with_nearest... ?} K -- Err --> L[temp_dest LEAKED - missing cleanup_dir_all] K -- Ok --> M[publish_temp_dir / swap_temp_to_dest] M --> N{rename succeeded?} N -- No --> O[cleanup_file/dir_all\nreturn Err] N -- Yes --> P[return Ok bytes]