♻️ Consolidate OwnerOptions and same_owner into PermissionStrategy#2582
Conversation
📝 WalkthroughWalkthroughIntroduce a resolver-based permission/ownership flow: add Changes
("create_.rs" denotes related helper files in the create flow updated similarly.) Sequence DiagramsequenceDiagram
participant CLI as CLI Args
participant NameResolver as resolve_name_id
participant Resolver as PermissionStrategyResolver
participant Strategy as PermissionStrategy
participant Command as Command (create/extract/append/update)
Note right of CLI: flags (keep/no_keep, same_owner, uname/gname, uid/gid, numeric_owner)
CLI->>NameResolver: resolve uname/gname and uid/gid
NameResolver-->>CLI: (uname, gname, uid, gid)
CLI->>Resolver: construct with flags + resolved names/ids
activate Resolver
Resolver->>Strategy: .resolve()
deactivate Resolver
Strategy-->>Command: PermissionStrategy (+ OwnerSource)
alt Preserve with OwnerSource
Command->>Command: apply metadata/ownership from OwnerSource
else Never
Command->>Command: skip ownership restoration
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 significantly refactors the way file permissions and ownership are managed within the system. By introducing a unified 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cli/src/command/core.rs (2)
704-712: Consider replacingunreachable!()with a debug assertion or explicit error.During creation,
NoRestoreshould never occur sincesame_owner: trueis always passed. However,unreachable!()will panic at runtime if this invariant is violated. Consider usingdebug_assert!with a fallback, or returning an error for better resilience against future refactoring.🔎 Alternative approach
- OwnerSource::NoRestore => unreachable!("NoRestore is not used during creation"), + OwnerSource::NoRestore => { + debug_assert!(false, "NoRestore is not used during creation"); + // Fallback: use filesystem metadata + (None, None, None, None) + }
745-753: Same concern aboutunreachable!()in Windows path.The Windows permission handling has the same
unreachable!()forNoRestore. Applying the same resilience pattern here would be beneficial.
📜 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
🧰 Additional context used
🧬 Code graph analysis (3)
cli/src/command/extract.rs (4)
cli/src/utils/os/unix/fs/owner.rs (3)
from_uid(8-16)name(30-32)name(70-72)lib/src/entry/meta.rs (2)
uname(210-212)gname(240-242)cli/src/utils/fs.rs (1)
lchown(49-67)cli/src/utils/os/windows/fs.rs (3)
lchown(25-36)lchown(68-68)lchown(69-69)
cli/src/command/stdio.rs (1)
lib/src/entry/meta.rs (2)
uname(210-212)gname(240-242)
cli/src/command/create.rs (1)
cli/src/command/core.rs (3)
collect_items_from_paths(365-379)create_entry(609-650)entry_option(652-672)
⏰ 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). (11)
- GitHub Check: tier1 (windows-11-arm, stable)
- GitHub Check: tier1 (macos-latest, stable)
- GitHub Check: tier1 (ubuntu-22.04-arm, nightly)
- GitHub Check: tier1 (windows-latest, stable)
- GitHub Check: Test WebAssembly (nightly-2025-12-09, wasm32-unknown-unknown)
- GitHub Check: Test WebAssembly (nightly-2025-12-09, wasm32-wasip2)
- GitHub Check: tier1 (windows-11-arm, nightly)
- GitHub Check: tier1 (macos-latest, beta)
- GitHub Check: tier1 (windows-latest, beta)
- GitHub Check: tier1 (windows-11-arm, beta)
- GitHub Check: tier1 (windows-11-arm, stable)
🔇 Additional comments (14)
cli/src/command/core/permission.rs (1)
1-25: Well-structured permission types.The new
OwnerSourceandPermissionStrategyenums cleanly model the dual-mode semantics for creation vs. extraction. The documentation comments clearly explain how each variant behaves in different contexts.cli/src/command/core.rs (2)
166-173: KeepOptions no longer Copy due to String fields.The removal of
CopyfromKeepOptionsis correct sincePermissionStrategy::Preservenow containsOwnerSource::FromSourcewithOption<String>fields. This is a necessary breaking change for callers that relied on implicit copying.
251-279: PermissionStrategyResolver logic is correct.The resolver correctly handles the priority:
no_keep_permission→ Never,keep_permission→ Preserve with owner handling based onsame_owner. Thenumeric_ownerflag appropriately sets empty strings for uname/gname to suppress name storage.cli/src/command/append.rs (1)
388-402: Correct resolver usage for append operation.The
PermissionStrategyResolveris properly configured withsame_owner: truefor append operations, which is the expected behavior since ownership is always stored during creation. The ownership override fields (uname, gname, uid, gid, numeric_owner) are correctly passed through.cli/src/command/create.rs (2)
474-488: Consistent resolver pattern in create command.The
PermissionStrategyResolverusage follows the same pattern asappend.rs, withsame_owner: truefor creation operations. The comment appropriately documents that this field is ignored during creation.
524-529: Simplified CreationContext without owner_options.The removal of
owner_optionsfromCreationContextis a clean simplification. Ownership handling is now fully encapsulated inPermissionStrategyvia the resolver.cli/src/command/update.rs (1)
397-411: Update command follows same resolver pattern.The update command correctly uses
same_owner: truesince it creates new entries when updating the archive. Consistent with the other creation-oriented commands.cli/src/command/stdio.rs (3)
637-662: Proper ownership resolution for stdio create.The
resolve_name_idhelper correctly handles the precedence:--owner/--groupspec takes precedence over individual--uname/--uidand--gname/--gidflags. The resolver is then correctly configured withsame_owner: truefor creation.
745-755: Correct extraction behavior with same_owner flag.For extraction,
same_owner: !args.no_same_ownercorrectly inverts the--no-same-ownerflag to determine ownership restoration behavior. This aligns with tar-compatible CLI semantics.
994-1003: Clean helper for NameIdPair resolution.The
resolve_name_idfunction provides a clean abstraction for handling the precedence between the combined--owner/--groupspec and individual name/id flags.cli/src/command/extract.rs (4)
417-431: Correct extraction permission strategy.The
PermissionStrategyResolverfor extraction correctly usessame_owner: !args.no_same_owner, which respects the user's choice between--same-ownerand--no-same-ownerflags.
972-998: Well-designed ownership resolution with clear priority.The
resolve_ownerfunction correctly implements the priority order documented in the comments:
- Override uid/gid if specified
- Override uname/gname if specified
- Archive's uname/gname with fallback to uid/gid
This matches the expected tar-compatible behavior.
1001-1048: Correct conditional ownership restoration.The
restore_permissionsfunction correctly:
- Only calls
lchownwhenOwnerSource::FromSource(i.e., when--same-owneris effective)- Always restores mode permissions via
chmodregardless of ownership settingThis separation ensures that file modes are preserved even when ownership restoration is skipped.
881-885: Pattern match correctly gates permission restoration.The
PermissionStrategy::Preservepattern match ensures permissions are only restored when the strategy is enabled, and theownerreference is correctly extracted for use inrestore_permissions.
![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 refactors the handling of file ownership and permissions by replacing the OwnerOptions struct with a new PermissionStrategyResolver struct and OwnerSource enum. The PermissionStrategy enum was updated to include ownership details, leading to widespread changes across CLI commands (append, create, extract, stdio, update) to utilize this new resolution mechanism. The CreateOptions and OutputOption structs, along with metadata application and restoration functions, were updated to remove direct OwnerOptions usage. Review comments pointed out a misleading comment for same_owner in creation contexts, suggesting it should explicitly state that same_owner: true is mandatory to prevent panics, and recommended refactoring the restore_permissions function to centralize duplicated ownership restoration logic between Unix and Windows platform-specific code for improved maintainability.
ChanTsune
force-pushed
the
cli/refactor/permissions
branch
from
b942572 to
7f26760
Compare
Refactor permission/ownership handling to follow the TimestampStrategy
pattern, creating a single source of truth for all permission-related
configuration.
Changes:
- Add OwnerSource enum with NoRestore and FromSource variants
- Change PermissionStrategy from Never/Always to Never/Preserve{owner}
- Add PermissionStrategyResolver with context-specific resolution:
- resolve_for_creation(): always FromSource (ownership stored)
- resolve_for_extraction(): respects same_owner flag
- Remove OwnerOptions struct and owner_options from CreateOptions
- Update all command modules to use the new resolver pattern
This consolidation eliminates the scattered ownership concerns
(OwnerOptions struct, same_owner boolean, PermissionStrategy enum)
into a unified architecture where ownership handling is embedded
within PermissionStrategy.
- Add chmod wrapper in utils/fs.rs following lchown pattern - Consolidate duplicate Unix/Windows chmod calls in extract.rs
ChanTsune
force-pushed
the
cli/refactor/permissions
branch
from
7f26760 to
849cb43
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cli/src/command/core.rs:
- Line 738: The match arm using OwnerSource::NoRestore currently calls
unreachable!() which can panic at runtime because resolve() may produce
NoRestore when same_owner: false; instead, handle this case explicitly by
returning an Err variant (or Propagate a Creation-specific error) from the
surrounding function rather than panicking: locate the match on OwnerSource in
functions that call resolve() (e.g., where OwnerSource::NoRestore is matched)
and replace unreachable!("NoRestore is not used during creation") with a
controlled error return (e.g., return Err(Error::InvalidOwnerState("NoRestore in
creation context")) or map it into the existing Result error type), or introduce
a context-specific resolve method that never yields NoRestore and use that where
creation is expected.
🧹 Nitpick comments (1)
cli/src/command/core.rs (1)
731-794: Reduce duplication in owner override extraction.The logic for extracting owner overrides from
OwnerSourceis duplicated between the Unix block (lines 737-745) and Windows block (lines 778-786). Consider extracting this into a helper function.Refactoring suggestion
+fn extract_owner_overrides(owner: &OwnerSource) -> (Option<u32>, Option<u32>, Option<&str>, Option<&str>) { + match owner { + OwnerSource::NoRestore => unreachable!("NoRestore is not used during creation"), + OwnerSource::FromSource { uid, gid, uname, gname } => { + (*uid, *gid, uname.as_deref(), gname.as_deref()) + } + } +} + #[cfg(unix)] if let PermissionStrategy::Preserve { ref owner } = keep_options.permission_strategy { use crate::utils::fs::{Group, User}; use std::os::unix::fs::{MetadataExt, PermissionsExt}; let mode = meta.permissions().mode() as u16; - // Extract owner overrides from OwnerSource - let (o_uid, o_gid, o_uname, o_gname) = match owner { - OwnerSource::NoRestore => unreachable!("NoRestore is not used during creation"), - OwnerSource::FromSource { uid, gid, uname, gname } => { - (*uid, *gid, uname.as_deref(), gname.as_deref()) - } - }; + let (o_uid, o_gid, o_uname, o_gname) = extract_owner_overrides(owner); // ... rest of the code } #[cfg(windows)] if let PermissionStrategy::Preserve { ref owner } = keep_options.permission_strategy { // ... setup code ... - let (o_uid, o_gid, o_uname, o_gname) = match owner { - OwnerSource::NoRestore => unreachable!("NoRestore is not used during creation"), - OwnerSource::FromSource { uid, gid, uname, gname } => { - (*uid, *gid, uname.clone(), gname.clone()) - } - }; + let (o_uid, o_gid, o_uname, o_gname) = extract_owner_overrides(owner); + let o_uname = o_uname.map(String::from); + let o_gname = o_gname.map(String::from); // ... rest of the code }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
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.rscli/src/utils/fs.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/src/command/core/permission.rs
- cli/src/utils/fs.rs
- cli/src/command/stdio.rs
🧰 Additional context used
🧬 Code graph analysis (2)
cli/src/command/append.rs (1)
cli/src/command/core.rs (2)
create_entry(642-683)entry_option(685-705)
cli/src/command/create.rs (1)
cli/src/command/core.rs (3)
collect_items_from_paths(398-412)create_entry(642-683)entry_option(685-705)
⏰ 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: build-local-artifacts (x86_64-unknown-linux-gnu)
- GitHub Check: rust_check_doc
- GitHub Check: Test WebAssembly (nightly-2025-12-09, wasm32-wasip1)
- GitHub Check: tier1 (windows-latest, nightly)
- GitHub Check: tier1 (windows-11-arm, stable)
- GitHub Check: tier1 (ubuntu-22.04-arm, beta)
- GitHub Check: tier1 (windows-latest, beta)
- GitHub Check: tier1 (ubuntu-22.04-arm, nightly)
- GitHub Check: tier1 (ubuntu-latest, stable)
- GitHub Check: tier1 (windows-latest, stable)
- GitHub Check: tier1 (ubuntu-latest, beta)
- GitHub Check: Test WebAssembly (stable, wasm32-unknown-emscripten)
- GitHub Check: msrv (ubuntu-latest, libpna)
- GitHub Check: Test WebAssembly (nightly-2025-12-09, wasm32-unknown-emscripten)
- GitHub Check: msrv (ubuntu-latest, portable-network-archive)
- GitHub Check: Test WebAssembly (stable, wasm32-wasip2)
- GitHub Check: Test WebAssembly (beta, wasm32-unknown-unknown)
- GitHub Check: Test WebAssembly (beta, wasm32-wasip1)
- GitHub Check: Test WebAssembly (stable, wasm32-wasip1)
- GitHub Check: Test WebAssembly (beta, wasm32-unknown-emscripten)
🔇 Additional comments (6)
cli/src/command/append.rs (1)
388-398: LGTM! Permission resolver correctly configured for creation context.The
PermissionStrategyResolveris properly initialized withsame_owner: true, which is required for the creation/append flow. The comment clarifies this requirement, and all ownership override fields (uname, gname, uid, gid, numeric_owner) are correctly threaded through.cli/src/command/update.rs (1)
397-407: LGTM! Consistent with creation context requirements.The resolver configuration mirrors the append command correctly, with
same_owner: trueas required for creation contexts.cli/src/command/create.rs (1)
475-485: LGTM! Past review feedback addressed.The comment now correctly clarifies that
same_ownermust betruefor creation, addressing the previous review concern about misleading documentation.cli/src/command/extract.rs (3)
417-427: LGTM! Extraction context correctly handles same_owner flag.Unlike the creation commands, the extraction path correctly uses
same_owner: !args.no_same_owner, respecting the user's choice about whether to restore ownership. This is the appropriate behavior for extraction contexts.
1015-1041: LGTM! Clear ownership resolution logic.The
resolve_ownerfunction implements a clear priority hierarchy:
- Override uid/gid if specified
- Override uname/gname if specified, searching by name
- Archive's uname/gname with fallback to archive's uid/gid
This makes the resolution logic explicit and maintainable.
1044-1072: LGTM! Past review feedback addressed.The DRY concern from the previous review has been addressed. The ownership restoration logic is now unified (lines 1048-1062) with a single call to
chmodat line 1064, eliminating the previous duplication between Unix and Windows blocks.
1 participant
Refactor permission/ownership handling to follow the TimestampStrategy pattern, creating a single source of truth for all permission-related configuration.
Changes:
This consolidation eliminates the scattered ownership concerns (OwnerOptions struct, same_owner boolean, PermissionStrategy enum) into a unified architecture where ownership handling is embedded within PermissionStrategy.
Summary by CodeRabbit
Refactor
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.