From 8404b5925dc3ab73f162eaf1365cf7d312513be0 Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Wed, 29 Mar 2023 15:04:33 +0100 Subject: [PATCH] radicle: force new or initial construction of LWWReg Using LWWReg::from will always set the clock value to the default, which can end up being used wrong. Instead, introduce a constructor method `initial` to indicate that it should be used on initial construction, while `new` should be used for newer values of the register. Signed-off-by: Fintan Halpenny X-Clacks-Overhead: GNU Terry Pratchett --- radicle-crdt/src/lwwreg.rs | 19 ++++++++------- radicle/src/cob/identity.rs | 6 ++--- radicle/src/cob/issue.rs | 8 +++---- radicle/src/cob/patch.rs | 48 +++++++++++++++++++++++++++++-------- 4 files changed, 55 insertions(+), 26 deletions(-) diff --git a/radicle-crdt/src/lwwreg.rs b/radicle-crdt/src/lwwreg.rs index e2027296..9c88701f 100644 --- a/radicle-crdt/src/lwwreg.rs +++ b/radicle-crdt/src/lwwreg.rs @@ -14,6 +14,16 @@ pub struct LWWReg { } impl LWWReg { + pub fn initial(value: T) -> Self + where + C: Default, + { + Self { + clock: Max::from(C::default()), + value, + } + } + pub fn new(value: T, clock: C) -> Self { Self { clock: Max::from(clock), @@ -46,15 +56,6 @@ impl LWWReg { } } -impl From for LWWReg { - fn from(value: T) -> Self { - Self { - clock: Max::from(C::default()), - value, - } - } -} - impl Default for LWWReg { fn default() -> Self { Self { diff --git a/radicle/src/cob/identity.rs b/radicle/src/cob/identity.rs index 0cfbea4e..43d201c9 100644 --- a/radicle/src/cob/identity.rs +++ b/radicle/src/cob/identity.rs @@ -167,9 +167,9 @@ impl Semilattice for Proposal { impl Default for Proposal { fn default() -> Self { Self { - title: Max::from(String::default()).into(), - description: Max::from(String::default()).into(), - state: Max::from(State::default()).into(), + title: LWWReg::initial(Max::from(String::default())), + description: LWWReg::initial(Max::from(String::default())), + state: LWWReg::initial(Max::from(State::default())), revisions: GMap::default(), timeline: GSet::default(), } diff --git a/radicle/src/cob/issue.rs b/radicle/src/cob/issue.rs index 67c4cb6f..495219c3 100644 --- a/radicle/src/cob/issue.rs +++ b/radicle/src/cob/issue.rs @@ -83,9 +83,9 @@ pub struct Issue { /// Actors assigned to this issue. assignees: LWWSet, /// Title of the issue. - title: LWWReg, clock::Lamport>, + title: LWWReg>, /// Current state of the issue. - state: LWWReg, clock::Lamport>, + state: LWWReg>, /// Associated tags. tags: LWWSet, /// Discussion around this issue. @@ -106,8 +106,8 @@ impl Default for Issue { fn default() -> Self { Self { assignees: LWWSet::default(), - title: Max::from(String::default()).into(), - state: Max::from(State::default()).into(), + title: LWWReg::initial(Max::from(String::default())), + state: LWWReg::initial(Max::from(State::default())), tags: LWWSet::default(), thread: Thread::default(), } diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index 5b218e2d..f48e04cf 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -160,10 +160,10 @@ impl Semilattice for Patch { impl Default for Patch { fn default() -> Self { Self { - title: Max::from(String::default()).into(), - description: Max::from(String::default()).into(), - state: Max::from(State::default()).into(), - target: Max::from(MergeTarget::default()).into(), + title: LWWReg::initial(Max::from(String::default())), + description: LWWReg::initial(Max::from(String::default())), + state: LWWReg::initial(Max::from(State::default())), + target: LWWReg::initial(Max::from(MergeTarget::default())), tags: LWWSet::default(), revisions: GMap::default(), timeline: GSet::default(), @@ -326,6 +326,7 @@ impl store::FromHistory for Patch { base, oid, timestamp, + op.clock, )), ); } @@ -417,10 +418,11 @@ impl Revision { base: git::Oid, oid: git::Oid, timestamp: Timestamp, + clock: Clock, ) -> Self { Self { author, - description: LWWReg::from(Max::from(description)), + description: LWWReg::new(Max::from(description), clock), base, oid, discussion: Thread::default(), @@ -1053,6 +1055,7 @@ impl<'a> Patches<'a> { #[cfg(test)] mod test { + use std::path::Path; use std::str::FromStr; use std::{array, iter}; @@ -1460,6 +1463,7 @@ mod test { let (_, signer, project) = test::setup::context(&tmp); let base = git::Oid::from_str("cb18e95ada2bb38aadd8e6cef0963ce37a87add3").unwrap(); let oid = git::Oid::from_str("518d5069f94c03427f694bb494ac1cd7d1339380").unwrap(); + let blob = git::Oid::from_str("518d5069f94c03427f694bb494ac1cd7d133999").unwrap(); let mut patches = Patches::open(&project).unwrap(); let mut patch = patches .create( @@ -1476,17 +1480,33 @@ mod test { let (rid, _) = patch.latest().unwrap(); let rid = *rid; + let inline = vec![CodeComment { + location: CodeLocation { + blob, + path: Path::new("file.rs").to_path_buf(), + commit: oid, + lines: 1..3, + }, + comment: "Nice!".to_owned(), + timestamp: Timestamp::new(0), + }]; patch .review( rid, Some(Verdict::Accept), Some("LGTM".to_owned()), - vec![], + inline.clone(), &signer, ) .unwrap(); patch - .review(rid, Some(Verdict::Reject), None, vec![], &signer) + .review( + rid, + Some(Verdict::Reject), + Some("LGTM".to_owned()), + vec![], + &signer, + ) .unwrap(); // Overwrite the verdict. let id = patch.id; @@ -1496,15 +1516,23 @@ mod test { let review = revision.reviews.get(signer.public_key()).unwrap(); assert_eq!(review.verdict(), Some(Verdict::Reject)); - assert_eq!(review.comment(), None); + assert_eq!(review.comment(), Some("LGTM")); + assert_eq!(review.inline().cloned().collect::>(), inline); patch - .review(rid, None, Some("Whoops!".to_owned()), vec![], &signer) + .review( + rid, + Some(Verdict::Reject), + Some("Whoops!".to_owned()), + vec![], + &signer, + ) .unwrap(); // Overwrite the comment. let (_, revision) = patch.latest().unwrap(); let review = revision.reviews.get(signer.public_key()).unwrap(); - assert_eq!(review.verdict(), None); + assert_eq!(review.verdict(), Some(Verdict::Reject)); assert_eq!(review.comment(), Some("Whoops!")); + assert_eq!(review.inline().cloned().collect::>(), inline); } #[test]