Skip to content

Commit 5fb7fc8

Browse files
committed
♻️ Split PermissionStrategy into ModeStrategy + OwnerStrategy
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.
1 parent 9e79faa commit 5fb7fc8

7 files changed

Lines changed: 224 additions & 198 deletions

File tree

cli/src/command/append.rs

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,17 @@ fn append_to_archive(args: AppendCommand) -> anyhow::Result<()> {
372372
}
373373
let password = password.as_deref();
374374
let option = entry_option(args.compression, args.cipher, args.hash, password);
375+
let (mode_strategy, owner_strategy) = PermissionStrategyResolver {
376+
keep_permission: args.keep_permission,
377+
no_keep_permission: args.no_keep_permission,
378+
same_owner: true, // Must be `true` for creation
379+
uname: args.uname,
380+
gname: args.gname,
381+
uid: args.uid,
382+
gid: args.gid,
383+
numeric_owner: args.numeric_owner,
384+
}
385+
.resolve();
375386
let keep_options = KeepOptions {
376387
timestamp_strategy: TimestampStrategyResolver {
377388
keep_timestamp: args.keep_timestamp,
@@ -385,17 +396,8 @@ fn append_to_archive(args: AppendCommand) -> anyhow::Result<()> {
385396
clamp_atime: args.clamp_atime,
386397
}
387398
.resolve(),
388-
permission_strategy: PermissionStrategyResolver {
389-
keep_permission: args.keep_permission,
390-
no_keep_permission: args.no_keep_permission,
391-
same_owner: true, // Must be `true` for creation
392-
uname: args.uname,
393-
gname: args.gname,
394-
uid: args.uid,
395-
gid: args.gid,
396-
numeric_owner: args.numeric_owner,
397-
}
398-
.resolve(),
399+
mode_strategy,
400+
owner_strategy,
399401
xattr_strategy: XattrStrategy::from_flags(args.keep_xattr, args.no_keep_xattr),
400402
acl_strategy: AclStrategy::from_flags(args.keep_acl, args.no_keep_acl),
401403
fflags_strategy: FflagsStrategy::Never,

cli/src/command/core.rs

Lines changed: 48 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ pub(crate) mod time_filter;
88
pub(crate) mod timestamp;
99

1010
pub(crate) use self::path::PathnameEditor;
11-
pub(crate) use self::permission::{OwnerSource, PermissionStrategy};
11+
pub(crate) use self::permission::{ModeStrategy, OwnerOptions, OwnerStrategy};
1212
pub(crate) use self::timestamp::{TimeSource, TimestampStrategy};
1313
use crate::{
1414
cli::{CipherAlgorithmArgs, CompressionAlgorithmArgs, HashAlgorithmArgs},
@@ -199,7 +199,8 @@ impl MacMetadataStrategy {
199199
#[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
200200
pub(crate) struct KeepOptions {
201201
pub(crate) timestamp_strategy: TimestampStrategy,
202-
pub(crate) permission_strategy: PermissionStrategy,
202+
pub(crate) mode_strategy: ModeStrategy,
203+
pub(crate) owner_strategy: OwnerStrategy,
203204
pub(crate) xattr_strategy: XattrStrategy,
204205
pub(crate) acl_strategy: AclStrategy,
205206
pub(crate) fflags_strategy: FflagsStrategy,
@@ -255,11 +256,11 @@ impl TimestampStrategyResolver {
255256
}
256257
}
257258

258-
/// Resolves CLI permission options into a `PermissionStrategy`.
259+
/// Resolves CLI permission options into split mode and owner strategies.
259260
///
260261
/// This struct encapsulates the CLI-specific logic for determining permission behavior:
261-
/// - `no_keep_permission` forces `Never`
262-
/// - `keep_permission` enables `Preserve` with ownership handling
262+
/// - `no_keep_permission` forces both mode and owner to `Never`
263+
/// - `keep_permission` enables `Preserve` for mode, and ownership handling via `same_owner`
263264
/// - `same_owner` controls whether to restore ownership (extraction only)
264265
/// - Owner override fields (uid, gid, uname, gname) are used in both creation and extraction
265266
pub(crate) struct PermissionStrategyResolver {
@@ -274,21 +275,22 @@ pub(crate) struct PermissionStrategyResolver {
274275
}
275276

276277
impl PermissionStrategyResolver {
277-
/// Resolves CLI options to PermissionStrategy.
278+
/// Resolves CLI options to split (ModeStrategy, OwnerStrategy).
278279
///
279280
/// The `same_owner` field controls ownership handling:
280-
/// - `true`: Restore/store ownership (FromSource)
281-
/// - `false`: Skip ownership restoration (NoRestore, extraction only)
281+
/// - `true`: Restore/store ownership (Preserve with options)
282+
/// - `false`: Skip ownership restoration (Never)
282283
///
283284
/// For creation contexts, pass `same_owner: true` since ownership
284285
/// is always stored when `--keep-permission` is enabled.
285-
pub(crate) fn resolve(self) -> PermissionStrategy {
286+
pub(crate) fn resolve(self) -> (ModeStrategy, OwnerStrategy) {
286287
if self.no_keep_permission {
287-
PermissionStrategy::Never
288+
(ModeStrategy::Never, OwnerStrategy::Never)
288289
} else if self.keep_permission {
289-
PermissionStrategy::Preserve {
290-
owner: if self.same_owner {
291-
OwnerSource::FromSource {
290+
let mode_strategy = ModeStrategy::Preserve;
291+
let owner_strategy = if self.same_owner {
292+
OwnerStrategy::Preserve {
293+
options: OwnerOptions {
292294
uname: if self.numeric_owner {
293295
Some(String::new())
294296
} else {
@@ -301,13 +303,14 @@ impl PermissionStrategyResolver {
301303
},
302304
uid: self.uid,
303305
gid: self.gid,
304-
}
305-
} else {
306-
OwnerSource::NoRestore
307-
},
308-
}
306+
},
307+
}
308+
} else {
309+
OwnerStrategy::Never
310+
};
311+
(mode_strategy, owner_strategy)
309312
} else {
310-
PermissionStrategy::Never
313+
(ModeStrategy::Never, OwnerStrategy::Never)
311314
}
312315
}
313316
}
@@ -729,69 +732,51 @@ pub(crate) fn apply_metadata(
729732
}
730733
}
731734
#[cfg(unix)]
732-
if let PermissionStrategy::Preserve { ref owner } = keep_options.permission_strategy {
735+
if let OwnerStrategy::Preserve { options } = &keep_options.owner_strategy {
733736
use crate::utils::fs::{Group, User};
734737
use std::os::unix::fs::{MetadataExt, PermissionsExt};
735738

736739
let mode = meta.permissions().mode() as u16;
737-
// Extract owner overrides from OwnerSource
738-
let (o_uid, o_gid, o_uname, o_gname) = match owner {
739-
OwnerSource::NoRestore => unreachable!("NoRestore is not used during creation"),
740-
OwnerSource::FromSource {
741-
uid,
742-
gid,
743-
uname,
744-
gname,
745-
} => (*uid, *gid, uname.as_deref(), gname.as_deref()),
740+
// Get owner info: use overrides from OwnerStrategy if Preserve, else use filesystem values
741+
let uid = options.uid.unwrap_or(meta.uid());
742+
let gid = options.gid.unwrap_or(meta.gid());
743+
let uname = match &options.uname {
744+
None => User::from_uid(uid.into())?
745+
.name()
746+
.unwrap_or_default()
747+
.into(),
748+
Some(uname) => uname.clone(),
749+
};
750+
let gname = match &options.gname {
751+
None => Group::from_gid(gid.into())?
752+
.name()
753+
.unwrap_or_default()
754+
.into(),
755+
Some(gname) => gname.clone(),
746756
};
747-
let uid = o_uid.unwrap_or(meta.uid());
748-
let gid = o_gid.unwrap_or(meta.gid());
749757
entry.permission(pna::Permission::new(
750758
uid.into(),
751-
match o_uname {
752-
None => User::from_uid(uid.into())?
753-
.name()
754-
.unwrap_or_default()
755-
.into(),
756-
Some(uname) => uname.into(),
757-
},
759+
uname,
758760
gid.into(),
759-
match o_gname {
760-
None => Group::from_gid(gid.into())?
761-
.name()
762-
.unwrap_or_default()
763-
.into(),
764-
Some(gname) => gname.into(),
765-
},
761+
gname,
766762
mode,
767763
));
768764
}
769765
#[cfg(windows)]
770-
if let PermissionStrategy::Preserve { ref owner } = keep_options.permission_strategy {
766+
if let OwnerStrategy::Preserve { options } = &keep_options.owner_strategy {
771767
use crate::utils::os::windows::{fs::stat, security::SecurityDescriptor};
772768

773769
let sd = SecurityDescriptor::try_from(path)?;
774770
let stat = stat(sd.path.as_ptr() as _)?;
775771
let mode = stat.st_mode;
776772
let user = sd.owner_sid()?;
777773
let group = sd.group_sid()?;
778-
// Extract owner overrides from OwnerSource
779-
let (o_uid, o_gid, o_uname, o_gname) = match owner {
780-
OwnerSource::NoRestore => unreachable!("NoRestore is not used during creation"),
781-
OwnerSource::FromSource {
782-
uid,
783-
gid,
784-
uname,
785-
gname,
786-
} => (*uid, *gid, uname.clone(), gname.clone()),
787-
};
788-
entry.permission(pna::Permission::new(
789-
o_uid.map_or(u64::MAX, Into::into),
790-
o_uname.unwrap_or(user.name),
791-
o_gid.map_or(u64::MAX, Into::into),
792-
o_gname.unwrap_or(group.name),
793-
mode,
794-
));
774+
// Get owner info: use overrides from OwnerStrategy
775+
let uid = options.uid.map_or(u64::MAX, Into::into);
776+
let gid = options.gid.map_or(u64::MAX, Into::into);
777+
let uname = options.uname.clone().unwrap_or(user.name);
778+
let gname = options.gname.clone().unwrap_or(group.name);
779+
entry.permission(pna::Permission::new(uid, uname, gid, gname, mode));
795780
}
796781
// On macOS, when mac_metadata_strategy is Always, AppleDouble packing via copyfile()
797782
// already includes xattrs and ACLs. Skip separate handling to avoid duplication.

cli/src/command/core/permission.rs

Lines changed: 26 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,30 @@
1-
/// How to handle ownership during permission operations.
2-
#[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
3-
pub(crate) enum OwnerSource {
4-
/// Don't restore ownership (extraction only - same as !same_owner)
5-
/// During extraction: chmod() is called but lchown() is NOT
6-
NoRestore,
7-
/// Use ownership from source with optional overrides.
8-
/// During creation: Override values stored in archive (None = use filesystem)
9-
/// During extraction: Override values restored from archive (None = use archive)
10-
FromSource {
11-
uname: Option<String>,
12-
gname: Option<String>,
13-
uid: Option<u32>,
14-
gid: Option<u32>,
15-
},
1+
/// Override values for ownership during permission operations.
2+
/// During creation: Override values stored in archive (None = use filesystem)
3+
/// During extraction: Override values restored from archive (None = use archive)
4+
#[derive(Clone, Debug, Default, Eq, PartialEq, Ord, PartialOrd, Hash)]
5+
pub(crate) struct OwnerOptions {
6+
pub(crate) uname: Option<String>,
7+
pub(crate) gname: Option<String>,
8+
pub(crate) uid: Option<u32>,
9+
pub(crate) gid: Option<u32>,
1610
}
1711

18-
#[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
19-
pub(crate) enum PermissionStrategy {
12+
/// How to handle file mode bits (permissions).
13+
#[derive(Clone, Copy, Debug, Default, Eq, PartialEq, Ord, PartialOrd, Hash)]
14+
pub(crate) enum ModeStrategy {
15+
/// Don't preserve mode bits
16+
#[default]
2017
Never,
21-
/// Preserve permissions with configurable ownership handling
22-
Preserve {
23-
owner: OwnerSource,
24-
},
18+
/// Preserve mode bits from source
19+
Preserve,
20+
}
21+
22+
/// How to handle file ownership (uid/gid/uname/gname).
23+
#[derive(Clone, Debug, Default, Eq, PartialEq, Ord, PartialOrd, Hash)]
24+
pub(crate) enum OwnerStrategy {
25+
/// Don't restore ownership
26+
#[default]
27+
Never,
28+
/// Restore ownership with optional overrides
29+
Preserve { options: OwnerOptions },
2530
}

cli/src/command/create.rs

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -459,6 +459,17 @@ fn create_archive(args: CreateCommand) -> anyhow::Result<()> {
459459
if let Some(parent) = archive_path.parent() {
460460
fs::create_dir_all(parent)?;
461461
}
462+
let (mode_strategy, owner_strategy) = PermissionStrategyResolver {
463+
keep_permission: args.keep_permission,
464+
no_keep_permission: args.no_keep_permission,
465+
same_owner: true, // Must be `true` for creation
466+
uname: args.uname,
467+
gname: args.gname,
468+
uid: args.uid,
469+
gid: args.gid,
470+
numeric_owner: args.numeric_owner,
471+
}
472+
.resolve();
462473
let keep_options = KeepOptions {
463474
timestamp_strategy: TimestampStrategyResolver {
464475
keep_timestamp: args.keep_timestamp,
@@ -472,17 +483,8 @@ fn create_archive(args: CreateCommand) -> anyhow::Result<()> {
472483
clamp_atime: args.clamp_atime,
473484
}
474485
.resolve(),
475-
permission_strategy: PermissionStrategyResolver {
476-
keep_permission: args.keep_permission,
477-
no_keep_permission: args.no_keep_permission,
478-
same_owner: true, // Must be `true` for creation
479-
uname: args.uname,
480-
gname: args.gname,
481-
uid: args.uid,
482-
gid: args.gid,
483-
numeric_owner: args.numeric_owner,
484-
}
485-
.resolve(),
486+
mode_strategy,
487+
owner_strategy,
486488
xattr_strategy: XattrStrategy::from_flags(args.keep_xattr, args.no_keep_xattr),
487489
acl_strategy: AclStrategy::from_flags(args.keep_acl, args.no_keep_acl),
488490
fflags_strategy: FflagsStrategy::Never,

0 commit comments

Comments
 (0)