remote-helper/service: Introduce `GitService`

Prefactor to introduce `trait GitService` around the behaviour of
`fetch_pack` and `send_pack`.

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

@@ -4,8 +4,8 @@ use std::{io, process::ExitStatus};
 use thiserror::Error;
 
 use radicle::git;
-use radicle::storage::ReadRepository;
 
+use crate::service::GitService;
 use crate::{read_line, Verbosity};
 
 #[derive(Debug, Error)]
@@ -33,9 +33,10 @@ pub enum Error {
 }
 
 /// Run a git fetch command.
-pub fn run<R: ReadRepository>(
+pub(super) fn run<G: GitService>(
     mut refs: Vec<(git::Oid, git::fmt::RefString)>,
-    stored: R,
+    stored: radicle::storage::git::Repository,
+    git: &G,
     stdin: &io::Stdin,
     verbosity: Verbosity,
 ) -> Result<(), Error> {
@@ -58,7 +59,7 @@ pub fn run<R: ReadRepository>(
     }
 
     // Verify them and prepare the final refspecs.
-    let oids = refs.into_iter().map(|(oid, _)| oid);
+    let oids = refs.into_iter().map(|(oid, _)| oid).collect();
 
     // Rely on the environment variable `GIT_DIR` pointing at the repository.
     let working = None;
@@ -72,7 +73,7 @@ pub fn run<R: ReadRepository>(
     // used in the working copy, this will always result in the object
     // missing. This seems to only be an issue with `libgit2`/`git2`
     // and not `git` itself.
-    let output = git::process::fetch_pack(working, &stored, oids, verbosity.into())?;
+    let output = git.fetch_pack(working, &stored, oids, verbosity.into())?;
 
     if !output.status.success() {
         return Err(Error::FetchPackFailed {
@@ -82,8 +83,5 @@ pub fn run<R: ReadRepository>(
         });
     }
 
-    // Nb. An empty line means we're done.
-    println!();
-
     Ok(())
 }

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

@@ -19,6 +19,7 @@
 mod fetch;
 mod list;
 mod push;
+mod service;
 
 use std::path::PathBuf;
 use std::process;
@@ -241,6 +242,7 @@ pub fn run(profile: radicle::Profile) -> Result<(), Error> {
     let mut line = String::new();
     let mut opts = Options::default();
     let mut expected_refs = Vec::new();
+    let git = service::RealGitService;
 
     if let Err(e) = radicle::io::set_file_limit(4096) {
         if debug {
@@ -290,15 +292,15 @@ pub fn run(profile: radicle::Profile) -> Result<(), Error> {
                 let oid = git::Oid::from_str(oid)?;
                 let refstr = git::fmt::RefString::try_from(*refstr)?;
 
-                return Ok(fetch::run(
+                fetch::run(vec![(oid, refstr)], stored, &git, &stdin, opts.verbosity)?;
-                    vec![(oid, refstr)],
+
-                    stored,
+                // Nb. An empty line means we're done
-                    &stdin,
+                println!();
-                    opts.verbosity,
+
-                )?);
+                return Ok(());
             }
             ["push", refspec] => {
-                return Ok(push::run(
+                let output = push::run(
                     vec![refspec.to_string()],
                     remote,
                     url,
@@ -307,7 +309,15 @@ pub fn run(profile: radicle::Profile) -> Result<(), Error> {
                     &stdin,
                     opts,
                     &expected_refs,
-                )?);
+                    &git,
+                )?;
+
+                for line in output {
+                    println!("{line}");
+                }
+                println!();
+
+                return Ok(());
             }
             ["list"] => {
                 let refs = list::for_fetch(&url, &profile, &stored)?;

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

@@ -31,6 +31,7 @@ use radicle::{git, rad};
 use radicle_cli as cli;
 use radicle_cli::terminal as term;
 
+use crate::service::GitService;
 use crate::{hint, read_line, warn, Options, Verbosity};
 
 #[derive(Debug, Error)]
@@ -252,7 +253,8 @@ pub fn run(
     stdin: &io::Stdin,
     opts: Options,
     expected_refs: &[String],
-) -> Result<(), Error> {
+    git: &impl GitService,
+) -> Result<Vec<String>, Error> {
     // Don't allow push if either of these conditions is true:
     //
     // 1. Our key is not in ssh-agent, which means we won't be able to sign the refs.
@@ -268,6 +270,7 @@ pub fn run(
     let mut line = String::new();
     let mut ok = HashMap::new();
     let hints = opts.hints || profile.hints();
+    let mut output = Vec::new();
 
     assert_eq!(signer.public_key(), &nid);
 
@@ -330,6 +333,7 @@ pub fn run(
                         &signer,
                         profile,
                         opts.clone(),
+                        git,
                     ),
                     PushAction::UpdatePatch { dst, patch } => patch_update(
                         src,
@@ -343,6 +347,7 @@ pub fn run(
                         &signer,
                         opts.clone(),
                         expected_refs,
+                        git,
                     ),
                     PushAction::PushRef { dst } => {
                         let identity = stored.identity()?;
@@ -364,6 +369,7 @@ pub fn run(
                             &signer,
                             opts.verbosity,
                             expected_refs,
+                            git,
                         )?;
                         // If we're trying to update the canonical head, make sure
                         // we don't diverge from the current head. This only applies
@@ -392,11 +398,11 @@ pub fn run(
         match result {
             // Let Git tooling know that this ref has been pushed.
             Ok(resource) => {
-                println!("ok {}", cmd.dst());
+                output.push(format!("ok {}", cmd.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) => output.push(format!("error {} {e}", cmd.dst())),
         }
     }
 
@@ -470,10 +476,7 @@ pub fn run(
         }
     }
 
-    // Done.
+    Ok(output)
-    println!();
-
-    Ok(())
 }
 
 fn patch_base(
@@ -503,15 +506,25 @@ fn patch_base(
 ///
 /// We choose to push a temporary reference to storage, which gets deleted on
 /// [`Drop::drop`].
-struct TempPatchRef<'a> {
+struct TempPatchRef<'a, G> {
     stored: &'a storage::git::Repository,
     reference: git::fmt::Namespaced<'a>,
+    git: &'a G,
 }
 
-impl<'a> TempPatchRef<'a> {
+impl<'a, G: GitService> TempPatchRef<'a, G> {
-    fn new(stored: &'a storage::git::Repository, head: &git::Oid, nid: &NodeId) -> Self {
+    fn new(
+        stored: &'a storage::git::Repository,
+        head: &git::Oid,
+        nid: &NodeId,
+        git: &'a G,
+    ) -> Self {
         let reference = git::refs::storage::staging::patch(nid, *head);
-        Self { stored, reference }
+        Self {
+            stored,
+            reference,
+            git,
+        }
     }
 
     fn push(&self, src: &git::Oid, verbosity: Verbosity) -> Result<(), Error> {
@@ -522,11 +535,12 @@ impl<'a> TempPatchRef<'a> {
             self.stored.raw(),
             verbosity,
             &[],
+            self.git,
         )
     }
 }
 
-impl<'a> Drop for TempPatchRef<'a> {
+impl<'a, G> Drop for TempPatchRef<'a, G> {
     fn drop(&mut self) {
         if let Err(err) = self
             .stored
@@ -544,7 +558,7 @@ impl<'a> Drop for TempPatchRef<'a> {
 }
 
 /// Open a new patch.
-fn patch_open<G>(
+fn patch_open<G, S>(
     head: &git::Oid,
     upstream: &Option<git::fmt::RefString>,
     nid: &NodeId,
@@ -557,11 +571,13 @@ fn patch_open<G>(
     signer: &Device<G>,
     profile: &Profile,
     opts: Options,
+    git: &S,
 ) -> Result<Option<ExplorerResource>, Error>
 where
     G: crypto::signature::Signer<crypto::Signature>,
+    S: GitService,
 {
-    let temp = TempPatchRef::new(stored, head, nid);
+    let temp = TempPatchRef::new(stored, head, nid, git);
     temp.push(head, opts.verbosity)?;
     let base = patch_base(head, &opts, stored)?;
 
@@ -680,7 +696,7 @@ where
 
 /// Update an existing patch.
 #[allow(clippy::too_many_arguments)]
-fn patch_update<G>(
+fn patch_update<G, S>(
     head: &git::Oid,
     dst: &git::fmt::Qualified,
     force: bool,
@@ -695,15 +711,17 @@ fn patch_update<G>(
     signer: &Device<G>,
     opts: Options,
     expected_refs: &[String],
+    git: &S,
 ) -> Result<Option<ExplorerResource>, Error>
 where
     G: crypto::signature::Signer<crypto::Signature>,
+    S: GitService,
 {
     let Ok(Some(patch)) = patches.get(&patch_id) else {
         return Err(Error::NotFound(patch_id));
     };
 
-    let temp = TempPatchRef::new(stored, head, nid);
+    let temp = TempPatchRef::new(stored, head, nid, git);
     temp.push(head, opts.verbosity)?;
 
     let base = patch_base(head, &opts, stored)?;
@@ -730,6 +748,7 @@ where
         stored.raw(),
         opts.verbosity,
         expected_refs,
+        git,
     )?;
 
     let mut patch_mut = patch::PatchMut::new(patch_id, patch, &mut patches);
@@ -766,7 +785,7 @@ where
     Ok(Some(ExplorerResource::Patch { id: patch_id }))
 }
 
-fn push<G>(
+fn push<G, S>(
     src: &git::Oid,
     dst: &git::fmt::Qualified,
     force: bool,
@@ -780,16 +799,26 @@ fn push<G>(
     signer: &Device<G>,
     verbosity: Verbosity,
     expected_refs: &[String],
+    git: &S,
 ) -> Result<Option<ExplorerResource>, Error>
 where
     G: crypto::signature::Signer<crypto::Signature>,
+    S: GitService,
 {
     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, expected_refs)?;
+    push_ref(
+        src,
+        &dst,
+        force,
+        stored.raw(),
+        verbosity,
+        expected_refs,
+        git,
+    )?;
 
     if let Some(old) = old {
         let proj = stored.project()?;
@@ -973,6 +1002,7 @@ fn push_ref(
     stored: &git::raw::Repository,
     verbosity: Verbosity,
     expected_refs: &[String],
+    git: &impl GitService,
 ) -> Result<(), Error> {
     let path = dunce::canonicalize(stored.path())?.display().to_string();
     // Nb. The *force* indicator (`+`) is processed by Git tooling before we even reach this code.
@@ -996,7 +1026,7 @@ fn push_ref(
     // Rely on the environment variable `GIT_DIR`.
     let working = None;
 
-    let output = radicle::git::run(working, args)?;
+    let output = git.send_pack(working, &args)?;
 
     if !output.status.success() {
         return Err(Error::SendPackFailed {

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

@@ -0,0 +1,40 @@
+use std::io;
+use std::path::Path;
+use std::process;
+
+use radicle::git;
+use radicle::storage;
+
+/// Abstraction for Git subprocess calls.
+pub(super) trait GitService {
+    /// Run `git fetch-pack`.
+    fn fetch_pack(
+        &self,
+        working: Option<&Path>,
+        stored: &storage::git::Repository,
+        oids: Vec<git::Oid>,
+        verbosity: git::Verbosity,
+    ) -> io::Result<process::Output>;
+
+    /// Run `git send-pack` (via `radicle::git::run`).
+    fn send_pack(&self, working: Option<&Path>, args: &[String]) -> io::Result<process::Output>;
+}
+
+/// Production implementation using real Git subprocesses.
+pub(super) struct RealGitService;
+
+impl GitService for RealGitService {
+    fn fetch_pack(
+        &self,
+        working: Option<&Path>,
+        stored: &storage::git::Repository,
+        oids: Vec<git::Oid>,
+        verbosity: git::Verbosity,
+    ) -> io::Result<process::Output> {
+        git::process::fetch_pack(working, stored, oids, verbosity)
+    }
+
+    fn send_pack(&self, working: Option<&Path>, args: &[String]) -> io::Result<process::Output> {
+        git::run(working, args)
+    }
+}