From 94cd7658b1c08d8819ab836154e82d772979eb92 Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Mon, 15 May 2023 12:07:51 +0200 Subject: [PATCH] cli: Merge patches on `git push` Allows patches to have their state updated to `Merged` with a `git push` to the default branch. Currently, due to the limitations of the COB store, this requires a linear search through all patches. This will eventually become a problem when projects have thousands of patches, so we should look into long-term solutions. --- radicle-cli/examples/rad-merge-via-push.md | 46 ++++++++++++ radicle-cli/tests/commands.rs | 29 ++++++++ radicle-cob/src/object/storage.rs | 4 +- radicle-cob/src/test/storage.rs | 6 +- radicle-remote-helper/src/push.rs | 84 ++++++++++++++++++++-- radicle/src/cob/patch.rs | 1 + radicle/src/storage/git/cob.rs | 7 +- 7 files changed, 165 insertions(+), 12 deletions(-) create mode 100644 radicle-cli/examples/rad-merge-via-push.md diff --git a/radicle-cli/examples/rad-merge-via-push.md b/radicle-cli/examples/rad-merge-via-push.md new file mode 100644 index 00000000..1ec7d0db --- /dev/null +++ b/radicle-cli/examples/rad-merge-via-push.md @@ -0,0 +1,46 @@ +Let's start by creating two patches. + +``` (stderr) RAD_SOCKET=/dev/null +$ git checkout -b feature/1 -q +$ git commit --allow-empty -q -m "First change" +$ git push rad HEAD:refs/patches +✓ Patch f4e9dcffb21bee746e0eee965933c7e237aa207a opened +To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi + * [new reference] HEAD -> refs/patches +``` +``` (stderr) RAD_SOCKET=/dev/null +$ git checkout -b feature/2 -q +$ git commit --allow-empty -q -m "Second change" +$ git push rad HEAD:refs/patches +✓ Patch dce2ff0b2baf6da67fae5143b828ebfab65d41e4 opened +To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi + * [new reference] HEAD -> refs/patches +``` + +Then let's merge the changes into `master`. + +``` (stderr) RAD_SOCKET=/dev/null +$ git checkout master +Switched to branch 'master' +$ git merge feature/1 +$ git merge feature/2 +``` + +When we push to `rad/master`, we automatically merge the patches: + +``` (stderr) RAD_SOCKET=/dev/null +$ git push rad master +✓ Patch dce2ff0b2baf6da67fae5143b828ebfab65d41e4 merged +✓ Patch f4e9dcffb21bee746e0eee965933c7e237aa207a merged +To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi + f2de534..e9fff34 master -> master +``` +``` +$ rad patch --merged +╭──────────────────────────────────────────────────────────────────────────────────╮ +│ ● ID Title Author Head + - Updated │ +├──────────────────────────────────────────────────────────────────────────────────┤ +│ ✔ dce2ff0 Second change z6MknSL…StBU8Vi (you) e9fff34 +0 -0 [ ... ] │ +│ ✔ f4e9dcf First change z6MknSL…StBU8Vi (you) 20aa5dd +0 -0 [ ... ] │ +╰──────────────────────────────────────────────────────────────────────────────────╯ +``` diff --git a/radicle-cli/tests/commands.rs b/radicle-cli/tests/commands.rs index babb0b93..0c923ae3 100644 --- a/radicle-cli/tests/commands.rs +++ b/radicle-cli/tests/commands.rs @@ -813,6 +813,35 @@ fn rad_remote() { .unwrap(); } +#[test] +fn rad_merge_via_push() { + logger::init(log::Level::Debug); + + let mut environment = Environment::new(); + let alice = environment.node("alice"); + let working = environment.tmp().join("working"); + + fixtures::repository(working.join("alice")); + + test( + "examples/rad-init.md", + working.join("alice"), + Some(&alice.home), + [], + ) + .unwrap(); + + let alice = alice.spawn(Config::default()); + + test( + "examples/rad-merge-via-push.md", + working.join("alice"), + Some(&alice.home), + [], + ) + .unwrap(); +} + #[test] fn git_push_and_pull() { logger::init(log::Level::Debug); diff --git a/radicle-cob/src/object/storage.rs b/radicle-cob/src/object/storage.rs index de7ab159..40b74861 100644 --- a/radicle-cob/src/object/storage.rs +++ b/radicle-cob/src/object/storage.rs @@ -1,6 +1,6 @@ // Copyright © 2021 The Radicle Link Contributors -use std::{collections::HashMap, error::Error}; +use std::{collections::BTreeMap, error::Error}; use git_ext::ref_format::RefString; use git_ext::Oid; @@ -70,7 +70,7 @@ pub trait Storage { /// Get all references to objects of a given type within a particular /// identity - fn types(&self, typename: &TypeName) -> Result, Self::TypesError>; + fn types(&self, typename: &TypeName) -> Result, Self::TypesError>; /// Update a ref to a particular collaborative object fn update( diff --git a/radicle-cob/src/test/storage.rs b/radicle-cob/src/test/storage.rs index 27ab3601..85f7d510 100644 --- a/radicle-cob/src/test/storage.rs +++ b/radicle-cob/src/test/storage.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, convert::TryFrom as _}; +use std::{collections::BTreeMap, convert::TryFrom as _}; use tempfile::TempDir; @@ -130,8 +130,8 @@ impl object::Storage for Storage { fn types( &self, typename: &crate::TypeName, - ) -> Result, Self::TypesError> { - let mut objects = HashMap::new(); + ) -> Result, Self::TypesError> { + let mut objects = BTreeMap::new(); for r in self.raw.references_glob("refs/rad/*")? { let r = r?; let name = r.name().unwrap(); diff --git a/radicle-remote-helper/src/push.rs b/radicle-remote-helper/src/push.rs index 41d0d3ed..449ff18b 100644 --- a/radicle-remote-helper/src/push.rs +++ b/radicle-remote-helper/src/push.rs @@ -3,14 +3,14 @@ use std::ffi::OsStr; use std::os::fd::{AsRawFd, FromRawFd}; use std::path::Path; use std::str::FromStr; -use std::{io, process}; +use std::{assert_eq, io, process}; -use radicle::storage::git::cob::object::ParseObjectId; use thiserror::Error; use radicle::cob::patch; use radicle::crypto::{PublicKey, Signer}; use radicle::node::{Handle, NodeId}; +use radicle::storage::git::cob::object::ParseObjectId; use radicle::storage::git::transport::local::Url; use radicle::storage::WriteRepository; use radicle::storage::{self, ReadRepository}; @@ -129,6 +129,8 @@ pub fn run( let mut line = String::new(); let mut ok = HashSet::new(); + assert_eq!(signer.public_key(), &nid); + // Read all the `push` lines. loop { let tokens = read_line(stdin, &mut line)?; @@ -168,7 +170,7 @@ pub fn run( } else if dst == &*rad::PATCHES_REFNAME { patch_open(src, &nid, &working, stored, &signer) } else { - push_ref(src, dst, *force, &nid, &working, stored.raw()) + push(src, dst, *force, &nid, &working, stored, &signer) } } }; @@ -236,7 +238,7 @@ fn patch_open( let (_, target) = stored.canonical_head()?; let base = stored.backend.merge_base(*target, commit.id())?; let result = match patches.create( - title, + &title, &description, patch::MergeTarget::default(), base, @@ -350,6 +352,80 @@ fn patch_update( Ok(()) } +fn push( + src: &git::RefStr, + dst: &git::RefStr, + force: bool, + nid: &NodeId, + working: &git::raw::Repository, + stored: &storage::git::Repository, + signer: &G, +) -> Result<(), Error> { + let head = working.find_reference(src.as_str())?; + let head = head.peel_to_commit()?.id(); + // It's ok for the destination reference to be unknown, eg. when pushing a new branch. + let old = stored + .backend + .find_reference(nid.to_namespace().join(dst).as_str()) + .ok(); + + push_ref(src, dst, force, nid, working, stored.raw())?; + + if let Some(old) = old { + let proj = stored.project()?; + let master = &*git::Qualified::from(git::lit::refs_heads(proj.default_branch())); + + // If we're pushing to the project's default branch, we want to see if any patches got + // merged, and if so, update the patch COB. + if dst == master { + let old = old.peel_to_commit()?.id(); + // Only delegates should publish the merge result to the COB. + if stored.delegates()?.contains(&nid.into()) { + patch_merge(old.into(), head.into(), working, stored, signer)?; + } + } + } + Ok(()) +} + +/// Merge a patch. +fn patch_merge( + old: git::Oid, + new: git::Oid, + working: &git::raw::Repository, + stored: &storage::git::Repository, + signer: &G, +) -> Result<(), Error> { + let mut revwalk = working.revwalk()?; + revwalk.push_range(&format!("{old}..{new}"))?; + + let commits = revwalk + .map(|r| r.map(git::Oid::from)) + .collect::, _>>()?; + + let mut patches = patch::Patches::open(stored)?; + for patch in patches.all()? { + let (id, patch, clock) = patch?; + let Some((revision_id, revision)) = patch.latest() else { + continue; + }; + + if patch.is_open() && commits.contains(&revision.head()) { + let revision_id = *revision_id; + let mut patch = patch::PatchMut::new(id, patch, clock, &mut patches); + + patch.merge(revision_id, new, signer)?; + + eprintln!( + "{} Patch {} merged", + cli::format::positive("✓"), + cli::format::tertiary(id) + ); + } + } + Ok(()) +} + /// Push a single reference to storage. fn push_ref( src: &git::RefStr, diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index 2cddf6be..6a7753a2 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -970,6 +970,7 @@ impl<'a, 'g> PatchMut<'a, 'g> { commit: git::Oid, signer: &G, ) -> Result { + // TODO: Don't allow merging the same revision twice? self.transaction("Merge revision", signer, |tx| tx.merge(revision, commit)) } diff --git a/radicle/src/storage/git/cob.rs b/radicle/src/storage/git/cob.rs index 7e7e97b6..0f7ec1cd 100644 --- a/radicle/src/storage/git/cob.rs +++ b/radicle/src/storage/git/cob.rs @@ -1,5 +1,5 @@ //! COB storage Git backend. -use std::collections::HashMap; +use std::collections::BTreeMap; use radicle_cob as cob; use radicle_cob::change; @@ -95,7 +95,8 @@ impl cob::object::Storage for Repository { fn types( &self, typename: &cob::TypeName, - ) -> Result, Self::TypesError> { + ) -> Result, Self::TypesError> { + // TODO: Use glob here. let mut references = self.backend.references()?.filter_map(|reference| { let reference = reference.ok()?; match RefStr::try_from_str(reference.name()?) { @@ -115,7 +116,7 @@ impl cob::object::Storage for Repository { } }); - references.try_fold(HashMap::new(), |mut objects, result| { + references.try_fold(BTreeMap::new(), |mut objects, result| { let (oid, reference) = result?; objects .entry(oid)