Radish alpha
h
rad:z3gqcJUoA1n9HaHKufZs5FCSGazv5
Radicle Heartwood Protocol & Stack
Radicle
Git
remote-helper: Improve Handling of Divergence
Merged lorenz opened 7 months ago

This patch improves how the remote helper handles divergence.

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 only calling push_ref after the base of the revision (and the user provided a message, just in case that errors) is known.

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.

remote-helper: Check base when matching revisions

It is impossible to preserve the head of a revision and only change the base of same revision via push.

Strenghthen the precondition for skipping updates to also consider the base commit.

1 file changed +148 -120 5cd016b5 5caa7b30
modified 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 dst = git::refs::storage::staging::patch(nid, head);
+
    let temp = TempPatchRef::new(stored, head, nid);
+
    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.
-
    // 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 {
+
    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,109 +581,92 @@ where
            &description,
            patch::MergeTarget::default(),
            base,
-
            head,
+
            *head,
            &[],
            signer,
        )
+
    }?;
+

+
    let action = if patch.is_draft() {
+
        "drafted"
+
    } else {
+
        "opened"
    };
-
    let result = match patch {
-
        Ok(patch) => {
-
            let action = if patch.is_draft() {
-
                "drafted"
-
            } else {
-
                "opened"
-
            };
-
            let patch = patch.id;
+
    let patch = patch.id;

-
            eprintln!(
-
                "{} Patch {} {action}",
-
                term::PREFIX_SUCCESS,
-
                term::format::tertiary(patch),
-
            );
+
    eprintln!(
+
        "{} Patch {} {action}",
+
        term::PREFIX_SUCCESS,
+
        term::format::tertiary(patch),
+
    );

-
            // Create long-lived patch head reference, now that we know the Patch ID.
-
            //
-
            //  refs/namespaces/<nid>/refs/heads/patches/<patch-id>
-
            //
-
            let refname = git::refs::patch(&patch).with_namespace(nid.into());
-
            let _ = stored.raw().reference(
-
                refname.as_str(),
-
                *head,
-
                true,
-
                "Create reference for patch head",
-
            )?;
+
    // Create long-lived patch head reference, now that we know the Patch ID.
+
    //
+
    //  refs/namespaces/<nid>/refs/heads/patches/<patch-id>
+
    //
+
    let refname = git::refs::patch(&patch).with_namespace(nid.into());
+
    let _ = stored.raw().reference(
+
        refname.as_str(),
+
        **head,
+
        true,
+
        "Create reference for patch head",
+
    )?;

-
            if let Some(upstream) = upstream {
-
                if let Some(local_branch) = opts.branch.to_branch_name(&patch) {
-
                    fn strip_refs_heads(qualified: git::Qualified) -> git::RefString {
-
                        let (_refs, _heads, x, xs) = qualified.non_empty_components();
-
                        std::iter::once(x).chain(xs).collect()
-
                    }
+
    if let Some(upstream) = upstream {
+
        if let Some(local_branch) = opts.branch.to_branch_name(&patch) {
+
            fn strip_refs_heads(qualified: git::Qualified) -> git::RefString {
+
                let (_refs, _heads, x, xs) = qualified.non_empty_components();
+
                std::iter::once(x).chain(xs).collect()
+
            }

-
                    working.reference(
-
                        &local_branch,
-
                        *head,
-
                        true,
-
                        "Create local branch for patch",
-
                    )?;
+
            working.reference(&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,
-
                        true,
-
                        "Create remote tracking branch for patch",
-
                    )?;
-
                    debug_assert!(remote_branch.is_remote());
+
            let remote_branch = git::refs::workdir::patch_upstream(&patch);
+
            let remote_branch = working.reference(
+
                &remote_branch,
+
                **head,
+
                true,
+
                "Create remote tracking branch for patch",
+
            )?;
+
            debug_assert!(remote_branch.is_remote());

-
                    let local_branch = strip_refs_heads(local_branch);
-
                    let upstream_branch = git::refs::patch(&patch);
-
                    git::set_upstream(working, upstream, &local_branch, &upstream_branch)?;
+
            let local_branch = strip_refs_heads(local_branch);
+
            let upstream_branch = git::refs::patch(&patch);
+
            git::set_upstream(working, upstream, &local_branch, &upstream_branch)?;

-
                    eprintln!(
-
                        "{} Branch {} created",
-
                        term::PREFIX_SUCCESS,
-
                        term::format::tertiary(&local_branch),
-
                    );
+
            eprintln!(
+
                "{} Branch {} created",
+
                term::PREFIX_SUCCESS,
+
                term::format::tertiary(&local_branch),
+
            );
+
            hint(format!(
+
                "to update, run `git push {upstream} {local_branch}`"
+
            ));
+
        }
+
        // Setup current branch so that pushing updates the patch.
+
        else if let Some(branch) =
+
            rad::setup_patch_upstream(&patch, *head, working, upstream, false)?
+
        {
+
            if let Some(name) = branch.name()? {
+
                if profile.hints() {
+
                    // Remove the remote portion of the name, i.e.
+
                    // rad/patches/deadbeef -> patches/deadbeef
+
                    let name = name.split_once('/').unwrap_or_default().1;
                    hint(format!(
-
                        "to update, run `git push {upstream} {local_branch}`"
+
                        "to update, run `git push` or `git push {upstream} -f HEAD:{name}`"
                    ));
                }
-
                // Setup current branch so that pushing updates the patch.
-
                else if let Some(branch) =
-
                    rad::setup_patch_upstream(&patch, head, working, upstream, false)?
-
                {
-
                    if let Some(name) = branch.name()? {
-
                        if profile.hints() {
-
                            // Remove the remote portion of the name, i.e.
-
                            // rad/patches/deadbeef -> patches/deadbeef
-
                            let name = name.split_once('/').unwrap_or_default().1;
-
                            hint(format!(
-
                                "to update, run `git push` or `git push {upstream} -f HEAD:{name}`"
-
                            ));
-
                        }
-
                    }
-
                }
            }
-
            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)
+
    Ok(Some(ExplorerResource::Patch { id: patch }))
}

/// 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,33 @@ 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));
    };

-
    // Don't update patch if it already has a revision matching this commit.
-
    if patch.revisions().any(|(_, r)| r.head() == commit) {
+
    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 matching revision.
+
    if patch
+
        .revisions()
+
        .any(|(_, r)| r.head() == *head && *r.base() == base)
+
    {
        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 head: git::Oid = commit;
-
    let base = if let Some(base) = opts.base {
-
        base
-
    } else {
-
        stored.merge_base(&target, &head)?
-
    };
+
    let dst = dst.with_namespace(nid.into());
+
    push_ref(head, &dst, force, stored.raw(), opts.verbosity)?;

    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 +727,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() {
-
        patch_merge(patch_mut, revision.id(), head, working, signer)?;
+
    if base == *head && patch_mut.is_open() {
+
        patch_merge(patch_mut, revision.id(), *head, working, signer)?;
    } else {
        eprintln!(
            "To compare against your previous revision {}, run:\n\n   {}\n",