a73x

e953328a

Read patches written by older git-collab versions

a73x   2026-08-08 18:44

Commit message
Read patches written by older git-collab versions
src/event.rs
Old New
@@ -48,6 +48,18 @@ pub enum Action {
48 IssueCommitLink { 48 IssueCommitLink {
49 commit: String, 49 commit: String,
50 }, 50 },
51 /// Patches created before revisions recorded their commit and tree omit
52 /// both fields; an older shape still calls the whole variant `PatchCreate`
53 /// and puts the head in `head_commit` (sometimes a branch name, sometimes
54 /// a raw OID — `PatchState::resolve_head` handles either). Defaulting
55 /// `commit`/`tree` to empty keeps those patches readable; an empty commit
56 /// means "not recorded", never "the null OID".
57 ///
58 /// Skipping the empty case on the way out matters as much as defaulting it
59 /// on the way in: signatures are checked by re-serializing the event, so
60 /// writing back a field the signer never wrote would invalidate every
61 /// legacy signature and `sync` would reject the ref. Nothing current code
62 /// writes is ever empty, so this never fires for a new event.
51 #[serde(rename = "patch.create", alias = "PatchCreate")] 63 #[serde(rename = "patch.create", alias = "PatchCreate")]
52 PatchCreate { 64 PatchCreate {
53 title: String, 65 title: String,
@@ -57,24 +69,44 @@ pub enum Action {
57 branch: String, 69 branch: String,
58 #[serde(default, skip_serializing_if = "Option::is_none")] 70 #[serde(default, skip_serializing_if = "Option::is_none")]
59 fixes: Option<String>, 71 fixes: Option<String>,
72 #[serde(default, skip_serializing_if = "String::is_empty")]
60 commit: String, 73 commit: String,
74 #[serde(default, skip_serializing_if = "String::is_empty")]
61 tree: String, 75 tree: String,
62 /// Base branch tip OID at creation time (for merge detection). 76 /// Base branch tip OID at creation time (for merge detection).
63 #[serde(default, skip_serializing_if = "Option::is_none")] 77 #[serde(default, skip_serializing_if = "Option::is_none")]
64 base_commit: Option<String>, 78 base_commit: Option<String>,
65 }, 79 },
66 #[serde(rename = "patch.revision")] 80 /// Historically written as `patch.revise` carrying only a note, before
81 /// revisions recorded the commit and tree they pointed at. Note that the
82 /// variant rename is not reversible on serialization, so these events
83 /// cannot round-trip for signature purposes however `commit`/`tree` are
84 /// handled — see issue 2a79b3ab.
85 #[serde(rename = "patch.revision", alias = "patch.revise")]
67 PatchRevision { 86 PatchRevision {
87 #[serde(default, skip_serializing_if = "String::is_empty")]
68 commit: String, 88 commit: String,
89 #[serde(default, skip_serializing_if = "String::is_empty")]
69 tree: String, 90 tree: String,
70 #[serde(default, skip_serializing_if = "Option::is_none")] 91 #[serde(default, skip_serializing_if = "Option::is_none")]
71 body: Option<String>, 92 body: Option<String>,
72 }, 93 },
94 /// `revision` is `None` on reviews written before reviews were scoped to a
95 /// revision. It is not the same as revision 1: `PatchState` attributes
96 /// such a review to whichever revision was current when it was written,
97 /// which is what the vote-per-revision rule needs to keep successive
98 /// review rounds by one author from superseding each other.
99 ///
100 /// A `Some` revision is part of the signed payload. A `None` one is
101 /// recovered from the event's position in the DAG, which is a weaker
102 /// guarantee — see the invariant documented at the attribution site in
103 /// `PatchState::from_ref_uncached`, and issue 33b5e541.
73 #[serde(rename = "patch.review")] 104 #[serde(rename = "patch.review")]
74 PatchReview { 105 PatchReview {
75 verdict: ReviewVerdict, 106 verdict: ReviewVerdict,
76 body: String, 107 body: String,
77 revision: u32, 108 #[serde(default, skip_serializing_if = "Option::is_none")]
109 revision: Option<u32>,
78 }, 110 },
79 #[serde(rename = "patch.comment")] 111 #[serde(rename = "patch.comment")]
80 PatchComment { body: String }, 112 PatchComment { body: String },
src/patch.rs
Old New
@@ -19,17 +19,25 @@ fn auto_detect_revision(
19 ) -> Result<Option<Revision>, Error> { 19 ) -> Result<Option<Revision>, Error> {
20 let current_rev = patch.revisions.last().map(|r| r.number).unwrap_or(0); 20 let current_rev = patch.revisions.last().map(|r| r.number).unwrap_or(0);
21 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
22 // Try to resolve the branch tip 35 // Try to resolve the branch tip
23 let tip_oid = match patch.resolve_head(repo) { 36 let tip_oid = match patch.resolve_head(repo) {
24 Ok(oid) => oid, 37 Ok(oid) => oid,
25 Err(_) => return Ok(None), // Branch deleted or unavailable 38 Err(_) => return Ok(None), // Branch deleted or unavailable
26 }; 39 };
27 40
28 let last_commit = patch
29 .revisions
30 .last()
31 .map(|r| r.commit.as_str())
32 .unwrap_or("");
33 let tip_hex = tip_oid.to_string(); 41 let tip_hex = tip_oid.to_string();
34 42
35 if tip_hex == last_commit { 43 if tip_hex == last_commit {
@@ -352,7 +360,7 @@ pub fn review(
352 action: Action::PatchReview { 360 action: Action::PatchReview {
353 verdict, 361 verdict,
354 body: body.to_string(), 362 body: body.to_string(),
355 revision: rev, 363 revision: Some(rev),
356 }, 364 },
357 clock: 0, 365 clock: 0,
358 }; 366 };
@@ -563,11 +571,9 @@ pub fn patch_log_to_writer(
563 } 571 }
564 572
565 for (i, rev) in patch.revisions.iter().enumerate() { 573 for (i, rev) in patch.revisions.iter().enumerate() {
566 let short_oid = if rev.commit.len() >= 8 { 574 let short_oid = rev
567 &rev.commit[..8] 575 .short_commit()
568 } else { 576 .unwrap_or_else(|| state::UNKNOWN_COMMIT.to_string());
569 &rev.commit
570 };
571 let label = if i == 0 { " (initial)" } else { "" }; 577 let label = if i == 0 { " (initial)" } else { "" };
572 let body_display = rev 578 let body_display = rev
573 .body 579 .body
src/server/http/repo/patches.rs
Old New
@@ -31,6 +31,12 @@ pub struct PatchesTemplate {
31 pub struct RevisionView { 31 pub struct RevisionView {
32 pub number: u32, 32 pub number: u32,
33 pub commit: String, 33 pub commit: String,
34 /// Abbreviated commit for display, or `None` when the revision recorded no
35 /// commit — patches created before revisions carried one. Truncated here
36 /// rather than in the template, where an out-of-range slice would panic
37 /// the request handler and drop the connection. The `None` case renders as
38 /// `state::UNKNOWN_COMMIT`, spelled out in the template.
39 pub short_commit: Option<String>,
34 pub timestamp: String, 40 pub timestamp: String,
35 pub body: Option<String>, 41 pub body: Option<String>,
36 } 42 }
@@ -153,6 +159,7 @@ pub async fn patch_detail(
153 .into_iter() 159 .into_iter()
154 .map(|r| RevisionView { 160 .map(|r| RevisionView {
155 number: r.number, 161 number: r.number,
162 short_commit: r.short_commit(),
156 commit: r.commit, 163 commit: r.commit,
157 timestamp: r.timestamp, 164 timestamp: r.timestamp,
158 body: r.body, 165 body: r.body,
src/server/http/templates/patch_detail.html
Old New
@@ -30,7 +30,7 @@
30 {% for rev in patch.revisions %} 30 {% for rev in patch.revisions %}
31 <tr> 31 <tr>
32 <td>{{ rev.number }}</td> 32 <td>{{ rev.number }}</td>
33 <td class="mono"><a href="/{{ repo_name }}/diff/{{ rev.commit }}">{{ rev.commit[..8] }}</a></td> 33 <td class="mono">{% if let Some(short) = rev.short_commit %}<a href="/{{ repo_name }}/diff/{{ rev.commit }}">{{ short }}</a>{% else %}<span style="color: #666;">unknown</span>{% endif %}</td>
34 <td class="mono" style="color: #666;">{{ rev.timestamp }}</td> 34 <td class="mono" style="color: #666;">{{ rev.timestamp }}</td>
35 <td>{% if let Some(body) = rev.body %}{{ body }}{% endif %}</td> 35 <td>{% if let Some(body) = rev.body %}{{ body }}{% endif %}</td>
36 </tr> 36 </tr>
src/state.rs
Old New
@@ -161,12 +161,31 @@ impl fmt::Display for PatchStatus {
161 #[derive(Debug, Clone, Serialize, Deserialize)] 161 #[derive(Debug, Clone, Serialize, Deserialize)]
162 pub struct Revision { 162 pub struct Revision {
163 pub number: u32, 163 pub number: u32,
164 /// Commit OID, or `""` when none was recorded — patches created before
165 /// revisions carried one. Consumers of the JSON output must treat the
166 /// empty string as "unknown", not as an OID: it will not parse as one.
164 pub commit: String, 167 pub commit: String,
168 /// Tree OID, or `""` alongside an unrecorded `commit`.
165 pub tree: String, 169 pub tree: String,
166 pub body: Option<String>, 170 pub body: Option<String>,
167 pub timestamp: String, 171 pub timestamp: String,
168 } 172 }
169 173
174 impl Revision {
175 /// Abbreviated commit for display, or `None` when none was recorded.
176 /// Truncating by chars rather than bytes keeps this total for any input.
177 pub fn short_commit(&self) -> Option<String> {
178 if self.commit.is_empty() {
179 return None;
180 }
181 Some(self.commit.chars().take(8).collect())
182 }
183 }
184
185 /// What to show in place of a commit that was never recorded. Shared so the
186 /// CLI, the TUI and the web UI say the same thing.
187 pub const UNKNOWN_COMMIT: &str = "unknown";
188
170 #[derive(Debug, Clone, Serialize, Deserialize)] 189 #[derive(Debug, Clone, Serialize, Deserialize)]
171 pub struct InlineComment { 190 pub struct InlineComment {
172 pub author: Author, 191 pub author: Author,
@@ -551,8 +570,12 @@ impl PatchState {
551 } 570 }
552 Action::PatchRevision { commit, tree, body } => { 571 Action::PatchRevision { commit, tree, body } => {
553 if let Some(ref mut s) = state { 572 if let Some(ref mut s) = state {
554 // Dedup by commit OID — skip if already seen 573 // Dedup by commit OID — skip if already seen. Legacy
555 let already_seen = s.revisions.iter().any(|r| r.commit == commit); 574 // revisions recorded no commit, and collapsing those
575 // into one another would also collapse the review
576 // rounds that sat between them.
577 let already_seen =
578 !commit.is_empty() && s.revisions.iter().any(|r| r.commit == commit);
556 if !already_seen { 579 if !already_seen {
557 let number = s.revisions.len() as u32 + 1; 580 let number = s.revisions.len() as u32 + 1;
558 s.revisions.push(Revision { 581 s.revisions.push(Revision {
@@ -571,6 +594,42 @@ impl PatchState {
571 revision, 594 revision,
572 } => { 595 } => {
573 if let Some(ref mut s) = state { 596 if let Some(ref mut s) = state {
597 // Reviews written before reviews were revision-scoped
598 // carry no revision. Attribute them to the revision
599 // that was current at this point in the walk, which is
600 // where the reviewer was in fact looking.
601 //
602 // INVARIANT: this is only well-defined because every
603 // event that can reach this branch sits in a linear
604 // prefix of the DAG. Current code always writes
605 // `Some(n)`, so no new event extends the region where
606 // position matters, and no collab ref in the wild has
607 // a merge commit. Walk order over a *forked* DAG is a
608 // partial order that `Sort::TOPOLOGICAL` completes by
609 // an internal tiebreak, so two clients holding the
610 // same events joined in different parent orders could
611 // attribute the same review to different revisions.
612 // That is worse than a display bug: attribution feeds
613 // the vote-supersession rule below, so a divergence
614 // can drop a review rather than relabel one. For the
615 // same reason, note that signatures cover event
616 // content but not parent links — a chain rewrite
617 // (already unauthenticated) can now re-attribute
618 // someone else's legacy review and delete a vote
619 // through that same rule. The robust fix is to fold in
620 // a total order over `(clock, oid)`, which is
621 // content-derived and therefore client-independent, as
622 // the status fold below already does: issue 33b5e541.
623 //
624 // `state` is `Some` only after a PatchCreate, which
625 // always seeds revision 1, so the list is never empty
626 // here; `max(1)` guards a malformed DAG only.
627 debug_assert!(
628 !s.revisions.is_empty(),
629 "PatchCreate always seeds revision 1"
630 );
631 let revision =
632 revision.unwrap_or_else(|| (s.revisions.len() as u32).max(1));
574 // A reviewer holds one current vote per revision: a new 633 // A reviewer holds one current vote per revision: a new
575 // vote supersedes their previous one. Comment-verdict 634 // vote supersedes their previous one. Comment-verdict
576 // reviews are not votes and accumulate. 635 // reviews are not votes and accumulate.
src/tui/mod.rs
Old New
@@ -422,7 +422,7 @@ mod tests {
422 let action = Action::PatchReview { 422 let action = Action::PatchReview {
423 verdict: ReviewVerdict::Approve, 423 verdict: ReviewVerdict::Approve,
424 body: "lgtm".to_string(), 424 body: "lgtm".to_string(),
425 revision: 1, 425 revision: Some(1),
426 }; 426 };
427 assert_eq!(action_type_label(&action), "Patch Review"); 427 assert_eq!(action_type_label(&action), "Patch Review");
428 } 428 }
@@ -492,7 +492,7 @@ mod tests {
492 action: Action::PatchReview { 492 action: Action::PatchReview {
493 verdict: ReviewVerdict::Approve, 493 verdict: ReviewVerdict::Approve,
494 body: "Looks good!".to_string(), 494 body: "Looks good!".to_string(),
495 revision: 1, 495 revision: Some(1),
496 }, 496 },
497 clock: 0, 497 clock: 0,
498 }; 498 };
src/tui/widgets.rs
Old New
@@ -647,11 +647,9 @@ fn build_patch_detail_text(app: &App) -> Text<'static> {
647 .add_modifier(Modifier::BOLD), 647 .add_modifier(Modifier::BOLD),
648 )); 648 ));
649 for (i, rev) in patch.revisions.iter().enumerate() { 649 for (i, rev) in patch.revisions.iter().enumerate() {
650 let short = if rev.commit.len() >= 8 { 650 let short = rev
651 &rev.commit[..8] 651 .short_commit()
652 } else { 652 .unwrap_or_else(|| crate::state::UNKNOWN_COMMIT.to_string());
653 &rev.commit
654 };
655 let marker = if i == app.patch_revision_idx { 653 let marker = if i == app.patch_revision_idx {
656 "> " 654 "> "
657 } else { 655 } else {
tests/common/mod.rs
Old New
@@ -195,6 +195,68 @@ pub fn init_repo(dir: &Path, author: &Author) -> Repository {
195 repo 195 repo
196 } 196 }
197 197
198 /// Append an event commit whose `action` JSON is supplied verbatim, with
199 /// `parent` as its only parent (`None` produces an orphan root commit).
200 /// Returns the new commit OID. The caller owns the ref.
201 ///
202 /// Historical on-disk shapes — `patch.revise`, reviews with no `revision`,
203 /// creates with no `commit`/`tree` — can no longer be produced by serializing
204 /// today's `Action`, so tests covering them write the JSON directly. The
205 /// commit tree and its ed25519 signature are built exactly as `dag` builds
206 /// them, so the result is indistinguishable from a real historical event.
207 pub fn write_raw_event(
208 repo: &Repository,
209 parent: Option<git2::Oid>,
210 action: serde_json::Value,
211 clock: u64,
212 ) -> git2::Oid {
213 use base64::engine::general_purpose::STANDARD;
214 use base64::Engine;
215 use ed25519_dalek::Signer;
216
217 let author = alice();
218 let sk = test_signing_key();
219 let event = serde_json::json!({
220 "timestamp": now(),
221 "author": { "name": author.name, "email": author.email },
222 "action": action,
223 "clock": clock,
224 });
225
226 // serde_json::Value serializes with sorted keys, which is what
227 // signing::canonical_json relies on.
228 let canonical = serde_json::to_string(&event).unwrap();
229 let signature = sk.sign(canonical.as_bytes());
230
231 let event_blob = repo
232 .blob(serde_json::to_string_pretty(&event).unwrap().as_bytes())
233 .unwrap();
234 let sig_blob = repo
235 .blob(STANDARD.encode(signature.to_bytes()).as_bytes())
236 .unwrap();
237 let pubkey_blob = repo
238 .blob(STANDARD.encode(sk.verifying_key().to_bytes()).as_bytes())
239 .unwrap();
240 let manifest_blob = repo
241 .blob(br#"{"version":1,"format":"git-collab"}"#)
242 .unwrap();
243
244 let mut tb = repo.treebuilder(None).unwrap();
245 tb.insert("event.json", event_blob, 0o100644).unwrap();
246 tb.insert("signature", sig_blob, 0o100644).unwrap();
247 tb.insert("pubkey", pubkey_blob, 0o100644).unwrap();
248 tb.insert("manifest.json", manifest_blob, 0o100644).unwrap();
249 let tree = repo.find_tree(tb.write().unwrap()).unwrap();
250
251 let sig = git2::Signature::now(&author.name, &author.email).unwrap();
252 let parents: Vec<git2::Commit> = parent
253 .map(|p| vec![repo.find_commit(p).unwrap()])
254 .unwrap_or_default();
255 let parent_refs: Vec<&git2::Commit> = parents.iter().collect();
256 repo.commit(None, &sig, &sig, "legacy event", &tree, &parent_refs)
257 .unwrap()
258 }
259
198 /// Open an issue using DAG primitives. Returns (ref_name, id). 260 /// Open an issue using DAG primitives. Returns (ref_name, id).
199 pub fn open_issue(repo: &Repository, author: &Author, title: &str) -> (String, String) { 261 pub fn open_issue(repo: &Repository, author: &Author, title: &str) -> (String, String) {
200 let sk = test_signing_key(); 262 let sk = test_signing_key();
@@ -322,7 +384,7 @@ pub fn add_review_on(
322 action: Action::PatchReview { 384 action: Action::PatchReview {
323 verdict, 385 verdict,
324 body: "review comment".to_string(), 386 body: "review comment".to_string(),
325 revision, 387 revision: Some(revision),
326 }, 388 },
327 clock: 0, 389 clock: 0,
328 }; 390 };
tests/fixtures/legacy_events/patch_create_head_commit_variant.json
Old New
@@ -0,0 +1,14 @@
1 {
2 "timestamp": "2026-03-21T15:42:16.382345234+00:00",
3 "author": {
4 "name": "a73x",
5 "email": "dev@a73x.sh"
6 },
7 "action": {
8 "type": "PatchCreate",
9 "title": "Add --json output flag to list/show commands",
10 "body": "Fixes 79125c77. Adds --json flag to issue list, issue show, patch list, patch show for machine-parseable output in agent workflows.",
11 "base_ref": "main",
12 "head_commit": "db059e663d7a435f288bd2832bee4e9f374474d2"
13 }
14 }
\ No newline at end of file 14 \ No newline at end of file
tests/fixtures/legacy_events/patch_create_missing_commit_tree.json
Old New
@@ -0,0 +1,16 @@
1 {
2 "timestamp": "2026-03-21T19:08:48.630385084+00:00",
3 "author": {
4 "name": "a73x",
5 "email": "dev@a73x.sh"
6 },
7 "action": {
8 "type": "patch.create",
9 "title": "Avoid double DAG walk after auto_detect_revision",
10 "body": "",
11 "base_ref": "main",
12 "branch": "worktree-agent-aef11cfd",
13 "fixes": "53e698d5"
14 },
15 "clock": 1
16 }
\ No newline at end of file 16 \ No newline at end of file
tests/fixtures/legacy_events/patch_review_missing_revision.json
Old New
@@ -0,0 +1,13 @@
1 {
2 "timestamp": "2026-03-21T19:09:05.299717785+00:00",
3 "author": {
4 "name": "a73x",
5 "email": "dev@a73x.sh"
6 },
7 "action": {
8 "type": "patch.review",
9 "verdict": "RequestChanges",
10 "body": "Good approach returning Option<Revision> instead of u32 — that's the right move. But you've introduced the same 3-line pattern in comment(), review(), and merge():\n\n let mut patch = patch;\n if let Some(rev) = auto_detect_revision(repo, &ref_name, &patch, &sk)? {\n patch.revisions.push(rev);\n }\n\nExtract that into a small helper, e.g. `auto_detect_and_update(repo, ref_name, &mut patch, &sk)?` that does the detect + push in one call. That way callers just need one line. Also avoids the awkward `let mut patch = patch;` rebinding."
11 },
12 "clock": 2
13 }
\ No newline at end of file 13 \ No newline at end of file
tests/fixtures/legacy_events/patch_review_with_revision.json
Old New
@@ -0,0 +1,14 @@
1 {
2 "timestamp": "2026-03-22T10:03:14.967814484+00:00",
3 "author": {
4 "name": "a73x",
5 "email": "dev@a73x.sh"
6 },
7 "action": {
8 "type": "patch.review",
9 "verdict": "Approve",
10 "body": "LGTM — confirmed no stale .clone() calls remain. Good to merge.",
11 "revision": 2
12 },
13 "clock": 5
14 }
\ No newline at end of file 14 \ No newline at end of file
tests/fixtures/legacy_events/patch_revise_body_only.json
Old New
@@ -0,0 +1,12 @@
1 {
2 "timestamp": "2026-03-21T19:10:36.869240870+00:00",
3 "author": {
4 "name": "a73x",
5 "email": "dev@a73x.sh"
6 },
7 "action": {
8 "type": "patch.revise",
9 "body": "Address review: extract auto_detect_and_update helper to eliminate 3x repeated pattern"
10 },
11 "clock": 3
12 }
\ No newline at end of file 12 \ No newline at end of file
tests/legacy_patch_test.rs
Old New
@@ -0,0 +1,418 @@
1 //! Recovery of patches written by older versions of git-collab.
2 //!
3 //! Between them, the historical shapes drop `commit`/`tree` from
4 //! `patch.create`, name the revision event `patch.revise`, and omit
5 //! `revision` from `patch.review`. Every event JSON asserted on here was
6 //! captured verbatim from a real patch in this repository's own collab refs
7 //! (`tests/fixtures/legacy_events/`).
8
9 mod common;
10
11 use std::path::PathBuf;
12
13 use common::{test_signing_key, write_raw_event, ServerHarness, TestRepo};
14 use git_collab::event::{Action, Event, ReviewVerdict};
15 use git_collab::signing::{self, DetachedSignature, VerifyStatus};
16 use git_collab::state::{PatchState, PatchStatus};
17 use serde_json::json;
18
19 fn fixture(name: &str) -> String {
20 let path = PathBuf::from(env!("CARGO_MANIFEST_DIR"))
21 .join("tests/fixtures/legacy_events")
22 .join(name);
23 std::fs::read_to_string(&path)
24 .unwrap_or_else(|e| panic!("reading fixture {}: {}", path.display(), e))
25 }
26
27 /// Sign a fixture's bytes as its original author would have, then verify that
28 /// signature against the event as deserialized by current code. `Valid` means
29 /// the event survives the deserialize/re-serialize round trip byte for byte,
30 /// which is what decides whether `sync` accepts a ref carrying it.
31 fn round_trip_status(name: &str) -> VerifyStatus {
32 use base64::engine::general_purpose::STANDARD;
33 use base64::Engine;
34 use ed25519_dalek::Signer;
35
36 let raw = fixture(name);
37 let original: serde_json::Value = serde_json::from_str(&raw).unwrap();
38 let canonical = serde_json::to_string(&original).unwrap();
39
40 let sk = test_signing_key();
41 let detached = DetachedSignature {
42 signature: STANDARD.encode(sk.sign(canonical.as_bytes()).to_bytes()),
43 pubkey: STANDARD.encode(sk.verifying_key().to_bytes()),
44 };
45
46 let event: Event = serde_json::from_str(&raw).unwrap();
47 signing::verify_detached(&event, &detached).unwrap()
48 }
49
50 // ===========================================================================
51 // Deserialization of real historical events
52 // ===========================================================================
53
54 #[test]
55 fn legacy_patch_create_without_commit_or_tree_deserializes() {
56 let event: Event = serde_json::from_str(&fixture("patch_create_missing_commit_tree.json"))
57 .expect("legacy patch.create must deserialize");
58
59 match event.action {
60 Action::PatchCreate {
61 title,
62 base_ref,
63 branch,
64 fixes,
65 commit,
66 tree,
67 base_commit,
68 ..
69 } => {
70 assert_eq!(title, "Avoid double DAG walk after auto_detect_revision");
71 assert_eq!(base_ref, "main");
72 assert_eq!(branch, "worktree-agent-aef11cfd");
73 assert_eq!(fixes.as_deref(), Some("53e698d5"));
74 assert!(commit.is_empty(), "unrecorded commit reads as empty");
75 assert!(tree.is_empty(), "unrecorded tree reads as empty");
76 assert!(base_commit.is_none());
77 }
78 other => panic!("expected PatchCreate, got {:?}", other),
79 }
80 }
81
82 #[test]
83 fn legacy_patch_create_with_head_commit_deserializes() {
84 let event: Event = serde_json::from_str(&fixture("patch_create_head_commit_variant.json"))
85 .expect("legacy PatchCreate must deserialize");
86
87 match event.action {
88 Action::PatchCreate {
89 title,
90 branch,
91 commit,
92 tree,
93 ..
94 } => {
95 assert_eq!(title, "Add --json output flag to list/show commands");
96 // This generation recorded the head SHA in a field named
97 // `head_commit`, which is aliased onto `branch`.
98 assert_eq!(branch, "db059e663d7a435f288bd2832bee4e9f374474d2");
99 assert!(commit.is_empty());
100 assert!(tree.is_empty());
101 }
102 other => panic!("expected PatchCreate, got {:?}", other),
103 }
104 assert_eq!(event.clock, 0, "this generation had no clock field");
105 }
106
107 #[test]
108 fn legacy_patch_review_without_revision_deserializes() {
109 let event: Event = serde_json::from_str(&fixture("patch_review_missing_revision.json"))
110 .expect("legacy patch.review must deserialize");
111
112 match event.action {
113 Action::PatchReview {
114 verdict,
115 body,
116 revision,
117 } => {
118 assert_eq!(verdict, ReviewVerdict::RequestChanges);
119 assert!(body.starts_with("Good approach returning Option<Revision>"));
120 assert_eq!(revision, None, "no revision was recorded");
121 }
122 other => panic!("expected PatchReview, got {:?}", other),
123 }
124 }
125
126 #[test]
127 fn current_patch_review_keeps_its_revision() {
128 let event: Event = serde_json::from_str(&fixture("patch_review_with_revision.json")).unwrap();
129
130 match event.action {
131 Action::PatchReview {
132 verdict, revision, ..
133 } => {
134 assert_eq!(verdict, ReviewVerdict::Approve);
135 assert_eq!(revision, Some(2));
136 }
137 other => panic!("expected PatchReview, got {:?}", other),
138 }
139 }
140
141 #[test]
142 fn legacy_patch_revise_deserializes_as_a_revision() {
143 let event: Event = serde_json::from_str(&fixture("patch_revise_body_only.json"))
144 .expect("legacy patch.revise must deserialize");
145
146 match event.action {
147 Action::PatchRevision { commit, tree, body } => {
148 assert!(commit.is_empty());
149 assert!(tree.is_empty());
150 assert_eq!(
151 body.as_deref(),
152 Some("Address review: extract auto_detect_and_update helper to eliminate 3x repeated pattern")
153 );
154 }
155 other => panic!("expected PatchRevision, got {:?}", other),
156 }
157 }
158
159 // ===========================================================================
160 // Signature round-tripping (decides whether `sync` accepts these refs)
161 // ===========================================================================
162
163 #[test]
164 fn legacy_patch_create_signature_survives_the_round_trip() {
165 // Deserializing adds no `commit`/`tree` back, so the bytes the signer
166 // signed are the bytes we re-serialize.
167 assert_eq!(
168 round_trip_status("patch_create_missing_commit_tree.json"),
169 VerifyStatus::Valid
170 );
171 }
172
173 #[test]
174 fn legacy_patch_review_signature_survives_the_round_trip() {
175 assert_eq!(
176 round_trip_status("patch_review_missing_revision.json"),
177 VerifyStatus::Valid
178 );
179 }
180
181 #[test]
182 fn current_event_signature_survives_the_round_trip() {
183 assert_eq!(
184 round_trip_status("patch_review_with_revision.json"),
185 VerifyStatus::Valid
186 );
187 }
188
189 #[test]
190 fn renamed_legacy_shapes_still_cannot_round_trip() {
191 // `patch.revise` re-serializes as `patch.revision`, and the older
192 // `PatchCreate`/`head_commit` spelling normalizes to the current one, so
193 // no serde attribute can make these byte-identical. Their refs stay
194 // unsyncable until verification compares stored bytes — issue 2a79b3ab.
195 // Asserted so the day that changes, this test says so.
196 assert_eq!(
197 round_trip_status("patch_revise_body_only.json"),
198 VerifyStatus::Invalid
199 );
200 assert_eq!(
201 round_trip_status("patch_create_head_commit_variant.json"),
202 VerifyStatus::Invalid
203 );
204 }
205
206 // ===========================================================================
207 // Materializing a legacy DAG into PatchState
208 // ===========================================================================
209
210 /// Build the event chain of patch 05e5fa7c ("Avoid double DAG walk after
211 /// auto_detect_revision"): create, then three review rounds interleaved with
212 /// two revises, then a merge. Returns (ref_name, id).
213 fn write_legacy_review_history(repo: &git2::Repository) -> (String, String) {
214 let root = write_raw_event(
215 repo,
216 None,
217 json!({
218 "type": "patch.create",
219 "title": "Avoid double DAG walk after auto_detect_revision",
220 "body": "",
221 "base_ref": "main",
222 "branch": "worktree-agent-aef11cfd",
223 "fixes": "53e698d5",
224 }),
225 1,
226 );
227 let mut tip = write_raw_event(
228 repo,
229 Some(root),
230 json!({
231 "type": "patch.review",
232 "verdict": "RequestChanges",
233 "body": "Extract that into a small helper.",
234 }),
235 2,
236 );
237 tip = write_raw_event(
238 repo,
239 Some(tip),
240 json!({
241 "type": "patch.revise",
242 "body": "Address review: extract auto_detect_and_update helper",
243 }),
244 3,
245 );
246 tip = write_raw_event(
247 repo,
248 Some(tip),
249 json!({
250 "type": "patch.review",
251 "verdict": "RequestChanges",
252 "body": "One remaining nit: the let mut p = p rebinding.",
253 }),
254 4,
255 );
256 tip = write_raw_event(
257 repo,
258 Some(tip),
259 json!({
260 "type": "patch.revise",
261 "body": "Address r2 feedback: remove let mut p = p rebinding",
262 }),
263 5,
264 );
265 tip = write_raw_event(
266 repo,
267 Some(tip),
268 json!({
269 "type": "patch.review",
270 "verdict": "Approve",
271 "body": "Clean. Ship it.",
272 }),
273 6,
274 );
275 tip = write_raw_event(repo, Some(tip), json!({ "type": "patch.merge" }), 7);
276
277 let id = root.to_string();
278 let ref_name = format!("refs/collab/patches/{}", id);
279 repo.reference(&ref_name, tip, false, "legacy patch")
280 .unwrap();
281 (ref_name, id)
282 }
283
284 #[test]
285 fn legacy_patch_with_review_history_loads_intact() {
286 let work = TestRepo::new("Alice", "alice@example.com");
287 let repo = git2::Repository::open(work.dir.path()).unwrap();
288 let (ref_name, id) = write_legacy_review_history(&repo);
289
290 let patch = PatchState::from_ref_uncached(&repo, &ref_name, &id)
291 .expect("legacy patch must materialize");
292
293 assert_eq!(
294 patch.title,
295 "Avoid double DAG walk after auto_detect_revision"
296 );
297 assert_eq!(patch.branch, "worktree-agent-aef11cfd");
298 assert_eq!(patch.status, PatchStatus::Merged);
299
300 // Each revise is its own revision even though none of them recorded a
301 // commit — collapsing them would also collapse the reviews between them.
302 let numbers: Vec<u32> = patch.revisions.iter().map(|r| r.number).collect();
303 assert_eq!(numbers, vec![1, 2, 3]);
304 assert!(patch.revisions.iter().all(|r| r.commit.is_empty()));
305 assert_eq!(
306 patch.revisions[1].body.as_deref(),
307 Some("Address review: extract auto_detect_and_update helper")
308 );
309 assert_eq!(
310 patch.revisions[2].body.as_deref(),
311 Some("Address r2 feedback: remove let mut p = p rebinding")
312 );
313
314 // All three reviews survive: they are attributed to the revision that was
315 // current when each was written, so the one-vote-per-revision rule does
316 // not treat them as one author revoting on the same revision.
317 let verdicts: Vec<ReviewVerdict> = patch.reviews.iter().map(|r| r.verdict).collect();
318 assert_eq!(
319 verdicts,
320 vec![
321 ReviewVerdict::RequestChanges,
322 ReviewVerdict::RequestChanges,
323 ReviewVerdict::Approve
324 ]
325 );
326 let review_revs: Vec<Option<u32>> = patch.reviews.iter().map(|r| r.revision).collect();
327 assert_eq!(review_revs, vec![Some(1), Some(2), Some(3)]);
328 }
329
330 #[test]
331 fn legacy_patch_appears_in_patch_list() {
332 let work = TestRepo::new("Alice", "alice@example.com");
333 let repo = git2::Repository::open(work.dir.path()).unwrap();
334 write_legacy_review_history(&repo);
335 drop(repo);
336
337 let out = work.run_ok(&["patch", "list", "--all"]);
338 assert!(
339 out.contains("Avoid double DAG walk"),
340 "legacy patch missing from list output: {}",
341 out
342 );
343 }
344
345 // ===========================================================================
346 // Writes against a legacy patch
347 // ===========================================================================
348
349 #[test]
350 fn commenting_on_a_legacy_patch_does_not_invent_a_revision() {
351 let work = TestRepo::new("Alice", "alice@example.com");
352 let head = work.git(&["rev-parse", "HEAD"]).trim().to_string();
353 let repo = git2::Repository::open(work.dir.path()).unwrap();
354
355 // The oldest patches stored the head SHA where `branch` now lives, so the
356 // head still resolves even though revision 1 recorded no commit.
357 let root = write_raw_event(
358 &repo,
359 None,
360 json!({
361 "type": "patch.create",
362 "title": "Legacy patch with resolvable head",
363 "body": "",
364 "base_ref": "main",
365 "head_commit": head,
366 }),
367 1,
368 );
369 let id = root.to_string();
370 repo.reference(
371 &format!("refs/collab/patches/{}", id),
372 root,
373 false,
374 "legacy",
375 )
376 .unwrap();
377 drop(repo);
378
379 work.run_ok(&["patch", "comment", &id[..8], "-b", "a comment"]);
380
381 let out = work.run_ok(&["patch", "show", &id[..8], "--json"]);
382 let shown: serde_json::Value = serde_json::from_str(&out).unwrap();
383 assert_eq!(
384 shown["revisions"].as_array().unwrap().len(),
385 1,
386 "auto-detection must not turn an unrecorded commit into a new revision: {}",
387 out
388 );
389 }
390
391 // ===========================================================================
392 // Server rendering
393 // ===========================================================================
394
395 #[test]
396 fn patch_detail_page_renders_a_revision_with_no_commit() {
397 let harness = ServerHarness::new("legacy-patch-detail");
398 harness.push_head();
399
400 let repo = git2::Repository::open(harness.work_repo().dir.path()).unwrap();
401 let (_ref_name, id) = write_legacy_review_history(&repo);
402 drop(repo);
403 harness.push_collab_refs();
404
405 let page = harness.get_ok(&format!("/{}/patches/{}", harness.repo_name(), id));
406 assert!(page.body.contains("Avoid double DAG walk"));
407 assert!(
408 page.body.contains(">unknown<"),
409 "revisions with no commit should say so, in the same words as the CLI: {}",
410 page.body
411 );
412 assert!(
413 !page.body.contains("/diff/\""),
414 "a revision with no commit must not link to an empty diff: {}",
415 page.body
416 );
417 assert!(page.body.contains("Ship it."), "reviews should render");
418 }
tests/review_test.rs
Old New
@@ -53,7 +53,7 @@ fn review_event(
53 action: Action::PatchReview { 53 action: Action::PatchReview {
54 verdict, 54 verdict,
55 body: body.to_string(), 55 body: body.to_string(),
56 revision, 56 revision: Some(revision),
57 }, 57 },
58 clock: 0, 58 clock: 0,
59 } 59 }
tests/sync_test.rs
Old New
@@ -443,7 +443,7 @@ fn test_patch_review_across_repos() {
443 action: Action::PatchReview { 443 action: Action::PatchReview {
444 verdict: ReviewVerdict::Approve, 444 verdict: ReviewVerdict::Approve,
445 body: "LGTM!".to_string(), 445 body: "LGTM!".to_string(),
446 revision: 1, 446 revision: Some(1),
447 }, 447 },
448 clock: 0, 448 clock: 0,
449 }; 449 };
@@ -507,7 +507,7 @@ fn test_concurrent_review_and_revise() {
507 action: Action::PatchReview { 507 action: Action::PatchReview {
508 verdict: ReviewVerdict::RequestChanges, 508 verdict: ReviewVerdict::RequestChanges,
509 body: "Needs work".to_string(), 509 body: "Needs work".to_string(),
510 revision: 1, 510 revision: Some(1),
511 }, 511 },
512 clock: 0, 512 clock: 0,
513 }; 513 };