Skip to content

Commit

Permalink
helper: Revert patches correctly
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
cloudhead committed Aug 23, 2024
1 parent 0dd06b9 commit c8fbcf2
Show file tree
Hide file tree
Showing 3 changed files with 178 additions and 8 deletions.
75 changes: 75 additions & 0 deletions radicle-cli/examples/rad-patch-revert-merge.md
Original file line number Diff line number Diff line change
@@ -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 │
╰────────────────────────────────────────────────────╯
```
20 changes: 20 additions & 0 deletions radicle-cli/tests/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
91 changes: 83 additions & 8 deletions radicle-remote-helper/src/push.rs
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,10 @@ fn push<G: Signer>(
nid: &NodeId,
working: &git::raw::Repository,
stored: &storage::git::Repository,
patches: patch::Cache<patch::Patches<'_, storage::git::Repository>, cob::cache::StoreWriter>,
mut patches: patch::Cache<
patch::Patches<'_, storage::git::Repository>,
cob::cache::StoreWriter,
>,
signer: &G,
) -> Result<Option<ExplorerResource>, Error> {
let head = match working.find_reference(src.as_str()) {
Expand All @@ -592,24 +595,93 @@ fn push<G: Signer>(
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<G: Signer>(
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::<Result<Vec<git::Oid>, _>>()?;
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::<Vec<_>>();

for (id, patch) in merged {
let revisions = patch
.revisions()
.map(|(id, r)| (id, r.head()))
.collect::<Vec<_>>();

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<G: Signer>(
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,
>,
Expand All @@ -622,14 +694,17 @@ fn patch_merge_all<G: Signer>(
let commits = revwalk
.map(|r| r.map(git::Oid::from))
.collect::<Result<Vec<git::Oid>, _>>()?;
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::<Vec<_>>();
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()
Expand All @@ -642,7 +717,7 @@ fn patch_merge_all<G: Signer>(
// 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;
Expand Down

0 comments on commit c8fbcf2

Please sign in to comment.