♻️ Split PermissionStrategy into ModeStrategy + OwnerStrategy#2593
Conversation
📝 WalkthroughWalkthroughThis change splits the previous monolithic PermissionStrategy into two distinct strategies: ModeStrategy (file mode handling) and OwnerStrategy (ownership handling). OwnerSource is replaced by OwnerOptions, resolver.resolve() now returns (ModeStrategy, OwnerStrategy), and call sites/metadata paths are updated to use the split strategies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @ChanTsune, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a significant internal refactoring of how file permissions are managed within the application. By splitting the monolithic Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great refactoring that splits the monolithic PermissionStrategy into more granular ModeStrategy and OwnerStrategy. This significantly improves the clarity and flexibility of permission handling. However, I've identified a critical issue in the extraction logic where file modes are not restored if owner preservation is disabled, which undermines the goal of this refactoring. I've provided suggestions to correct this behavior. Additionally, I've included a couple of medium-severity suggestions for improving code clarity and documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cli/src/command/extract.rs:
- Around line 10-15: restore_metadata() currently only applies permissions when
OwnerStrategy::Preserve and restore_permissions() always does both chown+chmod;
change the logic to split chown and chmod so chmod is controlled by
keep_options.mode_strategy (e.g., ModeStrategy::Preserve) independent of owner
handling. Update restore_metadata() to consult keep_options.mode_strategy for
applying file mode, and refactor restore_permissions() (or the helper that calls
chown/chmod) to call chown only when owner_strategy dictates but call chmod when
mode_strategy dictates; reference ModeStrategy, keep_options.mode_strategy,
OwnerStrategy::Preserve, restore_metadata(), and restore_permissions() when
making the changes.
🧹 Nitpick comments (4)
cli/src/command/core/permission.rs (1)
1-30: Good split + sensible defaults; consider clarifying “empty string” semantics for uname/gname.
OwnerOptions/ModeStrategy/OwnerStrategylook consistent and theNeverdefaults are appropriate. One small thing: elsewherenumeric_ownerappears to be represented bySome(String::new())for names; it’s worth documenting here thatSome("")is an intentional sentinel distinct fromNone.cli/src/command/append.rs (1)
375-405: LGTM: strategy resolution is now explicit and reused.This makes the KeepOptions construction clearer and aligns with the new split model.
cli/src/command/stdio.rs (1)
653-679: Consistent adoption of(mode_strategy, owner_strategy)across stdio modes; consider a tiny helper to DRY this up.The repeated
PermissionStrategyResolver { ... }.resolve()blocks are identical in shape; a private helper (or builder) could reduce copy/paste and keep future flag additions safer.Also applies to: 746-785, 917-950, 1045-1078
cli/src/command/core.rs (1)
712-780: Consider makingapply_metadata()explicitly consultkeep_options.mode_strategy(even if currently redundant).Right now, permission emission is gated by
OwnerStrategy::Preserve { .. }. If later you allowModeStrategy::PreservewithOwnerStrategy::Never(the motivating split), this function will silently stop storing mode bits. A smallmatch keep_options.mode_strategy { Preserve => { ... }, Never => {} }(and optionallydebug_assert!(!matches!(owner_strategy, Preserve) || matches!(mode_strategy, Preserve))) would make the invariant explicit.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cli/src/command/append.rscli/src/command/core.rscli/src/command/core/permission.rscli/src/command/create.rscli/src/command/extract.rscli/src/command/stdio.rscli/src/command/update.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Test WebAssembly (beta, wasm32-unknown-emscripten)
- GitHub Check: tier1 (windows-11-arm, nightly)
- GitHub Check: tier1 (macos-latest, stable)
- GitHub Check: plan
- GitHub Check: tier1 (windows-11-arm, beta)
- GitHub Check: tier1 (windows-latest, beta)
- GitHub Check: tier1 (windows-latest, nightly)
- GitHub Check: tier1 (ubuntu-latest, nightly)
- GitHub Check: tier1 (windows-11-arm, stable)
- GitHub Check: tier1 (ubuntu-latest, beta)
- GitHub Check: tier1 (ubuntu-latest, stable)
- GitHub Check: tier1 (ubuntu-22.04-arm, beta)
- GitHub Check: tier1 (ubuntu-22.04-arm, stable)
- GitHub Check: msrv (ubuntu-latest, portable-network-archive)
- GitHub Check: Test WebAssembly (stable, wasm32-wasip2)
- GitHub Check: Test WebAssembly (stable, wasm32-unknown-emscripten)
- GitHub Check: Test WebAssembly (stable, wasm32-unknown-unknown)
🔇 Additional comments (3)
cli/src/command/update.rs (1)
384-414: LGTM: consistent split-strategy wiring with append/create.cli/src/command/create.rs (1)
462-492: LGTM: create path correctly threads split strategies into KeepOptions.cli/src/command/core.rs (1)
11-11: Nice cleanup: KeepOptions + resolver now match the split model.Also applies to: 199-208, 259-316
This refactoring separates mode handling from owner handling internally, enabling the fine-grained control required for bsdtar-style `-p` flag semantics (which affects mode but not owner). Changes: - Create new types: ModeStrategy, OwnerStrategy, OwnerOptions - Update KeepOptions to use split strategies instead of combined - Update PermissionStrategyResolver to return (ModeStrategy, OwnerStrategy) - Update apply_metadata in core.rs for new strategy types - Update restore_permissions in extract.rs for new strategy types - Update all KeepOptions construction sites in create, extract, append, update, and stdio commands - Remove legacy PermissionStrategy and OwnerSource types No user-facing changes in this refactoring phase.
ChanTsune
force-pushed
the
cli/core/refactoring
branch
from
129b0b5 to
5fb7fc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cli/src/command/stdio.rs:
- Around line 653-663: The extraction logic ignores the --same-owner flag
because PermissionStrategyResolver is constructed with same_owner hardcoded to
true; change the argument definition to set default_value_t = true for the
same_owner clap Arg and construct the resolver with same_owner: args.same_owner
&& !args.no_same_owner so PermissionStrategyResolver { ..., same_owner, uname,
gname, uid, gid, numeric_owner: args.numeric_owner }.call resolve(); update both
occurrences mentioned (around the current call and the one at lines ~744-757) so
--same-owner and --no-same-owner behave correctly.
🧹 Nitpick comments (4)
cli/src/command/core/permission.rs (1)
1-10: ClarifyOwnerOptionssemantics forSome("")(numeric-owner sentinel).
OwnerOptionsdocs describeNonemeaning “use filesystem/archive”, but other code paths appear to useSome(String::new())to force “no name”. Please document that explicitly here (or encode it as a dedicated enum/flag) to avoid accidental semantic drift later.Proposed doc tweak
/// Override values for ownership during permission operations. /// During creation: Override values stored in archive (None = use filesystem) /// During extraction: Override values restored from archive (None = use archive) +/// +/// Note: Some call sites may use `Some("")` as a sentinel meaning "do not store/use names" +/// (numeric-owner behavior). Prefer an explicit enum/flag if this grows further.Also applies to: 22-30
cli/src/command/stdio.rs (1)
651-686: Resolver wiring to(mode_strategy, owner_strategy)is clean; consider deduplicating boilerplate.The repeated
PermissionStrategyResolver { ... }.resolve()blocks across create/extract/append/update in this file are nearly identical; a small helper (e.g.,fn resolve_strategies(...)) would reduce future drift.Also applies to: 917-951, 1045-1078
cli/src/command/core.rs (2)
199-208: Resolver split is solid; consider making numeric-owner intent explicit (avoidSome("")).Also applies to: 259-316
711-781:apply_metadatais currently gated byOwnerStrategy(ModeStrategy unused here).If the long-term goal is “preserve mode but not owner”, you’ll likely want
apply_metadatato emit permission metadata whenmode_strategy == Preserveeven ifowner_strategy == Never(with clearly-defined placeholder owner fields). If that’s intentionally deferred, a short comment would help prevent future confusion.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cli/src/command/append.rscli/src/command/core.rscli/src/command/core/permission.rscli/src/command/create.rscli/src/command/extract.rscli/src/command/stdio.rscli/src/command/update.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- cli/src/command/update.rs
- cli/src/command/append.rs
🧰 Additional context used
🧬 Code graph analysis (1)
cli/src/command/extract.rs (3)
cli/src/utils/fs.rs (2)
lchown(49-67)chmod(70-80)cli/src/utils/os/windows/fs.rs (4)
lchown(25-36)lchown(68-68)lchown(69-69)chmod(39-47)cli/src/utils/os/unix/fs.rs (1)
chmod(10-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: tier1 (windows-latest, beta)
- GitHub Check: tier1 (macos-latest, nightly)
- GitHub Check: rust_check_doc
- GitHub Check: tier1 (macos-latest, nightly)
- GitHub Check: tier1 (ubuntu-22.04-arm, beta)
- GitHub Check: tier1 (macos-latest, stable)
- GitHub Check: Test WebAssembly (beta, wasm32-wasip1)
- GitHub Check: Test WebAssembly (beta, wasm32-wasip2)
- GitHub Check: tier1 (ubuntu-22.04-arm, stable)
- GitHub Check: Test WebAssembly (nightly-2025-12-09, wasm32-wasip1)
- GitHub Check: tier1 (windows-latest, beta)
- GitHub Check: tier1 (macos-latest, beta)
- GitHub Check: Test WebAssembly (stable, wasm32-unknown-unknown)
- GitHub Check: tier1 (ubuntu-latest, stable)
- GitHub Check: tier1 (ubuntu-22.04-arm, nightly)
- GitHub Check: Test WebAssembly (beta, wasm32-unknown-emscripten)
- GitHub Check: Test WebAssembly (stable, wasm32-unknown-emscripten)
- GitHub Check: msrv (ubuntu-latest, libpna)
- GitHub Check: msrv (ubuntu-latest, pna)
- GitHub Check: tier3_cross (redoxos/redoxer, x86_64-unknown-redox)
🔇 Additional comments (3)
cli/src/command/create.rs (1)
462-492: LGTM: creation now stores split strategies inKeepOptionsconsistently.cli/src/command/extract.rs (2)
873-895: Good: owner restored before mode (avoids setuid/setgid clobbering).
1052-1094: Theutils::fs::chmodfunction is correctly available and configured for Windows builds. Both Windows (usinglibc::wchmod) and Unix implementations exist incli/src/utils/os/{windows,unix}/fs.rsand are properly isolated with matching#[cfg(windows)]and#[cfg(unix)]attributes. The wrapper function incli/src/utils/fs.rs:70is gated with#[cfg(any(windows, unix))], and the usage inrestore_modematches this cfg attribute. No issues found.
1 participant
This refactoring separates mode handling from owner handling internally, enabling the fine-grained control required for bsdtar-style
-pflag semantics (which affects mode but not owner).Changes:
No user-facing changes in this refactoring phase.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.