chore(ops): debt cleanup — file_ops, batch, search, archive - #113
Conversation
Reviewer's GuideNon-semantic ops cleanup across file operations, batch tests, search, and archive modules: minor refactors, dead-code removal/annotation, import deduplication, and documentation clarifications with no behavior changes. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Greptile SummaryThis PR performs behavior-preserving cleanup across archive, file-operation, batch, and search modules.
Confidence Score: 5/5The PR appears safe to merge, with no actionable behavioral, build, or security regressions identified. The functional-looking edits preserve existing cancellation, removal, directory traversal, wildcard construction, and module-resolution behavior; the remaining changes affect imports, tests, documentation, or annotations only.
|
| Filename | Overview |
|---|---|
| src/ops/file_ops/common.rs | Delegates mandatory cancellation checks to the equivalent optional helper and compiles the directory-symlink removal branch only on Windows without changing supported platform behavior. |
| src/ops/search/name.rs | Removes a redundant symlink predicate because DirEntry::file_type().is_dir() already excludes symlinks, while the separate symlink traversal path remains intact. |
| src/ops/search/pattern.rs | Replaces an Option flattening expression with an equivalent conditional while preserving all sensitivity and affix states. |
| src/ops/search.rs | Removes redundant module path attributes whose paths exactly match Rust's default module layout. |
| src/ops/file_ops/temp.rs | Adds diagnostic and compiler-use annotations to the temporary-directory guard without changing its lifecycle behavior. |
| src/ops/archive/tar.rs | Documents the intended lockstep relationship between archive creation and listing entry limits. |
| src/ops/archive/zip.rs | Documents non-exhaustive compression-method handling and uses the existing File import. |
| src/ops/chunk_copy.rs | Replaces fully qualified standard-library types with equivalent imports. |
Reviews (1): Last reviewed commit: "chore(ops): debt — file_ops, batch, sear..." | Re-trigger Greptile
PR Review:
|
| Gate | Result |
|---|---|
cargo fmt --check |
clean |
cargo clippy --locked --all-targets |
0 warnings |
cargo test --locked |
980 lib + 302 integration, 0 failed |
cargo build --release --locked |
ok |
Bugs / correctness
None found. The two semantically meaningful changes are verified behavior-preserving:
check_canceleddelegation (common.rs:43):check_optional_canceled(Some(cancel))produces identical behavior — sameErrorKind::Interrupted, same message, sameOrdering::Relaxed.#[cfg(windows)]gate on theremove_anysymlink branch (common.rs:263): correct. I confirmedis_dir_metahas no remaining Unix callers —delete.rs:6gates its ownuseto#[cfg(windows)], so after this gate the Unixis_dir_metastub is genuinely dead, justifying the new#[allow(dead_code)]. The claim "unreachable on Unix" holds:symlink_metadatalstat of a symlink is neveris_dir().
The smaller refactor claims all check out: name.rs !is_symlink() removal is valid (FileType::is_dir() excludes symlinks); pattern.rs .then(||...).flatten() → if/else is exactly equivalent; no remaining FQN sites or newly-orphaned imports; the tar/zip/MAX_LIST_ENTRIES (all 100_000) lockstep doc is factually accurate.
Issues to flag (all minor)
-
PR body test count is off. Claims
981 libtests; actual is980. No tests were added/removed, so it's just a miscount — worth correcting for traceability, not a code problem. -
#[allow(dead_code)]could be avoided (common.rs:30). Since the Unixis_dir_metastub now has zero callers, makingis_dir_metaWindows-only (#[cfg(windows)]only, dropping the#[cfg(not(windows))]stub + theallow) would remove a lint suppression. Keeping the stub for doc symmetry is a defensible alternative, but it does trade a#[allow]for intent. AGENTS.md discourages#[allow(...)]outside tests. -
move_ops.rsdoc placement (move_ops.rs:120-123). The new "Test-only convenience wrapper" paragraph is appended to a doc block (lines 110-118) that actually describesmove_entry_impl's case-rename semantics, not the wrapper. The placement is pre-existing, but the addition deepens the incongruity — consider moving the impl-specific prose ontomove_entry_impl. -
No Windows CI coverage. The PR modifies
#[cfg(windows)]removal/symlink logic that the ubuntu+macOS matrix never compiles. Pre-existing gap, not introduced here, but the Windows path-removal correctness now rests entirely on the doc comments rather than any build.
The rejection table is thorough and shows sound judgment (especially pattern #6 perf-not-debt, delete #8 intentional /private distinction, and model #5-9 API-shaping-not-debt). No requested changes blocking merge.
1 participant
Debt cleanup: file_ops, batch, search, archive (PR-16)
Non-semantic cleanup of ops modules — style, dead code, dedup, doc accuracy. No behavior changes.
CI gate (all pass, worktree)
cargo fmtcargo clippy --locked --all-targets -- -D warningscargo test --lockedcargo build --release --lockedFixed (16)
archive/sevenz.rsuse std::fs::{self};→use std::fs;archive/sevenz.rsSevenzExtractErrorduplication as forced bysevenz_rustcallback APIarchive/tar.rsMAX_CREATE_ENTRIES=MAX_LIST_ENTRIESlockstep relationshiparchive/zip.rscompression_method_name_ => Unknownas forced by#[non_exhaustive]zip crate APIarchive/zip.rsstd::fs::File→File(already imported)batch/tests.rs_sizes = helpers::path_sizes(...)bindingchunk_copy.rsstd::sync::mpsc::Sender/std::path::PathBuf→ importsfile_ops/common.rscheck_canceled— now delegates tocheck_optional_canceled(single impl)file_ops/common.rsis_symlink() && is_dir_meta()branch now#[cfg(windows)]— unreachable on Unixsearch/content.rsAtomicBool/Orderingimports into oneusesearch/name.rs!file_type.is_symlink()—is_dir()already excludes symlinkssearch/name.rsAtomicBool/Orderingimports into oneusesearch/pattern.rs.then(|| ...).flatten()→ idiomaticif/elsesearch.rs#[path = "search/..."]attrs on module declarationsfile_ops/move_ops.rs#[cfg(test)] pub fn move_entryas test-only convenience wrapperfile_ops/temp.rs#[must_use]+Debugderive onTempDirGuardRejected (with justification)
SearchOutcome::record_truncation(), which implements theget_or_insert-style dedup. Noif truncated.is_none()sites exist in the current code — the finding describes code that was already standardized.==onbyte_percent() == 100.0)byte_percent()returns the literal100.0(exactly representable in IEEE 754) whenbytes_done >= bytes_total. The test asserts on a hardcoded constant return, not a computed float.greedy_wildcard_matchO(n·m))*a*a*a*bshapes); the DP matcher is correct and the worst case is academic for filenames. A linear-time rewrite would be a perf PR, not a debt PR.pub fn matchesallocatesMatchScratch)matches()is a convenience API; the hot loop already usesmatches_with()with a reused scratch. Restructuring the public API shape is out of scope for a debt PR.l <= fdead guard intry_simple_wildcard)star_positions[1] > star_positions[0]is the guard (line 320). The oldl <= fcheck no longer exists in the current code — the finding references code already removed during the single-pass refactor.search_content_recursivefoldable)Finder+visitedset and seeds the context struct once per recursive scan. Folding it inline intosearch_contentwould duplicate the single-file vs directory branch and the setup — net more code, not less. The current split is the single-responsibility boundary.DisplayforSearchErrorKind,#[non_exhaustive], doc comments, and helper methods (is_empty/has_errors/merge) are API-shaping decisions that belong in a dedicated refactor, not a debt cleanup. Each would change the public API contract.error_slot.set(...) + return Error::Other(...)pattern is intrinsic tosevenz_rust's callback API: each call site sets a differentSevenzExtractErrorvariant before returning asevenz_rust::Error. A helper would need to take the enum variant as a parameter, adding indirection without removing the per-site classification logic. The repetition is in the error classification, not boilerplate.&Option<T>→Option<&T>params)cancel: &Option<Arc<AtomicBool>>from callers that hold theOptionby value. Changing toOption<&T>would ripple throughexecute_batch_with_byte_progressand all its callers inops::batch. API-shape change, not debt.TempFileReaderis a one-off adapter for the xz→temp-file path; extracting it to a named struct adds a file for a single match arm.Box<dyn Read>is forced by the multiple decompressor types (gz/bz2/zst/xz) returned fromwrap_decompress./privateprefix)/private,/Applicationsbut not/tmp,/flatpak, etc.). Deriving one from the other would add build-script complexity for ~90% overlap that is already self-documenting. The/privatewithout bare entry is intentional:/privateitself is a real dir users may manage;/private/etc,/private/tmp,/private/varare system symlinks.delete_recursive)mode & 0o7777mask / doc ratio)mode & 0o7777) is correct behavior —set_permissionsonly applies the permission bits, and the mask is defensive; #10 (doc:code ratio) is subjective and the doc explains why (non-interactive caller, ancestor creation).tests.rsbut that's a large mechanical extraction, not a debt-cleanup fix.too_many_argumentssuppress)copy_then_remove_srctakes three injected closures (for testability) plus the data they report on. AMoveContextstruct would group the closures but add a type whose only purpose is grouping — the params are intrinsic to the test-injection design.temp_dirvsTempDir) are test-maintenance gaps; #9 (inline error push vspush_read_error) — the inline pushes construct differentSearchErrorKindvariants per site, not boilerplate; #13 (ContentMatch alias private) is cosmetic.FileSearchContext/ContentSearchContextsharevisited/cancel/outcomebut differ in their match-type and pattern/finder fields — a shared base struct would need generics or trait objects, adding complexity for 3 fields. #8 (truncation check order) is a semantic nuance, not debt. #9 (re-export placement) is cosmetic.Summary by Sourcery
Clean up technical debt across ops modules with non-semantic refactors and documentation improvements in file operations, batch tests, search, and archive handling.
Enhancements:
Documentation:
Tests: