a73x

8127cbdf

Carry patches as revision refs instead of branches

a73x   2026-08-09 19:28

Commit message
Carry patches as revision refs instead of branches

Each revision becomes an immutable ref in the patch's own namespace:

    refs/collab/patches/<id>/events     the event DAG
    refs/collab/patches/<id>/r/<n>      revision n's commit, write-once

The `events` rename is forced — git will not let <id> be both a ref and a
directory. Verified end to end that the existing refspecs carry the whole
subtree: a patch created and synced with no `git push` of any branch is
fully visible to a second clone, revision commits included.

Fixes two live defects. A rebase-and-force-push used to leave an earlier
revision's commit reachable from no ref at all, so `git gc` was entitled
to delete the objects that `patch log`, interdiff and revision-anchored
inline comments point at. And duplicate detection matched patches by
branch name, which breaks for agents on generated worktree branches; it
now matches by commit, since a branch name is no longer part of a
patch's identity.

Merge detection is reworked rather than left to default. It read
PatchState.base_commit, which this change removes in favour of a
per-revision `base`; deleting the field alone would have left
`base_moved` permanently false via `unwrap_or(false)` and quietly
stopped every patch from ever reaching Merged. It now reads the latest
revision's base — which also means it no longer routes through
refs/heads/<branch> and no longer no-ops silently when the source branch
was deleted or never pushed.

Revision auto-detection is gone. `patch comment` and `patch review` used
to infer a revision from the branch tip as a side effect, appending
history the user never asked for, timestamped at the comment rather than
at the code change. `patch revise` is now the only way one is recorded,
and it takes -B/--branch, defaulting to HEAD.

delete and close operate on the subtree, so a closed patch keeps its
revisions under refs/collab/archive/patches/<id>/ and stays reviewable.

Patches in the old single-ref layout migrate on first use: the ref
becomes <id>/events and each revision whose commit is still present gets
an r/<n>. Revisions already lost to a force-push keep the
""-means-unknown convention, and revisions with no stored base fall back
to recomputing the merge-base — old patches degrade to what they already
did rather than failing.

sync needed one change the design did not anticipate: reconcile_refs
treated every fetched ref as an event DAG, so it would have tried to
verify signatures on revision refs and reconcile them as DAGs. It now
classifies them and adopts revision refs write-once.
README.md
Old New
@@ -41,9 +41,15 @@ $ git-collab patch diff a1b2c3d4 --between 1 2 # interdiff: what changed betwe
41 $ git-collab patch diff a1b2c3d4 --revision 2 # revision 2 against the base 41 $ git-collab patch diff a1b2c3d4 --revision 2 # revision 2 against the base
42 ``` 42 ```
43 43
44 Merging is not a command. A patch is a branch; when its commits become reachable 44 Each revision is an immutable ref in the patch's own namespace
45 from the base branch, `git-collab` notices and marks the patch merged. Merge 45 (`refs/collab/patches/<id>/r/<n>`), so `git-collab sync` carries the patch and
46 however you already merge. 46 every revision it ever had. Contributing needs no `git push` of a branch and no
47 write access to `refs/heads/*`, and rebasing your branch cannot strip an earlier
48 revision of the objects a reviewer is looking at.
49
50 Merging is not a command. When a patch's commits become reachable from the base
51 branch, `git-collab` notices and marks the patch merged. Merge however you
52 already merge.
47 53
48 ## Install 54 ## Install
49 55
@@ -68,6 +74,10 @@ $ ...work, commit...
68 $ git-collab patch create -t "Fix trailing newline in parser" 74 $ git-collab patch create -t "Fix trailing newline in parser"
69 75
70 $ git-collab sync # fetch, reconcile, push 76 $ git-collab sync # fetch, reconcile, push
77
78 $ ...address review, commit...
79 $ git-collab patch revise a1b2c3d4 -b "addressed review" # record revision 2
80 $ git-collab sync
71 ``` 81 ```
72 82
73 Reviewing someone else's patch: 83 Reviewing someone else's patch:
src/cli.rs
Old New
@@ -403,6 +403,9 @@ pub enum PatchCmd {
403 /// Revision description 403 /// Revision description
404 #[arg(short, long)] 404 #[arg(short, long)]
405 body: Option<String>, 405 body: Option<String>,
406 /// Source branch to snapshot (defaults to HEAD)
407 #[arg(short = 'B', long)]
408 branch: Option<String>,
406 }, 409 },
407 /// Show revision log for a patch 410 /// Show revision log for a patch
408 Log { 411 Log {
src/event.rs
Old New
@@ -77,7 +77,10 @@ pub enum Action {
77 commit: String, 77 commit: String,
78 #[serde(default, skip_serializing_if = "String::is_empty")] 78 #[serde(default, skip_serializing_if = "String::is_empty")]
79 tree: String, 79 tree: String,
80 /// Base branch tip OID at creation time (for merge detection). 80 /// Where revision 1 branched off `base_ref`: the merge-base of the
81 /// base branch and `commit`, recorded when the patch is created. Named
82 /// `base_commit` because it is part of the signed payload of events
83 /// already in the wild; `PatchRevision` calls the same thing `base`.
81 #[serde(default, skip_serializing_if = "Option::is_none")] 84 #[serde(default, skip_serializing_if = "Option::is_none")]
82 base_commit: Option<String>, 85 base_commit: Option<String>,
83 }, 86 },
@@ -94,6 +97,12 @@ pub enum Action {
94 tree: String, 97 tree: String,
95 #[serde(default, skip_serializing_if = "Option::is_none")] 98 #[serde(default, skip_serializing_if = "Option::is_none")]
96 body: Option<String>, 99 body: Option<String>,
100 /// Merge-base of the base branch and `commit` when this revision was
101 /// recorded. `None` on revisions written before it was stored, and
102 /// skipped on the way out so those events still round-trip byte for
103 /// byte for signature verification.
104 #[serde(default, skip_serializing_if = "Option::is_none")]
105 base: Option<String>,
97 }, 106 },
98 /// `revision` is `None` on reviews written before reviews were scoped to a 107 /// `revision` is `None` on reviews written before reviews were scoped to a
99 /// revision. It is not the same as revision 1: `PatchState` attributes 108 /// revision. It is not the same as revision 1: `PatchState` attributes
src/lib.rs
Old New
@@ -395,8 +395,11 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
395 println!("Title: {}", p.title); 395 println!("Title: {}", p.title);
396 println!("Author: {} <{}>", p.author.name, p.author.email); 396 println!("Author: {} <{}>", p.author.name, p.author.email);
397 match p.resolve_head(repo) { 397 match p.resolve_head(repo) {
398 Ok(_) => { 398 Ok(head) => {
399 println!("Branch: {} -> {}", p.branch, p.base_ref); 399 // The head is the latest recorded revision, not a
400 // branch tip; the branch is shown for provenance only.
401 println!("Head: {:.8} -> {}", head, p.base_ref);
402 println!("Branch: {}", p.branch);
400 if let Ok((ahead, behind)) = p.staleness(repo) { 403 if let Ok((ahead, behind)) = p.staleness(repo) {
401 let freshness = if behind == 0 { 404 let freshness = if behind == 0 {
402 "up-to-date" 405 "up-to-date"
@@ -410,7 +413,8 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
410 } 413 }
411 } 414 }
412 Err(_) => { 415 Err(_) => {
413 println!("Branch: {} (not found)", p.branch); 416 println!("Head: unknown -> {}", p.base_ref);
417 println!("Branch: {}", p.branch);
414 } 418 }
415 } 419 }
416 println!("Created: {}", p.created_at); 420 println!("Created: {}", p.created_at);
@@ -541,8 +545,8 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
541 println!("Review submitted."); 545 println!("Review submitted.");
542 Ok(()) 546 Ok(())
543 } 547 }
544 PatchCmd::Revise { id, body } => { 548 PatchCmd::Revise { id, body, branch } => {
545 patch::revise(repo, &id, body.as_deref())?; 549 patch::revise(repo, &id, body.as_deref(), branch.as_deref())?;
546 println!("Patch revised."); 550 println!("Patch revised.");
547 Ok(()) 551 Ok(())
548 } 552 }
src/log.rs
Old New
@@ -43,13 +43,15 @@ pub fn collect_events(repo: &Repository, limit: Option<usize>) -> Result<Vec<Log
43 } 43 }
44 } 44 }
45 45
46 // Walk patches 46 // Walk patches. Only the events ref of each patch is a DAG — the revision
47 let patch_refs = repo.references_glob("refs/collab/patches/*")?; 47 // refs beside it point at ordinary source commits.
48 let patch_refs = repo.references_glob("refs/collab/patches/*/events")?;
48 for r in patch_refs { 49 for r in patch_refs {
49 let r = r?; 50 let r = r?;
50 let ref_name = r.name().unwrap_or_default().to_string(); 51 let ref_name = r.name().unwrap_or_default().to_string();
51 let id = ref_name 52 let id = ref_name
52 .strip_prefix("refs/collab/patches/") 53 .strip_prefix("refs/collab/patches/")
54 .and_then(|rest| rest.strip_suffix("/events"))
53 .unwrap_or_default() 55 .unwrap_or_default()
54 .to_string(); 56 .to_string();
55 if let Ok(events) = dag::walk_events(repo, &ref_name) { 57 if let Ok(events) = dag::walk_events(repo, &ref_name) {
src/patch.rs
Old New
@@ -6,79 +6,40 @@ use crate::error::Error;
6 use crate::event::{Action, Event, ReviewVerdict}; 6 use crate::event::{Action, Event, ReviewVerdict};
7 use crate::identity::get_author; 7 use crate::identity::get_author;
8 use crate::signing; 8 use crate::signing;
9 use crate::state::{self, PatchState, PatchStatus, Revision}; 9 use crate::state::{self, PatchState, PatchStatus};
10 10
11 /// Auto-detect whether the branch tip has changed since the last recorded revision. 11 /// Where a revision branched off its base branch: the merge-base of the base
12 /// If it has, append a PatchRevision event and return the new `Revision` so callers 12 /// tip and the revision's commit, falling back to the base tip when there is
13 /// can update their in-memory `PatchState` without re-walking the DAG. 13 /// no merge-base.
14 fn auto_detect_revision( 14 ///
15 repo: &Repository, 15 /// Merge-base rather than branch tip is what makes the value stable. If the
16 ref_name: &str, 16 /// base branch advances and the author does not rebase, the merge-base does not
17 patch: &PatchState, 17 /// move, so a change in this value means the author actually rebased.
18 sk: &ed25519_dalek::SigningKey, 18 fn revision_base(repo: &Repository, base_ref: &str, commit: Oid) -> Result<Oid, Error> {
19 ) -> Result<Option<Revision>, Error> { 19 let base_oid = repo.refname_to_id(&format!("refs/heads/{}", base_ref))?;
20 let current_rev = patch.revisions.last().map(|r| r.number).unwrap_or(0); 20 Ok(repo.merge_base(base_oid, commit).unwrap_or(base_oid))
21
22 let last_commit = patch
23 .revisions
24 .last()
25 .map(|r| r.commit.as_str())
26 .unwrap_or("");
27
28 // Legacy patches recorded no commit on their revisions, so there is
29 // nothing to compare the branch tip against. Every tip would look like a
30 // change and append a revision that never happened.
31 if last_commit.is_empty() {
32 return Ok(None);
33 }
34
35 // Try to resolve the branch tip
36 let tip_oid = match patch.resolve_head(repo) {
37 Ok(oid) => oid,
38 Err(_) => return Ok(None), // Branch deleted or unavailable
39 };
40
41 let tip_hex = tip_oid.to_string();
42
43 if tip_hex == last_commit {
44 return Ok(None);
45 }
46
47 // Branch tip changed — insert a PatchRevision event
48 let commit = repo.find_commit(tip_oid)?;
49 let tree_oid = commit.tree()?.id();
50 let author = get_author(repo)?;
51 let timestamp = chrono::Utc::now().to_rfc3339();
52 let event = Event {
53 timestamp: timestamp.clone(),
54 author,
55 action: Action::PatchRevision {
56 commit: tip_hex.clone(),
57 tree: tree_oid.to_string(),
58 body: None,
59 },
60 clock: 0,
61 };
62 dag::append_event(repo, ref_name, &event, sk)?;
63 Ok(Some(Revision {
64 number: current_rev + 1,
65 commit: tip_hex,
66 tree: tree_oid.to_string(),
67 body: None,
68 timestamp,
69 }))
70 } 21 }
71 22
72 /// Auto-detect revision changes and update the in-memory PatchState in place. 23 /// Record revision `number`'s commit as a write-once ref beside the patch's
73 fn auto_detect_and_update( 24 /// event DAG. This is what keeps the objects behind a revision reachable once
25 /// the branch that produced them has been rebased away.
26 fn write_revision_ref(
74 repo: &Repository, 27 repo: &Repository,
75 ref_name: &str, 28 events_ref: &str,
76 patch: &mut PatchState, 29 number: u32,
77 sk: &ed25519_dalek::SigningKey, 30 commit: Oid,
78 ) -> Result<(), Error> { 31 ) -> Result<(), Error> {
79 if let Some(rev) = auto_detect_revision(repo, ref_name, patch, sk)? { 32 let name = state::patch_revision_ref(events_ref, number);
80 patch.revisions.push(rev); 33 if let Ok(existing) = repo.refname_to_id(&name) {
34 if existing != commit {
35 return Err(Error::Cmd(format!(
36 "revision {} already points at {} — revisions are write-once",
37 number, existing
38 )));
39 }
40 return Ok(());
81 } 41 }
42 repo.reference(&name, commit, false, "record revision")?;
82 Ok(()) 43 Ok(())
83 } 44 }
84 45
@@ -103,13 +64,19 @@ pub fn create(
103 .refname_to_id(&branch_ref) 64 .refname_to_id(&branch_ref)
104 .map_err(|e| crate::error::Error::Cmd(format!("branch '{}' not found: {}", branch, e)))?; 65 .map_err(|e| crate::error::Error::Cmd(format!("branch '{}' not found: {}", branch, e)))?;
105 66
106 // Check for duplicate: scan open patches for matching branch 67 // Duplicate detection is by commit, not by branch name. Patch identity is
68 // declared in the event DAG, so a branch name means nothing: two worktrees
69 // on the same generated name are not the same patch, and one worktree that
70 // renamed its branch has not stopped being one.
107 let patches = state::list_patches(repo)?; 71 let patches = state::list_patches(repo)?;
108 for p in &patches { 72 for p in &patches {
109 if p.status == PatchStatus::Open && p.branch == branch { 73 if p.status != PatchStatus::Open {
74 continue;
75 }
76 if p.revisions.last().map(|r| r.commit.as_str()) == Some(tip_oid.to_string().as_str()) {
110 return Err(crate::error::Error::Cmd(format!( 77 return Err(crate::error::Error::Cmd(format!(
111 "patch already exists for branch '{}'", 78 "patch {:.8} already stands at commit {:.8}; use `patch revise` to add a revision",
112 branch 79 p.id, tip_oid
113 ))); 80 )));
114 } 81 }
115 } 82 }
@@ -117,7 +84,7 @@ pub fn create(
117 // Get commit and tree OIDs for revision 1 84 // Get commit and tree OIDs for revision 1
118 let commit = repo.find_commit(tip_oid)?; 85 let commit = repo.find_commit(tip_oid)?;
119 let tree_oid = commit.tree()?.id(); 86 let tree_oid = commit.tree()?.id();
120 let base_oid = repo.refname_to_id(&format!("refs/heads/{}", base_ref))?; 87 let base_oid = revision_base(repo, base_ref, tip_oid)?;
121 88
122 let oid = dag::create_root_action( 89 let oid = dag::create_root_action(
123 repo, 90 repo,
@@ -133,8 +100,9 @@ pub fn create(
133 }, 100 },
134 )?; 101 )?;
135 let id = oid.to_string(); 102 let id = oid.to_string();
136 let ref_name = format!("refs/collab/patches/{}", id); 103 let events_ref = state::patch_events_ref(&id);
137 repo.reference(&ref_name, oid, false, "patch create")?; 104 repo.reference(&events_ref, oid, false, "patch create")?;
105 write_revision_ref(repo, &events_ref, 1, tip_oid)?;
138 Ok(id) 106 Ok(id)
139 } 107 }
140 108
@@ -147,8 +115,7 @@ pub struct ListEntry {
147 fn count_unread(repo: &git2::Repository, id: &str) -> Option<usize> { 115 fn count_unread(repo: &git2::Repository, id: &str) -> Option<usize> {
148 let seen_ref = format!("refs/collab/local/seen/patches/{}", id); 116 let seen_ref = format!("refs/collab/local/seen/patches/{}", id);
149 let seen_oid = repo.refname_to_id(&seen_ref).ok()?; 117 let seen_oid = repo.refname_to_id(&seen_ref).ok()?;
150 let ref_name = format!("refs/collab/patches/{}", id); 118 let tip = repo.refname_to_id(&state::patch_events_ref(id)).ok()?;
151 let tip = repo.refname_to_id(&ref_name).ok()?;
152 119
153 if seen_oid == tip { 120 if seen_oid == tip {
154 return Some(0); 121 return Some(0);
@@ -262,10 +229,7 @@ pub fn comment(
262 ) -> Result<(), crate::error::Error> { 229 ) -> Result<(), crate::error::Error> {
263 let sk = signing::load_signing_key(&signing::signing_key_dir()?)?; 230 let sk = signing::load_signing_key(&signing::signing_key_dir()?)?;
264 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; 231 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?;
265 let mut patch = PatchState::from_ref(repo, &ref_name, &id)?; 232 let patch = PatchState::from_ref(repo, &ref_name, &id)?;
266
267 // Auto-detect revision (runs for all comment types to record branch changes)
268 auto_detect_and_update(repo, &ref_name, &mut patch, &sk)?;
269 233
270 let author = get_author(repo)?; 234 let author = get_author(repo)?;
271 235
@@ -326,10 +290,7 @@ pub fn review(
326 ) -> Result<(), crate::error::Error> { 290 ) -> Result<(), crate::error::Error> {
327 let sk = signing::load_signing_key(&signing::signing_key_dir()?)?; 291 let sk = signing::load_signing_key(&signing::signing_key_dir()?)?;
328 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; 292 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?;
329 let mut patch = PatchState::from_ref(repo, &ref_name, &id)?; 293 let patch = PatchState::from_ref(repo, &ref_name, &id)?;
330
331 // Auto-detect revision
332 auto_detect_and_update(repo, &ref_name, &mut patch, &sk)?;
333 294
334 let rev = if let Some(target) = target_revision { 295 let rev = if let Some(target) = target_revision {
335 if !patch.revisions.iter().any(|r| r.number == target) { 296 if !patch.revisions.iter().any(|r| r.number == target) {
@@ -380,8 +341,8 @@ pub fn review(
380 clock: 0, 341 clock: 0,
381 }; 342 };
382 dag::append_event(repo, &ref_name, &close_event, &sk)?; 343 dag::append_event(repo, &ref_name, &close_event, &sk)?;
383 // Archive the ref 344 // Archive the whole subtree
384 if ref_name.starts_with("refs/collab/patches/") { 345 if ref_name.starts_with(state::PATCH_PREFIX) {
385 state::archive_patch_ref(repo, &id)?; 346 state::archive_patch_ref(repo, &id)?;
386 } 347 }
387 } 348 }
@@ -389,17 +350,31 @@ pub fn review(
389 Ok(()) 350 Ok(())
390 } 351 }
391 352
353 /// Record a new revision from `branch`, or from `HEAD` when none is named.
354 ///
355 /// `patch revise` is now the only way a revision is recorded. It used to share
356 /// the job with an auto-detection pass that fired as a side effect of `patch
357 /// comment` and `patch review`, which manufactured revisions the user never
358 /// asked for, timestamped at the comment rather than at the code change.
392 pub fn revise( 359 pub fn revise(
393 repo: &Repository, 360 repo: &Repository,
394 id_prefix: &str, 361 id_prefix: &str,
395 body: Option<&str>, 362 body: Option<&str>,
363 branch: Option<&str>,
396 ) -> Result<(), crate::error::Error> { 364 ) -> Result<(), crate::error::Error> {
397 let sk = signing::load_signing_key(&signing::signing_key_dir()?)?; 365 let sk = signing::load_signing_key(&signing::signing_key_dir()?)?;
398 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; 366 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?;
399 let patch = PatchState::from_ref(repo, &ref_name, &id)?; 367 let patch = PatchState::from_ref(repo, &ref_name, &id)?;
400 368
401 // Resolve current branch tip 369 let tip_oid = match branch {
402 let tip_oid = patch.resolve_head(repo)?; 370 Some(name) => repo
371 .refname_to_id(&format!("refs/heads/{}", name))
372 .map_err(|e| Error::Cmd(format!("branch '{}' not found: {}", name, e)))?,
373 None => repo
374 .head()?
375 .target()
376 .ok_or_else(|| Error::Cmd("cannot determine HEAD OID".to_string()))?,
377 };
403 let tip_hex = tip_oid.to_string(); 378 let tip_hex = tip_oid.to_string();
404 let last_commit = patch 379 let last_commit = patch
405 .revisions 380 .revisions
@@ -417,6 +392,8 @@ pub fn revise(
417 392
418 let commit = repo.find_commit(tip_oid)?; 393 let commit = repo.find_commit(tip_oid)?;
419 let tree_oid = commit.tree()?.id(); 394 let tree_oid = commit.tree()?.id();
395 let base_oid = revision_base(repo, &patch.base_ref, tip_oid)?;
396 let number = patch.revisions.last().map(|r| r.number).unwrap_or(0) + 1;
420 let author = get_author(repo)?; 397 let author = get_author(repo)?;
421 let event = Event { 398 let event = Event {
422 timestamp: chrono::Utc::now().to_rfc3339(), 399 timestamp: chrono::Utc::now().to_rfc3339(),
@@ -425,10 +402,12 @@ pub fn revise(
425 commit: tip_hex, 402 commit: tip_hex,
426 tree: tree_oid.to_string(), 403 tree: tree_oid.to_string(),
427 body: body.map(|s| s.to_string()), 404 body: body.map(|s| s.to_string()),
405 base: Some(base_oid.to_string()),
428 }, 406 },
429 clock: 0, 407 clock: 0,
430 }; 408 };
431 dag::append_event(repo, &ref_name, &event, &sk)?; 409 dag::append_event(repo, &ref_name, &event, &sk)?;
410 write_revision_ref(repo, &ref_name, number, tip_oid)?;
432 Ok(()) 411 Ok(())
433 } 412 }
434 413
@@ -678,8 +657,10 @@ pub fn checkout(repo: &Repository, id_prefix: &str) -> Result<(), crate::error::
678 } 657 }
679 658
680 pub fn delete(repo: &Repository, id_prefix: &str) -> Result<String, crate::error::Error> { 659 pub fn delete(repo: &Repository, id_prefix: &str) -> Result<String, crate::error::Error> {
681 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; 660 let (_ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?;
682 repo.find_reference(&ref_name)?.delete()?; 661 // A patch is a subtree — its events ref plus every revision ref — so
662 // deleting one ref would leave the revisions behind.
663 state::delete_patch_refs(repo, &id)?;
683 Ok(id) 664 Ok(id)
684 } 665 }
685 666
@@ -696,8 +677,8 @@ pub fn close(
696 reason: reason.map(|s| s.to_string()), 677 reason: reason.map(|s| s.to_string()),
697 }, 678 },
698 )?; 679 )?;
699 // Archive the ref (move to refs/collab/archive/patches/) 680 // Archive the whole subtree (move to refs/collab/archive/patches/)
700 if ref_name.starts_with("refs/collab/patches/") { 681 if ref_name.starts_with(state::PATCH_PREFIX) {
701 state::archive_patch_ref(repo, &id)?; 682 state::archive_patch_ref(repo, &id)?;
702 } 683 }
703 Ok(()) 684 Ok(())
src/state.rs
Old New
@@ -190,6 +190,13 @@ pub struct Revision {
190 pub tree: String, 190 pub tree: String,
191 pub body: Option<String>, 191 pub body: Option<String>,
192 pub timestamp: String, 192 pub timestamp: String,
193 /// Merge-base of the patch's base branch and `commit` when this revision
194 /// was recorded. `None` for revisions written before it was stored; those
195 /// fall back to recomputing it against the base branch as it stands now,
196 /// which is what every patch used to do. "Did the author rebase between
197 /// these two revisions" is exactly "did this change".
198 #[serde(default)]
199 pub base: Option<String>,
193 } 200 }
194 201
195 impl Revision { 202 impl Revision {
@@ -226,10 +233,10 @@ pub struct PatchState {
226 pub status: PatchStatus, 233 pub status: PatchStatus,
227 pub base_ref: String, 234 pub base_ref: String,
228 pub fixes: Option<String>, 235 pub fixes: Option<String>,
236 /// The branch the patch was created from, recorded for provenance only.
237 /// Nothing resolves through it any more: a patch is addressed by its own
238 /// revision refs, so an ephemeral or rewritten branch costs it nothing.
229 pub branch: String, 239 pub branch: String,
230 /// Base branch tip OID at patch creation time (None for old patches).
231 #[serde(default, skip_serializing_if = "Option::is_none")]
232 pub base_commit: Option<String>,
233 pub comments: Vec<Comment>, 240 pub comments: Vec<Comment>,
234 pub inline_comments: Vec<InlineComment>, 241 pub inline_comments: Vec<InlineComment>,
235 pub reviews: Vec<Review>, 242 pub reviews: Vec<Review>,
@@ -468,21 +475,47 @@ impl IssueState {
468 } 475 }
469 476
470 impl PatchState { 477 impl PatchState {
471 /// Resolve the current head commit OID for this patch by looking up `refs/heads/{branch}`. 478 /// The commit the patch currently stands at: the latest revision whose
479 /// commit was recorded and whose objects are still present. A revision ref
480 /// keeps those objects reachable, so this no longer depends on any
481 /// `refs/heads/*` ref surviving a rebase.
482 ///
483 /// The branch fallback is for patches old enough to have recorded no
484 /// commits at all — either a branch name or, in the oldest shape, a raw OID
485 /// stored where the branch name now lives.
472 pub fn resolve_head(&self, repo: &Repository) -> Result<Oid, crate::error::Error> { 486 pub fn resolve_head(&self, repo: &Repository) -> Result<Oid, crate::error::Error> {
473 // Try parsing as a hex OID first (for patches created with head_commit) 487 for rev in self.revisions.iter().rev() {
488 if let Ok(oid) = Oid::from_str(&rev.commit) {
489 if repo.find_commit(oid).is_ok() {
490 return Ok(oid);
491 }
492 }
493 }
474 if let Ok(oid) = Oid::from_str(&self.branch) { 494 if let Ok(oid) = Oid::from_str(&self.branch) {
475 if repo.find_commit(oid).is_ok() { 495 if repo.find_commit(oid).is_ok() {
476 return Ok(oid); 496 return Ok(oid);
477 } 497 }
478 } 498 }
479 // Fall back to branch name lookup
480 let ref_name = format!("refs/heads/{}", self.branch); 499 let ref_name = format!("refs/heads/{}", self.branch);
481 repo.refname_to_id(&ref_name).map_err(|e| { 500 repo.refname_to_id(&ref_name).map_err(|e| {
482 crate::error::Error::Cmd(format!("branch '{}' not found: {}", self.branch, e)) 501 crate::error::Error::Cmd(format!(
502 "patch has no recorded revision commit and branch '{}' was not found: {}",
503 self.branch, e
504 ))
483 }) 505 })
484 } 506 }
485 507
508 /// Where the latest revision branched off the base branch. Revisions
509 /// written before `base` was stored fall back to recomputing the
510 /// merge-base against the base branch as it stands now, which is what
511 /// every patch used to do at display time.
512 fn latest_base(&self, repo: &Repository, base_tip: Oid, head: Oid) -> Option<Oid> {
513 match self.revisions.last().and_then(|r| r.base.as_deref()) {
514 Some(base) => Oid::from_str(base).ok(),
515 None => repo.merge_base(base_tip, head).ok(),
516 }
517 }
518
486 /// Compute staleness: how many commits the branch is ahead of base, 519 /// Compute staleness: how many commits the branch is ahead of base,
487 /// and how many commits the base is ahead of the branch. 520 /// and how many commits the base is ahead of the branch.
488 /// Returns (ahead, behind). 521 /// Returns (ahead, behind).
@@ -496,9 +529,15 @@ impl PatchState {
496 529
497 /// Auto-detect merge: if the patch is still Open and its head is 530 /// Auto-detect merge: if the patch is still Open and its head is
498 /// reachable from the base branch tip, the user merged it outside of 531 /// reachable from the base branch tip, the user merged it outside of
499 /// git-collab. We compare the current base tip to the base_commit 532 /// git-collab. We compare the current base tip to where the *latest*
500 /// recorded at creation time — if it has moved to include the patch 533 /// revision stood — if the base has moved to include the patch head, the
501 /// head, the patch is merged. 534 /// patch is merged.
535 ///
536 /// Anchoring this to the latest revision rather than to creation matters
537 /// twice over. It survives a rebase, which moves the base out from under
538 /// an earlier revision; and it no longer routes through
539 /// `refs/heads/<branch>`, which used to make detection no-op silently
540 /// whenever the source branch had been deleted or was never pushed.
502 fn check_auto_merge(&mut self, repo: &Repository) { 541 fn check_auto_merge(&mut self, repo: &Repository) {
503 if self.status != PatchStatus::Open { 542 if self.status != PatchStatus::Open {
504 return; 543 return;
@@ -510,11 +549,10 @@ impl PatchState {
510 let Ok(base_tip) = repo.refname_to_id(&base_ref) else { 549 let Ok(base_tip) = repo.refname_to_id(&base_ref) else {
511 return; 550 return;
512 }; 551 };
513 let base_moved = self 552 let Some(base_at_revision) = self.latest_base(repo, base_tip, patch_head) else {
514 .base_commit 553 return;
515 .as_ref() 554 };
516 .map(|bc| bc != &base_tip.to_string()) 555 let base_moved = base_at_revision != base_tip;
517 .unwrap_or(false);
518 let reachable = base_tip == patch_head 556 let reachable = base_tip == patch_head
519 || repo 557 || repo
520 .graph_descendant_of(base_tip, patch_head) 558 .graph_descendant_of(base_tip, patch_head)
@@ -587,6 +625,7 @@ impl PatchState {
587 tree, 625 tree,
588 body: None, 626 body: None,
589 timestamp: event.timestamp.clone(), 627 timestamp: event.timestamp.clone(),
628 base: base_commit,
590 }]; 629 }];
591 state = Some(PatchState { 630 state = Some(PatchState {
592 id: id.to_string(), 631 id: id.to_string(),
@@ -596,7 +635,6 @@ impl PatchState {
596 base_ref, 635 base_ref,
597 fixes, 636 fixes,
598 branch, 637 branch,
599 base_commit,
600 comments: Vec::new(), 638 comments: Vec::new(),
601 inline_comments: Vec::new(), 639 inline_comments: Vec::new(),
602 reviews: Vec::new(), 640 reviews: Vec::new(),
@@ -606,7 +644,12 @@ impl PatchState {
606 author: event.author.clone(), 644 author: event.author.clone(),
607 }); 645 });
608 } 646 }
609 Action::PatchRevision { commit, tree, body } => { 647 Action::PatchRevision {
648 commit,
649 tree,
650 body,
651 base,
652 } => {
610 if let Some(ref mut s) = state { 653 if let Some(ref mut s) = state {
611 // Dedup by commit OID — skip if already seen. Legacy 654 // Dedup by commit OID — skip if already seen. Legacy
612 // revisions recorded no commit, and collapsing those 655 // revisions recorded no commit, and collapsing those
@@ -622,6 +665,7 @@ impl PatchState {
622 tree, 665 tree,
623 body, 666 body,
624 timestamp: event.timestamp.clone(), 667 timestamp: event.timestamp.clone(),
668 base,
625 }); 669 });
626 } 670 }
627 } 671 }
@@ -744,6 +788,39 @@ impl PatchState {
744 } 788 }
745 } 789 }
746 790
791 /// A patch owns a subtree of refs, not a single ref:
792 ///
793 /// ```text
794 /// refs/collab/patches/<id>/events the event DAG
795 /// refs/collab/patches/<id>/r/<n> revision n's commit, write-once
796 /// ```
797 ///
798 /// The `events` suffix is forced — git will not let `<id>` be both a ref and a
799 /// directory — and the split is what lets a revision's objects stay reachable
800 /// once the branch that produced them has been rebased away.
801 pub const PATCH_PREFIX: &str = "refs/collab/patches/";
802 pub const ARCHIVE_PATCH_PREFIX: &str = "refs/collab/archive/patches/";
803
804 /// The event DAG ref for a patch in the active namespace.
805 pub fn patch_events_ref(id: &str) -> String {
806 format!("{}{}/events", PATCH_PREFIX, id)
807 }
808
809 /// The ref holding revision `n`'s commit, under whichever namespace the
810 /// patch's events ref lives in.
811 pub fn patch_revision_ref(events_ref: &str, n: u32) -> String {
812 let base = events_ref.strip_suffix("/events").unwrap_or(events_ref);
813 format!("{}/r/{}", base, n)
814 }
815
816 /// Split a ref name under a patch prefix into (id, suffix), where suffix is
817 /// `events` or `r/<n>`. Returns `None` for the pre-migration single-ref layout,
818 /// which has no suffix at all.
819 fn split_patch_ref<'a>(ref_name: &'a str, prefix: &str) -> Option<(&'a str, &'a str)> {
820 let rest = ref_name.strip_prefix(prefix)?;
821 rest.split_once('/')
822 }
823
747 /// Enumerate collab refs under a given prefix, returning (ref_name, id) pairs. 824 /// Enumerate collab refs under a given prefix, returning (ref_name, id) pairs.
748 fn refs_under( 825 fn refs_under(
749 repo: &Repository, 826 repo: &Repository,
@@ -764,18 +841,48 @@ fn refs_under(
764 Ok(result) 841 Ok(result)
765 } 842 }
766 843
844 /// Enumerate the event DAG ref of every patch under a prefix, returning
845 /// (ref_name, id) pairs. Only `<id>/events` counts as a patch; the revision
846 /// refs beside it are commits, not DAGs.
847 fn patch_refs_under(
848 repo: &Repository,
849 prefix: &str,
850 ) -> Result<Vec<(String, String)>, crate::error::Error> {
851 let refs = repo.references_glob(&format!("{}*/events", prefix))?;
852 let mut result = Vec::new();
853 for r in refs {
854 let r = r?;
855 let ref_name = r.name().unwrap_or_default().to_string();
856 if let Some((id, "events")) = split_patch_ref(&ref_name, prefix) {
857 let id = id.to_string();
858 result.push((ref_name, id));
859 }
860 }
861 Ok(result)
862 }
863
767 fn collab_refs( 864 fn collab_refs(
768 repo: &Repository, 865 repo: &Repository,
769 kind: &str, 866 kind: &str,
770 ) -> Result<Vec<(String, String)>, crate::error::Error> { 867 ) -> Result<Vec<(String, String)>, crate::error::Error> {
771 refs_under(repo, &format!("refs/collab/{}/", kind)) 868 let prefix = format!("refs/collab/{}/", kind);
869 if kind == "patches" {
870 patch_refs_under(repo, &prefix)
871 } else {
872 refs_under(repo, &prefix)
873 }
772 } 874 }
773 875
774 fn collab_archive_refs( 876 fn collab_archive_refs(
775 repo: &Repository, 877 repo: &Repository,
776 kind: &str, 878 kind: &str,
777 ) -> Result<Vec<(String, String)>, crate::error::Error> { 879 ) -> Result<Vec<(String, String)>, crate::error::Error> {
778 refs_under(repo, &format!("refs/collab/archive/{}/", kind)) 880 let prefix = format!("refs/collab/archive/{}/", kind);
881 if kind == "patches" {
882 patch_refs_under(repo, &prefix)
883 } else {
884 refs_under(repo, &prefix)
885 }
779 } 886 }
780 887
781 /// Resolve a short ID prefix to a full ref. Searches both active and archive namespaces. 888 /// Resolve a short ID prefix to a full ref. Searches both active and archive namespaces.
@@ -839,6 +946,7 @@ pub fn list_issues(repo: &Repository) -> Result<Vec<IssueState>, crate::error::E
839 946
840 /// List active patch refs, excluding any that also have an archived ref. 947 /// List active patch refs, excluding any that also have an archived ref.
841 pub fn list_patches(repo: &Repository) -> Result<Vec<PatchState>, crate::error::Error> { 948 pub fn list_patches(repo: &Repository) -> Result<Vec<PatchState>, crate::error::Error> {
949 migrate_patch_layout(repo);
842 let archived_ids: std::collections::HashSet<String> = collab_archive_refs(repo, "patches")? 950 let archived_ids: std::collections::HashSet<String> = collab_archive_refs(repo, "patches")?
843 .into_iter() 951 .into_iter()
844 .map(|(_, id)| id) 952 .map(|(_, id)| id)
@@ -888,6 +996,7 @@ pub fn list_issues_with_archived(
888 pub fn list_patches_with_archived( 996 pub fn list_patches_with_archived(
889 repo: &Repository, 997 repo: &Repository,
890 ) -> Result<Vec<PatchState>, crate::error::Error> { 998 ) -> Result<Vec<PatchState>, crate::error::Error> {
999 migrate_patch_layout(repo);
891 let mut seen = std::collections::HashSet::new(); 1000 let mut seen = std::collections::HashSet::new();
892 let mut items = Vec::new(); 1001 let mut items = Vec::new();
893 1002
@@ -930,13 +1039,121 @@ pub fn unarchive_issue_ref(repo: &Repository, id: &str) -> Result<(), crate::err
930 Ok(()) 1039 Ok(())
931 } 1040 }
932 1041
933 /// Move a patch ref from active to archive namespace. 1042 /// Every ref in a patch's subtree under `prefix`, as (ref_name, suffix) pairs.
1043 fn patch_subtree(
1044 repo: &Repository,
1045 prefix: &str,
1046 id: &str,
1047 ) -> Result<Vec<(String, String)>, crate::error::Error> {
1048 let refs = repo.references_glob(&format!("{}{}/*", prefix, id))?;
1049 let mut result = Vec::new();
1050 for r in refs {
1051 let r = r?;
1052 let ref_name = r.name().unwrap_or_default().to_string();
1053 if let Some((found, suffix)) = split_patch_ref(&ref_name, prefix) {
1054 if found == id {
1055 let suffix = suffix.to_string();
1056 result.push((ref_name, suffix));
1057 }
1058 }
1059 }
1060 Ok(result)
1061 }
1062
1063 /// Move a patch's whole subtree from active to archive namespace. A patch is
1064 /// `<id>/events` plus every `<id>/r/<n>`, and moving only the events ref would
1065 /// leave a closed patch with no revisions to review.
934 pub fn archive_patch_ref(repo: &Repository, id: &str) -> Result<(), crate::error::Error> { 1066 pub fn archive_patch_ref(repo: &Repository, id: &str) -> Result<(), crate::error::Error> {
935 let old_ref = format!("refs/collab/patches/{}", id); 1067 for (old_ref, suffix) in patch_subtree(repo, PATCH_PREFIX, id)? {
936 let oid = repo.refname_to_id(&old_ref)?; 1068 let oid = repo.refname_to_id(&old_ref)?;
937 let new_ref = format!("refs/collab/archive/patches/{}", id); 1069 let new_ref = format!("{}{}/{}", ARCHIVE_PATCH_PREFIX, id, suffix);
938 repo.reference(&new_ref, oid, false, "archive patch")?; 1070 repo.reference(&new_ref, oid, true, "archive patch")?;
939 repo.find_reference(&old_ref)?.delete()?; 1071 repo.find_reference(&old_ref)?.delete()?;
1072 }
1073 Ok(())
1074 }
1075
1076 /// Delete every ref belonging to a patch, in either namespace.
1077 pub fn delete_patch_refs(repo: &Repository, id: &str) -> Result<(), crate::error::Error> {
1078 for prefix in [PATCH_PREFIX, ARCHIVE_PATCH_PREFIX] {
1079 for (ref_name, _) in patch_subtree(repo, prefix, id)? {
1080 repo.find_reference(&ref_name)?.delete()?;
1081 }
1082 }
1083 Ok(())
1084 }
1085
1086 /// Bring patches written in the pre-revision-refs layout — a single ref at
1087 /// `refs/collab/patches/<id>` — up to the current one. Called from every entry
1088 /// point that enumerates or resolves a patch, so an old repository migrates on
1089 /// first use rather than breaking.
1090 ///
1091 /// Failures are reported and skipped rather than propagated: one unmigratable
1092 /// patch must not make the whole list unreadable.
1093 pub fn migrate_patch_layout(repo: &Repository) {
1094 for prefix in [PATCH_PREFIX, ARCHIVE_PATCH_PREFIX] {
1095 let Ok(refs) = repo.references_glob(&format!("{}*", prefix)) else {
1096 continue;
1097 };
1098 // Collect first: the migration rewrites refs, and mutating the ref
1099 // store while iterating it is not sound.
1100 let old_layout: Vec<String> = refs
1101 .filter_map(|r| {
1102 let name = r.ok()?.name()?.to_string();
1103 // The new layout always has a suffix; the old one never does.
1104 match split_patch_ref(&name, prefix) {
1105 Some(_) => None,
1106 None => Some(name),
1107 }
1108 })
1109 .collect();
1110 for old_ref in old_layout {
1111 let id = old_ref.strip_prefix(prefix).unwrap_or_default().to_string();
1112 if let Err(e) = migrate_one_patch(repo, prefix, &id, &old_ref) {
1113 eprintln!("warning: could not migrate patch {:.8}: {}", id, e);
1114 }
1115 }
1116 }
1117 }
1118
1119 fn migrate_one_patch(
1120 repo: &Repository,
1121 prefix: &str,
1122 id: &str,
1123 old_ref: &str,
1124 ) -> Result<(), crate::error::Error> {
1125 let oid = repo.refname_to_id(old_ref)?;
1126
1127 // Read the revision list off the old ref before touching anything, so a
1128 // patch whose DAG will not materialize is left exactly as it was found.
1129 let state = PatchState::from_ref_uncached(repo, old_ref, id)?;
1130
1131 // The old ref and the new subtree cannot coexist: git refuses to have
1132 // `<id>` be both a ref and a directory. Park the tip somewhere outside the
1133 // patch namespace so an interrupted migration is still recoverable, then
1134 // do the delete-and-recreate.
1135 let parked = format!("refs/collab/local/migrating/patches/{}", id);
1136 repo.reference(&parked, oid, true, "migrate patch: park tip")?;
1137 repo.find_reference(old_ref)?.delete()?;
1138
1139 let events_ref = format!("{}{}/events", prefix, id);
1140 repo.reference(&events_ref, oid, true, "migrate patch: events")?;
1141
1142 // Give every revision whose commit is still present a ref of its own.
1143 // Revisions whose objects were already lost to a force-push cannot be
1144 // recovered and keep the `""`-means-unknown convention.
1145 for rev in &state.revisions {
1146 let Ok(commit) = Oid::from_str(&rev.commit) else {
1147 continue;
1148 };
1149 if repo.find_commit(commit).is_err() {
1150 continue;
1151 }
1152 let name = patch_revision_ref(&events_ref, rev.number);
1153 repo.reference(&name, commit, true, "migrate patch: revision")?;
1154 }
1155
1156 repo.find_reference(&parked)?.delete()?;
940 Ok(()) 1157 Ok(())
941 } 1158 }
942 1159
@@ -953,6 +1170,7 @@ pub fn resolve_patch_ref(
953 repo: &Repository, 1170 repo: &Repository,
954 prefix: &str, 1171 prefix: &str,
955 ) -> Result<(String, String), crate::error::Error> { 1172 ) -> Result<(String, String), crate::error::Error> {
1173 migrate_patch_layout(repo);
956 resolve_ref(repo, "patches", "patch", prefix) 1174 resolve_ref(repo, "patches", "patch", prefix)
957 } 1175 }
958 1176
src/sync.rs
Old New
@@ -612,6 +612,46 @@ fn push_refs(workdir: &Path, remote_name: &str, refs: &[String]) -> SyncResult {
612 } 612 }
613 } 613 }
614 614
615 /// What a fetched sync ref is, and where it belongs locally.
616 enum SyncRef {
617 /// An event DAG. Verified, then reconciled against the local ref.
618 Events { local_ref: String },
619 /// A patch revision: an ordinary source commit, not an event, so it
620 /// carries no signature to verify and is never merged. Write-once.
621 Revision { local_ref: String },
622 }
623
624 /// Classify a fetched sync ref by the part of its name after the kind prefix,
625 /// returning the patch/issue id alongside it. Patches own a subtree —
626 /// `<id>/events` and `<id>/r/<n>` — while issues remain a single ref.
627 ///
628 /// A bare `<id>` under `patches` is the pre-revision-refs layout, still pushed
629 /// by peers that have not migrated; adopting it as the events ref migrates it
630 /// on the way in.
631 fn classify_sync_ref(kind: &str, rest: &str) -> Option<(String, SyncRef)> {
632 if kind != "patches" {
633 return Some((
634 rest.to_string(),
635 SyncRef::Events {
636 local_ref: format!("refs/collab/{}/{}", kind, rest),
637 },
638 ));
639 }
640 let (id, suffix) = match rest.split_once('/') {
641 Some((id, suffix)) => (id, suffix),
642 None => (rest, "events"),
643 };
644 let local_ref = format!("refs/collab/patches/{}/{}", id, suffix);
645 let classified = if suffix == "events" {
646 SyncRef::Events { local_ref }
647 } else if suffix.starts_with("r/") {
648 SyncRef::Revision { local_ref }
649 } else {
650 return None;
651 };
652 Some((id.to_string(), classified))
653 }
654
615 /// Reconcile all refs of a given kind (issues or patches) from sync refs. 655 /// Reconcile all refs of a given kind (issues or patches) from sync refs.
616 fn reconcile_refs( 656 fn reconcile_refs(
617 repo: &Repository, 657 repo: &Repository,
@@ -620,13 +660,14 @@ fn reconcile_refs(
620 signing_key: &ed25519_dalek::SigningKey, 660 signing_key: &ed25519_dalek::SigningKey,
621 ) -> Result<(), Error> { 661 ) -> Result<(), Error> {
622 let sync_prefix = format!("refs/collab/sync/{}/", kind); 662 let sync_prefix = format!("refs/collab/sync/{}/", kind);
623 let sync_refs: Vec<(String, String)> = { 663 let sync_refs: Vec<(String, String, SyncRef)> = {
624 let refs = repo.references_glob(&format!("{}*", sync_prefix))?; 664 let refs = repo.references_glob(&format!("{}*", sync_prefix))?;
625 refs.filter_map(|r| { 665 refs.filter_map(|r| {
626 let r = r.ok()?; 666 let r = r.ok()?;
627 let name = r.name()?.to_string(); 667 let name = r.name()?.to_string();
628 let id = name.strip_prefix(&sync_prefix)?.to_string(); 668 let rest = name.strip_prefix(&sync_prefix)?;
629 Some((name, id)) 669 let (id, classified) = classify_sync_ref(kind, rest)?;
670 Some((name, id, classified))
630 }) 671 })
631 .collect() 672 .collect()
632 }; 673 };
@@ -635,13 +676,31 @@ fn reconcile_refs(
635 let trust_policy = trust::load_trust_policy(repo)?; 676 let trust_policy = trust::load_trust_policy(repo)?;
636 let mut warned_unconfigured = false; 677 let mut warned_unconfigured = false;
637 678
638 for (remote_ref, id) in &sync_refs { 679 for (remote_ref, id, classified) in &sync_refs {
639 // Validate the ref ID format before processing 680 // Validate the ref ID format before processing
640 if let Err(e) = validate_collab_ref_id(id) { 681 if let Err(e) = validate_collab_ref_id(id) {
641 eprintln!(" Skipping {} with invalid ref ID {:.8}: {}", kind, id, e); 682 eprintln!(" Skipping {} with invalid ref ID {:.8}: {}", kind, id, e);
642 continue; 683 continue;
643 } 684 }
644 685
686 // A revision ref points at the author's source commit. It carries no
687 // event signature, and it is write-once: an existing local revision is
688 // never overwritten by a remote one claiming the same number.
689 if let SyncRef::Revision { local_ref } = classified {
690 let oid = repo.refname_to_id(remote_ref)?;
691 match repo.refname_to_id(local_ref) {
692 Ok(existing) if existing != oid => eprintln!(
693 " Keeping local {} (remote claims {}); revisions are write-once",
694 local_ref, oid
695 ),
696 Ok(_) => {}
697 Err(_) => {
698 repo.reference(local_ref, oid, false, "sync: new revision from remote")?;
699 }
700 }
701 continue;
702 }
703
645 // Verify all commits on the remote ref before reconciling 704 // Verify all commits on the remote ref before reconciling
646 match signing::verify_ref(repo, remote_ref) { 705 match signing::verify_ref(repo, remote_ref) {
647 Ok(results) => { 706 Ok(results) => {
@@ -677,9 +736,11 @@ fn reconcile_refs(
677 } 736 }
678 } 737 }
679 738
680 let local_ref = format!("refs/collab/{}/{}", kind, id); 739 let SyncRef::Events { local_ref } = classified else {
681 if repo.refname_to_id(&local_ref).is_ok() { 740 continue;
682 match dag::reconcile(repo, &local_ref, remote_ref, author, signing_key) { 741 };
742 if repo.refname_to_id(local_ref).is_ok() {
743 match dag::reconcile(repo, local_ref, remote_ref, author, signing_key) {
683 Ok((_oid, outcome)) => { 744 Ok((_oid, outcome)) => {
684 let action = match outcome { 745 let action = match outcome {
685 dag::ReconcileOutcome::AlreadyCurrent => "already current", 746 dag::ReconcileOutcome::AlreadyCurrent => "already current",
@@ -693,7 +754,7 @@ fn reconcile_refs(
693 } 754 }
694 } else { 755 } else {
695 let oid = repo.refname_to_id(remote_ref)?; 756 let oid = repo.refname_to_id(remote_ref)?;
696 repo.reference(&local_ref, oid, false, "sync: new from remote")?; 757 repo.reference(local_ref, oid, false, "sync: new from remote")?;
697 println!(" New {} {:.8} from remote", kind, id); 758 println!(" New {} {:.8} from remote", kind, id);
698 } 759 }
699 } 760 }
src/tui/mod.rs
Old New
@@ -80,7 +80,6 @@ mod tests {
80 base_ref: "main".into(), 80 base_ref: "main".into(),
81 fixes: None, 81 fixes: None,
82 branch: format!("feature/{}", id), 82 branch: format!("feature/{}", id),
83 base_commit: None,
84 comments: vec![], 83 comments: vec![],
85 inline_comments: vec![], 84 inline_comments: vec![],
86 reviews: vec![], 85 reviews: vec![],
@@ -277,7 +276,6 @@ mod tests {
277 base_ref: "main".to_string(), 276 base_ref: "main".to_string(),
278 fixes: None, 277 fixes: None,
279 branch: format!("feature/p{:07x}", i), 278 branch: format!("feature/p{:07x}", i),
280 base_commit: None,
281 comments: Vec::new(), 279 comments: Vec::new(),
282 inline_comments: Vec::new(), 280 inline_comments: Vec::new(),
283 reviews: Vec::new(), 281 reviews: Vec::new(),
@@ -1034,7 +1032,6 @@ mod tests {
1034 base_ref: "main".into(), 1032 base_ref: "main".into(),
1035 fixes: Some("i1".into()), 1033 fixes: Some("i1".into()),
1036 branch: "feature/fix-thing".into(), 1034 branch: "feature/fix-thing".into(),
1037 base_commit: None,
1038 comments: vec![crate::state::Comment { 1035 comments: vec![crate::state::Comment {
1039 author: make_author(), 1036 author: make_author(),
1040 body: "Thread comment".into(), 1037 body: "Thread comment".into(),
@@ -1063,6 +1060,7 @@ mod tests {
1063 tree: "bbbb1111bbbb1111bbbb1111bbbb1111bbbb1111".into(), 1060 tree: "bbbb1111bbbb1111bbbb1111bbbb1111bbbb1111".into(),
1064 body: None, 1061 body: None,
1065 timestamp: "2026-01-01T00:00:00Z".into(), 1062 timestamp: "2026-01-01T00:00:00Z".into(),
1063 base: None,
1066 }, 1064 },
1067 Revision { 1065 Revision {
1068 number: 2, 1066 number: 2,
@@ -1070,6 +1068,7 @@ mod tests {
1070 tree: "bbbb2222bbbb2222bbbb2222bbbb2222bbbb2222".into(), 1068 tree: "bbbb2222bbbb2222bbbb2222bbbb2222bbbb2222".into(),
1071 body: Some("Addressed review comments".into()), 1069 body: Some("Addressed review comments".into()),
1072 timestamp: "2026-01-02T00:00:00Z".into(), 1070 timestamp: "2026-01-02T00:00:00Z".into(),
1071 base: None,
1073 }, 1072 },
1074 ], 1073 ],
1075 created_at: "2026-01-01T00:00:00Z".into(), 1074 created_at: "2026-01-01T00:00:00Z".into(),
src/tui/widgets.rs
Old New
@@ -71,7 +71,9 @@ pub(crate) fn format_event_detail(oid: &Oid, event: &crate::event::Event) -> Str
71 detail.push_str(&format!("\n{}\n", body)); 71 detail.push_str(&format!("\n{}\n", body));
72 } 72 }
73 } 73 }
74 Action::PatchRevision { commit, tree, body } => { 74 Action::PatchRevision {
75 commit, tree, body, ..
76 } => {
75 detail.push_str(&format!("\nCommit: {}\n", commit)); 77 detail.push_str(&format!("\nCommit: {}\n", commit));
76 detail.push_str(&format!("Tree: {}\n", tree)); 78 detail.push_str(&format!("Tree: {}\n", tree));
77 if let Some(b) = body { 79 if let Some(b) = body {
tests/adversarial_test.rs
Old New
@@ -550,6 +550,7 @@ fn arb_action() -> impl Strategy<Value = Action> {
550 commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), 550 commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(),
551 tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(), 551 tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(),
552 body, 552 body,
553 base: None,
553 }), 554 }),
554 (".*",).prop_map(|(body,)| Action::PatchComment { body }), 555 (".*",).prop_map(|(body,)| Action::PatchComment { body }),
555 Just(Action::PatchMerge), 556 Just(Action::PatchMerge),
tests/archive_test.rs
Old New
@@ -142,8 +142,8 @@ fn test_close_patch_moves_ref_to_archive() {
142 // Old ref should be gone 142 // Old ref should be gone
143 assert!(repo.refname_to_id(&ref_name).is_err()); 143 assert!(repo.refname_to_id(&ref_name).is_err());
144 144
145 // New archive ref should exist 145 // New archive ref should exist, under the patch's own subtree
146 let archive_ref = format!("refs/collab/archive/patches/{}", id); 146 let archive_ref = format!("refs/collab/archive/patches/{}/events", id);
147 assert!(repo.refname_to_id(&archive_ref).is_ok()); 147 assert!(repo.refname_to_id(&archive_ref).is_ok());
148 148
149 // Should still be able to materialize state from archive ref 149 // Should still be able to materialize state from archive ref
tests/cli_test.rs
Old New
@@ -1088,7 +1088,15 @@ fn test_full_patch_review_cycle() {
1088 ); 1088 );
1089 repo.git(&["checkout", "main"]); 1089 repo.git(&["checkout", "main"]);
1090 1090
1091 repo.run_ok(&["patch", "revise", &id, "-b", "Added documentation"]); 1091 repo.run_ok(&[
1092 "patch",
1093 "revise",
1094 &id,
1095 "-b",
1096 "Added documentation",
1097 "-B",
1098 "feature",
1099 ]);
1092 1100
1093 // Approve 1101 // Approve
1094 repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM now"]); 1102 repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM now"]);
tests/collab_test.rs
Old New
@@ -340,6 +340,7 @@ fn test_patch_review_workflow() {
340 commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), 340 commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(),
341 tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(), 341 tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(),
342 body: Some("Updated implementation".to_string()), 342 body: Some("Updated implementation".to_string()),
343 base: None,
343 }, 344 },
344 clock: 0, 345 clock: 0,
345 }; 346 };
@@ -659,7 +660,10 @@ fn add_commit_on_branch(
659 .unwrap() 660 .unwrap()
660 } 661 }
661 662
662 /// Create a branch-based patch using DAG primitives. 663 /// Create a branch-based patch using DAG primitives. The recorded commit is a
664 /// placeholder that is not in the object database, so these patches exercise
665 /// `resolve_head`'s legacy branch fallback — the path taken by patches old
666 /// enough to have recorded no usable revision commit.
663 fn create_branch_patch( 667 fn create_branch_patch(
664 repo: &git2::Repository, 668 repo: &git2::Repository,
665 author: &Author, 669 author: &Author,
@@ -685,7 +689,7 @@ fn create_branch_patch(
685 }; 689 };
686 let oid = dag::create_root_event(repo, &event, &sk).unwrap(); 690 let oid = dag::create_root_event(repo, &event, &sk).unwrap();
687 let id = oid.to_string(); 691 let id = oid.to_string();
688 let patch_ref = format!("refs/collab/patches/{}", id); 692 let patch_ref = git_collab::state::patch_events_ref(&id);
689 repo.reference(&patch_ref, oid, false, "test branch patch") 693 repo.reference(&patch_ref, oid, false, "test branch patch")
690 .unwrap(); 694 .unwrap();
691 (patch_ref, id) 695 (patch_ref, id)
@@ -697,6 +701,8 @@ fn create_branch_patch(
697 701
698 #[test] 702 #[test]
699 fn test_resolve_head_branch_based() { 703 fn test_resolve_head_branch_based() {
704 // Legacy fallback: with no usable revision commit recorded, the head is
705 // still read off `refs/heads/<branch>` and still follows it.
700 let tmp = TempDir::new().unwrap(); 706 let tmp = TempDir::new().unwrap();
701 let repo = init_repo(tmp.path(), &alice()); 707 let repo = init_repo(tmp.path(), &alice());
702 708
@@ -727,6 +733,8 @@ fn test_resolve_head_branch_based() {
727 733
728 #[test] 734 #[test]
729 fn test_resolve_head_deleted_branch_error() { 735 fn test_resolve_head_deleted_branch_error() {
736 // Only for patches on the legacy fallback: one with a revision ref has
737 // somewhere else to look, and no longer cares that the branch is gone.
730 let tmp = TempDir::new().unwrap(); 738 let tmp = TempDir::new().unwrap();
731 let repo = init_repo(tmp.path(), &alice()); 739 let repo = init_repo(tmp.path(), &alice());
732 740
@@ -815,7 +823,7 @@ fn test_create_patch_from_branch_populates_branch_field() {
815 add_commit_on_branch(&repo, "feature/foo", "feat.rs", b"feat"); 823 add_commit_on_branch(&repo, "feature/foo", "feat.rs", b"feat");
816 824
817 let id = patch::create(&repo, "My patch", "desc", "main", "feature/foo", None).unwrap(); 825 let id = patch::create(&repo, "My patch", "desc", "main", "feature/foo", None).unwrap();
818 let patch_ref = format!("refs/collab/patches/{}", id); 826 let patch_ref = git_collab::state::patch_events_ref(&id);
819 let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); 827 let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap();
820 828
821 assert_eq!(state.branch, "feature/foo"); 829 assert_eq!(state.branch, "feature/foo");
@@ -825,8 +833,10 @@ fn test_create_patch_from_branch_populates_branch_field() {
825 } 833 }
826 834
827 #[test] 835 #[test]
828 fn test_create_duplicate_patch_for_same_branch_returns_error() { 836 fn test_create_duplicate_patch_for_same_commit_returns_error() {
829 // T011: creating a duplicate patch for same branch returns error 837 // T011: a second patch for a commit an open patch already stands at is a
838 // duplicate. Detection is by commit, not by branch name — a branch name is
839 // no longer part of a patch's identity.
830 let cfg = ScopedTestConfig::new(); 840 let cfg = ScopedTestConfig::new();
831 cfg.ensure_signing_key(); 841 cfg.ensure_signing_key();
832 let tmp = TempDir::new().unwrap(); 842 let tmp = TempDir::new().unwrap();
@@ -835,17 +845,20 @@ fn test_create_duplicate_patch_for_same_branch_returns_error() {
835 let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x"); 845 let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x");
836 repo.branch("feature/dup", &repo.find_commit(tip).unwrap(), false) 846 repo.branch("feature/dup", &repo.find_commit(tip).unwrap(), false)
837 .unwrap(); 847 .unwrap();
848 // A second branch name for the very same commit, as two worktrees produce.
849 repo.branch("worktree-agent-dup", &repo.find_commit(tip).unwrap(), false)
850 .unwrap();
838 851
839 // First creation should succeed 852 // First creation should succeed
840 patch::create(&repo, "First", "", "main", "feature/dup", None).unwrap(); 853 let id = patch::create(&repo, "First", "", "main", "feature/dup", None).unwrap();
841 854
842 // Second creation for same branch should fail 855 let result = patch::create(&repo, "Second", "", "main", "worktree-agent-dup", None);
843 let result = patch::create(&repo, "Second", "", "main", "feature/dup", None); 856 assert!(result.is_err(), "duplicate commit patch should fail");
844 assert!(result.is_err(), "duplicate branch patch should fail");
845 let err_msg = result.unwrap_err().to_string(); 857 let err_msg = result.unwrap_err().to_string();
846 assert!( 858 assert!(
847 err_msg.contains("feature/dup"), 859 err_msg.contains(&id[..8]),
848 "error should mention the branch name" 860 "error should name the existing patch, got: {}",
861 err_msg
849 ); 862 );
850 } 863 }
851 864
@@ -991,7 +1004,7 @@ fn test_resolve_head_with_oid_string() {
991 }; 1004 };
992 let oid = dag::create_root_event(&repo, &event, &sk).unwrap(); 1005 let oid = dag::create_root_event(&repo, &event, &sk).unwrap();
993 let id = oid.to_string(); 1006 let id = oid.to_string();
994 let ref_name = format!("refs/collab/patches/{}", id); 1007 let ref_name = git_collab::state::patch_events_ref(&id);
995 repo.reference(&ref_name, oid, false, "test").unwrap(); 1008 repo.reference(&ref_name, oid, false, "test").unwrap();
996 1009
997 let state = PatchState::from_ref(&repo, &ref_name, &id).unwrap(); 1010 let state = PatchState::from_ref(&repo, &ref_name, &id).unwrap();
@@ -1023,7 +1036,7 @@ fn test_auto_detect_merged_patch_via_git_merge() {
1023 let id = patch::create(&repo, "Auto-detect test", "", "main", "feat", None).unwrap(); 1036 let id = patch::create(&repo, "Auto-detect test", "", "main", "feat", None).unwrap();
1024 1037
1025 // Verify it's open 1038 // Verify it's open
1026 let ref_name = format!("refs/collab/patches/{}", id); 1039 let ref_name = git_collab::state::patch_events_ref(&id);
1027 let state = PatchState::from_ref(&repo, &ref_name, &id).unwrap(); 1040 let state = PatchState::from_ref(&repo, &ref_name, &id).unwrap();
1028 assert_eq!(state.status, PatchStatus::Open); 1041 assert_eq!(state.status, PatchStatus::Open);
1029 1042
@@ -1065,7 +1078,7 @@ fn test_auto_detect_merged_patch_deleted_branch() {
1065 .unwrap(); 1078 .unwrap();
1066 1079
1067 // Should not crash, patch stays Open (can't verify merge without the branch) 1080 // Should not crash, patch stays Open (can't verify merge without the branch)
1068 let ref_name = format!("refs/collab/patches/{}", id); 1081 let ref_name = git_collab::state::patch_events_ref(&id);
1069 let state = PatchState::from_ref_uncached(&repo, &ref_name, &id).unwrap(); 1082 let state = PatchState::from_ref_uncached(&repo, &ref_name, &id).unwrap();
1070 assert_eq!(state.status, PatchStatus::Open); 1083 assert_eq!(state.status, PatchStatus::Open);
1071 } 1084 }
@@ -1088,7 +1101,7 @@ fn test_cache_does_not_defeat_auto_detect_merge() {
1088 1101
1089 // Create the patch via the high-level API (records base_commit) 1102 // Create the patch via the high-level API (records base_commit)
1090 let id = patch::create(&repo, "Cache merge test", "", "main", "feat", None).unwrap(); 1103 let id = patch::create(&repo, "Cache merge test", "", "main", "feat", None).unwrap();
1091 let ref_name = format!("refs/collab/patches/{}", id); 1104 let ref_name = git_collab::state::patch_events_ref(&id);
1092 1105
1093 // First call: populates cache, status should be Open 1106 // First call: populates cache, status should be Open
1094 let state = PatchState::from_ref(&repo, &ref_name, &id).unwrap(); 1107 let state = PatchState::from_ref(&repo, &ref_name, &id).unwrap();
@@ -1109,33 +1122,40 @@ fn test_cache_does_not_defeat_auto_detect_merge() {
1109 } 1122 }
1110 1123
1111 // --------------------------------------------------------------------------- 1124 // ---------------------------------------------------------------------------
1112 // Phase 6: US4 — Implicit Revision (T027) 1125 // The head is pinned to the recorded revision, not to the branch
1113 // --------------------------------------------------------------------------- 1126 // ---------------------------------------------------------------------------
1114 1127
1115 #[test] 1128 #[test]
1116 fn test_branch_push_auto_reflects_in_patch() { 1129 fn test_branch_movement_does_not_move_the_patch_head() {
1117 // T027: after pushing a commit to the branch, patch show reflects the new commit 1130 // A patch stands at the revision its author recorded. Commits landing on
1131 // the branch afterwards are not part of it until `patch revise` says so —
1132 // which is also what keeps a reviewer looking at what they were shown.
1133 let cfg = ScopedTestConfig::new();
1134 cfg.ensure_signing_key();
1118 let tmp = TempDir::new().unwrap(); 1135 let tmp = TempDir::new().unwrap();
1119 let repo = init_repo(tmp.path(), &alice()); 1136 let repo = init_repo(tmp.path(), &alice());
1120 make_initial_commit(&repo, "main"); 1137 make_initial_commit(&repo, "main");
1121 let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x"); 1138 let tip = add_commit_on_branch(&repo, "main", "f.rs", b"x");
1122 repo.branch("feat", &repo.find_commit(tip).unwrap(), false) 1139 repo.branch("feat", &repo.find_commit(tip).unwrap(), false)
1123 .unwrap(); 1140 .unwrap();
1124 add_commit_on_branch(&repo, "feat", "v1.rs", b"version 1"); 1141 let recorded = add_commit_on_branch(&repo, "feat", "v1.rs", b"version 1");
1125 1142
1126 let (patch_ref, id) = create_branch_patch(&repo, &alice(), "Auto revise", "feat", "main"); 1143 let id = patch::create(&repo, "Pinned head", "", "main", "feat", None).unwrap();
1144 let patch_ref = git_collab::state::patch_events_ref(&id);
1127 let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); 1145 let state = PatchState::from_ref(&repo, &patch_ref, &id).unwrap();
1128 let head1 = state.resolve_head(&repo).unwrap(); 1146 assert_eq!(state.resolve_head(&repo).unwrap(), recorded);
1129 1147
1130 // Add another commit to the branch (simulating a push) 1148 // Add another commit to the branch. The patch must not silently absorb it.
1131 let new_tip = add_commit_on_branch(&repo, "feat", "v2.rs", b"version 2"); 1149 let new_tip = add_commit_on_branch(&repo, "feat", "v2.rs", b"version 2");
1150 assert_ne!(new_tip, recorded);
1132 1151
1133 // Re-read the state and resolve head — should see the new commit 1152 let state2 = PatchState::from_ref_uncached(&repo, &patch_ref, &id).unwrap();
1134 let state2 = PatchState::from_ref(&repo, &patch_ref, &id).unwrap(); 1153 assert_eq!(
1135 let head2 = state2.resolve_head(&repo).unwrap(); 1154 state2.resolve_head(&repo).unwrap(),
1136 1155 recorded,
1137 assert_ne!(head1, head2, "head should change after branch update"); 1156 "the head follows the recorded revision, not the branch"
1138 assert_eq!(head2, new_tip, "head should be the new tip"); 1157 );
1158 assert_eq!(state2.revisions.len(), 1);
1139 } 1159 }
1140 1160
1141 // --------------------------------------------------------------------------- 1161 // ---------------------------------------------------------------------------
tests/common/mod.rs
Old New
@@ -359,7 +359,7 @@ pub fn create_patch(repo: &Repository, author: &Author, title: &str) -> (String,
359 }; 359 };
360 let oid = dag::create_root_event(repo, &event, &sk).unwrap(); 360 let oid = dag::create_root_event(repo, &event, &sk).unwrap();
361 let id = oid.to_string(); 361 let id = oid.to_string();
362 let ref_name = format!("refs/collab/patches/{}", id); 362 let ref_name = git_collab::state::patch_events_ref(&id);
363 repo.reference(&ref_name, oid, false, "test patch").unwrap(); 363 repo.reference(&ref_name, oid, false, "test patch").unwrap();
364 (ref_name, id) 364 (ref_name, id)
365 } 365 }
tests/legacy_patch_test.rs
Old New
@@ -144,9 +144,15 @@ fn legacy_patch_revise_deserializes_as_a_revision() {
144 .expect("legacy patch.revise must deserialize"); 144 .expect("legacy patch.revise must deserialize");
145 145
146 match event.action { 146 match event.action {
147 Action::PatchRevision { commit, tree, body } => { 147 Action::PatchRevision {
148 commit,
149 tree,
150 body,
151 base,
152 } => {
148 assert!(commit.is_empty()); 153 assert!(commit.is_empty());
149 assert!(tree.is_empty()); 154 assert!(tree.is_empty());
155 assert!(base.is_none(), "this generation recorded no base");
150 assert_eq!( 156 assert_eq!(
151 body.as_deref(), 157 body.as_deref(),
152 Some("Address review: extract auto_detect_and_update helper to eliminate 3x repeated pattern") 158 Some("Address review: extract auto_detect_and_update helper to eliminate 3x repeated pattern")
tests/review_test.rs
Old New
@@ -311,9 +311,11 @@ fn cli_re_approving_new_revision_is_allowed() {
311 311
312 repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM rev 1"]); 312 repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM rev 1"]);
313 313
314 // New commit → new revision auto-detected on next review 314 // New commit, recorded as a revision by the author — nothing else records
315 // one on their behalf.
315 repo.git(&["checkout", "feat-reapprove"]); 316 repo.git(&["checkout", "feat-reapprove"]);
316 repo.commit_file("v2.txt", "v2", "second commit"); 317 repo.commit_file("v2.txt", "v2", "second commit");
318 repo.run_ok(&["patch", "revise", &id]);
317 repo.git(&["checkout", "main"]); 319 repo.git(&["checkout", "main"]);
318 320
319 repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM rev 2"]); 321 repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM rev 2"]);
tests/revision_refs_test.rs
Old New
@@ -0,0 +1,630 @@
1 //! Patches carried as revision refs rather than branches.
2 //!
3 //! Each revision is an immutable ref in the patch's own namespace:
4 //!
5 //! refs/collab/patches/<id>/events the event DAG
6 //! refs/collab/patches/<id>/r/<n> revision n's commit, write-once
7 //!
8 //! so a patch and every revision it ever had travel under the refspecs `sync`
9 //! already uses, and nothing depends on a `refs/heads/*` ref surviving.
10
11 mod common;
12
13 use common::{write_raw_event, TestRepo};
14 use serde_json::json;
15 use tempfile::TempDir;
16
17 /// Every ref under the patch namespace, sorted, as `git for-each-ref` sees it.
18 fn patch_refs(repo: &TestRepo) -> Vec<String> {
19 let mut refs: Vec<String> = repo
20 .git(&["for-each-ref", "--format=%(refname)", "refs/collab/"])
21 .lines()
22 .map(str::to_string)
23 .collect();
24 refs.sort();
25 refs
26 }
27
28 fn ref_target(repo: &TestRepo, name: &str) -> Option<String> {
29 let out = repo.git(&["for-each-ref", "--format=%(objectname)", name]);
30 let trimmed = out.trim();
31 if trimmed.is_empty() {
32 None
33 } else {
34 Some(trimmed.to_string())
35 }
36 }
37
38 fn show_json(repo: &TestRepo, id: &str) -> serde_json::Value {
39 let out = repo.run_ok(&["patch", "show", id, "--json"]);
40 serde_json::from_str(&out).unwrap()
41 }
42
43 /// Create a patch on a fresh branch holding one commit. Returns (short id, full
44 /// id, tip commit).
45 fn patch_on_branch(repo: &TestRepo, branch: &str, file: &str) -> (String, String, String) {
46 repo.git(&["checkout", "-b", branch]);
47 let tip = repo.commit_file(file, "v1", &format!("add {}", file));
48 let out = repo.run_ok(&["patch", "create", "-t", branch, "-B", branch]);
49 let short = out
50 .trim()
51 .strip_prefix("Created patch ")
52 .unwrap_or_else(|| panic!("unexpected create output: {}", out))
53 .to_string();
54 let full = full_id(repo, &short);
55 (short, full, tip)
56 }
57
58 /// Expand a short patch id to the full 40-char id via the ref listing.
59 fn full_id(repo: &TestRepo, short: &str) -> String {
60 for name in patch_refs(repo) {
61 if let Some(rest) = name.strip_prefix("refs/collab/patches/") {
62 let id = rest.split('/').next().unwrap_or_default();
63 if id.starts_with(short) {
64 return id.to_string();
65 }
66 }
67 }
68 panic!("no patch ref matching {}", short);
69 }
70
71 // ===========================================================================
72 // Ref layout
73 // ===========================================================================
74
75 #[test]
76 fn patch_create_writes_an_events_ref_and_revision_one() {
77 let repo = TestRepo::new("Alice", "alice@example.com");
78 let (_short, id, tip) = patch_on_branch(&repo, "feat", "a.txt");
79
80 let refs = patch_refs(&repo);
81 assert!(
82 refs.contains(&format!("refs/collab/patches/{}/events", id)),
83 "expected an events ref, got {:?}",
84 refs
85 );
86 assert!(
87 refs.contains(&format!("refs/collab/patches/{}/r/1", id)),
88 "expected r/1, got {:?}",
89 refs
90 );
91 assert!(
92 !refs.contains(&format!("refs/collab/patches/{}", id)),
93 "the bare <id> ref must be gone — git cannot have it be both a ref and \
94 a directory: {:?}",
95 refs
96 );
97 assert_eq!(
98 ref_target(&repo, &format!("refs/collab/patches/{}/r/1", id)).as_deref(),
99 Some(tip.as_str()),
100 "r/1 must point at the commit the patch was created from"
101 );
102 }
103
104 #[test]
105 fn revise_writes_the_next_revision_ref_and_leaves_earlier_ones_alone() {
106 let repo = TestRepo::new("Alice", "alice@example.com");
107 let (short, id, r1_commit) = patch_on_branch(&repo, "feat", "a.txt");
108
109 let r2_commit = repo.commit_file("b.txt", "v2", "second commit");
110 repo.run_ok(&["patch", "revise", &short, "-b", "addressed review"]);
111
112 assert_eq!(
113 ref_target(&repo, &format!("refs/collab/patches/{}/r/1", id)).as_deref(),
114 Some(r1_commit.as_str()),
115 "r/1 is write-once and must not move"
116 );
117 assert_eq!(
118 ref_target(&repo, &format!("refs/collab/patches/{}/r/2", id)).as_deref(),
119 Some(r2_commit.as_str())
120 );
121
122 let json = show_json(&repo, &short);
123 assert_eq!(json["revisions"].as_array().unwrap().len(), 2);
124 }
125
126 #[test]
127 fn a_revision_ref_keeps_its_commit_alive_after_the_branch_is_rewritten() {
128 // The live defect: a rebase-and-force-push used to leave revision 1's
129 // commit reachable from no ref at all, so `git gc` was entitled to delete
130 // the objects that `patch log`, `patch diff --revision 1` and every
131 // revision-anchored inline comment point at.
132 let repo = TestRepo::new("Alice", "alice@example.com");
133 let (short, id, r1_commit) = patch_on_branch(&repo, "feat", "a.txt");
134
135 // Rewrite the branch out from under the patch, exactly as a rebase does.
136 repo.git(&["commit", "--amend", "-m", "rewritten"]);
137 let rewritten = repo.git(&["rev-parse", "HEAD"]).trim().to_string();
138 assert_ne!(rewritten, r1_commit);
139
140 let containing = repo.git(&[
141 "for-each-ref",
142 "--format=%(refname)",
143 "--contains",
144 &r1_commit,
145 "refs/heads/",
146 ]);
147 assert!(
148 containing.trim().is_empty(),
149 "the rewrite should have detached r1's commit from every branch, got {}",
150 containing
151 );
152
153 assert_eq!(
154 ref_target(&repo, &format!("refs/collab/patches/{}/r/1", id)).as_deref(),
155 Some(r1_commit.as_str()),
156 "the revision ref is what keeps the commit reachable"
157 );
158 // And it is still usable: a historical diff resolves the objects.
159 let diff = repo.run_ok(&["patch", "diff", &short, "--revision", "1"]);
160 assert!(diff.contains("a.txt"), "r1 diff lost its content: {}", diff);
161 }
162
163 #[test]
164 fn nested_revision_refs_are_visible_to_the_patch_glob() {
165 // The design's central claim is that the existing `refs/collab/patches/*`
166 // patterns carry revision refs unchanged. That only holds if the glob
167 // crosses `/`, which is what enumeration and `sync`'s push list rely on.
168 let tmp = TempDir::new().unwrap();
169 let repo = git2::Repository::init(tmp.path()).unwrap();
170 let sig = git2::Signature::now("A", "a@example.com").unwrap();
171 let tree_oid = repo.treebuilder(None).unwrap().write().unwrap();
172 let tree = repo.find_tree(tree_oid).unwrap();
173 let oid = repo
174 .commit(Some("refs/heads/main"), &sig, &sig, "init", &tree, &[])
175 .unwrap();
176 repo.reference("refs/collab/patches/abc/events", oid, false, "t")
177 .unwrap();
178 repo.reference("refs/collab/patches/abc/r/1", oid, false, "t")
179 .unwrap();
180
181 let names: Vec<String> = repo
182 .references_glob("refs/collab/patches/*")
183 .unwrap()
184 .filter_map(|r| r.ok()?.name().map(str::to_string))
185 .collect();
186 assert!(
187 names.contains(&"refs/collab/patches/abc/r/1".to_string()),
188 "refs/collab/patches/* must match nested refs, got {:?}",
189 names
190 );
191 }
192
193 // ===========================================================================
194 // Revision recording is explicit
195 // ===========================================================================
196
197 #[test]
198 fn commenting_does_not_manufacture_a_revision() {
199 let repo = TestRepo::new("Alice", "alice@example.com");
200 let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");
201 repo.commit_file("b.txt", "v2", "work the user has not submitted");
202
203 repo.run_ok(&["patch", "comment", &short, "-b", "a thought"]);
204
205 let json = show_json(&repo, &short);
206 assert_eq!(
207 json["revisions"].as_array().unwrap().len(),
208 1,
209 "only `patch revise` records a revision"
210 );
211 }
212
213 #[test]
214 fn reviewing_does_not_manufacture_a_revision() {
215 let repo = TestRepo::new("Alice", "alice@example.com");
216 let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");
217 repo.commit_file("b.txt", "v2", "work the reviewer cannot see");
218
219 repo.run_ok(&["patch", "review", &short, "-v", "comment", "-b", "noted"]);
220
221 let json = show_json(&repo, &short);
222 assert_eq!(json["revisions"].as_array().unwrap().len(), 1);
223 assert_eq!(json["reviews"][0]["revision"], 1);
224 }
225
226 #[test]
227 fn revise_reads_head_by_default_and_a_named_branch_on_request() {
228 let repo = TestRepo::new("Alice", "alice@example.com");
229 let (short, _id, _tip) = patch_on_branch(&repo, "wt-1", "a.txt");
230
231 // A second worktree-style branch, with a name the patch has never heard of.
232 repo.git(&["checkout", "-b", "wt-2"]);
233 let r2 = repo.commit_file("b.txt", "v2", "more work");
234 repo.git(&["checkout", "main"]);
235
236 repo.run_ok(&["patch", "revise", &short, "-B", "wt-2"]);
237
238 let json = show_json(&repo, &short);
239 let revisions = json["revisions"].as_array().unwrap();
240 assert_eq!(revisions.len(), 2);
241 assert_eq!(revisions[1]["commit"], r2);
242 }
243
244 // ===========================================================================
245 // Identity is declared, not derived from a branch name
246 // ===========================================================================
247
248 #[test]
249 fn a_second_patch_for_the_same_commit_is_rejected_by_id_not_by_branch_name() {
250 let repo = TestRepo::new("Alice", "alice@example.com");
251 let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");
252
253 let err = repo.run_err(&["patch", "create", "-t", "again", "-B", "feat"]);
254 assert!(
255 err.contains(&short),
256 "the duplicate error should name the existing patch, got {}",
257 err
258 );
259 }
260
261 #[test]
262 fn two_branches_with_generated_names_get_their_own_patches() {
263 let repo = TestRepo::new("Alice", "alice@example.com");
264 let (first, _id, _tip) = patch_on_branch(&repo, "worktree-agent-aaaa", "a.txt");
265
266 repo.git(&["checkout", "main"]);
267 let (second, _id2, _tip2) = patch_on_branch(&repo, "worktree-agent-bbbb", "b.txt");
268
269 assert_ne!(first, second);
270 let out = repo.run_ok(&["patch", "list"]);
271 assert!(out.contains(&first) && out.contains(&second), "{}", out);
272 }
273
274 // ===========================================================================
275 // Merge detection, anchored to the latest revision's base
276 // ===========================================================================
277
278 #[test]
279 fn merging_the_base_branch_forward_marks_the_patch_merged() {
280 let repo = TestRepo::new("Alice", "alice@example.com");
281 let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");
282
283 repo.git(&["checkout", "main"]);
284 repo.git(&["merge", "--ff-only", "feat"]);
285
286 assert_eq!(show_json(&repo, &short)["status"], "merged");
287 }
288
289 #[test]
290 fn an_unmerged_patch_stays_open() {
291 // Guards the failure mode where dropping `base_commit` leaves `base_moved`
292 // permanently false — or permanently true — and merge detection silently
293 // stops telling the truth.
294 let repo = TestRepo::new("Alice", "alice@example.com");
295 let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");
296
297 repo.git(&["checkout", "main"]);
298 repo.commit_file("unrelated.txt", "x", "main moves on its own");
299
300 assert_eq!(show_json(&repo, &short)["status"], "open");
301 }
302
303 #[test]
304 fn merge_detection_survives_deleting_the_source_branch() {
305 // Merge detection used to go through `refs/heads/<branch>` and no-op
306 // silently when it was absent, so deleting the branch after merging left
307 // the patch Open forever with no diagnostic.
308 let repo = TestRepo::new("Alice", "alice@example.com");
309 let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");
310
311 repo.git(&["checkout", "main"]);
312 repo.git(&["merge", "--ff-only", "feat"]);
313 repo.git(&["branch", "-D", "feat"]);
314
315 assert_eq!(show_json(&repo, &short)["status"], "merged");
316 }
317
318 #[test]
319 fn merge_detection_reads_the_latest_revisions_base_not_the_first() {
320 // After a rebase the patch's base moves. Detection must compare against
321 // where the *latest* revision stood, not where revision 1 did.
322 let repo = TestRepo::new("Alice", "alice@example.com");
323 let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");
324
325 repo.git(&["checkout", "main"]);
326 repo.commit_file("upstream.txt", "u", "upstream work");
327 repo.git(&["checkout", "feat"]);
328 repo.git(&["rebase", "main"]);
329 repo.run_ok(&["patch", "revise", &short, "-b", "rebased"]);
330
331 assert_eq!(
332 show_json(&repo, &short)["status"], "open",
333 "rebasing is not merging"
334 );
335
336 repo.git(&["checkout", "main"]);
337 repo.git(&["merge", "--ff-only", "feat"]);
338 assert_eq!(show_json(&repo, &short)["status"], "merged");
339 }
340
341 #[test]
342 fn each_revision_records_the_base_it_was_written_against() {
343 let repo = TestRepo::new("Alice", "alice@example.com");
344 let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");
345 let base1 = repo.git(&["rev-parse", "main"]).trim().to_string();
346
347 repo.git(&["checkout", "main"]);
348 repo.commit_file("upstream.txt", "u", "upstream work");
349 let base2 = repo.git(&["rev-parse", "main"]).trim().to_string();
350 repo.git(&["checkout", "feat"]);
351 repo.git(&["rebase", "main"]);
352 repo.run_ok(&["patch", "revise", &short, "-b", "rebased"]);
353
354 let json = show_json(&repo, &short);
355 let revisions = json["revisions"].as_array().unwrap();
356 assert_eq!(revisions[0]["base"], base1);
357 assert_eq!(revisions[1]["base"], base2);
358 }
359
360 #[test]
361 fn a_revision_recorded_without_a_rebase_keeps_the_same_base() {
362 let repo = TestRepo::new("Alice", "alice@example.com");
363 let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");
364 let base = repo.git(&["rev-parse", "main"]).trim().to_string();
365
366 repo.commit_file("b.txt", "v2", "more work, same base");
367 repo.run_ok(&["patch", "revise", &short]);
368
369 let json = show_json(&repo, &short);
370 let revisions = json["revisions"].as_array().unwrap();
371 assert_eq!(revisions[0]["base"], base);
372 assert_eq!(
373 revisions[1]["base"], base,
374 "no rebase happened, so the base must not move"
375 );
376 }
377
378 // ===========================================================================
379 // Revision-anchored review data outlives a rebase
380 // ===========================================================================
381
382 #[test]
383 fn an_inline_comment_on_revision_one_still_resolves_after_a_rebase() {
384 let repo = TestRepo::new("Alice", "alice@example.com");
385 let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");
386 repo.run_ok(&[
387 "patch", "comment", &short, "-b", "here", "--file", "a.txt", "--line", "1",
388 ]);
389
390 repo.git(&["checkout", "main"]);
391 repo.commit_file("upstream.txt", "u", "upstream work");
392 repo.git(&["checkout", "feat"]);
393 repo.git(&["rebase", "main"]);
394 repo.run_ok(&["patch", "revise", &short, "-b", "rebased"]);
395
396 let json = show_json(&repo, &short);
397 assert_eq!(json["inline_comments"][0]["revision"], 1);
398
399 let diff = repo.run_ok(&["patch", "diff", &short, "--revision", "1"]);
400 assert!(
401 diff.contains("a.txt"),
402 "revision 1's objects must still resolve: {}",
403 diff
404 );
405 }
406
407 // ===========================================================================
408 // Ref lifecycle: the whole subtree moves
409 // ===========================================================================
410
411 #[test]
412 fn closing_a_patch_archives_every_revision_ref() {
413 let repo = TestRepo::new("Alice", "alice@example.com");
414 let (short, id, r1) = patch_on_branch(&repo, "feat", "a.txt");
415 repo.commit_file("b.txt", "v2", "second commit");
416 repo.run_ok(&["patch", "revise", &short]);
417
418 repo.run_ok(&["patch", "close", &short, "-r", "not now"]);
419
420 let refs = patch_refs(&repo);
421 assert!(
422 refs.iter()
423 .all(|r| !r.starts_with(&format!("refs/collab/patches/{}/", id))),
424 "nothing may be left in the active namespace: {:?}",
425 refs
426 );
427 for suffix in ["events", "r/1", "r/2"] {
428 let name = format!("refs/collab/archive/patches/{}/{}", id, suffix);
429 assert!(refs.contains(&name), "missing {} in {:?}", name, refs);
430 }
431 assert_eq!(
432 ref_target(&repo, &format!("refs/collab/archive/patches/{}/r/1", id)).as_deref(),
433 Some(r1.as_str())
434 );
435
436 // And the closed patch is still reviewable.
437 let json = show_json(&repo, &short);
438 assert_eq!(json["status"], "closed");
439 assert_eq!(json["revisions"].as_array().unwrap().len(), 2);
440 }
441
442 #[test]
443 fn deleting_a_patch_removes_every_ref_in_its_namespace() {
444 let repo = TestRepo::new("Alice", "alice@example.com");
445 let (short, id, _r1) = patch_on_branch(&repo, "feat", "a.txt");
446 repo.commit_file("b.txt", "v2", "second commit");
447 repo.run_ok(&["patch", "revise", &short]);
448
449 repo.run_ok(&["patch", "delete", &short]);
450
451 let refs = patch_refs(&repo);
452 assert!(
453 refs.iter().all(|r| !r.contains(&id)),
454 "patch namespace not fully removed: {:?}",
455 refs
456 );
457 }
458
459 // ===========================================================================
460 // Migration from the single-ref layout
461 // ===========================================================================
462
463 /// Rewrite a patch back into the pre-revision-refs layout: one ref at
464 /// `refs/collab/patches/<id>` and no revision refs at all.
465 fn demote_to_old_layout(repo: &TestRepo, id: &str) {
466 let events = format!("refs/collab/patches/{}/events", id);
467 let tip = ref_target(repo, &events).expect("events ref");
468 for name in patch_refs(repo) {
469 if name.starts_with(&format!("refs/collab/patches/{}/", id)) {
470 repo.git(&["update-ref", "-d", &name]);
471 }
472 }
473 repo.git(&["update-ref", &format!("refs/collab/patches/{}", id), &tip]);
474 }
475
476 #[test]
477 fn an_old_layout_patch_is_migrated_on_first_use() {
478 let repo = TestRepo::new("Alice", "alice@example.com");
479 let (short, id, r1) = patch_on_branch(&repo, "feat", "a.txt");
480 repo.commit_file("b.txt", "v2", "second commit");
481 repo.run_ok(&["patch", "revise", &short]);
482 let r2 = repo.git(&["rev-parse", "HEAD"]).trim().to_string();
483 demote_to_old_layout(&repo, &id);
484
485 // Any read is "first use".
486 let out = repo.run_ok(&["patch", "list"]);
487 assert!(out.contains(&short), "{}", out);
488
489 let refs = patch_refs(&repo);
490 assert!(
491 refs.contains(&format!("refs/collab/patches/{}/events", id)),
492 "{:?}",
493 refs
494 );
495 assert!(
496 !refs.contains(&format!("refs/collab/patches/{}", id)),
497 "the old ref must be gone: {:?}",
498 refs
499 );
500 assert_eq!(
501 ref_target(&repo, &format!("refs/collab/patches/{}/r/1", id)).as_deref(),
502 Some(r1.as_str())
503 );
504 assert_eq!(
505 ref_target(&repo, &format!("refs/collab/patches/{}/r/2", id)).as_deref(),
506 Some(r2.as_str())
507 );
508
509 // The migrated patch still diffs.
510 let diff = repo.run_ok(&["patch", "diff", &short, "--revision", "1"]);
511 assert!(diff.contains("a.txt"), "{}", diff);
512 }
513
514 #[test]
515 fn migration_skips_a_revision_whose_commit_is_already_lost() {
516 // Revisions whose objects were stripped by a force-push before the patch
517 // was migrated cannot be recovered. Migration must leave them without a
518 // ref rather than failing and taking the whole patch down with it.
519 let repo = TestRepo::new("Alice", "alice@example.com");
520 let head = repo.git(&["rev-parse", "HEAD"]).trim().to_string();
521 let git_repo = git2::Repository::open(repo.dir.path()).unwrap();
522 let tree = git_repo
523 .find_commit(git2::Oid::from_str(&head).unwrap())
524 .unwrap()
525 .tree()
526 .unwrap()
527 .id()
528 .to_string();
529 let missing = "1111111111111111111111111111111111111111";
530
531 let root = write_raw_event(
532 &git_repo,
533 None,
534 json!({
535 "type": "patch.create",
536 "title": "Patch whose r1 was force-pushed away",
537 "body": "",
538 "base_ref": "main",
539 "branch": "gone",
540 "commit": missing,
541 "tree": tree,
542 }),
543 1,
544 );
545 let tip = write_raw_event(
546 &git_repo,
547 Some(root),
548 json!({
549 "type": "patch.revision",
550 "commit": head,
551 "tree": tree,
552 "body": "still here",
553 }),
554 2,
555 );
556 let id = root.to_string();
557 git_repo
558 .reference(
559 &format!("refs/collab/patches/{}", id),
560 tip,
561 false,
562 "old layout",
563 )
564 .unwrap();
565 drop(git_repo);
566
567 let out = repo.run_ok(&["patch", "list", "--all"]);
568 assert!(out.contains("force-pushed away"), "{}", out);
569
570 let refs = patch_refs(&repo);
571 assert!(
572 refs.contains(&format!("refs/collab/patches/{}/events", id)),
573 "{:?}",
574 refs
575 );
576 assert!(
577 !refs.contains(&format!("refs/collab/patches/{}/r/1", id)),
578 "a lost commit must not get a ref: {:?}",
579 refs
580 );
581 assert_eq!(
582 ref_target(&repo, &format!("refs/collab/patches/{}/r/2", id)).as_deref(),
583 Some(head.as_str()),
584 "the surviving revision still gets one"
585 );
586 }
587
588 #[test]
589 fn a_fully_legacy_patch_with_no_recorded_commits_still_materializes() {
590 let repo = TestRepo::new("Alice", "alice@example.com");
591 let git_repo = git2::Repository::open(repo.dir.path()).unwrap();
592 let root = write_raw_event(
593 &git_repo,
594 None,
595 json!({
596 "type": "patch.create",
597 "title": "Ancient patch",
598 "body": "",
599 "base_ref": "main",
600 "branch": "long-gone",
601 }),
602 1,
603 );
604 let id = root.to_string();
605 git_repo
606 .reference(
607 &format!("refs/collab/patches/{}", id),
608 root,
609 false,
610 "old layout",
611 )
612 .unwrap();
613 drop(git_repo);
614
615 let out = repo.run_ok(&["patch", "list", "--all"]);
616 assert!(out.contains("Ancient patch"), "{}", out);
617
618 let refs = patch_refs(&repo);
619 assert!(
620 refs.contains(&format!("refs/collab/patches/{}/events", id)),
621 "{:?}",
622 refs
623 );
624 assert!(
625 refs.iter()
626 .all(|r| !r.starts_with(&format!("refs/collab/patches/{}/r/", id))),
627 "no commits were ever recorded, so there is nothing to point a ref at: {:?}",
628 refs
629 );
630 }
tests/revision_test.rs
Old New
@@ -28,51 +28,14 @@ fn test_patch_create_records_revision_1() {
28 } 28 }
29 29
30 // =========================================================================== 30 // ===========================================================================
31 // Auto-detect revision on comment 31 // Revisions are recorded only by `patch revise`
32 //
33 // Comments and reviews used to infer one from the branch tip as a side effect,
34 // which manufactured revisions the user never asked for; that path is gone and
35 // its replacement is asserted in tests/revision_refs_test.rs.
32 // =========================================================================== 36 // ===========================================================================
33 37
34 #[test] 38 #[test]
35 fn test_auto_detect_revision_on_comment() {
36 let repo = TestRepo::new("Alice", "alice@example.com");
37
38 // Create feature branch and patch
39 repo.git(&["checkout", "-b", "feat-auto"]);
40 repo.commit_file("v1.txt", "v1", "initial commit");
41 let out = repo.run_ok(&[
42 "patch",
43 "create",
44 "-t",
45 "Auto-detect test",
46 "-B",
47 "feat-auto",
48 ]);
49 let id = out
50 .trim()
51 .strip_prefix("Created patch ")
52 .unwrap()
53 .to_string();
54
55 // Push a new commit to the branch
56 repo.git(&["checkout", "feat-auto"]);
57 repo.commit_file("v2.txt", "v2", "second commit");
58 repo.git(&["checkout", "main"]);
59
60 // Comment should auto-insert a PatchRevision first
61 repo.run_ok(&["patch", "comment", &id, "-b", "Looks interesting"]);
62
63 let out = repo.run_ok(&["patch", "show", &id, "--json"]);
64 let json: serde_json::Value = serde_json::from_str(&out).unwrap();
65 let revisions = json["revisions"].as_array().unwrap();
66 assert_eq!(
67 revisions.len(),
68 2,
69 "should have 2 revisions (create + auto-detect)"
70 );
71 assert_eq!(revisions[0]["number"], 1);
72 assert_eq!(revisions[1]["number"], 2);
73 }
74
75 #[test]
76 fn test_no_revision_when_branch_unchanged() { 39 fn test_no_revision_when_branch_unchanged() {
77 let repo = TestRepo::new("Alice", "alice@example.com"); 40 let repo = TestRepo::new("Alice", "alice@example.com");
78 41
@@ -101,12 +64,8 @@ fn test_no_revision_when_branch_unchanged() {
101 assert_eq!(revisions.len(), 1, "should still have only 1 revision"); 64 assert_eq!(revisions.len(), 1, "should still have only 1 revision");
102 } 65 }
103 66
104 // ===========================================================================
105 // Auto-detect revision on review
106 // ===========================================================================
107
108 #[test] 67 #[test]
109 fn test_auto_detect_revision_on_review() { 68 fn test_review_anchors_to_the_latest_recorded_revision() {
110 let repo = TestRepo::new("Alice", "alice@example.com"); 69 let repo = TestRepo::new("Alice", "alice@example.com");
111 70
112 repo.git(&["checkout", "-b", "feat-review"]); 71 repo.git(&["checkout", "-b", "feat-review"]);
@@ -118,12 +77,10 @@ fn test_auto_detect_revision_on_review() {
118 .unwrap() 77 .unwrap()
119 .to_string(); 78 .to_string();
120 79
121 // Push a new commit
122 repo.git(&["checkout", "feat-review"]);
123 repo.commit_file("v2.txt", "v2", "second commit"); 80 repo.commit_file("v2.txt", "v2", "second commit");
81 repo.run_ok(&["patch", "revise", &id]);
124 repo.git(&["checkout", "main"]); 82 repo.git(&["checkout", "main"]);
125 83
126 // Review should auto-insert a PatchRevision
127 repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM"]); 84 repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM"]);
128 85
129 let out = repo.run_ok(&["patch", "show", &id, "--json"]); 86 let out = repo.run_ok(&["patch", "show", &id, "--json"]);
@@ -334,9 +291,10 @@ fn test_show_revision_filter() {
334 "1", 291 "1",
335 ]); 292 ]);
336 293
337 // Push v2 and comment on r2 294 // Record r2 and comment on it
338 repo.git(&["checkout", "feat-show-rev"]); 295 repo.git(&["checkout", "feat-show-rev"]);
339 repo.commit_file("b.txt", "b", "v2"); 296 repo.commit_file("b.txt", "b", "v2");
297 repo.run_ok(&["patch", "revise", &id]);
340 repo.git(&["checkout", "main"]); 298 repo.git(&["checkout", "main"]);
341 repo.run_ok(&[ 299 repo.run_ok(&[
342 "patch", 300 "patch",
@@ -385,13 +343,11 @@ fn test_interdiff_between_revisions() {
385 .unwrap() 343 .unwrap()
386 .to_string(); 344 .to_string();
387 345
388 // Push v2 346 // Push v2 and record it as revision 2
389 repo.git(&["checkout", "feat-interdiff"]); 347 repo.git(&["checkout", "feat-interdiff"]);
390 repo.commit_file("b.txt", "world", "v2"); 348 repo.commit_file("b.txt", "world", "v2");
391 repo.git(&["checkout", "main"]);
392
393 // Create revision 2 via revise
394 repo.run_ok(&["patch", "revise", &id]); 349 repo.run_ok(&["patch", "revise", &id]);
350 repo.git(&["checkout", "main"]);
395 351
396 // Interdiff between r1 and r2 should show b.txt added 352 // Interdiff between r1 and r2 should show b.txt added
397 let out = repo.run_ok(&["patch", "diff", &id, "--between", "1", "2"]); 353 let out = repo.run_ok(&["patch", "diff", &id, "--between", "1", "2"]);
@@ -421,8 +377,8 @@ fn test_interdiff_single_arg_means_n_to_latest() {
421 // Push v2 377 // Push v2
422 repo.git(&["checkout", "feat-between-single"]); 378 repo.git(&["checkout", "feat-between-single"]);
423 repo.commit_file("c.txt", "new", "v2"); 379 repo.commit_file("c.txt", "new", "v2");
424 repo.git(&["checkout", "main"]);
425 repo.run_ok(&["patch", "revise", &id]); 380 repo.run_ok(&["patch", "revise", &id]);
381 repo.git(&["checkout", "main"]);
426 382
427 // --between 1 (single arg) = 1..latest 383 // --between 1 (single arg) = 1..latest
428 let out = repo.run_ok(&["patch", "diff", &id, "--between", "1"]); 384 let out = repo.run_ok(&["patch", "diff", &id, "--between", "1"]);
@@ -515,14 +471,12 @@ fn test_patch_log() {
515 // Push v2 471 // Push v2
516 repo.git(&["checkout", "feat-log"]); 472 repo.git(&["checkout", "feat-log"]);
517 repo.commit_file("b.txt", "world", "v2"); 473 repo.commit_file("b.txt", "world", "v2");
518 repo.git(&["checkout", "main"]);
519 repo.run_ok(&["patch", "revise", &id, "-b", "added b.txt"]); 474 repo.run_ok(&["patch", "revise", &id, "-b", "added b.txt"]);
520 475
521 // Push v3 476 // Push v3
522 repo.git(&["checkout", "feat-log"]);
523 repo.commit_file("c.txt", "!", "v3"); 477 repo.commit_file("c.txt", "!", "v3");
524 repo.git(&["checkout", "main"]);
525 repo.run_ok(&["patch", "revise", &id]); 478 repo.run_ok(&["patch", "revise", &id]);
479 repo.git(&["checkout", "main"]);
526 480
527 let out = repo.run_ok(&["patch", "log", &id]); 481 let out = repo.run_ok(&["patch", "log", &id]);
528 assert!(out.contains("r1")); 482 assert!(out.contains("r1"));
@@ -640,8 +594,9 @@ fn test_review_shows_revision_context_in_show() {
640 // =========================================================================== 594 // ===========================================================================
641 595
642 #[test] 596 #[test]
643 fn test_revision_dedup_by_commit_oid() { 597 fn test_revising_the_same_commit_twice_is_refused() {
644 // Two comments with no branch change should not create duplicate revisions 598 // Revising a tip already recorded is a no-op the CLI rejects, so the
599 // revision list cannot grow without the code actually moving.
645 let repo = TestRepo::new("Alice", "alice@example.com"); 600 let repo = TestRepo::new("Alice", "alice@example.com");
646 601
647 repo.git(&["checkout", "-b", "feat-dedup"]); 602 repo.git(&["checkout", "-b", "feat-dedup"]);
@@ -653,12 +608,12 @@ fn test_revision_dedup_by_commit_oid() {
653 .unwrap() 608 .unwrap()
654 .to_string(); 609 .to_string();
655 610
656 // Push v2
657 repo.git(&["checkout", "feat-dedup"]);
658 repo.commit_file("b.txt", "b", "v2"); 611 repo.commit_file("b.txt", "b", "v2");
659 repo.git(&["checkout", "main"]); 612 repo.run_ok(&["patch", "revise", &id]);
613
614 let err = repo.run_err(&["patch", "revise", &id]);
615 assert!(err.contains("no changes since revision 2"), "{}", err);
660 616
661 // Two comments — first should auto-detect revision, second should not create another
662 repo.run_ok(&["patch", "comment", &id, "-b", "first"]); 617 repo.run_ok(&["patch", "comment", &id, "-b", "first"]);
663 repo.run_ok(&["patch", "comment", &id, "-b", "second"]); 618 repo.run_ok(&["patch", "comment", &id, "-b", "second"]);
664 619
@@ -727,7 +682,7 @@ fn test_concurrent_revision_dedup_after_reconcile() {
727 }; 682 };
728 let patch_oid = dag::create_root_event(&repo, &create_event, &sk).unwrap(); 683 let patch_oid = dag::create_root_event(&repo, &create_event, &sk).unwrap();
729 let id = patch_oid.to_string(); 684 let id = patch_oid.to_string();
730 let ref_name = format!("refs/collab/patches/{}", id); 685 let ref_name = git_collab::state::patch_events_ref(&id);
731 repo.reference(&ref_name, patch_oid, false, "test").unwrap(); 686 repo.reference(&ref_name, patch_oid, false, "test").unwrap();
732 687
733 // Push a new commit on the branch 688 // Push a new commit on the branch
@@ -760,6 +715,7 @@ fn test_concurrent_revision_dedup_after_reconcile() {
760 commit: new_commit.to_string(), 715 commit: new_commit.to_string(),
761 tree: new_tree_oid.to_string(), 716 tree: new_tree_oid.to_string(),
762 body: None, 717 body: None,
718 base: None,
763 }, 719 },
764 clock: 0, 720 clock: 0,
765 }; 721 };
@@ -776,6 +732,7 @@ fn test_concurrent_revision_dedup_after_reconcile() {
776 commit: new_commit.to_string(), 732 commit: new_commit.to_string(),
777 tree: new_tree_oid.to_string(), 733 tree: new_tree_oid.to_string(),
778 body: None, 734 body: None,
735 base: None,
779 }, 736 },
780 clock: 0, 737 clock: 0,
781 }; 738 };
tests/sort_test.rs
Old New
@@ -73,7 +73,7 @@ fn create_patch_at(
73 }; 73 };
74 let oid = dag::create_root_event(repo, &event, &sk).unwrap(); 74 let oid = dag::create_root_event(repo, &event, &sk).unwrap();
75 let id = oid.to_string(); 75 let id = oid.to_string();
76 let ref_name = format!("refs/collab/patches/{}", id); 76 let ref_name = git_collab::state::patch_events_ref(&id);
77 repo.reference(&ref_name, oid, false, "test patch").unwrap(); 77 repo.reference(&ref_name, oid, false, "test patch").unwrap();
78 (ref_name, id) 78 (ref_name, id)
79 } 79 }
tests/sync_test.rs
Old New
@@ -662,7 +662,7 @@ fn test_patch_review_across_repos() {
662 let sk = test_signing_key(); 662 let sk = test_signing_key();
663 let oid = dag::create_root_event(&alice_repo, &event, &sk).unwrap(); 663 let oid = dag::create_root_event(&alice_repo, &event, &sk).unwrap();
664 let id = oid.to_string(); 664 let id = oid.to_string();
665 let alice_ref = format!("refs/collab/patches/{}", id); 665 let alice_ref = git_collab::state::patch_events_ref(&id);
666 alice_repo 666 alice_repo
667 .reference(&alice_ref, oid, false, "patch create") 667 .reference(&alice_ref, oid, false, "patch create")
668 .unwrap(); 668 .unwrap();
@@ -670,7 +670,7 @@ fn test_patch_review_across_repos() {
670 sync::sync(&alice_repo, "origin").unwrap(); 670 sync::sync(&alice_repo, "origin").unwrap();
671 671
672 sync::sync(&bob_repo, "origin").unwrap(); 672 sync::sync(&bob_repo, "origin").unwrap();
673 let bob_ref = format!("refs/collab/patches/{}", id); 673 let bob_ref = git_collab::state::patch_events_ref(&id);
674 let review_event = Event { 674 let review_event = Event {
675 timestamp: now(), 675 timestamp: now(),
676 author: bob(), 676 author: bob(),
@@ -715,7 +715,7 @@ fn test_concurrent_review_and_revise() {
715 let sk = test_signing_key(); 715 let sk = test_signing_key();
716 let oid = dag::create_root_event(&alice_repo, &event, &sk).unwrap(); 716 let oid = dag::create_root_event(&alice_repo, &event, &sk).unwrap();
717 let id = oid.to_string(); 717 let id = oid.to_string();
718 let alice_ref = format!("refs/collab/patches/{}", id); 718 let alice_ref = git_collab::state::patch_events_ref(&id);
719 alice_repo 719 alice_repo
720 .reference(&alice_ref, oid, false, "patch") 720 .reference(&alice_ref, oid, false, "patch")
721 .unwrap(); 721 .unwrap();
@@ -729,12 +729,13 @@ fn test_concurrent_review_and_revise() {
729 commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), 729 commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(),
730 tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(), 730 tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(),
731 body: Some("Updated description".to_string()), 731 body: Some("Updated description".to_string()),
732 base: None,
732 }, 733 },
733 clock: 0, 734 clock: 0,
734 }; 735 };
735 dag::append_event(&alice_repo, &alice_ref, &revise_event, &sk).unwrap(); 736 dag::append_event(&alice_repo, &alice_ref, &revise_event, &sk).unwrap();
736 737
737 let bob_ref = format!("refs/collab/patches/{}", id); 738 let bob_ref = git_collab::state::patch_events_ref(&id);
738 let review_event = Event { 739 let review_event = Event {
739 timestamp: now(), 740 timestamp: now(),
740 author: bob(), 741 author: bob(),
@@ -1818,3 +1819,71 @@ fn cli_issue_show_renders_linked_commits_section() {
1818 show 1819 show
1819 ); 1820 );
1820 } 1821 }
1822
1823 // ---------------------------------------------------------------------------
1824 // Patches travel without pushing a branch
1825 // ---------------------------------------------------------------------------
1826
1827 #[test]
1828 fn patch_revisions_reach_a_second_clone_without_pushing_any_branch() {
1829 // The whole point of revision refs: a contributor needs no write access to
1830 // refs/heads/*, so `sync` alone must carry the patch, its revisions and the
1831 // objects behind them.
1832 let cluster = TestCluster::new();
1833 let alice_repo = cluster.alice_repo();
1834
1835 let base = make_commit_with_message(&alice_repo, "base");
1836 let feat = {
1837 let parent = alice_repo.find_commit(base).unwrap();
1838 let sig = git2::Signature::now("Alice", "alice@example.com").unwrap();
1839 let blob = alice_repo.blob(b"feature work").unwrap();
1840 let mut tb = alice_repo
1841 .treebuilder(Some(&parent.tree().unwrap()))
1842 .unwrap();
1843 tb.insert("feature.txt", blob, 0o100644).unwrap();
1844 let tree_oid = tb.write().unwrap();
1845 let tree = alice_repo.find_tree(tree_oid).unwrap();
1846 alice_repo
1847 .commit(
1848 Some("refs/heads/feat"),
1849 &sig,
1850 &sig,
1851 "feature",
1852 &tree,
1853 &[&parent],
1854 )
1855 .unwrap()
1856 };
1857
1858 cluster.run_collab_ok(
1859 cluster.alice_dir.path(),
1860 &["patch", "create", "-t", "No branch push", "-B", "feat"],
1861 );
1862 sync::sync(&alice_repo, "origin").unwrap();
1863
1864 let bob_repo = cluster.bob_repo();
1865 sync::sync(&bob_repo, "origin").unwrap();
1866
1867 // Reopen: git2 caches the ref list at open time, and sync fetched via git.
1868 let bob_repo = Repository::open(cluster.bob_dir.path()).unwrap();
1869 let patches = state::list_patches(&bob_repo).unwrap();
1870 assert_eq!(patches.len(), 1, "bob should see alice's patch");
1871 let patch = &patches[0];
1872 assert_eq!(patch.title, "No branch push");
1873
1874 let revision_ref = format!("refs/collab/patches/{}/r/1", patch.id);
1875 assert_eq!(
1876 bob_repo.refname_to_id(&revision_ref).ok(),
1877 Some(feat),
1878 "revision 1's ref must have travelled"
1879 );
1880 assert!(
1881 bob_repo.find_commit(feat).is_ok(),
1882 "and so must the objects behind it"
1883 );
1884 assert!(
1885 bob_repo.find_reference("refs/heads/feat").is_err(),
1886 "no branch was ever pushed"
1887 );
1888 assert_eq!(patch.resolve_head(&bob_repo).unwrap(), feat);
1889 }