af1ab022
Implement Gerrit-style patchsets (spec 015): revision tracking, interdiff, patch log
a73x 2026-03-21 19:02
Add revision tracking to patches: PatchCreate now records commit/tree OIDs as revision 1, auto-detect inserts PatchRevision events when branch tip changes, and explicit `patch revise` creates snapshots with dedup. Reviews and inline comments are anchored to revision numbers. New capabilities: - `patch show --revision N` filters comments/reviews to a specific revision - `patch diff --revision N` shows historical diff for a revision - `patch diff --between N M` computes interdiff between revisions - `patch log` shows revision history with file-change summaries - `patch review --revision N` targets a specific revision - Merge policy: configurable require_approval_on_latest via refs/collab/config - TUI shows revision count (rN) in patch list Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
diff --git a/CLAUDE.md b/CLAUDE.md index aa10e98..b31fa5c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,6 +8,7 @@ Auto-generated from all feature plans. Last updated: 2026-03-21 - Rust 2021 edition + ratatui 0.30, crossterm 0.29, git2 0.19 (004-dashboard-filtering) - N/A (ephemeral filter state, no persistence) (004-dashboard-filtering) - Rust 2021 edition + git2 0.19, clap 4 (derive), serde/serde_json 1, chrono 0.4, ed25519-dalek 2 (012-patch-branch-refactor) - Git object database (collab DAG refs under `.git/refs/collab/`) (015-gerrit-style-patchsets) - Rust 2021 edition + git2 0.19, clap 4, serde/serde_json 1, chrono 0.4, thiserror 2. New: `ed25519-dalek`, `rand`, `base64` (001-gpg-event-signing) @@ -27,9 +28,9 @@ cargo test [ONLY COMMANDS FOR ACTIVE TECHNOLOGIES][ONLY COMMANDS FOR ACTIVE TECH Rust 2021 edition: Follow standard conventions ## Recent Changes - 015-gerrit-style-patchsets: Added Rust 2021 edition + git2 0.19, clap 4 (derive), serde/serde_json 1, chrono 0.4, ed25519-dalek 2 - 012-patch-branch-refactor: Added Rust 2021 edition + git2 0.19, clap 4 (derive), serde/serde_json 1, chrono 0.4, ed25519-dalek 2 - 004-dashboard-filtering: Added Rust 2021 edition + ratatui 0.30, crossterm 0.29, git2 0.19 - 003-key-trust-allowlist: Added Rust 2021 edition + git2 0.19, clap 4 (derive), ed25519-dalek 2, base64 0.22, serde/serde_json 1, dirs 5, thiserror 2 <!-- MANUAL ADDITIONS START --> diff --git a/benches/core_ops.rs b/benches/core_ops.rs index ef69a7f..180e8f8 100644 --- a/benches/core_ops.rs +++ b/benches/core_ops.rs @@ -76,6 +76,8 @@ fn setup_patches(n: usize) -> (Repository, TempDir) { base_ref: "main".to_string(), branch: format!("feature-{}", i), fixes: None, commit: "dummy_commit".to_string(), tree: "dummy_tree".to_string(), }, clock: 0, }; @@ -215,6 +217,8 @@ fn bench_patch_from_ref(c: &mut Criterion) { base_ref: "main".to_string(), branch: format!("feature-bench-{}", count), fixes: None, commit: "dummy_commit".to_string(), tree: "dummy_tree".to_string(), }, clock: 0, }; diff --git a/specs/015-gerrit-style-patchsets/tasks.md b/specs/015-gerrit-style-patchsets/tasks.md index ea535b7..7c119b8 100644 --- a/specs/015-gerrit-style-patchsets/tasks.md +++ b/specs/015-gerrit-style-patchsets/tasks.md @@ -20,12 +20,12 @@ **⚠️ CRITICAL**: No user story work can begin until this phase is complete. <!-- parallel-group: 1 (max 3 concurrent) --> - [ ] T001 [P] Add `PatchRevision` variant to `Action` enum in `src/event.rs`: fields `commit: String`, `tree: String`, `body: Option<String>`, serialized as `"patch.revision"`. Add required `commit: String` and `tree: String` fields to `Action::PatchCreate`. Add required `revision: u32` field to `Action::PatchInlineComment` and `Action::PatchReview`. Remove the `PatchRevise` variant entirely. - [ ] T002 [P] Add `Revision` struct to `src/state.rs`: `{ number: u32, commit: String, tree: String, body: Option<String>, timestamp: String }` with Serialize/Deserialize. Add `revisions: Vec<Revision>` field to `PatchState`. - [ ] T003 [P] Add `revision: u32` field to `InlineComment` struct and `Review` struct in `src/state.rs`. - [x] T001 [P] Add `PatchRevision` variant to `Action` enum in `src/event.rs`: fields `commit: String`, `tree: String`, `body: Option<String>`, serialized as `"patch.revision"`. Add required `commit: String` and `tree: String` fields to `Action::PatchCreate`. Add required `revision: u32` field to `Action::PatchInlineComment` and `Action::PatchReview`. Remove the `PatchRevise` variant entirely. - [x] T002 [P] Add `Revision` struct to `src/state.rs`: `{ number: u32, commit: String, tree: String, body: Option<String>, timestamp: String }` with Serialize/Deserialize. Add `revisions: Vec<Revision>` field to `PatchState`. - [x] T003 [P] Add `revision: u32` field to `InlineComment` struct and `Review` struct in `src/state.rs`. <!-- sequential --> - [ ] T004 Update `PatchState::from_ref_uncached()` materialization in `src/state.rs`: handle `Action::PatchCreate` to extract `commit`/`tree` into revision 1. Handle `Action::PatchRevision` to append revisions with dedup by commit OID (skip if commit already seen). Derive revision numbers from insertion order (1-indexed). Handle `Action::PatchInlineComment` and `Action::PatchReview` to store the `revision` field on `InlineComment`/`Review` structs. Remove the `Action::PatchRevise` match arm. - [x] T004 Update `PatchState::from_ref_uncached()` materialization in `src/state.rs`: handle `Action::PatchCreate` to extract `commit`/`tree` into revision 1. Handle `Action::PatchRevision` to append revisions with dedup by commit OID (skip if commit already seen). Derive revision numbers from insertion order (1-indexed). Handle `Action::PatchInlineComment` and `Action::PatchReview` to store the `revision` field on `InlineComment`/`Review` structs. Remove the `Action::PatchRevise` match arm. **Checkpoint**: Event schema and state materialization support revisions. Existing tests updated for new required fields. @@ -38,13 +38,13 @@ **Independent Test**: Create a patch, push a commit, run `patch comment`, verify a PatchRevision event was auto-inserted before the comment. <!-- sequential --> - [ ] T005 [US1] Implement `auto_detect_revision()` helper in `src/patch.rs`: takes `repo`, `ref_name`, `&PatchState`, and `&SigningKey`. Resolves current branch tip via `resolve_head()`. Compares to last revision's commit OID in `state.revisions`. If different, appends a `PatchRevision` event (commit OID + tree OID from `repo.find_commit(tip)?.tree()?.id()`). Returns the current revision number (u32). If branch is deleted or resolve fails, returns last known revision number without inserting. Note: auto-detect runs for ALL write ops including thread comments (records revision boundary), even though thread comments don't get a revision field. - [ ] T006 [US1] Modify `comment()` in `src/patch.rs` to call `auto_detect_revision()` before appending the comment event. Set the `revision` field on `PatchInlineComment` to the returned revision number. Thread comments (`PatchComment`) do not get a revision field — no change needed there. - [ ] T007 [US1] Modify `review()` in `src/patch.rs` to call `auto_detect_revision()` before appending the review event. Set the `revision` field on `PatchReview` to the returned revision number. - [ ] T008 [US1] Modify `merge()` in `src/patch.rs` to call `auto_detect_revision()` before appending the merge event (no revision field on PatchMerge, but the revision should be recorded if branch changed). - [ ] T009 [US1] Modify `create()` in `src/patch.rs` to populate `commit` and `tree` fields on `PatchCreate` event: resolve the branch tip commit OID and its tree OID. - [ ] T010 [US1] Rewrite `revise()` in `src/patch.rs`: create a `PatchRevision` event with commit OID, tree OID, and optional body. Compare branch tip to last recorded revision — reject with error "no changes since revision N" if unchanged. - [ ] T010a [US1] Wire Phase 2 dispatch in `src/lib.rs`: update `PatchCmd::Revise` dispatch to call the rewritten `revise()`. Ensure `PatchCmd::Comment`, `PatchCmd::Review`, and `PatchCmd::Merge` dispatch passes through to the modified functions that now call `auto_detect_revision()`. - [x] T005 [US1] Implement `auto_detect_revision()` helper in `src/patch.rs`: takes `repo`, `ref_name`, `&PatchState`, and `&SigningKey`. Resolves current branch tip via `resolve_head()`. Compares to last revision's commit OID in `state.revisions`. If different, appends a `PatchRevision` event (commit OID + tree OID from `repo.find_commit(tip)?.tree()?.id()`). Returns the current revision number (u32). If branch is deleted or resolve fails, returns last known revision number without inserting. Note: auto-detect runs for ALL write ops including thread comments (records revision boundary), even though thread comments don't get a revision field. - [x] T006 [US1] Modify `comment()` in `src/patch.rs` to call `auto_detect_revision()` before appending the comment event. Set the `revision` field on `PatchInlineComment` to the returned revision number. Thread comments (`PatchComment`) do not get a revision field — no change needed there. - [x] T007 [US1] Modify `review()` in `src/patch.rs` to call `auto_detect_revision()` before appending the review event. Set the `revision` field on `PatchReview` to the returned revision number. - [x] T008 [US1] Modify `merge()` in `src/patch.rs` to call `auto_detect_revision()` before appending the merge event (no revision field on PatchMerge, but the revision should be recorded if branch changed). - [x] T009 [US1] Modify `create()` in `src/patch.rs` to populate `commit` and `tree` fields on `PatchCreate` event: resolve the branch tip commit OID and its tree OID. - [x] T010 [US1] Rewrite `revise()` in `src/patch.rs`: create a `PatchRevision` event with commit OID, tree OID, and optional body. Compare branch tip to last recorded revision — reject with error "no changes since revision N" if unchanged. - [x] T010a [US1] Wire Phase 2 dispatch in `src/lib.rs`: update `PatchCmd::Revise` dispatch to call the rewritten `revise()`. Ensure `PatchCmd::Comment`, `PatchCmd::Review`, and `PatchCmd::Merge` dispatch passes through to the modified functions that now call `auto_detect_revision()`. **Checkpoint**: `patch create` records revision 1. `patch comment`, `patch review`, `patch merge` auto-detect branch changes. `patch revise` creates explicit revisions. @@ -57,12 +57,12 @@ **Independent Test**: Create a patch, comment on revision 1, push a commit, comment on revision 2, verify `patch show --revision 1` shows only revision 1's comments. <!-- sequential --> - [ ] T011 [US2] Add `--revision` optional arg (type `Option<u32>`) to `PatchCmd::Comment` in `src/cli.rs`. Validate: if `--revision` is provided with `--file`/`--line` (inline comment), accept it. If `--revision` is provided without `--file`/`--line` (thread comment), reject with error "thread comments are not revision-scoped". - [ ] T012 [US2] Modify `comment()` in `src/patch.rs` to accept an optional `target_revision: Option<u32>` parameter. If provided, validate it exists in `state.revisions` (error if not). Use it instead of the auto-detected current revision for the `revision` field on the inline comment event. - [ ] T013 [US2] Add `--revision` optional arg (type `Option<u32>`) to `PatchCmd::Show` in `src/cli.rs`. - [ ] T014 [US2] Modify patch show output in `src/lib.rs` (or wherever show is dispatched): when `--revision N` is provided, filter `inline_comments` and `reviews` to only those with `revision == Some(N)`. Thread comments are always shown. When no `--revision`, group inline comments/reviews by revision number in output. Display "(revision: unknown)" for comments with `revision: None`. - [ ] T015 [US2] Update `show_json()` in `src/patch.rs` to include `revisions` array in JSON output. Ensure `InlineComment` and `Review` structs serialize their `revision` field. - [ ] T015a [US2] Wire Phase 3 dispatch in `src/lib.rs`: pass `--revision` arg through to `comment()` and show output. List revisions in `patch show` output. - [x] T011 [US2] Add `--revision` optional arg (type `Option<u32>`) to `PatchCmd::Comment` in `src/cli.rs`. Validate: if `--revision` is provided with `--file`/`--line` (inline comment), accept it. If `--revision` is provided without `--file`/`--line` (thread comment), reject with error "thread comments are not revision-scoped". - [x] T012 [US2] Modify `comment()` in `src/patch.rs` to accept an optional `target_revision: Option<u32>` parameter. If provided, validate it exists in `state.revisions` (error if not). Use it instead of the auto-detected current revision for the `revision` field on the inline comment event. - [x] T013 [US2] Add `--revision` optional arg (type `Option<u32>`) to `PatchCmd::Show` in `src/cli.rs`. - [x] T014 [US2] Modify patch show output in `src/lib.rs` (or wherever show is dispatched): when `--revision N` is provided, filter `inline_comments` and `reviews` to only those with `revision == Some(N)`. Thread comments are always shown. When no `--revision`, group inline comments/reviews by revision number in output. Display "(revision: unknown)" for comments with `revision: None`. - [x] T015 [US2] Update `show_json()` in `src/patch.rs` to include `revisions` array in JSON output. Ensure `InlineComment` and `Review` structs serialize their `revision` field. - [x] T015a [US2] Wire Phase 3 dispatch in `src/lib.rs`: pass `--revision` arg through to `comment()` and show output. List revisions in `patch show` output. **Checkpoint**: Inline comments anchored to revisions. `patch show --revision N` filters correctly. @@ -75,10 +75,10 @@ **Independent Test**: Create a patch with 3 revisions modifying different files, run `patch diff --between 1 3`, verify diff shows only changes between those snapshots. <!-- sequential --> - [ ] T016 [US3] Implement `interdiff()` function in `src/patch.rs`: takes `repo`, `&PatchState`, `from_rev: u32`, `to_rev: u32`. Looks up tree OIDs from `state.revisions` for both revision numbers (error if not found). Computes `repo.diff_tree_to_tree(from_tree, to_tree, None)` and formats as unified diff (reuse the existing diff formatting logic from `generate_diff`). - [ ] T017 [US3] Modify `generate_diff()` in `src/patch.rs` to accept an optional `revision: Option<u32>` parameter. If provided, use that revision's tree OID (from `state.revisions`) instead of resolving the current branch tip. Merge-base still computed against the base branch. - [ ] T018 [US3] Add `--revision` optional arg (type `Option<u32>`) and `--between` arg (type `Option<Vec<u32>>`, 1-2 values) to `PatchCmd::Diff` in `src/cli.rs`. Validate: `--revision` and `--between` are mutually exclusive (error if both). `--between` with single value N means "N to latest". - [ ] T019 [US3] Wire Phase 4 dispatch in `src/lib.rs`: if `--between` provided, call `interdiff()`. If `--revision` provided, call `generate_diff()` with the revision parameter. If neither, call `generate_diff()` without revision (existing behavior). - [x] T016 [US3] Implement `interdiff()` function in `src/patch.rs`: takes `repo`, `&PatchState`, `from_rev: u32`, `to_rev: u32`. Looks up tree OIDs from `state.revisions` for both revision numbers (error if not found). Computes `repo.diff_tree_to_tree(from_tree, to_tree, None)` and formats as unified diff (reuse the existing diff formatting logic from `generate_diff`). - [x] T017 [US3] Modify `generate_diff()` in `src/patch.rs` to accept an optional `revision: Option<u32>` parameter. If provided, use that revision's tree OID (from `state.revisions`) instead of resolving the current branch tip. Merge-base still computed against the base branch. - [x] T018 [US3] Add `--revision` optional arg (type `Option<u32>`) and `--between` arg (type `Option<Vec<u32>>`, 1-2 values) to `PatchCmd::Diff` in `src/cli.rs`. Validate: `--revision` and `--between` are mutually exclusive (error if both). `--between` with single value N means "N to latest". - [x] T019 [US3] Wire Phase 4 dispatch in `src/lib.rs`: if `--between` provided, call `interdiff()`. If `--revision` provided, call `generate_diff()` with the revision parameter. If neither, call `generate_diff()` without revision (existing behavior). **Checkpoint**: Interdiff works between any two revisions. Historical diff shows a specific revision against the base. @@ -91,9 +91,9 @@ **Independent Test**: Create a patch with 3 revisions, run `patch log`, verify all 3 listed with correct data. <!-- sequential --> - [ ] T020 [US4] Implement `patch_log()` function in `src/patch.rs`: takes `repo` and `id_prefix`. Materializes PatchState, iterates `state.revisions`. For each consecutive pair, computes file-change summary by diffing their tree OIDs (`repo.diff_tree_to_tree`). Returns structured data (Vec of revision info with file stats). - [ ] T021 [US4] Implement `patch_log_to_writer()` in `src/patch.rs` for human-readable output: format each revision as `Revision N <short_oid> <timestamp> <files_summary> "<body>"`. First revision gets "(initial)" label. Implement `patch_log_json()` for JSON output. - [ ] T022 [US4] Add `PatchCmd::Log` variant to `src/cli.rs` with `id: String` and `--json` flag. Wire Phase 5 dispatch in `src/lib.rs` to call `patch_log_to_writer()` or `patch_log_json()`. - [x] T020 [US4] Implement `patch_log()` function in `src/patch.rs`: takes `repo` and `id_prefix`. Materializes PatchState, iterates `state.revisions`. For each consecutive pair, computes file-change summary by diffing their tree OIDs (`repo.diff_tree_to_tree`). Returns structured data (Vec of revision info with file stats). - [x] T021 [US4] Implement `patch_log_to_writer()` in `src/patch.rs` for human-readable output: format each revision as `Revision N <short_oid> <timestamp> <files_summary> "<body>"`. First revision gets "(initial)" label. Implement `patch_log_json()` for JSON output. - [x] T022 [US4] Add `PatchCmd::Log` variant to `src/cli.rs` with `id: String` and `--json` flag. Wire Phase 5 dispatch in `src/lib.rs` to call `patch_log_to_writer()` or `patch_log_json()`. **Checkpoint**: `patch log` shows revision history with file-change summaries. @@ -106,11 +106,11 @@ **Independent Test**: Approve on revision 1, push new code (revision 2), verify `patch show` displays "approved (revision 1)". <!-- sequential --> - [ ] T023 [US5] Add `--revision` optional arg (type `Option<u32>`) to `PatchCmd::Review` in `src/cli.rs`. - [ ] T024 [US5] Modify `review()` in `src/patch.rs` to accept an optional `target_revision: Option<u32>` parameter. If provided, validate it exists in `state.revisions` (error if not). Use it instead of the auto-detected revision for the `revision` field on the review event. - [ ] T025 [US5] Update review display in patch show output: show review verdict with revision context, e.g., "approved (revision 3)". Highlight latest review per reviewer. - [ ] T026 [US5] Implement merge policy config reader: read `merge.require_approval_on_latest` from `refs/collab/config` (JSON blob). Default to `false` if config ref doesn't exist or key is absent. - [ ] T027 [US5] Modify `merge()` in `src/patch.rs`: after existing checks, if `merge.require_approval_on_latest` is true, verify at least one `Approve` review has `revision == latest_revision_number`. Error if not: "merge requires approval on the latest revision (revision N)". - [x] T023 [US5] Add `--revision` optional arg (type `Option<u32>`) to `PatchCmd::Review` in `src/cli.rs`. - [x] T024 [US5] Modify `review()` in `src/patch.rs` to accept an optional `target_revision: Option<u32>` parameter. If provided, validate it exists in `state.revisions` (error if not). Use it instead of the auto-detected revision for the `revision` field on the review event. - [x] T025 [US5] Update review display in patch show output: show review verdict with revision context, e.g., "approved (revision 3)". Highlight latest review per reviewer. - [x] T026 [US5] Implement merge policy config reader: read `merge.require_approval_on_latest` from `refs/collab/config` (JSON blob). Default to `false` if config ref doesn't exist or key is absent. - [x] T027 [US5] Modify `merge()` in `src/patch.rs`: after existing checks, if `merge.require_approval_on_latest` is true, verify at least one `Approve` review has `revision == latest_revision_number`. Error if not: "merge requires approval on the latest revision (revision N)". **Checkpoint**: Reviews show revision context. Merge policy enforces latest-revision approval when configured. @@ -121,8 +121,8 @@ **Purpose**: TUI updates, backward compat verification, dispatch wiring <!-- parallel-group: 1 (max 2 concurrent) --> - [ ] T028 [P] Update `src/tui/widgets.rs`: show revision count (e.g., "r3") alongside patch status in the patch list view. In patch detail view, show revision list and annotate inline comments with their revision number. - [ ] T029 [P] Create `tests/revision_test.rs`: integration tests for revision recording (auto-detect on comment/review/merge), dedup by commit OID, revision numbering from DAG order, interdiff between revisions, anchored comments/reviews, `patch revise` rejection when unchanged, `--between N` single-arg shorthand, sync round-trip for PatchRevision events. - [x] T028 [P] Update `src/tui/widgets.rs`: show revision count (e.g., "r3") alongside patch status in the patch list view. In patch detail view, show revision list and annotate inline comments with their revision number. - [x] T029 [P] Create `tests/revision_test.rs`: integration tests for revision recording (auto-detect on comment/review/merge), dedup by commit OID, revision numbering from DAG order, interdiff between revisions, anchored comments/reviews, `patch revise` rejection when unchanged, `--between N` single-arg shorthand, sync round-trip for PatchRevision events. --- diff --git a/src/cli.rs b/src/cli.rs index 5c9acff..86fe1fc 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -266,11 +266,20 @@ pub enum PatchCmd { /// Output as JSON #[arg(long)] json: bool, /// Show only comments/reviews from this revision #[arg(long)] revision: Option<u32>, }, /// Show diff between base and head Diff { /// Patch ID (prefix match) id: String, /// Show historical diff for a specific revision against the base #[arg(long)] revision: Option<u32>, /// Show interdiff between two revisions (N M or just N for N..latest) #[arg(long, num_args = 1..=2)] between: Option<Vec<u32>>, }, /// Comment on a patch (use --file and --line for inline comments) Comment { @@ -285,6 +294,9 @@ pub enum PatchCmd { /// Line number for inline comment #[arg(short, long)] line: Option<u32>, /// Target revision for inline comment #[arg(long)] revision: Option<u32>, }, /// Review a patch Review { @@ -296,15 +308,26 @@ pub enum PatchCmd { /// Review body #[arg(short, long)] body: String, /// Target revision for review #[arg(long)] revision: Option<u32>, }, /// Revise a patch (record a revision note) /// Revise a patch (record a new revision snapshot) Revise { /// Patch ID (prefix match) id: String, /// Updated description /// Revision description #[arg(short, long)] body: Option<String>, }, /// Show revision log for a patch Log { /// Patch ID (prefix match) id: String, /// Output as JSON #[arg(long)] json: bool, }, /// Merge a patch into its base branch Merge { /// Patch ID (prefix match) diff --git a/src/dag.rs b/src/dag.rs index b4091c9..9e7b577 100644 --- a/src/dag.rs +++ b/src/dag.rs @@ -332,7 +332,7 @@ fn commit_message(action: &Action) -> String { Action::IssueClose { .. } => "issue: close".to_string(), Action::IssueReopen => "issue: reopen".to_string(), Action::PatchCreate { title, .. } => format!("patch: create \"{}\"", title), Action::PatchRevise { .. } => "patch: revise".to_string(), Action::PatchRevision { .. } => "patch: revision".to_string(), Action::PatchReview { verdict, .. } => format!("patch: review ({:?})", verdict), Action::PatchComment { .. } => "patch: comment".to_string(), Action::PatchInlineComment { ref file, line, .. } => { diff --git a/src/event.rs b/src/event.rs index fbcd2d6..86804c1 100644 --- a/src/event.rs +++ b/src/event.rs @@ -65,15 +65,21 @@ pub enum Action { branch: String, #[serde(default, skip_serializing_if = "Option::is_none")] fixes: Option<String>, commit: String, tree: String, }, #[serde(rename = "patch.revise")] PatchRevise { #[serde(rename = "patch.revision")] PatchRevision { commit: String, tree: String, #[serde(default, skip_serializing_if = "Option::is_none")] body: Option<String>, }, #[serde(rename = "patch.review")] PatchReview { verdict: ReviewVerdict, body: String, revision: u32, }, #[serde(rename = "patch.comment")] PatchComment { @@ -84,6 +90,7 @@ pub enum Action { file: String, line: u32, body: String, revision: u32, }, #[serde(rename = "patch.close")] PatchClose { diff --git a/src/lib.rs b/src/lib.rs index 822c0ac..18e9672 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -209,7 +209,7 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> { } Ok(()) } PatchCmd::Show { id, json } => { PatchCmd::Show { id, json, revision } => { if json { let output = patch::show_json(repo, &id)?; println!("{}", output); @@ -221,7 +221,8 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> { PatchStatus::Closed => "closed", PatchStatus::Merged => "merged", }; println!("Patch {} [{}]", &p.id[..8], status); let rev_count = p.revisions.len(); println!("Patch {} [{}] (r{})", &p.id[..8], status, rev_count); println!("Title: {}", p.title); println!("Author: {} <{}>", p.author.name, p.author.email); match p.resolve_head(repo) { @@ -240,27 +241,51 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> { if let Some(ref fixes) = p.fixes { println!("Fixes: {:.8}", fixes); } // Show revisions if !p.revisions.is_empty() { println!("\n--- Revisions ---"); for rev in &p.revisions { let short = if rev.commit.len() >= 8 { &rev.commit[..8] } else { &rev.commit }; let body_display = rev.body.as_deref().map(|b| format!(" {}", b)).unwrap_or_default(); println!(" r{}: {} ({}){}", rev.number, short, rev.timestamp, body_display); } } if !p.body.is_empty() { println!("\n{}", p.body); } if !p.reviews.is_empty() { // Filter reviews by revision if requested let reviews: Vec<_> = if let Some(rev) = revision { p.reviews.iter().filter(|r| r.revision == Some(rev)).collect() } else { p.reviews.iter().collect() }; if !reviews.is_empty() { println!("\n--- Reviews ---"); for r in &p.reviews { for r in &reviews { let rev_label = r.revision.map(|n| format!(" (revision {})", n)).unwrap_or_default(); println!( "\n{} ({:?}) - {}:\n{}", r.author.name, r.verdict, r.timestamp, r.body "\n{} ({:?}) - {}{}:\n{}", r.author.name, r.verdict, r.timestamp, rev_label, r.body ); } } if !p.inline_comments.is_empty() { // Filter inline comments by revision if requested let inline_comments: Vec<_> = if let Some(rev) = revision { p.inline_comments.iter().filter(|c| c.revision == Some(rev)).collect() } else { p.inline_comments.iter().collect() }; if !inline_comments.is_empty() { println!("\n--- Inline Comments ---"); for c in &p.inline_comments { for c in &inline_comments { let rev_label = c.revision.map(|n| format!(" r{}", n)).unwrap_or_default(); println!( "\n{} on {}:{} ({}):\n {}", c.author.name, c.file, c.line, c.timestamp, c.body "\n{} on {}:{} ({}{}):\n {}", c.author.name, c.file, c.line, c.timestamp, rev_label, c.body ); } } // Thread comments always shown if !p.comments.is_empty() { println!("\n--- Comments ---"); for c in &p.comments { @@ -269,8 +294,18 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> { } Ok(()) } PatchCmd::Diff { id } => { let diff = patch::diff(repo, &id)?; PatchCmd::Diff { id, revision, between } => { if revision.is_some() && between.is_some() { return Err(error::Error::Cmd( "--revision and --between are mutually exclusive".to_string(), )); } let between_pair = between.map(|v| { let from = v[0]; let to = v.get(1).copied(); (from, to) }); let diff = patch::diff(repo, &id, revision, between_pair)?; if diff.is_empty() { println!("No diff available (commits may be identical)."); } else { @@ -283,12 +318,13 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> { body, file, line, revision, } => { patch::comment(repo, &id, &body, file.as_deref(), line)?; patch::comment(repo, &id, &body, file.as_deref(), line, revision)?; println!("Comment added."); Ok(()) } PatchCmd::Review { id, verdict, body } => { PatchCmd::Review { id, verdict, body, revision } => { let v = match verdict.as_str() { "approve" => ReviewVerdict::Approve, "request-changes" => ReviewVerdict::RequestChanges, @@ -300,7 +336,7 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> { .into()); } }; patch::review(repo, &id, v, &body)?; patch::review(repo, &id, v, &body, revision)?; println!("Review submitted."); Ok(()) } @@ -309,6 +345,16 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> { println!("Patch revised."); Ok(()) } PatchCmd::Log { id, json } => { let p = patch::patch_log(repo, &id)?; if json { let output = patch::patch_log_json(&p)?; println!("{}", output); } else { patch::patch_log_to_writer(repo, &p, &mut std::io::stdout())?; } Ok(()) } PatchCmd::Merge { id } => { let p = patch::merge(repo, &id)?; println!("Patch {:.8} merged into {}.", p.id, p.base_ref); diff --git a/src/log.rs b/src/log.rs index 143962e..b9ad607 100644 --- a/src/log.rs +++ b/src/log.rs @@ -116,7 +116,7 @@ fn action_type_name(action: &Action) -> String { Action::IssueUnassign { .. } => "IssueUnassign".to_string(), Action::IssueReopen => "IssueReopen".to_string(), Action::PatchCreate { .. } => "PatchCreate".to_string(), Action::PatchRevise { .. } => "PatchRevise".to_string(), Action::PatchRevision { .. } => "PatchRevision".to_string(), Action::PatchReview { .. } => "PatchReview".to_string(), Action::PatchComment { .. } => "PatchComment".to_string(), Action::PatchInlineComment { .. } => "PatchInlineComment".to_string(), @@ -150,9 +150,9 @@ fn action_summary(action: &Action) -> String { Action::IssueUnassign { assignee } => format!("unassign \"{}\"", assignee), Action::IssueReopen => "reopen".to_string(), Action::PatchCreate { title, .. } => format!("create \"{}\"", title), Action::PatchRevise { body, .. } => match body { Some(b) => format!("revise: {}", truncate(b, 50)), None => "revise".to_string(), Action::PatchRevision { body, .. } => match body { Some(b) => format!("revision: {}", truncate(b, 50)), None => "revision".to_string(), }, Action::PatchReview { verdict, .. } => format!("review: {:?}", verdict), Action::PatchComment { body } => truncate(body, 60), diff --git a/src/patch.rs b/src/patch.rs index 7b70e49..590f2f5 100644 --- a/src/patch.rs +++ b/src/patch.rs @@ -1,4 +1,4 @@ use git2::{DiffFormat, Repository}; use git2::{DiffFormat, Oid, Repository}; use crate::cli::SortMode; use crate::dag; @@ -8,6 +8,47 @@ use crate::identity::get_author; use crate::signing; use crate::state::{self, PatchState, PatchStatus}; /// Auto-detect whether the branch tip has changed since the last recorded revision. /// If it has, append a PatchRevision event. Returns the current revision number. fn auto_detect_revision( repo: &Repository, ref_name: &str, patch: &PatchState, sk: &ed25519_dalek::SigningKey, ) -> Result<u32, Error> { let current_rev = patch.revisions.last().map(|r| r.number).unwrap_or(0); // Try to resolve the branch tip let tip_oid = match patch.resolve_head(repo) { Ok(oid) => oid, Err(_) => return Ok(current_rev), // Branch deleted or unavailable }; let last_commit = patch.revisions.last().map(|r| r.commit.as_str()).unwrap_or(""); let tip_hex = tip_oid.to_string(); if tip_hex == last_commit { return Ok(current_rev); } // Branch tip changed — insert a PatchRevision event let commit = repo.find_commit(tip_oid)?; let tree_oid = commit.tree()?.id(); let author = get_author(repo)?; let event = Event { timestamp: chrono::Utc::now().to_rfc3339(), author, action: Action::PatchRevision { commit: tip_hex, tree: tree_oid.to_string(), body: None, }, clock: 0, }; dag::append_event(repo, ref_name, &event, sk)?; Ok(current_rev + 1) } pub fn create( repo: &Repository, title: &str, @@ -23,9 +64,9 @@ pub fn create( )); } // Verify branch exists // Verify branch exists and get tip let branch_ref = format!("refs/heads/{}", branch); repo.refname_to_id(&branch_ref) let tip_oid = repo.refname_to_id(&branch_ref) .map_err(|e| crate::error::Error::Cmd(format!("branch '{}' not found: {}", branch, e)))?; // Check for duplicate: scan open patches for matching branch @@ -39,6 +80,10 @@ pub fn create( } } // Get commit and tree OIDs for revision 1 let commit = repo.find_commit(tip_oid)?; let tree_oid = commit.tree()?.id(); let sk = signing::load_signing_key(&signing::signing_key_dir()?)?; let author = get_author(repo)?; let event = Event { @@ -50,6 +95,8 @@ pub fn create( base_ref: base_ref.to_string(), branch: branch.to_string(), fixes: fixes.map(|s| s.to_string()), commit: tip_oid.to_string(), tree: tree_oid.to_string(), }, clock: 0, }; @@ -155,26 +202,60 @@ pub fn comment( body: &str, file: Option<&str>, line: Option<u32>, target_revision: Option<u32>, ) -> Result<(), crate::error::Error> { let sk = signing::load_signing_key(&signing::signing_key_dir()?)?; let (ref_name, _id) = state::resolve_patch_ref(repo, id_prefix)?; let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; let patch = PatchState::from_ref(repo, &ref_name, &id)?; // Auto-detect revision (runs for all comment types to record branch changes) let current_rev = auto_detect_revision(repo, &ref_name, &patch, &sk)?; // Re-read state after potential revision insertion let patch = if current_rev != patch.revisions.last().map(|r| r.number).unwrap_or(0) { PatchState::from_ref_uncached(repo, &ref_name, &id)? } else { patch }; let author = get_author(repo)?; let action = match (file, line) { (Some(f), Some(l)) => Action::PatchInlineComment { file: f.to_string(), line: l, body: body.to_string(), }, (Some(f), Some(l)) => { // Determine revision for the inline comment let rev = if let Some(target) = target_revision { // Validate target revision exists if !patch.revisions.iter().any(|r| r.number == target) { return Err(Error::Cmd(format!("revision {} not found", target))); } target } else { patch.revisions.last().map(|r| r.number).unwrap_or(1) }; Action::PatchInlineComment { file: f.to_string(), line: l, body: body.to_string(), revision: rev, } } (Some(_), None) | (None, Some(_)) => { return Err(git2::Error::from_str( "--file and --line must both be provided for inline comments", ) .into()); } (None, None) => Action::PatchComment { body: body.to_string(), }, (None, None) => { // Thread comment — not revision-anchored if target_revision.is_some() { return Err(Error::Cmd( "thread comments are not revision-scoped; use --file and --line for inline comments".to_string(), )); } Action::PatchComment { body: body.to_string(), } } }; let event = Event { @@ -192,9 +273,31 @@ pub fn review( id_prefix: &str, verdict: ReviewVerdict, body: &str, target_revision: Option<u32>, ) -> Result<(), crate::error::Error> { let sk = signing::load_signing_key(&signing::signing_key_dir()?)?; let (ref_name, _id) = state::resolve_patch_ref(repo, id_prefix)?; let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; let patch = PatchState::from_ref(repo, &ref_name, &id)?; // Auto-detect revision let current_rev = auto_detect_revision(repo, &ref_name, &patch, &sk)?; // Re-read state after potential revision insertion let patch = if current_rev != patch.revisions.last().map(|r| r.number).unwrap_or(0) { PatchState::from_ref_uncached(repo, &ref_name, &id)? } else { patch }; let rev = if let Some(target) = target_revision { if !patch.revisions.iter().any(|r| r.number == target) { return Err(Error::Cmd(format!("revision {} not found", target))); } target } else { patch.revisions.last().map(|r| r.number).unwrap_or(1) }; let author = get_author(repo)?; let event = Event { timestamp: chrono::Utc::now().to_rfc3339(), @@ -202,6 +305,7 @@ pub fn review( action: Action::PatchReview { verdict, body: body.to_string(), revision: rev, }, clock: 0, }; @@ -215,12 +319,31 @@ pub fn revise( body: Option<&str>, ) -> Result<(), crate::error::Error> { let sk = signing::load_signing_key(&signing::signing_key_dir()?)?; let (ref_name, _id) = state::resolve_patch_ref(repo, id_prefix)?; let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; let patch = PatchState::from_ref(repo, &ref_name, &id)?; // Resolve current branch tip let tip_oid = patch.resolve_head(repo)?; let tip_hex = tip_oid.to_string(); let last_commit = patch.revisions.last().map(|r| r.commit.as_str()).unwrap_or(""); if tip_hex == last_commit { let current_rev = patch.revisions.last().map(|r| r.number).unwrap_or(0); return Err(Error::Cmd(format!( "no changes since revision {}", current_rev ))); } let commit = repo.find_commit(tip_oid)?; let tree_oid = commit.tree()?.id(); let author = get_author(repo)?; let event = Event { timestamp: chrono::Utc::now().to_rfc3339(), author, action: Action::PatchRevise { action: Action::PatchRevision { commit: tip_hex, tree: tree_oid.to_string(), body: body.map(|s| s.to_string()), }, clock: 0, @@ -242,7 +365,27 @@ pub fn merge(repo: &Repository, id_prefix: &str) -> Result<PatchState, crate::er .into()); } // Resolve the head commit: use branch tip for branch-based patches, stored OID otherwise // Auto-detect revision before merge auto_detect_revision(repo, &ref_name, &p, &sk)?; // Check merge policy let config = read_collab_config(repo); if config.require_approval_on_latest { // Re-read state after potential revision insertion let p = PatchState::from_ref_uncached(repo, &ref_name, &id)?; let latest_rev = p.revisions.last().map(|r| r.number).unwrap_or(0); let has_approval = p.reviews.iter().any(|r| { r.verdict == ReviewVerdict::Approve && r.revision == Some(latest_rev) }); if !has_approval { return Err(Error::Cmd(format!( "merge requires approval on the latest revision (revision {})", latest_rev ))); } } // Resolve the head commit let head_oid = p.resolve_head(repo)?; let head_commit = repo.find_commit(head_oid) .map_err(|_| git2::Error::from_str("cannot resolve head commit in patch"))?; @@ -319,10 +462,23 @@ pub fn merge(repo: &Repository, id_prefix: &str) -> Result<PatchState, crate::er } /// Generate a unified diff between a patch's base branch and head commit. pub fn diff(repo: &Repository, id_prefix: &str) -> Result<String, Error> { pub fn diff( repo: &Repository, id_prefix: &str, revision: Option<u32>, between: Option<(u32, Option<u32>)>, ) -> Result<String, Error> { let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; let p = PatchState::from_ref(repo, &ref_name, &id)?; generate_diff(repo, &p) if let Some((from, to)) = between { let to_rev = to.unwrap_or_else(|| p.revisions.last().map(|r| r.number).unwrap_or(1)); interdiff(repo, &p, from, to_rev) } else if let Some(rev) = revision { generate_diff_at_revision(repo, &p, rev) } else { generate_diff(repo, &p) } } /// Generate a diff string from a patch's base and head using three-dot (merge-base) diff. @@ -346,12 +502,67 @@ pub fn generate_diff(repo: &Repository, patch: &state::PatchState) -> Result<Str }; let git_diff = repo.diff_tree_to_tree(base_tree.as_ref(), Some(&head_tree), None)?; format_diff(&git_diff) } /// Generate a diff for a specific revision against the base branch (historical full diff). fn generate_diff_at_revision( repo: &Repository, patch: &PatchState, rev_number: u32, ) -> Result<String, Error> { let revision = patch.revisions.iter().find(|r| r.number == rev_number) .ok_or_else(|| Error::Cmd(format!("revision {} not found", rev_number)))?; let tree_oid = Oid::from_str(&revision.tree)?; let head_tree = repo.find_tree(tree_oid)?; let base_ref = format!("refs/heads/{}", patch.base_ref); let commit_oid = Oid::from_str(&revision.commit)?; let base_tree = if let Ok(base_oid) = repo.refname_to_id(&base_ref) { if let Ok(merge_base_oid) = repo.merge_base(base_oid, commit_oid) { let merge_base_commit = repo.find_commit(merge_base_oid)?; Some(merge_base_commit.tree()?) } else { let base_commit = repo.find_commit(base_oid)?; Some(base_commit.tree()?) } } else { None }; let git_diff = repo.diff_tree_to_tree(base_tree.as_ref(), Some(&head_tree), None)?; format_diff(&git_diff) } /// Compute the interdiff between two revisions. pub fn interdiff( repo: &Repository, patch: &PatchState, from_rev: u32, to_rev: u32, ) -> Result<String, Error> { let from = patch.revisions.iter().find(|r| r.number == from_rev) .ok_or_else(|| Error::Cmd(format!("revision {} not found", from_rev)))?; let to = patch.revisions.iter().find(|r| r.number == to_rev) .ok_or_else(|| Error::Cmd(format!("revision {} not found", to_rev)))?; let from_tree_oid = Oid::from_str(&from.tree)?; let to_tree_oid = Oid::from_str(&to.tree)?; let from_tree = repo.find_tree(from_tree_oid)?; let to_tree = repo.find_tree(to_tree_oid)?; let git_diff = repo.diff_tree_to_tree(Some(&from_tree), Some(&to_tree), None)?; format_diff(&git_diff) } /// Format a git2::Diff as a unified diff string. fn format_diff(git_diff: &git2::Diff) -> Result<String, Error> { let mut output = String::new(); let mut lines = 0usize; git_diff.print(DiffFormat::Patch, |_delta, _hunk, line| { if lines >= 5000 { return true; // stop appending but don't abort libgit2 return true; } let prefix = match line.origin() { '+' => "+", @@ -374,6 +585,65 @@ pub fn generate_diff(repo: &Repository, patch: &state::PatchState) -> Result<Str Ok(output) } /// Patch log: list all revisions with timestamps and file-change summaries. pub fn patch_log( repo: &Repository, id_prefix: &str, ) -> Result<PatchState, Error> { let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; PatchState::from_ref(repo, &ref_name, &id) } pub fn patch_log_to_writer( repo: &Repository, patch: &PatchState, writer: &mut dyn std::io::Write, ) -> Result<(), Error> { if patch.revisions.is_empty() { writeln!(writer, "No revisions recorded.").ok(); return Ok(()); } for (i, rev) in patch.revisions.iter().enumerate() { let short_oid = if rev.commit.len() >= 8 { &rev.commit[..8] } else { &rev.commit }; let label = if i == 0 { " (initial)" } else { "" }; let body_display = rev.body.as_deref().map(|b| format!(" \"{}\"", b)).unwrap_or_default(); // Compute file-change summary between consecutive revisions let file_summary = if i > 0 { let prev = &patch.revisions[i - 1]; match (Oid::from_str(&prev.tree), Oid::from_str(&rev.tree)) { (Ok(from_oid), Ok(to_oid)) => { if let (Ok(from_tree), Ok(to_tree)) = (repo.find_tree(from_oid), repo.find_tree(to_oid)) { if let Ok(diff) = repo.diff_tree_to_tree(Some(&from_tree), Some(&to_tree), None) { let stats = diff.stats().ok(); stats.map(|s| format!(" {} file(s) changed, +{} -{}", s.files_changed(), s.insertions(), s.deletions())).unwrap_or_default() } else { String::new() } } else { String::new() } } _ => String::new(), } } else { String::new() }; writeln!( writer, "Revision {} {} {}{}{}{}", rev.number, short_oid, rev.timestamp, label, file_summary, body_display ).ok(); } Ok(()) } pub fn patch_log_json(patch: &PatchState) -> Result<String, Error> { Ok(serde_json::to_string_pretty(&patch.revisions)?) } pub fn delete(repo: &Repository, id_prefix: &str) -> Result<String, crate::error::Error> { let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; repo.find_reference(&ref_name)?.delete()?; @@ -404,3 +674,42 @@ pub fn close( Ok(()) } /// Collab config (from refs/collab/config). #[derive(Default)] struct CollabConfig { require_approval_on_latest: bool, } fn read_collab_config(repo: &Repository) -> CollabConfig { let config_ref = "refs/collab/config"; let tip = match repo.refname_to_id(config_ref) { Ok(oid) => oid, Err(_) => return CollabConfig::default(), }; let commit = match repo.find_commit(tip) { Ok(c) => c, Err(_) => return CollabConfig::default(), }; let tree = match commit.tree() { Ok(t) => t, Err(_) => return CollabConfig::default(), }; let entry = match tree.get_name("config.json") { Some(e) => e, None => return CollabConfig::default(), }; let blob = match repo.find_blob(entry.id()) { Ok(b) => b, Err(_) => return CollabConfig::default(), }; let val: serde_json::Value = match serde_json::from_slice(blob.content()) { Ok(v) => v, Err(_) => return CollabConfig::default(), }; CollabConfig { require_approval_on_latest: val .pointer("/merge/require_approval_on_latest") .and_then(|v| v.as_bool()) .unwrap_or(false), } } diff --git a/src/state.rs b/src/state.rs index 0378c93..5521693 100644 --- a/src/state.rs +++ b/src/state.rs @@ -98,6 +98,8 @@ pub struct Review { pub verdict: ReviewVerdict, pub body: String, pub timestamp: String, #[serde(default)] pub revision: Option<u32>, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -110,12 +112,23 @@ pub enum PatchStatus { } #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Revision { pub number: u32, pub commit: String, pub tree: String, pub body: Option<String>, pub timestamp: String, } #[derive(Debug, Clone, Serialize, Deserialize)] pub struct InlineComment { pub author: Author, pub file: String, pub line: u32, pub body: String, pub timestamp: String, #[serde(default)] pub revision: Option<u32>, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -130,6 +143,7 @@ pub struct PatchState { pub comments: Vec<Comment>, pub inline_comments: Vec<InlineComment>, pub reviews: Vec<Review>, pub revisions: Vec<Revision>, pub created_at: String, #[serde(default)] pub last_updated: String, @@ -344,7 +358,16 @@ impl PatchState { base_ref, branch, fixes, commit, tree, } => { let revisions = vec![Revision { number: 1, commit: commit.clone(), tree, body: None, timestamp: event.timestamp.clone(), }]; state = Some(PatchState { id: id.to_string(), title, @@ -356,25 +379,36 @@ impl PatchState { comments: Vec::new(), inline_comments: Vec::new(), reviews: Vec::new(), revisions, created_at: event.timestamp.clone(), last_updated: String::new(), author: event.author.clone(), }); } Action::PatchRevise { body } => { Action::PatchRevision { commit, tree, body } => { if let Some(ref mut s) = state { if let Some(b) = body { s.body = b; // Dedup by commit OID — skip if already seen let already_seen = s.revisions.iter().any(|r| r.commit == commit); if !already_seen { let number = s.revisions.len() as u32 + 1; s.revisions.push(Revision { number, commit, tree, body, timestamp: event.timestamp.clone(), }); } } } Action::PatchReview { verdict, body } => { Action::PatchReview { verdict, body, revision } => { if let Some(ref mut s) = state { s.reviews.push(Review { author: event.author.clone(), verdict, body, timestamp: event.timestamp.clone(), revision: Some(revision), }); } } @@ -388,7 +422,7 @@ impl PatchState { }); } } Action::PatchInlineComment { file, line, body } => { Action::PatchInlineComment { file, line, body, revision } => { if let Some(ref mut s) = state { s.inline_comments.push(InlineComment { author: event.author.clone(), @@ -396,6 +430,7 @@ impl PatchState { line, body, timestamp: event.timestamp.clone(), revision: Some(revision), }); } } diff --git a/src/tui/mod.rs b/src/tui/mod.rs index 5ea36ad..bf73cc5 100644 --- a/src/tui/mod.rs +++ b/src/tui/mod.rs @@ -82,6 +82,7 @@ mod tests { comments: vec![], inline_comments: vec![], reviews: vec![], revisions: vec![], created_at: String::new(), last_updated: String::new(), author: make_author(), @@ -276,6 +277,7 @@ mod tests { comments: Vec::new(), inline_comments: Vec::new(), reviews: Vec::new(), revisions: Vec::new(), created_at: "2026-01-01T00:00:00Z".to_string(), last_updated: "2026-01-01T00:00:00Z".to_string(), author: test_author(), @@ -404,6 +406,8 @@ mod tests { base_ref: "main".to_string(), branch: "feature/test".to_string(), fixes: None, commit: "abc123".to_string(), tree: "def456".to_string(), }; assert_eq!(action_type_label(&action), "Patch Create"); } @@ -413,6 +417,7 @@ mod tests { let action = Action::PatchReview { verdict: ReviewVerdict::Approve, body: "lgtm".to_string(), revision: 1, }; assert_eq!(action_type_label(&action), "Patch Review"); } @@ -423,6 +428,7 @@ mod tests { file: "src/main.rs".to_string(), line: 42, body: "nit".to_string(), revision: 1, }; assert_eq!(action_type_label(&action), "Inline Comment"); } @@ -481,6 +487,7 @@ mod tests { action: Action::PatchReview { verdict: ReviewVerdict::Approve, body: "Looks good!".to_string(), revision: 1, }, clock: 0, }; diff --git a/src/tui/widgets.rs b/src/tui/widgets.rs index f7d32ef..d514475 100644 --- a/src/tui/widgets.rs +++ b/src/tui/widgets.rs @@ -14,7 +14,7 @@ pub(crate) fn action_type_label(action: &Action) -> &str { Action::IssueClose { .. } => "Issue Close", Action::IssueReopen => "Issue Reopen", Action::PatchCreate { .. } => "Patch Create", Action::PatchRevise { .. } => "Patch Revise", Action::PatchRevision { .. } => "Patch Revision", Action::PatchReview { .. } => "Patch Review", Action::PatchComment { .. } => "Patch Comment", Action::PatchInlineComment { .. } => "Inline Comment", @@ -68,20 +68,22 @@ pub(crate) fn format_event_detail(oid: &Oid, event: &crate::event::Event) -> Str detail.push_str(&format!("\n{}\n", body)); } } Action::PatchRevise { body } => { Action::PatchRevision { commit, tree, body } => { detail.push_str(&format!("\nCommit: {}\n", commit)); detail.push_str(&format!("Tree: {}\n", tree)); if let Some(b) = body { if !b.is_empty() { detail.push_str(&format!("\n{}\n", b)); } } } Action::PatchReview { verdict, body } => { Action::PatchReview { verdict, body, .. } => { detail.push_str(&format!("\nVerdict: {:?}\n", verdict)); if !body.is_empty() { detail.push_str(&format!("\n{}\n", body)); } } Action::PatchInlineComment { file, line, body } => { Action::PatchInlineComment { file, line, body, .. } => { detail.push_str(&format!("\nFile: {}:{}\n", file, line)); if !body.is_empty() { detail.push_str(&format!("\n{}\n", body)); diff --git a/tests/adversarial_test.rs b/tests/adversarial_test.rs index c56105d..c996ea2 100644 --- a/tests/adversarial_test.rs +++ b/tests/adversarial_test.rs @@ -542,9 +542,15 @@ fn arb_action() -> impl Strategy<Value = Action> { base_ref, branch, fixes: None, commit: "dummy_commit".to_string(), tree: "dummy_tree".to_string(), } }), proptest::option::of(".*").prop_map(|body| Action::PatchRevise { body }), proptest::option::of(".*").prop_map(|body| Action::PatchRevision { commit: "dummy_commit".to_string(), tree: "dummy_tree".to_string(), body, }), (".*",).prop_map(|(body,)| Action::PatchComment { body }), Just(Action::PatchMerge), Just(Action::Merge), diff --git a/tests/collab_test.rs b/tests/collab_test.rs index 9233a7d..24bbeac 100644 --- a/tests/collab_test.rs +++ b/tests/collab_test.rs @@ -332,7 +332,9 @@ fn test_patch_review_workflow() { let event = Event { timestamp: now(), author: alice(), action: Action::PatchRevise { action: Action::PatchRevision { commit: "dummy_commit".to_string(), tree: "dummy_tree".to_string(), body: Some("Updated implementation".to_string()), }, clock: 0, @@ -343,7 +345,9 @@ fn test_patch_review_workflow() { let state = PatchState::from_ref(&repo, &ref_name, &id).unwrap(); assert_eq!(state.reviews.len(), 2); assert_eq!(state.body, "Updated implementation"); // PatchRevision body is on the revision, not the patch body assert_eq!(state.revisions.len(), 2); // revision 1 from create + revision 2 from PatchRevision assert_eq!(state.revisions[1].body.as_deref(), Some("Updated implementation")); } #[test] @@ -637,6 +641,8 @@ fn create_branch_patch( base_ref: base_ref.to_string(), branch: branch.to_string(), fixes: None, commit: "dummy_commit".to_string(), tree: "dummy_tree".to_string(), }, clock: 0, }; @@ -850,7 +856,9 @@ fn test_patch_create_with_head_commit_field_deserializes() { "title": "Agent patch", "body": "from worktree", "base_ref": "main", "head_commit": "abc123def456" "head_commit": "abc123def456", "commit": "abc123", "tree": "def456" } }"#; let event: Event = serde_json::from_str(json).unwrap(); @@ -872,7 +880,9 @@ fn test_patch_create_with_branch_field_still_works() { "title": "Normal patch", "body": "", "base_ref": "main", "branch": "feature-branch" "branch": "feature-branch", "commit": "abc123", "tree": "def456" } }"#; let event: Event = serde_json::from_str(json).unwrap(); @@ -903,6 +913,8 @@ fn test_resolve_head_with_oid_string() { base_ref: "main".to_string(), branch: tip.to_string(), fixes: None, commit: "dummy_commit".to_string(), tree: "dummy_tree".to_string(), }, clock: 0, }; diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 3d87e7e..fdc2113 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -45,7 +45,8 @@ pub fn setup_signing_key(config_dir: &Path) { git_collab::signing::generate_keypair(config_dir).expect("generate test keypair"); } /// Create a non-bare repo in a directory with user identity configured. /// Create a non-bare repo in a directory with user identity configured /// and an initial empty commit on `main`. pub fn init_repo(dir: &Path, author: &Author) -> Repository { let repo = Repository::init(dir).expect("init repo"); { @@ -53,6 +54,13 @@ pub fn init_repo(dir: &Path, author: &Author) -> Repository { config.set_str("user.name", &author.name).unwrap(); config.set_str("user.email", &author.email).unwrap(); } // Create initial empty commit so that HEAD and refs/heads/main exist { let sig = git2::Signature::now(&author.name, &author.email).unwrap(); let tree_oid = repo.treebuilder(None).unwrap().write().unwrap(); let tree = repo.find_tree(tree_oid).unwrap(); repo.commit(Some("refs/heads/main"), &sig, &sig, "initial", &tree, &[]).unwrap(); } repo } @@ -117,6 +125,11 @@ pub fn reopen_issue(repo: &Repository, ref_name: &str, author: &Author) { /// Create a patch using DAG primitives. Returns (ref_name, id). pub fn create_patch(repo: &Repository, author: &Author, title: &str) -> (String, String) { let sk = test_signing_key(); // Get branch tip for commit/tree fields let head = repo.head().unwrap(); let commit_oid = head.target().unwrap(); let commit = repo.find_commit(commit_oid).unwrap(); let tree_oid = commit.tree().unwrap().id(); let event = Event { timestamp: now(), author: author.clone(), @@ -126,6 +139,8 @@ pub fn create_patch(repo: &Repository, author: &Author, title: &str) -> (String, base_ref: "main".to_string(), branch: "test-branch".to_string(), fixes: None, commit: commit_oid.to_string(), tree: tree_oid.to_string(), }, clock: 0, }; @@ -145,6 +160,7 @@ pub fn add_review(repo: &Repository, ref_name: &str, author: &Author, verdict: R action: Action::PatchReview { verdict, body: "review comment".to_string(), revision: 1, }, clock: 0, }; diff --git a/tests/crdt_test.rs b/tests/crdt_test.rs index 426ef11..fb39350 100644 --- a/tests/crdt_test.rs +++ b/tests/crdt_test.rs @@ -361,6 +361,8 @@ fn concurrent_patch_close_merge_higher_clock_wins() { base_ref: "main".to_string(), branch: "test-branch".to_string(), fixes: None, commit: "dummy_commit".to_string(), tree: "dummy_tree".to_string(), }, clock: 0, }; diff --git a/tests/revision_test.rs b/tests/revision_test.rs new file mode 100644 index 0000000..6a6306d --- /dev/null +++ b/tests/revision_test.rs @@ -0,0 +1,767 @@ mod common; use common::TestRepo; use common::{alice, bob, init_repo, test_signing_key, now}; use git_collab::dag; use git_collab::event::{Action, Event}; use git_collab::state::PatchState; use tempfile::TempDir; // =========================================================================== // Revision recording on patch create // =========================================================================== #[test] fn test_patch_create_records_revision_1() { let repo = TestRepo::new("Alice", "alice@example.com"); let id = repo.patch_create("Feature X"); let out = repo.run_ok(&["patch", "show", &id, "--json"]); let json: serde_json::Value = serde_json::from_str(&out).unwrap(); let revisions = json["revisions"].as_array().unwrap(); assert_eq!(revisions.len(), 1); assert_eq!(revisions[0]["number"], 1); assert!(!revisions[0]["commit"].as_str().unwrap().is_empty()); assert!(!revisions[0]["tree"].as_str().unwrap().is_empty()); } // =========================================================================== // Auto-detect revision on comment // =========================================================================== #[test] fn test_auto_detect_revision_on_comment() { let repo = TestRepo::new("Alice", "alice@example.com"); // Create feature branch and patch repo.git(&["checkout", "-b", "feat-auto"]); repo.commit_file("v1.txt", "v1", "initial commit"); let out = repo.run_ok(&["patch", "create", "-t", "Auto-detect test", "-B", "feat-auto"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); // Push a new commit to the branch repo.git(&["checkout", "feat-auto"]); repo.commit_file("v2.txt", "v2", "second commit"); repo.git(&["checkout", "main"]); // Comment should auto-insert a PatchRevision first repo.run_ok(&["patch", "comment", &id, "-b", "Looks interesting"]); let out = repo.run_ok(&["patch", "show", &id, "--json"]); let json: serde_json::Value = serde_json::from_str(&out).unwrap(); let revisions = json["revisions"].as_array().unwrap(); assert_eq!(revisions.len(), 2, "should have 2 revisions (create + auto-detect)"); assert_eq!(revisions[0]["number"], 1); assert_eq!(revisions[1]["number"], 2); } #[test] fn test_no_revision_when_branch_unchanged() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-no-change"]); repo.commit_file("v1.txt", "v1", "initial commit"); let out = repo.run_ok(&["patch", "create", "-t", "No change test", "-B", "feat-no-change"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); // Comment WITHOUT branch change — no new revision repo.run_ok(&["patch", "comment", &id, "-b", "Just a thought"]); let out = repo.run_ok(&["patch", "show", &id, "--json"]); let json: serde_json::Value = serde_json::from_str(&out).unwrap(); let revisions = json["revisions"].as_array().unwrap(); assert_eq!(revisions.len(), 1, "should still have only 1 revision"); } // =========================================================================== // Auto-detect revision on review // =========================================================================== #[test] fn test_auto_detect_revision_on_review() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-review"]); repo.commit_file("v1.txt", "v1", "initial commit"); let out = repo.run_ok(&["patch", "create", "-t", "Review test", "-B", "feat-review"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); // Push a new commit repo.git(&["checkout", "feat-review"]); repo.commit_file("v2.txt", "v2", "second commit"); repo.git(&["checkout", "main"]); // Review should auto-insert a PatchRevision repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM"]); let out = repo.run_ok(&["patch", "show", &id, "--json"]); let json: serde_json::Value = serde_json::from_str(&out).unwrap(); let revisions = json["revisions"].as_array().unwrap(); assert_eq!(revisions.len(), 2); // Review should be anchored to revision 2 let reviews = json["reviews"].as_array().unwrap(); assert_eq!(reviews.len(), 1); assert_eq!(reviews[0]["revision"], 2); } // =========================================================================== // Explicit revise // =========================================================================== #[test] fn test_revise_creates_revision() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-revise"]); repo.commit_file("v1.txt", "v1", "initial commit"); let out = repo.run_ok(&["patch", "create", "-t", "Revise test", "-B", "feat-revise"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); // Push new commit repo.git(&["checkout", "feat-revise"]); repo.commit_file("v2.txt", "v2", "second commit"); repo.git(&["checkout", "main"]); repo.run_ok(&["patch", "revise", &id, "-b", "Addressed feedback"]); let out = repo.run_ok(&["patch", "show", &id, "--json"]); let json: serde_json::Value = serde_json::from_str(&out).unwrap(); let revisions = json["revisions"].as_array().unwrap(); assert_eq!(revisions.len(), 2); assert_eq!(revisions[1]["body"].as_str().unwrap(), "Addressed feedback"); } #[test] fn test_revise_rejects_when_unchanged() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-revise-err"]); repo.commit_file("v1.txt", "v1", "initial commit"); let out = repo.run_ok(&["patch", "create", "-t", "Revise error test", "-B", "feat-revise-err"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); // No new commit — revise should fail let err = repo.run_err(&["patch", "revise", &id]); assert!(err.contains("no changes since revision")); } // =========================================================================== // Inline comments anchored to revisions // =========================================================================== #[test] fn test_inline_comment_anchored_to_revision() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-inline"]); repo.commit_file("src/lib.rs", "fn hello() {}", "initial"); let out = repo.run_ok(&["patch", "create", "-t", "Inline test", "-B", "feat-inline"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); // Comment on revision 1 repo.run_ok(&[ "patch", "comment", &id, "-b", "nit: naming", "-f", "src/lib.rs", "-l", "1", ]); let out = repo.run_ok(&["patch", "show", &id, "--json"]); let json: serde_json::Value = serde_json::from_str(&out).unwrap(); let inline = json["inline_comments"].as_array().unwrap(); assert_eq!(inline.len(), 1); assert_eq!(inline[0]["revision"], 1); } #[test] fn test_inline_comment_explicit_revision() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-inline-rev"]); repo.commit_file("lib.rs", "fn a() {}", "v1"); let out = repo.run_ok(&["patch", "create", "-t", "Inline rev test", "-B", "feat-inline-rev"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); // Push v2 repo.git(&["checkout", "feat-inline-rev"]); repo.commit_file("lib.rs", "fn b() {}", "v2"); repo.git(&["checkout", "main"]); // Auto-detect by commenting repo.run_ok(&["patch", "comment", &id, "-b", "trigger revision"]); // Now explicitly target revision 1 repo.run_ok(&[ "patch", "comment", &id, "-b", "old nit", "-f", "lib.rs", "-l", "1", "--revision", "1", ]); let out = repo.run_ok(&["patch", "show", &id, "--json"]); let json: serde_json::Value = serde_json::from_str(&out).unwrap(); let inline = json["inline_comments"].as_array().unwrap(); assert_eq!(inline.len(), 1); assert_eq!(inline[0]["revision"], 1); } #[test] fn test_thread_comment_rejects_revision_flag() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-thread"]); repo.commit_file("x.txt", "x", "init"); let out = repo.run_ok(&["patch", "create", "-t", "Thread test", "-B", "feat-thread"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); let err = repo.run_err(&["patch", "comment", &id, "-b", "note", "--revision", "1"]); assert!(err.contains("thread comments are not revision-scoped")); } // =========================================================================== // patch show --revision N filters // =========================================================================== #[test] fn test_show_revision_filter() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-show-rev"]); repo.commit_file("a.txt", "a", "v1"); let out = repo.run_ok(&["patch", "create", "-t", "Show rev test", "-B", "feat-show-rev"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); // Comment on r1 repo.run_ok(&[ "patch", "comment", &id, "-b", "r1 comment", "-f", "a.txt", "-l", "1", ]); // Push v2 and comment on r2 repo.git(&["checkout", "feat-show-rev"]); repo.commit_file("b.txt", "b", "v2"); repo.git(&["checkout", "main"]); repo.run_ok(&[ "patch", "comment", &id, "-b", "r2 comment", "-f", "b.txt", "-l", "1", ]); // Show --revision 1: should show r1 comment, not r2 let out = repo.run_ok(&["patch", "show", &id, "--revision", "1"]); assert!(out.contains("r1 comment")); assert!(!out.contains("r2 comment")); // Show --revision 2: should show r2 comment, not r1 let out = repo.run_ok(&["patch", "show", &id, "--revision", "2"]); assert!(out.contains("r2 comment")); assert!(!out.contains("r1 comment")); } // =========================================================================== // Interdiff between revisions // =========================================================================== #[test] fn test_interdiff_between_revisions() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-interdiff"]); repo.commit_file("a.txt", "hello", "v1"); let out = repo.run_ok(&["patch", "create", "-t", "Interdiff test", "-B", "feat-interdiff"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); // Push v2 repo.git(&["checkout", "feat-interdiff"]); repo.commit_file("b.txt", "world", "v2"); repo.git(&["checkout", "main"]); // Create revision 2 via revise repo.run_ok(&["patch", "revise", &id]); // Interdiff between r1 and r2 should show b.txt added let out = repo.run_ok(&["patch", "diff", &id, "--between", "1", "2"]); assert!(out.contains("b.txt")); } #[test] fn test_interdiff_single_arg_means_n_to_latest() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-between-single"]); repo.commit_file("a.txt", "hello", "v1"); let out = repo.run_ok(&["patch", "create", "-t", "Between single test", "-B", "feat-between-single"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); // Push v2 repo.git(&["checkout", "feat-between-single"]); repo.commit_file("c.txt", "new", "v2"); repo.git(&["checkout", "main"]); repo.run_ok(&["patch", "revise", &id]); // --between 1 (single arg) = 1..latest let out = repo.run_ok(&["patch", "diff", &id, "--between", "1"]); assert!(out.contains("c.txt")); } #[test] fn test_interdiff_nonexistent_revision_errors() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-bad-rev"]); repo.commit_file("a.txt", "a", "v1"); let out = repo.run_ok(&["patch", "create", "-t", "Bad rev test", "-B", "feat-bad-rev"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); let err = repo.run_err(&["patch", "diff", &id, "--between", "1", "2"]); assert!(err.contains("revision 2 not found")); } #[test] fn test_diff_revision_flag_shows_historical_diff() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-hist"]); repo.commit_file("a.txt", "hello", "v1"); let out = repo.run_ok(&["patch", "create", "-t", "Hist diff test", "-B", "feat-hist"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); // Push v2 (adds b.txt) repo.git(&["checkout", "feat-hist"]); repo.commit_file("b.txt", "world", "v2"); repo.git(&["checkout", "main"]); repo.run_ok(&["patch", "revise", &id]); // --revision 1 should show only a.txt let out = repo.run_ok(&["patch", "diff", &id, "--revision", "1"]); assert!(out.contains("a.txt")); assert!(!out.contains("b.txt")); } #[test] fn test_diff_revision_and_between_mutually_exclusive() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-mutex"]); repo.commit_file("a.txt", "a", "v1"); let out = repo.run_ok(&["patch", "create", "-t", "Mutex test", "-B", "feat-mutex"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); let err = repo.run_err(&["patch", "diff", &id, "--revision", "1", "--between", "1"]); assert!(err.contains("mutually exclusive")); } // =========================================================================== // Patch log // =========================================================================== #[test] fn test_patch_log() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-log"]); repo.commit_file("a.txt", "hello", "v1"); let out = repo.run_ok(&["patch", "create", "-t", "Log test", "-B", "feat-log"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); // Push v2 repo.git(&["checkout", "feat-log"]); repo.commit_file("b.txt", "world", "v2"); repo.git(&["checkout", "main"]); repo.run_ok(&["patch", "revise", &id, "-b", "added b.txt"]); // Push v3 repo.git(&["checkout", "feat-log"]); repo.commit_file("c.txt", "!", "v3"); repo.git(&["checkout", "main"]); repo.run_ok(&["patch", "revise", &id]); let out = repo.run_ok(&["patch", "log", &id]); assert!(out.contains("Revision 1")); assert!(out.contains("(initial)")); assert!(out.contains("Revision 2")); assert!(out.contains("added b.txt")); assert!(out.contains("Revision 3")); } #[test] fn test_patch_log_json() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-log-json"]); repo.commit_file("a.txt", "a", "v1"); let out = repo.run_ok(&["patch", "create", "-t", "Log JSON test", "-B", "feat-log-json"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); let out = repo.run_ok(&["patch", "log", &id, "--json"]); let json: serde_json::Value = serde_json::from_str(&out).unwrap(); let revisions = json.as_array().unwrap(); assert_eq!(revisions.len(), 1); assert_eq!(revisions[0]["number"], 1); } // =========================================================================== // Reviews anchored to revisions // =========================================================================== #[test] fn test_review_explicit_revision() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-rev-explicit"]); repo.commit_file("a.txt", "a", "v1"); let out = repo.run_ok(&["patch", "create", "-t", "Rev review", "-B", "feat-rev-explicit"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); // Push v2 repo.git(&["checkout", "feat-rev-explicit"]); repo.commit_file("b.txt", "b", "v2"); repo.git(&["checkout", "main"]); repo.run_ok(&["patch", "revise", &id]); // Review targeting revision 1 repo.run_ok(&[ "patch", "review", &id, "-v", "request-changes", "-b", "fix r1", "--revision", "1", ]); let out = repo.run_ok(&["patch", "show", &id, "--json"]); let json: serde_json::Value = serde_json::from_str(&out).unwrap(); let reviews = json["reviews"].as_array().unwrap(); assert_eq!(reviews.len(), 1); assert_eq!(reviews[0]["revision"], 1); } #[test] fn test_review_shows_revision_context_in_show() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-rev-show"]); repo.commit_file("a.txt", "a", "v1"); let out = repo.run_ok(&["patch", "create", "-t", "Review show", "-B", "feat-rev-show"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM"]); let out = repo.run_ok(&["patch", "show", &id]); assert!(out.contains("(revision 1)")); } // =========================================================================== // Dedup by commit OID // =========================================================================== #[test] fn test_revision_dedup_by_commit_oid() { // Two comments with no branch change should not create duplicate revisions let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-dedup"]); repo.commit_file("a.txt", "a", "v1"); let out = repo.run_ok(&["patch", "create", "-t", "Dedup test", "-B", "feat-dedup"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); // Push v2 repo.git(&["checkout", "feat-dedup"]); repo.commit_file("b.txt", "b", "v2"); repo.git(&["checkout", "main"]); // Two comments — first should auto-detect revision, second should not create another repo.run_ok(&["patch", "comment", &id, "-b", "first"]); repo.run_ok(&["patch", "comment", &id, "-b", "second"]); let out = repo.run_ok(&["patch", "show", &id, "--json"]); let json: serde_json::Value = serde_json::from_str(&out).unwrap(); let revisions = json["revisions"].as_array().unwrap(); assert_eq!(revisions.len(), 2, "should have exactly 2 revisions, not 3"); } // =========================================================================== // Revision count in show output // =========================================================================== #[test] fn test_show_displays_revision_count() { let repo = TestRepo::new("Alice", "alice@example.com"); repo.git(&["checkout", "-b", "feat-count"]); repo.commit_file("a.txt", "a", "v1"); let out = repo.run_ok(&["patch", "create", "-t", "Count test", "-B", "feat-count"]); let id = out.trim().strip_prefix("Created patch ").unwrap().to_string(); let out = repo.run_ok(&["patch", "show", &id]); assert!(out.contains("(r1)")); } // =========================================================================== // D1: Concurrent PatchRevision dedup after DAG reconciliation // =========================================================================== #[test] fn test_concurrent_revision_dedup_after_reconcile() { // Two collaborators independently detect the same branch change and // create PatchRevision events with the same commit OID. After DAG // reconciliation, only one revision boundary should exist. let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); // Create a branch with a commit let head = repo.head().unwrap().target().unwrap(); let head_commit = repo.find_commit(head).unwrap(); repo.branch("feat-concurrent", &head_commit, false).unwrap(); let sk = test_signing_key(); // Create a patch (revision 1) let tree_oid = head_commit.tree().unwrap().id(); let create_event = Event { timestamp: now(), author: alice(), action: Action::PatchCreate { title: "Concurrent test".to_string(), body: "".to_string(), base_ref: "main".to_string(), branch: "feat-concurrent".to_string(), fixes: None, commit: head.to_string(), tree: tree_oid.to_string(), }, clock: 0, }; let patch_oid = dag::create_root_event(&repo, &create_event, &sk).unwrap(); let id = patch_oid.to_string(); let ref_name = format!("refs/collab/patches/{}", id); repo.reference(&ref_name, patch_oid, false, "test").unwrap(); // Push a new commit on the branch let sig = git2::Signature::now("Alice", "alice@example.com").unwrap(); let blob = repo.blob(b"new content").unwrap(); let mut tb = repo.treebuilder(Some(&head_commit.tree().unwrap())).unwrap(); tb.insert("new.txt", blob, 0o100644).unwrap(); let new_tree_oid = tb.write().unwrap(); let new_tree = repo.find_tree(new_tree_oid).unwrap(); let new_commit = repo.commit( Some("refs/heads/feat-concurrent"), &sig, &sig, "new commit", &new_tree, &[&head_commit], ).unwrap(); let root_tip = repo.refname_to_id(&ref_name).unwrap(); // Alice appends a PatchRevision for the new commit let rev_event = Event { timestamp: now(), author: alice(), action: Action::PatchRevision { commit: new_commit.to_string(), tree: new_tree_oid.to_string(), body: None, }, clock: 0, }; dag::append_event(&repo, &ref_name, &rev_event, &sk).unwrap(); let alice_tip = repo.refname_to_id(&ref_name).unwrap(); // Reset ref back to root, then Bob also appends same PatchRevision repo.reference(&ref_name, root_tip, true, "reset for bob").unwrap(); let rev_event_bob = Event { timestamp: now(), author: bob(), action: Action::PatchRevision { commit: new_commit.to_string(), tree: new_tree_oid.to_string(), body: None, }, clock: 0, }; dag::append_event(&repo, &ref_name, &rev_event_bob, &sk).unwrap(); let bob_tip = repo.refname_to_id(&ref_name).unwrap(); // Reconcile: create a remote ref pointing to bob's tip, local to alice's tip let remote_ref = format!("refs/collab/sync/origin/patches/{}", id); repo.reference(&remote_ref, bob_tip, true, "remote").unwrap(); repo.reference(&ref_name, alice_tip, true, "restore alice").unwrap(); dag::reconcile(&repo, &ref_name, &remote_ref, &alice(), &sk).unwrap(); // Materialize and verify: should have exactly 2 revisions (create + 1 deduped) let state = PatchState::from_ref(&repo, &ref_name, &id).unwrap(); assert_eq!( state.revisions.len(), 2, "duplicate PatchRevision events with same commit OID should be deduped to one revision" ); assert_eq!(state.revisions[0].number, 1); assert_eq!(state.revisions[1].number, 2); assert_eq!(state.revisions[1].commit, new_commit.to_string()); } // =========================================================================== // D2: Merge policy enforcement (require_approval_on_latest) // =========================================================================== #[test] fn test_merge_policy_require_approval_on_latest() { let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); let sk = test_signing_key(); // Create a branch with a commit let head = repo.head().unwrap().target().unwrap(); let head_commit = repo.find_commit(head).unwrap(); repo.branch("feat-policy", &head_commit, false).unwrap(); // Add a commit on the branch so it differs from main let sig = git2::Signature::now("Alice", "alice@example.com").unwrap(); let blob = repo.blob(b"feature code").unwrap(); let mut tb = repo.treebuilder(Some(&head_commit.tree().unwrap())).unwrap(); tb.insert("feature.txt", blob, 0o100644).unwrap(); let feat_tree_oid = tb.write().unwrap(); let feat_tree = repo.find_tree(feat_tree_oid).unwrap(); let feat_commit = repo.commit( Some("refs/heads/feat-policy"), &sig, &sig, "add feature", &feat_tree, &[&head_commit], ).unwrap(); // Create patch (revision 1) let create_event = Event { timestamp: now(), author: alice(), action: Action::PatchCreate { title: "Policy test".to_string(), body: "".to_string(), base_ref: "main".to_string(), branch: "feat-policy".to_string(), fixes: None, commit: feat_commit.to_string(), tree: feat_tree_oid.to_string(), }, clock: 0, }; let patch_oid = dag::create_root_event(&repo, &create_event, &sk).unwrap(); let patch_id = patch_oid.to_string(); let patch_ref = format!("refs/collab/patches/{}", patch_id); repo.reference(&patch_ref, patch_oid, false, "test").unwrap(); // Approve on revision 1 let review_event = Event { timestamp: now(), author: bob(), action: Action::PatchReview { verdict: git_collab::event::ReviewVerdict::Approve, body: "LGTM".to_string(), revision: 1, }, clock: 0, }; dag::append_event(&repo, &patch_ref, &review_event, &sk).unwrap(); // Push revision 2 (new commit on branch) let blob2 = repo.blob(b"v2 code").unwrap(); let parent = repo.find_commit(feat_commit).unwrap(); let mut tb2 = repo.treebuilder(Some(&parent.tree().unwrap())).unwrap(); tb2.insert("v2.txt", blob2, 0o100644).unwrap(); let v2_tree_oid = tb2.write().unwrap(); let v2_tree = repo.find_tree(v2_tree_oid).unwrap(); let v2_commit = repo.commit( Some("refs/heads/feat-policy"), &sig, &sig, "v2", &v2_tree, &[&parent], ).unwrap(); // Record revision 2 let rev_event = Event { timestamp: now(), author: alice(), action: Action::PatchRevision { commit: v2_commit.to_string(), tree: v2_tree_oid.to_string(), body: None, }, clock: 0, }; dag::append_event(&repo, &patch_ref, &rev_event, &sk).unwrap(); // Write merge policy config: require_approval_on_latest = true let config_json = br#"{"merge":{"require_approval_on_latest":true}}"#; let config_blob = repo.blob(config_json).unwrap(); let mut ctb = repo.treebuilder(None).unwrap(); ctb.insert("config.json", config_blob, 0o100644).unwrap(); let config_tree_oid = ctb.write().unwrap(); let config_tree = repo.find_tree(config_tree_oid).unwrap(); repo.commit( Some("refs/collab/config"), &sig, &sig, "set merge policy", &config_tree, &[], ).unwrap(); // Try to merge via the library — should fail because approval is on revision 1, not 2 let result = git_collab::patch::merge(&repo, &patch_id[..8]); assert!(result.is_err(), "merge should fail when approval is not on latest revision"); let err_msg = result.unwrap_err().to_string(); assert!( err_msg.contains("merge requires approval on the latest revision"), "error message should mention latest revision requirement, got: {}", err_msg ); } #[test] fn test_merge_policy_passes_when_approved_on_latest() { let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); let sk = test_signing_key(); // Create a branch with a commit let head = repo.head().unwrap().target().unwrap(); let head_commit = repo.find_commit(head).unwrap(); repo.branch("feat-policy-ok", &head_commit, false).unwrap(); let sig = git2::Signature::now("Alice", "alice@example.com").unwrap(); let blob = repo.blob(b"feature code").unwrap(); let mut tb = repo.treebuilder(Some(&head_commit.tree().unwrap())).unwrap(); tb.insert("feature.txt", blob, 0o100644).unwrap(); let feat_tree_oid = tb.write().unwrap(); let feat_tree = repo.find_tree(feat_tree_oid).unwrap(); let feat_commit = repo.commit( Some("refs/heads/feat-policy-ok"), &sig, &sig, "add feature", &feat_tree, &[&head_commit], ).unwrap(); // Create patch let create_event = Event { timestamp: now(), author: alice(), action: Action::PatchCreate { title: "Policy pass test".to_string(), body: "".to_string(), base_ref: "main".to_string(), branch: "feat-policy-ok".to_string(), fixes: None, commit: feat_commit.to_string(), tree: feat_tree_oid.to_string(), }, clock: 0, }; let patch_oid = dag::create_root_event(&repo, &create_event, &sk).unwrap(); let patch_id = patch_oid.to_string(); let patch_ref = format!("refs/collab/patches/{}", patch_id); repo.reference(&patch_ref, patch_oid, false, "test").unwrap(); // Approve on revision 1 (which IS the latest) let review_event = Event { timestamp: now(), author: bob(), action: Action::PatchReview { verdict: git_collab::event::ReviewVerdict::Approve, body: "LGTM".to_string(), revision: 1, }, clock: 0, }; dag::append_event(&repo, &patch_ref, &review_event, &sk).unwrap(); // Write merge policy config let config_json = br#"{"merge":{"require_approval_on_latest":true}}"#; let config_blob = repo.blob(config_json).unwrap(); let mut ctb = repo.treebuilder(None).unwrap(); ctb.insert("config.json", config_blob, 0o100644).unwrap(); let config_tree_oid = ctb.write().unwrap(); let config_tree = repo.find_tree(config_tree_oid).unwrap(); repo.commit( Some("refs/collab/config"), &sig, &sig, "set merge policy", &config_tree, &[], ).unwrap(); // Merge should succeed — approval is on latest revision (1) let result = git_collab::patch::merge(&repo, &patch_id[..8]); assert!(result.is_ok(), "merge should succeed when approval is on latest revision: {:?}", result.err()); } diff --git a/tests/signing_test.rs b/tests/signing_test.rs index 6e440b9..cc6b452 100644 --- a/tests/signing_test.rs +++ b/tests/signing_test.rs @@ -210,6 +210,8 @@ fn event_json_uses_namespaced_action_types() { base_ref: "main".to_string(), branch: "feature/fix-bug".to_string(), fixes: Some("deadbeef".to_string()), commit: "dummy_commit".to_string(), tree: "dummy_tree".to_string(), }, clock: 0, }; diff --git a/tests/sort_test.rs b/tests/sort_test.rs index 29f148b..ca4f800 100644 --- a/tests/sort_test.rs +++ b/tests/sort_test.rs @@ -87,6 +87,8 @@ fn create_patch_at( base_ref: "main".to_string(), branch: format!("branch-{}", title.replace(' ', "-")), fixes: None, commit: "dummy_commit".to_string(), tree: "dummy_tree".to_string(), }, clock: 0, }; diff --git a/tests/sync_test.rs b/tests/sync_test.rs index ba2715c..ced4c59 100644 --- a/tests/sync_test.rs +++ b/tests/sync_test.rs @@ -291,6 +291,8 @@ fn test_patch_review_across_repos() { base_ref: "main".to_string(), branch: "feature/x".to_string(), fixes: None, commit: "dummy_commit".to_string(), tree: "dummy_tree".to_string(), }, clock: 0, }; @@ -312,6 +314,7 @@ fn test_patch_review_across_repos() { action: Action::PatchReview { verdict: ReviewVerdict::Approve, body: "LGTM!".to_string(), revision: 1, }, clock: 0, }; @@ -340,6 +343,8 @@ fn test_concurrent_review_and_revise() { base_ref: "main".to_string(), branch: "feature/wip".to_string(), fixes: None, commit: "dummy_commit".to_string(), tree: "dummy_tree".to_string(), }, clock: 0, }; @@ -356,7 +361,9 @@ fn test_concurrent_review_and_revise() { let revise_event = Event { timestamp: now(), author: alice(), action: Action::PatchRevise { action: Action::PatchRevision { commit: "dummy_commit".to_string(), tree: "dummy_tree".to_string(), body: Some("Updated description".to_string()), }, clock: 0, @@ -370,6 +377,7 @@ fn test_concurrent_review_and_revise() { action: Action::PatchReview { verdict: ReviewVerdict::RequestChanges, body: "Needs work".to_string(), revision: 1, }, clock: 0, }; @@ -457,15 +465,6 @@ fn test_unsigned_event_sync_rejected() { // Set up Alice's repo with an unsigned event directly via git2 let alice_dir = TempDir::new().unwrap(); let alice_repo = common::init_repo(alice_dir.path(), &alice()); // Create initial commit so repo is not empty { let sig = git2::Signature::now("Alice", "alice@example.com").unwrap(); let tree_oid = alice_repo.treebuilder(None).unwrap().write().unwrap(); let tree = alice_repo.find_tree(tree_oid).unwrap(); alice_repo .commit(Some("refs/heads/main"), &sig, &sig, "init", &tree, &[]) .unwrap(); } // Create an unsigned event let event = Event { @@ -507,14 +506,6 @@ fn test_tampered_event_sync_rejected() { // Set up a repo with a tampered event let dir = TempDir::new().unwrap(); let repo = common::init_repo(dir.path(), &alice()); // Create initial commit { let sig = git2::Signature::now("Alice", "alice@example.com").unwrap(); let tree_oid = repo.treebuilder(None).unwrap().write().unwrap(); let tree = repo.find_tree(tree_oid).unwrap(); repo.commit(Some("refs/heads/main"), &sig, &sig, "init", &tree, &[]) .unwrap(); } // Create a tampered event (signed then modified) let event = Event {