a73x

4084866f

Count one review vote per author per revision

a73x   2026-07-03 10:23

Commit message
Count one review vote per author per revision
src/cache.rs
Old New
@@ -15,9 +15,16 @@ fn sanitize_ref_name(ref_name: &str) -> String {
15 ref_name.replace('/', "_") 15 ref_name.replace('/', "_")
16 } 16 }
17 17
18 /// Cache format version. Bump whenever state-fold logic changes so that
19 /// entries computed with older logic are discarded even if the ref tip
20 /// hasn't moved. v2: review vote supersession per (author, revision).
21 const CACHE_FORMAT_VERSION: u32 = 2;
22
18 /// Cache entry stored on disk: the tip OID at cache time + serialized state. 23 /// Cache entry stored on disk: the tip OID at cache time + serialized state.
19 #[derive(serde::Serialize, serde::Deserialize)] 24 #[derive(serde::Serialize, serde::Deserialize)]
20 struct CacheEntry { 25 struct CacheEntry {
26 #[serde(default)]
27 version: u32,
21 tip_oid: String, 28 tip_oid: String,
22 state: serde_json::Value, 29 state: serde_json::Value,
23 } 30 }
@@ -31,7 +38,7 @@ pub fn get_cached_state<T: DeserializeOwned>(repo: &Repository, ref_name: &str)
31 let path = cache_dir(repo).join(sanitize_ref_name(ref_name)); 38 let path = cache_dir(repo).join(sanitize_ref_name(ref_name));
32 let data = fs::read_to_string(&path).ok()?; 39 let data = fs::read_to_string(&path).ok()?;
33 let entry: CacheEntry = serde_json::from_str(&data).ok()?; 40 let entry: CacheEntry = serde_json::from_str(&data).ok()?;
34 if entry.tip_oid != current_tip.to_string() { 41 if entry.version != CACHE_FORMAT_VERSION || entry.tip_oid != current_tip.to_string() {
35 return None; 42 return None;
36 } 43 }
37 serde_json::from_value(entry.state).ok() 44 serde_json::from_value(entry.state).ok()
@@ -46,6 +53,7 @@ pub fn set_cached_state<T: Serialize>(repo: &Repository, ref_name: &str, tip_oid
46 return; 53 return;
47 } 54 }
48 let entry = CacheEntry { 55 let entry = CacheEntry {
56 version: CACHE_FORMAT_VERSION,
49 tip_oid: tip_oid.to_string(), 57 tip_oid: tip_oid.to_string(),
50 state: match serde_json::to_value(state) { 58 state: match serde_json::to_value(state) {
51 Ok(v) => v, 59 Ok(v) => v,
src/event.rs
Old New
@@ -98,6 +98,13 @@ pub enum ReviewVerdict {
98 } 98 }
99 99
100 impl ReviewVerdict { 100 impl ReviewVerdict {
101 /// Whether this verdict is a vote on the patch (approve/request-changes/reject),
102 /// as opposed to a non-binding comment. A reviewer holds at most one current
103 /// vote per revision; comments accumulate freely.
104 pub fn is_vote(&self) -> bool {
105 !matches!(self, ReviewVerdict::Comment)
106 }
107
101 pub fn as_str(&self) -> &'static str { 108 pub fn as_str(&self) -> &'static str {
102 match self { 109 match self {
103 ReviewVerdict::Approve => "approve", 110 ReviewVerdict::Approve => "approve",
src/patch.rs
Old New
@@ -288,6 +288,21 @@ pub fn review(
288 288
289 let is_reject = verdict == ReviewVerdict::Reject; 289 let is_reject = verdict == ReviewVerdict::Reject;
290 let author = get_author(repo)?; 290 let author = get_author(repo)?;
291
292 // Re-submitting the same vote for the same revision is a no-op; reject it.
293 // A different verdict is allowed and supersedes the previous vote.
294 if verdict.is_vote() {
295 let duplicate = patch.reviews.iter().any(|r| {
296 r.verdict == verdict && r.author.email == author.email && r.revision == Some(rev)
297 });
298 if duplicate {
299 return Err(Error::Cmd(format!(
300 "you already reviewed revision {} with verdict '{}'; submit a different verdict to change your vote",
301 rev, verdict
302 )));
303 }
304 }
305
291 let event = Event { 306 let event = Event {
292 timestamp: chrono::Utc::now().to_rfc3339(), 307 timestamp: chrono::Utc::now().to_rfc3339(),
293 author: author.clone(), 308 author: author.clone(),
src/state.rs
Old New
@@ -507,6 +507,16 @@ impl PatchState {
507 revision, 507 revision,
508 } => { 508 } => {
509 if let Some(ref mut s) = state { 509 if let Some(ref mut s) = state {
510 // A reviewer holds one current vote per revision: a new
511 // vote supersedes their previous one. Comment-verdict
512 // reviews are not votes and accumulate.
513 if verdict.is_vote() {
514 s.reviews.retain(|r| {
515 !(r.verdict.is_vote()
516 && r.author.email == event.author.email
517 && r.revision == Some(revision))
518 });
519 }
510 s.reviews.push(Review { 520 s.reviews.push(Review {
511 author: event.author.clone(), 521 author: event.author.clone(),
512 verdict, 522 verdict,
tests/cache_test.rs
Old New
@@ -191,3 +191,29 @@ fn list_issues_uses_cache() {
191 titles2.sort(); 191 titles2.sort();
192 assert_eq!(titles1, titles2); 192 assert_eq!(titles1, titles2);
193 } 193 }
194
195 #[test]
196 fn cache_entry_from_older_format_is_a_miss() {
197 let dir = TempDir::new().unwrap();
198 let repo = init_repo(dir.path(), &alice());
199 let (ref_name, id) = open_issue(&repo, &alice(), "Version test");
200
201 // Populate cache
202 IssueState::from_ref(&repo, &ref_name, &id).unwrap();
203 assert!(cache::get_cached_state::<IssueState>(&repo, &ref_name).is_some());
204
205 // Rewrite the cache file without a format version, simulating an entry
206 // written before fold-logic changes (e.g. review supersession).
207 let path = repo
208 .path()
209 .join("collab")
210 .join("cache")
211 .join(ref_name.replace('/', "_"));
212 let data = std::fs::read_to_string(&path).unwrap();
213 let mut entry: serde_json::Value = serde_json::from_str(&data).unwrap();
214 entry.as_object_mut().unwrap().remove("version");
215 std::fs::write(&path, serde_json::to_string(&entry).unwrap()).unwrap();
216
217 // Stale-format entries must not be served
218 assert!(cache::get_cached_state::<IssueState>(&repo, &ref_name).is_none());
219 }
tests/collab_test.rs
Old New
@@ -9,7 +9,8 @@ use git_collab::signing::{self, DetachedSignature, VerifyStatus};
9 use git_collab::state::{self, IssueState, IssueStatus, PatchState, PatchStatus}; 9 use git_collab::state::{self, IssueState, IssueStatus, PatchState, PatchStatus};
10 10
11 use common::{ 11 use common::{
12 add_comment, add_review, alice, bob, close_issue, create_patch, init_repo, now, open_issue, 12 add_comment, add_review, add_review_on, alice, bob, close_issue, create_patch, init_repo, now,
13 open_issue,
13 reopen_issue, setup_signing_key, test_signing_key, 14 reopen_issue, setup_signing_key, test_signing_key,
14 }; 15 };
15 16
@@ -345,7 +346,7 @@ fn test_patch_review_workflow() {
345 }; 346 };
346 dag::append_event(&repo, &ref_name, &event, &sk).unwrap(); 347 dag::append_event(&repo, &ref_name, &event, &sk).unwrap();
347 348
348 add_review(&repo, &ref_name, &bob(), ReviewVerdict::Approve); 349 add_review_on(&repo, &ref_name, &bob(), ReviewVerdict::Approve, 2);
349 350
350 let state = PatchState::from_ref(&repo, &ref_name, &id).unwrap(); 351 let state = PatchState::from_ref(&repo, &ref_name, &id).unwrap();
351 assert_eq!(state.reviews.len(), 2); 352 assert_eq!(state.reviews.len(), 2);
tests/common/mod.rs
Old New
@@ -281,8 +281,19 @@ pub fn create_patch(repo: &Repository, author: &Author, title: &str) -> (String,
281 (ref_name, id) 281 (ref_name, id)
282 } 282 }
283 283
284 /// Append a review event to a patch ref. 284 /// Append a review event for revision 1 to a patch ref.
285 pub fn add_review(repo: &Repository, ref_name: &str, author: &Author, verdict: ReviewVerdict) { 285 pub fn add_review(repo: &Repository, ref_name: &str, author: &Author, verdict: ReviewVerdict) {
286 add_review_on(repo, ref_name, author, verdict, 1);
287 }
288
289 /// Append a review event for a specific revision to a patch ref.
290 pub fn add_review_on(
291 repo: &Repository,
292 ref_name: &str,
293 author: &Author,
294 verdict: ReviewVerdict,
295 revision: u32,
296 ) {
286 let sk = test_signing_key(); 297 let sk = test_signing_key();
287 let event = Event { 298 let event = Event {
288 timestamp: now(), 299 timestamp: now(),
@@ -290,7 +301,7 @@ pub fn add_review(repo: &Repository, ref_name: &str, author: &Author, verdict: R
290 action: Action::PatchReview { 301 action: Action::PatchReview {
291 verdict, 302 verdict,
292 body: "review comment".to_string(), 303 body: "review comment".to_string(),
293 revision: 1, 304 revision,
294 }, 305 },
295 clock: 0, 306 clock: 0,
296 }; 307 };
tests/review_test.rs
Old New
@@ -0,0 +1,233 @@
1 mod common;
2
3 use common::TestRepo;
4 use common::{alice, bob, init_repo, test_signing_key};
5
6 use git_collab::dag;
7 use git_collab::event::{Action, Event, ReviewVerdict};
8 use git_collab::state::PatchState;
9
10 use tempfile::TempDir;
11
12 // ===========================================================================
13 // Review supersession: one current vote per (author, revision)
14 // ===========================================================================
15
16 fn setup_patch_dag(repo: &git2::Repository) -> &'static str {
17 let sk = test_signing_key();
18 let main_oid = repo.refname_to_id("refs/heads/main").unwrap();
19 let initial_commit = repo.find_commit(main_oid).unwrap();
20 repo.branch("test-branch", &initial_commit, false).unwrap();
21
22 let create = Event {
23 timestamp: "2026-01-01T00:00:00Z".to_string(),
24 author: alice(),
25 action: Action::PatchCreate {
26 title: "test patch".to_string(),
27 body: "body".to_string(),
28 base_ref: "main".to_string(),
29 branch: "test-branch".to_string(),
30 fixes: None,
31 commit: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(),
32 tree: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb".to_string(),
33 base_commit: None,
34 },
35 clock: 0,
36 };
37 let root_oid = dag::create_root_event(repo, &create, &sk).unwrap();
38 let local_ref = "refs/collab/patches/test-patch";
39 repo.reference(local_ref, root_oid, false, "test").unwrap();
40 local_ref
41 }
42
43 fn review_event(author: git_collab::event::Author, verdict: ReviewVerdict, body: &str, revision: u32, ts: &str) -> Event {
44 Event {
45 timestamp: ts.to_string(),
46 author,
47 action: Action::PatchReview {
48 verdict,
49 body: body.to_string(),
50 revision,
51 },
52 clock: 0,
53 }
54 }
55
56 #[test]
57 fn duplicate_vote_same_author_same_revision_collapses_to_latest() {
58 let dir = TempDir::new().unwrap();
59 let repo = init_repo(dir.path(), &alice());
60 let sk = test_signing_key();
61 let ref_name = setup_patch_dag(&repo);
62
63 let e1 = review_event(bob(), ReviewVerdict::Approve, "lgtm", 1, "2026-01-02T00:00:00Z");
64 let e2 = review_event(bob(), ReviewVerdict::Approve, "lgtm again", 1, "2026-01-03T00:00:00Z");
65 dag::append_event(&repo, ref_name, &e1, &sk).unwrap();
66 dag::append_event(&repo, ref_name, &e2, &sk).unwrap();
67
68 let state = PatchState::from_ref(&repo, ref_name, "test-patch").unwrap();
69 assert_eq!(state.reviews.len(), 1, "duplicate approvals should collapse");
70 assert_eq!(state.reviews[0].body, "lgtm again", "latest review wins");
71 assert_eq!(state.reviews[0].verdict, ReviewVerdict::Approve);
72 }
73
74 #[test]
75 fn changed_vote_same_author_same_revision_supersedes() {
76 let dir = TempDir::new().unwrap();
77 let repo = init_repo(dir.path(), &alice());
78 let sk = test_signing_key();
79 let ref_name = setup_patch_dag(&repo);
80
81 let e1 = review_event(bob(), ReviewVerdict::Approve, "lgtm", 1, "2026-01-02T00:00:00Z");
82 let e2 = review_event(
83 bob(),
84 ReviewVerdict::RequestChanges,
85 "wait, found a bug",
86 1,
87 "2026-01-03T00:00:00Z",
88 );
89 dag::append_event(&repo, ref_name, &e1, &sk).unwrap();
90 dag::append_event(&repo, ref_name, &e2, &sk).unwrap();
91
92 let state = PatchState::from_ref(&repo, ref_name, "test-patch").unwrap();
93 assert_eq!(state.reviews.len(), 1, "new vote should supersede old one");
94 assert_eq!(state.reviews[0].verdict, ReviewVerdict::RequestChanges);
95 assert_eq!(state.reviews[0].body, "wait, found a bug");
96 }
97
98 #[test]
99 fn votes_on_different_revisions_are_kept() {
100 let dir = TempDir::new().unwrap();
101 let repo = init_repo(dir.path(), &alice());
102 let sk = test_signing_key();
103 let ref_name = setup_patch_dag(&repo);
104
105 let e1 = review_event(bob(), ReviewVerdict::Approve, "lgtm rev 1", 1, "2026-01-02T00:00:00Z");
106 let e2 = review_event(bob(), ReviewVerdict::Approve, "lgtm rev 2", 2, "2026-01-03T00:00:00Z");
107 dag::append_event(&repo, ref_name, &e1, &sk).unwrap();
108 dag::append_event(&repo, ref_name, &e2, &sk).unwrap();
109
110 let state = PatchState::from_ref(&repo, ref_name, "test-patch").unwrap();
111 assert_eq!(state.reviews.len(), 2, "votes on different revisions both count");
112 }
113
114 #[test]
115 fn votes_from_different_authors_are_kept() {
116 let dir = TempDir::new().unwrap();
117 let repo = init_repo(dir.path(), &alice());
118 let sk = test_signing_key();
119 let ref_name = setup_patch_dag(&repo);
120
121 let e1 = review_event(alice(), ReviewVerdict::Approve, "lgtm", 1, "2026-01-02T00:00:00Z");
122 let e2 = review_event(bob(), ReviewVerdict::Approve, "lgtm too", 1, "2026-01-03T00:00:00Z");
123 dag::append_event(&repo, ref_name, &e1, &sk).unwrap();
124 dag::append_event(&repo, ref_name, &e2, &sk).unwrap();
125
126 let state = PatchState::from_ref(&repo, ref_name, "test-patch").unwrap();
127 assert_eq!(state.reviews.len(), 2, "different authors' votes both count");
128 }
129
130 #[test]
131 fn comment_verdict_reviews_always_append() {
132 let dir = TempDir::new().unwrap();
133 let repo = init_repo(dir.path(), &alice());
134 let sk = test_signing_key();
135 let ref_name = setup_patch_dag(&repo);
136
137 let e1 = review_event(bob(), ReviewVerdict::Comment, "first thought", 1, "2026-01-02T00:00:00Z");
138 let e2 = review_event(bob(), ReviewVerdict::Comment, "second thought", 1, "2026-01-03T00:00:00Z");
139 dag::append_event(&repo, ref_name, &e1, &sk).unwrap();
140 dag::append_event(&repo, ref_name, &e2, &sk).unwrap();
141
142 let state = PatchState::from_ref(&repo, ref_name, "test-patch").unwrap();
143 assert_eq!(state.reviews.len(), 2, "comment-verdict reviews are not votes; keep all");
144 }
145
146 #[test]
147 fn vote_does_not_supersede_comment_verdict_review() {
148 let dir = TempDir::new().unwrap();
149 let repo = init_repo(dir.path(), &alice());
150 let sk = test_signing_key();
151 let ref_name = setup_patch_dag(&repo);
152
153 let e1 = review_event(bob(), ReviewVerdict::Comment, "just a note", 1, "2026-01-02T00:00:00Z");
154 let e2 = review_event(bob(), ReviewVerdict::Approve, "lgtm", 1, "2026-01-03T00:00:00Z");
155 dag::append_event(&repo, ref_name, &e1, &sk).unwrap();
156 dag::append_event(&repo, ref_name, &e2, &sk).unwrap();
157
158 let state = PatchState::from_ref(&repo, ref_name, "test-patch").unwrap();
159 assert_eq!(state.reviews.len(), 2, "vote should not replace a comment-verdict review");
160 }
161
162 // ===========================================================================
163 // CLI guard: re-submitting the same verdict for the same revision errors
164 // ===========================================================================
165
166 #[test]
167 fn cli_duplicate_approve_same_revision_errors() {
168 let repo = TestRepo::new("Alice", "alice@example.com");
169 let id = repo.patch_create("Dup approve");
170
171 repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM"]);
172 let err = repo.run_err(&["patch", "review", &id, "-v", "approve", "-b", "LGTM again"]);
173 assert!(
174 err.contains("already"),
175 "expected duplicate-approve error, got: {}",
176 err
177 );
178
179 let out = repo.run_ok(&["patch", "show", &id, "--json"]);
180 let json: serde_json::Value = serde_json::from_str(&out).unwrap();
181 assert_eq!(json["reviews"].as_array().unwrap().len(), 1);
182 }
183
184 #[test]
185 fn cli_changing_verdict_same_revision_is_allowed() {
186 let repo = TestRepo::new("Alice", "alice@example.com");
187 let id = repo.patch_create("Change verdict");
188
189 repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM"]);
190 repo.run_ok(&[
191 "patch",
192 "review",
193 &id,
194 "-v",
195 "request-changes",
196 "-b",
197 "actually, please fix X",
198 ]);
199
200 let out = repo.run_ok(&["patch", "show", &id, "--json"]);
201 let json: serde_json::Value = serde_json::from_str(&out).unwrap();
202 let reviews = json["reviews"].as_array().unwrap();
203 assert_eq!(reviews.len(), 1, "changed vote supersedes the old one");
204 assert_eq!(reviews[0]["verdict"], "request-changes");
205 }
206
207 #[test]
208 fn cli_re_approving_new_revision_is_allowed() {
209 let repo = TestRepo::new("Alice", "alice@example.com");
210
211 repo.git(&["checkout", "-b", "feat-reapprove"]);
212 repo.commit_file("v1.txt", "v1", "initial commit");
213 let out = repo.run_ok(&["patch", "create", "-t", "Reapprove", "-B", "feat-reapprove"]);
214 let id = out
215 .trim()
216 .strip_prefix("Created patch ")
217 .unwrap()
218 .to_string();
219
220 repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM rev 1"]);
221
222 // New commit → new revision auto-detected on next review
223 repo.git(&["checkout", "feat-reapprove"]);
224 repo.commit_file("v2.txt", "v2", "second commit");
225 repo.git(&["checkout", "main"]);
226
227 repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM rev 2"]);
228
229 let out = repo.run_ok(&["patch", "show", &id, "--json"]);
230 let json: serde_json::Value = serde_json::from_str(&out).unwrap();
231 let reviews = json["reviews"].as_array().unwrap();
232 assert_eq!(reviews.len(), 2, "approvals on different revisions both count");
233 }