josh-proxy: use clap derive (#1292)

flokli · web-flow · commit 465dd8f9118f · 2023-11-01T23:00:19.000+01:00
This simplifies the argument parsing a lot - we can almost parse the needed structure just with that, including all the manual default handling. The only thing that does change is the handling of upstreams - rather than populating two fields in the Arg struct, it now contains a `Vec<Remote>`. We can use clap to ensure there's at least one element (same for local), but comparison of individual args and further validation (max 2, not two of the same type) is now left to `josh-proxy.rs`. For this, a `make_upstream` is introduced, turning that list of enums with URLs into a JoshProxyUpstream. Error handling in run_proxy isn't awfully verbose, just exit nonzero, so I opted to log *that* specific error with an eprintln!, but happy to also add it for all errors (in main()). This is in preparation for #1288.
diff --git a/josh-proxy/src/bin/josh-proxy.rs b/josh-proxy/src/bin/josh-proxy.rs
@@ -1,7 +1,10 @@
 #![deny(warnings)]
 #[macro_use]
 extern crate lazy_static;
+extern crate clap;
 
+use clap::Parser;
+use josh_proxy::cli;
 use josh_proxy::{run_git_with_auth, FetchError, MetaConfig, RemoteAuth, RepoConfig, RepoUpdate};
 use opentelemetry::global;
 use opentelemetry::sdk::propagation::TraceContextPropagator;
@@ -37,7 +40,7 @@ fn version_str() -> String {
 }
 
 lazy_static! {
-    static ref ARGS: josh_proxy::cli::Args = josh_proxy::cli::parse_args_or_exit(1);
+    static ref ARGS: josh_proxy::cli::Args = josh_proxy::cli::Args::parse();
 }
 
 josh::regex_parsed!(
@@ -1424,20 +1427,41 @@ fn trace_http_response_code(trace_span: Span, http_status: StatusCode) {
     };
 }
 
+/// Turn a list of [cli::Remote] into a [JoshProxyUpstream] struct.
+fn make_upstream(remotes: &Vec<cli::Remote>) -> josh::JoshResult<JoshProxyUpstream> {
+    if remotes.is_empty() {
+        unreachable!() // already checked in the parser
+    } else if remotes.len() == 1 {
+        Ok(match &remotes[0] {
+            cli::Remote::Http(url) => JoshProxyUpstream::Http(url.to_string()),
+            cli::Remote::Ssh(url) => JoshProxyUpstream::Ssh(url.to_string()),
+        })
+    } else if remotes.len() == 2 {
+        Ok(match (&remotes[0], &remotes[1]) {
+            (cli::Remote::Http(_), cli::Remote::Http(_))
+            | (cli::Remote::Ssh(_), cli::Remote::Ssh(_)) => {
+                return Err(josh_error("two cli::remotes of the same type passed"))
+            }
+            (cli::Remote::Http(http_url), cli::Remote::Ssh(ssh_url))
+            | (cli::Remote::Ssh(ssh_url), cli::Remote::Http(http_url)) => JoshProxyUpstream::Both {
+                http: http_url.to_string(),
+                ssh: ssh_url.to_string(),
+            },
+        })
+    } else {
+        Err(josh_error("too many remotes"))
+    }
+}
+
 #[tokio::main]
 async fn run_proxy() -> josh::JoshResult<i32> {
     let addr = format!("[::]:{}", ARGS.port).parse()?;
-    let upstream = match (&ARGS.remote.http, &ARGS.remote.ssh) {
-        (Some(http), None) => JoshProxyUpstream::Http(http.clone()),
-        (None, Some(ssh)) => JoshProxyUpstream::Ssh(ssh.clone()),
-        (Some(http), Some(ssh)) => JoshProxyUpstream::Both {
-            http: http.clone(),
-            ssh: ssh.clone(),
-        },
-        (None, None) => return Err(josh_error("missing remote host url")),
-    };
+    let upstream = make_upstream(&ARGS.remote).map_err(|e| {
+        eprintln!("Upstream parsing error: {}", &e);
+        e
+    })?;
 
-    let local = std::path::PathBuf::from(&ARGS.local);
+    let local = std::path::PathBuf::from(&ARGS.local.as_ref().unwrap());
     let local = if local.is_absolute() {
         local
     } else {
diff --git a/josh-proxy/src/cli.rs b/josh-proxy/src/cli.rs
@@ -1,175 +1,52 @@
-use josh::{josh_error, JoshResult};
+#[derive(Clone, Debug)]
+pub enum Remote {
+    Http(String),
+    Ssh(String),
+}
 
-pub struct Remote {
-    pub http: Option<String>,
-    pub ssh: Option<String>,
+fn parse_remote(s: &str) -> Result<Remote, &'static str> {
+    match s {
+        s if s.starts_with("http://") || s.starts_with("https://") => {
+            Ok(Remote::Http(s.to_string()))
+        }
+        s if s.starts_with("ssh://") => Ok(Remote::Ssh(s.to_string())),
+        _ => return Err("unsupported scheme"),
+    }
 }
 
+#[derive(clap::Parser, Debug)]
+#[command(name = "josh-proxy")]
 pub struct Args {
-    pub remote: Remote,
-    pub local: String,
+    #[arg(long, required = true, value_parser = parse_remote)]
+    pub remote: Vec<Remote>,
+    #[arg(long, required = true)]
+    pub local: Option<String>,
+    #[arg(name = "poll", long)]
     pub poll_user: Option<String>,
+    #[arg(long, help = "Run git gc during maintenance")]
     pub gc: bool,
+    #[arg(long)]
     pub require_auth: bool,
+    #[arg(long)]
     pub no_background: bool,
+
+    #[arg(
+        short,
+        help = "DEPRECATED - no effect! Number of concurrent upstream git fetch/push operations"
+    )]
+    _n: Option<String>,
+
+    #[arg(long, default_value = "8000")]
     pub port: u16,
+    #[arg(
+        short,
+        default_value = "0",
+        help = "Duration between forced cache refresh"
+    )]
+    #[arg(long, short)]
     pub cache_duration: u64,
+    #[arg(long, help = "Proxy static resource requests to a different URL")]
     pub static_resource_proxy_target: Option<String>,
+    #[arg(long, help = "Filter to be prefixed to all queries of this instance")]
     pub filter_prefix: Option<String>,
 }
-
-fn parse_int<T: std::str::FromStr>(
-    matches: &clap::ArgMatches,
-    arg_name: &str,
-    default: Option<T>,
-) -> JoshResult<T>
-where
-    <T as std::str::FromStr>::Err: std::fmt::Display,
-{
-    let arg = matches.get_one::<String>(arg_name).map(|s| s.as_str());
-
-    let arg = match (arg, default) {
-        (None, None) => {
-            return Err(josh_error(&format!(
-                "missing required argument: {}",
-                arg_name
-            )))
-        }
-        (None, Some(default)) => Ok(default),
-        (Some(value), _) => value.parse::<T>(),
-    };
-
-    arg.map_err(|e| josh_error(&format!("error parsing argument {}: {}", arg_name, e)))
-}
-
-fn make_command() -> clap::Command {
-    clap::Command::new("josh-proxy")
-        .arg(
-            clap::Arg::new("remote")
-                .long("remote")
-                .action(clap::ArgAction::Append),
-        )
-        .arg(clap::Arg::new("local").long("local"))
-        .arg(clap::Arg::new("poll").long("poll"))
-        .arg(
-            clap::Arg::new("gc")
-                .long("gc")
-                .action(clap::ArgAction::SetTrue)
-                .help("Run git gc during maintenance"),
-        )
-        .arg(
-            clap::Arg::new("require-auth")
-                .long("require-auth")
-                .action(clap::ArgAction::SetTrue),
-        )
-        .arg(
-            clap::Arg::new("no-background")
-                .long("no-background")
-                .action(clap::ArgAction::SetTrue),
-        )
-        .arg(clap::Arg::new("n").short('n').help(
-            "DEPRECATED - no effect! Number of concurrent upstream git fetch/push operations",
-        ))
-        .arg(clap::Arg::new("port").long("port"))
-        .arg(
-            clap::Arg::new("cache-duration")
-                .long("cache-duration")
-                .short('c')
-                .help("Duration between forced cache refresh"),
-        )
-        .arg(
-            clap::Arg::new("static-resource-proxy-target")
-                .long("static-resource-proxy-target")
-                .help("Proxy static resource requests to a different URL"),
-        )
-        .arg(
-            clap::Arg::new("filter-prefix")
-                .long("filter-prefix")
-                .help("Filter to be prefixed to all queries of this instance"),
-        )
-}
-
-fn parse_remotes(values: &[String]) -> JoshResult<Remote> {
-    let mut result = Remote {
-        http: None,
-        ssh: None,
-    };
-
-    for value in values {
-        match value {
-            v if v.starts_with("http://") || v.starts_with("https://") => {
-                result.http = match result.http {
-                    None => Some(v.clone()),
-                    Some(v) => return Err(josh_error(&format!("HTTP remote already set: {}", v))),
-                };
-            }
-            v if v.starts_with("ssh://") => {
-                result.ssh = match result.ssh {
-                    None => Some(v.clone()),
-                    Some(v) => return Err(josh_error(&format!("SSH remote already set: {}", v))),
-                };
-            }
-            _ => {
-                return Err(josh_error(&format!(
-                    "Unsupported remote protocol: {}",
-                    value
-                )))
-            }
-        }
-    }
-
-    Ok(result)
-}
-
-pub fn parse_args() -> josh::JoshResult<Args> {
-    let args = make_command().get_matches_from(std::env::args());
-
-    let remote = args
-        .get_many::<String>("remote")
-        .ok_or(josh_error("no remote specified"))?
-        .cloned()
-        .collect::<Vec<_>>();
-    let remote = parse_remotes(&remote)?;
-
-    let local = args
-        .get_one::<String>("local")
-        .ok_or(josh_error("missing local directory"))?
-        .clone();
-
-    let poll_user = args.get_one::<String>("poll").map(String::clone);
-    let port = parse_int::<u16>(&args, "port", Some(8000))?;
-    let cache_duration = parse_int::<u64>(&args, "cache-duration", Some(0))?;
-    let static_resource_proxy_target = args
-        .get_one::<String>("static-resource-proxy-target")
-        .map(String::clone);
-
-    let filter_prefix = args.get_one::<String>("filter-prefix").map(String::clone);
-
-    Ok(Args {
-        remote,
-        local,
-        poll_user,
-        gc: args.get_flag("gc"),
-        require_auth: args.get_flag("require-auth"),
-        no_background: args.get_flag("no-background"),
-        port,
-        cache_duration,
-        static_resource_proxy_target,
-        filter_prefix,
-    })
-}
-
-pub fn parse_args_or_exit(code: i32) -> Args {
-    match parse_args() {
-        Err(e) => {
-            eprintln!("Argument parsing error: {}", e.0);
-            std::process::exit(code);
-        }
-        Ok(args) => args,
-    }
-}
-
-#[test]
-fn verify_app() {
-    make_command().debug_assert();
-}
diff --git a/tests/proxy/shell.t b/tests/proxy/shell.t
@@ -1,12 +1,12 @@
   $ . ${TESTDIR}/setup_test_env.sh
   $ kill -9 $(cat ${TESTTMP}/proxy_pid)
   $ ${TARGET_DIR}/debug/josh-proxy --help
-  Usage: josh-proxy [OPTIONS]
+  Usage: josh-proxy [OPTIONS] --remote <REMOTE> --local <LOCAL>
   
   Options:
-        --remote <remote>
+        --remote <REMOTE>
             
-        --local <local>
+        --local <LOCAL>
             
         --poll <poll>
             
@@ -16,15 +16,15 @@
             
         --no-background
             
-    -n <n>
+    -n <N>
             DEPRECATED - no effect! Number of concurrent upstream git fetch/push operations
-        --port <port>
-            
-    -c, --cache-duration <cache-duration>
-            Duration between forced cache refresh
-        --static-resource-proxy-target <static-resource-proxy-target>
+        --port <PORT>
+            [default: 8000]
+    -c, --cache-duration <CACHE_DURATION>
+            Duration between forced cache refresh [default: 0]
+        --static-resource-proxy-target <STATIC_RESOURCE_PROXY_TARGET>
             Proxy static resource requests to a different URL
-        --filter-prefix <filter-prefix>
+        --filter-prefix <FILTER_PREFIX>
             Filter to be prefixed to all queries of this instance
     -h, --help
             Print help
