Radish alpha
h
heartwood
rad:z3gqcJUoA1n9HaHKufZs5FCSGazv5
Radicle Heartwood Protocol & Stack
Radicle
Git
Patches › 37aa2b6c
remote-helper: Update Patch by ID Prefix
Draft lorenz opened 7 months ago

rad patch list does not show full patch IDs by default, and overly long patch IDs are cumbersome to handle.

If a unique prefix is used, allow updating patches identified by that prefix.

E.g. when there is exactly one patch with an ID that has a prefix of “a”, then pushing to refs/heads/patches/a updates that patch.

Activity Changes +154 -53
4 files changed +154 -53 ee49e287 → 130cf109
modified crates/radicle-cli/examples/rad-patch-update.md
@@ -72,3 +72,30 @@ $ rad patch show b6a23eb08656de0ef1fcc0b5fe8820841e5cb2e5
│ ↑ Revision ea7def3 @ 4d27214 by alice (you) now    │

╰────────────────────────────────────────────────────╯

```
+
+ 
Of course it is possible to update the patch via `git push` directly:
+
+ 
```
+ 
$ git mv README.md README.markdown
+ 
$ git commit -q -m "Longer file extension"
+ 
$ git push rad HEAD:patches/b6a23eb08656d
+ 
$ rad patch show b6a23eb08656de0ef1fcc0b5fe8820841e5cb2e5
+ 
╭────────────────────────────────────────────────────╮
+ 
│ Title     Not a real change                        │
+ 
│ Patch     b6a23eb08656de0ef1fcc0b5fe8820841e5cb2e5 │
+ 
│ Author    alice (you)                              │
+ 
│ Head      344e9ca07a4bf4b68cae3676d42727e17135f889 │
+ 
│ Base      [..                                    ] │
+ 
│ Branches  feature/1                                │
+ 
│ Commits   ahead 3, behind 0                        │
+ 
│ Status    open                                     │
+ 
├────────────────────────────────────────────────────┤
+ 
│ 344e9ca Longer file extension                      │
+ 
│ 4d27214 Rename readme file                         │
+ 
│ 51b2f0f Not a real change                          │
+ 
├────────────────────────────────────────────────────┤
+ 
│ ● Revision b6a23eb @ 51b2f0f by alice (you) now    │
+ 
│ ↑ Revision ea7def3 @ 4d27214 by alice (you) now    │
+ 
│ ↑ Revision 226fa43 @ 344e9ca by alice (you) now    │
+ 
╰────────────────────────────────────────────────────╯
+ 
```
 

\ No newline at end of file
modified crates/radicle-remote-helper/src/push.rs
@@ -6,7 +6,6 @@ mod error;
use std::collections::HashMap;

use std::io::IsTerminal;

use std::process::ExitStatus;
- 
use std::str::FromStr;

use std::{assert_eq, io};



use radicle::identity::crefs::GetCanonicalRefs as _;
@@ -211,8 +210,8 @@ impl Command {
enum PushAction {

    OpenPatch,

    UpdatePatch {
- 
        dst: git::Qualified<'static>,
- 
        patch: patch::PatchId,
+ 
        patch_id: patch::PatchId,
+ 
        patch: Box<patch::Patch>,

    },

    PushRef {

        dst: git::Qualified<'static>,
@@ -220,28 +219,50 @@ enum PushAction {
}



impl PushAction {
- 
    fn new(dst: &git::RefString) -> Result<Self, error::PushAction> {
- 
        if dst == &*rad::PATCHES_REFNAME {
- 
            Ok(Self::OpenPatch)
- 
        } else {
- 
            let dst = git::Qualified::from_refstr(dst)
- 
                .ok_or_else(|| error::PushAction::InvalidRef {
- 
                    refname: dst.clone(),
- 
                })?
- 
                .to_owned();
-
- 
            if let Some(oid) = dst.strip_prefix(git::refname!("refs/heads/patches")) {
- 
                let patch = git::Oid::from_str(oid)
- 
                    .map_err(|err| error::PushAction::InvalidPatchId {
- 
                        suffix: oid.to_string(),
- 
                        source: err,
- 
                    })
- 
                    .map(patch::PatchId::from)?;
- 
                Ok(Self::UpdatePatch { dst, patch })
- 
            } else {
- 
                Ok(Self::PushRef { dst })
- 
            }
+ 
    fn new(
+ 
        dst: git::RefString,
+ 
        patches: &cob::patch::Cache<
+ 
            cob::patch::Patches<'_, storage::git::Repository>,
+ 
            cob::cache::StoreWriter,
+ 
        >,
+ 
    ) -> Result<Self, error::PushAction> {
+ 
        if dst == *rad::PATCHES_REFNAME {
+ 
            return Ok(Self::OpenPatch);
+ 
        }
+
+ 
        let dst = git::Qualified::from_refstr(&dst)
+ 
            .ok_or_else(|| error::PushAction::InvalidRef {
+ 
                refname: dst.clone(),
+ 
            })?
+ 
            .to_owned();
+
+ 
        let Some(oid) = dst.strip_prefix(git::refname!("refs/heads/patches")) else {
+ 
            return Ok(Self::PushRef { dst });
+ 
        };
+
+ 
        let candidates = {
+ 
            let result: Result<Vec<_>, _> = patches.list_by_prefix(oid.to_string())?.collect();
+ 
            result?
+ 
        };
+
+ 
        if candidates.is_empty() {
+ 
            return Err(error::PushAction::InvalidPatchId {
+ 
                suffix: oid.to_string(),
+ 
            });
+ 
        } else if candidates.len() > 1 {
+ 
            return Err(error::PushAction::AmbiguousPatchId {
+ 
                suffix: oid.to_string(),
+ 
                candidates: candidates.iter().map(|(id, _)| id.to_string()).collect(),
+ 
            });

        }
+
+ 
        let mut candidates = candidates;
+ 
        let (patch_id, patch) = candidates.remove(0);
+
+ 
        Ok(Self::UpdatePatch {
+ 
            patch_id,
+ 
            patch: Box::new(patch),
+ 
        })

    }

}
@@ -301,8 +322,9 @@ pub fn run(
        let Ok(cmd) = Command::parse(&spec, &working) else {

            return Err(Error::InvalidCommand(format!("push {spec}")));

        };
- 
        let result = match &cmd {
- 
            Command::Delete(dst) => {
+ 
        let dst = cmd.dst().to_string();
+ 
        let result = match cmd {
+ 
            Command::Delete(ref dst) => {

                // Delete refs.

                let refname = nid.to_namespace().join(dst);

                let (canonical_ref, _) = &stored.head()?;
@@ -319,7 +341,7 @@ pub fn run(
            }

            Command::Push(git::Refspec { src, dst, force }) => {

                let patches = crate::patches_mut(profile, stored)?;
- 
                let action = PushAction::new(dst)?;
+ 
                let action = PushAction::new(dst, &patches)?;



                match action {

                    PushAction::OpenPatch => patch_open(
@@ -333,11 +355,14 @@ pub fn run(
                        profile,

                        opts.clone(),

                    ),
- 
                    PushAction::UpdatePatch { dst, patch } => patch_update(
- 
                        src,
- 
                        &dst,
- 
                        *force,
+ 
                    PushAction::UpdatePatch {
+ 
                        patch_id,

                        patch,
+ 
                    } => patch_update(
+ 
                        src,
+ 
                        force,
+ 
                        patch_id,
+ 
                        *patch,

                        &nid,

                        &working,

                        stored,
@@ -357,7 +382,7 @@ pub fn run(
                        let explorer = push(

                            src,

                            &dst,
- 
                            *force,
+ 
                            force,

                            &nid,

                            &working,

                            stored,
@@ -373,9 +398,9 @@ pub fn run(
                        // canonical branch.

                        if let Some(canonical) = rules.canonical(dst.clone(), stored) {

                            let object = working
- 
                                .find_object(**src, None)
+ 
                                .find_object(*src, None)

                                .map(|obj| git::canonical::Object::new(&obj))?
- 
                                .ok_or(Error::UnknownObjectType { oid: *src })?;
+ 
                                .ok_or(Error::UnknownObjectType { oid: src })?;



                            let canonical = canonical::Canonical::new(me, object, canonical)?;

                            match canonical.quorum() {
@@ -392,11 +417,11 @@ pub fn run(
        match result {

            // Let Git tooling know that this ref has been pushed.

            Ok(resource) => {
- 
                println!("ok {}", cmd.dst());
+ 
                println!("ok {dst}");

                ok.insert(spec, resource);

            }

            // Let Git tooling know that there was an error pushing the ref.
- 
            Err(e) => println!("error {} {e}", cmd.dst()),
+ 
            Err(e) => println!("error {dst} {e}"),

        }

    }
@@ -478,7 +503,7 @@ pub fn run(


/// Open a new patch.

fn patch_open<G>(
- 
    src: &git::Oid,
+ 
    src: git::Oid,

    upstream: &Option<git::RefString>,

    nid: &NodeId,

    working: &git::raw::Repository,
@@ -494,7 +519,7 @@ fn patch_open<G>(
where

    G: crypto::signature::Signer<crypto::Signature>,

{
- 
    let head = *src;
+ 
    let head = src;

    let dst = git::refs::storage::staging::patch(nid, head);



    // Before creating the patch, we must push the associated git objects to storage.
@@ -504,7 +529,7 @@ where
    //

    // 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)?;
+ 
    push_ref(&src, &dst, false, stored.raw(), opts.verbosity)?;



    let (_, target) = stored.canonical_head()?;

    let base = if let Some(base) = opts.base {
@@ -636,10 +661,10 @@ where
/// Update an existing patch.

#[allow(clippy::too_many_arguments)]

fn patch_update<G>(
- 
    src: &git::Oid,
- 
    dst: &git::Qualified,
+ 
    commit: git::Oid,

    force: bool,

    patch_id: patch::PatchId,
+ 
    patch: patch::Patch,

    nid: &NodeId,

    working: &git::raw::Repository,

    stored: &storage::git::Repository,
@@ -653,14 +678,9 @@ fn patch_update<G>(
where

    G: crypto::signature::Signer<crypto::Signature>,

{
- 
    let commit = *src;
- 
    let dst = dst.with_namespace(nid.into());
+ 
    let dst = git::refs::patch(&patch_id).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));
- 
    };
+ 
    push_ref(&commit, &dst, force, stored.raw(), opts.verbosity)?;



    // Don't update patch if it already has a revision matching this commit.

    if patch.revisions().any(|(_, r)| r.head() == commit) {
@@ -715,7 +735,7 @@ where
}



fn push<G>(
- 
    src: &git::Oid,
+ 
    head: git::Oid,

    dst: &git::Qualified,

    force: bool,

    nid: &NodeId,
@@ -731,12 +751,11 @@ fn push<G>(
where

    G: crypto::signature::Signer<crypto::Signature>,

{
- 
    let head = *src;

    let dst = dst.with_namespace(nid.into());

    // It's ok for the destination reference to be unknown, eg. when pushing a new branch.

    let old = stored.backend.find_reference(dst.as_str()).ok();
- 
    push_ref(src, &dst, force, stored.raw(), verbosity)?;
+ 
    push_ref(&head, &dst, force, stored.raw(), verbosity)?;



    if let Some(old) = old {

        let proj = stored.project()?;
modified crates/radicle-remote-helper/src/push/error.rs
@@ -1,5 +1,6 @@
use radicle::git;

use radicle::git::canonical;
+ 
use radicle::patch;

use thiserror::Error;



#[derive(Debug, Error)]
@@ -36,11 +37,17 @@ pub struct HeadsDiverge {


#[derive(Debug, Error)]

pub enum PushAction {
+ 
    #[error(transparent)]
+ 
    Cache(#[from] patch::cache::Error),

    #[error("invalid reference {refname}, expected qualified reference starting with `refs/`")]

    InvalidRef { refname: git::RefString },

    #[error("found refs/heads/patches/{suffix} where {suffix} was an invalid Patch ID")]
- 
    InvalidPatchId {
+ 
    InvalidPatchId { suffix: String },
+ 
    #[error(
+ 
        "found refs/heads/patches/{suffix} where {suffix} was an ambiguous among {candidates:?}"
+ 
    )]
+ 
    AmbiguousPatchId {

        suffix: String,
- 
        source: git::raw::Error,
+ 
        candidates: Vec<String>,

    },

}
modified crates/radicle/src/cob/patch/cache.rs
@@ -46,6 +46,10 @@ pub trait Patches {
    /// [`Patches::drafted`], [`Patches::merged`].

    fn list_by_status(&self, status: &Status) -> Result<Self::Iter<'_>, Self::Error>;
+ 
    /// List all patches in the store with an id that starts with the
+ 
    /// given `prefix`.
+ 
    fn list_by_prefix(&self, prefix: String) -> Result<Self::Iter<'_>, Self::Error>;
+ 


    /// Get the [`PatchCounts`] of all the patches in the store.

    fn counts(&self) -> Result<PatchCounts, Self::Error>;
@@ -469,6 +473,10 @@ where
        query::list_by_status(&self.cache.db, &self.rid(), status)

    }
+ 
    fn list_by_prefix(&self, prefix: String) -> Result<Self::Iter<'_>, Self::Error> {
+ 
        query::list_by_prefix(&self.cache.db, &self.rid(), prefix)
+ 
    }
+ 


    fn counts(&self) -> Result<PatchCounts, Self::Error> {

        query::counts(&self.cache.db, &self.rid())

    }
@@ -530,6 +538,22 @@ where
            .map_err(super::Error::from)

    }
+ 
    fn list_by_prefix(&self, prefix: String) -> Result<Self::Iter<'_>, Self::Error> {
+ 
        self.store
+ 
            .all()
+ 
            .map(move |inner| NoCacheIter {
+ 
                inner: Box::new(inner.into_iter().filter_map(move |res| {
+ 
                    match res {
+ 
                        Ok((id, patch)) => (id.to_string().starts_with(&prefix))
+ 
                            .then_some((id, patch))
+ 
                            .map(Ok),
+ 
                        Err(e) => Some(Err(e.into())),
+ 
                    }
+ 
                })),
+ 
            })
+ 
            .map_err(super::Error::from)
+ 
    }
+ 


    fn counts(&self) -> Result<PatchCounts, Self::Error> {

        self.store.counts().map_err(super::Error::from)

    }
@@ -561,6 +585,10 @@ where
        query::list_by_status(&self.cache.db, &self.rid(), status)

    }
+ 
    fn list_by_prefix(&self, prefix: String) -> Result<Self::Iter<'_>, Self::Error> {
+ 
        query::list_by_prefix(&self.cache.db, &self.rid(), prefix)
+ 
    }
+ 


    fn counts(&self) -> Result<PatchCounts, Self::Error> {

        query::counts(&self.cache.db, &self.rid())

    }
@@ -650,6 +678,26 @@ mod query {
        })

    }
+ 
    pub(super) fn list_by_prefix<'a>(
+ 
        db: &'a sql::ConnectionThreadSafe,
+ 
        rid: &RepoId,
+ 
        prefix: String,
+ 
    ) -> Result<PatchesIter<'a>, Error> {
+ 
        let mut stmt = db.prepare(
+ 
            "SELECT patches.id, patch
+ 
             FROM patches
+ 
             WHERE repo = ?1
+ 
             AND id LIKE ?2 || '%'
+ 
             ORDER BY id
+ 
            ",
+ 
        )?;
+ 
        stmt.bind((1, rid))?;
+ 
        stmt.bind((2, sql::Value::String(prefix)))?;
+ 
        Ok(PatchesIter {
+ 
            inner: stmt.into_iter(),
+ 
        })
+ 
    }
+ 


    pub(super) fn list_by_status<'a>(

        db: &'a sql::ConnectionThreadSafe,

        rid: &RepoId,