From 84aaf9efd08563df2d55ab5f2557badc0522ed23 Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Mon, 10 Jun 2024 10:32:10 +0200 Subject: [PATCH] remote-helper: relax the quorum checks The rules for checking the quorum of the remote helper, when pushing to the canonical branch, can prevent the operator from even attempting to try converge to a state of agreeance. Instead, the checking of the quorum rules are relaxed. It is still the case that if the peer is pushing a diverging commit while in a working state they are warned and the push does not go through. However, there are two cases where it is fine to force push a change: 1. When pushing a new tip, as a delegate, there is still a canonical tip and the new tip still converges with the history, i.e. it is equal to, behind, or ahead of another delegate tip. 2. If there is no quorum in the current state, for example there were multiple concurrent pushes, then the peer is free to continue to push to their default branch in attempt to converge with the other peers. The example tests ensure that these cases are covered, while also preserving the old behaviour. Signed-off-by: Fintan Halpenny X-Clacks-Overhead: GNU Terry Pratchett --- radicle-cli/examples/git/git-push-amend.md | 27 +++++ radicle-cli/examples/git/git-push-converge.md | 79 +++++++++---- radicle-cli/examples/git/git-push-diverge.md | 11 +- radicle-cli/examples/git/git-push-rollback.md | 60 ++++++++++ radicle-cli/tests/commands.rs | 95 ++++++++++++++++ radicle-remote-helper/src/lib.rs | 13 +-- radicle-remote-helper/src/push.rs | 104 ++++++++++-------- 7 files changed, 305 insertions(+), 84 deletions(-) create mode 100644 radicle-cli/examples/git/git-push-amend.md create mode 100644 radicle-cli/examples/git/git-push-rollback.md diff --git a/radicle-cli/examples/git/git-push-amend.md b/radicle-cli/examples/git/git-push-amend.md new file mode 100644 index 00000000..f656d626 --- /dev/null +++ b/radicle-cli/examples/git/git-push-amend.md @@ -0,0 +1,27 @@ +``` ~alice +$ rad id update --title "Add Bob" --delegate did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk --repo rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji -q +c036c0d89ce26aef3ad7da402157dba16b5163b4 +``` + +``` ~bob +$ rad sync --fetch +✓ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6MknSL…StBU8Vi.. +✓ Fetched repository from 1 seed(s) +``` + +``` ~alice +$ git commit -m "New changes" --allow-empty -q +$ git push rad master -o no-sync +``` + +``` ~alice +$ git commit --amend -m "Neue Änderungen" --allow-empty -q +``` + +``` ~alice (stderr) +$ git push rad master -f +✓ Canonical head updated to 9170c8795d3a78f0381a0ffafb20ea69fb0f5b6b +✓ Synced with 1 node(s) +To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi + + fb25886...9170c87 master -> master (forced update) +``` diff --git a/radicle-cli/examples/git/git-push-converge.md b/radicle-cli/examples/git/git-push-converge.md index 9ae27f7f..c99ecec8 100644 --- a/radicle-cli/examples/git/git-push-converge.md +++ b/radicle-cli/examples/git/git-push-converge.md @@ -2,11 +2,11 @@ In this scenario we check that we can easily reset our canonical head to the head of another delegate when there is divergence between the 3 delegates. First we add our new delegates, Bob & Eve, to our repo, while also setting the -`threshold` to `2`: +`threshold` to `3`: ``` ~alice -$ rad id update --title "Add Bob & Eve" --description "" --delegate did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk --delegate did:key:z6Mkux1aUQD2voWWukVb5nNUR7thrHveQG4pDQua8nVhib7Z --threshold 2 --repo rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji -q -73bd6ea3f88eb0687afd13ee13bfb31a9eb0ccd2 +$ rad id update --title "Add Bob & Eve" --delegate did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk --delegate did:key:z6Mkux1aUQD2voWWukVb5nNUR7thrHveQG4pDQua8nVhib7Z --threshold 3 --repo rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji -q +3143236b2e40338f5574ec04e935a5ab80a6868a ``` Bob and Eve will fetch the changes to ensure they hear about their delegate @@ -37,14 +37,16 @@ f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354 refs/heads/master ``` ``` ~bob -$ git commit -m "Bob's commit" --allow-empty -q +$ git add README +$ git commit -m "Bob's commit" -q $ git push rad -o no-sync $ git ls-remote rad f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354 refs/heads/master ``` ``` ~eve -$ git commit -m "Eve's commit" --allow-empty -q +$ git add README +$ git commit -m "Eve's commit" -q $ git push rad -o no-sync $ git ls-remote rad f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354 refs/heads/master @@ -74,8 +76,12 @@ remedied by the delegates agreeing upon which way to move forward. In this case, Alice resets her `master` to `bob/master`: ``` ~alice -$ git reset bob/master --hard -HEAD is now at 0801f02 Bob's commit +$ git merge bob/master +Merge made by the 'ort' strategy. + README | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) +$ git merge eve/master -s ours +Merge made by the 'ours' strategy. ``` She can then force push to update the canonical head to the new agreed upon @@ -83,29 +89,54 @@ commit: ``` ~alice (stderr) $ git push rad -f -✓ Canonical head updated to 0801f020f11a08ec7a96d1710ec24e5e005499d1 -✓ Synced with 2 node(s) +warn: no quorum was found for `refs/heads/master` +warn: it is recommended to find a commit to agree upon +✓ Synced with 1 node(s) To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi - + d09e634...0801f02 master -> master (forced update) -``` - -We can convince ourselves that the canonical branch has indeed changed by using -`git ls-remote` for each of the delegates: - -``` ~alice -$ git ls-remote rad -0801f020f11a08ec7a96d1710ec24e5e005499d1 refs/heads/master + d09e634..0f9bd80 master -> master ``` ``` ~bob -$ git ls-remote rad -0801f020f11a08ec7a96d1710ec24e5e005499d1 refs/heads/master +$ rad remote add did:key:z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi --name alice +✓ Follow policy updated for z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi (alice) +✗ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6MknSL…StBU8Vi.. error: no quorum was found +✗ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6Mkux1…nVhib7Z.. error: no quorum was found +✓ Remote alice added +✓ Remote-tracking branch alice/master created for z6MknSL…StBU8Vi +$ git reset --hard alice/master +HEAD is now at 0f9bd80 Merge remote-tracking branch 'eve/master' ``` +When we check Bob's `rad` remote, we see that the commit for `refs/heads/master` +has changed. This is actually Eve's commit as part of Alice merging above. Now +that Alice, Bob, and Eve all have this commit as part of their history it has +become the canonical `master`. + +``` ~bob (stderr) +$ git push rad +✓ Canonical head updated to 3a75f66dd0020c9a0355cc6ec21f15de989e2001 +✓ Synced with 2 node(s) +To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk + 2a37862..0f9bd80 master -> master +``` + +Once Eve also resets to the merge commits, the canonical `master` is set to this tip. + ``` ~eve -$ git ls-remote rad -0801f020f11a08ec7a96d1710ec24e5e005499d1 refs/heads/master +$ rad remote add did:key:z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi --name alice +✓ Follow policy updated for z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi (alice) +✓ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6Mkt67…v4N1tRk.. +✓ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6MknSL…StBU8Vi.. +✓ Remote alice added +✓ Remote-tracking branch alice/master created for z6MknSL…StBU8Vi +$ git reset --hard alice/master +HEAD is now at 0f9bd80 Merge remote-tracking branch 'eve/master' ``` - - +``` ~eve (stderr) +$ git push rad +✓ Canonical head updated to 0f9bd8035c04b3f73f5408e73e8454879b20800b +✓ Synced with 2 node(s) +To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6Mkux1aUQD2voWWukVb5nNUR7thrHveQG4pDQua8nVhib7Z + 3a75f66..0f9bd80 master -> master +``` diff --git a/radicle-cli/examples/git/git-push-diverge.md b/radicle-cli/examples/git/git-push-diverge.md index 09adb837..cbf8fb3d 100644 --- a/radicle-cli/examples/git/git-push-diverge.md +++ b/radicle-cli/examples/git/git-push-diverge.md @@ -66,19 +66,14 @@ To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkE f2de534..f6cff86 master -> master ``` -One thing of note is that we can't push an older commit either, by default: +One thing of note is that we can revert to an older commit as long as we are +still ahead of the other delegates. ``` ~alice $ git reset --hard HEAD^ -q ``` -``` ~alice (fail) -$ git push -f -``` - -We have to use the `allow.rollback` option: - ``` ~alice RAD_SOCKET=/dev/null (stderr) -$ git push -f -o allow.rollback +$ git push -f ✓ Canonical head updated to 319a7dc3b195368ded4b099f8c90bbb80addccd3 To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi + f6cff86...319a7dc master -> master (forced update) diff --git a/radicle-cli/examples/git/git-push-rollback.md b/radicle-cli/examples/git/git-push-rollback.md new file mode 100644 index 00000000..1994624b --- /dev/null +++ b/radicle-cli/examples/git/git-push-rollback.md @@ -0,0 +1,60 @@ +In this scenario, we will explore being able to rollback to a previous commit. + +First we add a second delegate, Bob, to our repo. We also change the threshold +to 2: + +``` ~alice +$ rad id update --title "Add Bob" --delegate did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk --repo rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji --threshold 2 -q +069e7d58faa9a7473d27f5510d676af33282796f +``` + +Bob then syncs these changes and adds a new commit: + +``` ~bob +$ rad sync --fetch +✓ Fetching rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji from z6MknSL…StBU8Vi.. +✓ Fetched repository from 1 seed(s) +$ git commit -m "Third commit" --allow-empty -q +$ git push rad +$ git branch -arv + alice@z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi/master f2de534 Second commit + rad/master 319a7dc Third commit +``` + +Alice merges these changes and pushes them, which updates the canonical head: + +``` ~alice +$ rad remote add did:key:z6Mkt67GdsW7715MEfRuP4pSZxJRJh6kj6Y48WRqVv4N1tRk --name bob --fetch --no-sync +✓ Remote bob added +✓ Remote-tracking branch bob/master created for z6Mkt67…v4N1tRk +$ git merge bob/master +Updating f2de534..319a7dc +Fast-forward +``` + +``` ~alice (stderr) +$ git push rad +✓ Canonical head updated to 319a7dc3b195368ded4b099f8c90bbb80addccd3 +✓ Synced with 1 node(s) +To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi + f2de534..319a7dc master -> master +``` + +Alice decides that she changes her mind about these changes and rolls back to +the previous commit: + +``` ~alice +$ git reset --hard f2de534 +HEAD is now at f2de534 Second commit +``` + +Since the canonical head is still decidable from this commit she is allowed to +push and the new canonical head becomes the previous commit again: + +``` ~alice (stderr) +$ git push rad -f +✓ Canonical head updated to f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354 +✓ Synced with 1 node(s) +To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi + + 319a7dc...f2de534 master -> master (forced update) +``` diff --git a/radicle-cli/tests/commands.rs b/radicle-cli/tests/commands.rs index c3fa230e..1eddcdd4 100644 --- a/radicle-cli/tests/commands.rs +++ b/radicle-cli/tests/commands.rs @@ -2443,6 +2443,8 @@ fn git_push_diverge() { #[test] fn git_push_converge() { + use std::fs; + let mut environment = Environment::new(); let alice = environment.node(Config::test(Alias::new("alice"))); let bob = environment.node(Config::test(Alias::new("bob"))); @@ -2471,6 +2473,17 @@ fn git_push_converge() { alice.has_remote_of(&acme, &bob.id); alice.has_remote_of(&acme, &eve.id); + fs::write( + working.join("bob").join("heartwood").join("README"), + "Hello\n", + ) + .unwrap(); + fs::write( + working.join("eve").join("heartwood").join("README"), + "Hello, world!\n", + ) + .unwrap(); + formula(&environment.tmp(), "examples/git/git-push-converge.md") .unwrap() .home( @@ -2492,6 +2505,88 @@ fn git_push_converge() { .unwrap(); } +#[test] +fn git_push_amend() { + let mut environment = Environment::new(); + let alice = environment.node(Config::test(Alias::new("alice"))); + let bob = environment.node(Config::test(Alias::new("bob"))); + let working = environment.tmp().join("working"); + let acme = RepoId::from_str("z42hL2jL4XNk6K8oHQaSWfMgCL7ji").unwrap(); + + fixtures::repository(working.join("alice")); + + test( + "examples/rad-init.md", + working.join("alice"), + Some(&alice.home), + [], + ) + .unwrap(); + + let alice = alice.spawn(); + let mut bob = bob.spawn(); + + bob.connect(&alice).converge([&alice]); + bob.fork(acme, working.join("bob")).unwrap(); + alice.has_remote_of(&acme, &bob.id); + + formula(&environment.tmp(), "examples/git/git-push-amend.md") + .unwrap() + .home( + "alice", + working.join("alice"), + [("RAD_HOME", alice.home.path().display())], + ) + .home( + "bob", + working.join("bob").join("heartwood"), + [("RAD_HOME", bob.home.path().display())], + ) + .run() + .unwrap(); +} + +#[test] +fn git_push_rollback() { + let mut environment = Environment::new(); + let alice = environment.node(Config::test(Alias::new("alice"))); + let bob = environment.node(Config::test(Alias::new("bob"))); + let working = environment.tmp().join("working"); + let acme = RepoId::from_str("z42hL2jL4XNk6K8oHQaSWfMgCL7ji").unwrap(); + + fixtures::repository(working.join("alice")); + + test( + "examples/rad-init.md", + working.join("alice"), + Some(&alice.home), + [], + ) + .unwrap(); + + let alice = alice.spawn(); + let mut bob = bob.spawn(); + + bob.connect(&alice).converge([&alice]); + bob.fork(acme, working.join("bob")).unwrap(); + alice.has_remote_of(&acme, &bob.id); + + formula(&environment.tmp(), "examples/git/git-push-rollback.md") + .unwrap() + .home( + "alice", + working.join("alice"), + [("RAD_HOME", alice.home.path().display())], + ) + .home( + "bob", + working.join("bob").join("heartwood"), + [("RAD_HOME", bob.home.path().display())], + ) + .run() + .unwrap(); +} + #[test] fn rad_push_and_pull_patches() { let mut environment = Environment::new(); diff --git a/radicle-remote-helper/src/lib.rs b/radicle-remote-helper/src/lib.rs index bcee1452..165a27e1 100644 --- a/radicle-remote-helper/src/lib.rs +++ b/radicle-remote-helper/src/lib.rs @@ -68,11 +68,6 @@ pub enum Error { List(#[from] list::Error), } -#[derive(Debug, Default, Clone)] -pub struct Allow { - rollback: bool, -} - #[derive(Debug, Default, Clone)] pub struct Options { /// Don't sync after push. @@ -87,8 +82,6 @@ pub struct Options { base: Option, /// Patch message. message: cli::patch::Message, - /// Operations allowed. - allow: Allow, } /// Run the radicle remote helper using the given profile. @@ -215,7 +208,6 @@ fn push_option(args: &[&str], opts: &mut Options) -> Result<(), Error> { ["sync.debug"] => opts.sync_debug = true, ["no-sync"] => opts.no_sync = true, ["patch.draft"] => opts.draft = true, - ["allow.rollback"] => opts.allow.rollback = true, _ => { let args = args.join(" "); @@ -259,3 +251,8 @@ pub(crate) fn read_line<'a>(stdin: &io::Stdin, line: &'a mut String) -> io::Resu pub(crate) fn hint(s: impl fmt::Display) { eprintln!("{}", cli::format::hint(format!("hint: {s}"))); } + +/// Write a warning to the user. +pub(crate) fn warn(s: impl fmt::Display) { + eprintln!("{}", cli::format::hint(format!("warn: {s}"))); +} diff --git a/radicle-remote-helper/src/push.rs b/radicle-remote-helper/src/push.rs index fb63b40d..d2bb13f4 100644 --- a/radicle-remote-helper/src/push.rs +++ b/radicle-remote-helper/src/push.rs @@ -13,6 +13,8 @@ use radicle::cob::patch; use radicle::cob::patch::cache::Patches as _; use radicle::crypto::Signer; use radicle::explorer::ExplorerResource; +use radicle::git::canonical; +use radicle::git::canonical::Canonical; use radicle::identity::Did; use radicle::node; use radicle::node::{Handle, NodeId}; @@ -24,7 +26,7 @@ use radicle::{git, rad}; use radicle_cli as cli; use radicle_cli::terminal as term; -use crate::{hint, read_line, Options}; +use crate::{hint, read_line, warn, Options}; #[derive(Debug, Error)] pub enum Error { @@ -191,7 +193,6 @@ pub fn run( _ => return Err(Error::InvalidCommand(line.trim().to_owned())), } } - let canonical = stored.head()?; let delegates = stored.delegates()?; // For each refspec, push a ref or delete a ref. @@ -203,7 +204,7 @@ pub fn run( Command::Delete(dst) => { // Delete refs. let refname = nid.to_namespace().join(dst); - let (canonical_ref, _) = &canonical; + let (canonical_ref, _) = &stored.head()?; if *dst == canonical_ref.to_ref_string() && delegates.contains(&Did::from(nid)) { return Err(Error::DeleteForbidden(dst.clone())); @@ -249,7 +250,10 @@ pub fn run( opts.clone(), ) } else { - let (canonical_ref, canonical_oid) = &canonical; + let identity = stored.identity()?; + let project = identity.project()?; + let canonical_ref = git::refs::branch(project.default_branch()); + let me = Did::from(nid); // If we're trying to update the canonical head, make sure // we don't diverge from the current head. This only applies @@ -257,41 +261,51 @@ pub fn run( // // Note that we *do* allow rolling back to a previous commit on the // canonical branch. - if dst == *canonical_ref - && delegates.contains(&Did::from(nid)) - && delegates.len() > 1 - { - if let Err(e) = working.find_commit(**canonical_oid) { - return if git::ext::is_not_found_err(&e) { - Err(Error::MissingCanonicalHead(*canonical_oid)) - } else { - Err(e.into()) - }; - } + if dst == canonical_ref && delegates.contains(&me) && delegates.len() > 1 { let head = working.find_reference(src.as_str())?; let head = head.peel_to_commit()?.id(); - // Rollback is allowed and head is an ancestor of the canonical head. - let rollback = opts.allow.rollback - && working.graph_descendant_of(**canonical_oid, head)?; - - if head != **canonical_oid - // Canonical head is *not* an ancestor of head. - && !working.graph_descendant_of(head, **canonical_oid)? - // Not a rollback. - && !rollback - { - if hints { - hint( - "you are attempting to push a commit that would cause \ - your upstream to diverge from the canonical head", - ); - hint( - "to integrate the remote changes, run `git pull --rebase` \ - and try again", - ); - } - return Err(Error::HeadsDiverge(head.into(), *canonical_oid)); + let mut canonical = + Canonical::default_branch(stored, &project, &identity.delegates)?; + let converges = canonical::converges( + canonical + .tips() + .filter_map(|(did, tip)| (*did != me).then_some(tip)), + head.into(), + &working, + )?; + if converges { + canonical.modify_vote(me, head.into()); } + + match canonical.quorum(identity.threshold, &working) { + Ok(canonical_oid) => { + // Canonical head is an ancestor of head. + let is_ff = head == *canonical_oid + || working.graph_descendant_of(head, *canonical_oid)?; + + if !is_ff && !converges { + if hints { + hint( + "you are attempting to push a commit that would cause \ + your upstream to diverge from the canonical head", + ); + hint( + "to integrate the remote changes, run `git pull --rebase` \ + and try again", + ); + } + return Err(Error::HeadsDiverge( + head.into(), + canonical_oid, + )); + } + } + Err(canonical::QuorumError::NoQuorum) => { + warn(format!("no quorum was found for `{canonical_ref}`")); + warn("it is recommended to find a commit to agree upon"); + } + Err(e) => return Err(e.into()), + }; } push( src, @@ -322,15 +336,17 @@ pub fn run( // Sign refs and sync if at least one ref pushed successfully. if !ok.is_empty() { let _ = stored.sign_refs(&signer)?; - let head = stored.set_head()?; - if head.is_updated() { - eprintln!( - "{} Canonical head updated to {}", - term::format::positive("✓"), - term::format::secondary(head.new), - ); - } + // N.b. if an error occurs then there may be no quorum + if let Ok(head) = stored.set_head() { + if head.is_updated() { + eprintln!( + "{} Canonical head updated to {}", + term::format::positive("✓"), + term::format::secondary(head.new), + ); + } + }; if !opts.no_sync { if profile.policies()?.is_seeding(&stored.id)? {