cli and local_server: allow more complicated ports specifications

The default is now `--port 8080-8090 --port 0` to try 11
ports, and then to tell the OS to use a random open
port if all of those were busy.

CHANGELOG.md

@@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Breaking changes
 
+* The `--port-range` option was removed. Instead, `--port` can now be repeated and accepts dash-separated ranges. For example, the default is now equivalent to `--port 8080-8090 --port 0`.
+
+* This also corresponds to a breaking change to the `local_server` library interface
+
 ### New features
 
 ### Fixed bugs

src/bin/diffedit3.rs

@@ -1,22 +1,81 @@
+use std::str::FromStr;
+
 use clap::Parser;
 use diffedit3::local_server::{run_server, MergeToolError};
+use thiserror::Error;
+
+#[derive(Debug, PartialEq, Eq, Clone)]
+struct PortRange {
+    min_port: usize,
+    max_port: usize,
+}
+
+impl PortRange {
+    fn iter(&self) -> impl Iterator<Item = usize> {
+        self.min_port..=self.max_port
+    }
+}
+
+#[derive(Debug, PartialEq, Eq, Clone, Error)]
+enum ParsePortRangeError {
+    #[error(
+        "A port range must be one unsigned integer or two unsigned integers separated by a -, for \
+         example 1234 or 1234-56789"
+    )]
+    SyntaxError,
+    #[error("The minimum port {0} cannot be greater than the maximum port {1}")]
+    InequalityError(usize, usize),
+}
+
+impl FromStr for PortRange {
+    type Err = ParsePortRangeError;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let parse_usize = |s: &str| {
+            s.parse::<usize>()
+                .map_err(|_| ParsePortRangeError::SyntaxError)
+        };
+        match s.split_once('-') {
+            None => {
+                let port = parse_usize(s)?;
+                Ok(Self {
+                    min_port: port,
+                    max_port: port,
+                })
+            }
+            Some((first, second)) => {
+                let (min_port, max_port) = (parse_usize(first)?, parse_usize(second)?);
+                if min_port <= max_port {
+                    Ok(Self { min_port, max_port })
+                } else {
+                    Err(ParsePortRangeError::InequalityError(min_port, max_port))
+                }
+            }
+        }
+    }
+}
 
 /// Compare three directories in a browser and allow editing one of them
 #[derive(Parser)]
 #[command(version, about)]
 pub struct LocalServerCli {
     #[command(flatten)]
     lib_cli: diffedit3::fs::Cli,
-    /// Port to use for `http://127.0.0.1`
-    #[arg(long, short, conflicts_with = "port_range")]
-    port: Option<usize>,
-    // TODO: Change syntax from `--port-range 8080 8085` to `--port 8080-8085`?
-    /// Minimum and maximum port numbers to try for `http://127.0.0.1`.
+    /// Port or port range to use for `http://127.0.0.1` (can be repeated)
+    ///
+    /// Port 0 is a special value that instructs the OS to pick a random unused
+    /// port number. Ports 1-1023 are generally unavailable for use by
+    /// unpriveledged processes.
     ///
-    /// First, the minimum port is tried. If that is busy, the next port is
-    /// tried, and so on.
-    #[arg(long, num_args(2), default_values = ["8080", "8090"])]
-    port_range: Vec<usize>,
+    /// For example, the default is equivalent to `--port 8080-8090 --port 0`.
+    /// This means that port number 8080 is tried first. If that is busy, port
+    /// 8081 is tried, and so on. If all ports between 8080 and 8090 (inclusive)
+    /// are busy, port 0 is tried, meaning that the OS picks a random open port.
+    #[arg(
+        long, short, default_values = ["8080-8090", "0"],
+        value_name = "PORT_OR_PORT_RANGE", value_parser = PortRange::from_str
+    )]
+    port: Vec<PortRange>,
     /// Do not try to open the browser automatically
     ///
     /// See <https://crates.io/crates/open> for a brief description of how the
@@ -58,17 +117,13 @@ async fn main() -> Result<(), MergeToolError> {
         tracing_subscriber::fmt::init();
     }
 
-    let (min_port, max_port) = match cli.port {
-        Some(port) => (port, port),
-        None => (cli.port_range[0], cli.port_range[1]), // Clap guarantees exactly two values
-    };
-    if min_port > max_port {
-        exit_with_cli_error(format!(
-            "Error: the minimum port {min_port} cannot be greater than the maximum port \
-             {max_port}."
-        ));
-    };
-    if let Err(err) = run_server(input, min_port, max_port, !cli.no_browser).await {
+    if let Err(err) = run_server(
+        input,
+        Box::new(cli.port.into_iter().flat_map(|port| port.iter())),
+        !cli.no_browser,
+    )
+    .await
+    {
         std::process::exit(match err {
             MergeToolError::IOError(err) => {
                 eprintln!("{err}");

src/local_server.rs

@@ -16,6 +16,7 @@ use thiserror::Error;
 
 use crate::DataInterface;
 type DataInterfacePointer = Arc<Mutex<Box<dyn DataInterface>>>;
+pub type PortsToTry = Box<dyn Iterator<Item = usize>>;
 
 #[derive(rust_embed::RustEmbed)]
 #[folder = "webapp/dist/"]
@@ -131,8 +132,7 @@ fn acceptor_to_socket_address(
 
 pub async fn run_server(
     input: Box<dyn crate::DataInterface>,
-    min_port: usize,
-    max_port: usize,
+    ports_to_try: PortsToTry,
     open_browser: bool,
 ) -> Result<(), MergeToolError> {
     let (terminate_channel, mut terminate_rx): (ExitCodeSender, _) = tokio::sync::mpsc::channel(10);
@@ -147,21 +147,19 @@ pub async fn run_server(
         .nest("/", EmbeddedFilesEndpoint::<StaticFiles>::new())
         .nest("/api", apis);
 
-    let mut port = min_port;
-    let mut error = None;
-    let acceptor = loop {
-        if port > max_port {
-            return Err(MergeToolError::FailedToOpenPort(error.unwrap()));
+    let acceptor = 'acceptor: {
+        let mut error = None;
+        for port in ports_to_try {
+            let listener = TcpListener::bind(format!("127.0.0.1:{}", port));
+            match listener.into_acceptor().await {
+                Ok(a) => break 'acceptor a,
+                Err(err) => {
+                    eprintln!("Couldn't bind to port {port}.");
+                    error = Some(err)
+                }
+            };
         }
-        let listener = TcpListener::bind(format!("127.0.0.1:{}", port));
-        match listener.into_acceptor().await {
-            Ok(a) => break a,
-            Err(err) => {
-                eprintln!("Couldn't bind to port {port}.");
-                error = Some(err)
-            }
-        };
-        port += 1;
+        return Err(MergeToolError::FailedToOpenPort(error.unwrap()));
     };
 
     // Get the actual address we bound to. The primary reason to do this instead of
@@ -212,13 +210,12 @@ pub async fn run_server(
 /// Initialize tokio correctly and call `run_server`
 pub fn run_server_sync(
     input: Box<dyn crate::DataInterface>,
-    min_port: usize,
-    max_port: usize,
+    ports_to_try: PortsToTry,
     open_browser: bool,
 ) -> Result<(), MergeToolError> {
     tokio::runtime::Builder::new_multi_thread()
         .enable_all()
         .build()
         .unwrap()
-        .block_on(run_server(input, min_port, max_port, open_browser))
+        .block_on(run_server(input, ports_to_try, open_browser))
 }