From c8fbcf2a7aeb0dc8b5c2f709532ae464fedc3677 Mon Sep 17 00:00:00 2001 From: cloudhead Date: Thu, 22 Aug 2024 16:32:24 +0200 Subject: [PATCH] helper: Revert patches correctly When pushing to the default branch and updating the canonical head, check to see if any merged patches are reverted due to this change. If so, make sure the COB cache reflects this. --- .../examples/rad-patch-revert-merge.md | 75 +++++++++++++++ radicle-cli/tests/commands.rs | 20 ++++ radicle-remote-helper/src/push.rs | 91 +++++++++++++++++-- 3 files changed, 178 insertions(+), 8 deletions(-) create mode 100644 radicle-cli/examples/rad-patch-revert-merge.md diff --git a/radicle-cli/examples/rad-patch-revert-merge.md b/radicle-cli/examples/rad-patch-revert-merge.md new file mode 100644 index 00000000..efb33561 --- /dev/null +++ b/radicle-cli/examples/rad-patch-revert-merge.md @@ -0,0 +1,75 @@ +Let's create a patch, merge it and then revert it. + +``` (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 696ec5508494692899337afe6713fe1796d0315c opened +To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi + * [new reference] HEAD -> refs/patches +$ git checkout master +Switched to branch 'master' +$ git merge feature/1 +$ git push rad master +✓ Patch 696ec5508494692899337afe6713fe1796d0315c merged +✓ Canonical head updated to 20aa5dde6210796c3a2f04079b42316a31d02689 +To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi + f2de534..20aa5dd master -> master +``` + +First we see the patch as merged. + +``` +$ rad patch show 696ec5508494692899337afe6713fe1796d0315c +╭────────────────────────────────────────────────────────────────╮ +│ Title First change │ +│ Patch 696ec5508494692899337afe6713fe1796d0315c │ +│ Author alice (you) │ +│ Head 20aa5dde6210796c3a2f04079b42316a31d02689 │ +│ Branches feature/1, master │ +│ Commits up to date │ +│ Status merged │ +├────────────────────────────────────────────────────────────────┤ +│ 20aa5dd First change │ +├────────────────────────────────────────────────────────────────┤ +│ ● opened by alice (you) (20aa5dd) now │ +│ └─ ✓ merged by alice (you) at revision 696ec55 (20aa5dd) now │ +╰────────────────────────────────────────────────────────────────╯ +``` + +Now let's revert the patch by pushing a new `master` that doesn't include +the commit. + +``` +$ git reset --hard HEAD^ +HEAD is now at f2de534 Second commit +``` + +When pushing, notice that we're told our patch is reverted. + +``` (stderr) RAD_SOCKET=/dev/null +$ git push rad master --force +! Patch 696ec5508494692899337afe6713fe1796d0315c reverted at revision 696ec55 +✓ Canonical head updated to f2de534b5e81d7c6e2dcaf58c3dd91573c0a0354 +To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi + + 20aa5dd...f2de534 master -> master (forced update) +``` + +The patch shows up as open again. + +``` +$ rad patch show 696ec5508494692899337afe6713fe1796d0315c +╭────────────────────────────────────────────────────╮ +│ Title First change │ +│ Patch 696ec5508494692899337afe6713fe1796d0315c │ +│ Author alice (you) │ +│ Head 20aa5dde6210796c3a2f04079b42316a31d02689 │ +│ Branches feature/1 │ +│ Commits ahead 1, behind 0 │ +│ Status open │ +├────────────────────────────────────────────────────┤ +│ 20aa5dd First change │ +├────────────────────────────────────────────────────┤ +│ ● opened by alice (you) (20aa5dd) now │ +╰────────────────────────────────────────────────────╯ +``` diff --git a/radicle-cli/tests/commands.rs b/radicle-cli/tests/commands.rs index 71026e10..864106c5 100644 --- a/radicle-cli/tests/commands.rs +++ b/radicle-cli/tests/commands.rs @@ -1156,6 +1156,26 @@ fn rad_patch_merge_draft() { .unwrap(); } +#[test] +fn rad_patch_revert_merge() { + let mut environment = Environment::new(); + let profile = environment.profile(config::profile("alice")); + let working = tempfile::tempdir().unwrap(); + let home = &profile.home; + + // Setup a test repository. + fixtures::repository(working.path()); + + test("examples/rad-init.md", working.path(), Some(home), []).unwrap(); + test( + "examples/rad-patch-revert-merge.md", + working.path(), + Some(home), + [], + ) + .unwrap(); +} + #[test] #[cfg(not(target_os = "macos"))] fn rad_review_by_hunk() { diff --git a/radicle-remote-helper/src/push.rs b/radicle-remote-helper/src/push.rs index d2bb13f4..d479ce33 100644 --- a/radicle-remote-helper/src/push.rs +++ b/radicle-remote-helper/src/push.rs @@ -566,7 +566,10 @@ fn push( nid: &NodeId, working: &git::raw::Repository, stored: &storage::git::Repository, - patches: patch::Cache, cob::cache::StoreWriter>, + mut patches: patch::Cache< + patch::Patches<'_, storage::git::Repository>, + cob::cache::StoreWriter, + >, signer: &G, ) -> Result, Error> { let head = match working.find_reference(src.as_str()) { @@ -592,24 +595,93 @@ fn push( 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. + // merged or reverted, and if so, update the patch COB. if &*dst.strip_namespace() == master { let old = old.peel_to_commit()?.id(); - // Only delegates should publish the merge result to the COB. + // Only delegates affect the merge state of the COB. if stored.delegates()?.contains(&nid.into()) { - patch_merge_all(old.into(), head.into(), working, patches, signer)?; + patch_revert_all( + old.into(), + head.into(), + &stored.backend, + &mut patches, + signer, + )?; + patch_merge_all(old.into(), head.into(), working, &mut patches, signer)?; } } } Ok(Some(ExplorerResource::Tree { oid: head.into() })) } +/// Revert all patches that are no longer included in the base branch. +fn patch_revert_all( + old: git::Oid, + new: git::Oid, + stored: &git::raw::Repository, + patches: &mut patch::Cache< + patch::Patches<'_, storage::git::Repository>, + cob::cache::StoreWriter, + >, + _signer: &G, +) -> Result<(), Error> { + // Find all commits reachable from the old OID but not from the new OID. + let mut revwalk = stored.revwalk()?; + revwalk.push(*old)?; + revwalk.hide(*new)?; + + // List of commits that have been dropped. + let dropped = revwalk + .map(|r| r.map(git::Oid::from)) + .collect::, _>>()?; + if dropped.is_empty() { + return Ok(()); + } + + // Get the list of merged patches. + let merged = patches + .merged()? + // Skip patches that failed to load. + .filter_map(|patch| patch.ok()) + .collect::>(); + + for (id, patch) in merged { + let revisions = patch + .revisions() + .map(|(id, r)| (id, r.head())) + .collect::>(); + + for commit in &dropped { + if let Some((revision_id, _)) = revisions.iter().find(|(_, head)| commit == head) { + // Simply refreshing the cache entry will pick up on the fact that this patch + // is no longer merged in the canonical branch. + match patches.write(&id) { + Ok(()) => { + eprintln!( + "{} Patch {} reverted at revision {}", + term::format::yellow("!"), + term::format::tertiary(&id), + term::format::dim(term::format::oid(*revision_id)), + ); + } + Err(e) => { + eprintln!("{} Error reverting patch {id}: {e}", term::ERROR_PREFIX); + } + } + break; + } + } + } + + Ok(()) +} + /// Merge all patches that have been included in the base branch. fn patch_merge_all( old: git::Oid, new: git::Oid, working: &git::raw::Repository, - mut patches: patch::Cache< + patches: &mut patch::Cache< patch::Patches<'_, storage::git::Repository>, cob::cache::StoreWriter, >, @@ -622,14 +694,17 @@ fn patch_merge_all( let commits = revwalk .map(|r| r.map(git::Oid::from)) .collect::, _>>()?; + if commits.is_empty() { + return Ok(()); + } - let all = patches + let open = patches .opened()? .chain(patches.drafted()?) // Skip patches that failed to load. .filter_map(|patch| patch.ok()) .collect::>(); - for (id, patch) in all { + for (id, patch) in open { // Later revisions are more likely to be merged, so we build the list backwards. let revisions = patch .revisions() @@ -642,7 +717,7 @@ fn patch_merge_all( // revision that is closest to the tip of the commit chain we're pushing. for commit in &commits { if let Some((revision_id, _)) = revisions.iter().find(|(_, head)| commit == head) { - let patch = patch::PatchMut::new(id, patch, &mut patches); + let patch = patch::PatchMut::new(id, patch, patches); patch_merge(patch, *revision_id, new, working, signer)?; break;