Cli/stdio/mac metadata#2575
Conversation
📝 WalkthroughWalkthroughAdds macOS AppleDouble support: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as "pna CLI"
participant Core as "command::core"
participant FS as "copyfile (macOS)"
participant Archive
User->>CLI: pna --mac-metadata --unstable create ...
CLI->>CLI: parse flags → MacMetadataStrategy::Always
CLI->>Core: CreateOptions { mac_metadata_strategy: Always, ... }
Core->>Core: collect metadata for entry
Core->>FS: pack_apple_double(source_path)
activate FS
FS->>FS: copyfile(PACK) → write temp file → read bytes
FS-->>Core: appleDouble bytes
deactivate FS
Core->>Archive: append entry + attach `maMd` chunk with metadata bytes
Core-->>User: archive written
sequenceDiagram
autonumber
participant User
participant CLI as "pna CLI"
participant Core as "command::core"
participant FS as "copyfile (macOS)"
participant Disk
User->>CLI: pna --mac-metadata --unstable extract archive
CLI->>CLI: parse flags → MacMetadataStrategy::Always
CLI->>Core: OutputOptions { mac_metadata_strategy: Always, ... }
Core->>Core: extract_entry -> locate `maMd` chunk
Core->>FS: unpack_apple_double(metadata_bytes, target_path)
activate FS
FS->>FS: write temp -> copyfile(UNPACK) -> restore xattr/acl
FS-->>Core: success / error
deactivate FS
Core-->>User: extraction complete (with restored mac metadata)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 introduces robust support for handling macOS-specific metadata, such as extended attributes, ACLs, and resource forks, within archives. It achieves this by implementing a new 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 introduces support for archiving and extracting macOS-specific metadata (like extended attributes, ACLs, and resource forks) using the AppleDouble format. This is achieved by leveraging the copyfile() system call on macOS and introducing a new private chunk type maMd. The feature is exposed through new --mac-metadata and --no-mac-metadata flags under the experimental stdio command.
The implementation is comprehensive, including FFI bindings, logic for packing/unpacking metadata, and integration tests. However, I've identified two critical issues where this new functionality leads to redundant storage and restoration of extended attributes and ACLs, as they are handled by both the new AppleDouble mechanism and the existing specific handlers. I've also included a couple of medium-severity suggestions to improve code style and maintainability in the flag resolution logic.
![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 (3)
cli/tests/cli/stdio/option_mac_metadata.rs (2)
268-276: Consider adding error message context on assertion failure.The assertions at lines 274 and 276 could benefit from descriptive messages to aid debugging if they fail.
🔎 Suggested improvement
- assert!(output.status.success()); - let extracted_value = String::from_utf8_lossy(&output.stdout); - assert!(extracted_value.trim() == xattr_value); + assert!( + output.status.success(), + "xattr command failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + let extracted_value = String::from_utf8_lossy(&output.stdout); + assert_eq!( + extracted_value.trim(), + xattr_value, + "xattr value mismatch" + );
11-26: Tests lack cleanup of created files.The tests create files in the current working directory but don't clean them up afterward. Consider using
tempfileor adding cleanup in aDropguard to avoid polluting the test environment.cli/src/utils/os/unix/fs.rs (1)
125-157: Consider more robust temp file handling.The current implementation uses a manually constructed temp file path. While functional, this approach has minor edge cases:
- The temp directory might not be writable
- If the process crashes mid-operation, orphan temp files remain
The current approach is acceptable for this use case, but you might consider using
tempfilecrate for more robust handling in the future.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cli/src/chunk.rscli/src/chunk/mac_metadata.rscli/src/command/append.rscli/src/command/core.rscli/src/command/create.rscli/src/command/extract.rscli/src/command/stdio.rscli/src/command/update.rscli/src/ext.rscli/src/utils/os/unix/fs.rscli/tests/cli/stdio.rscli/tests/cli/stdio/option_mac_metadata.rs
🧰 Additional context used
🧬 Code graph analysis (5)
cli/src/chunk.rs (1)
cli/src/ext.rs (2)
mac_metadata(18-18)mac_metadata(65-70)
cli/src/command/append.rs (1)
cli/src/command/core.rs (2)
create_entry(636-711)entry_option(713-733)
cli/src/chunk/mac_metadata.rs (1)
lib/src/chunk/types.rs (1)
from_unchecked(225-227)
cli/src/command/extract.rs (2)
cli/src/command/core.rs (1)
collect_split_archives(581-598)cli/src/utils/os/unix/fs.rs (2)
copyfile(114-120)unpack_apple_double(160-195)
cli/src/command/create.rs (1)
cli/src/command/core.rs (2)
create_entry(636-711)entry_option(713-733)
🔇 Additional comments (17)
cli/tests/cli/stdio.rs (1)
6-6: LGTM!The new test module import aligns with the existing test organization and supports testing of the mac metadata CLI options.
cli/src/chunk.rs (1)
3-3: LGTM!The module declaration and re-export follow the existing pattern used for
aclandfflagmodules, properly exposing the macOS metadata chunk type through the public API.Also applies to: 7-7
cli/src/command/append.rs (2)
10-10: LGTM!The import of
MacMetadataStrategyis correctly added to support the new mac metadata handling infrastructure.
423-423: LGTM!Initializing
mac_metadata_strategytoNeveris appropriate for the append command, maintaining consistency with the existing architecture where append operations typically preserve the original archive structure without adding new metadata types by default.cli/src/command/update.rs (2)
14-14: LGTM!The import of
MacMetadataStrategycorrectly integrates the new mac metadata handling into the update command.
432-432: LGTM!Initializing
mac_metadata_strategytoNeveris appropriate for the update command, consistent with the append command's behavior and maintaining backward compatibility.cli/src/chunk/mac_metadata.rs (1)
1-10: LGTM!The chunk type definition is well-structured:
- The documentation clearly explains the AppleDouble format purpose and PNA naming convention semantics.
- The
unsafeusage infrom_uncheckedis justified because the hardcoded byte stringb"maMd"is a compile-time constant that adheres to the documented naming convention by construction.- The chunk type naming follows the PNA convention correctly: ancillary, private, reserved, safe-to-copy.
cli/src/ext.rs (1)
63-70: LGTM!The
mac_metadata()implementation follows the established pattern used byfflags()and correctly scansextra_chunksfor themaMdchunk type. The use offind()withmap()is idiomatic and efficient.cli/src/command/create.rs (1)
554-560: LGTM!Hardcoding
MacMetadataStrategy::Neveris appropriate here since the regularcreatecommand doesn't expose--mac-metadataflags. The experimental mac metadata feature is only available through thestdiocommand path.cli/src/command/extract.rs (2)
945-971: LGTM!The macOS metadata restoration logic is well-implemented:
- Uses
unpack_apple_doubleon macOS with appropriate logging- Gracefully degrades with a warning on non-macOS platforms when metadata is present
- Failures are logged as warnings rather than hard errors, allowing extraction to continue
This matches the pattern used for other metadata types (xattrs, ACLs).
449-449: Consistent with create.rs - mac_metadata_strategy hardcoded to Never.The regular extract command doesn't expose
--mac-metadataflags, so hardcodingNeveris appropriate. Only the stdio command provides access to this experimental feature.cli/src/command/stdio.rs (2)
1073-1107: Clear precedence logic for xattr/ACL resolution.The resolution functions correctly implement a clear precedence:
- Explicit
--keep-xattr/--no-keep-xattrflags take precedence- On macOS,
--mac-metadata/--no-mac-metadataact as defaults- Platform-aware: non-macOS ignores mac_metadata flags
This allows users fine-grained control while
--mac-metadataprovides a convenient shorthand on macOS.
207-218: LGTM - flags properly gated behind --unstable.Both
--mac-metadataand--no-mac-metadatacorrectly require--unstable, and the ArgGroup at line 65 ensures mutual exclusivity. The help text clearly describes the feature.cli/src/command/core.rs (3)
182-212: Well-designed platform-aware enum.The
MacMetadataStrategyenum is well-designed:
- Uses
#[derive(Default)]withNeveras the default variant- Platform-aware
from_flags: on non-macOS, always returnsNeverregardless of flags, preventing users from enabling a feature that won't work- Clear documentation explaining the AppleDouble format purpose
875-908: Robust error handling for mac metadata packing.The implementation handles errors gracefully:
- Empty AppleDouble data is skipped (no metadata to pack)
NotFounderrors are silently ignored (file simply has no mac metadata)- Other errors log a warning but don't fail the archive operation
- Debug logging with byte count aids troubleshooting
This mirrors the pattern used for other metadata types and ensures archive creation succeeds even when some metadata can't be captured.
654-708: mac_metadata_strategy correctly propagated through all entry types.The strategy is passed to
apply_metadatafor allStoreAsvariants (Hardlink, Symlink, File, Dir), ensuring consistent metadata handling across entry types.cli/src/utils/os/unix/fs.rs (1)
114-121: Copyfile signature matches macOS SDK.The declaration is correct:
flags: u32properly maps tocopyfile_flags_t(typedef'd asuint32_tin the SDK). All parameters match the official macOS copyfile signature:fromandtoare*const libc::c_char(const char*), and the return type islibc::c_int(int).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ChanTsune
force-pushed
the
cli/stdio/mac-metadata
branch
from
fae5af4 to
21ac20a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @cli/src/utils/os/unix/fs.rs:
- Around line 174-175: The temp file can be left on disk if CString::new for
c_target fails after c_temp succeeded; ensure the temp file is always removed on
all early-return paths by either constructing both CStrings (CString::new for
temp_file and target_path) before creating/writing the temp_file, or wrap the
temp file lifecycle in a scope guard/cleanup that calls
std::fs::remove_file(temp_file) on error; adjust the code around temp_file,
CString::new, c_temp and c_target to guarantee removal of the temp_file when any
subsequent step (including creating c_target) returns Err.
- Around line 159-195: The temp file created in unpack_apple_double is not
removed if writing fails; ensure temp_file is cleaned up on any early return by
creating the temp file, writing, and the subsequent copy in a closure or block
that returns io::Result and then always calling fs::remove_file(&temp_file) in a
finally-like place (e.g., use a match/if let on the closure's result or a
scopeguard/Drop helper) so that failures from file.write_all or any other error
still trigger removal of temp_file; refer to unpack_apple_double, temp_file,
file.write_all, and the existing fs::remove_file call to implement this
guaranteed cleanup.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/command/stdio.rs (1)
681-691: Critical: Mac metadata strategy not propagated in create flow.The
run_create_archivefunction constructsCreationContextbut does not includemac_metadata_strategy. Whencreate_archive_fileis called (line 693 or 699), it hardcodesmac_metadata_strategy: MacMetadataStrategy::Never(seecli/src/command/create.rsline 559). This means the--mac-metadataand--no-mac-metadataflags parsed at lines 207-218 are ignored during archive creation via the stdio interface.In contrast, the extract (line 784-787), append (line 947-950), and update (line 1086-1089) paths correctly propagate the strategy using
MacMetadataStrategy::from_flags(args.mac_metadata, args.no_mac_metadata).Fix: Either add
mac_metadata_strategyto theCreationContextstruct and pass it through, or constructCreateOptionsdirectly inrun_create_archivewith the resolved strategy before callingcreate_archive_file.
♻️ Duplicate comments (3)
cli/src/command/core.rs (1)
875-908: Critical: Duplicate metadata when using MacMetadataStrategy::Always on macOS.The past review comment is still valid. When
mac_metadata_strategyisAlwayson macOS,pack_apple_doubleincludes extended attributes and ACLs in themaMdchunk. However, the existing xattr handling (lines 837-853) and ACL handling (lines 805-835) also run independently, resulting in duplicate data in the archive.To fix this, skip the separate xattr and ACL handling on macOS when
mac_metadata_strategyisAlways:🔎 Proposed fix
For xattrs (around line 837):
#[cfg(unix)] if let XattrStrategy::Always = keep_options.xattr_strategy { + #[cfg(target_os = "macos")] + if mac_metadata_strategy == MacMetadataStrategy::Always { + // Skip: xattrs are already packed in maMd chunk + } else { + match utils::os::unix::fs::xattrs::get_xattrs(path) { + // ... existing xattr logic ... + } + } + #[cfg(not(target_os = "macos"))] match utils::os::unix::fs::xattrs::get_xattrs(path) { - Ok(xattrs) => { - for attr in xattrs { - entry.add_xattr(attr); - } - } - Err(e) if e.kind() == std::io::ErrorKind::Unsupported => { - log::warn!( - "Extended attributes are not supported on filesystem for '{}': {}", - path.display(), - e - ); - } - Err(e) => return Err(e), + // ... existing xattr logic ... } }Similar changes needed for ACLs (around line 813).
cli/src/command/extract.rs (2)
906-918: Address potential conflict with macOS AppleDouble restoration.When
mac_metadata_strategyisAlwayson macOS (lines 945-971),unpack_apple_doubleusescopyfile()withCOPYFILE_XATTRandCOPYFILE_ACLflags, which restores extended attributes and ACLs from the AppleDouble format. However, this block (lines 906-918) unconditionally applies xattrs whenXattrStrategy::Always, and line 924 unconditionally restores ACLs when enabled.This can lead to:
- Redundant operations (AppleDouble already restored them)
- Conflicts if the individually-stored xattrs/ACLs differ from those in the AppleDouble blob
- Potential overwrites where the later restoration replaces the earlier one
Recommendation: Skip individual xattr restoration on macOS when
mac_metadata_strategyisAlways:Proposed fix
#[cfg(unix)] if let XattrStrategy::Always = keep_options.xattr_strategy { + #[cfg(target_os = "macos")] + let should_skip = matches!(mac_metadata_strategy, MacMetadataStrategy::Always); + #[cfg(not(target_os = "macos"))] + let should_skip = false; + + if !should_skip { match utils::os::unix::fs::xattrs::set_xattrs(path, item.xattrs()) { Ok(()) => {} Err(e) if e.kind() == io::ErrorKind::Unsupported => { log::warn!( "Extended attributes are not supported on filesystem for '{}': {}", path.display(), e ); } Err(e) => return Err(e), } + } }Apply similar logic for ACL restoration at line 924.
Based on past review comments from gemini-code-assist[bot].
964-971: Optional: Simplify nested cfg blocks.The nested
#[cfg(not(target_os = "macos"))]and conditional check could potentially be collapsed for readability, though the current structure is clear.Alternative structure
-#[cfg(not(target_os = "macos"))] -if let MacMetadataStrategy::Always = mac_metadata_strategy { - if item.mac_metadata().is_some() { - log::warn!( - "macOS metadata present but cannot be restored on this platform: '{}'", - path.display() - ); - } -} +#[cfg(not(target_os = "macos"))] +if matches!(mac_metadata_strategy, MacMetadataStrategy::Always) && item.mac_metadata().is_some() { + log::warn!( + "macOS metadata present but cannot be restored on this platform: '{}'", + path.display() + ); +}Related to static analysis hints from github-advanced-security[bot].
🧹 Nitpick comments (1)
cli/src/utils/os/unix/fs.rs (1)
125-157: Consider handling potential temp file collisions.While the combination of process ID and atomic counter makes collisions unlikely, there's no explicit check or retry mechanism if the temp file already exists. If this function is called concurrently across multiple processes with the same PID (e.g., after a PID wrap), collisions could occur.
Consider either:
- Using
O_EXCLsemantics viaOpenOptions::new().create_new(true)to fail fast on collision- Adding a retry loop with a different counter value
That said, this is a minor edge case and the current implementation is acceptable for typical usage.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cli/src/chunk.rscli/src/chunk/mac_metadata.rscli/src/command/append.rscli/src/command/core.rscli/src/command/create.rscli/src/command/extract.rscli/src/command/stdio.rscli/src/command/update.rscli/src/ext.rscli/src/utils/os/unix/fs.rscli/tests/cli/stdio.rscli/tests/cli/stdio/option_mac_metadata.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- cli/tests/cli/stdio/option_mac_metadata.rs
- cli/src/command/append.rs
🧰 Additional context used
🧬 Code graph analysis (6)
cli/src/chunk.rs (1)
cli/src/ext.rs (2)
mac_metadata(18-18)mac_metadata(65-70)
cli/src/command/create.rs (1)
cli/src/command/core.rs (1)
create_entry(636-711)
cli/src/command/core.rs (2)
cli/src/ext.rs (2)
mac_metadata(18-18)mac_metadata(65-70)cli/src/utils/os/unix/fs.rs (2)
copyfile(114-120)pack_apple_double(125-157)
cli/src/command/stdio.rs (1)
cli/src/command/core.rs (6)
from_flags(117-125)from_flags(135-143)from_flags(153-161)from_flags(171-179)from_flags(198-206)from_flags(209-211)
cli/src/command/extract.rs (1)
cli/src/utils/os/unix/fs.rs (2)
copyfile(114-120)unpack_apple_double(160-195)
cli/src/chunk/mac_metadata.rs (1)
lib/src/chunk/types.rs (1)
from_unchecked(225-227)
🔇 Additional comments (13)
cli/src/chunk/mac_metadata.rs (1)
1-10: LGTM! Chunk type definition follows PNA conventions.The
unsafeusage withfrom_uncheckedis appropriate here since the byte pattern*b"maMd"is statically defined and follows the documented PNA chunk naming convention. The documentation clearly explains the semantic meaning of each character's case, which aligns with the ancillary, private chunk specification.cli/src/utils/os/unix/fs.rs (1)
89-121: LGTM! FFI setup is correct.The copyfile FFI declaration and flag constants match the macOS copyfile API. Using
AtomicU64withRelaxedordering for the counter is appropriate since we only need uniqueness, not ordering guarantees.cli/src/command/core.rs (3)
312-312: LGTM! Field addition is correct.The
mac_metadata_strategyfield is properly added toCreateOptionsand will be propagated through entry creation.
636-710: LGTM! Consistent propagation of mac_metadata_strategy.All
StoreAsvariants (Hardlink, Symlink, File, Dir) correctly passmac_metadata_strategytoapply_metadata. The implementation is consistent across all paths.
182-212: CLI flags are properly wired and the Always variant is reachable.The search results confirm that
MacMetadataStrategy::from_flags()is called incli/src/command/stdio.rs(lines 784, 947, 1086) to properly instantiate the strategy based on CLI arguments. The test suite incli/tests/cli/stdio/option_mac_metadata.rsincludes comprehensive test coverage exercising both--mac-metadataand--no-mac-metadataflags across create, extract, append, and update modes.The
Alwaysvariant is actively used in the codebase atcli/src/command/core.rs:877andcli/src/command/extract.rs:947(within#[cfg(target_os = "macos")]gates). Static analysis tools may report the variant as unreachable due to not fully understanding platform-specific conditional compilation or test execution paths. On macOS, the testsstdio_mac_metadata_preserves_xattrs()andstdio_no_mac_metadata_excludes_xattrs()exercise the actual metadata handling.Note: Other command modules (create.rs, append.rs, update.rs, extract.rs) currently hardcode
MacMetadataStrategy::Never, which means the flag functionality is only available via thestdiocommand. This is the current design, not an oversight in the reviewed code.cli/tests/cli/stdio.rs (1)
6-6: LGTM! Test module declaration added.The new test module for mac_metadata options is properly declared alongside existing test modules.
cli/src/chunk.rs (1)
3-7: LGTM! Module wiring follows established pattern.The
mac_metadatamodule is properly declared and re-exported, following the same pattern as the existingaclandfflagmodules.cli/src/ext.rs (1)
64-70: LGTM!The implementation follows the same pattern as
acl()andfflags()methods, correctly scanning extra chunks for themaMdchunk type and returning its data.cli/src/command/create.rs (1)
559-559: Hardcoded to Never in create command.The
createcommand hardcodesmac_metadata_strategytoNever, while the stdio interface supports--mac-metadataflags. This appears intentional for this PR, limiting Mac metadata support to the stdio (tar-style) interface initially.cli/src/command/update.rs (1)
432-432: Consistent with create command pattern.Like the
createcommand, the standaloneupdatecommand hardcodesmac_metadata_strategytoNever, while stdio interface support is provided separately.cli/src/command/extract.rs (2)
449-449: Consistent with standalone command pattern.The standalone
extractcommand hardcodesmac_metadata_strategytoNever, matching the pattern used increateandupdatecommands. Mac metadata support is provided through the stdio interface, which correctly usesMacMetadataStrategy::from_flags()(seecli/src/command/stdio.rslines 784-787).
886-888: Updated documentation reflects new functionality.The docstring accurately describes the metadata restoration behavior including the new macOS AppleDouble support.
cli/src/command/stdio.rs (1)
207-218: Flags work consistently across platforms as designed.The
MacMetadataStrategy::from_flagsimplementation correctly handles platform differences: on macOS, it processes the flags and can returnAlwaysorNever; on non-macOS platforms, it ignores the flag values and always returnsNever(lines 197-211 incli/src/command/core.rs). Both flags properly require theunstablefeature across all platforms, and the runtime handling inapply_metadata(lines ~265-271 incli/src/command/core.rs) logs a warning if someone attempts to use--mac-metadataon non-macOS systems. The behavior is intentional and appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cli/tests/cli/stdio/option_mac_metadata.rs (1)
273-275: Consider usingassert_eq!for clearer test failure messages.The current assertion works correctly, but
assert_eq!would provide more informative output if the test fails by showing both the expected and actual values.🔎 Proposed refactor
- assert!(output.status.success()); - let extracted_value = String::from_utf8_lossy(&output.stdout); - assert!(extracted_value.trim() == "test_value"); + assert!(output.status.success()); + let extracted_value = String::from_utf8_lossy(&output.stdout); + assert_eq!(extracted_value.trim(), "test_value");
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/tests/cli/stdio/option_mac_metadata.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). (16)
- GitHub Check: Run cargo fuzz (split_archive)
- GitHub Check: Run cargo fuzz (aes_cbc)
- GitHub Check: tier1 (windows-latest, beta)
- GitHub Check: tier1 (macos-latest, stable)
- GitHub Check: Analyze (rust)
- GitHub Check: Run rust-clippy analyzing
- GitHub Check: tier1 (windows-latest, beta)
- GitHub Check: Test WebAssembly (nightly-2025-12-09, wasm32-unknown-emscripten)
- GitHub Check: Test WebAssembly (beta, wasm32-unknown-unknown)
- GitHub Check: Test WebAssembly (nightly-2025-12-09, wasm32-wasip2)
- GitHub Check: Test WebAssembly (stable, wasm32-wasip2)
- GitHub Check: Test WebAssembly (stable, wasm32-wasip1)
- GitHub Check: Test WebAssembly (stable, wasm32-unknown-unknown)
- GitHub Check: Test WebAssembly (beta, wasm32-unknown-emscripten)
- GitHub Check: Test WebAssembly (stable, wasm32-unknown-emscripten)
- GitHub Check: tier3_cross (redoxos/redoxer, x86_64-unknown-redox)
🔇 Additional comments (2)
cli/tests/cli/stdio/option_mac_metadata.rs (2)
12-202: LGTM!The test coverage for CLI flag acceptance and validation is thorough and well-structured. The tests appropriately verify:
- Flag acceptance for create, extract, append, and update modes
- The requirement that --mac-metadata needs --unstable
- Mutual exclusivity of --mac-metadata and --no-mac-metadata
The test isolation is good with unique file/directory names for each test case.
282-343: LGTM!This test correctly verifies that
--no-mac-metadataexcludes extended attributes from the archive. The approach of creating without metadata but attempting to extract with--mac-metadatais a good way to confirm nothing was stored.
ChanTsune
force-pushed
the
cli/stdio/mac-metadata
branch
from
9de410e to
d5b1ee9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cli/src/command/extract.rs (1)
945-971: Consider skipping individual xattr/ACL restoration when AppleDouble is used.When
MacMetadataStrategy::Alwaysis active on macOS, theunpack_apple_doublecall restores xattrs and ACLs viacopyfile(). However, the individual xattr restoration (lines 906-918) and ACL restoration (line 924) still run before this, resulting in redundant operations that get overwritten.While not incorrect, this could be optimized by skipping the individual restorations when AppleDouble handling is enabled on macOS.
cli/src/command/core.rs (1)
875-908: Critical: Data duplication when Mac metadata is enabled.When
mac_metadata_strategyisAlwayson macOS, this logic packs extended attributes and ACLs into themaMdchunk viacopyfile()withCOPYFILE_XATTR | COPYFILE_ACLflags. However, if--mac-metadataalso enables--keep-xattrand--keep-aclby default (as mentioned in past comments), the existing xattr logic (lines 837-853) and ACL logic (lines 805-835) will also execute, resulting in duplicate storage of xattrs and ACLs—once in their specific chunks (xAtT,faCl,faCe) and again within themaMdchunk.To prevent duplication, skip the xattr and ACL handling blocks on macOS when
mac_metadata_strategyisAlways.🔎 Proposed fix to prevent duplication
For xattrs (lines 837-853):
#[cfg(unix)] if let XattrStrategy::Always = keep_options.xattr_strategy { + #[cfg(target_os = "macos")] + let skip_due_to_mac_metadata = mac_metadata_strategy == MacMetadataStrategy::Always; + #[cfg(not(target_os = "macos"))] + let skip_due_to_mac_metadata = false; + + if !skip_due_to_mac_metadata { match utils::os::unix::fs::xattrs::get_xattrs(path) { Ok(xattrs) => { for attr in xattrs { entry.add_xattr(attr); } } Err(e) if e.kind() == std::io::ErrorKind::Unsupported => { log::warn!( "Extended attributes are not supported on filesystem for '{}': {}", path.display(), e ); } Err(e) => return Err(e), } + } }For ACLs (lines 805-835), apply a similar guard:
#[cfg(feature = "acl")] { #[cfg(any( target_os = "linux", target_os = "freebsd", target_os = "macos", windows ))] if let AclStrategy::Always = keep_options.acl_strategy { + #[cfg(target_os = "macos")] + let skip_due_to_mac_metadata = mac_metadata_strategy == MacMetadataStrategy::Always; + #[cfg(not(target_os = "macos"))] + let skip_due_to_mac_metadata = false; + + if !skip_due_to_mac_metadata { use crate::chunk; use pna::RawChunk; let acl = utils::acl::get_facl(path)?; entry.add_extra_chunk(RawChunk::from_data(chunk::faCl, acl.platform.to_bytes())); for ace in acl.entries { entry.add_extra_chunk(RawChunk::from_data(chunk::faCe, ace.to_bytes())); } + } } // ... rest of the block }
🧹 Nitpick comments (4)
cli/tests/cli/stdio/option_mac_metadata.rs (1)
273-276: Consider usingassert_eq!for better failure diagnostics.Using
assert_eq!instead ofassert!with equality comparison provides more informative error messages when tests fail.🔎 Proposed fix
- assert!(output.status.success()); - let extracted_value = String::from_utf8_lossy(&output.stdout); - assert!(extracted_value.trim() == "test_value"); + assert!(output.status.success()); + let extracted_value = String::from_utf8_lossy(&output.stdout); + assert_eq!(extracted_value.trim(), "test_value");cli/src/utils/os/unix/fs.rs (2)
129-131: Unused variable in scopeguard closure.The closure parameter
fis unused. Consider using_to silence the warning or removing the parameter name.🔎 Proposed fix
- let clean = scopeguard::guard(&temp_file, |f| { + let clean = scopeguard::guard(&temp_file, |_| { let _ = fs::remove_file(&temp_file); });
153-155: Same unused variable issue in the second scopeguard closure.🔎 Proposed fix
- let clean = scopeguard::guard(&temp_file, |f| { + let clean = scopeguard::guard(&temp_file, |_| { let _ = fs::remove_file(&temp_file); });cli/src/command/extract.rs (1)
964-971: Collapsible if statement.Per static analysis, these nested
ifstatements can be collapsed.🔎 Proposed fix
#[cfg(not(target_os = "macos"))] - if let MacMetadataStrategy::Always = mac_metadata_strategy { - if item.mac_metadata().is_some() { - log::warn!( - "macOS metadata present but cannot be restored on this platform: '{}'", - path.display() - ); - } + if matches!(mac_metadata_strategy, MacMetadataStrategy::Always) && item.mac_metadata().is_some() { + log::warn!( + "macOS metadata present but cannot be restored on this platform: '{}'", + path.display() + ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cli/Cargo.tomlcli/src/chunk.rscli/src/chunk/mac_metadata.rscli/src/command/append.rscli/src/command/core.rscli/src/command/create.rscli/src/command/extract.rscli/src/command/stdio.rscli/src/command/update.rscli/src/ext.rscli/src/utils/os/unix/fs.rscli/tests/cli/stdio.rscli/tests/cli/stdio/option_mac_metadata.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- cli/src/chunk.rs
- cli/src/command/create.rs
- cli/src/command/update.rs
- cli/tests/cli/stdio.rs
🧰 Additional context used
🧬 Code graph analysis (4)
cli/src/command/stdio.rs (1)
cli/src/command/core.rs (6)
from_flags(117-125)from_flags(135-143)from_flags(153-161)from_flags(171-179)from_flags(198-206)from_flags(209-211)
cli/src/command/extract.rs (2)
cli/src/command/core.rs (1)
collect_split_archives(581-598)cli/src/utils/os/unix/fs.rs (2)
copyfile(107-120)unpack_apple_double(148-170)
cli/src/chunk/mac_metadata.rs (1)
lib/src/chunk/types.rs (1)
from_unchecked(225-227)
cli/src/command/core.rs (2)
cli/src/ext.rs (2)
mac_metadata(18-18)mac_metadata(65-70)cli/src/utils/os/unix/fs.rs (2)
copyfile(107-120)pack_apple_double(124-145)
🔇 Additional comments (14)
cli/Cargo.toml (1)
43-43: Version 1.2.0 is current and secure.The scopeguard dependency at version 1.2.0 is the latest stable version available. No security advisories or CVEs are reported for this version. The change is safe to proceed.
cli/src/ext.rs (1)
17-18: LGTM!The new
mac_metadata()trait method and implementation follow the existing patterns in the file (similar tofflags()). The use offind()withmap()is appropriate for returning the first matching chunk's data.Also applies to: 64-70
cli/src/chunk/mac_metadata.rs (1)
1-10: LGTM!The chunk type definition is well-documented with clear explanation of the PNA naming convention. Using
from_uncheckedwith a compile-time constant byte array is safe since the validity of the chunk type is verified at compile time.cli/src/command/append.rs (1)
10-12: LGTM!The append command correctly hardcodes
MacMetadataStrategy::Neversince it doesn't expose the--mac-metadataCLI flags. This is consistent with the design where only the stdio path exposes this unstable feature.Also applies to: 423-423
cli/tests/cli/stdio/option_mac_metadata.rs (1)
1-7: Good test coverage for the new feature.The tests comprehensively cover the
--mac-metadataoption behavior including flag acceptance,--unstablerequirement, mutual exclusivity with--no-mac-metadata, and various command modes. The macOS-specific tests properly validate xattr preservation.cli/src/utils/os/unix/fs.rs (1)
89-171: Good use of scopeguard for resource cleanup.The implementation properly addresses previous concerns about temp file leaks by using
scopeguard::guard(). This ensures cleanup occurs on all exit paths including early returns and panics.cli/src/command/extract.rs (1)
449-449: Extract command defaults toMacMetadataStrategy::Never.The extract command hardcodes
Neversince it doesn't expose the--mac-metadataCLI flags (only the stdio path does). This is consistent with the append command.cli/src/command/stdio.rs (5)
65-65: LGTM!The new
--mac-metadataand--no-mac-metadataCLI flags are properly:
- Gated behind
--unstablerequirement- Made mutually exclusive via the
mac-metadata-flagArgGroup- Have clear help text explaining their purpose
Also applies to: 207-218
784-788: MacMetadataStrategy properly wired for extract mode.The strategy is correctly derived from CLI flags using
MacMetadataStrategy::from_flags().
947-951: MacMetadataStrategy properly wired for append mode.Consistent with the extract path.
1086-1090: MacMetadataStrategy properly wired for update mode.Consistent with the other paths.
681-700: Verify: Create path doesn't use MacMetadataStrategy.The
run_create_archivefunction usesCreationContextwhich doesn't includemac_metadata_strategy, whilerun_appendandrun_updateuseCreateOptionswhich does. This means--mac-metadatawon't have any effect in create mode (-c), even though the flags are accepted.Is this intentional? If so, consider emitting a warning when
--mac-metadatais used with-cmode.#!/bin/bash # Check if CreationContext has mac_metadata_strategy field rg -n "struct CreationContext" -A 20 cli/src/command/create.rs # Check if CreateOptions has mac_metadata_strategy field rg -n "struct CreateOptions" -A 20 cli/src/command/core.rscli/src/command/core.rs (2)
636-711: LGTM! Strategy correctly propagated across all entry types.The
mac_metadata_strategyis consistently passed through allStoreAsvariants (Hardlink, Symlink, File, Dir) toapply_metadata.
182-212: Enum definition and flag wiring are correct with comprehensive test coverage.The
MacMetadataStrategyenum and platform-specificfrom_flagsimplementations are well-structured and properly integrated. The CLI flags are correctly wired incli/src/command/stdio.rs(lines 784-787, 947-950, 1086-1089), with comprehensive test coverage incli/tests/cli/stdio/option_mac_metadata.rs, including macOS-specific tests that verify theAlwaysvariant preserves extended attributes. The static analysis warning about theAlwaysvariant is a false positive caused by platform-specific compilation on non-macOS CI environments—the variant is properly tested on macOS.
github-actions
Bot
added
the
dependencies
ChanTsune
force-pushed
the
cli/stdio/mac-metadata
branch
from
d5b1ee9 to
0b5bdba
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cli/src/command/extract.rs (2)
945-971: Potential redundant metadata restoration when combining flags.A past review noted that when
mac_metadata_strategyisAlways,unpack_apple_doublerestores xattrs and ACLs viacopyfile(). However, the code above (lines 906-918 for xattrs, line 924 for ACLs) may also restore these if their respective strategies areAlways.On macOS, if a user passes
--mac-metadata --keep-xattr --keep-acl, metadata could be applied twice:
- First via chunk-based restoration (xattrs chunk, ACL chunks)
- Then via
unpack_apple_double(which sets xattrs/ACLs from AppleDouble)This isn't incorrect (the AppleDouble data would overwrite), but it's inefficient. Consider either:
- Documenting that
--mac-metadatasupersedes--keep-xattr/--keep-aclon macOS- Skipping individual xattr/ACL restoration when
mac_metadata_strategy == Alwayson macOSSince this is marked unstable and the behavior is safe (just potentially redundant), this can be deferred.
963-971: Consider collapsing nestedifstatements.Static analysis flagged this nested structure. The two conditions can be combined:
🔎 Proposed fix
#[cfg(not(target_os = "macos"))] -if let MacMetadataStrategy::Always = mac_metadata_strategy { - if item.mac_metadata().is_some() { - log::warn!( - "macOS metadata present but cannot be restored on this platform: '{}'", - path.display() - ); - } +if let MacMetadataStrategy::Always = mac_metadata_strategy && item.mac_metadata().is_some() { + log::warn!( + "macOS metadata present but cannot be restored on this platform: '{}'", + path.display() + ); }cli/src/command/core.rs (1)
875-908: Potential duplicate metadata when combining--mac-metadatawith--keep-xattr/--keep-acl.Similar to the extraction path, when packing entries with
mac_metadata_strategy == Always, the AppleDouble blob includes xattrs and ACLs. Ifkeep_options.xattr_strategyorkeep_options.acl_strategyis alsoAlways, the same data may be stored twice:
- In dedicated xattr/ACL chunks (lines 837-853, 805-821)
- In the
maMdAppleDouble chunk (lines 875-903)This increases archive size without benefit. A past review suggested skipping individual xattr/ACL storage when
mac_metadata_strategy == Alwayson macOS. Since this is an unstable feature, this optimization can be deferred, but consider documenting the interaction.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cli/Cargo.tomlcli/src/chunk.rscli/src/chunk/mac_metadata.rscli/src/command/append.rscli/src/command/core.rscli/src/command/create.rscli/src/command/extract.rscli/src/command/stdio.rscli/src/command/update.rscli/src/ext.rscli/src/utils/os/unix/fs.rscli/tests/cli/stdio.rscli/tests/cli/stdio/option_mac_metadata.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- cli/Cargo.toml
- cli/src/chunk/mac_metadata.rs
- cli/src/command/append.rs
- cli/src/chunk.rs
- cli/tests/cli/stdio.rs
- cli/tests/cli/stdio/option_mac_metadata.rs
🧰 Additional context used
🧬 Code graph analysis (3)
cli/src/command/create.rs (1)
cli/src/command/core.rs (1)
create_entry(636-711)
cli/src/command/extract.rs (1)
cli/src/utils/os/unix/fs.rs (2)
copyfile(107-120)unpack_apple_double(148-170)
cli/src/command/stdio.rs (1)
cli/src/command/core.rs (6)
from_flags(117-125)from_flags(135-143)from_flags(153-161)from_flags(171-179)from_flags(198-206)from_flags(209-211)
⏰ 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). (19)
- GitHub Check: Test WebAssembly (nightly-2025-12-09, wasm32-wasip1)
- GitHub Check: tier1 (ubuntu-22.04-arm, nightly)
- GitHub Check: tier1 (windows-11-arm, beta)
- GitHub Check: tier1 (ubuntu-22.04-arm, stable)
- GitHub Check: Test WebAssembly (beta, wasm32-wasip2)
- GitHub Check: Test WebAssembly (nightly-2025-12-09, wasm32-wasip2)
- GitHub Check: Test WebAssembly (nightly-2025-12-09, wasm32-wasip1)
- GitHub Check: Test WebAssembly (stable, wasm32-wasip2)
- GitHub Check: tier1 (macos-latest, stable)
- GitHub Check: tier1 (macos-latest, beta)
- GitHub Check: tier1 (ubuntu-22.04-arm, stable)
- GitHub Check: tier1 (ubuntu-22.04-arm, beta)
- GitHub Check: tier1 (windows-11-arm, stable)
- GitHub Check: tier1 (ubuntu-latest, nightly)
- GitHub Check: tier1 (ubuntu-22.04-arm, nightly)
- GitHub Check: tier1 (windows-11-arm, nightly)
- GitHub Check: tier1 (ubuntu-latest, beta)
- GitHub Check: msrv (ubuntu-latest, portable-network-archive)
- GitHub Check: msrv (ubuntu-latest, libpna)
🔇 Additional comments (12)
cli/src/utils/os/unix/fs.rs (1)
89-171: LGTM! AppleDouble handling with proper cleanup.The
scopeguardusage correctly addresses the previous resource leak concerns. The temp file is now cleaned up on all exit paths, including early returns fromcopyfileandfs::readfailures.Minor observation: The
pack_apple_doublefunction reads the entire AppleDouble data into memory. For very large resource forks, this could be memory-intensive, but this is acceptable for typical use cases.cli/src/ext.rs (1)
64-70: LGTM! Clean implementation following existing patterns.The
mac_metadata()accessor correctly scansextra_chunksfor themaMdchunk type, consistent with the existingfflags()andacl()implementations.cli/src/command/update.rs (1)
432-432: Verify:--mac-metadataflag not exposed in UpdateCommand.The
mac_metadata_strategyis hardcoded toNeverhere, while thestdiopath usesMacMetadataStrategy::from_flags()to respect CLI arguments. This means the regularpna updatesubcommand won't support Mac metadata preservation.If this is intentional (e.g., feature is stdio-only for now), consider adding a comment. Otherwise, the
UpdateCommandstruct should gainmac_metadataandno_mac_metadataflags similar toStdioCommand.cli/src/command/create.rs (2)
502-502: Consistent with update.rs:--mac-metadatanot exposed in CreateCommand.Same as the update command,
mac_metadata_strategyis hardcoded toNever. The stdio interface supports the flags, but the standalonepna createsubcommand does not. If this is a phased rollout, consider tracking this as a follow-up task.
535-563: LGTM! Proper threading of mac_metadata_strategy through creation paths.The
mac_metadata_strategyis correctly destructured fromCreationContextand passed intoCreateOptions, ensuring consistent propagation through both file-based and split archive creation flows.cli/src/command/stdio.rs (5)
207-218: LGTM! Well-designed CLI flags with appropriate gating.The
--mac-metadataand--no-mac-metadataflags are properly:
- Grouped to prevent conflicting usage
- Gated behind
--unstablefor experimental features- Documented with clear help text describing the scope (extended attributes and ACLs)
691-694: LGTM! Correct propagation in create path.
MacMetadataStrategy::from_flags()is used correctly, respecting the platform-specific behavior defined incore.rs.
788-791: LGTM! Correct propagation in extract path.The extract flow properly receives the mac_metadata_strategy via
OutputOption.
951-954: LGTM! Correct propagation in append path.
1090-1093: LGTM! Correct propagation in update path.cli/src/command/core.rs (2)
182-212: LGTM! Well-designed platform-aware strategy enum.The
MacMetadataStrategyenum correctly uses conditional compilation to:
- On macOS: Allow
Alwayswhen--mac-metadatais passed- On non-macOS: Always return
Neverregardless of flagsThe static analysis warnings about "variant
Alwaysis never constructed" are expected on non-macOS builds and can be safely ignored—the variant is used on macOS.
654-708: LGTM! Consistent propagation through all StoreAs variants.The
mac_metadata_strategyis correctly passed toapply_metadatafor all entry types (Hardlink, Symlink, File, Dir), ensuring consistent behavior.
ChanTsune
force-pushed
the
cli/stdio/mac-metadata
branch
from
0b5bdba to
9aec838
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In @cli/src/command/core.rs:
- Around line 841-874: When keep_options.mac_metadata_strategy is
MacMetadataStrategy::Always on macOS, avoid duplicating metadata by skipping the
separate xattr and ACL collection paths; detect this case (check
keep_options.mac_metadata_strategy == MacMetadataStrategy::Always) before
executing the xattr and ACL handling blocks and short-circuit them so only
utils::os::unix::fs::copyfile::pack_apple_double(...) is used to produce the
maMd chunk (crate::chunk::maMd) added via entry.add_extra_chunk(...). Ensure the
conditional gating surrounds the existing xattr collection logic and the ACL
handling logic so they do not run when pack_apple_double is used.
In @cli/src/command/stdio.rs:
- Around line 792-795: The OutputOption construction duplicates
mac_metadata_strategy: remove the top-level mac_metadata_strategy initialization
and rely on the one inside keep_options; update the OutputOption builder to keep
only the mac_metadata_strategy set via KeepOptions (using
MacMetadataStrategy::from_flags(args.mac_metadata, args.no_mac_metadata)) and
delete the duplicate mac_metadata_strategy field at the top level so there is a
single source of truth for mac_metadata_strategy within KeepOptions.
In @cli/tests/cli/stdio/option_mac_metadata.rs:
- Around line 289-297: The test currently calls
Command::new("xattr").args(...).status().expect("Failed to set xattr") but does
not verify the process exit code; change it to capture the returned Status from
.status() and assert that status.success() (or call .expect_err/unwrap with a
message that includes the Status) so the test fails if xattr returns a non-zero
exit code; look for the Command::new("xattr") invocation in
option_mac_metadata.rs and replace the direct .expect on .status() with a check
against status.success().
- Around line 221-229: The test calls Command::new("xattr").status().expect(...)
which only ensures the process ran but not that it succeeded; capture the
returned Status (e.g., let status = Command::new("xattr")...
.status().expect(...)) and assert status.success(), failing the test with a
message that includes status.code() or the full Status so a non-zero exit is
detected and reported.
🧹 Nitpick comments (2)
cli/Cargo.toml (1)
43-43: Consider removing duplicatescopeguardfrom[dev-dependencies].
scopeguardis now declared in both[dependencies](line 43) and[dev-dependencies](line 74). Since it's a production dependency, the entry in[dev-dependencies]is redundant—Cargo will use the one from[dependencies]for both production and test builds.🔎 Proposed fix
[dev-dependencies] maplit = "1.0.2" path-slash = "0.2.1" rust-embed = { version = "8.9.0", features = ["debug-embed"] } same-file = "1.0.6" -scopeguard = "1.2.0" walkdir = "2.5.0"Also applies to: 74-74
cli/tests/cli/stdio/option_mac_metadata.rs (1)
273-276: Useassert_eq!for clearer test failure messages.Using
assert_eq!instead ofassert!(...== ...)provides better diagnostic output when tests fail.🔎 Proposed fix
- assert!(output.status.success()); - let extracted_value = String::from_utf8_lossy(&output.stdout); - assert!(extracted_value.trim() == "test_value"); + assert!(output.status.success(), "xattr read command failed"); + let extracted_value = String::from_utf8_lossy(&output.stdout); + assert_eq!(extracted_value.trim(), "test_value");
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cli/Cargo.tomlcli/src/chunk.rscli/src/chunk/mac_metadata.rscli/src/command/append.rscli/src/command/core.rscli/src/command/create.rscli/src/command/extract.rscli/src/command/stdio.rscli/src/command/update.rscli/src/ext.rscli/src/utils/os/unix/fs.rscli/tests/cli/stdio.rscli/tests/cli/stdio/option_mac_metadata.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/src/chunk.rs
- cli/src/command/append.rs
- cli/src/command/create.rs
🧰 Additional context used
🧬 Code graph analysis (4)
cli/src/chunk/mac_metadata.rs (1)
lib/src/chunk/types.rs (1)
from_unchecked(225-227)
cli/src/command/stdio.rs (1)
cli/src/command/core.rs (6)
from_flags(117-125)from_flags(135-143)from_flags(153-161)from_flags(171-179)from_flags(198-206)from_flags(209-211)
cli/src/command/extract.rs (2)
cli/src/command/core.rs (1)
collect_split_archives(581-598)cli/src/utils/os/unix/fs.rs (2)
copyfile(107-120)unpack_apple_double(148-170)
cli/src/command/core.rs (2)
cli/src/ext.rs (2)
mac_metadata(18-18)mac_metadata(65-70)cli/src/utils/os/unix/fs.rs (2)
copyfile(107-120)pack_apple_double(124-145)
🔇 Additional comments (6)
cli/src/chunk/mac_metadata.rs (1)
1-10: LGTM!The
maMdchunk type constant is well-documented with clear explanation of the PNA chunk naming semantics. The use ofunsafe { ChunkType::from_unchecked(*b"maMd") }is appropriate here since the 4-byte literal is a compile-time constant.cli/src/ext.rs (1)
17-18: LGTM!The
mac_metadatatrait method and implementation follow the established pattern of other methods likeaclandfflags. The implementation correctly scansextra_chunksfor themaMdchunk type and returns the raw data if present.Also applies to: 64-70
cli/src/utils/os/unix/fs.rs (1)
89-171: Well-implemented macOS AppleDouble handling with proper resource cleanup.The
scopeguardusage correctly ensures temporary files are cleaned up on all exit paths, addressing the previous review concerns. The implementation properly wraps the macOScopyfile()API.Minor note: The explicit
drop(clean)calls at lines 143 and 168 are technically unnecessary since the guard would be dropped at scope end anyway, but they do make the cleanup point explicit.cli/tests/cli/stdio.rs (1)
6-6: LGTM!Module declaration properly added in alphabetical order within the test suite.
cli/src/command/update.rs (1)
404-404: Verify:mac_metadata_strategyis hardcoded toNeverfor the update command.Unlike other commands (create, extract, append) that appear to have CLI flags for
--mac-metadata, the update command hardcodesMacMetadataStrategy::Never.Is this an intentional scope limitation for this PR, or should
--mac-metadata/--no-mac-metadataflags be added toUpdateCommandfor consistency?cli/src/command/core.rs (1)
182-212: LGTM! Clean platform-aware strategy implementation.The
MacMetadataStrategyenum is well-designed with:
- Clear documentation explaining AppleDouble behavior
- Platform-aware
from_flagsconstructors usingcfg(target_os = "macos")- Proper default (Never) for non-macOS platforms
ChanTsune
force-pushed
the
cli/stdio/mac-metadata
branch
from
9aec838 to
9b4198d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cli/Cargo.toml (1)
43-43: Redundantscopeguardin[dev-dependencies].
scopeguardis now listed in both[dependencies](line 43) and[dev-dependencies](line 74). Since runtime dependencies are automatically available to tests, the[dev-dependencies]entry is redundant and can be removed.🔎 Proposed fix
Remove line 74:
[dev-dependencies] maplit = "1.0.2" path-slash = "0.2.1" rust-embed = { version = "8.9.0", features = ["debug-embed"] } same-file = "1.0.6" -scopeguard = "1.2.0" walkdir = "2.5.0"Also applies to: 74-74
cli/src/command/extract.rs (1)
977-985: Minor: Collapsibleifstatements.The static analysis flagged that the nested
ifat lines 978-985 can be collapsed. This is a minor style improvement.🔎 Proposed fix
#[cfg(not(target_os = "macos"))] - if let MacMetadataStrategy::Always = mac_metadata_strategy { - if item.mac_metadata().is_some() { - log::warn!( - "macOS metadata present but cannot be restored on this platform: '{}'", - path.display() - ); - } + if matches!(mac_metadata_strategy, MacMetadataStrategy::Always) && item.mac_metadata().is_some() { + log::warn!( + "macOS metadata present but cannot be restored on this platform: '{}'", + path.display() + ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cli/Cargo.tomlcli/src/chunk.rscli/src/chunk/mac_metadata.rscli/src/command/append.rscli/src/command/core.rscli/src/command/create.rscli/src/command/extract.rscli/src/command/stdio.rscli/src/command/update.rscli/src/ext.rscli/src/utils/os/unix/fs.rscli/tests/cli/stdio.rscli/tests/cli/stdio/option_mac_metadata.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- cli/src/chunk/mac_metadata.rs
- cli/src/chunk.rs
- cli/src/command/update.rs
- cli/src/ext.rs
- cli/tests/cli/stdio.rs
- cli/src/command/stdio.rs
🔇 Additional comments (16)
cli/tests/cli/stdio/option_mac_metadata.rs (2)
1-46: Good test coverage for CLI flag acceptance.The tests properly verify that
--mac-metadataand--no-mac-metadataare accepted with--unstable, and that--mac-metadatafails without--unstable. The mutual exclusivity test at lines 71-88 correctly validates that both flags cannot be used together.
204-276: macOS xattr preservation test looks well-structured.The test flow (set xattr → create archive with
--mac-metadata→ extract → verify xattr) correctly validates the AppleDouble roundtrip. The verification at line 273-275 properly checks both the command success and the extracted value.cli/src/command/create.rs (2)
9-12: Import addition looks correct.The
MacMetadataStrategyis properly imported alongside other strategy types from the core module.
461-482:mac_metadata_strategy: MacMetadataStrategy::Neveris appropriate here.For the standard
createcommand, defaulting toNeveris correct since the--mac-metadataflag isn't exposed in this command's argument struct. The stdio command path handles the flag separately.cli/src/command/append.rs (2)
9-12: Import correctly added.
MacMetadataStrategyis properly imported from the core module.
375-396: Consistent initialization ofmac_metadata_strategytoNever.This follows the same pattern as
create.rs, correctly defaulting toNeverfor the standard append command.cli/src/utils/os/unix/fs.rs (3)
89-120: Well-designed copyfile wrapper with proper error handling.The
copyfilefunction correctly usesnix::NixPathfor safe path handling and properly propagates errors vianix::errno::Errno::result. The module is correctly gated behind#[cfg(target_os = "macos")].
122-145:pack_apple_doublecorrectly uses scopeguard for cleanup.The
scopeguard::guardat line 129 ensures the temp file is removed regardless of how the function exits, addressing the resource leak concerns from past reviews. The implementation correctly:
- Creates a unique temp file
- Packs metadata via copyfile
- Reads the result
- Cleans up via scopeguard drop
147-171:unpack_apple_doubleproperly handles temp file lifecycle.Same scopeguard pattern ensures cleanup on all exit paths. The explicit
drop(clean)at line 168 is slightly redundant (would happen anyway at function end) but doesn't cause issues.cli/src/command/core.rs (3)
182-212:MacMetadataStrategyenum is well-designed.The platform-specific
from_flagsimplementations correctly ensure thatAlwaysis only returned on macOS, while non-macOS platforms always returnNever. The#[derive(Default)]with#[default]onNeveris appropriate.Note: The static analysis warnings about "
Alwaysis never constructed" are false positives in cross-platform builds sinceAlwaysis only constructed via the macOS-specificfrom_flags.
771-831: Duplicate metadata archival issue properly addressed.The
skip_xattr_aclflag (lines 771-779) correctly prevents duplicate xattr and ACL archival whenmac_metadata_strategyisAlwayson macOS, since AppleDouble packing already captures this data. This addresses the critical issue flagged in past reviews.The conditional guards at lines 782 and 813 properly skip the individual xattr/ACL handling when AppleDouble is being used.
853-886: AppleDouble packing implementation looks correct.The macOS-specific packing logic:
- Gracefully ignores
NotFounderrors (file has no Mac metadata)- Logs warnings for other failures without failing the operation
- Uses the
maMdchunk type for the AppleDouble data- Non-macOS platforms correctly emit a warning
cli/src/command/extract.rs (4)
11-14: Imports correctly added.
MacMetadataStrategyis properly imported alongside other strategy types.
404-451: Consistent initialization ofmac_metadata_strategytoNever.Both
KeepOptions(line 424) andOutputOption(line 450) correctly default toNeverfor the standard extract command.
906-938: Redundant metadata restoration issue properly addressed.The
skip_xattr_acllogic (lines 906-913) correctly prevents duplicate xattr and ACL restoration when:
mac_metadata_strategyisAlwaysAND- The entry actually has
mac_metadataThis is more precise than the archival side, which is correct because during extraction we need to check if the entry has AppleDouble data before skipping individual restoration.
959-985: AppleDouble restoration implementation looks correct.The macOS-specific restoration logic:
- Only processes entries that have
mac_metadata- Logs debug message on success
- Logs warnings on failure without failing the operation
- Non-macOS platforms correctly warn when mac_metadata is present but cannot be restored
ChanTsune
force-pushed
the
cli/stdio/mac-metadata
branch
from
9b4198d to
3647698
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cli/src/chunk/mac_metadata.rs (1)
10-10: Add SAFETY comment to justify the unsafe block.The use of
ChunkType::from_uncheckedrequires a SAFETY comment explaining why the byte pattern*b"maMd"satisfies the function's safety requirements. While the doc comment above describes the naming convention, a direct SAFETY comment above the unsafe block would clarify that this specific pattern is valid.🔎 Proposed addition
#[allow(non_upper_case_globals)] -pub const maMd: ChunkType = unsafe { ChunkType::from_unchecked(*b"maMd") }; +// SAFETY: The byte pattern "maMd" follows the PNA chunk naming convention: +// ancillary, private, reserved, safe-to-copy as documented above. +pub const maMd: ChunkType = unsafe { ChunkType::from_unchecked(*b"maMd") };cli/tests/cli/stdio/option_mac_metadata.rs (1)
12-202: Consider test cleanup for better isolation.While the tests use unique file/directory names per test (e.g.,
stdio_mac_metadata_option_accepted.txt), they don't appear to explicitly clean up created artifacts. Consider adding cleanup code or using temporary directories to prevent potential test pollution across runs.Example approach using tempfile or explicit cleanup
You could either:
- Use the
tempfilecrate to create temporary directories that auto-cleanup- Add explicit cleanup in test teardown or at test start
Example pattern:
#[test] fn my_test() { setup(); let temp_dir = tempfile::tempdir().unwrap(); let file = temp_dir.path().join("test.txt"); // ... test code using file ... // temp_dir automatically cleaned up when dropped }Or add cleanup at the start of each test:
let file = "stdio_test.txt"; let _ = fs::remove_file(file); // Clean up if exists from previous run fs::write(file, "content").unwrap();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cli/Cargo.tomlcli/src/chunk.rscli/src/chunk/mac_metadata.rscli/src/command/append.rscli/src/command/core.rscli/src/command/create.rscli/src/command/extract.rscli/src/command/stdio.rscli/src/command/update.rscli/src/ext.rscli/src/utils/os/unix/fs.rscli/tests/cli/stdio.rscli/tests/cli/stdio/option_mac_metadata.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- cli/src/command/update.rs
- cli/Cargo.toml
- cli/src/ext.rs
- cli/src/chunk.rs
- cli/src/command/stdio.rs
🧰 Additional context used
🧬 Code graph analysis (3)
cli/src/chunk/mac_metadata.rs (1)
lib/src/chunk/types.rs (1)
from_unchecked(225-227)
cli/src/command/core.rs (3)
cli/src/ext.rs (2)
mac_metadata(18-18)mac_metadata(65-70)cli/src/utils/os/unix/fs/xattrs.rs (1)
get_xattrs(5-20)cli/src/utils/os/unix/fs.rs (2)
copyfile(107-120)pack_apple_double(124-145)
cli/src/command/extract.rs (2)
cli/src/utils/os/unix/fs/xattrs.rs (1)
set_xattrs(22-35)cli/src/utils/os/unix/fs.rs (2)
copyfile(107-120)unpack_apple_double(148-170)
🔇 Additional comments (21)
cli/tests/cli/stdio.rs (1)
6-6: LGTM!The module declaration correctly integrates the new macOS metadata tests into the stdio test suite.
cli/src/command/create.rs (2)
10-12: LGTM!The import addition correctly brings
MacMetadataStrategyinto scope for use inKeepOptionsinitialization.
481-481: LGTM!Initializing
mac_metadata_strategytoNeveris correct here sinceCreateCommanddoesn't expose--mac-metadataCLI flags. The macOS metadata functionality is available through the stdio command path (as evidenced by the test file).cli/src/command/append.rs (2)
10-12: LGTM!The import correctly adds
MacMetadataStrategyto the append command's dependencies.
395-395: LGTM!Consistent with
create.rs, initializingmac_metadata_strategytoNeveris appropriate sinceAppendCommanddoesn't define CLI flags for macOS metadata handling.cli/tests/cli/stdio/option_mac_metadata.rs (6)
12-26: LGTM!These tests correctly verify that the CLI accepts
--mac-metadataand--no-mac-metadataoptions when paired with--unstable. The tests are appropriately scoped to option parsing validation.Also applies to: 32-46
52-66: LGTM!These validation tests correctly verify that:
--mac-metadatarequires the--unstableflag--mac-metadataand--no-mac-metadataare mutually exclusiveThe use of stderr predicates ensures proper error messaging.
Also applies to: 72-88
94-126: LGTM!These tests provide comprehensive coverage of
--mac-metadataacross all stdio modes (extract, append, update). The test structure is consistent and correctly validates that the option is recognized in each operational context.Also applies to: 132-163, 169-202
221-234: LGTM!The xattr command status checking has been correctly implemented. Both tests now:
- Capture the returned
Statusfrom.status()- Assert that
status.success()is true- Provide informative error messages with the exit code
This addresses the previous review feedback.
Also applies to: 294-307
214-281: LGTM!This test correctly verifies the end-to-end preservation of macOS extended attributes when using
--mac-metadata. The test flow is sound:
- Creates a file with xattr
- Archives it with
--mac-metadata- Extracts with
--mac-metadata- Verifies the xattr is restored
287-353: LGTM!This test correctly verifies that xattrs are NOT preserved when using
--no-mac-metadata. The assertion at line 352 correctly expects the xattr read command to fail (because the attribute doesn't exist in the extracted file).cli/src/utils/os/unix/fs.rs (4)
89-106: LGTM! Well-structured module with proper documentation.The module is correctly gated with
#[cfg(target_os = "macos")]and includes helpful documentation referencing the Apple man page. TheTEMP_COUNTERwith process ID combination provides unique temporary file names across concurrent operations.
107-120: LGTM! Clean wrapper around libc::copyfile.The function properly uses
nix::NixPathfor safe path-to-CString conversions and correctly checks the return value viaErrno::result.
122-145: LGTM! Proper resource management with scopeguard.The implementation correctly uses
scopeguardto ensure the temporary file is cleaned up on all code paths (success or error). The order of operations (read data, then cleanup) is correct.
147-170: LGTM! Resource leak concerns from past reviews properly addressed.The
scopeguardpattern ensures the temporary file is cleaned up on all paths, including early returns fromwrite_allorcopyfilefailures. The inner block ensures the file handle is dropped before callingcopyfile.cli/src/command/core.rs (3)
182-212: LGTM! Well-designed strategy enum with proper platform gating.The
MacMetadataStrategyenum follows the established pattern of other strategy enums in this file. The platform-specificfrom_flagsimplementations correctly ensure thatAlwaysis only returned on macOS, preventing misuse on other platforms.
771-831: LGTM! Correctly prevents duplicate metadata when using AppleDouble.The
skip_xattr_aclflag properly gates the individual xattr and ACL handling whenmac_metadata_strategyisAlwayson macOS. This addresses the previous review concerns about data duplication.
853-886: LGTM! Appropriate error handling for metadata packing.The implementation correctly:
- Silently skips files without Mac metadata (
NotFound)- Logs warnings for other failures without failing the archive operation
- Only adds the chunk when data is non-empty
- Provides a clear warning on non-macOS platforms
cli/src/command/extract.rs (3)
11-14: LGTM! Properly imports and initializes MacMetadataStrategy.The import is correctly added and
mac_metadata_strategyis initialized toNeverinextract_archive, which is the expected default behavior for the extract command.Also applies to: 424-424
895-929: LGTM! Correctly guards xattr/ACL restoration based on AppleDouble presence.The
skip_xattr_aclcondition appropriately checks bothmac_metadata_strategy == AlwaysANDitem.mac_metadata().is_some(). This ensures that entries without AppleDouble data still get their individual xattrs/ACLs restored, which is the correct behavior for archives containing a mix of entries.
950-976: LGTM! Consistent error handling for metadata restoration.The implementation:
- Logs success at debug level
- Logs failures as warnings without failing extraction (consistent with create flow)
- On non-macOS, only warns when metadata is actually present in the entry
The lenient error handling is appropriate for metadata operations that shouldn't block file extraction.
Add --mac-metadata and --no-mac-metadata flags to preserve macOS extended attributes, ACLs, and resource forks using the AppleDouble format via copyfile(). - Introduce MacMetadataStrategy enum with Always/Never variants - Integrate with KeepOptions for consistent metadata handling - Add maMd chunk type for storing packed AppleDouble data - Support all stdio modes: create (-c), extract (-x), append (-r), update (-u) - Require --unstable flag as feature is experimental
ChanTsune
force-pushed
the
cli/stdio/mac-metadata
branch
from
e5a59c1 to
b4b2533
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cli/tests/cli/stdio/option_mac_metadata.rs (2)
52-66: Consider adding a corresponding test for--no-mac-metadatarequiring--unstable.The test
stdio_mac_metadata_requires_unstableverifies that--mac-metadatafails without--unstable, but there's no equivalent test verifying that--no-mac-metadataalso requires--unstable. This creates an asymmetry in test coverage.🔎 Suggested test to add
/// Precondition: A file exists in the filesystem. /// Action: Attempt to use --no-mac-metadata without --unstable flag. /// Expectation: Command fails because --no-mac-metadata requires --unstable. #[test] fn stdio_no_mac_metadata_requires_unstable() { setup(); let file = "stdio_no_mac_metadata_requires_unstable.txt"; fs::write(file, "test content").unwrap(); let mut cmd = cargo_bin_cmd!("pna"); cmd.arg("experimental") .arg("stdio") .arg("-c") .arg("--no-mac-metadata") .arg(file) .assert() .failure() .stderr(predicate::str::contains("--unstable")); }
94-126: Extract/append/update tests only cover--mac-metadata, not--no-mac-metadata.The tests for extract (lines 94-126), append (lines 132-163), and update (lines 169-202) verify that
--mac-metadatais accepted in these modes, but there are no corresponding tests for--no-mac-metadata. While not critical, this creates incomplete coverage of the flag matrix across different stdio operations.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cli/Cargo.tomlcli/src/chunk.rscli/src/chunk/mac_metadata.rscli/src/command/append.rscli/src/command/core.rscli/src/command/create.rscli/src/command/extract.rscli/src/command/stdio.rscli/src/command/update.rscli/src/ext.rscli/src/utils/os/unix/fs.rscli/tests/cli/stdio.rscli/tests/cli/stdio/option_mac_metadata.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- cli/Cargo.toml
- cli/src/ext.rs
- cli/src/command/append.rs
- cli/tests/cli/stdio.rs
- cli/src/utils/os/unix/fs.rs
🧰 Additional context used
🧬 Code graph analysis (7)
cli/src/chunk.rs (1)
cli/src/ext.rs (2)
mac_metadata(18-18)mac_metadata(65-70)
cli/tests/cli/stdio/option_mac_metadata.rs (2)
cli/src/command/core.rs (1)
new(283-304)cli/src/ext.rs (1)
new(98-104)
cli/src/command/core.rs (4)
cli/src/ext.rs (2)
mac_metadata(18-18)mac_metadata(65-70)cli/src/utils/os/unix/fs/xattrs.rs (1)
get_xattrs(5-20)cli/src/utils/os/unix/fs.rs (2)
copyfile(107-120)pack_apple_double(124-145)lib/src/chunk.rs (1)
from_data(288-300)
cli/src/chunk/mac_metadata.rs (1)
lib/src/chunk/types.rs (1)
from_unchecked(225-227)
cli/src/command/create.rs (1)
cli/src/command/core.rs (3)
collect_items_from_paths(392-406)create_entry(636-678)entry_option(680-700)
cli/src/command/extract.rs (3)
lib/src/entry.rs (1)
xattrs(888-890)cli/src/utils/os/unix/fs/xattrs.rs (1)
set_xattrs(22-35)cli/src/utils/os/unix/fs.rs (2)
copyfile(107-120)unpack_apple_double(148-170)
cli/src/command/stdio.rs (1)
cli/src/command/core.rs (6)
from_flags(117-125)from_flags(135-143)from_flags(153-161)from_flags(171-179)from_flags(198-206)from_flags(209-211)
🔇 Additional comments (14)
cli/src/command/core.rs (3)
182-212: LGTM: Well-designed platform-aware strategy pattern.The
MacMetadataStrategyenum with platform-conditionalfrom_flagsimplementations cleanly handles the macOS-only nature of AppleDouble metadata. The default ofNeveris appropriate, and the non-macOS variant correctly ignores the flags to prevent runtime errors on unsupported platforms.
771-779: LGTM: Correct deduplication logic for macOS metadata.The
skip_xattr_aclflag properly prevents duplicate metadata whenMacMetadataStrategy::Alwaysis active on macOS, sincepack_apple_doublealready captures xattrs and ACLs viacopyfile(). This addresses the previously identified critical issue about data duplication.
853-886: LGTM: Robust AppleDouble packing with appropriate error handling.The implementation correctly:
- Skips empty AppleDouble data
- Treats
NotFoundas a non-error (file has no Mac metadata)- Logs warnings for other errors without failing the entire operation
- Emits a platform warning on non-macOS when
Alwaysis configuredcli/src/command/update.rs (1)
404-404: Verify: Update command lacks--mac-metadataCLI flag support.The
mac_metadata_strategyis hardcoded toNever, but theUpdateCommandstruct doesn't include--mac-metadata/--no-mac-metadataflags likeStdioCommanddoes. If this is intentional (deferring feature parity), consider adding a TODO comment. Otherwise, the flags should be added for consistency with other archive operations.cli/src/command/create.rs (1)
481-481: Verify: Create command lacks--mac-metadataCLI flag support.Similar to the update command,
mac_metadata_strategyis hardcoded toNever. TheStdioCommandsupports--mac-metadata, but the standaloneCreateCommanddoes not. If this is intentional (e.g., only supporting via stdio for bsdtar compatibility), consider documenting this design decision. Otherwise, add the flags for feature parity.cli/src/command/stdio.rs (3)
65-65: LGTM: Proper argument group for mutual exclusion.The
mac-metadata-flaggroup correctly ensures--mac-metadataand--no-mac-metadataare mutually exclusive.
207-218: LGTM: Clear help text and appropriate unstable gating.The CLI flags are well-documented with descriptive help text. Requiring
--unstableis appropriate for this new feature.
671-674: LGTM: Consistent propagation across all stdio operations.The
MacMetadataStrategy::from_flagsis correctly and consistently used in all four operations (create, extract, append, update), ensuring uniform behavior across the stdio interface.Also applies to: 769-772, 932-935, 1073-1076
cli/src/command/extract.rs (3)
895-904: LGTM: Correct deduplication logic for extraction.The
skip_xattr_aclflag correctly checks bothMacMetadataStrategy::AlwaysAND the presence ofmac_metadatabefore skipping separate xattr/ACL restoration. This is more precise than the creation side (which only checks strategy) because during extraction, the entry might not have mac_metadata even if the strategy isAlways.
950-976: LGTM: Robust AppleDouble unpacking with appropriate fallback.The implementation correctly:
- Only unpacks when both strategy is
AlwaysAND mac_metadata is present- Logs debug on success for troubleshooting
- Logs warnings for failures without stopping extraction
- Provides informative warning on non-macOS platforms when metadata cannot be restored
424-424: Verify: Extract command lacks--mac-metadataCLI flag support.Similar to create and update commands,
mac_metadata_strategyis hardcoded toNeverin the standaloneExtractCommand. The stdio interface supports the flags, but standalone commands do not. Consider adding flags for consistency or documenting this as intentional.cli/src/chunk.rs (1)
3-3: LGTM: Consistent module structure.The new
mac_metadatamodule follows the existing pattern established byaclandfflagmodules, with proper declaration and public re-export.Also applies to: 7-7
cli/src/chunk/mac_metadata.rs (1)
1-10: LGTM: Well-documented chunk type with correct naming semantics.The
maMdchunk type follows PNA conventions appropriately:
- Ancillary (lowercase first letter) - archive is still usable without it
- Private (lowercase second letter) - not part of the public PNA specification
- Reserved (uppercase third letter) - follows convention
- Safe to copy (lowercase fourth letter) - can be copied when modifying the archive
The use of
unsafe { ChunkType::from_unchecked(...) }is acceptable here since*b"maMd"is a compile-time constant valid 4-byte ASCII sequence.cli/tests/cli/stdio/option_mac_metadata.rs (1)
206-356: Well-structured macOS xattr tests.The macOS-specific tests properly verify:
- Extended attributes are preserved when using
--mac-metadataduring both creation and extraction- Extended attributes are excluded when using
--no-mac-metadataduring creation, even if extraction attempts to restore them- Exit status checks for the
xattrcommand are correctly implemented (addressing previous review feedback)The test logic correctly uses
assert!(!output.status.success())at line 354 to verify that the xattr command fails when the attribute doesn't exist, which is the expected behavior when--no-mac-metadatawas used during archive creation.
2 participants
Summary by CodeRabbit
New Features
Behavior Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.