remote-helper: Only update patch after evaluating base

Without quorum for the default branch, the remote helper would report
an error, suggesting that the patch was not updated, while,
inconsistently, actually updating the ref in storage without creating
a new revision.

    $ git push …
    To rad://z…5/z…z
    …
     ! [remote rejected]       612… -> patches/a77… (…)
    error: failed to push some refs to 'rad://z…5/z…z'
    …
    $ git ls-remote rad://z…5/z…z
    612…        refs/heads/patches/a77…

This is very confusing.

Fix this by using a temporary reference name that is different from the
reference name that the patch would use, implementing cleanup via
`Drop`.

While at it, also move the logic into `patch_base` and share it
between `patch_open` and `patch_update`. Computation of the canonical
head via `stored.canonical_head` is moved to a branch that is only
taken if the user did not specify a base explicitly. This allows to
update patches even when there is no quorum for the default branch.

crates/radicle-remote-helper/src/push.rs

@@ -476,9 +476,69 @@ pub fn run(
     Ok(())
 }
 
+fn patch_base(
+    head: &git::Oid,
+    opts: &Options,
+    stored: &storage::git::Repository,
+) -> Result<git::Oid, Error> {
+    Ok(if let Some(base) = opts.base {
+        base
+    } else {
+        // Computation of the canonical head is required only if the user
+        // did not specify a base explicitly. This allows the user to
+        // continue updating patches even while the canonical head cannot
+        // be computed, e.g. while they wait for their fellow delegates
+        // to converge and sync.
+        let (_, target) = stored.canonical_head()?;
+        stored.merge_base(&target, head)?
+    })
+}
+
+/// Before opening or updating patches, we want to evaluate the merge base of the
+/// patch and the default branch. In order to do that, the respective heads must
+/// be present in the same Git repository.
+///
+/// Unfortunately, we don't have an easy way to transfer the objects without
+/// creating a reference (be it in storage or working copy).
+///
+/// We choose to push a temporary reference to storage, which gets deleted on
+/// [`Drop::drop`].
+struct TempPatchRef<'a> {
+    stored: &'a storage::git::Repository,
+    reference: git::Namespaced<'a>,
+}
+
+impl<'a> TempPatchRef<'a> {
+    fn new(stored: &'a storage::git::Repository, head: &git::Oid, nid: &NodeId) -> Self {
+        let reference = git::refs::storage::staging::patch(nid, *head);
+        Self { stored, reference }
+    }
+
+    fn push(&self, src: &git::Oid, verbosity: Verbosity) -> Result<(), Error> {
+        push_ref(src, &self.reference, false, self.stored.raw(), verbosity)
+    }
+}
+
+impl<'a> Drop for TempPatchRef<'a> {
+    fn drop(&mut self) {
+        if let Err(err) = self
+            .stored
+            .raw()
+            .find_reference(&self.reference)
+            .and_then(|mut r| r.delete())
+        {
+            eprintln!(
+                "{} Failed to delete temporary reference {} in storage: {err}",
+                term::PREFIX_WARNING,
+                term::format::tertiary(&self.reference),
+            );
+        }
+    }
+}
+
 /// Open a new patch.
 fn patch_open<G>(
-    src: &git::Oid,
+    head: &git::Oid,
     upstream: &Option<git::RefString>,
     nid: &NodeId,
     working: &git::raw::Repository,
@@ -494,29 +554,16 @@ fn patch_open<G>(
 where
     G: crypto::signature::Signer<crypto::Signature>,
 {
-    let head = *src;
+    let temp = TempPatchRef::new(stored, head, nid);
-    let dst = git::refs::storage::staging::patch(nid, head);
+    temp.push(head, opts.verbosity)?;
+    let base = patch_base(head, &opts, stored)?;
 
-    // Before creating the patch, we must push the associated git objects to storage.
+    if base == *head {
-    // Unfortunately, we don't have an easy way to transfer the missing objects without
-    // creating a temporary reference on the remote. The temporary reference is deleted
-    // once the patch is open, or in case of error.
-    //
-    // In case the reference is not properly deleted, the next attempt to open a patch should
-    // not fail, since the reference will already exist with the correct OID.
-    push_ref(src, &dst, false, stored.raw(), opts.verbosity)?;
-
-    let (_, target) = stored.canonical_head()?;
-    let base = if let Some(base) = opts.base {
-        base
-    } else {
-        stored.merge_base(&target, &head)?
-    };
-    if base == head {
         return Err(Error::EmptyPatch);
     }
+
     let (title, description) =
-        term::patch::get_create_message(opts.message, &stored.backend, &base, &head)?;
+        term::patch::get_create_message(opts.message, &stored.backend, &base, head)?;
 
     let patch = if opts.draft {
         patches.draft(
@@ -524,7 +571,7 @@ where
             &description,
             patch::MergeTarget::default(),
             base,
-            head,
+            *head,
             &[],
             signer,
         )
@@ -534,13 +581,12 @@ where
             &description,
             patch::MergeTarget::default(),
             base,
-            head,
+            *head,
             &[],
             signer,
         )
-    };
+    }?;
-    let result = match patch {
+
-        Ok(patch) => {
     let action = if patch.is_draft() {
         "drafted"
     } else {
@@ -561,7 +607,7 @@ where
     let refname = git::refs::patch(&patch).with_namespace(nid.into());
     let _ = stored.raw().reference(
         refname.as_str(),
-                *head,
+        **head,
         true,
         "Create reference for patch head",
     )?;
@@ -573,17 +619,12 @@ where
                 std::iter::once(x).chain(xs).collect()
             }
 
-                    working.reference(
+            working.reference(&local_branch, **head, true, "Create local branch for patch")?;
-                        &local_branch,
-                        *head,
-                        true,
-                        "Create local branch for patch",
-                    )?;
 
             let remote_branch = git::refs::workdir::patch_upstream(&patch);
             let remote_branch = working.reference(
                 &remote_branch,
-                        *head,
+                **head,
                 true,
                 "Create remote tracking branch for patch",
             )?;
@@ -604,7 +645,7 @@ where
         }
         // Setup current branch so that pushing updates the patch.
         else if let Some(branch) =
-                    rad::setup_patch_upstream(&patch, head, working, upstream, false)?
+            rad::setup_patch_upstream(&patch, *head, working, upstream, false)?
         {
             if let Some(name) = branch.name()? {
                 if profile.hints() {
@@ -618,25 +659,14 @@ where
             }
         }
     }
+
     Ok(Some(ExplorerResource::Patch { id: patch }))
-        }
-        Err(e) => Err(e),
-    };
-
-    // Delete short-lived patch head reference.
-    stored
-        .raw()
-        .find_reference(&dst)
-        .map(|mut r| r.delete())
-        .ok();
-
-    result.map_err(Error::from)
 }
 
 /// Update an existing patch.
 #[allow(clippy::too_many_arguments)]
 fn patch_update<G>(
-    src: &git::Oid,
+    head: &git::Oid,
     dst: &git::Qualified,
     force: bool,
     patch_id: patch::PatchId,
@@ -653,35 +683,30 @@ fn patch_update<G>(
 where
     G: crypto::signature::Signer<crypto::Signature>,
 {
-    let commit = *src;
-    let dst = dst.with_namespace(nid.into());
-
-    push_ref(src, &dst, force, stored.raw(), opts.verbosity)?;
-
     let Ok(Some(patch)) = patches.get(&patch_id) else {
         return Err(Error::NotFound(patch_id));
     };
 
+    let temp = TempPatchRef::new(stored, head, nid);
+    temp.push(head, opts.verbosity)?;
+
+    let base = patch_base(head, &opts, stored)?;
+
     // Don't update patch if it already has a revision matching this commit.
-    if patch.revisions().any(|(_, r)| r.head() == commit) {
+    if patch.revisions().any(|(_, r)| r.head() == *head) {
         return Ok(None);
     }
 
     let (latest_id, latest) = patch.latest();
     let latest = latest.clone();
 
-    let message = term::patch::get_update_message(opts.message, &stored.backend, &latest, &commit)?;
+    let message = term::patch::get_update_message(opts.message, &stored.backend, &latest, head)?;
 
-    let (_, target) = stored.canonical_head()?;
+    let dst = dst.with_namespace(nid.into());
-    let head: git::Oid = commit;
+    push_ref(head, &dst, force, stored.raw(), opts.verbosity)?;
-    let base = if let Some(base) = opts.base {
-        base
-    } else {
-        stored.merge_base(&target, &head)?
-    };
 
     let mut patch_mut = patch::PatchMut::new(patch_id, patch, &mut patches);
-    let revision = patch_mut.update(message, base, head, signer)?;
+    let revision = patch_mut.update(message, base, *head, signer)?;
     let Some(revision) = patch_mut.revision(&revision).cloned() else {
         return Err(Error::RevisionNotFound(revision));
     };
@@ -699,8 +724,8 @@ where
     // 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_mut.is_open() {
+    if base == *head && patch_mut.is_open() {
-        patch_merge(patch_mut, revision.id(), head, working, signer)?;
+        patch_merge(patch_mut, revision.id(), *head, working, signer)?;
     } else {
         eprintln!(
             "To compare against your previous revision {}, run:\n\n   {}\n",