✨ Add --format option to diff subcommand - #3243
Conversation
📝 WalkthroughWalkthroughThe diff command adds a ChangesDiff output formats
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DiffCommand
participant compare_entry
participant report
participant stdout
User->>DiffCommand: run diff with --format
DiffCommand->>compare_entry: compare archive entries with selected Format
compare_entry->>report: submit DiffKind and path
report->>stdout: emit plain text or JSONL record
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. 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 introduces a --format option to the diff command, allowing users to output differences in either the default plain text format or a structured jsonl format. To support this, serde_json was added as a dependency, and the diff reporting logic was refactored to use a unified report function. Comprehensive integration tests were also added to verify the new formatting options. The reviewer feedback suggests using scopeguard::guard in the newly added integration tests to ensure that temporary test directories are reliably cleaned up even if a test fails or panics.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
30ccda2 to
0581d88
Compare
github-actions
Bot
added
dependencies
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cli/src/command/diff.rs (1)
34-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFormat arg design: LGTM, minor DRY nit on the default.
The
--formatplumbing andFormatenum are correct (ValueEnum+rename_all = "lower"correctly mapsJsonL→"jsonl"). One small nit:Formatalready declares its default via#[default] Plain(line 45), butdiff_archive(line 67) re-specifiesFormat::Plaininunwrap_or. Usingunwrap_or_default()would keep the single source of truth and avoid future drift if the default variant ever changes.♻️ Suggested tweak
- format: args.format.unwrap_or(Format::Plain), + format: args.format.unwrap_or_default(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/command/diff.rs` around lines 34 - 48, Update the format selection in diff_archive to use Format::default() via unwrap_or_default() instead of explicitly specifying Format::Plain, while preserving the existing behavior and keeping the enum’s #[default] declaration as the single source of truth.cli/tests/cli/diff/option_format.rs (1)
1-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage for content/missing/hardlink/no-diff/plain; consider adding the Unix-only metadata kinds.
Tests validate
kindvaluescontent,missing, andhardlink, plus the empty-output and plain-format paths. Per the PR objectives,mode,mtime,uid, andgidare also validkindvalues (Unix-only) but aren't exercised here, so thewire_name()mapping for those variants isn't covered by any jsonl test.Consider adding a
#[cfg(unix)]test that changes a file's mode/mtime/uid/gid relative to the archived entry and asserts the correspondingkindrecords appear.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/tests/cli/diff/option_format.rs` around lines 1 - 217, Add a Unix-only JSONL integration test alongside the existing diff format tests that archives a file, changes its metadata, runs experimental diff with --format jsonl, and parses stdout records. Assert that the output includes records with kind values mode, mtime, uid, and gid, covering the wire_name() mappings for these metadata variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/src/command/diff.rs`:
- Around line 34-48: Update the format selection in diff_archive to use
Format::default() via unwrap_or_default() instead of explicitly specifying
Format::Plain, while preserving the existing behavior and keeping the enum’s
#[default] declaration as the single source of truth.
In `@cli/tests/cli/diff/option_format.rs`:
- Around line 1-217: Add a Unix-only JSONL integration test alongside the
existing diff format tests that archives a file, changes its metadata, runs
experimental diff with --format jsonl, and parses stdout records. Assert that
the output includes records with kind values mode, mtime, uid, and gid, covering
the wire_name() mappings for these metadata variants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 08e180b4-63c5-48b6-8c18-0691fba656a5
📒 Files selected for processing (4)
cli/Cargo.tomlcli/src/command/diff.rscli/tests/cli/diff.rscli/tests/cli/diff/option_format.rs
0581d88 to
f5164d2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/tests/cli/diff/option_format.rs (1)
9-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extending JSONL coverage to other
kindvalues.Current tests cover
content,missing, andhardlink(plus multi-diff/no-diff/plain-equivalence), but notmode/mtime/uid/gid/type/symlink, which share the samereport()/DiffRecordpath but aren't directly exercised in JSONL form.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/tests/cli/diff/option_format.rs` around lines 9 - 39, Extend the JSONL diff tests around diff_with_format_jsonl_and_content_difference to cover the remaining DiffRecord kind values: mode, mtime, uid, gid, type, and symlink. Create fixtures that trigger each difference, invoke the same experimental diff --format jsonl path, and assert the JSONL output reports the expected path and kind while preserving existing content, missing, hardlink, multi-diff, no-diff, and plain-format coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/tests/cli/diff/option_format.rs`:
- Around line 9-39: Extend the JSONL diff tests around
diff_with_format_jsonl_and_content_difference to cover the remaining DiffRecord
kind values: mode, mtime, uid, gid, type, and symlink. Create fixtures that
trigger each difference, invoke the same experimental diff --format jsonl path,
and assert the JSONL output reports the expected path and kind while preserving
existing content, missing, hardlink, multi-diff, no-diff, and plain-format
coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eb78def4-cf40-4546-9106-6790828b495e
📒 Files selected for processing (3)
cli/src/command/diff.rscli/tests/cli/diff.rscli/tests/cli/diff/option_format.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/tests/cli/diff.rs
f5164d2 to
55a20fc
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/tests/cli/diff/option_format.rs (1)
9-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared setup boilerplate for the JSONL test suite.
Each of the 9 tests repeats the same pattern: remove/create a unique dir, write fixture files,
createan archive with--overwrite, mutate the filesystem, then rundiff --format jsonl|plainand assert. A small helper (e.g.fn setup_dir(name: &str) -> Stringplus afn create_archive(archive: &str, files: &[&str])wrapper) would remove most of this duplication and make future test additions cheaper.♻️ Sketch of a shared helper
fn setup_dir(name: &str) -> String { let _ = fs::remove_dir_all(name); fs::create_dir_all(name).unwrap(); name.to_string() } fn create_archive(archive: &str, extra_args: &[&str], files: &[&str]) { cargo_bin_cmd!("pna") .args(["create", "-f", archive, "--overwrite"]) .args(extra_args) .args(files) .assert() .success(); }Also applies to: 45-75, 83-116, 122-167, 173-201, 207-246, 253-286, 294-359, 365-404
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/tests/cli/diff/option_format.rs` around lines 9 - 39, Extract the repeated directory and archive creation setup from the JSONL and plain diff tests into shared helpers near the test module, such as setup_dir and create_archive. Update all nine affected tests, including diff_with_format_jsonl_and_content_difference, to use these helpers while preserving each test’s unique fixture files, archive arguments, filesystem mutations, and diff assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/tests/cli/diff/option_format.rs`:
- Around line 9-39: Extract the repeated directory and archive creation setup
from the JSONL and plain diff tests into shared helpers near the test module,
such as setup_dir and create_archive. Update all nine affected tests, including
diff_with_format_jsonl_and_content_difference, to use these helpers while
preserving each test’s unique fixture files, archive arguments, filesystem
mutations, and diff assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b5ee94f9-ce51-4774-911a-44eec18424e9
📒 Files selected for processing (3)
cli/src/command/diff.rscli/tests/cli/diff.rscli/tests/cli/diff/option_format.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- cli/tests/cli/diff.rs
- cli/src/command/diff.rs
jsonl emits one JSON Lines record per difference: {"path", "kind", "target"?}.
55a20fc to
824b91d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/tests/cli/diff/option_format.rs (1)
10-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer structural JSON assertions over exact stdout string matches.
Every JSONL test here asserts the full stdout via a literal
format!-built JSON string (e.g. Line 40, 77, 119, 197-201, 242, 308-316). The PR objectives state the schema is meant to evolve by adding fields only, so any future additive field will require touching every one of these exact-match assertions simultaneously, even for kinds unrelated to the new field. Parsing each line into a value and asserting the relevant subset of fields would decouple these tests from unrelated schema growth.♻️ Example refactor for the content-difference test
- .stdout(format!(r#"{{"path":"{file_path}","kind":"content"}}"#) + "\n"); + .success(); // replaced above with .code(1) as before + + let stdout = String::from_utf8(output.get_output().stdout.clone()).unwrap(); + let lines: Vec<serde_json::Value> = stdout + .lines() + .map(|l| serde_json::from_str(l).unwrap()) + .collect(); + assert_eq!(lines.len(), 1); + assert_eq!(lines[0]["path"], file_path); + assert_eq!(lines[0]["kind"], "content"); + assert!(lines[0].get("target").is_none());Also applies to: 47-78, 86-120, 162-202, 209-243, 251-317
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/tests/cli/diff/option_format.rs` around lines 10 - 41, Update the JSONL assertions in the tests diff_with_format_jsonl_and_content_difference and the other affected diff-format tests to parse each stdout line as JSON and assert only the fields relevant to that scenario, rather than matching exact format! strings. Preserve checks for the expected path, kind, and any scenario-specific fields while allowing additional schema fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/tests/cli/diff/option_format.rs`:
- Around line 10-41: Update the JSONL assertions in the tests
diff_with_format_jsonl_and_content_difference and the other affected diff-format
tests to parse each stdout line as JSON and assert only the fields relevant to
that scenario, rather than matching exact format! strings. Preserve checks for
the expected path, kind, and any scenario-specific fields while allowing
additional schema fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5fe9d208-1494-4c25-a60a-65dceb3dda77
📒 Files selected for processing (3)
cli/src/command/diff.rscli/tests/cli/diff.rscli/tests/cli/diff/option_format.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- cli/tests/cli/diff.rs
- cli/src/command/diff.rs
1 participant
Summary
pna experimental diffgains--format <plain|jsonl>.plain(default) keeps the current tar-style text unchanged;jsonlemits one JSON object per difference per line:{"path", "kind", "target"?}, wherekindis one ofmissing,size,content,mode,mtime,uid,gid,type,symlink,hardlink, andtargetappears only forhardlink. Part of #3228.Notes
pna list --format(jsonl).kindnames are a fixed wire vocabulary decoupled from internal enum names; the schema evolves by adding fields only.mode/mtime/uid/gidrecords occur only on Unix (metadata comparison is Unix-only).Summary by CodeRabbit
--formatoption for experimental diff output:plain(default) orjsonl.jsonlis gated behind the--unstableflag.jsonlandplain, including content changes, missing entries, hardlink breaks, symlink and Unix metadata differences, ordering, and no-difference output.