From 2ea9a911afd97c6e78212ebca10fda0b04fe9a46 Mon Sep 17 00:00:00 2001 From: Alexis Sellier Date: Tue, 25 Jul 2023 12:19:49 +0200 Subject: [PATCH] remote-helper: Handle patch update after merge If you accidently merge updated patch code before updating the patch in storage, you end up in this weird state where you can't update the patch any longer. This fixes it. --- .../examples/rad-merge-after-update.md | 33 ++++++++ radicle-cli/tests/commands.rs | 27 ++++++ radicle-remote-helper/src/push.rs | 82 ++++++++++++------- 3 files changed, 111 insertions(+), 31 deletions(-) create mode 100644 radicle-cli/examples/rad-merge-after-update.md diff --git a/radicle-cli/examples/rad-merge-after-update.md b/radicle-cli/examples/rad-merge-after-update.md new file mode 100644 index 00000000..13f9d1e0 --- /dev/null +++ b/radicle-cli/examples/rad-merge-after-update.md @@ -0,0 +1,33 @@ +Let's start by creating a patch. + +``` (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 0ec956c94256fa101db4c32956ce195a1aa0edf2 opened +To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi + * [new reference] HEAD -> refs/patches +``` + +Then let's update the code and merge the updated code without updating the patch: + +``` (stderr) RAD_SOCKET=/dev/null +$ git commit --amend --allow-empty -q -m "Amended change" +$ git checkout master -q +$ git merge feature/1 -q +$ git push rad master +To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi + f2de534..954bcdb master -> master +``` + +As we can see, no patch is merged. Now if we go back to our patch and try to +update it, we expect it to be updated and merged: + +``` (stderr) RAD_SOCKET=/dev/null +$ git checkout feature/1 -q +$ git push -f +✓ Patch 0ec956c updated to 8175b00f4d75059976930cfcb75ef08454c87055 +✓ Patch 0ec956c94256fa101db4c32956ce195a1aa0edf2 merged +To rad://z42hL2jL4XNk6K8oHQaSWfMgCL7ji/z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi + + 20aa5dd...954bcdb feature/1 -> patches/0ec956c94256fa101db4c32956ce195a1aa0edf2 (forced update) +``` diff --git a/radicle-cli/tests/commands.rs b/radicle-cli/tests/commands.rs index 1d701b03..a8a2692f 100644 --- a/radicle-cli/tests/commands.rs +++ b/radicle-cli/tests/commands.rs @@ -966,6 +966,33 @@ fn rad_merge_via_push() { .unwrap(); } +#[test] +fn rad_merge_after_update() { + let mut environment = Environment::new(); + let alice = environment.node(Config::new(Alias::new("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(); + + test( + "examples/rad-merge-after-update.md", + working.join("alice"), + Some(&alice.home), + [], + ) + .unwrap(); +} + #[test] fn git_push_and_pull() { logger::init(log::Level::Debug); diff --git a/radicle-remote-helper/src/push.rs b/radicle-remote-helper/src/push.rs index d195b216..fca88855 100644 --- a/radicle-remote-helper/src/push.rs +++ b/radicle-remote-helper/src/push.rs @@ -386,9 +386,6 @@ fn patch_update( } else { stored.merge_base(&target, &head)? }; - if base == head { - return Err(Error::EmptyPatch); - } let revision = patch.update(message, base, head, signer)?; eprintln!( @@ -398,6 +395,16 @@ fn patch_update( cli::format::tertiary(revision) ); + // In this case, the patch was already merged via git, and pushed to storage. + // To handle this situation, we simply update the patch state to "merged". + // + // This can happen if for eg. a patch commit is amended, the patch branch is merged + // and pushed, but the patch hasn't yet been updated. On push to the patch branch, + // it'll seem like the patch is "empty", because the changes are already in the base branch. + if base == head && patch.is_open() { + patch_merge(patch, revision, head, working, stored, signer)?; + } + Ok(()) } @@ -428,15 +435,15 @@ fn push( 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)?; + patch_merge_all(old.into(), head.into(), working, stored, signer)?; } } } Ok(()) } -/// Merge a patch. -fn patch_merge( +/// 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, @@ -457,37 +464,50 @@ fn patch_merge( if patch.is_open() && commits.contains(&revision.head()) { let revision_id = *revision_id; - let mut patch = patch::PatchMut::new(id, patch, &mut patches); + let patch = patch::PatchMut::new(id, patch, &mut patches); - patch.merge(revision_id, new, signer)?; - - eprintln!( - "{} Patch {} merged", - cli::format::positive("✓"), - cli::format::tertiary(id) - ); - - // Delete patch references that were created when the patch was opened. - // Note that we don't return an error if we can't delete the refs, since it's - // not critical. - let nid = signer.public_key(); - let stored_ref = git::refs::storage::patch(nid, &patch.id); - let working_ref = git::refs::workdir::patch_upstream(&patch.id); - - stored - .raw() - .find_reference(&stored_ref) - .and_then(|mut r| r.delete()) - .ok(); - working - .find_reference(&working_ref) - .and_then(|mut r| r.delete()) - .ok(); + patch_merge(patch, revision_id, new, working, stored, signer)?; } } Ok(()) } +fn patch_merge( + mut patch: patch::PatchMut, + revision: patch::RevisionId, + commit: git::Oid, + working: &git::raw::Repository, + stored: &storage::git::Repository, + signer: &G, +) -> Result<(), Error> { + patch.merge(revision, commit, signer)?; + + eprintln!( + "{} Patch {} merged", + cli::format::positive("✓"), + cli::format::tertiary(patch.id) + ); + + // Delete patch references that were created when the patch was opened. + // Note that we don't return an error if we can't delete the refs, since it's + // not critical. + let nid = signer.public_key(); + let stored_ref = git::refs::storage::patch(nid, &patch.id); + let working_ref = git::refs::workdir::patch_upstream(&patch.id); + + stored + .raw() + .find_reference(&stored_ref) + .and_then(|mut r| r.delete()) + .ok(); + working + .find_reference(&working_ref) + .and_then(|mut r| r.delete()) + .ok(); + + Ok(()) +} + /// Push a single reference to storage. fn push_ref( src: &git::RefStr,