53884c09
Merge patch-as-branch refactor into 012-patch-branch-refactor
a73x 2026-03-21 10:44
Resolves conflicts preserving all existing features (signing, import, editor, commit browser, dashboard tests) while integrating branch-based patch model. Ports branch resolution logic into lib.rs dispatch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
diff --git a/CLAUDE.md b/CLAUDE.md index 98a95ff..aa10e98 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -7,6 +7,7 @@ Auto-generated from all feature plans. Last updated: 2026-03-21 - Git refs under `.git/refs/collab/`, trusted keys file at `.git/collab/trusted-keys` (plain text, not a git object) (003-key-trust-allowlist) - 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) - 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) @@ -26,10 +27,10 @@ cargo test [ONLY COMMANDS FOR ACTIVE TECHNOLOGIES][ONLY COMMANDS FOR ACTIVE TECH Rust 2021 edition: Follow standard conventions ## Recent Changes - 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 - 001-gpg-event-signing: Added Rust 2021 edition + git2 0.19, clap 4, serde/serde_json 1, chrono 0.4, thiserror 2. New: `ed25519-dalek`, `rand`, `base64` <!-- MANUAL ADDITIONS START --> <!-- MANUAL ADDITIONS END --> diff --git a/specs/012-patch-branch-refactor/checklists/requirements.md b/specs/012-patch-branch-refactor/checklists/requirements.md new file mode 100644 index 0000000..26943ca --- /dev/null +++ b/specs/012-patch-branch-refactor/checklists/requirements.md @@ -0,0 +1,34 @@ # Specification Quality Checklist: Patch-as-Branch Refactor **Purpose**: Validate specification completeness and quality before proceeding to planning **Created**: 2026-03-21 **Feature**: [spec.md](../spec.md) ## Content Quality - [x] No implementation details (languages, frameworks, APIs) - [x] Focused on user value and business needs - [x] Written for non-technical stakeholders - [x] All mandatory sections completed ## Requirement Completeness - [x] No [NEEDS CLARIFICATION] markers remain - [x] Requirements are testable and unambiguous - [x] Success criteria are measurable - [x] Success criteria are technology-agnostic (no implementation details) - [x] All acceptance scenarios are defined - [x] Edge cases are identified - [x] Scope is clearly bounded - [x] Dependencies and assumptions identified ## Feature Readiness - [x] All functional requirements have clear acceptance criteria - [x] User scenarios cover primary flows - [x] Feature meets measurable outcomes defined in Success Criteria - [x] No implementation details leak into specification ## Notes - All items pass. Spec is ready for `/speckit.plan`. diff --git a/specs/012-patch-branch-refactor/contracts/cli-commands.md b/specs/012-patch-branch-refactor/contracts/cli-commands.md new file mode 100644 index 0000000..1cf15ab --- /dev/null +++ b/specs/012-patch-branch-refactor/contracts/cli-commands.md @@ -0,0 +1,75 @@ # CLI Contract Changes: Patch-as-Branch Refactor ## Modified Commands ### `git collab patch create` **Before**: ``` git collab patch create --title "Fix bug" --base main --head abc123 ``` **After**: ``` git collab patch create --title "Fix bug" --base main --branch feature/fix-bug git collab patch create --title "Fix bug" --base main # defaults to current branch ``` **Arguments**: - `--title` / `-t`: Patch title (required) - `--body` / `-b`: Patch description (optional, default "") - `--base`: Target branch (optional, default "main") - `--branch` / `-B`: Source branch (optional, defaults to current branch) - `--head`: Deprecated alias for `--branch`, hidden from help - `--fixes`: Issue ID this patch fixes (optional) **Errors**: - Current branch is the same as base → "cannot create patch from base branch" - Active patch already exists for this branch → "patch already exists for branch '{name}'" - Detached HEAD with no --branch → auto-creates branch `collab/patch/{short-oid}` ### `git collab patch show <id>` **Output changes**: ``` Patch a89c37fa [open] Title: Fix the bug Author: dev <dev@example.com> Branch: feature/fix-bug → main Commits: 3 ahead, 0 behind (up-to-date) Created: 2026-03-21T09:00:00+00:00 ``` When outdated: ``` Commits: 3 ahead, 5 behind (outdated) ``` When branch deleted: ``` Branch: feature/fix-bug (not found) ``` ### `git collab patch diff <id>` **No argument changes**. Behavior change: - Branch-based patches: shows three-dot diff (merge-base...branch) - Old patches: unchanged behavior ### `git collab patch merge <id>` **No argument changes**. Behavior change: - Branch-based patches: merges branch into base (standard git merge) - Old patches: unchanged behavior ### `git collab patch revise <id>` **Behavior change**: Optional for branch-based patches. Can still be used to record a revision note. The `--head` flag is ignored for branch-based patches (branch tip is always used). ## Unchanged Commands - `git collab patch list` - `git collab patch comment` - `git collab patch review` - `git collab patch close` - `git collab patch import` (creates a branch, then creates a branch-based patch) diff --git a/specs/012-patch-branch-refactor/data-model.md b/specs/012-patch-branch-refactor/data-model.md new file mode 100644 index 0000000..1a8cacb --- /dev/null +++ b/specs/012-patch-branch-refactor/data-model.md @@ -0,0 +1,63 @@ # Data Model: Patch-as-Branch Refactor ## Entity Changes ### PatchCreate Action (Modified) Current fields: - `title: String` - `body: String` - `base_ref: String` — target branch for merge (e.g., "main") - `head_commit: String` — snapshot OID of the reviewed commit - `fixes: Option<String>` — linked issue ID New fields: - `title: String` — unchanged - `body: String` — unchanged - `base_ref: String` — unchanged - `head_commit: String` — **kept for backward compat**, stores branch tip at creation time - `branch: Option<String>` — **NEW**: the branch name (e.g., "feature/foo"). When present, this is the source of truth. - `fixes: Option<String>` — unchanged ### PatchState (Modified) Current fields remain. Add: - `branch: Option<String>` — the branch name, populated from PatchCreate event. `None` for old-format patches. The `head_commit` field becomes computed for branch-based patches: - If `branch` is `Some`, resolve `refs/heads/{branch}` to get current tip - If `branch` is `None`, use stored `head_commit` (old behavior) ### PatchRevise Action (Deprecated) - Kept for backward compat but no longer required for branch-based patches - Branch-based patches auto-track the branch tip - Can still be used to record explicit revision notes in the DAG ### Staleness Info (New, Computed) Not stored — computed on demand from git: - `ahead: usize` — commits on branch not on base - `behind: usize` — commits on base not on branch - `is_merged: bool` — whether branch commits are ancestors of base tip - `has_conflicts: bool` — whether merge would conflict (optional, expensive) ## State Transitions ``` PatchCreate (with branch) → Open ↓ (branch updated externally) → still Open, diff changes automatically ↓ PatchReview → still Open (reviews attached) ↓ PatchMerge → Merged (git merge performed) ↓ PatchClose → Closed ↓ (branch deleted) → Open but "branch not found" error on diff/merge ``` ## Ref Structure (Unchanged) ``` refs/collab/patches/{id} → review DAG (comments, reviews, events) refs/heads/{branch} → actual code (normal git branch) ``` The review DAG and the code branch are separate refs. The DAG references the branch by name (string), not by OID. diff --git a/specs/012-patch-branch-refactor/plan.md b/specs/012-patch-branch-refactor/plan.md new file mode 100644 index 0000000..a7f3358 --- /dev/null +++ b/specs/012-patch-branch-refactor/plan.md @@ -0,0 +1,80 @@ # Implementation Plan: Patch-as-Branch Refactor **Branch**: `012-patch-branch-refactor` | **Date**: 2026-03-21 | **Spec**: [spec.md](spec.md) ## Summary Refactor patches from storing commit OID snapshots to referencing git branches. The review DAG (comments, reviews, inline comments) stays as-is. The code changes are tracked by the branch itself. This aligns git-collab with how git natively works and with Radicle's proven model of separating review from code. ## Technical Context **Language/Version**: Rust 2021 edition **Primary Dependencies**: git2 0.19, clap 4 (derive), serde/serde_json 1, chrono 0.4, ed25519-dalek 2 **Testing**: cargo test **Project Type**: CLI tool with TUI dashboard ## Design Decisions ### 1. Event Schema Change Add optional `branch: Option<String>` to `Action::PatchCreate` in `src/event.rs`. Serde's `skip_serializing_if = "Option::is_none"` ensures old events without this field deserialize correctly. The `head_commit` field is kept — for new patches it stores the branch tip at creation time (for reference), for old patches it remains the source of truth. ### 2. PatchState Branch Resolution In `src/state.rs`, `PatchState` gains a `branch: Option<String>` field. When building state from the DAG: - If `branch` is `Some`, the `head_commit` is resolved live from `refs/heads/{branch}` via `repo.refname_to_id()` - If `branch` is `None`, use the stored `head_commit` (backward compat) New computed method `PatchState::staleness(repo) -> (ahead, behind)` using `repo.graph_ahead_behind()`. ### 3. CLI Changes In `src/cli.rs`, `PatchCmd::Create`: - `--head` becomes `--branch` (short: `-B`). Defaults to current branch name (from `repo.head()`). - `--head` is kept as hidden/deprecated alias for backward compat. - If `--branch` points to a detached HEAD or commit OID, auto-create a branch `collab/patch/{short-oid}`. ### 4. Diff via Merge-Base In `src/patch.rs`, `generate_diff()` changes: - For branch-based patches: compute merge-base between base and branch tips, diff merge-base tree against branch tip tree (three-dot semantics) - For old patches: keep current behavior (direct tree diff) ### 5. Merge via Branch In `src/patch.rs`, `merge()` changes: - For branch-based patches: resolve branch tip live, merge the branch into base (same git merge, just different head resolution) - For old patches: keep current behavior ### 6. Revise Becomes Optional `patch revise` still works but is no longer required. For branch-based patches, pushing to the branch IS the revision. The `revise` command can optionally record a note in the DAG. ### 7. Duplicate Patch Prevention Before creating a new patch, scan all open patches to check if any already reference the same branch. Error if found. ### 8. TUI Updates In `src/tui.rs`, patch detail view: - Show branch name instead of (or alongside) head commit - Show staleness info: "3 commits ahead, 2 behind main" - Show "branch not found" if branch was deleted ## Source Code ```text src/ ├── event.rs # Add optional `branch` field to PatchCreate ├── state.rs # PatchState: add branch field, live head resolution, staleness ├── cli.rs # Change --head to --branch, default to current branch ├── patch.rs # Update create/merge/diff/revise to use branch resolution ├── lib.rs # Update dispatch for new CLI args ├── tui.rs # Show branch name and staleness in patch detail ``` ## Migration Strategy 1. **No data migration needed** — old events without `branch` field deserialize with `branch: None`, triggering old-format code paths 2. **New patches automatically use branch model** — `patch create` always populates `branch` 3. **Old patches work indefinitely** — the `head_commit` fallback is permanent, not a temporary shim diff --git a/specs/012-patch-branch-refactor/research.md b/specs/012-patch-branch-refactor/research.md new file mode 100644 index 0000000..085e8f2 --- /dev/null +++ b/specs/012-patch-branch-refactor/research.md @@ -0,0 +1,60 @@ # Research: Patch-as-Branch Refactor ## Decision 1: Branch Name Storage in PatchCreate Event **Decision**: Add an optional `branch` field to `Action::PatchCreate`. When present, it replaces `head_commit` as the source of truth. When absent (old-format patches), fall back to `head_commit`. **Rationale**: Adding an optional field maintains backward compatibility — old events deserialize without the field, new events include it. The `head_commit` field is kept but becomes redundant for new patches (it can store the branch tip at creation time for reference). **Alternatives considered**: - Replace `head_commit` entirely → breaks backward compat - Store branch name outside the DAG (e.g., in a separate ref) → adds complexity, loses atomicity - Use a new Action variant `PatchCreateV2` → awkward, fragments the event model ## Decision 2: How to Resolve Branch to Current Head **Decision**: Use `repo.refname_to_id(&format!("refs/heads/{}", branch))` to resolve the branch to its current tip commit. This is always live — never stale. **Rationale**: This is the fundamental shift. Instead of storing a snapshot OID, we look up the branch ref every time. If the developer pushes new commits, the next `patch show` automatically reflects them. **Alternatives considered**: - Cache the head OID → defeats the purpose of live branch tracking - Use `revparse_single` on the branch name → works but `refname_to_id` is more explicit ## Decision 3: Staleness Detection **Decision**: Use `repo.merge_base(base_tip, branch_tip)` to find the common ancestor, then `repo.graph_ahead_behind(branch_tip, base_tip)` to get (ahead, behind) counts. **Rationale**: git2 provides `graph_ahead_behind` which returns exactly what we need — how many commits the branch is ahead (patch size) and how many the base has moved (staleness). No need to shell out to `git rev-list`. **Alternatives considered**: - Shell out to `git rev-list --count` → unnecessary, git2 has the API - Walk the revwalk manually → `graph_ahead_behind` does this already ## Decision 4: Diff Generation **Decision**: Use three-dot diff semantics — diff between merge-base and branch tip. This shows only the changes introduced by the branch, not changes on the base since the branch diverged. **Rationale**: This matches `git diff base...branch` which is what reviewers expect. The existing `generate_diff` already does tree-to-tree diff; it just needs to use merge-base instead of base tip directly. **Alternatives considered**: - Two-dot diff (base tip vs branch tip) → includes base changes, confusing for review - Show both → over-complicated for the common case ## Decision 5: Merge Strategy **Decision**: Use `repo.merge_analysis()` + `repo.merge()` from git2 for the actual merge, matching standard `git merge` behavior. Already partially implemented in the existing `patch::merge()`. **Rationale**: The existing merge function already does most of this — it resolves the head commit, does analysis, and merges. The change is just where the head commit comes from (branch ref instead of stored OID). ## Decision 6: CLI Changes **Decision**: Change `--head` to `--branch` (or `--head` becomes optional, defaulting to current branch name). The `--base` flag stays as-is. **Rationale**: The mental model shifts from "submit this commit for review" to "submit this branch for review". The flag name should reflect this. ## Decision 7: Radicle-Inspired Patterns **Decision**: Adopt Radicle's principle of separation — branches are pure git, the collab DAG is pure review. Don't try to store code state in the DAG. **Rationale**: Radicle stores patches as COBs (collaborative objects) that reference branches, not commits. Their experience shows this model scales well for peer-to-peer collaboration. Our simpler append-only DAG achieves the same separation. diff --git a/specs/012-patch-branch-refactor/review.md b/specs/012-patch-branch-refactor/review.md new file mode 100644 index 0000000..749e8ca --- /dev/null +++ b/specs/012-patch-branch-refactor/review.md @@ -0,0 +1,114 @@ # Pre-Implementation Review: 012-patch-branch-refactor **Reviewer**: Cross-model review (Claude Opus 4.6) **Date**: 2026-03-21 **Verdict**: APPROVED with minor findings ## Summary Table | Check | Status | Notes | |-------------------------------|--------|--------------------------------------------------------------| | Spec-Plan Alignment | PASS | All 5 user stories addressed in plan | | Plan-Tasks Completeness | PASS | 38 tasks cover all plan items | | Dependency Ordering | PASS | Phases correctly ordered; parallel opportunities valid | | `graph_ahead_behind()` exists | PASS | Confirmed in git2 0.19 Repository docs (2 references found) | | `refname_to_id()` works | PASS | Already used 11+ times across codebase | | Serde optional field compat | PASS | Pattern already used for `fixes: Option<String>` on PatchCreate | | CLI `--head` to `--branch` | PASS | clap derive supports hidden deprecated aliases | | Breaking changes risk | WARN | See finding F3 below | | Migration issues | PASS | No migration needed; `Option` deserialization handles it | | TDD coverage | WARN | See finding F5 below | ## Detailed Findings ### F1: Serde Backward Compat Pattern Already Proven (PASS) The plan proposes adding `branch: Option<String>` with `#[serde(default, skip_serializing_if = "Option::is_none")]`. This exact pattern is already used in the codebase for the `fixes` field on `Action::PatchCreate` (event.rs:51). Deserialization of old events without the field will work correctly, producing `None`. ### F2: git2 API Feasibility Confirmed (PASS) - **`repo.graph_ahead_behind(oid1, oid2)`**: Present in git2 0.19. Returns `(usize, usize)` for (ahead, behind). Confirmed in generated docs. - **`repo.refname_to_id()`**: Already used extensively in `patch.rs`, `dag.rs`, `sync.rs`, `signing.rs`, and `issue.rs`. Well-tested pattern. - **`repo.merge_base()`**: Already used in `dag.rs` for sync reconciliation. ### F3: CLI Breaking Change Risk (WARN - Low) The plan changes `--head` to `--branch` with `--head` kept as a hidden alias. Current code in `cli.rs:169` shows `--head` is `Option<String>` defaulting to HEAD when `None`. The plan's new `--branch` flag has the same default behavior (current branch from `repo.head()`). **Risk**: The `--head` flag currently accepts a commit OID (e.g., `--head abc123`). The new `--branch` flag accepts a branch name. If users pass OIDs to `--branch`, the auto-create-branch-from-detached-HEAD logic in T017 must handle this case. The plan mentions this (spec line 23: "detached HEAD or commit OID"), but T017's description is vague on the OID-to-branch path. **Recommendation**: T017 should explicitly state: "if `--branch` value is not a valid ref but IS a valid OID, create branch `collab/patch/{short-oid}` pointing at that OID." ### F4: `patch revise` Interaction with Branch Model (PASS with note) The plan says `patch revise` becomes optional for branch-based patches (T028). Current `revise()` in `patch.rs:112-131` stores a new `head_commit` in a `PatchRevise` event, and `PatchState::from_ref()` (state.rs:216-223) updates `self.head_commit` from it. For branch-based patches, `resolve_head()` (T004) resolves from the branch ref, so `PatchRevise` events updating `head_commit` would be ignored (branch takes precedence). This is correct behavior, but worth documenting for developers: **revise events on branch-based patches only serve as notes, not as head updates**. ### F5: TDD Coverage Gaps (WARN - Medium) The task list has 14 test tasks (T007-T012, T019-T021, T024-T025, T027, T029-T031). Missing test coverage: 1. **No test for branch deletion edge case** (spec line 95-96, FR-009). The TUI task T035 handles display, but no test verifies `resolve_head()` returns a clear error when the branch is deleted. 2. **No test for `patch import` with branch model** (T036 implementation but no test task). Import currently creates a detached commit and passes the OID to `patch::create()`. After refactor, it needs to create a branch first. This is a behavior change that should be tested. 3. **No test for duplicate patch detection across branch renames**. If branch "foo" has a patch and the user renames it to "bar", then creates a patch for "bar", the duplicate check (T016) passes but the old patch for "foo" now has a dangling branch reference. **Recommendation**: Add T039 (test for deleted branch error), T040 (test for import creating a branch), and consider whether branch rename is an edge case worth covering. ### F6: `generate_diff()` Currently Does Two-Dot Diff (PASS with note) Current `generate_diff()` in `patch.rs:222-266` diffs the base branch tip tree against the head commit tree. This is two-dot semantics. The plan (T022) correctly identifies this needs to change to three-dot (merge-base to branch tip) for branch-based patches, while keeping old behavior for OID-based patches. The current code at `patch.rs:232-237` resolves the base via `refname_to_id`. The refactored version needs to first find the merge-base between base tip and branch tip, then diff merge-base tree against branch tip tree. This is straightforward with the existing git2 APIs. ### F7: Spec-Plan-Tasks Alignment Verified (PASS) | User Story | Spec Section | Plan Section | Tasks | Count | |-----------|-------------|-------------|-------|-------| | US1: Create from branch | Lines 10-24 | "CLI Changes" + "Duplicate Prevention" | T009-T018 | 10 | | US2: Staleness and diff | Lines 27-41 | "Diff via Merge-Base" | T019-T023 | 5 | | US3: Merge as git merge | Lines 44-58 | "Merge via Branch" | T024-T026 | 3 | | US4: Implicit revision | Lines 61-74 | "Revise Becomes Optional" | T027-T028 | 2 | | US5: Backward compat | Lines 77-90 | "Migration Strategy" | T029-T032 | 4 | | Cross-cutting | Edge cases | Plan sections 7-8 | T033-T038 | 6 | All functional requirements (FR-001 through FR-011) map to at least one task. All success criteria (SC-001 through SC-006) are covered by the test tasks. ### F8: Phase Dependency Analysis (PASS) The dependency graph is correct: ``` Phase 1 (schema) --> Phase 2 (resolution helpers) | +----------+----------+----------+ | | | | Phase 3 Phase 5 Phase 7 Phase 8 (US1) (US3) (US5) (TUI) | Phase 4 Phase 6 (US2) (US4) | Phase 9 (polish) ``` Phase 5 (US3/merge) is listed as depending on Phase 2, not Phase 3. This is correct because merge only needs `resolve_head()` from Phase 2, and the existing `merge()` function already works with OID-based patches. Phase 6 (US4) depends on Phase 3 because implicit revision requires branch-based patches to exist. This is correct. ### F9: Existing Merge Function Already Handles Non-FF (PASS) The existing `merge()` in `patch.rs:133-212` already handles fast-forward, same-commit, and non-fast-forward (merge commit) cases, including conflict detection. The refactor (T026) only needs to change how `head_commit` is resolved (branch ref vs stored OID). The merge logic itself remains unchanged. ## Recommendations 1. **Add missing test tasks** for branch deletion error handling (F5.1) and patch import with branches (F5.2) 2. **Clarify T017** to handle OID arguments passed to `--branch` (F3) 3. **Document in code** that `PatchRevise` events on branch-based patches are informational only (F4) 4. **Consider** adding a `patch checkout` command that checks out the branch associated with a patch -- natural UX complement to branch-based patches (not blocking, future enhancement) ## Conclusion The feature is well-designed and feasible. The existing codebase already uses all required git2 APIs, the serde backward-compat pattern is proven, and the migration strategy (no migration needed) is sound. The 38 tasks provide good coverage with 2-3 test gaps that should be addressed before implementation. The phased approach with MVP at Phase 3 is a good risk management strategy. diff --git a/specs/012-patch-branch-refactor/spec.md b/specs/012-patch-branch-refactor/spec.md new file mode 100644 index 0000000..ffd0b93 --- /dev/null +++ b/specs/012-patch-branch-refactor/spec.md @@ -0,0 +1,140 @@ # Feature Specification: Patch-as-Branch Refactor **Feature Branch**: `012-patch-branch-refactor` **Created**: 2026-03-21 **Status**: Draft **Input**: User description: "Refactor patches to be branches with a review layer. Currently patches store a head commit OID and base ref name in the DAG, duplicating what git branches already track. Instead, a patch should BE a branch — the patch DAG becomes purely the review layer on top of normal git branches." ## User Scenarios & Testing *(mandatory)* ### User Story 1 - Create a Patch from a Branch (Priority: P1) A developer has been working on a feature branch. They want to submit it for review. They run a create command that associates their existing branch with a review conversation. The system records the branch name and target base, and the review DAG is created. The branch itself is untouched — it's still a normal git branch they can push, rebase, and update as usual. **Why this priority**: This is the foundational change — without branch-based patch creation, nothing else works. **Independent Test**: Create a branch with commits, run patch create referencing that branch, verify the patch is listed and the branch is unmodified. **Acceptance Scenarios**: 1. **Given** a developer has a feature branch with commits ahead of main, **When** they create a patch referencing that branch, **Then** a review conversation is created and the patch appears in the patch list showing the branch name and base. 2. **Given** a developer creates a patch from a branch, **When** they view the patch, **Then** it shows the branch name, base ref, number of commits ahead, and whether it can be fast-forward merged. 3. **Given** a developer has no local branch (e.g., they have a detached HEAD or a commit OID), **When** they create a patch, **Then** the system creates a branch for them automatically and associates it with the review. 4. **Given** a patch already exists for a branch, **When** the developer tries to create another patch for the same branch, **Then** the system rejects it with an error (one active patch per branch). --- ### User Story 2 - View Patch Staleness and Diff via Git (Priority: P2) A reviewer wants to see the current state of a patch. The system uses git's own tools to show the diff between the branch and its base, detect how many commits the base has moved ahead, and whether the patch would merge cleanly. No custom diff logic — just git. **Why this priority**: Reviewers need accurate, up-to-date information to make decisions. Leveraging git ensures this is always correct. **Independent Test**: Create a patch, advance the base branch, then run patch show and verify it reports the patch as outdated with the correct commit count. **Acceptance Scenarios**: 1. **Given** a patch exists and the base branch has not moved, **When** the reviewer views the patch, **Then** it shows the patch is up-to-date. 2. **Given** a patch exists and the base branch has moved 3 commits ahead, **When** the reviewer views the patch, **Then** it shows "outdated: base is 3 commits ahead" or similar. 3. **Given** a patch exists, **When** the reviewer requests the diff, **Then** the system shows the diff between the branch and the merge-base with the target branch (the same as `git diff base...branch`). 4. **Given** a patch exists and the branch has been rebased or force-pushed, **When** the reviewer views the patch, **Then** the diff reflects the current state of the branch, not a stale snapshot. --- ### User Story 3 - Merge a Patch as a Normal Git Merge (Priority: P3) A maintainer approves a patch and wants to merge it. The system performs a standard git merge of the patch branch into the base branch — the same operation as `git merge <branch>`. After merging, the patch is marked as merged in the review DAG. **Why this priority**: Merge is the final step in the review lifecycle, and using git's native merge ensures compatibility with all existing git workflows. **Independent Test**: Create a patch, approve it, merge it, verify the branch is merged into the base and the patch status is updated. **Acceptance Scenarios**: 1. **Given** a patch is approved, **When** the maintainer merges it, **Then** the patch branch is merged into the base branch using a standard git merge. 2. **Given** a patch merge would result in conflicts, **When** the maintainer attempts to merge, **Then** the system reports the conflicts and does not complete the merge. 3. **Given** a patch is merged, **When** anyone views the patch, **Then** it shows status "merged" and the merge commit. 4. **Given** a patch is merged, **When** listing patches, **Then** it no longer appears in the default (open) list but is visible with the `--all` flag. --- ### User Story 4 - Revise a Patch by Updating the Branch (Priority: P4) A developer receives review feedback and pushes new commits to their branch (or rebases it). The system detects the branch has changed and the review conversation continues against the updated code. No explicit "revise" command is needed — the branch IS the patch, so updating the branch updates the patch. **Why this priority**: This simplifies the workflow significantly — developers just use normal git operations (commit, push, rebase) and the review stays attached. **Independent Test**: Create a patch, push a new commit to the branch, verify patch show reflects the new commit without any explicit revise command. **Acceptance Scenarios**: 1. **Given** a patch exists for a branch, **When** the developer pushes new commits to that branch, **Then** the patch diff and status automatically reflect the updated branch state. 2. **Given** a patch exists and the developer rebases the branch, **When** the reviewer views the patch, **Then** the diff shows the rebased state. 3. **Given** a patch has existing reviews, **When** the branch is updated, **Then** the existing review comments remain attached to the patch conversation. --- ### User Story 5 - Backward Compatibility with Existing Patches (Priority: P5) Existing patches that were created with the old model (storing head_commit OID) should continue to work. The system should be able to read and display old-format patches while all new patches use the branch-based model. **Why this priority**: Migration safety — existing data must not be broken. **Independent Test**: Load a repository with old-format patches, verify they still appear in patch list and can be viewed/commented on. **Acceptance Scenarios**: 1. **Given** a repository has patches created with the old format (head_commit OID), **When** listing patches, **Then** old patches appear alongside new branch-based patches. 2. **Given** an old-format patch exists, **When** viewing it, **Then** the system shows the stored head commit and diff as before. 3. **Given** an old-format patch exists, **When** attempting to merge it, **Then** the system uses the stored head commit for the merge (old behavior). --- ### Edge Cases - What happens when the branch referenced by a patch is deleted? The patch shows an error status ("branch not found") but the review conversation is preserved. - What happens when two patches target the same base branch? Both are allowed — they are independent branches with independent reviews. - What happens when a patch branch is merged outside of git-collab (e.g., via `git merge` directly)? The patch status should ideally detect this, but at minimum the review conversation is preserved. - What happens when the user runs `patch create` without specifying a branch? The system uses the current branch (HEAD) if it's not the base branch, or errors if HEAD is the base branch. - What happens with the existing `patch revise` command? It becomes optional/deprecated — updating the branch is the primary way to revise. The command can remain for explicitly recording a revision event in the DAG. - What happens with `patch import`? Imported patches create a branch from the imported commits, then attach a review DAG to that branch. ## Requirements *(mandatory)* ### Functional Requirements - **FR-001**: System MUST associate patches with git branches rather than storing commit OIDs in the review DAG. - **FR-002**: System MUST use git's native merge-base and rev-list to determine patch staleness relative to the base branch. - **FR-003**: System MUST compute patch diffs using git's diff between the branch and the merge-base with the target (three-dot diff). - **FR-004**: System MUST perform patch merges as standard git merges of the branch into the base. - **FR-005**: System MUST preserve the review conversation (comments, reviews, inline comments) independently of branch state changes. - **FR-006**: System MUST support reading and displaying patches created with the old format (head_commit OID). - **FR-007**: System MUST automatically reflect branch updates (new commits, rebases) in patch status and diff without requiring an explicit revise command. - **FR-008**: System MUST prevent creating multiple active patches for the same branch. - **FR-009**: System MUST detect and report when a patch branch has been deleted. - **FR-010**: System MUST default to the current branch when no branch is specified during patch creation. - **FR-011**: System MUST create a branch automatically when a patch is created from a detached HEAD or commit OID. ### Key Entities - **Patch**: A review conversation attached to a git branch. Contains a branch name, target base ref, and a DAG of review events. The branch itself holds the code changes. - **Review Event**: An entry in the review DAG — comments, reviews (with verdicts), inline comments. These are independent of the branch state and persist across rebases and force-pushes. ## Success Criteria *(mandatory)* ### Measurable Outcomes - **SC-001**: All existing patch workflows (create, list, show, comment, review, merge, close) continue to function with the new branch-based model. - **SC-002**: Patch diff always reflects the current state of the branch, not a stale snapshot. - **SC-003**: Patch staleness is accurately reported using git's own rev-list, showing exact commit counts. - **SC-004**: Existing old-format patches remain readable and functional after the refactor. - **SC-005**: Developers can update a patch by simply pushing to the branch — no explicit revise step required. - **SC-006**: Patch merge uses standard git merge, producing the same result as `git merge <branch>`. ## Assumptions - The branch name is stored in the PatchCreate event in the review DAG, replacing (or supplementing) the head_commit field. - The review DAG (collab refs) remains the same structure — only the PatchCreate and PatchRevise event payloads change. - Sync continues to work — branch refs are synced via normal git push/fetch, and collab refs are synced as before. - The `patch revise` command is not removed but becomes optional — it can be used to explicitly record a revision event with a message. - The `patch import` command creates a branch from imported commits before attaching the review DAG. diff --git a/specs/012-patch-branch-refactor/tasks.md b/specs/012-patch-branch-refactor/tasks.md new file mode 100644 index 0000000..e0a6664 --- /dev/null +++ b/specs/012-patch-branch-refactor/tasks.md @@ -0,0 +1,213 @@ # Tasks: Patch-as-Branch Refactor **Input**: Design documents from `/specs/012-patch-branch-refactor/` **Prerequisites**: plan.md, spec.md, data-model.md, contracts/cli-commands.md, research.md **Tests**: User prefers TDD — test tasks included. **Organization**: Tasks grouped by user story for independent implementation and testing. ## Format: `[ID] [P?] [Story] Description` - **[P]**: Can run in parallel (different files, no dependencies) - **[Story]**: Which user story this task belongs to (e.g., US1, US2, US3) ## Phase 1: Setup (Event Schema Change) **Purpose**: Add `branch` field to the event model — foundational for all user stories - [ ] T001 Add optional `branch: Option<String>` field with `#[serde(default, skip_serializing_if = "Option::is_none")]` to `Action::PatchCreate` in `src/event.rs` - [ ] T002 Add `branch: Option<String>` field to `PatchState` struct in `src/state.rs` - [ ] T003 Populate `PatchState.branch` from `PatchCreate` event's `branch` field in `PatchState::from_ref()` in `src/state.rs` **Checkpoint**: Existing tests pass — the new optional field doesn't break deserialization of old events --- ## Phase 2: Foundational (Branch Resolution Infrastructure) **Purpose**: Core helper methods that all user stories depend on - [ ] T004 Add method `PatchState::resolve_head(&self, repo: &Repository) -> Result<git2::Oid, Error>` in `src/state.rs` — if `branch` is `Some`, resolve `refs/heads/{branch}` via `repo.refname_to_id()`; if `None`, use `repo.revparse_single(&self.head_commit)` - [ ] T005 Add method `PatchState::staleness(&self, repo: &Repository) -> Result<(usize, usize), Error>` in `src/state.rs` — uses `repo.graph_ahead_behind(branch_tip, base_tip)` to return (ahead, behind) - [ ] T006 Add method `PatchState::is_branch_based(&self) -> bool` in `src/state.rs` — returns `self.branch.is_some()` - [ ] T007 Write tests for `resolve_head()` with both branch-based and OID-based patches in `src/state.rs` tests module or `tests/` - [ ] T008 Write tests for `staleness()` with up-to-date and outdated scenarios in `tests/` **Checkpoint**: Helper methods work for both old and new patch formats --- ## Phase 3: User Story 1 — Create Patch from Branch (Priority: P1) 🎯 MVP **Goal**: `patch create` references a branch instead of a commit OID **Independent Test**: Create a branch with commits, run `patch create`, verify patch lists showing branch name ### Tests for User Story 1 - [ ] T009 [P] [US1] Write test: creating a patch from current branch populates `branch` field in DAG event in `tests/` - [ ] T010 [P] [US1] Write test: creating a patch from detached HEAD auto-creates a branch in `tests/` - [ ] T011 [P] [US1] Write test: creating a duplicate patch for same branch returns error in `tests/` - [ ] T012 [P] [US1] Write test: creating a patch when on base branch returns error in `tests/` ### Implementation for User Story 1 - [ ] T013 [US1] Change `--head` to `--branch` (`-B`) in `PatchCmd::Create`, keep `--head` as hidden deprecated alias in `src/cli.rs` - [ ] T014 [US1] Update `patch::create()` signature: replace `head_commit: &str` with `branch: &str` parameter in `src/patch.rs` - [ ] T015 [US1] In `patch::create()`, resolve branch to tip OID, store both `branch` and `head_commit` (tip at creation) in `PatchCreate` event in `src/patch.rs` - [ ] T016 [US1] Add duplicate branch check in `patch::create()` — scan open patches for matching `branch` field in `src/patch.rs` - [ ] T017 [US1] Update dispatch in `src/lib.rs`: resolve `--branch` default to current branch name from `repo.head()`, error if on base branch, auto-create branch for detached HEAD - [ ] T018 [US1] Update `patch show` output to display branch name and staleness info (ahead/behind) in `src/lib.rs` **Checkpoint**: `patch create` works with branches; `patch show` displays branch name and staleness --- ## Phase 4: User Story 2 — Staleness and Diff via Git (Priority: P2) **Goal**: `patch show` reports staleness, `patch diff` uses three-dot semantics **Independent Test**: Create a patch, advance base, verify staleness reported and diff uses merge-base ### Tests for User Story 2 - [ ] T019 [P] [US2] Write test: `patch show` on up-to-date patch shows "0 behind" in `tests/` - [ ] T020 [P] [US2] Write test: `patch show` on outdated patch shows correct behind count in `tests/` - [ ] T021 [P] [US2] Write test: `patch diff` uses merge-base for branch-based patches in `tests/` ### Implementation for User Story 2 - [ ] T022 [US2] Update `generate_diff()` in `src/patch.rs`: for branch-based patches, compute merge-base between base tip and branch tip, diff merge-base tree against branch tip tree - [ ] T023 [US2] Update `patch::show()` to use `resolve_head()` instead of stored `head_commit` for diff/display in `src/patch.rs` **Checkpoint**: Staleness and diff always reflect current branch state --- ## Phase 5: User Story 3 — Merge as Git Merge (Priority: P3) **Goal**: `patch merge` performs a standard git merge of the branch into base **Independent Test**: Create and approve a patch, merge it, verify branch merged into base ### Tests for User Story 3 - [ ] T024 [P] [US3] Write test: merging a branch-based patch performs git merge and updates DAG in `tests/` - [ ] T025 [P] [US3] Write test: merging a conflicting patch reports error in `tests/` ### Implementation for User Story 3 - [ ] T026 [US3] Update `patch::merge()` to use `resolve_head()` for branch-based patches instead of stored `head_commit` in `src/patch.rs` **Checkpoint**: Merge uses live branch state, producing standard git merge commits --- ## Phase 6: User Story 4 — Implicit Revision (Priority: P4) **Goal**: Branch updates automatically reflected without explicit revise **Independent Test**: Create a patch, push new commit to branch, verify `patch show` reflects it ### Tests for User Story 4 - [ ] T027 [P] [US4] Write test: after pushing a commit to the branch, `patch show` and `patch diff` reflect the new commit without revise in `tests/` ### Implementation for User Story 4 - [ ] T028 [US4] No implementation needed — `resolve_head()` already resolves live. Verify `patch revise` still works for explicit notes in `src/patch.rs` **Checkpoint**: Updating the branch IS revising the patch --- ## Phase 7: User Story 5 — Backward Compatibility (Priority: P5) **Goal**: Old-format patches (no `branch` field) continue to work **Independent Test**: Load a repo with old-format patches, verify list/show/diff/merge all work ### Tests for User Story 5 - [ ] T029 [P] [US5] Write test: old-format patch without `branch` field deserializes and displays correctly in `tests/` - [ ] T030 [P] [US5] Write test: old-format patch diff uses stored `head_commit` in `tests/` - [ ] T031 [P] [US5] Write test: old-format patch merge uses stored `head_commit` in `tests/` ### Implementation for User Story 5 - [ ] T032 [US5] Verify all code paths have `is_branch_based()` guards — no implementation changes expected if Phase 2 was done correctly **Checkpoint**: Old and new format patches coexist without issues --- ## Phase 8: TUI Updates **Purpose**: Dashboard reflects branch-based patches - [ ] T033 [P] Update patch detail view in `src/tui.rs` to show branch name instead of head commit for branch-based patches - [ ] T034 [P] Update patch detail view in `src/tui.rs` to show staleness (ahead/behind) info - [ ] T035 Show "branch not found" in patch detail when branch has been deleted in `src/tui.rs` --- ## Phase 9: Polish & Cross-Cutting Concerns - [ ] T036 Update `patch import` to create a branch and use branch-based `patch::create()` in `src/patch.rs` - [ ] T037 Run `cargo clippy` and fix any warnings - [ ] T038 Run full test suite and verify all tests pass --- ## Dependencies & Execution Order ### Phase Dependencies - **Setup (Phase 1)**: No dependencies — event schema change first - **Foundational (Phase 2)**: Depends on Phase 1 (branch field exists) - **US1 (Phase 3)**: Depends on Phase 2 (resolve_head exists) - **US2 (Phase 4)**: Depends on Phase 3 (branch-based patches can be created) - **US3 (Phase 5)**: Depends on Phase 2 (resolve_head exists) - **US4 (Phase 6)**: Depends on Phase 3 (branch-based patches exist) - **US5 (Phase 7)**: Depends on Phase 2 (resolve_head handles both formats) - **TUI (Phase 8)**: Depends on Phase 3 (branch field in PatchState) - **Polish (Phase 9)**: Depends on all phases ### Parallel Opportunities - US3 and US4 can run in parallel after US1 completes - US5 can run in parallel with US2/US3/US4 - All test tasks within a phase marked [P] can run in parallel - TUI updates (Phase 8) can run in parallel with US3/US4/US5 --- ## Implementation Strategy ### MVP First (User Story 1 Only) 1. Complete Phase 1: Event schema change 2. Complete Phase 2: Branch resolution infrastructure 3. Complete Phase 3: Branch-based patch creation 4. **STOP and VALIDATE**: Create patches from branches, verify listing and display ### Incremental Delivery 1. Setup + Foundational → Branch resolution ready 2. Add US1 → Branch-based patch creation works (MVP!) 3. Add US2 → Staleness and three-dot diff 4. Add US3 → Git-native merge 5. Add US4 → Implicit revision (mostly free) 6. Add US5 → Backward compat verified 7. TUI + Polish → Complete --- ## Notes - Total tasks: 38 - Tasks per story: Setup=3, Foundation=5, US1=10, US2=5, US3=3, US4=2, US5=4, TUI=3, Polish=3 - Parallel opportunities: Test tasks within phases, US3/US4/US5 after US1 - Suggested MVP scope: Phase 1 + Phase 2 + Phase 3 (branch-based creation) - Key risk: CLI flag change (`--head` to `--branch`) may affect existing scripts/workflows diff --git a/src/cli.rs b/src/cli.rs index e7f547b..5acc21e 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -165,8 +165,11 @@ pub enum PatchCmd { /// Base branch ref #[arg(long, default_value = "main")] base: String, /// Head commit to review (defaults to HEAD) #[arg(long)] /// Source branch (defaults to current branch) #[arg(short = 'B', long)] branch: Option<String>, /// Deprecated: use --branch instead #[arg(long, hide = true)] head: Option<String>, /// Issue ID this patch fixes (auto-closes on merge) #[arg(long)] diff --git a/src/event.rs b/src/event.rs index 289930d..609f5b2 100644 --- a/src/event.rs +++ b/src/event.rs @@ -50,6 +50,8 @@ pub enum Action { head_commit: String, #[serde(default, skip_serializing_if = "Option::is_none")] fixes: Option<String>, #[serde(default, skip_serializing_if = "Option::is_none")] branch: Option<String>, }, PatchRevise { body: Option<String>, diff --git a/src/lib.rs b/src/lib.rs index 8909477..59269cf 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -131,14 +131,61 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> { title, body, base, branch, head, fixes, } => { let head = match head { Some(h) => h, None => repo.head()?.peel_to_commit()?.id().to_string(), // Resolve branch name: --branch takes priority, then --head (deprecated), then current branch let branch_name = if let Some(b) = branch { b } else if let Some(h) = head { // Deprecated --head: if it looks like a branch name, use it; // otherwise try to resolve as OID and create auto-branch if repo.refname_to_id(&format!("refs/heads/{}", h)).is_ok() { h } else if let Ok(obj) = repo.revparse_single(&h) { // OID provided — auto-create a branch let commit = obj.into_commit().map_err(|_| { error::Error::Cmd("--head value is not a commit".to_string()) })?; let short_oid = &commit.id().to_string()[..8]; let auto_branch = format!("collab/patch/{}", short_oid); repo.branch(&auto_branch, &commit, false)?; auto_branch } else { return Err(error::Error::Cmd(format!( "cannot resolve '{}' as a branch or commit", h ))); } } else { // Default to current branch let head_ref = repo.head().map_err(|_| { error::Error::Cmd("cannot determine current branch (detached HEAD?)".to_string()) })?; if head_ref.is_branch() { let name = head_ref.shorthand().ok_or_else(|| { error::Error::Cmd("cannot determine branch name".to_string()) })?; if name == base { return Err(error::Error::Cmd( "cannot create patch from base branch; switch to a feature branch first".to_string(), )); } name.to_string() } else { // Detached HEAD — auto-create branch let oid = head_ref.target().ok_or_else(|| { error::Error::Cmd("cannot determine HEAD OID".to_string()) })?; let commit = repo.find_commit(oid)?; let short_oid = &oid.to_string()[..8]; let auto_branch = format!("collab/patch/{}", short_oid); repo.branch(&auto_branch, &commit, false)?; auto_branch } }; let id = patch::create(repo, &title, &body, &base, &head, fixes.as_deref())?; let id = patch::create(repo, &title, &body, &base, &branch_name, fixes.as_deref())?; println!("Created patch {:.8}", id); Ok(()) } @@ -171,7 +218,22 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> { println!("Patch {} [{}]", &p.id[..8], status); println!("Title: {}", p.title); println!("Author: {} <{}>", p.author.name, p.author.email); println!("Base: {} Head: {:.8}", p.base_ref, p.head_commit); if let Some(ref branch) = p.branch { match p.resolve_head(repo) { Ok(_) => { println!("Branch: {} -> {}", branch, p.base_ref); if let Ok((ahead, behind)) = p.staleness(repo) { let freshness = if behind == 0 { "up-to-date" } else { "outdated" }; println!("Commits: {} ahead, {} behind ({})", ahead, behind, freshness); } } Err(_) => { println!("Branch: {} (not found)", branch); } } } else { println!("Base: {} Head: {:.8}", p.base_ref, p.head_commit); } println!("Created: {}", p.created_at); if let Some(ref fixes) = p.fixes { println!("Fixes: {:.8}", fixes); diff --git a/src/patch.rs b/src/patch.rs index 6247f81..d3ee8e8 100644 --- a/src/patch.rs +++ b/src/patch.rs @@ -14,9 +14,37 @@ pub fn create( title: &str, body: &str, base_ref: &str, head_commit: &str, branch: &str, fixes: Option<&str>, ) -> Result<String, crate::error::Error> { // Reject creating a patch from the base branch if branch == base_ref { return Err(crate::error::Error::Cmd( "cannot create patch from base branch".to_string(), )); } // Resolve branch to its tip OID let branch_ref = format!("refs/heads/{}", branch); 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 let patches = state::list_patches(repo)?; for p in &patches { if p.status == PatchStatus::Open { if let Some(ref existing_branch) = p.branch { if existing_branch == branch { return Err(crate::error::Error::Cmd(format!( "patch already exists for branch '{}'", branch ))); } } } } let sk = signing::load_signing_key(&signing::signing_key_dir()?)?; let author = get_author(repo)?; let event = Event { @@ -26,7 +54,8 @@ pub fn create( title: title.to_string(), body: body.to_string(), base_ref: base_ref.to_string(), head_commit: head_commit.to_string(), head_commit: tip_oid.to_string(), branch: Some(branch.to_string()), fixes: fixes.map(|s| s.to_string()), }, }; @@ -143,13 +172,10 @@ pub fn merge(repo: &Repository, id_prefix: &str) -> Result<PatchState, crate::er .into()); } // Resolve the head commit (supports short OIDs via revparse) let head_obj = repo .revparse_single(&p.head_commit) // Resolve the head commit: use branch tip for branch-based patches, stored OID otherwise 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"))?; let head_commit = head_obj .into_commit() .map_err(|_| git2::Error::from_str("head ref is not a commit"))?; let base_ref = format!("refs/heads/{}", p.base_ref); let base_oid = repo.refname_to_id(&base_ref)?; @@ -411,12 +437,22 @@ pub fn import(repo: &Repository, patch_path: &Path) -> Result<String, Error> { }; let new_tree = repo.find_tree(tree_oid)?; // Create a commit on a detached ref (no branch update) // Determine the base branch name from HEAD let base_ref = repo .head()? .shorthand() .unwrap_or("main") .to_string(); // Create a commit on a new branch for the import let author = get_author(repo)?; let sig = crate::identity::author_signature(&author)?; let commit_msg = format!("imported: {}", header.subject); let short_id = &header.subject.replace(|c: char| !c.is_alphanumeric(), "-"); let import_branch = format!("collab/import/{}", &short_id[..short_id.len().min(30)]); let branch_ref = format!("refs/heads/{}", import_branch); let commit_oid = repo.commit( None, // don't update any ref Some(&branch_ref), &sig, &sig, &commit_msg, @@ -424,22 +460,17 @@ pub fn import(repo: &Repository, patch_path: &Path) -> Result<String, Error> { &[&head_commit], )?; // Determine the base branch name from HEAD let base_ref = repo .head()? .shorthand() .unwrap_or("main") .to_string(); // Create a DAG entry using the existing patch create infrastructure let id = create( repo, &header.subject, &header.body, &base_ref, &commit_oid.to_string(), &import_branch, None, )?; // Suppress unused variable warning let _ = commit_oid; Ok(id) } diff --git a/src/state.rs b/src/state.rs index e7823a0..161fdc1 100644 --- a/src/state.rs +++ b/src/state.rs @@ -67,6 +67,7 @@ pub struct PatchState { pub base_ref: String, pub head_commit: String, pub fixes: Option<String>, pub branch: Option<String>, pub comments: Vec<Comment>, pub inline_comments: Vec<InlineComment>, pub reviews: Vec<Review>, @@ -179,6 +180,46 @@ impl IssueState { } impl PatchState { /// Returns true if this patch tracks a git branch (new format). /// Returns false for old-format patches that only store a commit OID. pub fn is_branch_based(&self) -> bool { self.branch.is_some() } /// Resolve the current head commit OID for this patch. /// For branch-based patches, resolves `refs/heads/{branch}` live. /// For old-format patches, uses `revparse_single` on the stored `head_commit`. pub fn resolve_head(&self, repo: &Repository) -> Result<Oid, crate::error::Error> { if let Some(ref branch) = self.branch { let ref_name = format!("refs/heads/{}", branch); repo.refname_to_id(&ref_name).map_err(|e| { crate::error::Error::Cmd(format!( "branch '{}' not found: {}", branch, e )) }) } else { let obj = repo.revparse_single(&self.head_commit).map_err(|e| { crate::error::Error::Cmd(format!( "cannot resolve head commit '{}': {}", self.head_commit, e )) })?; Ok(obj.id()) } } /// Compute staleness: how many commits the branch is ahead of base, /// and how many commits the base is ahead of the branch. /// Returns (ahead, behind). pub fn staleness(&self, repo: &Repository) -> Result<(usize, usize), crate::error::Error> { let branch_tip = self.resolve_head(repo)?; let base_ref = format!("refs/heads/{}", self.base_ref); let base_tip = repo.refname_to_id(&base_ref)?; let (ahead, behind) = repo.graph_ahead_behind(branch_tip, base_tip)?; Ok((ahead, behind)) } pub fn from_ref( repo: &Repository, ref_name: &str, @@ -197,6 +238,7 @@ impl PatchState { base_ref, head_commit, fixes, branch, } => { state = Some(PatchState { id: id.to_string(), @@ -206,6 +248,7 @@ impl PatchState { base_ref, head_commit, fixes, branch, comments: Vec::new(), inline_comments: Vec::new(), reviews: Vec::new(), diff --git a/src/tui.rs b/src/tui.rs index 0d0840d..b55c2fd 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -12,7 +12,6 @@ use ratatui::widgets::{Block, Borders, List, ListItem, ListState, Paragraph, Tab use crate::error::Error; use crate::event::Action; use crate::issue as issue_mod; use crate::patch as patch_mod; use crate::state::{self, IssueState, IssueStatus, PatchState, PatchStatus}; #[derive(Debug, PartialEq)] @@ -76,12 +75,21 @@ enum InputMode { CreateBody, } /// Cached staleness info for a patch. #[derive(Clone)] struct PatchBranchInfo { branch: Option<String>, staleness: Option<(usize, usize)>, branch_exists: bool, } struct App { tab: Tab, issues: Vec<IssueState>, patches: Vec<PatchState>, list_state: ListState, diff_cache: HashMap<String, String>, branch_info_cache: HashMap<String, PatchBranchInfo>, scroll: u16, pane: Pane, mode: ViewMode, @@ -107,6 +115,7 @@ impl App { patches, list_state, diff_cache: HashMap::new(), branch_info_cache: HashMap::new(), scroll: 0, pane: Pane::ItemList, mode: ViewMode::Details, @@ -482,6 +491,79 @@ fn action_type_label(action: &Action) -> &str { } } fn generate_diff(repo: &Repository, patch: &PatchState) -> String { let result = (|| -> Result<String, Error> { let head_oid = patch.resolve_head(repo)?; let head_commit = repo.find_commit(head_oid) .map_err(|e| Error::Cmd(format!("bad head ref: {}", e)))?; let head_tree = head_commit.tree()?; let base_ref = format!("refs/heads/{}", patch.base_ref); // For branch-based patches, use three-dot diff (merge-base to branch tip) // For old-format patches, use two-dot diff (base tip to head) let base_tree = if let Ok(base_oid) = repo.refname_to_id(&base_ref) { if patch.is_branch_based() { // Three-dot: find merge-base, diff from there if let Ok(merge_base_oid) = repo.merge_base(base_oid, head_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 { let base_commit = repo.find_commit(base_oid)?; Some(base_commit.tree()?) } } else { None }; let diff = repo.diff_tree_to_tree(base_tree.as_ref(), Some(&head_tree), None)?; let mut output = String::new(); let mut lines = 0usize; diff.print(git2::DiffFormat::Patch, |_delta, _hunk, line| { if lines >= 5000 { return false; } let prefix = match line.origin() { '+' => "+", '-' => "-", ' ' => " ", 'H' | 'F' => "", _ => "", }; if !prefix.is_empty() || matches!(line.origin(), 'H' | 'F') { output.push_str(prefix); } if let Ok(content) = std::str::from_utf8(line.content()) { output.push_str(content); } lines += 1; true })?; if lines >= 5000 { output.push_str("\n[truncated at 5000 lines]"); } Ok(output) })(); match result { Ok(diff) => { if diff.is_empty() { "No diff available (commits may be identical)".to_string() } else { diff } } Err(e) => format!("Diff unavailable: {}", e), } } fn format_event_detail(oid: &Oid, event: &crate::event::Event) -> String { let short_oid = &oid.to_string()[..7]; let action_label = action_type_label(&event.action); @@ -593,21 +675,54 @@ fn run_loop( repo: &Repository, ) -> Result<(), Error> { loop { // Cache diff for selected patch if needed if app.tab == Tab::Patches && app.mode == ViewMode::Diff { // Cache diff and branch info for selected patch if needed if app.tab == Tab::Patches { if let Some(idx) = app.list_state.selected() { let visible = app.visible_patches(); if let Some(patch) = visible.get(idx) { if !app.diff_cache.contains_key(&patch.id) { // Collect info we need without holding the borrow let patch_data = { let visible = app.visible_patches(); visible.get(idx).map(|patch| { let id = patch.id.clone(); let diff = match patch_mod::generate_diff(repo, patch) { Ok(d) if d.is_empty() => { "No diff available (commits may be identical)".to_string() } Ok(d) => d, Err(e) => format!("Diff unavailable: {}", e), let needs_branch_info = !app.branch_info_cache.contains_key(&id); let needs_diff = app.mode == ViewMode::Diff && !app.diff_cache.contains_key(&id); let branch_info = if needs_branch_info { Some(if patch.is_branch_based() { let branch_exists = patch.resolve_head(repo).is_ok(); let staleness = if branch_exists { patch.staleness(repo).ok() } else { None }; PatchBranchInfo { branch: patch.branch.clone(), staleness, branch_exists, } } else { PatchBranchInfo { branch: None, staleness: None, branch_exists: false, } }) } else { None }; app.diff_cache.insert(id, diff); let diff = if needs_diff { Some(generate_diff(repo, patch)) } else { None }; (id, branch_info, diff) }) }; if let Some((id, branch_info, diff)) = patch_data { if let Some(info) = branch_info { app.branch_info_cache.insert(id.clone(), info); } if let Some(d) = diff { app.diff_cache.insert(id, d); } } } @@ -1044,7 +1159,10 @@ fn render_detail(frame: &mut Frame, app: &mut App, area: Rect) { let selected_idx = app.list_state.selected().unwrap_or(0); match visible.get(selected_idx) { Some(patch) => match app.mode { ViewMode::Details => build_patch_detail(patch), ViewMode::Details => { let branch_info = app.branch_info_cache.get(&patch.id).cloned(); build_patch_detail(patch, branch_info.as_ref()) } ViewMode::Diff => { let diff_text = app .diff_cache @@ -1202,7 +1320,7 @@ fn build_issue_detail(issue: &IssueState, patches: &[PatchState]) -> Text<'stati Text::from(lines) } fn build_patch_detail(patch: &PatchState) -> Text<'static> { fn build_patch_detail(patch: &PatchState, branch_info: Option<&PatchBranchInfo>) -> Text<'static> { let status = match patch.status { PatchStatus::Open => "open", PatchStatus::Closed => "closed", @@ -1228,7 +1346,40 @@ fn build_patch_detail(patch: &PatchState) -> Text<'static> { Span::styled("Author: ", Style::default().fg(Color::DarkGray)), Span::raw(format!("{} <{}>", patch.author.name, patch.author.email)), ]), Line::from(vec![ ]; // Show branch info for branch-based patches, head commit for old-format if let Some(info) = branch_info { if let Some(ref branch) = info.branch { if info.branch_exists { lines.push(Line::from(vec![ Span::styled("Branch: ", Style::default().fg(Color::DarkGray)), Span::styled( branch.clone(), Style::default().fg(Color::Cyan), ), Span::raw(format!(" -> {}", patch.base_ref)), ])); if let Some((ahead, behind)) = info.staleness { let freshness = if behind == 0 { "up-to-date" } else { "outdated" }; lines.push(Line::from(vec![ Span::styled("Commits: ", Style::default().fg(Color::DarkGray)), Span::raw(format!("{} ahead, {} behind ({})", ahead, behind, freshness)), ])); } } else { lines.push(Line::from(vec![ Span::styled("Branch: ", Style::default().fg(Color::DarkGray)), Span::styled( branch.clone(), Style::default().fg(Color::Red), ), Span::styled(" (not found)", Style::default().fg(Color::Red)), ])); } } } else { lines.push(Line::from(vec![ Span::styled("Base: ", Style::default().fg(Color::DarkGray)), Span::raw(patch.base_ref.clone()), Span::raw(" "), @@ -1237,12 +1388,13 @@ fn build_patch_detail(patch: &PatchState) -> Text<'static> { format!("{:.8}", patch.head_commit), Style::default().fg(Color::Cyan), ), ]), Line::from(vec![ Span::styled("Created: ", Style::default().fg(Color::DarkGray)), Span::raw(patch.created_at.clone()), ]), ]; ])); } lines.push(Line::from(vec![ Span::styled("Created: ", Style::default().fg(Color::DarkGray)), Span::raw(patch.created_at.clone()), ])); if let Some(ref fixes) = patch.fixes { lines.push(Line::from(vec![ @@ -1588,6 +1740,7 @@ mod tests { base_ref: "main".into(), head_commit: "abc123".into(), fixes: None, branch: None, comments: vec![], inline_comments: vec![], reviews: vec![], @@ -1805,6 +1958,7 @@ mod tests { base_ref: "main".to_string(), head_commit: format!("h{:07x}", i), fixes: None, branch: None, comments: Vec::new(), inline_comments: Vec::new(), reviews: Vec::new(), @@ -1930,6 +2084,7 @@ mod tests { base_ref: "main".to_string(), head_commit: "abc".to_string(), fixes: None, branch: None, }; assert_eq!(action_type_label(&action), "Patch Create"); } diff --git a/tests/cli_test.rs b/tests/cli_test.rs index eecd115..c9c1af1 100644 --- a/tests/cli_test.rs +++ b/tests/cli_test.rs @@ -431,7 +431,11 @@ fn test_patch_revise() { let repo = TestRepo::new("Alice", "alice@example.com"); let id = repo.patch_create("WIP feature"); // Switch to the patch's branch and add a new commit repo.git(&["checkout", "test/wip-feature"]); let new_head = repo.commit_file("v2.txt", "v2", "version 2"); repo.git(&["checkout", "main"]); let out = repo.run_ok(&[ "patch", "revise", @@ -444,8 +448,9 @@ fn test_patch_revise() { assert!(out.contains("Patch revised")); let out = repo.run_ok(&["patch", "show", &id]); assert!(out.contains(&new_head[..8])); assert!(out.contains("Updated implementation")); // Branch-based patch show displays the branch name, not the raw head OID assert!(out.contains("test/wip-feature")); } #[test] diff --git a/tests/collab_test.rs b/tests/collab_test.rs index 41453f5..0e51acd 100644 --- a/tests/collab_test.rs +++ b/tests/collab_test.rs @@ -553,3 +553,487 @@ fn test_issue_open_without_signing_key_returns_key_not_found() { other => panic!("expected KeyNotFound error, got: {:?}", other), } } // --------------------------------------------------------------------------- // Helpers for branch-based patch tests // --------------------------------------------------------------------------- /// Create an initial commit on the given branch so the repo is not empty. fn make_initial_commit(repo: &git2::Repository, branch: &str) -> git2::Oid { let sig = git2::Signature::now("Test", "test@example.com").unwrap(); let mut tb = repo.treebuilder(None).unwrap(); let blob = repo.blob(b"init").unwrap(); tb.insert("README.md", blob, 0o100644).unwrap(); let tree_oid = tb.write().unwrap(); let tree = repo.find_tree(tree_oid).unwrap(); let oid = repo.commit(None, &sig, &sig, "initial commit", &tree, &[]).unwrap(); let ref_name = format!("refs/heads/{}", branch); repo.reference(&ref_name, oid, true, "init branch").unwrap(); // Set HEAD to this branch repo.set_head(&ref_name).unwrap(); oid } /// Add a commit on the given branch, returns the new OID. fn add_commit_on_branch(repo: &git2::Repository, branch: &str, filename: &str, content: &[u8]) -> git2::Oid { let ref_name = format!("refs/heads/{}", branch); let parent_oid = repo.refname_to_id(&ref_name).unwrap(); let parent = repo.find_commit(parent_oid).unwrap(); let sig = git2::Signature::now("Test", "test@example.com").unwrap(); // Build a tree with the new file added to parent's tree let parent_tree = parent.tree().unwrap(); let mut tb = repo.treebuilder(Some(&parent_tree)).unwrap(); let blob = repo.blob(content).unwrap(); tb.insert(filename, blob, 0o100644).unwrap(); let tree_oid = tb.write().unwrap(); let tree = repo.find_tree(tree_oid).unwrap(); repo.commit(Some(&ref_name), &sig, &sig, &format!("add {}", filename), &tree, &[&parent]).unwrap() } /// Create a branch-based patch using DAG primitives. fn create_branch_patch( repo: &git2::Repository, author: &Author, title: &str, branch: &str, base_ref: &str, ) -> (String, String) { let sk = test_signing_key(); let ref_name = format!("refs/heads/{}", branch); let tip = repo.refname_to_id(&ref_name).unwrap(); let event = Event { timestamp: now(), author: author.clone(), action: Action::PatchCreate { title: title.to_string(), body: "".to_string(), base_ref: base_ref.to_string(), head_commit: tip.to_string(), branch: Some(branch.to_string()), fixes: None, }, }; let oid = dag::create_root_event(repo, &event, &sk).unwrap(); let id = oid.to_string(); let patch_ref = format!("refs/collab/patches/{}", id); repo.reference(&patch_ref, oid, false, "test branch patch").unwrap(); (patch_ref, id) } // --------------------------------------------------------------------------- // Phase 2: Branch resolution infrastructure tests (T007, T008) // --------------------------------------------------------------------------- #[test] fn test_resolve_head_branch_based() { let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); // Create main branch and feature branch make_initial_commit(&repo, "main"); let feature_tip = add_commit_on_branch(&repo, "main", "feature.rs", b"fn feature() {}"); // Create the feature branch at the current tip repo.branch("feature/test", &repo.find_commit(feature_tip).unwrap(), false).unwrap(); let (patch_ref, id) = create_branch_patch(&repo, &alice(), "Test patch", "feature/test", "main"); let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); // resolve_head should return the branch tip let resolved = state.resolve_head(&repo).unwrap(); assert_eq!(resolved, feature_tip); // Now add a commit to the branch — resolve_head should return the new tip let new_tip = add_commit_on_branch(&repo, "feature/test", "more.rs", b"more code"); let resolved2 = state.resolve_head(&repo).unwrap(); assert_eq!(resolved2, new_tip); } #[test] fn test_resolve_head_oid_based() { let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); let commit_oid = add_commit_on_branch(&repo, "main", "file.rs", b"content"); // Create old-format (OID-based) patch let (_patch_ref, _id) = create_patch(&repo, &alice(), "Old patch"); // The test helper uses "abc123" as head_commit which won't resolve. // Create a proper old-format patch with a real OID. let sk = test_signing_key(); let event = Event { timestamp: now(), author: alice(), action: Action::PatchCreate { title: "OID patch".to_string(), body: "".to_string(), base_ref: "main".to_string(), head_commit: commit_oid.to_string(), branch: None, fixes: None, }, }; let oid = dag::create_root_event(&repo, &event, &sk).unwrap(); let id = oid.to_string(); let patch_ref = format!("refs/collab/patches/{}", id); repo.reference(&patch_ref, oid, false, "test oid patch").unwrap(); let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); let resolved = state.resolve_head(&repo).unwrap(); assert_eq!(resolved, commit_oid); } #[test] fn test_resolve_head_deleted_branch_error() { let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x"); repo.branch("ephemeral", &repo.find_commit(tip).unwrap(), false).unwrap(); let (patch_ref, id) = create_branch_patch(&repo, &alice(), "Ephemeral patch", "ephemeral", "main"); // Delete the branch let mut branch = repo.find_branch("ephemeral", git2::BranchType::Local).unwrap(); branch.delete().unwrap(); let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); let result = state.resolve_head(&repo); assert!(result.is_err(), "resolve_head should error when branch is deleted"); } #[test] fn test_is_branch_based() { let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x"); repo.branch("feat", &repo.find_commit(tip).unwrap(), false).unwrap(); // Branch-based patch let (patch_ref, id) = create_branch_patch(&repo, &alice(), "Branch patch", "feat", "main"); let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); assert!(state.is_branch_based()); // OID-based patch let (patch_ref2, id2) = create_patch(&repo, &alice(), "OID patch"); let state2 = PatchState::from_ref(&repo, &patch_ref2, &id2).unwrap(); assert!(!state2.is_branch_based()); } #[test] fn test_staleness_up_to_date() { let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x"); repo.branch("feat", &repo.find_commit(tip).unwrap(), false).unwrap(); // Add one commit on the feature branch add_commit_on_branch(&repo, "feat", "feat.rs", b"feature code"); let (patch_ref, id) = create_branch_patch(&repo, &alice(), "Up-to-date", "feat", "main"); let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); let (ahead, behind) = state.staleness(&repo).unwrap(); assert_eq!(ahead, 1, "feature branch has 1 commit ahead of main"); assert_eq!(behind, 0, "main has not moved, so 0 behind"); } #[test] fn test_staleness_outdated() { let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x"); repo.branch("feat", &repo.find_commit(tip).unwrap(), false).unwrap(); // Add commit on feature branch add_commit_on_branch(&repo, "feat", "feat.rs", b"feature code"); // Advance main by 2 commits add_commit_on_branch(&repo, "main", "main1.rs", b"main work 1"); add_commit_on_branch(&repo, "main", "main2.rs", b"main work 2"); let (patch_ref, id) = create_branch_patch(&repo, &alice(), "Outdated", "feat", "main"); let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); let (ahead, behind) = state.staleness(&repo).unwrap(); assert_eq!(ahead, 1, "feature has 1 commit ahead"); assert_eq!(behind, 2, "main has moved 2 commits ahead"); } // --------------------------------------------------------------------------- // Phase 3: US1 — Create Patch from Branch (T009-T012) // --------------------------------------------------------------------------- use git_collab::patch; #[test] fn test_create_patch_from_branch_populates_branch_field() { // T009: creating a patch from current branch populates `branch` field let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x"); repo.branch("feature/foo", &repo.find_commit(tip).unwrap(), false).unwrap(); add_commit_on_branch(&repo, "feature/foo", "feat.rs", b"feat"); let id = patch::create(&repo, "My patch", "desc", "main", "feature/foo", None).unwrap(); let patch_ref = format!("refs/collab/patches/{}", id); let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); assert!(state.is_branch_based()); assert_eq!(state.branch, Some("feature/foo".to_string())); // head_commit should be set to the branch tip at creation time let branch_tip = repo.refname_to_id("refs/heads/feature/foo").unwrap(); assert_eq!(state.head_commit, branch_tip.to_string()); } #[test] fn test_create_duplicate_patch_for_same_branch_returns_error() { // T011: creating a duplicate patch for same branch returns error let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x"); repo.branch("feature/dup", &repo.find_commit(tip).unwrap(), false).unwrap(); // First creation should succeed patch::create(&repo, "First", "", "main", "feature/dup", None).unwrap(); // Second creation for same branch should fail let result = patch::create(&repo, "Second", "", "main", "feature/dup", None); assert!(result.is_err(), "duplicate branch patch should fail"); let err_msg = result.unwrap_err().to_string(); assert!(err_msg.contains("feature/dup"), "error should mention the branch name"); } #[test] fn test_create_patch_from_base_branch_returns_error() { // T012: creating a patch when on base branch returns error let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); let result = patch::create(&repo, "Bad patch", "", "main", "main", None); assert!(result.is_err(), "creating patch from base branch should fail"); let err_msg = result.unwrap_err().to_string(); assert!(err_msg.contains("base branch"), "error should mention base branch"); } // --------------------------------------------------------------------------- // Phase 4: US2 — Staleness and Diff (T019-T021) // --------------------------------------------------------------------------- #[test] fn test_patch_show_up_to_date_staleness() { // T019: patch show on up-to-date patch shows "0 behind" let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x"); repo.branch("feat", &repo.find_commit(tip).unwrap(), false).unwrap(); add_commit_on_branch(&repo, "feat", "feat.rs", b"code"); let (patch_ref, id) = create_branch_patch(&repo, &alice(), "Up to date", "feat", "main"); let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); let (_, behind) = state.staleness(&repo).unwrap(); assert_eq!(behind, 0); } #[test] fn test_patch_show_outdated_staleness() { // T020: patch show on outdated patch shows correct behind count let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x"); repo.branch("feat", &repo.find_commit(tip).unwrap(), false).unwrap(); add_commit_on_branch(&repo, "feat", "feat.rs", b"code"); // Advance main add_commit_on_branch(&repo, "main", "m1.rs", b"m1"); add_commit_on_branch(&repo, "main", "m2.rs", b"m2"); add_commit_on_branch(&repo, "main", "m3.rs", b"m3"); let (patch_ref, id) = create_branch_patch(&repo, &alice(), "Outdated", "feat", "main"); let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); let (ahead, behind) = state.staleness(&repo).unwrap(); assert_eq!(ahead, 1); assert_eq!(behind, 3); } // --------------------------------------------------------------------------- // Phase 5: US3 — Merge (T024-T025) // --------------------------------------------------------------------------- #[test] fn test_merge_branch_based_patch() { // T024: merging a branch-based patch performs git merge and updates DAG let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x"); repo.branch("feat", &repo.find_commit(tip).unwrap(), false).unwrap(); add_commit_on_branch(&repo, "feat", "feat.rs", b"feature code"); let id = patch::create(&repo, "Merge test", "", "main", "feat", None).unwrap(); // Merge the patch patch::merge(&repo, &id).unwrap(); // Check that the patch is now merged let patch_ref = format!("refs/collab/patches/{}", id); let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); assert_eq!(state.status, PatchStatus::Merged); // Check that the base branch now has the feature commit let base_tip = repo.refname_to_id("refs/heads/main").unwrap(); let feat_tip = repo.refname_to_id("refs/heads/feat").unwrap(); assert!( repo.graph_descendant_of(base_tip, feat_tip).unwrap() || base_tip == feat_tip, "base should contain the feature branch" ); } #[test] fn test_merge_conflicting_patch_reports_error() { // T025: merging a conflicting patch reports error let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); let tip = add_commit_on_branch(&repo, "main", "conflict.rs", b"original"); repo.branch("feat", &repo.find_commit(tip).unwrap(), false).unwrap(); // Both branches modify the same file differently add_commit_on_branch(&repo, "feat", "conflict.rs", b"feature version"); add_commit_on_branch(&repo, "main", "conflict.rs", b"main version"); let id = patch::create(&repo, "Conflict test", "", "main", "feat", None).unwrap(); let result = patch::merge(&repo, &id); assert!(result.is_err(), "conflicting merge should fail"); let err_msg = result.unwrap_err().to_string(); assert!(err_msg.contains("conflict"), "error should mention conflicts"); } // --------------------------------------------------------------------------- // Phase 6: US4 — Implicit Revision (T027) // --------------------------------------------------------------------------- #[test] fn test_branch_push_auto_reflects_in_patch() { // T027: after pushing a commit to the branch, patch show reflects the new commit let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x"); repo.branch("feat", &repo.find_commit(tip).unwrap(), false).unwrap(); add_commit_on_branch(&repo, "feat", "v1.rs", b"version 1"); let (patch_ref, id) = create_branch_patch(&repo, &alice(), "Auto revise", "feat", "main"); let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); let head1 = state.resolve_head(&repo).unwrap(); // Add another commit to the branch (simulating a push) let new_tip = add_commit_on_branch(&repo, "feat", "v2.rs", b"version 2"); // Re-read the state and resolve head — should see the new commit let state2 = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); let head2 = state2.resolve_head(&repo).unwrap(); assert_ne!(head1, head2, "head should change after branch update"); assert_eq!(head2, new_tip, "head should be the new tip"); } // --------------------------------------------------------------------------- // Phase 7: US5 — Backward Compatibility (T029-T031) // --------------------------------------------------------------------------- #[test] fn test_old_format_patch_without_branch_deserializes() { // T029: old-format patch without branch field works let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); let (patch_ref, id) = create_patch(&repo, &alice(), "Old format patch"); let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); assert!(!state.is_branch_based()); assert_eq!(state.branch, None); assert_eq!(state.head_commit, "abc123"); } #[test] fn test_old_format_patch_diff_uses_stored_head() { // T030: old-format patch diff uses stored head_commit let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); let commit_oid = add_commit_on_branch(&repo, "main", "file.rs", b"content"); // Create OID-based patch with real commit let sk = test_signing_key(); let event = Event { timestamp: now(), author: alice(), action: Action::PatchCreate { title: "OID diff test".to_string(), body: "".to_string(), base_ref: "main".to_string(), head_commit: commit_oid.to_string(), branch: None, fixes: None, }, }; let oid = dag::create_root_event(&repo, &event, &sk).unwrap(); let id = oid.to_string(); let patch_ref = format!("refs/collab/patches/{}", id); repo.reference(&patch_ref, oid, false, "test").unwrap(); let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); let resolved = state.resolve_head(&repo).unwrap(); assert_eq!(resolved, commit_oid, "old-format should resolve to stored head_commit"); } #[test] fn test_old_format_patch_merge_uses_stored_head() { // T031: old-format patch merge uses stored head_commit let tmp = TempDir::new().unwrap(); let repo = init_repo(tmp.path(), &alice()); make_initial_commit(&repo, "main"); let commit_oid = add_commit_on_branch(&repo, "main", "file.rs", b"content"); // Create a branch for the head commit so we can merge it repo.branch("old-feat", &repo.find_commit(commit_oid).unwrap(), false).unwrap(); add_commit_on_branch(&repo, "old-feat", "old.rs", b"old feature"); let feat_tip = repo.refname_to_id("refs/heads/old-feat").unwrap(); // Create OID-based patch with the feature tip let sk = test_signing_key(); let event = Event { timestamp: now(), author: alice(), action: Action::PatchCreate { title: "Old merge test".to_string(), body: "".to_string(), base_ref: "main".to_string(), head_commit: feat_tip.to_string(), branch: None, fixes: None, }, }; let oid = dag::create_root_event(&repo, &event, &sk).unwrap(); let id = oid.to_string(); let patch_ref = format!("refs/collab/patches/{}", id); repo.reference(&patch_ref, oid, false, "test").unwrap(); // Merge should succeed using the stored OID patch::merge(&repo, &id[..8]).unwrap(); let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); assert_eq!(state.status, PatchStatus::Merged); } diff --git a/tests/common/mod.rs b/tests/common/mod.rs index fe2665c..b827a36 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -121,6 +121,7 @@ pub fn create_patch(repo: &Repository, author: &Author, title: &str) -> (String, base_ref: "main".to_string(), head_commit: "abc123".to_string(), fixes: None, branch: None, }, }; let oid = dag::create_root_event(repo, &event, &sk).unwrap(); @@ -223,10 +224,20 @@ impl TestRepo { .to_string() } /// Create a patch with HEAD as the head commit. Returns the 8-char short ID. /// Create a patch from a new branch. Returns the 8-char short ID. pub fn patch_create(&self, title: &str) -> String { let head = self.git(&["rev-parse", "HEAD"]).trim().to_string(); let out = self.run_ok(&["patch", "create", "-t", title, "--head", &head]); // Create a unique branch for this patch let sanitized = title.replace(|c: char| !c.is_alphanumeric(), "-").to_lowercase(); let branch_name = format!("test/{}", sanitized); self.git(&["checkout", "-b", &branch_name]); self.commit_file( &format!("{}.txt", sanitized), &format!("content for {}", title), &format!("commit for {}", title), ); let out = self.run_ok(&["patch", "create", "-t", title, "-B", &branch_name]); // Go back to main for subsequent operations self.git(&["checkout", "main"]); out.trim() .strip_prefix("Created patch ") .unwrap_or_else(|| panic!("unexpected patch create output: {}", out)) diff --git a/tests/signing_test.rs b/tests/signing_test.rs index 0d0da0e..abde304 100644 --- a/tests/signing_test.rs +++ b/tests/signing_test.rs @@ -213,6 +213,7 @@ fn signed_event_flatten_round_trip_with_tagged_enum() { base_ref: "main".to_string(), head_commit: "abc123".to_string(), fixes: Some("deadbeef".to_string()), branch: None, }, }; diff --git a/tests/sync_test.rs b/tests/sync_test.rs index 0c349d7..c668533 100644 --- a/tests/sync_test.rs +++ b/tests/sync_test.rs @@ -285,6 +285,7 @@ fn test_patch_review_across_repos() { base_ref: "main".to_string(), head_commit: "abc123".to_string(), fixes: None, branch: None, }, }; let sk = test_signing_key(); @@ -332,6 +333,7 @@ fn test_concurrent_review_and_revise() { base_ref: "main".to_string(), head_commit: "v1".to_string(), fixes: None, branch: None, }, }; let sk = test_signing_key();