⚗️ Add --fflags option to cli and add private chunk for store fflags#2373
Conversation
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 a significant new feature: the preservation of file flags during archiving and extraction. It provides a command-line interface option ( 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 a valuable feature for preserving file flags, specifically on macOS, by adding a --fflags option to the stdio command. The implementation is well-structured, introducing a new ffLg chunk type and integrating the logic for archiving and extracting file flags. The changes are consistent with the existing codebase structure. I have a couple of suggestions to enhance code style and idiomatic Rust usage, which will improve maintainability.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ChanTsune
force-pushed
the
cli/stdio/flags
branch
2 times, most recently
from
7766cef to
c4a81b6
Compare
ChanTsune
force-pushed
the
cli/stdio/flags
branch
from
c4a81b6 to
0521c57
Compare
ChanTsune
force-pushed
the
cli/stdio/flags
branch
from
0521c57 to
62cf039
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds file-flag (fflag) metadata support: a new fflag chunk type, platform-aware flag get/set helpers, CLI options to preserve/restore flags, and integration into archive create/append/extract/update flows with default-off behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (stdio)
participant Core as Command Core
participant Arch as Archive Writer/Reader
participant FS as Utils::fs
participant OS as OS-specific fs
rect rgba(200,230,255,0.4)
CLI->>Core: build KeepOptions(fflags_strategy)
end
alt Creating/Appending with Always
Core->>FS: get_flags(path)
FS->>OS: platform-specific get_flags
OS-->>FS: Vec<String> flags
FS-->>Core: flags
Core->>Arch: append fflag_chunks(flags)
else Default (Never)
CLI->>Core: no fflag actions
end
opt Extract/Restore with Always
Arch->>Core: read entry.extra_chunks (ffLg)
Core->>FS: set_flags(path, flags)
FS->>OS: platform-specific set_flags
OS-->>FS: ok / Err(Unsupported)
FS-->>Core: result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cli/src/command/extract.rs (1)
759-781: Consider using iterator methods for cleaner code.The current implementation works correctly, but could be more idiomatic using
filter_mapas suggested in a previous review:🔎 Proposed refactor using filter_map
if let FflagsStrategy::Always = keep_options.fflags_strategy { - let mut flags = Vec::new(); - for chunk in item.extra_chunks() { - if chunk.ty() == crate::chunk::ffLg { - if let Ok(flag) = std::str::from_utf8(chunk.data()) { - flags.push(flag.to_string()); - } - } - } - if !flags.is_empty() { + let flags: Vec<String> = item + .extra_chunks() + .filter_map(|chunk| { + if chunk.ty() == crate::chunk::ffLg { + std::str::from_utf8(chunk.data()).ok().map(str::to_string) + } else { + None + } + }) + .collect(); + if !flags.is_empty() { match utils::fs::set_flags(path, &flags) {Note: The
!flags.is_empty()check before callingset_flagsis good practice even thoughset_flagsalso checks for empty flags, as it avoids unnecessary function call overhead.
🧹 Nitpick comments (2)
verify_fflags.sh (2)
20-26: Consider usingstatinstead ofls | grepfor more robust flag checking.The static analysis tool (SC2010) correctly identifies that parsing
lsoutput is fragile. While the controlled filenametest_filemitigates the risk here, usingstatwould be more robust.🔎 Proposed fix for macOS
# Verify -if ls -lO test_file | grep -q "nodump"; then +if stat -f "%Sf" test_file | grep -q "nodump"; then echo "Verification SUCCESS: 'nodump' flag preserved." else echo "Verification FAILED: 'nodump' flag missing." - ls -lO test_file + stat -f "%Sf" test_file exit 1 fi
1-9: Consider adding platform check and cleanup on success.The script uses BSD-specific commands (
chflags,ls -lO). Consider adding a platform check or documenting the macOS/FreeBSD requirement. Also, the script leaves artifacts after success.🔎 Proposed improvements
#!/bin/bash set -e +# This script requires macOS or FreeBSD (uses chflags) +if ! command -v chflags &> /dev/null; then + echo "Skipping: chflags not available on this platform" + exit 0 +fi + +# Cleanup function +cleanup() { + rm -rf test_file test.pna out +} +trap cleanup EXIT + # Clean up previous run rm -rf test_file test.pna out
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cli/src/chunk.rscli/src/chunk/fflag.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/utils/fs.rscli/src/utils/fs/nodump.rscli/src/utils/os/unix/fs.rsverify_fflags.sh
🧰 Additional context used
🧬 Code graph analysis (9)
cli/src/utils/fs.rs (1)
cli/src/utils/os/unix/fs.rs (6)
get_flags(24-58)get_flags(101-153)get_flags(220-257)set_flags(61-80)set_flags(156-215)set_flags(260-291)
cli/src/command/update.rs (1)
cli/src/command/core.rs (4)
collect_items(344-470)collect_split_archives(477-494)create_entry(520-590)entry_option(592-612)
cli/src/chunk/fflag.rs (2)
lib/src/chunk/types.rs (1)
from_unchecked(178-180)lib/src/chunk.rs (1)
from_data(288-300)
cli/src/command/core.rs (3)
cli/src/utils/fs.rs (1)
get_flags(69-89)cli/src/utils/os/unix/fs.rs (3)
get_flags(24-58)get_flags(101-153)get_flags(220-257)cli/src/chunk/fflag.rs (1)
fflag_chunk(12-14)
cli/src/command/append.rs (1)
cli/src/command/core.rs (1)
create_entry(520-590)
cli/src/utils/fs/nodump.rs (2)
cli/src/utils/fs.rs (1)
get_flags(69-89)cli/src/utils/os/unix/fs.rs (3)
get_flags(24-58)get_flags(101-153)get_flags(220-257)
cli/src/utils/os/unix/fs.rs (4)
cli/src/utils/fs.rs (2)
get_flags(69-89)set_flags(91-111)lib/src/entry/header.rs (1)
path(80-82)cli/src/utils/os/windows/fs.rs (2)
std(50-50)stat(49-57)cli/src/command/core.rs (2)
new(199-220)empty(250-254)
cli/src/command/extract.rs (2)
cli/src/utils/fs.rs (1)
set_flags(91-111)cli/src/utils/os/unix/fs.rs (3)
set_flags(61-80)set_flags(156-215)set_flags(260-291)
cli/src/command/stdio.rs (1)
cli/src/command/core.rs (6)
new(199-220)from_flags(97-105)from_flags(115-123)from_flags(133-141)from_flags(151-159)from_flags(169-177)
🪛 Shellcheck (0.11.0)
verify_fflags.sh
[warning] 20-20: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
🔇 Additional comments (24)
cli/src/chunk.rs (1)
1-5: LGTM!The module structure follows the existing pattern established by the
aclmodule. The glob re-export is appropriate for exposing the fflag chunk types and constructors.cli/src/command/create.rs (1)
415-427: LGTM!The
KeepOptionsinitialization correctly addsfflags_strategy: FflagsStrategy::Never. Since theCreateCommandstruct doesn't define--keep-fflags/--no-keep-fflagsCLI arguments, hardcodingNeveris the appropriate default for this command path.cli/src/command/append.rs (1)
332-344: LGTM!The
KeepOptionsinitialization correctly includesfflags_strategy: FflagsStrategy::Never, consistent with theCreateCommandpattern. The import changes are properly aligned.cli/src/chunk/fflag.rs (2)
3-10: Intentional naming convention acknowledged.The
#[allow(non_upper_case_globals)]attribute is correctly applied. The documentation clearly explains the PNA chunk naming semantics where case conveys meaning (ancillary, private, reserved, safe-to-copy). This addresses the static analysis warning appropriately.
12-14: LGTM!The
fflag_chunkfunction correctly constructs aRawChunkfrom the flag string bytes using theffLgchunk type.cli/src/utils/fs/nodump.rs (1)
1-9: LGTM!The refactored
is_nodumpfunction correctly delegates to the centralizedget_flagsutility. The string comparison against"nodump"aligns with the flag names returned by the platform-specific implementations incli/src/utils/os/unix/fs.rs.cli/src/utils/fs.rs (1)
69-111: LGTM!The
get_flagsandset_flagsfunctions correctly implement platform-gated dispatch with appropriate fallbacks. The silent no-op on unsupported platforms (returning emptyVecorOk(())) is reasonable for cross-platform compatibility, though you may want to document this behavior if users expect feedback on unsupported platforms.cli/src/command/update.rs (1)
341-353: LGTM!The
KeepOptionsinitialization correctly includesfflags_strategy: FflagsStrategy::Never, consistent with the pattern inCreateCommandandAppendCommand. The import changes properly includeFflagsStrategyfromcore.cli/src/command/core.rs (3)
162-178: LGTM! Consistent with established patterns.The
FflagsStrategyenum and itsfrom_flagsimplementation follow the same structure as the existingXattrStrategy,AclStrategy,PermissionStrategy, andTimestampStrategytypes, ensuring consistency across the codebase.
186-186: LGTM!The new
fflags_strategyfield integrates cleanly with the existingKeepOptionsstruct.
752-768: LGTM! Consistent error handling and chunk creation.The implementation correctly:
- Uses the full path
crate::chunk::fflag_chunk(&flag)(addressing the past review comment aboutuseinside a loop)- Matches the error handling pattern used for xattrs above
- Properly propagates non-Unsupported errors
cli/src/command/extract.rs (2)
12-14: LGTM!Imports correctly updated to include
FflagsStrategy.
284-284: Verify:fflags_strategyis hardcoded toNeverfor extraction.Unlike timestamp, permission, xattr, and ACL strategies which are configurable via CLI flags (
--keep-*/--no-keep-*), the file flags strategy is hardcoded toNeverin the extract command. Therestore_metadatafunction (lines 759-781) does implement the restoration logic, but it will never execute because the strategy is alwaysNever.Is this intentional for this PR, or should
--keep-fflags/--no-keep-fflagsCLI arguments be added toExtractCommandto enable file flag restoration?cli/src/command/stdio.rs (6)
11-13: LGTM!Imports correctly updated to include
FflagsStrategy.
76-76: LGTM!The new
keep-fflags-flagargument group follows the established pattern for other keep-*-flag groups (e.g.,keep-acl-flag,keep-xattr-flag).
194-205: Verify: Help text mentions "(unstable)" but no unstable requirement.The help text for both
--keep-fflagsand--no-keep-fflagsindicates the feature is "(unstable)", but unlike--keep-acl/--no-keep-acl(which havegroup(ArgGroup::new("unstable-acl").args(["keep_acl", "no_keep_acl"]).requires("unstable"))), there is no corresponding unstable group constraint for fflags.If this feature should require the
--unstableflag, consider adding:group(ArgGroup::new("unstable-fflags").args(["keep_fflags", "no_keep_fflags"]).requires("unstable")),If the feature is stable, the "(unstable)" text should be removed from the help strings.
542-542: LGTM!Correctly wires
FflagsStrategy::from_flagsinrun_create_archive.
623-623: LGTM!Correctly wires
FflagsStrategy::from_flagsinrun_extract_archive.
754-754: LGTM!Correctly wires
FflagsStrategy::from_flagsinrun_append.cli/src/utils/os/unix/fs.rs (5)
23-58: LGTM!The macOS
get_flagsimplementation correctly useslstatto retrieve file flags and maps them to standard BSD flag names. The flag mappings (nodump, uchg, uappnd, opaque, hidden, archived, schg, sappnd) align with the BSD conventions.
87-98: LGTM!The Linux flag constants are correctly defined with appropriate reference to the kernel documentation. The values match the kernel's
linux/fs.hdefinitions.
100-153: LGTM!The Linux
get_flagsimplementation is well-structured with proper handling of:
- Symlinks (ELOOP returns empty vector)
- Unsupported filesystems (ENOTTY/EOPNOTSUPP returns empty vector)
- Correct mapping of Linux flags to libarchive-compatible BSD-style names
The comments explaining the semantic equivalence (e.g.,
FS_IMMUTABLE_FL≈SF_IMMUTABLE) are helpful for understanding cross-platform behavior.
155-215: LGTM!The Linux
set_flagsimplementation correctly:
- Preserves existing flags by reading
current_flagsfirst- Optimizes by skipping the
setflagssyscall when flags haven't changed- Supports useful aliases (e.g.,
simmutableforschg,compressforcompr)- Properly handles symlinks and unsupported filesystems with appropriate error types
219-257: LGTM!The FreeBSD
get_flagsimplementation correctly uses thenixcrate'slstatfor a cleaner interface and includes FreeBSD-specific flags (uunlnk,sunlnk) not present on macOS.
ChanTsune
force-pushed
the
cli/stdio/flags
branch
from
62cf039 to
8b41cec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cli/src/utils/os/unix/fs.rs (2)
60-85: macOSset_flagsoverwrites all flags instead of preserving existing ones.This issue was flagged in a previous review. Unlike the Linux implementation which reads current flags first and ORs in the new flags, the macOS implementation builds
flag_bitsfrom scratch. This means existing flags not in the input list will be cleared.
259-291: FreeBSDset_flagshas the same issue as macOS.As noted in a previous review, this implementation overwrites all flags instead of preserving existing ones. The fix should mirror the Linux implementation's approach of reading existing flags first.
🧹 Nitpick comments (3)
verify_fflags.sh (2)
1-9: Consider adding a platform check.This script uses BSD-specific commands (
chflags,ls -lO) that are only available on macOS and FreeBSD. Consider adding an early check to provide a helpful error message on unsupported platforms.🔎 Proposed platform check
#!/bin/bash set -e +# Check for BSD-style chflags support (macOS/FreeBSD) +if ! command -v chflags &> /dev/null; then + echo "Error: chflags command not found. This script requires macOS or FreeBSD." + exit 1 +fi + # Clean up previous run rm -rf test_file test.pna out
20-26: Optional: Usestatinstead ofls | grepfor more robust flag checking.The static analysis tool flagged
ls | grep(SC2010). While acceptable for a test script, usingstatprovides a more reliable approach. Additionally, consider adding cleanup at the end.🔎 Proposed improvements
# Verify -if ls -lO test_file | grep -q "nodump"; then +if stat -f "%Sf" test_file 2>/dev/null | grep -q "nodump"; then echo "Verification SUCCESS: 'nodump' flag preserved." else echo "Verification FAILED: 'nodump' flag missing." ls -lO test_file exit 1 fi + +# Cleanup +cd .. +rm -rf test_file test.pna outcli/src/command/extract.rs (1)
283-283: Consider adding--keep-fflags/--no-keep-fflagsflags toExtractCommand.The
fflags_strategyis hardcoded toNever, whilestdio.rsexposes CLI flags for this feature. For consistency, consider adding the same CLI arguments toExtractCommandso users ofpna extractcan also restore file flags.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cli/src/chunk.rscli/src/chunk/fflag.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/fs.rscli/src/utils/fs/nodump.rscli/src/utils/os/unix/fs.rsverify_fflags.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/src/command/append.rs
- cli/src/command/update.rs
- cli/src/chunk/fflag.rs
🧰 Additional context used
🧬 Code graph analysis (5)
cli/src/utils/fs.rs (1)
cli/src/utils/os/unix/fs.rs (6)
get_flags(24-58)get_flags(101-153)get_flags(220-257)set_flags(61-80)set_flags(156-215)set_flags(260-291)
cli/src/command/create.rs (1)
cli/src/command/core.rs (1)
create_entry(520-590)
cli/src/command/stdio.rs (1)
cli/src/command/core.rs (5)
from_flags(97-105)from_flags(115-123)from_flags(133-141)from_flags(151-159)from_flags(169-177)
cli/src/command/extract.rs (2)
cli/src/utils/fs.rs (1)
set_flags(91-111)cli/src/utils/os/unix/fs.rs (3)
set_flags(61-80)set_flags(156-215)set_flags(260-291)
cli/src/utils/os/unix/fs.rs (1)
cli/src/utils/fs.rs (2)
get_flags(69-89)set_flags(91-111)
🪛 Shellcheck (0.11.0)
verify_fflags.sh
[warning] 20-20: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
🔇 Additional comments (16)
cli/src/chunk.rs (1)
1-5: LGTM!The module structure follows the existing pattern established for the
aclmodule, maintaining consistency in how chunk types are organized and exposed.cli/src/utils/os/unix/fs.rs (2)
155-215: Linux implementation correctly preserves existing flags.Good implementation - it reads the current flags with
fs_ioc_getflags, ORs in the new flags, and only writes if the flags actually changed. This is the pattern that macOS and FreeBSD implementations should follow.
23-58: macOSget_flagsimplementation looks correct.Properly uses
lstatto read flags from symlinks without following them, and correctly maps BSD file flags to human-readable names.cli/src/ext.rs (1)
47-60: LGTM!The
fflags()implementation is clean and follows the established pattern. Usingfilter_mapto silently skip invalid UTF-8 data is appropriate since flag names should always be valid ASCII strings.cli/src/utils/fs/nodump.rs (1)
1-9: Excellent refactor!This is a clean simplification that eliminates platform-specific code duplication by delegating to the unified
get_flagsimplementation. The code is now much more maintainable.cli/src/command/create.rs (1)
415-427: LGTM!The
fflags_strategyis correctly defaulted toNeverfor the create command, maintaining backward compatibility. The fflags feature is appropriately gated behind the experimental stdio interface as indicated by the PR objectives.cli/src/utils/fs.rs (1)
69-111: LGTM!The platform abstraction is well-structured with clear
cfgguards and appropriate fallbacks for unsupported platforms. This follows the established pattern used by other functions in this module likemvandlchown.cli/src/command/core.rs (2)
162-178: LGTM!The
FflagsStrategyenum follows the exact pattern established by other strategy enums (XattrStrategy,AclStrategy,PermissionStrategy,TimestampStrategy), maintaining excellent consistency in the codebase.
752-768: LGTM!The fflags integration into
apply_metadatafollows the established error handling pattern used for xattrs:
- Logs a warning for unsupported filesystems
- Propagates unexpected errors
- Uses the full path
crate::chunk::fflag_chunkas suggested in a previous reviewcli/src/command/extract.rs (2)
11-13: LGTM!Import of
FflagsStrategyis correctly added alongside the other strategy types.
758-773: LGTM!The file flags restoration logic correctly follows the established pattern for metadata restoration:
- Checks strategy before proceeding
- Guards against empty flags to avoid unnecessary operations
- Handles
Unsupportederrors gracefully with a warning- Propagates other errors appropriately
cli/src/command/stdio.rs (5)
11-13: LGTM!Import of
FflagsStrategyis correctly added to the core imports.
194-205: LGTM!The CLI arguments follow the established pattern for preserve/no-preserve flags with appropriate aliases and help text.
542-542: LGTM!Correctly wires the CLI flags to
FflagsStrategy::from_flags()following the established pattern.
623-623: LGTM!Correctly enables file flags preservation during extraction when the appropriate flags are set.
754-754: LGTM!Correctly wires fflags strategy for the append operation, completing the integration across all stdio modes.
ChanTsune
force-pushed
the
cli/stdio/flags
branch
from
e531ef3 to
c9d1016
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
verify_fflags.sh (2)
12-17: Hard-coded path may cause test failures.The script uses a hard-coded path
./target/debug/pna, which will fail if the binary hasn't been built or is located elsewhere (e.g.,target/release/pnafor release builds).🔎 Proposed fix to support flexible binary paths
+# Allow overriding the pna binary location +PNA_BIN="${PNA_BIN:-./target/debug/pna}" + +if [ ! -x "$PNA_BIN" ]; then + echo "Error: pna binary not found at $PNA_BIN" + exit 1 +fi + # Archive -./target/debug/pna experimental stdio -c --fflags test_file > test.pna +$PNA_BIN experimental stdio -c --fflags test_file > test.pna # Extract mkdir out cd out -../target/debug/pna experimental stdio -x --fflags < ../test.pna +../$PNA_BIN experimental stdio -x --fflags < ../test.pna
20-26: Use a more robust flag verification method.The
ls -lO | greppattern is fragile and will fail with non-alphanumeric filenames. Consider usingstator accessing file metadata directly.🔎 Proposed fix using stat command
# Verify -if ls -lO test_file | grep -q "nodump"; then +# On macOS, stat -f %Sf shows flags; on Linux, use lsattr +if stat -f %Sf test_file 2>/dev/null | grep -q nodump || \ + lsattr test_file 2>/dev/null | grep -q d; then echo "Verification SUCCESS: 'nodump' flag preserved." else echo "Verification FAILED: 'nodump' flag missing." - ls -lO test_file + stat -f %Sf test_file 2>/dev/null || lsattr test_file 2>/dev/null || echo "Cannot read flags" exit 1 ficli/src/command/stdio.rs (1)
76-76: Missing unstable requirement group for fflags flags.While the individual
--keep-fflagsand--no-keep-fflagsflags correctly haverequires = "unstable"(lines 197, 204), there's no correspondingArgGroupwith.requires("unstable")like the ACL pattern at lines 33-34. For consistency, add an unstable requirement group:🔎 Proposed fix
group(ArgGroup::new("mtime-newer-than-source").args(["newer_mtime", "newer_mtime_than"])), + group(ArgGroup::new("unstable-fflags").args(["keep_fflags", "no_keep_fflags"]).requires("unstable")), group(ArgGroup::new("keep-fflags-flag").args(["keep_fflags", "no_keep_fflags"])),Based on coding guidelines, following established patterns (ACL) for consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cli/src/chunk.rscli/src/chunk/fflag.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/fs.rscli/src/utils/fs/nodump.rscli/src/utils/os/unix/fs.rsverify_fflags.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- cli/src/command/update.rs
- cli/src/utils/fs.rs
- cli/src/command/core.rs
- cli/src/ext.rs
- cli/src/chunk.rs
- cli/src/command/append.rs
🧰 Additional context used
🧬 Code graph analysis (5)
cli/src/chunk/fflag.rs (2)
lib/src/chunk/types.rs (1)
from_unchecked(178-180)lib/src/chunk.rs (1)
from_data(288-300)
cli/src/utils/fs/nodump.rs (2)
cli/src/utils/os/unix/fs.rs (3)
get_flags(24-58)get_flags(108-160)get_flags(235-272)cli/src/utils/fs.rs (1)
get_flags(69-89)
cli/src/command/stdio.rs (1)
cli/src/command/core.rs (5)
from_flags(97-105)from_flags(115-123)from_flags(133-141)from_flags(151-159)from_flags(169-177)
cli/src/utils/os/unix/fs.rs (1)
cli/src/utils/fs.rs (2)
get_flags(69-89)set_flags(91-111)
cli/src/command/extract.rs (3)
cli/src/command/core.rs (1)
collect_split_archives(477-494)cli/src/utils/os/unix/fs.rs (3)
set_flags(68-87)set_flags(171-230)set_flags(282-313)cli/src/utils/fs.rs (1)
set_flags(91-111)
🪛 Shellcheck (0.11.0)
verify_fflags.sh
[warning] 20-20: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Test WebAssembly (beta, wasm32-unknown-emscripten)
- GitHub Check: tier1 (macos-latest, stable)
- GitHub Check: tier1 (windows-11-arm, nightly)
- GitHub Check: Test WebAssembly (nightly-2025-12-09, wasm32-wasip1)
- 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-unknown-unknown)
- GitHub Check: tier1 (windows-latest, nightly)
- GitHub Check: tier1 (ubuntu-22.04-arm, stable)
- GitHub Check: Test WebAssembly (beta, wasm32-wasip1)
- GitHub Check: tier1 (ubuntu-22.04-arm, nightly)
- GitHub Check: Test WebAssembly (beta, wasm32-unknown-unknown)
- GitHub Check: Test WebAssembly (stable, wasm32-unknown-unknown)
- GitHub Check: Test WebAssembly (stable, wasm32-wasip1)
- GitHub Check: Test WebAssembly (stable, wasm32-wasip2)
- GitHub Check: Test WebAssembly (beta, wasm32-unknown-emscripten)
- GitHub Check: msrv (ubuntu-latest, portable-network-archive)
🔇 Additional comments (8)
cli/src/chunk/fflag.rs (1)
1-14: LGTM! Chunk type definition follows PNA conventions.The implementation correctly defines the private
ffLgchunk type and helper function. The non-upper-case naming flagged by the linter is intentional—it follows PNA chunk naming conventions where case carries semantic meaning, as documented in lines 4-8.cli/src/command/create.rs (1)
426-426: Verify that hard-codedFflagsStrategy::Neveris intentional.The
fflags_strategyis hard-coded toNeverin thecreatecommand, meaning file flags will never be preserved. From reviewingcli/src/command/stdio.rs, it appears that fflags support is only available via the experimentalstdiocommand (with--keep-fflags/--no-keep-fflagsflags), not the regularcreatecommand.Is this intentional, or should the regular
createcommand also support file flags via CLI flags similar to--keep-xattr,--keep-acl, etc.?cli/src/command/extract.rs (2)
283-283: Verify that hard-codedFflagsStrategy::Neveris intentional.Similar to
cli/src/command/create.rs, thefflags_strategyis hard-coded toNeverin theextractcommand. File flags support appears to be limited to the experimentalstdiocommand. Please confirm this is intentional.
758-773: Flag restoration logic is correct and follows established patterns.The implementation correctly:
- Checks for
FflagsStrategy::Alwaysbefore attempting restoration- Retrieves flags via
item.fflags()extension method- Handles empty flag lists efficiently
- Gracefully handles unsupported filesystems with a warning
- Propagates other errors appropriately
The pattern matches the ACL and xattr restoration logic, ensuring consistency across the codebase.
cli/src/command/stdio.rs (1)
544-544: Fflags strategy correctly wired through all command flows.The
FflagsStrategy::from_flagscalls are properly integrated into create, extract, and append flows, enabling user-controlled file flag preservation via CLI flags.Also applies to: 625-625, 756-756
cli/src/utils/os/unix/fs.rs (3)
23-58: get_flags implementations look correct across all platforms.The platform-specific
get_flagsimplementations correctly:
- macOS: Use
lstatto readst_flagsand map BSD flag bits to string names- Linux: Use
FS_IOC_GETFLAGSioctl with proper handling for symlinks and unsupported filesystems- FreeBSD: Use
lstatto readst_flagswith comprehensive BSD flag supportAll implementations properly handle platform-specific flags and edge cases.
Also applies to: 107-160, 234-272
170-230: Linux set_flags correctly merges with existing flags.The Linux implementation properly:
- Reads current flags via
FS_IOC_GETFLAGSbefore modifying- Merges new flags with existing flags using bitwise OR
- Only writes if flags changed (line 225)
- Handles symlinks and unsupported filesystems gracefully
This preserves flags not explicitly set by the caller, matching the documented libarchive behavior for Linux.
60-92: Cross-platform flag handling behavior is documented and matches libarchive.The code correctly implements platform-specific flag handling: macOS overwrites all flags by directly calling
lchflags()with the new flag bits, while Linux reads existing flags and merges them usingFS_IOC_GETFLAGS. This behavior difference is intentionally documented in the code (lines 62-66 for macOS, lines 164-168 for Linux) and matches upstream libarchive behavior, where BSD systems usechflags()to replace all flags while Linux uses ioctl to merge them.
ChanTsune
force-pushed
the
cli/stdio/flags
branch
from
c9d1016 to
8787ede
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/src/utils/os/unix/fs.rs (1)
60-92: macOS set_flags should check for empty flags array like FreeBSD and Linux implementations.The macOS implementation is inconsistent with the other Unix platforms: both FreeBSD (lines 285-287) and Linux (lines 177-179) return early when
flags.is_empty(), but macOS lacks this check. If an empty array is passed, it will calllchflags(path, 0)and clear all existing flags.Note: The code comment claims libarchive uses
chflags()directly on BSD systems to overwrite flags, but libarchive actually reads existing flags viastat()first, then callschflags()/lchflags()with the modified flags. This makes the current implementation's overwrite-without-reading approach a deliberate deviation from libarchive, not a match of its behavior.Add an empty check for consistency:
#[cfg(target_os = "macos")] pub(crate) fn set_flags(path: &Path, flags: &[String]) -> io::Result<()> { use std::os::unix::ffi::OsStrExt; + + if flags.is_empty() { + return Ok(()); + } + let c_path = std::ffi::CString::new(path.as_os_str().as_bytes())?;
🧹 Nitpick comments (1)
verify_fflags.sh (1)
1-26: The--fflagsflag is properly defined; remove verification concern.Concerns #1 and #2 remain valid:
Platform-specific commands:
chflags nodump(line 9) is BSD/macOS specific. On Linux, the equivalent ischattr +d. Consider documenting this limitation or adding platform detection.Fragile grep pattern: Line 20 uses
ls -lO | grepwhich shellcheck flags (SC2010) because it's brittle with non-alphanumeric filenames. Consider using a more robust check likels -lO test_file | awk '{print $5}' | grep -q nodumpor platform-specific commands such asstat -f %Sf test_file(macOS) orlsattr test_file(Linux).The
--fflagsflag is defined incli/src/command/stdio.rsas a visible alias to--keep-fflagsand is correctly used by the script.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
cli/src/chunk.rscli/src/chunk/fflag.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/fs.rscli/src/utils/fs/nodump.rscli/src/utils/os/unix/fs.rsverify_fflags.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/src/utils/fs.rs
- cli/src/chunk.rs
- cli/src/ext.rs
🧰 Additional context used
🧬 Code graph analysis (4)
cli/src/chunk/fflag.rs (2)
lib/src/chunk/types.rs (1)
from_unchecked(225-227)lib/src/chunk.rs (1)
from_data(288-300)
cli/src/command/append.rs (1)
cli/src/command/core.rs (2)
collect_items_from_paths(329-343)create_entry(554-624)
cli/src/command/stdio.rs (1)
cli/src/command/core.rs (5)
from_flags(115-123)from_flags(133-141)from_flags(151-159)from_flags(169-177)from_flags(187-195)
cli/src/command/core.rs (1)
cli/src/chunk/fflag.rs (1)
fflag_chunk(12-14)
🪛 Shellcheck (0.11.0)
verify_fflags.sh
[warning] 20-20: Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames.
(SC2010)
🔇 Additional comments (17)
cli/src/chunk/fflag.rs (1)
1-14: LGTM!The
ffLgchunk type definition andfflag_chunkhelper follow PNA naming conventions correctly. The documentation clearly explains the semantic meaning of the case pattern, and the implementation is straightforward and correct.cli/src/command/append.rs (1)
9-13: LGTM! Infrastructure wiring is consistent.The
FflagsStrategyis correctly imported and wired throughKeepOptions, defaulting toNever. This follows the established pattern for other metadata preservation strategies (timestamps, permissions, xattrs, ACLs).Also applies to: 366-366
cli/src/command/create.rs (1)
9-12: LGTM!The
fflags_strategyfield is correctly added toKeepOptionswith aNeverdefault, matching the pattern used in other command modules.Also applies to: 448-448
cli/src/command/extract.rs (2)
11-12: LGTM!The
FflagsStrategyimport and initialization follow the established pattern for metadata preservation strategies.Also applies to: 305-305
769-784: Excellent consistency with existing metadata restoration patterns.The file flags restoration logic correctly mirrors the patterns used for xattrs (lines 746-757) and ACLs (lines 764-768):
- Checks strategy variant (
FflagsStrategy::Always)- Extracts flags via
item.fflags()- Calls platform utility
utils::fs::set_flags- Gracefully handles
Unsupportedwith a warning- Propagates other errors
The empty-check optimization before calling
set_flagsis a nice touch.cli/src/command/update.rs (1)
13-17: LGTM!Consistent integration of
fflags_strategyinto the update workflow, following the same pattern as create and append commands.Also applies to: 374-374
cli/src/utils/fs/nodump.rs (1)
1-9: Excellent refactoring to unified flag handling.The refactor from platform-specific implementations to the unified
get_flagsapproach significantly simplifies the code while maintaining correctness. The logic is clear and concise.cli/src/command/core.rs (3)
180-196: LGTM!The
FflagsStrategyenum andfrom_flagsimplementation follow the exact same pattern asXattrStrategy,AclStrategy, and other metadata strategies. The precedence logic (no_keep → keep → Never) is consistent and correct.
204-204: LGTM!The
fflags_strategyfield addition toKeepOptionsis consistent with the struct's existing metadata preservation fields.
786-802: Excellent integration following established patterns.The file flags archiving logic correctly mirrors the patterns used for xattrs (lines 765-781) and ACLs (lines 733-759):
- Guards on
FflagsStrategy::Always- Retrieves flags via
utils::fs::get_flags(path)- Iterates and embeds each flag as a chunk using
crate::chunk::fflag_chunk- Gracefully handles
Unsupportederrors with warnings- Propagates other I/O errors
The use of the full path
crate::chunk::fflag_chunkis appropriate and avoids namespace pollution.cli/src/command/stdio.rs (3)
189-202: LGTM: CLI flag definitions are well-structured.The fflags CLI arguments follow established patterns for other metadata preservation flags (ACL, xattr, etc.). Both flags properly require the
--unstableflag, include appropriate aliases, and have clear help text.
63-63: LGTM: ArgGroup properly enforces mutual exclusion.The
keep-fflags-flaggroup correctly ensures only one of--keep-fflagsor--no-keep-fflagscan be specified. Since both individual arguments already require the--unstableflag (lines 192, 199), no additional group-level requirement is needed.
586-586: LGTM: FflagsStrategy properly integrated across all archive operations.The fflags handling is consistently integrated into all three archive operations (create, extract, append) using
FflagsStrategy::from_flags(). This matches the pattern used for other metadata strategies (ACL, xattr, timestamp, permission) and ensures uniform behavior across the CLI.Also applies to: 669-669, 803-803
cli/src/utils/os/unix/fs.rs (4)
23-58: LGTM: macOS get_flags implementation is correct.The implementation properly uses
lstatto read file flags without following symlinks and correctly maps BSDst_flagsbits to their corresponding string names. Error handling is appropriate.
94-230: LGTM: Linux implementation is well-designed.The Linux implementation correctly:
- Uses
FS_IOC_GETFLAGS/FS_IOC_SETFLAGSioctls with proper error handling- Opens files with
O_NOFOLLOWto avoid following symlinks- Gracefully handles unsupported filesystems and symlinks by returning appropriate errors or empty results
- Reads current flags and merges new flags (preserving existing ones)
- Only writes if flags actually changed (line 225-227)
- Returns early for empty flags arrays (line 177-179)
The documented merge behavior (lines 162-169) aligns with libarchive's Linux implementation and provides better cross-platform safety.
232-272: LGTM: FreeBSD get_flags implementation is correct.The FreeBSD implementation properly uses
lstatto read file flags without following symlinks. The flag mapping is more complete than macOS, includinguunlnkandsunlnkflags specific to FreeBSD.
274-313: FreeBSD set_flags follows the same design as macOS.The FreeBSD implementation:
- Properly checks for empty flags and returns early (lines 285-287), unlike macOS
- Overwrites flags rather than merging, matching macOS and documented bsdtar behavior (lines 274-280)
- Uses
lchflagscorrectly to avoid following symlinks- Accepts both canonical names and aliases for flags (e.g., "schg" and "simmutable")
The past review comment about merging vs. overwriting applies here as well, but the developer has documented this as intentional to match libarchive/bsdtar's BSD behavior. The same verification of the libarchive source would clarify whether this is the correct approach.
ChanTsune
force-pushed
the
cli/stdio/flags
branch
from
8787ede to
b049852
Compare
ChanTsune
force-pushed
the
cli/stdio/flags
branch
from
b049852 to
8205cb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
cli/src/chunk/fflag.rs (1)
1-14: Implementation follows PNA chunk naming conventions correctly.The constant name
ffLgintentionally uses non-standard casing to convey semantic meaning per the PNA chunk specification (documented in lines 4-8). The#[allow(non_upper_case_globals)]attribute is appropriately used here. The implementation is clean and follows the same pattern as other chunk types in the codebase.Note: A past review flagged the naming, but this is a false positive given the semantic requirements of the PNA format.
cli/src/utils/fs/nodump.rs (1)
1-9: Excellent refactoring to unified flag handling.The refactoring from platform-specific implementations to the unified
get_flagsapproach significantly simplifies the code while maintaining correct behavior. This aligns well with the new cross-platform flag infrastructure introduced in this PR.As noted in a past review, this implementation is clean and correct.
🧹 Nitpick comments (1)
cli/src/utils/os/unix/fs.rs (1)
60-92: LGTM! macOS set_flags behavior is documented and intentional.The past review comment about overwriting flags has been addressed. The documentation (lines 60-66) clearly explains that this implementation intentionally overwrites all existing flags rather than merging them, which matches libarchive/bsdtar behavior on BSD systems.
The implementation:
- Uses
lchflagsto avoid following symlinks- Maps all relevant flag names to BSD flag constants
- Properly handles errors from the syscall
- Includes aliases for consistency (e.g., both "uchg" and "uimmutable" would work based on the pattern)
Note: The current implementation only handles the canonical names in the match statement. You may want to add aliases for consistency with FreeBSD (lines 295-302) if users might provide alternative flag names.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cli/src/chunk.rscli/src/chunk/fflag.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/fs.rscli/src/utils/fs/nodump.rscli/src/utils/os/unix/fs.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- cli/src/chunk.rs
- cli/src/command/create.rs
- cli/src/command/core.rs
- cli/src/command/update.rs
- cli/src/ext.rs
🧰 Additional context used
🧬 Code graph analysis (6)
cli/src/chunk/fflag.rs (2)
lib/src/chunk/types.rs (1)
from_unchecked(225-227)lib/src/chunk.rs (1)
from_data(288-300)
cli/src/command/stdio.rs (1)
cli/src/command/core.rs (4)
from_flags(117-125)from_flags(135-143)from_flags(153-161)from_flags(171-179)
cli/src/command/extract.rs (2)
cli/src/utils/fs.rs (1)
set_flags(91-111)cli/src/utils/os/unix/fs.rs (3)
set_flags(68-87)set_flags(171-230)set_flags(282-313)
cli/src/command/append.rs (1)
cli/src/command/core.rs (2)
create_entry(603-645)entry_option(647-667)
cli/src/utils/fs/nodump.rs (2)
cli/src/utils/fs.rs (1)
get_flags(69-89)cli/src/utils/os/unix/fs.rs (3)
get_flags(24-58)get_flags(108-160)get_flags(235-272)
cli/src/utils/os/unix/fs.rs (5)
cli/src/utils/fs.rs (2)
get_flags(69-89)set_flags(91-111)lib/src/entry/header.rs (1)
path(80-82)cli/src/utils/os/windows/fs.rs (2)
std(50-50)stat(49-57)cli/src/command/core.rs (1)
new(250-271)cli/src/ext.rs (1)
new(88-94)
🔇 Additional comments (11)
cli/src/utils/fs.rs (1)
69-111: Platform-specific flag handling implemented correctly.The
get_flagsandset_flagsfunctions provide appropriate platform-specific implementations for macOS, Linux, Android, and FreeBSD, with safe no-op fallbacks for unsupported platforms. The conditional compilation structure is clear and the fallback behavior (returning empty Vec/Ok(())) is appropriate for platforms without file flag support.cli/src/command/extract.rs (1)
917-932: File flags restoration logic is well-structured.The restoration logic correctly:
- Checks for
FflagsStrategy::Alwaysbefore attempting restoration- Skips empty flag lists to avoid unnecessary operations
- Handles
Unsupportederrors gracefully with warnings (important for cross-platform compatibility)- Propagates other errors appropriately
cli/src/command/stdio.rs (4)
11-12: LGTM! FflagsStrategy import correctly added.The import is properly placed and matches the pattern used for other strategy types (AclStrategy, PermissionStrategy, XattrStrategy).
63-63: Past review comment addressed.The ArgGroup for
keep-fflags-flagis now present. The past review comment about adding unstable requirements has been marked as addressed in commits f9dff62 to c9d1016.
191-204: LGTM! File flags CLI arguments properly defined.The
--keep-fflagsand--no-keep-fflagsflags are correctly implemented with:
- Appropriate visible aliases (
preserve-fflags,fflags,no-preserve-fflags,no-fflags)- Unstable requirement (
requires = "unstable")- Clear help text indicating unstable status
The pattern matches other similar flags like ACL and xattr.
656-656: LGTM! Consistent fflags strategy initialization.The
FflagsStrategy::from_flags(args.keep_fflags, args.no_keep_fflags)pattern is correctly applied in:
run_create_archive(line 656)run_extract_archive(line 750)run_append(line 909)This matches the established pattern for other strategy types.
Also applies to: 750-750, 909-909
cli/src/utils/os/unix/fs.rs (5)
23-58: LGTM! macOS get_flags implementation is correct.The implementation properly:
- Uses
lstatto avoid following symlinks- Checks for errors and returns appropriate
io::Error- Maps all relevant macOS file flags to libarchive-compatible names
- Returns an empty vector on error rather than failing silently
94-160: LGTM! Linux get_flags implementation with proper error handling.The implementation correctly:
- Uses
O_NOFOLLOWto avoid following symlinks- Returns empty Vec for symlinks (ELOOP) since Linux doesn't support flags on symlinks
- Returns empty Vec for unsupported filesystems (ENOTTY, EOPNOTSUPP) like tmpfs and nfs
- Maps Linux-specific flags to libarchive-compatible names
- Includes helpful comments about the equivalence between Linux and BSD flags
The error handling for unsupported filesystems (lines 128-131) is particularly good as it allows graceful degradation.
162-230: LGTM! Linux set_flags correctly merges with existing flags.The implementation properly:
- Reads current flags first (lines 196-207) to preserve unmodified flags
- Merges new flags with existing ones (line 210:
new_flags = current_flags)- Only calls
FS_IOC_SETFLAGSif flags changed (line 225)- Handles symlinks by returning an error (lines 186-192)
- Handles unsupported filesystems by returning an appropriate error (lines 200-205)
- Includes aliases for flag names (e.g., "simmutable", "sappend", "compress")
The documentation (lines 162-169) clearly explains this merge behavior differs from BSD systems, which is the correct approach for Linux per libarchive.
232-272: LGTM! FreeBSD get_flags implementation is correct.The implementation:
- Uses
nix::sys::stat::lstatwhich doesn't follow symlinks- Properly casts
st_flagsto the correct type- Maps all relevant FreeBSD file flags including the additional flags not present on macOS (UF_NOUNLINK/SF_NOUNLINK)
- Returns appropriate error types
274-313: LGTM! FreeBSD set_flags behavior is documented and intentional.Similar to macOS, the past review comment about overwriting flags has been addressed with documentation (lines 274-280) explaining this matches libarchive/bsdtar behavior on BSD systems.
The implementation:
- Returns early if flags is empty (lines 285-287)
- Uses
lchflagsto avoid following symlinks- Maps all relevant flag names including FreeBSD-specific ones (uunlnk, sunlnk)
- Includes aliases for consistency (e.g., "uimmutable"/"uchg", "simmutable"/"schg")
- Properly handles errors from the syscall
- Ignores Linux-specific flags (line 303-304) which is appropriate
The alias handling here (lines 295-302) is more complete than the macOS implementation, which is good.
Add the `--fflags` CLI option and store file flags in a private chunk. Document Linux (merge) vs BSD/macOS (overwrite) behavior to match libarchive/bsdtar’s cross-platform differences.
ChanTsune
force-pushed
the
cli/stdio/flags
branch
from
8205cb9 to
430819f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cli/src/command/append.rs (1)
394-394: Missing CLI flags to control fflags preservation.The
fflags_strategyis hardcoded toFflagsStrategy::Never, making the file flags feature unreachable in the append workflow. The PR title mentions adding--fflagsoption, but no corresponding CLI flags exist inAppendCommand.Similar to other preservation options (lines 89-138), you should add
keep_fflagsandno_keep_fflagsboolean fields with appropriateArgGroupconstraints, then useFflagsStrategy::from_flags()instead of hardcodingNever.cli/src/command/extract.rs (2)
69-341: Missing CLI options to control file flags restoration.The
ExtractCommandstruct lacks CLI flags (e.g.,--keep-fflags,--no-keep-fflags) to control file flags restoration, making the feature implemented in lines 917-932 unreachable. All other preservation options (timestamps, permissions, xattrs, ACLs) have corresponding CLI flags.Add the CLI flags and ArgGroup constraint:
🔎 Proposed fix
Add to the
ExtractCommandstruct (after line 170):no_keep_acl: bool, + #[arg( + long, + visible_alias = "preserve-fflags", + requires = "unstable", + help = "Restore file flags (unstable)" + )] + keep_fflags: bool, + #[arg( + long, + visible_alias = "no-preserve-fflags", + requires = "unstable", + help = "Do not restore file flags. This is the inverse option of --preserve-fflags (unstable)" + )] + no_keep_fflags: bool, #[arg(long, value_name = "NAME", help = "Restore user from given name")]Add ArgGroup constraint (after line 52):
group(ArgGroup::new("keep-acl-flag").args(["keep_acl", "no_keep_acl"])), + group(ArgGroup::new("keep-fflags-flag").args(["keep_fflags", "no_keep_fflags"])), group(ArgGroup::new("path-transform").args(["substitutions", "transforms"])),Based on past review comments, the missing CLI integration prevents the fflags feature from being user-accessible.
422-422: Replace hardcodedFflagsStrategy::Neverwith flag-based initialization.Once the CLI flags are added to
ExtractCommand, update this line to useFflagsStrategy::from_flagslike the other preservation strategies (see lines 416, 420, 421).🔎 Proposed fix
- fflags_strategy: FflagsStrategy::Never, + fflags_strategy: FflagsStrategy::from_flags(args.keep_fflags, args.no_keep_fflags),Based on past review comments, this hardcoded value prevents the fflags restoration feature from being reachable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cli/src/chunk.rscli/src/chunk/fflag.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/fs.rscli/src/utils/fs/nodump.rscli/src/utils/os/unix/fs.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- cli/src/utils/fs.rs
- cli/src/command/core.rs
- cli/src/chunk.rs
- cli/src/command/create.rs
- cli/src/command/stdio.rs
- cli/src/utils/fs/nodump.rs
🧰 Additional context used
🧬 Code graph analysis (3)
cli/src/command/append.rs (1)
cli/src/command/core.rs (3)
collect_items_from_paths(359-373)create_entry(603-645)entry_option(647-667)
cli/src/utils/os/unix/fs.rs (1)
cli/src/utils/fs.rs (2)
get_flags(69-89)set_flags(91-111)
cli/src/chunk/fflag.rs (2)
lib/src/chunk/types.rs (1)
from_unchecked(225-227)lib/src/chunk.rs (1)
from_data(288-300)
⏰ 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). (14)
- GitHub Check: Analyze (actions)
- GitHub Check: tier1 (windows-latest, stable)
- GitHub Check: tier1 (macos-latest, beta)
- GitHub Check: tier1 (macos-latest, nightly)
- GitHub Check: tier1 (macos-latest, stable)
- GitHub Check: Test WebAssembly (nightly-2025-12-09, wasm32-wasip1)
- GitHub Check: Test WebAssembly (stable, wasm32-wasip1)
- GitHub Check: Test WebAssembly (beta, wasm32-unknown-unknown)
- GitHub Check: Test WebAssembly (stable, wasm32-unknown-unknown)
- GitHub Check: msrv (ubuntu-latest, portable-network-archive)
- GitHub Check: Test WebAssembly (stable, wasm32-unknown-emscripten)
- GitHub Check: msrv (ubuntu-latest, libpna)
- GitHub Check: tier3_cross (redoxos/redoxer, x86_64-unknown-redox)
- GitHub Check: rust_check_doc
🔇 Additional comments (10)
cli/src/chunk/fflag.rs (2)
1-11: LGTM! Naming convention is intentional per PNA specification.The
ffLgnaming follows PNA chunk naming semantics where case indicates metadata properties. The#[allow(non_upper_case_globals)]is appropriate here as the case carries semantic meaning per the documented convention.
12-14: LGTM! Clean helper function.The implementation correctly creates a RawChunk from string data using the ffLg chunk type.
cli/src/ext.rs (1)
16-16: LGTM! Clean implementation of fflags extraction.The method correctly filters ffLg chunks and converts their UTF-8 data to strings, gracefully handling invalid UTF-8 by skipping it.
Also applies to: 48-60
cli/src/utils/os/unix/fs.rs (6)
23-58: LGTM! Correct implementation of macOS flag retrieval.The function properly uses
lstatto avoid following symlinks and correctly maps BSD flag constants to libarchive-compatible names.
60-92: Acknowledged: Overwrites existing flags (intentional to match libarchive behavior).The implementation overwrites all existing flags rather than merging, which matches libarchive/bsdtar behavior on BSD systems. This is documented and intentional, though it creates cross-platform inconsistency with the Linux implementation (which merges flags).
94-160: LGTM! Robust Linux flag retrieval with proper error handling.The implementation correctly handles symlinks (ELOOP) and unsupported filesystems (ENOTTY/EOPNOTSUPP), returning empty vectors in those cases rather than failing. Flag mapping to libarchive-compatible names is correct.
162-230: LGTM! Linux set_flags correctly merges with existing flags.The implementation reads current flags first (line 197) and merges them with new flags (line 210), which matches libarchive behavior on Linux. This differs from BSD systems but is documented and intentional.
235-272: LGTM! Clean FreeBSD flag retrieval implementation.The function correctly uses
lstat(via nix crate) to retrieve file flags without following symlinks, and properly maps BSD flags to libarchive-compatible names.
274-313: Acknowledged: Overwrites existing flags (intentional to match libarchive behavior).Like the macOS implementation, this overwrites all existing flags rather than merging. This is documented and intentional to match libarchive/bsdtar behavior on BSD systems, creating cross-platform inconsistency with Linux (which merges flags).
cli/src/command/extract.rs (1)
917-932: File flags restoration logic is well-implemented but currently unreachable.The restoration logic correctly follows the pattern established for extended attributes (lines 894-906):
- Checks the strategy before attempting restoration
- Gracefully handles unsupported filesystems with a warning
- Only attempts to set flags when they exist
- Propagates other errors appropriately
However, this code is currently unreachable because
fflags_strategyis hardcoded toFflagsStrategy::Neveron line 422.Once the CLI flags are added and line 422 is updated to use
FflagsStrategy::from_flags, verify that:
item.fflags()returns the expected collection typeutils::fs::set_flagscorrectly applies the flags on supported platforms- The error handling properly distinguishes between unsupported filesystem and other failure modes
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.