Cli/list/ o/ show fflags#2569
Conversation
|
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 a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (7)
🔇 Additional comments (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @ChanTsune, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the 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 file flags (fflags) in the CLI. The primary change is the addition of a -O/--show-fflags option to the list command, allowing users to view file flags in various output formats. The implementation is thorough, including a new ffLg chunk type, platform-specific logic for macOS, Linux, and FreeBSD to get/set these flags, and comprehensive tests for the new listing functionality. The underlying infrastructure for creating and extracting archives with file flags is also added. The code is well-structured, but I have a couple of suggestions to improve consistency and safety in the macOS-specific implementation.
![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 (4)
cli/src/ext.rs (1)
47-60: Clean implementation with a note on error handling.The implementation is idiomatic and efficient. One consideration: invalid UTF-8 data in ffLg chunks is silently ignored (line 54). This is likely acceptable for robustness, but you may want to log a warning for corrupted flag data to aid debugging.
Optional: Log invalid UTF-8 flag data
#[inline] fn fflags(&self) -> Vec<String> { self.extra_chunks() .iter() .filter_map(|c| { if c.ty() == chunk::ffLg { - std::str::from_utf8(c.data()).ok().map(str::to_string) + match std::str::from_utf8(c.data()) { + Ok(s) => Some(s.to_string()), + Err(e) => { + log::warn!("Invalid UTF-8 in ffLg chunk: {e}"); + None + } + } } else { None } }) .collect() }cli/src/command/append.rs (1)
394-394: Consider adding CLI flags for file flags preservation in append mode.The
fflags_strategyis hardcoded toFflagsStrategy::Never, while other strategies likepermission_strategy,xattr_strategy, andacl_strategyusefrom_flags()to respect user-provided CLI options. This inconsistency means users cannot control file flags preservation during append operations, even though the stdio.rs implementation (which also handles append) supports--keep-fflagsand--no-keep-fflagsflags.Proposed enhancement to add fflags CLI support
Add the CLI flags to
AppendCommand:+ #[arg( + long, + visible_aliases = ["preserve-fflags", "fflags"], + requires = "unstable", + help = "Archiving the file flags of the files (unstable)" + )] + keep_fflags: bool, + #[arg( + long, + visible_aliases = ["no-preserve-fflags", "no-fflags"], + requires = "unstable", + help = "Do not archive file flags of files. This is the inverse option of --keep-fflags (unstable)" + )] + no_keep_fflags: bool,Add the ArgGroup to the command attributes:
+ group(ArgGroup::new("keep-fflags-flag").args(["keep_fflags", "no_keep_fflags"])),Update the initialization:
- fflags_strategy: FflagsStrategy::Never, + fflags_strategy: FflagsStrategy::from_flags(args.keep_fflags, args.no_keep_fflags),cli/src/command/update.rs (1)
403-403: Consider adding CLI flags for file flags preservation in update mode.Similar to the append command, the
fflags_strategyis hardcoded toFflagsStrategy::Neverinstead of usingfrom_flags()to respect user preferences. The stdio.rs implementation supports--keep-fflagsin update mode (line 1047), but the standaloneUpdateCommanddoes not expose these flags, creating an inconsistency between the two interfaces.Proposed enhancement to add fflags CLI support
Add the CLI flags to
UpdateCommand(similar to other preserve options):+ #[arg( + long, + visible_aliases = ["preserve-fflags", "fflags"], + requires = "unstable", + help = "Archiving the file flags of the files (unstable)" + )] + keep_fflags: bool, + #[arg( + long, + visible_aliases = ["no-preserve-fflags", "no-fflags"], + requires = "unstable", + help = "Do not archive file flags of files. This is the inverse option of --keep-fflags (unstable)" + )] + no_keep_fflags: bool,Add the ArgGroup:
+ group(ArgGroup::new("keep-fflags-flag").args(["keep_fflags", "no_keep_fflags"])),Update the initialization:
- fflags_strategy: FflagsStrategy::Never, + fflags_strategy: FflagsStrategy::from_flags(args.keep_fflags, args.no_keep_fflags),cli/src/command/extract.rs (1)
422-422: Consider adding CLI flags for file flags preservation in extract mode.The
fflags_strategyis hardcoded toFflagsStrategy::Never, preventing users from restoring file flags during extraction. The stdio.rs implementation (line 750) supports--keep-fflagsflags for extract operations, but the standaloneExtractCommanddoes not expose these options.Proposed enhancement to add fflags CLI support
Add the CLI flags to
ExtractCommand:+ #[arg( + long, + visible_alias = "preserve-fflags", + requires = "unstable", + help = "Restore the file flags of the files (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 --keep-fflags (unstable)" + )] + no_keep_fflags: bool,Add the ArgGroup:
+ group(ArgGroup::new("keep-fflags-flag").args(["keep_fflags", "no_keep_fflags"])),Update the initialization:
- fflags_strategy: FflagsStrategy::Never, + fflags_strategy: FflagsStrategy::from_flags(args.keep_fflags, args.no_keep_fflags),
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
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/list.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.rscli/tests/cli/list.rscli/tests/cli/list/option_show_fflags.rsresources/test/zstd_keep_fflags.pna
🧰 Additional context used
🧬 Code graph analysis (6)
cli/src/utils/os/unix/fs.rs (1)
cli/src/utils/fs.rs (2)
get_flags(69-89)set_flags(91-111)
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/fs.rs (1)
cli/src/utils/os/unix/fs.rs (6)
get_flags(24-58)get_flags(108-160)get_flags(235-272)set_flags(68-87)set_flags(171-230)set_flags(282-313)
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/core.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)
⏰ 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). (7)
- GitHub Check: Test on NetBSD
- GitHub Check: tier1 (ubuntu-22.04-arm, nightly)
- GitHub Check: tier1 (ubuntu-22.04-arm, stable)
- GitHub Check: tier1 (windows-11-arm, nightly)
- GitHub Check: tier1 (macos-latest, stable)
- GitHub Check: tier1 (macos-latest, nightly)
- GitHub Check: msrv (ubuntu-latest, portable-network-archive)
🔇 Additional comments (39)
cli/src/utils/fs/nodump.rs (1)
1-9: Clean refactoring to use centralized flag API.The implementation correctly delegates to the new
get_flagsAPI and performs a simple string comparison. This is a good simplification over the previous platform-specific code paths.cli/src/utils/os/unix/fs.rs (7)
23-58: Solid macOS flag retrieval implementation.The lstat-based approach correctly extracts BSD file flags and maps them to human-readable names. The flag mapping aligns with standard BSD conventions.
94-105: Correct Linux file flag constants.The flag definitions match the Linux kernel's
linux/fs.hheader values and cover the most commonly used ext2/ext3/ext4/btrfs attributes.
107-160: Robust Linux flag retrieval with proper edge case handling.The implementation correctly handles symlinks (ELOOP), unsupported filesystems (ENOTTY/EOPNOTSUPP), and maps Linux flags to libarchive-compatible BSD names. The ioctl approach is the standard method for Linux file attributes.
162-230: Linux flag setter with correct merge semantics.The read-modify-write approach matches libarchive behavior on Linux. Good optimizations include early return for empty flags and skipping the write when flags haven't changed. The error handling for symlinks and unsupported filesystems is appropriate.
234-272: FreeBSD flag retrieval with platform-specific flags.Correctly includes FreeBSD-specific flags (UF_NOUNLINK, SF_NOUNLINK) not present on macOS. Uses nix's lstat wrapper for cleaner error handling.
282-313: FreeBSD flag setter - verify empty flags behavior.The early return on line 285-287 prevents clearing all flags when an empty slice is passed. Note that the macOS implementation doesn't have this early return, meaning it would call
lchflags(path, 0)for empty flags (clearing all flags).If the intent is to never clear flags when given an empty slice, this is correct. If parity with macOS is desired, consider adding the same check to macOS
set_flags.
60-92: Extern declaration forlchflagsis correct.The function signature matches macOS specifications, and all flag constants are available in the libc crate. The implementation correctly handles symlink-safe flag setting with proper error handling.
cli/src/chunk.rs (1)
2-5: LGTM!The module declaration and re-export follow the existing pattern established by the
aclmodule.cli/tests/cli/list.rs (1)
20-21: LGTM!The test module declaration follows the existing pattern and appropriately excludes wasm targets where file flags aren't applicable.
cli/src/chunk/fflag.rs (1)
1-14: Well-documented chunk type definition.The chunk naming convention is clearly documented. The
unsafeusage is acceptable here since*b"ffLg"is a compile-time constant with exactly 4 ASCII bytes, which is the requirement forChunkType.The
#[allow(non_upper_case_globals)]attribute appropriately handles Rust's naming conventions while preserving the semantically meaningful case in the chunk identifier.cli/src/command/create.rs (2)
9-9: LGTM!Import addition for the new
FflagsStrategyenum.
480-480: Sensible default for file flags strategy.Setting
FflagsStrategy::Neveras the default is appropriate for backward compatibility. Users will need to explicitly opt-in to preserve file flags.cli/src/utils/fs.rs (2)
69-89: LGTM! Platform-specific flag retrieval is well-structured.The
get_flagsfunction properly delegates to platform-specific implementations on supported Unix variants (macOS, Linux, Android, FreeBSD) and returns an empty vector on unsupported platforms. This pattern is consistent with other platform-specific functions in this file likelchown.
91-111: LGTM! Platform-specific flag setting follows established patterns.The
set_flagsfunction correctly implements a no-op for unsupported platforms and delegates to the Unix-specific implementation on supported systems. The error handling strategy (returningOk(())on unsupported platforms) aligns with the existing codebase patterns.cli/src/command/extract.rs (1)
917-932: LGTM! File flags restoration follows established patterns.The file flags restoration logic correctly mirrors the ACL and xattr restoration patterns:
- Checks the strategy before applying flags
- Handles empty flag lists appropriately
- Provides user-friendly warnings for unsupported filesystems
- Propagates actual errors for proper failure handling
cli/src/command/stdio.rs (6)
191-204: LGTM! CLI arguments for file flags are well-structured.The
keep_fflagsandno_keep_fflagsarguments are properly defined with:
- Appropriate visibility aliases for user convenience
- Unstable feature gate for experimental functionality
- Clear help text describing the behavior
The ArgGroup at line 63 correctly prevents conflicting flags.
656-656: LGTM! File flags strategy properly wired in create mode.The
FflagsStrategy::from_flagscall correctly translates CLI arguments into the strategy, allowing users to control file flags preservation during archive creation.
750-750: LGTM! File flags strategy properly wired in extract mode.Consistent with create mode, the extract path correctly uses
from_flagsto respect user preferences for file flags restoration.
910-910: LGTM! File flags strategy properly wired in append mode.The append path correctly uses
from_flags, enabling user control over file flags preservation when appending to archives.
1047-1047: LGTM! File flags strategy properly wired in update mode.Consistent with other modes, the update path properly uses
from_flagsto respect user preferences.
827-827: The feature is already implemented.The list command has a fully functional
--show-fflags/-Oflag (defined in cli/src/command/list.rs lines 74-78) with comprehensive test coverage in cli/tests/cli/list/option_show_fflags.rs. Users can already control fflags display via-Oin list mode.The hardcoded
show_fflags: falsein cli/src/command/stdio.rs line 827 is part of a separate tar-compatible stdin/stdout interface (therun_list_archivefunction), whereshow_xattrandshow_aclare also hardcoded. This appears to be an intentionally simplified mode with restricted display options.cli/tests/cli/list/option_show_fflags.rs (6)
8-28: LGTM! Comprehensive test coverage for table format.The test validates that file flags are correctly displayed in table format with the
-Oflag, including multiple flag combinations and proper formatting.
34-56: LGTM! Header validation is thorough.The test confirms that the "Fflags" column header is properly included and aligned when using
--headerwith-O.
62-85: LGTM! CSV format handling is correct.The test validates CSV output including proper quoting for multi-flag entries (e.g.,
"hidden,schg"), which is essential for CSV parsing.
91-117: LGTM! JSONL format correctly represents fflags as arrays.The test validates that file flags are properly serialized as JSON arrays, enabling structured parsing of the output.
123-148: LGTM! Important negative test case.This test validates that fflags are not included in JSON output when the
-Oflag is not specified, ensuring clean output by default.
154-173: LGTM! Ensures backward compatibility.This test confirms that the fflags column is not displayed by default, maintaining existing behavior when the
-Oflag is not used.cli/src/command/core.rs (3)
164-180: LGTM! Consistent implementation following established patterns.The
FflagsStrategyenum and itsfrom_flagsconstructor follow the same design asXattrStrategy,AclStrategy, andPermissionStrategy, ensuring consistency across the codebase.
188-188: LGTM! Proper integration into KeepOptions.The
fflags_strategyfield is appropriately placed alongside other metadata preservation strategies.
791-807: LGTM! Correct implementation with appropriate error handling.The file flags handling follows the same pattern as extended attributes (lines 770-786), including proper error handling for unsupported filesystems and platforms. The warning for unsupported cases is helpful for users.
cli/src/command/list.rs (8)
73-78: LGTM! Clear and well-documented CLI option.The
-O/--show-fflagsflag follows existing conventions and provides helpful examples of flag types in the help text.
319-319: LGTM! Proper data model extension.The
fflagsfield is appropriately added toTableRowand populated using theNormalEntryExt::fflags()method, maintaining consistency with other metadata fields.Also applies to: 393-393
419-419: LGTM! Proper options propagation.The
show_fflagsflag is correctly threaded fromListCommandargs toListOptions, maintaining the established pattern.Also applies to: 506-506
730-793: LGTM! Clean conditional column implementation.The header and row building logic elegantly handles the optional Fflags column using
Option::then()andflatten(), maintaining consistency between headers and data. The formatting correctly shows "-" for no flags and comma-separated values for multiple flags.
829-852: LGTM! Correct column indexing and styling.The column alignment calculation properly adjusts for the optional Fflags column:
- Without
-O: aligns columns 3-4 (Raw Size, Compressed Size)- With
-O: aligns columns 4-5 (Raw Size, Compressed Size after Fflags)The colorization array uses
Optionandflatten()to conditionally include the Fflags color, maintaining the correct color-to-column mapping.
1044-1045: LGTM! Proper JSON serialization with conditional inclusion.The
fflagsfield inFileInfouses#[serde(skip_serializing_if = "Option::is_none")]to cleanly omit the field when not requested, and the conditional logic correctly includes the flags array only whenshow_fflagsis true.Also applies to: 1062-1107
1160-1222: LGTM! CSV/TSV output correctly handles optional fflags column.The delimited output logic consistently applies the conditional column pattern. The comma-joining for multi-flag entries (line 1208) is safe since flag names are simple identifiers, and the
csvcrate automatically handles quoting when needed.
865-881: LGTM! Useful helper for name formatting.The
detailed_format_namefunction consolidates entry name formatting logic, properly handling both theclassifyoption (adding/and@indicators) andhide_control_charsoption. The implementation correctly handles all entry types.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ChanTsune
force-pushed
the
cli/list/-O/--show-fflags
branch
from
e819cb0 to
3596fc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/src/command/list.rs (1)
1194-1213: Closure usage is appropriate here.Line 1208 uses
.then(|| row.fflags.join(","))instead of.then_some(row.fflags.join(",")). This is correct because:
- The closure avoids evaluating
join(",")whenshow_fflagsis false- Using
then_some()would always execute the join operationIf static analysis flags this as an "unnecessary closure," it would be a false positive.
🧹 Nitpick comments (1)
cli/src/command/list.rs (1)
865-881: LGTM!Good refactoring to extract the name formatting logic into a dedicated helper function. This improves code maintainability and reduces duplication.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cli/src/command/list.rscli/src/command/stdio.rscli/tests/cli/list.rscli/tests/cli/list/option_show_fflags.rsresources/test/zstd_keep_fflags.pna
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/tests/cli/list.rs
- cli/src/command/stdio.rs
- cli/tests/cli/list/option_show_fflags.rs
🔇 Additional comments (11)
cli/src/command/list.rs (11)
73-78: LGTM!The CLI argument definition is clear and follows standard patterns. The short flag
-Ois a reasonable choice for displaying file flags.
319-319: LGTM!The
fflagsfield is appropriately typed asVec<String>to hold multiple flag names.
393-393: LGTM!The fflags are correctly populated from the entry's metadata.
419-419: LGTM!The
show_fflagsoption is correctly threaded through the command to the options struct.Also applies to: 506-506
730-742: LGTM!The conditional header construction using
then_some()andflatten()is clean and correctly includes the Fflags column when enabled.
756-793: LGTM!The row construction correctly includes the optional fflags column with appropriate formatting (comma-separated list or "-" for empty). The closure at line 766 is necessary for the conditional logic.
829-852: LGTM!The alignment and colorization logic correctly accounts for the conditional Fflags column. The size column indices are properly calculated: when fflags are shown, Raw Size is column 4 and Compressed Size is column 5; otherwise they're columns 3 and 4.
1044-1045: LGTM!The conditional serialization of fflags using
skip_serializing_ifensures the field is only included in JSON output when requested.
1062-1066: LGTM!The signature update to accept
optionsparameter is necessary to support the conditional fflags output.
1103-1107: LGTM!The fflags are correctly included in JSONL output only when
show_fflagsis enabled.
1160-1175: LGTM!The CSV/TSV header correctly includes the fflags column when enabled, following the same pattern used for the table format.
ChanTsune
force-pushed
the
cli/list/-O/--show-fflags
branch
from
3596fc7 to
f32bc2f
Compare
2 participants
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.