Fix critical bug in CLI update command and stabilize it#3007
Conversation
- Register `update` subcommand as a top-level command. - Fix path matching in `update` to respect path transformations like `--strip-components`. - Deprecate `experimental update` with a warning. - Update integration tests to use the stabilized `update` command. - Clean up test code formatting. Co-authored-by: ChanTsune <41658782+ChanTsune@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![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 stabilizes the update command, moving it from the experimental subcommand to a top-level command with a visible alias "u". The experimental update command now issues a deprecation warning. Functional changes include integrating the pathname_editor into the update process to allow entry name transformations. Feedback focuses on maintaining consistency in the experimental command descriptions, addressing an unused archive_missing_ctime flag, and optimizing the performance of the update operation by reconsidering the use of IndexMap and shift_remove to avoid
| cmd.execute(ctx) | ||
| } | ||
| ExperimentalCommands::Update(cmd) => cmd.execute(ctx), | ||
| ExperimentalCommands::Update(cmd) => { |
There was a problem hiding this comment.
While stabilizing the update command, the about text for the Update variant in the ExperimentalCommands enum (line 80) should be updated to reflect its stabilized status and future removal, maintaining consistency with the Delete and Sort variants.
| use clap::{ArgGroup, Parser, ValueHint}; | ||
| use indexmap::IndexMap; | ||
| use pna::{Archive, EntryName, Metadata, prelude::*}; | ||
| use pna::{Archive, Metadata, prelude::*}; |
There was a problem hiding this comment.
While updating the imports and stabilizing the update command, note that the archive_missing_ctime field (defined at line 280) is currently unused in the update_archive and run_update_archive functions. Since the staleness check in is_newer_than_archive only utilizes modification time (mtime), this flag should either be integrated into the logic or removed to avoid providing a non-functional CLI option.
| .edit_entry_name(&item.path) | ||
| .map(|name| (name, (idx, item))) | ||
| }) | ||
| .collect::<IndexMap<_, _>>(); |
There was a problem hiding this comment.
The use of IndexMap here, combined with shift_remove at line 600, results in ReorderByIndex (line 647) ensures the final archive entries are correctly ordered, the insertion order within this map is not critical for the output. Consider using a HashMap for swap_remove at line 600 to significantly improve performance for large updates.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/update.rs`:
- Around line 583-592: The current collection into target_files_mapping uses
collect::<IndexMap<_, _>>() which will silently overwrite earlier entries when
create_options.pathname_editor.edit_entry_name(&item.path) returns the same name
for multiple target_items; change the logic that builds target_files_mapping
(the block using target_items.into_iter().enumerate().filter_map(...) and
collected into target_files_mapping) to explicitly detect duplicates: iterate
and attempt to insert each (name, (idx, item)) into the map, check
target_files_mapping.contains_key(&name) (or use entry API) and on collision
return an Err or accumulate a clear diagnostic mentioning the duplicate
transformed name and the conflicting source paths (using edit_entry_name and the
original item.path) so callers can fail fast instead of silently overwriting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32e129de-5bb2-4b93-8d26-2370188fbf74
📒 Files selected for processing (25)
cli/src/cli.rscli/src/command.rscli/src/command/experimental.rscli/src/command/update.rscli/tests/cli/update/encryption.rscli/tests/cli/update/entry_order.rscli/tests/cli/update/error.rscli/tests/cli/update/no_timestamp_archive.rscli/tests/cli/update/option_archive_missing_mtime.rscli/tests/cli/update/option_atime.rscli/tests/cli/update/option_ctime.rscli/tests/cli/update/option_exclude.rscli/tests/cli/update/option_exclude_vcs.rscli/tests/cli/update/option_files_from_stdin.rscli/tests/cli/update/option_mtime.rscli/tests/cli/update/option_newer_ctime.rscli/tests/cli/update/option_newer_ctime_than.rscli/tests/cli/update/option_newer_mtime.rscli/tests/cli/update/option_newer_mtime_than.rscli/tests/cli/update/option_older_ctime.rscli/tests/cli/update/option_older_ctime_than.rscli/tests/cli/update/option_older_mtime.rscli/tests/cli/update/option_older_mtime_than.rscli/tests/cli/update/option_recursive.rscli/tests/cli/update/option_sync.rs
| let mut target_files_mapping = target_items | ||
| .into_iter() | ||
| .enumerate() | ||
| .map(|(idx, item)| (EntryName::from_lossy(&item.path), (idx, item))) | ||
| .filter_map(|(idx, item)| { | ||
| create_options | ||
| .pathname_editor | ||
| .edit_entry_name(&item.path) | ||
| .map(|name| (name, (idx, item))) | ||
| }) | ||
| .collect::<IndexMap<_, _>>(); |
There was a problem hiding this comment.
Detect transformed-name collisions instead of silently overwriting.
This direct collect::<IndexMap<_, _>>() can silently replace earlier items when multiple source paths normalize to the same entry name. That can skip intended updates/additions with no warning.
Suggested fix
- let mut target_files_mapping = target_items
- .into_iter()
- .enumerate()
- .filter_map(|(idx, item)| {
- create_options
- .pathname_editor
- .edit_entry_name(&item.path)
- .map(|name| (name, (idx, item)))
- })
- .collect::<IndexMap<_, _>>();
+ let mut target_files_mapping = IndexMap::new();
+ for (idx, item) in target_items.into_iter().enumerate() {
+ let Some(name) = create_options.pathname_editor.edit_entry_name(&item.path) else {
+ continue;
+ };
+ if let Some((_, prev_item)) = target_files_mapping.get(&name) {
+ anyhow::bail!(
+ "multiple input paths map to the same archive entry '{}': '{}' and '{}'",
+ name,
+ prev_item.path.display(),
+ item.path.display()
+ );
+ }
+ target_files_mapping.insert(name, (idx, item));
+ }📝 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.
| let mut target_files_mapping = target_items | |
| .into_iter() | |
| .enumerate() | |
| .map(|(idx, item)| (EntryName::from_lossy(&item.path), (idx, item))) | |
| .filter_map(|(idx, item)| { | |
| create_options | |
| .pathname_editor | |
| .edit_entry_name(&item.path) | |
| .map(|name| (name, (idx, item))) | |
| }) | |
| .collect::<IndexMap<_, _>>(); | |
| let mut target_files_mapping = IndexMap::new(); | |
| for (idx, item) in target_items.into_iter().enumerate() { | |
| let Some(name) = create_options.pathname_editor.edit_entry_name(&item.path) else { | |
| continue; | |
| }; | |
| if let Some((_, prev_item)) = target_files_mapping.get(&name) { | |
| anyhow::bail!( | |
| "multiple input paths map to the same archive entry '{}': '{}' and '{}'", | |
| name, | |
| prev_item.path.display(), | |
| item.path.display() | |
| ); | |
| } | |
| target_files_mapping.insert(name, (idx, item)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/command/update.rs` around lines 583 - 592, The current collection
into target_files_mapping uses collect::<IndexMap<_, _>>() which will silently
overwrite earlier entries when
create_options.pathname_editor.edit_entry_name(&item.path) returns the same name
for multiple target_items; change the logic that builds target_files_mapping
(the block using target_items.into_iter().enumerate().filter_map(...) and
collected into target_files_mapping) to explicitly detect duplicates: iterate
and attempt to insert each (name, (idx, item)) into the map, check
target_files_mapping.contains_key(&name) (or use entry API) and on collision
return an Err or accumulate a clear diagnostic mentioning the duplicate
transformed name and the conflicting source paths (using edit_entry_name and the
original item.path) so callers can fail fast instead of silently overwriting.
The `update` command failed to correctly match filesystem paths against existing archive entries when path transformations (like `--strip-components`) were used. This led to duplicate entries instead of updates. This fix ensures that filesystem paths are normalized using the same logic as archive entry names before being used as keys in the matching logic. The `update` command remains an experimental feature. Co-authored-by: ChanTsune <41658782+ChanTsune@users.noreply.github.com>
ChanTsune
deleted the
fix-cli-update-bug-and-stabilize-14259518636105600921
branch
1 participant
This PR fixes a critical bug in the PNA CLI where the
updatecommand was not accessible from the top-level interface and, when invoked viaexperimental update, failed to correctly match filesystem paths against archive entries when path transformations (like--strip-components) were used. This led to duplicate entries instead of updates.Changes:
Updateto the mainCommandsenum.run_update_archiveto usepathname_editorfor path normalization during entry matching.updatecommand and added a deprecation warning toexperimental update.updatecommand.PR created automatically by Jules for task 14259518636105600921 started by @ChanTsune
Summary by CodeRabbit
Release Notes
Bug Fixes
Documentation