Cli/fflag/introduce fflag get set#2782
Open
ChanTsune wants to merge 7 commits into
Open
Conversation
Contributor
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 functionality for managing file flags (fflags) within PNA archives. It provides new experimental CLI commands to get and set file flags, along with options to preserve these flags during archive creation, extraction, and modification. The changes enhance metadata handling by integrating platform-specific file flag support, making the archiving process more comprehensive for systems that utilize fflags. Highlights
Changelog
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
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new private Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User/CLI
participant Dispatcher as Experimental Dispatcher
participant Cmd as FflagCommand
participant Archive as PNA Archive Reader/Writer
participant Transform as Transform Engine
participant Codec as Archive Codec
User->>Dispatcher: pna experimental fflag ... (get/set)
Dispatcher->>Cmd: dispatch FflagCommand
Cmd->>Archive: open/read archive entries & ffLg/fflag chunks
Archive-->>Cmd: entries + current flags
alt get
Cmd->>Cmd: parse filters (name/regex) / dump/long format
Cmd->>User: emit results (simple/long/dump)
else set
Cmd->>Cmd: determine strategy (restore vs apply ops)
alt restore
Cmd->>Cmd: parse dump -> per-entry flag map
else apply
Cmd->>Cmd: parse FlagOperations (clears then sets)
end
Cmd->>Transform: apply computed flags to entries
Transform->>Codec: produce transformed archive data
Codec->>Archive: write new archive, replace ffLg with fflag chunks
Cmd->>User: report success/failures
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new experimental command fflag with get and set subcommands to manipulate file flags within an archive. This is a significant feature addition, including a new chunk type ffLg, platform-specific handling for file flags on macOS, Linux, and FreeBSD, and extensive test coverage.
The implementation is solid and well-structured. I have a couple of minor suggestions to improve code clarity and make it more idiomatic. Overall, great work on this feature!
Comment on lines
+193
to
+211
| if let Some(base) = flag.strip_prefix("no") { | ||
| // "nodump" is a flag itself, not "no" + "dump" | ||
| if base == "dump" { | ||
| return FlagOp::Set(normalize_flag_name(flag)); | ||
| } | ||
| if is_known_flag(base) || is_known_flag(flag) { | ||
| // If base is known, it's a clear operation | ||
| if is_known_flag(base) { | ||
| return FlagOp::Clear(normalize_flag_name(base)); | ||
| } | ||
| } | ||
| // Check if the full "no*" form is itself a known flag | ||
| if is_known_flag(flag) { | ||
| return FlagOp::Set(normalize_flag_name(flag)); | ||
| } | ||
| // Unknown but has "no" prefix - treat as clear | ||
| log::warn!("Unknown flag: {}", base); | ||
| return FlagOp::Clear(base.into()); | ||
| } |
Contributor
There was a problem hiding this comment.
The logic for parsing flag operations with a "no" prefix can be simplified for better readability. The current implementation has some redundant checks.
if let Some(base) = flag.strip_prefix("no") {
// "nodump" is a flag itself, not "no" + "dump"
if base == "dump" {
return FlagOp::Set(normalize_flag_name(flag));
}
if is_known_flag(base) {
return FlagOp::Clear(normalize_flag_name(base));
}
// Check if the full "no*" form is itself a known flag
if is_known_flag(flag) {
return FlagOp::Set(normalize_flag_name(flag));
}
// Unknown but has "no" prefix - treat as clear
log::warn!("Unknown flag: {}", base);
return FlagOp::Clear(base.into());
}
Comment on lines
+463
to
+477
| // Remove existing ffLg chunks | ||
| let extra_without_fflags: Vec<_> = entry | ||
| .extra_chunks() | ||
| .iter() | ||
| .filter(|c| c.ty() != chunk::ffLg) | ||
| .cloned() | ||
| .collect(); | ||
|
|
||
| // Add new ffLg chunks | ||
| let mut extra_chunks = extra_without_fflags; | ||
| for flag in flags { | ||
| extra_chunks.push(chunk::fflag_chunk(flag).into()); | ||
| } | ||
|
|
||
| entry.with_extra_chunks(extra_chunks) |
Contributor
There was a problem hiding this comment.
This function can be written more idiomatically using iterator chaining, which would make it more concise.
// Rebuild extra chunks, removing old ffLg chunks and adding the new ones.
let extra_chunks = entry
.extra_chunks()
.iter()
.filter(|c| c.ty() != chunk::ffLg)
.cloned()
.chain(flags.iter().map(|flag| chunk::fflag_chunk(flag).into()))
.collect();
entry.with_extra_chunks(extra_chunks)![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
cli/tests/cli/fflag/set/chflags_style.rs (1)
10-188: Same OS-gating concern as option_match.rs.
These tests depend on BSD-style flags and should be guarded to supported OSes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/tests/cli/fflag/set/chflags_style.rs` around lines 10 - 188, These tests use BSD-style file flags and must be OS-gated; update the three test functions (fflag_set_chflags_style_set, fflag_set_chflags_style_clear, fflag_set_dump_clears_nodump) to run only on supported OSes (e.g., macOS/FreeBSD) by adding an appropriate gate — either a compile-time attribute like #[cfg(any(target_os = "macos", target_os = "freebsd"))] or a runtime skip check at the top of each test (call a helper like ensure_bsd_flags_supported() that returns early/uses insta::skip_if) so the tests are not executed on non-BSD platforms. Ensure the gating is applied to the exact test functions named above.cli/tests/cli/fflag/get/option_long.rs (1)
8-134: Same OS-gating concern as option_match.rs.
These tests useuchg/hiddenand will fail on Linux/Windows; please guard them to supported OSes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/tests/cli/fflag/get/option_long.rs` around lines 8 - 134, The tests fflag_get_long_format and fflag_get_long_multiple_entries use BSD/macOS-specific file flags (uchg/hidden) and must be OS-gated; wrap or annotate these test functions (fn fflag_get_long_format and fn fflag_get_long_multiple_entries) so they only compile/run on supported OSes (e.g. add #[cfg(any(target_os = "macos", target_os = "freebsd", target_os = "openbsd"))] above each test or use cfg_attr to ignore them elsewhere), ensuring the tests are excluded on Linux/Windows.cli/tests/cli/fflag/set/clear_flags.rs (1)
10-175: Same OS-gating concern as option_match.rs.
These tests rely on BSD-style flags and should be guarded to supported OSes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/tests/cli/fflag/set/clear_flags.rs` around lines 10 - 175, These tests use BSD-style file flags and must be OS-gated; add a conditional compile attribute above each test function (fflag_clear_single_flag, fflag_clear_multiple_flags, fflag_clear_nonexistent_flag) to run only on BSD-like OSes—for example annotate each test with #[cfg(any(target_os = "freebsd", target_os = "openbsd", target_os = "netbsd", target_os = "dragonfly", target_os = "macos"))] (or the same set used in option_match.rs) so the tests are skipped on unsupported platforms.cli/tests/cli/fflag/get/option_name.rs (1)
10-130: Same OS-gating concern as option_match.rs.
These tests rely on BSD-style flags and should be guarded to supported OSes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/tests/cli/fflag/get/option_name.rs` around lines 10 - 130, Tests fflag_get_filter_by_name and fflag_get_filter_by_name_no_match rely on BSD-style file flags and must be OS-gated; add conditional compilation so they only run on BSD-like OSes. Wrap the tests (or the module) with a cfg attribute such as #[cfg(any(target_os = "freebsd", target_os = "openbsd", target_os = "netbsd", target_os = "macos"))] (or equivalent gating used in option_match.rs) so fflag_get_filter_by_name and fflag_get_filter_by_name_no_match are compiled/run only on supported platforms.
🧹 Nitpick comments (4)
cli/src/command/create.rs (1)
480-480: HardcodedFflagsStrategy::Neveris intentional for this PR but worth tracking.There is no
--keep-fflags(or--preserve-fflags) CLI argument wired intoCreateCommand, so file flags will never be captured duringcreate. This is the right scope for an initial experimental landing — flags are managed explicitly via the newfflag setcommand — but if automatic preservation is a future goal, this is the field and the struct to extend.If you'd like, I can open a follow-up issue or draft the
--keep-fflagswiring (including theArgGroup, the newCreateCommandfield, and theKeepOptionsresolver) for a future PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/command/create.rs` at line 480, CreateCommand currently hardcodes fflags_strategy: FflagsStrategy::Never which prevents preserving file flags; to add an opt-in flag preserve behavior, wire a new CLI boolean (e.g., --keep-fflags / --preserve-fflags) into CreateCommand, add a corresponding field on the CreateCommand struct, include it in the appropriate ArgGroup, and update the resolver (KeepOptions or similar) to set fflags_strategy to the desired FflagsStrategy when the flag is present instead of FflagsStrategy::Never; reference the CreateCommand struct, the fflags_strategy field and the FflagsStrategy enum when making these changes.cli/tests/cli/fflag/get/option_dump.rs (1)
54-57: Consider asserting that the specific flags appear in the dump output.The test sets three flags (
uchg,nodump,hidden) but only asserts that"flags="is present without verifying those flags appear in the output. Adding assertions for the actual flag names would strengthen the test and catch regressions in dump formatting..stdout(predicates::str::contains( "# file: fflag_get_dump/testfile.txt", )) - .stdout(predicates::str::contains("flags=")); + .stdout(predicates::str::contains("flags=")) + .stdout(predicates::str::contains("uchg")) + .stdout(predicates::str::contains("nodump")) + .stdout(predicates::str::contains("hidden"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/tests/cli/fflag/get/option_dump.rs` around lines 54 - 57, The test in option_dump.rs currently only asserts "flags=" is present but not the actual flag names; update the test to assert that the expected flags "uchg", "nodump", and "hidden" appear in the dump output (either by checking the combined substring like "flags=uchg,nodump,hidden" or by adding three separate stdout predicates that each contains "uchg", "nodump", and "hidden") so the test verifies the actual flags are printed.cli/src/command/core.rs (1)
791-807: Consider adding a warning on platforms where fflags are silently a no-op.The fflags handling correctly delegates to
utils::fs::get_flags, which returnsOk(Vec::new())on unsupported platforms. However, unlike xattr (line 789) and ACL (lines 762-763), this means a user who passes--keep-fflagson an unsupported platform gets no feedback that the flags are being silently ignored. The other strategies explicitly warn the user in that scenario.If this is intentional (e.g., because the platform dispatch is more granular than a simple unix/non-unix split), a brief comment documenting the rationale would help future readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/command/core.rs` around lines 791 - 807, The code currently treats utils::fs::get_flags(path) returning Ok(Vec::new()) as silently doing nothing when keep_options.fflags_strategy is FflagsStrategy::Always, so users passing --keep-fflags on unsupported platforms get no feedback; update the FflagsStrategy::Always handling in the block using utils::fs::get_flags to detect an empty flags Vec and emit a warning (similar to xattr/ACL handling) that fflags are unsupported or no flags were found for the given path, mentioning the path variable, before skipping adding chunks via entry.add_extra_chunk(crate::chunk::fflag_chunk(&flag)); alternatively add a brief comment explaining why silent no-op is intentional if you choose not to warn.cli/src/command/extract.rs (1)
403-423: Consider exposing keep-fflags options forextract.
fflags_strategyis hard-coded toNever, so the extract subcommand can’t preserve file flags even if the archive contains them. If this command should support fflags like the stdio path, add--keep-fflags/--no-keep-fflags(unstable) and drive this viaFflagsStrategy::from_flags(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/command/extract.rs` around lines 403 - 423, The extract command currently hard-codes fflags_strategy to FflagsStrategy::Never which prevents preserving file flags; update the extract CLI to accept unstable flags (e.g. --keep-fflags / --no-keep-fflags) on the args struct and then replace FflagsStrategy::Never in the keep_options construction with FflagsStrategy::from_flags(args.keep_fflags, args.no_keep_fflags) so the KeepOptions uses the requested behavior; ensure the new args are wired into the extract subcommand parsing and marked unstable if intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/command/extract.rs`:
- Around line 917-932: The code silently drops requested file flags on platforms
where utils::fs::set_flags is a no-op; when keep_options.fflags_strategy ==
FflagsStrategy::Always and item.fflags() is non-empty you should emit a warning
instead of silently succeeding. Fix by either (a) updating utils::fs::set_flags
to return Err(std::io::ErrorKind::Unsupported) on unsupported platforms so the
existing Err(e) branch logs a warning, or (b) adding a cfg!(not(unix)) check in
the FflagsStrategy::Always branch around utils::fs::set_flags(path, &flags) and
call log::warn!(...) (similar to the xattr handling) when flags are non-empty to
indicate file flags aren’t supported for path. Ensure you reference
keep_options.fflags_strategy, item.fflags(), utils::fs::set_flags, and path in
your change so the warning is emitted where flags would otherwise be dropped.
In `@cli/src/command/fflag.rs`:
- Around line 219-238: The apply method in FlagOperations should normalize
stored flags before building the HashSet so alias names (e.g., "uimmutable") map
to their canonical forms and clears like FlagOp::Clear("nouchg") will actually
remove their aliases; update FlagOperations::apply to map each item of
current_flags through the existing normalization routine (or add a small
alias->canonical mapping if none exists) when creating flags, and ensure
FlagOp::Set and FlagOp::Clear also use the same normalization for their flag
names when inserting/removing so canonicalization is consistent throughout.
In `@cli/src/ext.rs`:
- Around line 49-60: fflags() currently swallows UTF-8 decode failures for
chunk::ffLg by using .ok(), which hides corrupted chunks unlike acl() which
returns io::Result; change fflags() to return io::Result<Vec<String>> (aligning
with acl()), iterate self.extra_chunks(), and for each chunk with c.ty() ==
chunk::ffLg attempt
std::str::from_utf8(c.data()).map(str::to_string).map_err(|e|
io::Error::new(io::ErrorKind::InvalidData, format!("ffLg chunk UTF-8 decode
error: {}", e)))? and collect results, propagating the first error; update the
trait/signature that declares fflags() and adjust all call sites to handle the
Result (or log errors where appropriate) so decode failures are surfaced instead
of silently dropped.
In `@cli/src/utils/os/unix/fs.rs`:
- Around line 171-229: The set_flags function currently only ORs bits into
new_flags and returns early on empty flags, so it can never clear bits; change
the logic to build two masks (set_mask and clear_mask) by parsing each entry in
flags (treat entries prefixed with "no" as clears, accept synonyms like
"schg"/"simmutable", "sappnd"/"sappend", "compr"/"compress", etc.), remove the
early return on empty flags, then compute new_flags = (current_flags | set_mask)
& !clear_mask and call fs_ioc_setflags(fd.as_raw_fd(), &new_flags) only if
new_flags != current_flags; reference set_flags, current_flags, new_flags,
fs_ioc_setflags, and the flags slice when implementing.
In `@cli/tests/cli/fflag/get/option_match.rs`:
- Around line 10-75: The test fflag_get_match_single_flag uses BSD-only flags
("uchg"/"hidden") so it must be gated to BSD-like OSes; add a compile-time cfg
or a runtime skip so it only runs on supported platforms. For example, annotate
the test function fflag_get_match_single_flag with a cfg attribute like
#[cfg(any(target_os = "macos", target_os = "freebsd", target_os = "openbsd",
target_os = "netbsd"))] or, alternatively, add an early runtime check using if
!cfg!(any(target_os = "macos", target_os = "freebsd", target_os = "openbsd",
target_os = "netbsd")) { eprintln!("skipping: fflags not supported"); return; }
before invoking the cargo_bin_cmd! calls so the create/set/get assertions for
uchg/hidden only execute where those flags are supported.
---
Duplicate comments:
In `@cli/tests/cli/fflag/get/option_long.rs`:
- Around line 8-134: The tests fflag_get_long_format and
fflag_get_long_multiple_entries use BSD/macOS-specific file flags (uchg/hidden)
and must be OS-gated; wrap or annotate these test functions (fn
fflag_get_long_format and fn fflag_get_long_multiple_entries) so they only
compile/run on supported OSes (e.g. add #[cfg(any(target_os = "macos", target_os
= "freebsd", target_os = "openbsd"))] above each test or use cfg_attr to ignore
them elsewhere), ensuring the tests are excluded on Linux/Windows.
In `@cli/tests/cli/fflag/get/option_name.rs`:
- Around line 10-130: Tests fflag_get_filter_by_name and
fflag_get_filter_by_name_no_match rely on BSD-style file flags and must be
OS-gated; add conditional compilation so they only run on BSD-like OSes. Wrap
the tests (or the module) with a cfg attribute such as #[cfg(any(target_os =
"freebsd", target_os = "openbsd", target_os = "netbsd", target_os = "macos"))]
(or equivalent gating used in option_match.rs) so fflag_get_filter_by_name and
fflag_get_filter_by_name_no_match are compiled/run only on supported platforms.
In `@cli/tests/cli/fflag/set/chflags_style.rs`:
- Around line 10-188: These tests use BSD-style file flags and must be OS-gated;
update the three test functions (fflag_set_chflags_style_set,
fflag_set_chflags_style_clear, fflag_set_dump_clears_nodump) to run only on
supported OSes (e.g., macOS/FreeBSD) by adding an appropriate gate — either a
compile-time attribute like #[cfg(any(target_os = "macos", target_os =
"freebsd"))] or a runtime skip check at the top of each test (call a helper like
ensure_bsd_flags_supported() that returns early/uses insta::skip_if) so the
tests are not executed on non-BSD platforms. Ensure the gating is applied to the
exact test functions named above.
In `@cli/tests/cli/fflag/set/clear_flags.rs`:
- Around line 10-175: These tests use BSD-style file flags and must be OS-gated;
add a conditional compile attribute above each test function
(fflag_clear_single_flag, fflag_clear_multiple_flags,
fflag_clear_nonexistent_flag) to run only on BSD-like OSes—for example annotate
each test with #[cfg(any(target_os = "freebsd", target_os = "openbsd", target_os
= "netbsd", target_os = "dragonfly", target_os = "macos"))] (or the same set
used in option_match.rs) so the tests are skipped on unsupported platforms.
---
Nitpick comments:
In `@cli/src/command/core.rs`:
- Around line 791-807: The code currently treats utils::fs::get_flags(path)
returning Ok(Vec::new()) as silently doing nothing when
keep_options.fflags_strategy is FflagsStrategy::Always, so users passing
--keep-fflags on unsupported platforms get no feedback; update the
FflagsStrategy::Always handling in the block using utils::fs::get_flags to
detect an empty flags Vec and emit a warning (similar to xattr/ACL handling)
that fflags are unsupported or no flags were found for the given path,
mentioning the path variable, before skipping adding chunks via
entry.add_extra_chunk(crate::chunk::fflag_chunk(&flag)); alternatively add a
brief comment explaining why silent no-op is intentional if you choose not to
warn.
In `@cli/src/command/create.rs`:
- Line 480: CreateCommand currently hardcodes fflags_strategy:
FflagsStrategy::Never which prevents preserving file flags; to add an opt-in
flag preserve behavior, wire a new CLI boolean (e.g., --keep-fflags /
--preserve-fflags) into CreateCommand, add a corresponding field on the
CreateCommand struct, include it in the appropriate ArgGroup, and update the
resolver (KeepOptions or similar) to set fflags_strategy to the desired
FflagsStrategy when the flag is present instead of FflagsStrategy::Never;
reference the CreateCommand struct, the fflags_strategy field and the
FflagsStrategy enum when making these changes.
In `@cli/src/command/extract.rs`:
- Around line 403-423: The extract command currently hard-codes fflags_strategy
to FflagsStrategy::Never which prevents preserving file flags; update the
extract CLI to accept unstable flags (e.g. --keep-fflags / --no-keep-fflags) on
the args struct and then replace FflagsStrategy::Never in the keep_options
construction with FflagsStrategy::from_flags(args.keep_fflags,
args.no_keep_fflags) so the KeepOptions uses the requested behavior; ensure the
new args are wired into the extract subcommand parsing and marked unstable if
intended.
In `@cli/tests/cli/fflag/get/option_dump.rs`:
- Around line 54-57: The test in option_dump.rs currently only asserts "flags="
is present but not the actual flag names; update the test to assert that the
expected flags "uchg", "nodump", and "hidden" appear in the dump output (either
by checking the combined substring like "flags=uchg,nodump,hidden" or by adding
three separate stdout predicates that each contains "uchg", "nodump", and
"hidden") so the test verifies the actual flags are printed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
cli/src/chunk.rscli/src/chunk/fflag.rscli/src/command.rscli/src/command/append.rscli/src/command/core.rscli/src/command/create.rscli/src/command/experimental.rscli/src/command/extract.rscli/src/command/fflag.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/fflag.rscli/tests/cli/fflag/compatibility.rscli/tests/cli/fflag/get.rscli/tests/cli/fflag/get/basic.rscli/tests/cli/fflag/get/missing_file.rscli/tests/cli/fflag/get/option_dump.rscli/tests/cli/fflag/get/option_long.rscli/tests/cli/fflag/get/option_match.rscli/tests/cli/fflag/get/option_name.rscli/tests/cli/fflag/set.rscli/tests/cli/fflag/set/basic.rscli/tests/cli/fflag/set/chflags_style.rscli/tests/cli/fflag/set/clear_flags.rscli/tests/cli/fflag/set/missing_file.rscli/tests/cli/fflag/set/mixed_operations.rscli/tests/cli/fflag/set/multiple_entries.rscli/tests/cli/fflag/set/option_restore.rscli/tests/cli/main.rsresources/test/zstd_keep_fflags.pna
Comment on lines
+49
to
+60
| 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) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .collect() | ||
| } |
Contributor
There was a problem hiding this comment.
Silent UTF-8 decode failure silently drops ffLg chunks — inconsistent with acl().
acl() propagates parse errors via io::Result<Acls>, but fflags() uses .ok() to silently discard any ffLg chunk whose data isn't valid UTF-8. A corrupted or malformed chunk will be quietly skipped, making it hard to diagnose archive corruption or writer bugs. At minimum, consider logging the skipped chunk, or align the signature with acl() by returning io::Result<Vec<String>>.
🛡️ Proposed fix — surface decode errors
- #[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)
- } else {
- None
- }
- })
- .collect()
- }
+ #[inline]
+ fn fflags(&self) -> io::Result<Vec<String>> {
+ self.extra_chunks()
+ .iter()
+ .filter_map(|c| {
+ if c.ty() == chunk::ffLg {
+ Some(
+ std::str::from_utf8(c.data())
+ .map(str::to_string)
+ .map_err(io::Error::other),
+ )
+ } else {
+ None
+ }
+ })
+ .collect()
+ }Also update the trait definition and all call sites accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/ext.rs` around lines 49 - 60, fflags() currently swallows UTF-8
decode failures for chunk::ffLg by using .ok(), which hides corrupted chunks
unlike acl() which returns io::Result; change fflags() to return
io::Result<Vec<String>> (aligning with acl()), iterate self.extra_chunks(), and
for each chunk with c.ty() == chunk::ffLg attempt
std::str::from_utf8(c.data()).map(str::to_string).map_err(|e|
io::Error::new(io::ErrorKind::InvalidData, format!("ffLg chunk UTF-8 decode
error: {}", e)))? and collect results, propagating the first error; update the
trait/signature that declares fflags() and adjust all call sites to handle the
Result (or log errors where appropriate) so decode failures are surfaced instead
of silently dropped.
Comment on lines
+171
to
+229
| pub(crate) fn set_flags(path: &Path, flags: &[String]) -> io::Result<()> { | ||
| use linux_flags::*; | ||
| use nix::fcntl::{OFlag, open}; | ||
| use nix::sys::stat::Mode; | ||
| use std::os::fd::AsRawFd; | ||
|
|
||
| if flags.is_empty() { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| nix::ioctl_read_bad!(fs_ioc_getflags, libc::FS_IOC_GETFLAGS, libc::c_int); | ||
| nix::ioctl_write_ptr_bad!(fs_ioc_setflags, libc::FS_IOC_SETFLAGS, libc::c_int); | ||
|
|
||
| let fd = match open(path, OFlag::O_RDONLY | OFlag::O_NOFOLLOW, Mode::empty()) { | ||
| Ok(fd) => fd, | ||
| Err(nix::errno::Errno::ELOOP) => { | ||
| // Symlinks don't support file flags on Linux | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::Unsupported, | ||
| "symlinks do not support file flags", | ||
| )); | ||
| } | ||
| Err(e) => return Err(e.into()), | ||
| }; | ||
|
|
||
| // Get current flags to preserve flags we're not setting | ||
| let mut current_flags: libc::c_int = 0; | ||
| match unsafe { fs_ioc_getflags(fd.as_raw_fd(), &mut current_flags) } { | ||
| Ok(_) => {} | ||
| Err(nix::errno::Errno::ENOTTY | nix::errno::Errno::EOPNOTSUPP) => { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::Unsupported, | ||
| "filesystem does not support file flags", | ||
| )); | ||
| } | ||
| Err(e) => return Err(e.into()), | ||
| } | ||
|
|
||
| // Build new flags bitmap | ||
| let mut new_flags = current_flags; | ||
| for flag in flags { | ||
| match flag.as_str() { | ||
| "nodump" => new_flags |= FS_NODUMP_FL, | ||
| // Accept both libarchive names and aliases | ||
| "schg" | "simmutable" => new_flags |= FS_IMMUTABLE_FL, | ||
| "sappnd" | "sappend" => new_flags |= FS_APPEND_FL, | ||
| "noatime" => new_flags |= FS_NOATIME_FL, | ||
| "compr" | "compress" => new_flags |= FS_COMPR_FL, | ||
| "nocow" => new_flags |= FS_NOCOW_FL, | ||
| // Ignore flags not supported on Linux (e.g., uchg, uappnd, opaque, hidden, archived) | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| if new_flags != current_flags { | ||
| unsafe { fs_ioc_setflags(fd.as_raw_fd(), &new_flags) }?; | ||
| } | ||
|
|
||
| Ok(()) |
Contributor
There was a problem hiding this comment.
Linux set_flags can’t clear flags.
new_flags starts from current_flags and only ORs in new bits, and empty lists short‑circuit. This makes nouchg/dump and “clear all” semantics ineffective on Linux.
🛠️ Suggested fix: track set/clear bits
- if flags.is_empty() {
- return Ok(());
- }
+ // Allow clear-only operations; bail out only if nothing to change.
+ let mut set_bits: libc::c_int = 0;
+ let mut clear_bits: libc::c_int = 0;
nix::ioctl_read_bad!(fs_ioc_getflags, libc::FS_IOC_GETFLAGS, libc::c_int);
nix::ioctl_write_ptr_bad!(fs_ioc_setflags, libc::FS_IOC_SETFLAGS, libc::c_int);
@@
- // Build new flags bitmap
- let mut new_flags = current_flags;
- for flag in flags {
- match flag.as_str() {
- "nodump" => new_flags |= FS_NODUMP_FL,
- // Accept both libarchive names and aliases
- "schg" | "simmutable" => new_flags |= FS_IMMUTABLE_FL,
- "sappnd" | "sappend" => new_flags |= FS_APPEND_FL,
- "noatime" => new_flags |= FS_NOATIME_FL,
- "compr" | "compress" => new_flags |= FS_COMPR_FL,
- "nocow" => new_flags |= FS_NOCOW_FL,
- // Ignore flags not supported on Linux (e.g., uchg, uappnd, opaque, hidden, archived)
- _ => {}
- }
- }
+ for flag in flags {
+ let (is_clear, name) = flag
+ .strip_prefix("no")
+ .map(|n| (true, n))
+ .unwrap_or((false, flag.as_str()));
+ match name {
+ "nodump" => if is_clear { clear_bits |= FS_NODUMP_FL } else { set_bits |= FS_NODUMP_FL },
+ "schg" | "simmutable" => if is_clear { clear_bits |= FS_IMMUTABLE_FL } else { set_bits |= FS_IMMUTABLE_FL },
+ "sappnd" | "sappend" => if is_clear { clear_bits |= FS_APPEND_FL } else { set_bits |= FS_APPEND_FL },
+ "noatime" => if is_clear { clear_bits |= FS_NOATIME_FL } else { set_bits |= FS_NOATIME_FL },
+ "compr" | "compress" => if is_clear { clear_bits |= FS_COMPR_FL } else { set_bits |= FS_COMPR_FL },
+ "nocow" => if is_clear { clear_bits |= FS_NOCOW_FL } else { set_bits |= FS_NOCOW_FL },
+ "dump" => clear_bits |= FS_NODUMP_FL,
+ _ => {}
+ }
+ }
+ if set_bits == 0 && clear_bits == 0 {
+ return Ok(());
+ }
- if new_flags != current_flags {
+ let new_flags = (current_flags & !clear_bits) | set_bits;
+ if new_flags != current_flags {
unsafe { fs_ioc_setflags(fd.as_raw_fd(), &new_flags) }?;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/utils/os/unix/fs.rs` around lines 171 - 229, The set_flags function
currently only ORs bits into new_flags and returns early on empty flags, so it
can never clear bits; change the logic to build two masks (set_mask and
clear_mask) by parsing each entry in flags (treat entries prefixed with "no" as
clears, accept synonyms like "schg"/"simmutable", "sappnd"/"sappend",
"compr"/"compress", etc.), remove the early return on empty flags, then compute
new_flags = (current_flags | set_mask) & !clear_mask and call
fs_ioc_setflags(fd.as_raw_fd(), &new_flags) only if new_flags != current_flags;
reference set_flags, current_flags, new_flags, fs_ioc_setflags, and the flags
slice when implementing.
Comment on lines
+10
to
+75
| fn fflag_get_match_single_flag() { | ||
| setup(); | ||
| fs::create_dir_all("fflag_get_match").unwrap(); | ||
|
|
||
| fs::write("fflag_get_match/file1.txt", "content 1").unwrap(); | ||
| fs::write("fflag_get_match/file2.txt", "content 2").unwrap(); | ||
|
|
||
| cargo_bin_cmd!("pna") | ||
| .args([ | ||
| "--quiet", | ||
| "c", | ||
| "fflag_get_match/test.pna", | ||
| "--overwrite", | ||
| "fflag_get_match/file1.txt", | ||
| "fflag_get_match/file2.txt", | ||
| ]) | ||
| .assert() | ||
| .success(); | ||
|
|
||
| // Set uchg on file1 | ||
| cargo_bin_cmd!("pna") | ||
| .args([ | ||
| "--quiet", | ||
| "experimental", | ||
| "fflag", | ||
| "set", | ||
| "-f", | ||
| "fflag_get_match/test.pna", | ||
| "uchg", | ||
| "fflag_get_match/file1.txt", | ||
| ]) | ||
| .assert() | ||
| .success(); | ||
|
|
||
| // Set hidden on file2 | ||
| cargo_bin_cmd!("pna") | ||
| .args([ | ||
| "--quiet", | ||
| "experimental", | ||
| "fflag", | ||
| "set", | ||
| "-f", | ||
| "fflag_get_match/test.pna", | ||
| "hidden", | ||
| "fflag_get_match/file2.txt", | ||
| ]) | ||
| .assert() | ||
| .success(); | ||
|
|
||
| // Match only uchg | ||
| cargo_bin_cmd!("pna") | ||
| .args([ | ||
| "--quiet", | ||
| "experimental", | ||
| "fflag", | ||
| "get", | ||
| "-f", | ||
| "fflag_get_match/test.pna", | ||
| "--match", | ||
| "uchg", | ||
| "*", | ||
| ]) | ||
| .assert() | ||
| .success() | ||
| .stdout(predicates::str::contains("file1.txt")) | ||
| .stdout(predicates::str::contains("file2.txt").not()); |
Contributor
There was a problem hiding this comment.
Gate BSD-specific flag tests to supported OSes.
These tests rely on uchg (and elsewhere hidden), which are not supported on Linux/Windows. On those platforms fflag set is a no-op or maps to different names, so these assertions will fail. Add #[cfg] guards (or a runtime skip) so they only run where those flags are supported.
🛠️ Suggested guard (example)
-#[test]
+#[cfg(any(target_os = "macos", target_os = "freebsd"))]
+#[test]
fn fflag_get_match_single_flag() {
...
}
-#[test]
+#[cfg(any(target_os = "macos", target_os = "freebsd"))]
+#[test]
fn fflag_get_match_no_matches() {
...
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/tests/cli/fflag/get/option_match.rs` around lines 10 - 75, The test
fflag_get_match_single_flag uses BSD-only flags ("uchg"/"hidden") so it must be
gated to BSD-like OSes; add a compile-time cfg or a runtime skip so it only runs
on supported platforms. For example, annotate the test function
fflag_get_match_single_flag with a cfg attribute like #[cfg(any(target_os =
"macos", target_os = "freebsd", target_os = "openbsd", target_os = "netbsd"))]
or, alternatively, add an early runtime check using if !cfg!(any(target_os =
"macos", target_os = "freebsd", target_os = "openbsd", target_os = "netbsd")) {
eprintln!("skipping: fflags not supported"); return; } before invoking the
cargo_bin_cmd! calls so the create/set/get assertions for uchg/hidden only
execute where those flags are supported.
ChanTsune
force-pushed
the
cli/fflag/introduce-fflag-get-set
branch
from
665241b to
fba1796
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/command/experimental.rs`:
- Line 31: The Fflag command implementation has the wrong execute signature:
change FflagCommand::execute to accept &GlobalContext (matching the Command
trait and how ExperimentalCommands::Fflag calls it) instead of
&crate::cli::GlobalArgs; update the function signature in
cli/src/command/fflag.rs to use GlobalContext and mirror the pattern used by
AclCommand so the types align with the trait and other command implementations.
In `@cli/src/command/fflag.rs`:
- Around line 32-40: The implementations of the Command trait use the wrong
context type: update the fn execute signatures to match the trait by replacing
&crate::cli::GlobalArgs with &crate::cli::GlobalContext (or the exact
GlobalContext type used by the Command trait) for FflagCommand::execute and the
other two Command impls in this file (the Get/Set wrapper impls referenced in
the review); ensure the match arms still call cmd.execute(ctx) unchanged and
adjust any use/imports so GlobalContext is in scope.
In `@cli/tests/cli/fflag/get/option_dump.rs`:
- Around line 1-3: The test file is missing the predicates import used by
predicates::str::contains(...) in the assertions; add an import for that symbol
(e.g. add use predicates::str::contains;) at the top of the file so the calls to
predicates::str::contains in the tests compile and run. Ensure the import sits
alongside the existing top-level imports (crate::utils::setup,
assert_cmd::cargo::cargo_bin_cmd, std::fs) so predicates::str::contains is
available to the assertions.
In `@cli/tests/cli/fflag/set/basic.rs`:
- Around line 1-3: The test is missing the predicates import needed for calls to
predicates::str::contains used in this file; add the appropriate import (e.g.,
bring in predicates::str::contains or predicates::prelude::* ) at the top of
cli/tests/cli/fflag/set/basic.rs with the other use statements so the calls to
predicates::str::contains(...) compile.
In `@cli/tests/cli/fflag/set/mixed_operations.rs`:
- Around line 1-4: The predicates import is too narrow—tests call
predicates::str::contains(...) but the code only imports PredicateBooleanExt;
update the import statement that currently references
predicates::prelude::PredicateBooleanExt to import the entire prelude
(predicates::prelude::*) so all predicate helpers (including str::contains) are
available for the tests in mixed_operations.rs.
In `@cli/tests/cli/fflag/set/multiple_entries.rs`:
- Around line 1-4: The test imports only PredicateBooleanExt but also uses
predicates::str::contains in this file; update the import to include the full
predicates prelude (e.g., replace or augment the current import of
PredicateBooleanExt with predicates::prelude::*), or explicitly import
predicates::str::contains alongside PredicateBooleanExt so calls to
contains(...) resolve (affects the import line near the top that currently
references PredicateBooleanExt).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0d4574f-23e4-41ca-b9a7-85f5f09f102d
📒 Files selected for processing (21)
cli/src/command.rscli/src/command/experimental.rscli/src/command/fflag.rscli/tests/cli/fflag.rscli/tests/cli/fflag/compatibility.rscli/tests/cli/fflag/get.rscli/tests/cli/fflag/get/basic.rscli/tests/cli/fflag/get/missing_file.rscli/tests/cli/fflag/get/option_dump.rscli/tests/cli/fflag/get/option_long.rscli/tests/cli/fflag/get/option_match.rscli/tests/cli/fflag/get/option_name.rscli/tests/cli/fflag/set.rscli/tests/cli/fflag/set/basic.rscli/tests/cli/fflag/set/chflags_style.rscli/tests/cli/fflag/set/clear_flags.rscli/tests/cli/fflag/set/missing_file.rscli/tests/cli/fflag/set/mixed_operations.rscli/tests/cli/fflag/set/multiple_entries.rscli/tests/cli/fflag/set/option_restore.rscli/tests/cli/main.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- cli/tests/cli/fflag/set.rs
- cli/tests/cli/fflag/get/option_match.rs
- cli/tests/cli/fflag/set/missing_file.rs
- cli/tests/cli/fflag.rs
- cli/tests/cli/fflag/set/option_restore.rs
- cli/tests/cli/fflag/get.rs
- cli/tests/cli/fflag/get/basic.rs
- cli/tests/cli/fflag/get/option_long.rs
- cli/tests/cli/fflag/compatibility.rs
- cli/tests/cli/fflag/set/chflags_style.rs
- cli/src/command.rs
- cli/tests/cli/fflag/set/clear_flags.rs
Comment on lines
+1
to
+3
| use crate::utils::setup; | ||
| use assert_cmd::cargo::cargo_bin_cmd; | ||
| use std::fs; |
Contributor
There was a problem hiding this comment.
Missing predicates import.
The tests use predicates::str::contains(...) (lines 54-57, 94-95) but the crate is not imported.
🛠️ Proposed fix
use crate::utils::setup;
use assert_cmd::cargo::cargo_bin_cmd;
+use predicates::prelude::*;
use std::fs;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| use crate::utils::setup; | |
| use assert_cmd::cargo::cargo_bin_cmd; | |
| use std::fs; | |
| use crate::utils::setup; | |
| use assert_cmd::cargo::cargo_bin_cmd; | |
| use predicates::prelude::*; | |
| use std::fs; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/tests/cli/fflag/get/option_dump.rs` around lines 1 - 3, The test file is
missing the predicates import used by predicates::str::contains(...) in the
assertions; add an import for that symbol (e.g. add use
predicates::str::contains;) at the top of the file so the calls to
predicates::str::contains in the tests compile and run. Ensure the import sits
alongside the existing top-level imports (crate::utils::setup,
assert_cmd::cargo::cargo_bin_cmd, std::fs) so predicates::str::contains is
available to the assertions.
Comment on lines
+1
to
+3
| use crate::utils::setup; | ||
| use assert_cmd::cargo::cargo_bin_cmd; | ||
| use std::fs; |
Contributor
There was a problem hiding this comment.
Missing predicates import causes compilation failure.
The tests use predicates::str::contains(...) (lines 53, 104-106, 172-173) but the predicates crate is not imported. This will fail to compile.
🛠️ Proposed fix
use crate::utils::setup;
use assert_cmd::cargo::cargo_bin_cmd;
+use predicates::prelude::*;
use std::fs;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| use crate::utils::setup; | |
| use assert_cmd::cargo::cargo_bin_cmd; | |
| use std::fs; | |
| use crate::utils::setup; | |
| use assert_cmd::cargo::cargo_bin_cmd; | |
| use predicates::prelude::*; | |
| use std::fs; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/tests/cli/fflag/set/basic.rs` around lines 1 - 3, The test is missing the
predicates import needed for calls to predicates::str::contains used in this
file; add the appropriate import (e.g., bring in predicates::str::contains or
predicates::prelude::* ) at the top of cli/tests/cli/fflag/set/basic.rs with the
other use statements so the calls to predicates::str::contains(...) compile.
Comment on lines
+1
to
+4
| use crate::utils::setup; | ||
| use assert_cmd::cargo::cargo_bin_cmd; | ||
| use predicates::prelude::PredicateBooleanExt; | ||
| use std::fs; |
Contributor
There was a problem hiding this comment.
Same incomplete predicates import as in multiple_entries.rs.
The import only brings in PredicateBooleanExt but tests use predicates::str::contains(...). Use predicates::prelude::* instead.
🛠️ Proposed fix
use crate::utils::setup;
use assert_cmd::cargo::cargo_bin_cmd;
-use predicates::prelude::PredicateBooleanExt;
+use predicates::prelude::*;
use std::fs;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| use crate::utils::setup; | |
| use assert_cmd::cargo::cargo_bin_cmd; | |
| use predicates::prelude::PredicateBooleanExt; | |
| use std::fs; | |
| use crate::utils::setup; | |
| use assert_cmd::cargo::cargo_bin_cmd; | |
| use predicates::prelude::*; | |
| use std::fs; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/tests/cli/fflag/set/mixed_operations.rs` around lines 1 - 4, The
predicates import is too narrow—tests call predicates::str::contains(...) but
the code only imports PredicateBooleanExt; update the import statement that
currently references predicates::prelude::PredicateBooleanExt to import the
entire prelude (predicates::prelude::*) so all predicate helpers (including
str::contains) are available for the tests in mixed_operations.rs.
Comment on lines
+1
to
+4
| use crate::utils::setup; | ||
| use assert_cmd::cargo::cargo_bin_cmd; | ||
| use predicates::prelude::PredicateBooleanExt; | ||
| use std::fs; |
Contributor
There was a problem hiding this comment.
Incomplete predicates import.
Line 3 imports only PredicateBooleanExt (for .not()), but the tests also use predicates::str::contains(...) at lines 134-135 and 150-151, which requires the full path or importing predicates::prelude::*.
🛠️ Proposed fix
use crate::utils::setup;
use assert_cmd::cargo::cargo_bin_cmd;
-use predicates::prelude::PredicateBooleanExt;
+use predicates::prelude::*;
use std::fs;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| use crate::utils::setup; | |
| use assert_cmd::cargo::cargo_bin_cmd; | |
| use predicates::prelude::PredicateBooleanExt; | |
| use std::fs; | |
| use crate::utils::setup; | |
| use assert_cmd::cargo::cargo_bin_cmd; | |
| use predicates::prelude::*; | |
| use std::fs; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/tests/cli/fflag/set/multiple_entries.rs` around lines 1 - 4, The test
imports only PredicateBooleanExt but also uses predicates::str::contains in this
file; update the import to include the full predicates prelude (e.g., replace or
augment the current import of PredicateBooleanExt with predicates::prelude::*),
or explicitly import predicates::str::contains alongside PredicateBooleanExt so
calls to contains(...) resolve (affects the import line near the top that
currently references PredicateBooleanExt).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/src/command/fflag.rs (1)
198-210: Simplify redundant conditional check.Lines 198-203 contain a redundant outer condition. The
|| is_known_flag(flag)part of the condition on line 198 doesn't affect the outcome because:
- If
is_known_flag(base)is true → returns at line 201- If only
is_known_flag(flag)is true → falls through to line 205 anyway♻️ Proposed simplification
- if is_known_flag(base) || is_known_flag(flag) { - // If base is known, it's a clear operation - if is_known_flag(base) { - return FlagOp::Clear(normalize_flag_name(base)); - } + // If base is known, it's a clear operation + if is_known_flag(base) { + return FlagOp::Clear(normalize_flag_name(base)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/command/fflag.rs` around lines 198 - 210, The outer condition "if is_known_flag(base) || is_known_flag(flag)" is redundant; change it to only check "is_known_flag(base)" so the early-return clear path (return FlagOp::Clear(normalize_flag_name(base))) is executed only when base is known, and let the later "if is_known_flag(flag) { return FlagOp::Set(normalize_flag_name(flag)); }" handle the case where the full "no*" form is known; keep the existing fallback log::warn!("Unknown flag: {}", base) and return FlagOp::Clear(base.into()) behavior intact and reference the functions is_known_flag, normalize_flag_name and the variants FlagOp::Clear / FlagOp::Set to locate the code to edit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/command/fflag.rs`:
- Around line 483-506: parse_fflag_dump currently uses
result.insert(file.clone(), flags) which silently overwrites previous entries;
change it to detect duplicates and merge (or warn) instead: when encountering a
file path that's already in result (in function parse_fflag_dump), retrieve the
existing Vec<String> via result.get_mut(file) and extend it with the new flags,
deduplicating (e.g., using a temporary HashSet) before storing back; also emit a
warning (e.g., eprintln! or log::warn!) noting the duplicate path and that flags
were merged so callers can see malformed dumps.
---
Nitpick comments:
In `@cli/src/command/fflag.rs`:
- Around line 198-210: The outer condition "if is_known_flag(base) ||
is_known_flag(flag)" is redundant; change it to only check "is_known_flag(base)"
so the early-return clear path (return FlagOp::Clear(normalize_flag_name(base)))
is executed only when base is known, and let the later "if is_known_flag(flag) {
return FlagOp::Set(normalize_flag_name(flag)); }" handle the case where the full
"no*" form is known; keep the existing fallback log::warn!("Unknown flag: {}",
base) and return FlagOp::Clear(base.into()) behavior intact and reference the
functions is_known_flag, normalize_flag_name and the variants FlagOp::Clear /
FlagOp::Set to locate the code to edit.
Comment on lines
+483
to
+506
| fn parse_fflag_dump(reader: impl io::BufRead) -> io::Result<HashMap<String, Vec<String>>> { | ||
| let mut result = HashMap::new(); | ||
| let mut current_file: Option<String> = None; | ||
|
|
||
| for line in reader.lines() { | ||
| let line = line?; | ||
| if line.is_empty() { | ||
| continue; | ||
| } | ||
| if let Some(path) = line.strip_prefix("# file: ") { | ||
| current_file = Some(path.to_string()); | ||
| } else if let Some(file) = ¤t_file { | ||
| if let Some(flags_str) = line.strip_prefix("flags=") { | ||
| let flags: Vec<String> = flags_str | ||
| .split(',') | ||
| .map(|f| f.trim().to_string()) | ||
| .filter(|f| !f.is_empty()) | ||
| .collect(); | ||
| result.insert(file.clone(), flags); | ||
| } | ||
| } | ||
| } | ||
| Ok(result) | ||
| } |
Contributor
There was a problem hiding this comment.
Duplicate file entries in dump silently overwrites previous flags.
If a dump file contains the same file path multiple times, HashMap::insert will silently replace the previous entry's flags with the new ones (line 501). This could lead to unexpected behavior if users have malformed dump files.
Consider either:
- Warning when a duplicate is encountered
- Merging flags from duplicate entries
🛠️ Proposed fix to warn on duplicates
if let Some(flags_str) = line.strip_prefix("flags=") {
let flags: Vec<String> = flags_str
.split(',')
.map(|f| f.trim().to_string())
.filter(|f| !f.is_empty())
.collect();
- result.insert(file.clone(), flags);
+ if result.insert(file.clone(), flags).is_some() {
+ log::warn!("Duplicate entry in dump file: {}", file);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/command/fflag.rs` around lines 483 - 506, parse_fflag_dump currently
uses result.insert(file.clone(), flags) which silently overwrites previous
entries; change it to detect duplicates and merge (or warn) instead: when
encountering a file path that's already in result (in function
parse_fflag_dump), retrieve the existing Vec<String> via result.get_mut(file)
and extend it with the new flags, deduplicating (e.g., using a temporary
HashSet) before storing back; also emit a warning (e.g., eprintln! or
log::warn!) noting the duplicate path and that flags were merged so callers can
see malformed dumps.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
cli/src/command/fflag.rs (2)
397-404: Glob validation occurs after archive is persisted.If
ensure_all_matched()fails at line 402, the modified archive has already been written at line 399. The user would see an error but the archive would still be modified. Consider validating glob coverage before persisting to maintain atomicity.♻️ Suggested fix: validate before persisting
drop(source); + if let SetFflagStrategy::Apply { ref globs, .. } = set_strategy { + globs.ensure_all_matched()?; + } + temp_file.persist(output_path)?; - if let SetFflagStrategy::Apply { globs, .. } = set_strategy { - globs.ensure_all_matched()?; - } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/command/fflag.rs` around lines 397 - 404, The code persists the temporary archive via temp_file.persist(output_path) before validating that globs matched, so a failing globs.ensure_all_matched() would leave a modified archive; move the validation earlier: when set_strategy matches SetFflagStrategy::Apply, call globs.ensure_all_matched() (or otherwise validate glob coverage) before calling temp_file.persist(output_path) and before drop(source), so the persist only happens after validation succeeds; ensure errors from ensure_all_matched() abort without persisting.
193-211: Consider simplifying the nested conditional logic.The current flow is correct but the outer condition at line 198 checks
is_known_flag(flag)without using it inside the block—falling through to line 205 instead. This makes the control flow harder to follow.♻️ Suggested refactor for clarity
if let Some(base) = flag.strip_prefix("no") { // "nodump" is a flag itself, not "no" + "dump" if base == "dump" { return FlagOp::Set(normalize_flag_name(flag)); } - if is_known_flag(base) || is_known_flag(flag) { - // If base is known, it's a clear operation - if is_known_flag(base) { - return FlagOp::Clear(normalize_flag_name(base)); - } + // If base is a known flag, this is a clear operation (e.g., "nouchg" clears "uchg") + if is_known_flag(base) { + return FlagOp::Clear(normalize_flag_name(base)); } // Check if the full "no*" form is itself a known flag if is_known_flag(flag) { return FlagOp::Set(normalize_flag_name(flag)); } // Unknown but has "no" prefix - treat as clear log::warn!("Unknown flag: {}", base); return FlagOp::Clear(base.into()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/command/fflag.rs` around lines 193 - 211, The nested conditionals around handling "no" prefixes are confusing and contain a redundant is_known_flag(flag) check; simplify by: when flag.strip_prefix("no") yields base, first special-case base == "dump" and return FlagOp::Set(normalize_flag_name(flag)), then if is_known_flag(base) return FlagOp::Clear(normalize_flag_name(base)), else if is_known_flag(flag) return FlagOp::Set(normalize_flag_name(flag)), otherwise log::warn!("Unknown flag: {}", base) and return FlagOp::Clear(base.into()); this removes the unused outer is_known_flag(flag) branch and makes logic around is_known_flag, normalize_flag_name, FlagOp::Set/Clear and log::warn clearer.cli/tests/cli/fflag/compatibility.rs (1)
80-94: Test assertions could be more precise.The current checks verify that strings appear somewhere in the output but don't confirm the association between filenames and their flags. For example, if
file1.txthadnodumpinstead ofuchg, this test would still pass as long as both strings appear somewhere.Consider using line-based or section-based assertions if stronger verification is desired. This is a minor concern given the other tests provide coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/tests/cli/fflag/compatibility.rs` around lines 80 - 94, The assertions are too loose because they only check for presence of filenames and flags anywhere in output_str; update the test to locate the specific line (or section) for each filename by splitting output_str into lines and finding the line that contains "file1.txt", "file2.txt", and "file3.txt" (use the existing output_str variable), then assert that the corresponding lines contain the exact expected flags ("uchg" for file1.txt, "nodump" for file2.txt, and both "hidden" and "schg" for file3.txt); this ensures each filename is directly associated with its expected flags rather than global presence checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cli/src/command/fflag.rs`:
- Around line 397-404: The code persists the temporary archive via
temp_file.persist(output_path) before validating that globs matched, so a
failing globs.ensure_all_matched() would leave a modified archive; move the
validation earlier: when set_strategy matches SetFflagStrategy::Apply, call
globs.ensure_all_matched() (or otherwise validate glob coverage) before calling
temp_file.persist(output_path) and before drop(source), so the persist only
happens after validation succeeds; ensure errors from ensure_all_matched() abort
without persisting.
- Around line 193-211: The nested conditionals around handling "no" prefixes are
confusing and contain a redundant is_known_flag(flag) check; simplify by: when
flag.strip_prefix("no") yields base, first special-case base == "dump" and
return FlagOp::Set(normalize_flag_name(flag)), then if is_known_flag(base)
return FlagOp::Clear(normalize_flag_name(base)), else if is_known_flag(flag)
return FlagOp::Set(normalize_flag_name(flag)), otherwise log::warn!("Unknown
flag: {}", base) and return FlagOp::Clear(base.into()); this removes the unused
outer is_known_flag(flag) branch and makes logic around is_known_flag,
normalize_flag_name, FlagOp::Set/Clear and log::warn clearer.
In `@cli/tests/cli/fflag/compatibility.rs`:
- Around line 80-94: The assertions are too loose because they only check for
presence of filenames and flags anywhere in output_str; update the test to
locate the specific line (or section) for each filename by splitting output_str
into lines and finding the line that contains "file1.txt", "file2.txt", and
"file3.txt" (use the existing output_str variable), then assert that the
corresponding lines contain the exact expected flags ("uchg" for file1.txt,
"nodump" for file2.txt, and both "hidden" and "schg" for file3.txt); this
ensures each filename is directly associated with its expected flags rather than
global presence checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f84ba3e-5332-4be9-8792-67181635503d
📒 Files selected for processing (3)
cli/src/command/fflag.rscli/tests/cli/fflag/compatibility.rscli/tests/cli/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/tests/cli/main.rs
- Fix rustfmt formatting for FilterOption::new() call - Gate fflag test module with #[cfg(not(target_family = "wasm"))] to avoid assert_cmd/predicates compilation on wasm targets - Fix race condition in fflag_compatibility_add_flags by extracting the test archive to an isolated subdirectory instead of shared tmpdir
All compatibility tests were extracting zstd_keep_fflags.pna to the shared temp directory. When tests run in parallel and pna uses mmap (memmap feature), one test's extract_in (fs::write = truncate + write) can invalidate another test's mmap, causing SIGBUS on OpenBSD.
- Make parse_flag_op return Result and reject unknown flag names, preventing silent archive corruption from typos or misinterpreted file path arguments - Check for empty flag operations before rewriting archive, preventing no-op archive rewrites - Make parse_fflag_dump return errors for unexpected lines and data before file headers, following parse_acl_dump convention
ChanTsune
force-pushed
the
cli/fflag/introduce-fflag-get-set
branch
from
b3fcd3f to
104c617
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
getandsetsubcommands with dump, long, name/regex filtering, chflags-style ops, glob targets, and restore-from-dump support.Tests