remote-helper/protocol: Introduce Line and Command

A new module to model the domain of protocol lines and commands
being exchanged is introduced.

This is to increase readability and to pave the way for a
future sans I/O version of the binary crate.

Co-authored-by: Lorenz Leutgeb <lorenz.leutgeb@radicle.xyz>

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

@@ -6,13 +6,13 @@ use thiserror::Error;
 use radicle::git;
 
 use crate::service::GitService;
-use crate::{read_line, Verbosity};
+use crate::Verbosity;
 
 #[derive(Debug, Error)]
 pub enum Error {
-    /// Invalid command received.
+    /// Protocol error.
-    #[error("invalid command `{0}`")]
+    #[error("protocol error: {0}")]
-    InvalidCommand(String),
+    Protocol(#[from] crate::protocol::Error),
     /// I/O error.
     #[error("i/o error: {0}")]
     Io(#[from] io::Error),
@@ -30,31 +30,33 @@ pub enum Error {
         stderr: String,
         stdout: String,
     },
+
+    /// Received an unexpected command after the first `fetch` command.
+    #[error("unexpected command after first `fetch`: {0:?}")]
+    UnexpectedCommand(crate::protocol::Command),
 }
 
 /// Run a git fetch command.
 pub(super) fn run<G: GitService>(
     mut refs: Vec<(git::Oid, git::fmt::RefString)>,
-    stored: radicle::storage::git::Repository,
+    stored: &radicle::storage::git::Repository,
     git: &G,
-    stdin: &io::Stdin,
+    command_reader: &mut crate::protocol::LineReader<impl io::Read>,
     verbosity: Verbosity,
 ) -> Result<(), Error> {
     // Read all the `fetch` lines.
-    let mut line = String::new();
+    for line in command_reader.by_ref() {
-    loop {
+        match line?? {
-        let tokens = read_line(stdin, &mut line)?;
+            crate::protocol::Line::Valid(crate::protocol::Command::Fetch { oid, refstr }) => {
-        match tokens.as_slice() {
+                let oid = git::Oid::from_str(&oid)?;
-            ["fetch", oid, refstr] => {
+                let refstr = git::fmt::RefString::try_from(refstr)?;
-                let oid = git::Oid::from_str(oid)?;
-                let refstr = git::fmt::RefString::try_from(*refstr)?;
-
                 refs.push((oid, refstr));
             }
-            // An empty line means end of input.
+            crate::protocol::Line::Blank => {
-            [] => break,
+                // An empty line means end of input.
-            // Once the first `fetch` command is received, we don't expect anything else.
+                break;
-            _ => return Err(Error::InvalidCommand(line.trim().to_owned())),
+            }
+            crate::protocol::Line::Valid(command) => return Err(Error::UnexpectedCommand(command)),
         }
     }
 
@@ -73,7 +75,7 @@ pub(super) fn run<G: GitService>(
     // 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.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 {

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

@@ -18,13 +18,15 @@
 
 mod fetch;
 mod list;
+mod protocol;
 mod push;
 mod service;
 
+use std::io::{self, BufRead, Write};
 use std::path::PathBuf;
 use std::process;
 use std::str::FromStr;
-use std::{env, fmt, io};
+use std::{env, fmt};
 
 use thiserror::Error;
 
@@ -36,6 +38,8 @@ use radicle::{cob, profile};
 use radicle::{git, storage, Profile};
 use radicle_cli::terminal as cli;
 
+use crate::protocol::{Command, Line, LineReader};
+
 pub const VERSION: Version = Version {
     name: env!("CARGO_BIN_NAME"),
     commit: env!("GIT_HEAD"),
@@ -85,9 +89,6 @@ pub enum Error {
     /// Remote repository not found (or empty).
     #[error("remote repository `{0}` not found")]
     RepositoryNotFound(PathBuf),
-    /// Invalid command received.
-    #[error("invalid command `{0}`")]
-    InvalidCommand(String),
     /// Invalid arguments received.
     #[error("invalid arguments: {0:?}")]
     InvalidArguments(Vec<String>),
@@ -121,6 +122,9 @@ pub enum Error {
     /// Invalid object ID.
     #[error("invalid oid: {0}")]
     InvalidOid(#[from] radicle::git::ParseOidError),
+    /// Protocol error.
+    #[error(transparent)]
+    Protocol(#[from] protocol::Error),
 }
 
 /// Models values for the `verbosity` option, see
@@ -239,9 +243,7 @@ pub fn run(profile: radicle::Profile) -> Result<(), Error> {
     let debug = radicle::profile::env::debug();
 
     let stdin = io::stdin();
-    let mut line = String::new();
+    let stdout = io::stdout();
-    let mut opts = Options::default();
-    let mut expected_refs = Vec::new();
     let git = service::RealGitService;
     let mut node = service::RealNodeSession::new(&profile);
 
@@ -251,98 +253,152 @@ pub fn run(profile: radicle::Profile) -> Result<(), Error> {
         }
     }
 
-    loop {
+    run_loop(
-        let tokens = read_line(&stdin, &mut line)?;
+        stdin.lock(),
+        stdout.lock(),
+        &git,
+        &mut node,
+        &stored,
+        &profile,
+        remote,
+        url,
+    )
+}
+
+#[allow(clippy::too_many_arguments)]
+fn run_loop<R: BufRead, W: Write, G: service::GitService, N: service::NodeSession>(
+    mut input: R,
+    mut output: W,
+    git: &G,
+    node: &mut N,
+    stored: &storage::git::Repository,
+    profile: &Profile,
+    remote: Option<git::fmt::RefString>,
+    url: Url,
+) -> Result<(), Error> {
+    let mut opts = Options::default();
+    let mut expected_refs = Vec::new();
+    let debug = radicle::profile::env::debug();
+
+    let mut command_reader = LineReader::new(&mut input);
+
+    while let Some(line) = command_reader.next() {
+        let line = line??;
 
         if debug {
-            eprintln!("{}: {}", VERSION.name, &tokens.join(" "));
+            eprintln!("{}: {:?}", VERSION.name, line);
         }
 
-        match tokens.as_slice() {
+        match line {
-            ["capabilities"] => {
+            Line::Valid(Command::Capabilities) => {
-                println!("option");
+                writeln!(output, "option")?;
-                println!("push"); // Implies `list` command.
+                writeln!(output, "push")?; // Implies `list` command.
-                println!("fetch");
+                writeln!(output, "fetch")?;
-                println!();
+                writeln!(output)?;
             }
-            ["option", "verbosity", verbosity] => match verbosity.parse::<Verbosity>() {
+            Line::Valid(Command::Option { key, value }) => match key.as_str() {
-                Ok(verbosity) => {
+                "verbosity" => {
-                    opts.verbosity = verbosity;
+                    if let Some(val) = value {
-                    println!("ok");
+                        match val.parse::<Verbosity>() {
+                            Ok(verbosity) => {
+                                opts.verbosity = verbosity;
+                                writeln!(output, "ok")?;
+                            }
+                            Err(err) => {
+                                writeln!(output, "error {err}")?;
+                            }
+                        }
+                    } else {
+                        writeln!(output, "error missing value for verbosity")?;
+                    }
                 }
-                Err(err) => {
+                "push-option" => {
-                    println!("error {err}");
+                    if let Some(val) = value {
+                        let args = val.split(' ').collect::<Vec<_>>();
+                        // Nb. Git documentation says that we can print `error <msg>` or `unsupported`
+                        // for options that are not supported, but this results in Git saying that
+                        // "push-option" itself is an unsupported option, which is not helpful or correct.
+                        // Hence, we just exit with an error in this case.
+                        push_option(&args, &mut opts)?;
+                        writeln!(output, "ok")?;
+                    } else {
+                        writeln!(output, "error missing value for push-option")?;
+                    }
+                }
+                "cas" => {
+                    if let Some(val) = value {
+                        expected_refs.push(val);
+                        writeln!(output, "ok")?;
+                    } else {
+                        writeln!(output, "error missing value for cas")?;
+                    }
+                }
+                "progress" => {
+                    writeln!(output, "unsupported")?;
+                }
+                _ => {
+                    writeln!(output, "unsupported")?;
                 }
             },
-            ["option", "push-option", args @ ..] => {
+            Line::Valid(Command::Fetch { oid, refstr }) => {
-                // Nb. Git documentation says that we can print `error <msg>` or `unsupported`
+                let oid = git::Oid::from_str(&oid)?;
-                // for options that are not supported, but this results in Git saying that
+                let refstr = git::fmt::RefString::try_from(refstr.as_str())?;
-                // "push-option" itself is an unsupported option, which is not helpful or correct.
-                // Hence, we just exit with an error in this case.
-                push_option(args, &mut opts)?;
-                println!("ok");
-            }
-            ["option", "cas", refstr] => {
-                expected_refs.push((*refstr).to_owned());
-                println!("ok");
-            }
-            ["option", "progress", ..] | ["option", ..] => {
-                println!("unsupported");
-            }
-            ["fetch", oid, refstr] => {
-                let oid = git::Oid::from_str(oid)?;
-                let refstr = git::fmt::RefString::try_from(*refstr)?;
 
-                fetch::run(vec![(oid, refstr)], stored, &git, &stdin, opts.verbosity)?;
+                fetch::run(
-
+                    vec![(oid, refstr)],
-                // Nb. An empty line means we're done
+                    stored,
-                println!();
+                    git,
-
+                    &mut command_reader,
-                return Ok(());
+                    opts.verbosity,
-            }
-            ["push", refspec] => {
-                let output = push::run(
-                    vec![refspec.to_string()],
-                    remote,
-                    url,
-                    &stored,
-                    &profile,
-                    &stdin,
-                    opts,
-                    &expected_refs,
-                    &git,
-                    &mut node,
                 )?;
 
-                for line in output {
+                // Nb. An empty line means we're done
-                    println!("{line}");
+                writeln!(output)?;
-                }
-                println!();
 
                 return Ok(());
             }
-            ["list"] => {
+            Line::Valid(Command::Push(refspec)) => {
-                let refs = list::for_fetch(&url, &profile, &stored)?;
+                let result = push::run(
-                for line in refs {
+                    vec![refspec],
-                    println!("{line}");
+                    remote.clone(),
+                    url.clone(),
+                    stored,
+                    profile,
+                    &mut command_reader,
+                    opts.clone(),
+                    &expected_refs,
+                    git,
+                    node,
+                )?;
+
+                for line in result {
+                    writeln!(output, "{line}")?;
                 }
-                println!();
+                writeln!(output)?;
-            }
+
-            ["list", "for-push"] => {
-                let refs = list::for_push(&profile, &stored)?;
-                for line in refs {
-                    println!("{line}");
-                }
-                println!();
-            }
-            [] => {
                 return Ok(());
             }
-            _ => {
+            Line::Valid(Command::List) => {
-                return Err(Error::InvalidCommand(line.trim().to_owned()));
+                let refs = list::for_fetch(&url, profile, stored)?;
+                for line in refs {
+                    writeln!(output, "{line}")?;
+                }
+                writeln!(output)?;
+            }
+            Line::Valid(Command::ListForPush) => {
+                let refs = list::for_push(profile, stored)?;
+                for line in refs {
+                    writeln!(output, "{line}")?;
+                }
+                writeln!(output)?;
+            }
+            Line::Blank => {
+                break;
             }
         }
     }
+
+    Ok(())
 }
 
 /// Parse a single push option. Returns `Ok` if it was successful.
@@ -394,20 +450,6 @@ fn push_option(args: &[&str], opts: &mut Options) -> Result<(), Error> {
     Ok(())
 }
 
-/// Read one line from stdin, and split it into tokens.
-pub(crate) fn read_line<'a>(stdin: &io::Stdin, line: &'a mut String) -> io::Result<Vec<&'a str>> {
-    line.clear();
-
-    let read = stdin.read_line(line)?;
-    if read == 0 {
-        return Ok(vec![]);
-    }
-    let line = line.trim();
-    let tokens = line.split(' ').filter(|t| !t.is_empty()).collect();
-
-    Ok(tokens)
-}
-
 /// Write a hint to the user.
 pub(crate) fn hint(s: impl fmt::Display) {
     eprintln!("{}", cli::format::hint(format!("hint: {s}")));

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

@@ -0,0 +1,230 @@
+use thiserror::Error;
+
+#[derive(Debug, Error)]
+pub(super) enum Error {
+    #[error("invalid command `{0}`")]
+    InvalidCommand(String),
+}
+
+#[derive(Debug, PartialEq, Eq)]
+pub(super) enum Command {
+    Capabilities,
+    List,
+    ListForPush,
+    Fetch { oid: String, refstr: String },
+    Push(String),
+    Option { key: String, value: Option<String> },
+}
+
+#[derive(Debug, PartialEq, Eq)]
+pub(super) enum Line {
+    Valid(Command),
+    Blank,
+}
+
+impl Command {
+    pub(super) fn parse_line(line: &str) -> Result<Line, Error> {
+        let line = line.trim();
+        if line.is_empty() {
+            return Ok(Line::Blank);
+        }
+
+        // Split the command verb from the rest of the line.
+        let (cmd, args) = line.split_once(' ').unwrap_or((line, ""));
+        let args = args.trim();
+
+        match cmd {
+            "capabilities" => Ok(Line::Valid(Command::Capabilities)),
+            "list" => {
+                if args == "for-push" {
+                    Ok(Line::Valid(Command::ListForPush))
+                } else if args.is_empty() {
+                    Ok(Line::Valid(Command::List))
+                } else {
+                    Err(Error::InvalidCommand(line.to_owned()))
+                }
+            }
+            "fetch" => {
+                // fetch <oid> <name>
+                // Use split_whitespace to handle multiple spaces between OID and Ref,
+                // which is permitted.
+                let mut parts = args.split_whitespace();
+                let oid = parts
+                    .next()
+                    .ok_or_else(|| Error::InvalidCommand(line.to_owned()))?;
+                let refstr = parts
+                    .next()
+                    .ok_or_else(|| Error::InvalidCommand(line.to_owned()))?;
+                Ok(Line::Valid(Command::Fetch {
+                    oid: oid.to_owned(),
+                    refstr: refstr.to_owned(),
+                }))
+            }
+            "push" => Ok(Line::Valid(Command::Push(args.to_owned()))),
+            "option" => {
+                // option <key> [value]
+                // Use split_once to preserve whitespace in the value.
+                let (key, val) = args.split_once(' ').unwrap_or((args, ""));
+                let value = if val.is_empty() {
+                    None
+                } else {
+                    Some(val.to_owned())
+                };
+                Ok(Line::Valid(Command::Option {
+                    key: key.to_owned(),
+                    value,
+                }))
+            }
+            _ => Err(Error::InvalidCommand(line.to_owned())),
+        }
+    }
+}
+
+mod io {
+    use std::io::{self, prelude::*};
+
+    use super::*;
+
+    pub(crate) struct LineReader<R: Read> {
+        inner: io::BufReader<R>,
+    }
+
+    impl<R: Read> LineReader<R> {
+        pub(crate) fn new(reader: R) -> Self {
+            Self {
+                inner: io::BufReader::new(reader),
+            }
+        }
+
+        pub(crate) fn read_line(&mut self) -> io::Result<Result<Line, Error>> {
+            let mut line = String::new();
+            if self.inner.read_line(&mut line)? == 0 {
+                // EOF reached
+                return Ok(Ok(Line::Blank));
+            }
+            Ok(Command::parse_line(&line))
+        }
+    }
+
+    impl<R: Read> Iterator for LineReader<R> {
+        type Item = io::Result<Result<Line, Error>>;
+
+        fn next(&mut self) -> Option<Self::Item> {
+            match self.read_line() {
+                Ok(line) => Some(Ok(line)),
+                Err(e) => Some(Err(e)),
+            }
+        }
+    }
+}
+
+pub(crate) use io::*;
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_capabilities() {
+        assert_eq!(
+            Command::parse_line("capabilities").unwrap(),
+            Line::Valid(Command::Capabilities)
+        );
+    }
+
+    #[test]
+    fn test_list() {
+        assert_eq!(
+            Command::parse_line("list").unwrap(),
+            Line::Valid(Command::List)
+        );
+    }
+
+    #[test]
+    fn test_list_for_push() {
+        assert_eq!(
+            Command::parse_line("list for-push").unwrap(),
+            Line::Valid(Command::ListForPush)
+        );
+    }
+
+    #[test]
+    fn test_fetch() {
+        assert_eq!(
+            Command::parse_line("fetch oid ref").unwrap(),
+            Line::Valid(Command::Fetch {
+                oid: "oid".to_owned(),
+                refstr: "ref".to_owned()
+            })
+        );
+    }
+
+    #[test]
+    fn test_fetch_whitespace() {
+        assert_eq!(
+            Command::parse_line("fetch   oid     ref").unwrap(),
+            Line::Valid(Command::Fetch {
+                oid: "oid".to_owned(),
+                refstr: "ref".to_owned()
+            })
+        );
+    }
+
+    #[test]
+    fn test_push() {
+        assert_eq!(
+            Command::parse_line("push src:dst").unwrap(),
+            Line::Valid(Command::Push("src:dst".to_owned()))
+        );
+    }
+
+    #[test]
+    fn test_push_force() {
+        assert_eq!(
+            Command::parse_line("push +src:dst").unwrap(),
+            Line::Valid(Command::Push("+src:dst".to_owned()))
+        );
+    }
+
+    #[test]
+    fn test_push_delete() {
+        assert_eq!(
+            Command::parse_line("push :dst").unwrap(),
+            Line::Valid(Command::Push(":dst".to_owned()))
+        );
+    }
+
+    #[test]
+    fn test_option() {
+        assert_eq!(
+            Command::parse_line("option verbosity 2").unwrap(),
+            Line::Valid(Command::Option {
+                key: "verbosity".to_owned(),
+                value: Some("2".to_owned())
+            })
+        );
+    }
+
+    #[test]
+    fn test_option_whitespace_preservation() {
+        assert_eq!(
+            Command::parse_line("option patch.message Fix:  whitespace").unwrap(),
+            Line::Valid(Command::Option {
+                key: "patch.message".to_owned(),
+                value: Some("Fix:  whitespace".to_owned())
+            })
+        );
+    }
+
+    #[test]
+    fn test_empty() {
+        assert_eq!(Command::parse_line("").unwrap(), Line::Blank);
+        assert_eq!(Command::parse_line("   ").unwrap(), Line::Blank);
+    }
+
+    #[test]
+    fn test_invalid() {
+        assert!(Command::parse_line("invalid command").is_err());
+        assert!(Command::parse_line("list invalid").is_err());
+    }
+}

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

@@ -31,7 +31,7 @@ use radicle_cli::terminal as term;
 
 use crate::service::GitService;
 use crate::service::NodeSession;
-use crate::{hint, read_line, warn, Options, Verbosity};
+use crate::{hint, warn, Options, Verbosity};
 
 #[derive(Debug, Error)]
 pub enum Error {
@@ -50,9 +50,9 @@ pub enum Error {
     /// Identity payload error.
     #[error("payload: {0}")]
     Payload(#[from] radicle::identity::doc::PayloadError),
-    /// Invalid command received.
+    /// Protocol error.
-    #[error("invalid command `{0}`")]
+    #[error("protocol error: {0}")]
-    InvalidCommand(String),
+    Protocol(#[from] crate::protocol::Error),
     /// I/O error.
     #[error("i/o error: {0}")]
     Io(#[from] io::Error),
@@ -128,6 +128,13 @@ pub enum Error {
         stderr: String,
         stdout: String,
     },
+
+    /// Received an unexpected command after the first `push` command.
+    #[error("unexpected command after first `push`: {0:?}")]
+    UnexpectedCommand(crate::protocol::Command),
+
+    #[error(transparent)]
+    CommandError(#[from] CommandError),
 }
 
 /// Push command.
@@ -139,7 +146,7 @@ enum Command {
 }
 
 #[derive(Debug, thiserror::Error)]
-enum CommandError {
+pub(super) enum CommandError {
     #[error("expected refspec of the form `[<src>]:<dst>`, got {rev}")]
     Empty { rev: String },
     #[error("failed to parse destination reference ({rev}): {err}")]
@@ -249,7 +256,7 @@ pub fn run(
     url: Url,
     stored: &storage::git::Repository,
     profile: &Profile,
-    stdin: &io::Stdin,
+    command_reader: &mut crate::protocol::LineReader<impl io::Read>,
     opts: Options,
     expected_refs: &[String],
     git: &impl GitService,
@@ -267,7 +274,6 @@ pub fn run(
             .ok_or(Error::KeyMismatch(ns.into()))
     })?;
     let signer = profile.signer()?;
-    let mut line = String::new();
     let mut ok = HashMap::new();
     let hints = opts.hints || profile.hints();
     let mut output = Vec::new();
@@ -275,16 +281,16 @@ pub fn run(
     assert_eq!(signer.public_key(), &nid);
 
     // Read all the `push` lines.
-    loop {
+    for line in command_reader.by_ref() {
-        let tokens = read_line(stdin, &mut line)?;
+        match line?? {
-        match tokens.as_slice() {
+            crate::protocol::Line::Blank => {
-            ["push", spec] => {
+                // An empty line means end of input.
-                specs.push(spec.to_string());
+                break;
             }
-            // An empty line means end of input.
+            crate::protocol::Line::Valid(crate::protocol::Command::Push(spec)) => {
-            [] => break,
+                specs.push(spec);
-            // Once the first `push` command is received, we don't expect anything else.
+            }
-            _ => return Err(Error::InvalidCommand(line.trim().to_owned())),
+            crate::protocol::Line::Valid(command) => return Err(Error::UnexpectedCommand(command)),
         }
     }
     let delegates = stored.delegates()?;
@@ -299,9 +305,7 @@ pub fn run(
 
     // For each refspec, push a ref or delete a ref.
     for spec in specs {
-        let Ok(cmd) = Command::parse(&spec, &working) else {
+        let cmd = Command::parse(&spec, &working)?;
-            return Err(Error::InvalidCommand(format!("push {spec}")));
-        };
         let result = match &cmd {
             Command::Delete(dst) => {
                 // Delete refs.