Skip to content

Fix critical bug in CLI update command and stabilize it #3007

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ChanTsune wants to merge 2 commits into from
Closed

Fix critical bug in CLI update command and stabilize it #3007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc923a8
👽 cli: fix critical bug in update command and stabilize it
google-labs-jules[bot] Apr 30, 2026
f7f0b48
👽 cli: fix path normalization bug in update command
google-labs-jules[bot] Apr 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions cli/src/command/update.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use crate::{
};
use clap::{ArgGroup, Parser, ValueHint};
use indexmap::IndexMap;
use pna::{Archive, EntryName, Metadata, prelude::*};
use pna::{Archive, Metadata, prelude::*};
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

use std::{env, fs, io, path::PathBuf};

#[derive(Parser, Clone, Debug)]
Expand Down Expand Up @@ -583,7 +583,12 @@ where
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<_, _>>();
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of IndexMap here, combined with shift_remove at line 600, results in $O(M)$ complexity for each archive entry lookup (where $M$ is the number of target files), leading to $O(N \times M)$ total complexity for the update operation. Since 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 $O(1)$ lookups or using swap_remove at line 600 to significantly improve performance for large updates.

Comment on lines 583 to 592
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
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.


rayon::scope_fifo(|s| -> anyhow::Result<()> {
Expand Down
2 changes: 1 addition & 1 deletion cli/tests/cli/update/no_timestamp_archive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ fn update_no_timestamp_archive_always_updates() {
}

/// Precondition: An archive created with `--no-keep-timestamp` (entries have no mtime).
/// Action: Delete a source file and run `pna experimental update --sync`.
/// Action: Delete a source file and run `pna update --sync`.
/// Expectation: Deleted file is removed from archive; remaining entries preserved.
#[test]
fn update_no_timestamp_archive_with_sync() {
Expand Down
2 changes: 1 addition & 1 deletion cli/tests/cli/update/option_archive_missing_mtime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ fn update_default_mtime_missing_still_updates() {
}

/// Precondition: Archive contains an entry without mtime.
/// Action: Run `pna experimental update --archive-missing-mtime=exclude` on its own
/// Action: Run `pna update --archive-missing-mtime=exclude` on its own
/// (no time-filter flag). Update's Path B staleness judgment fires for every entry
/// regardless of time-filter flags, so the archive-missing policy takes effect.
/// Expectation: The entry is kept (pass-through) because `exclude` treats mtime-missing
Expand Down
Loading