Show real mapping grammar in --help, make --via repeatable, trim comments

- -l/-r/-d now show their actual grammar as the clap value name (<[BIND:]PORT:HOST:PORT>, <[BIND:]PORT>) instead of a generic <SPEC>, matching vmic's convention of showing real syntax in --help. The grammar is no longer also repeated in the flag's help text, since the value name already carries it. - --via changes from a single comma-separated flag to a repeatable one (Vec<String> + value_delimiter = ','), so both `--via a --via b` and `--via a,b` work and can be mixed. commands/mod.rs's edits_from_mapping no longer needs to split the string itself - clap does it. - Trimmed CLI help text that restated grammar/rationale already covered by the value name or by --via being required. - Normalized code comments for tone/format consistency: dropped cross-references to vmic's own internals as justification (this codebase should read as self-contained), removed markdown-style *emphasis* asterisks that don't render in plain comments, tightened several run-on sentences into plain declarative ones, and fixed one comment in wipe.rs that inaccurately described close_instance's force path (it sends SIGKILL, not SIGTERM). - spec/porthole-spec.md's --via row updated to document the repeatable form alongside comma-separated. Cargo.toml/Cargo.lock/atomic.rs also carry an external toml crate bump and formatting pass picked up from the working tree.
2026-08-13 17:18:33 +02:00
parent 4a8faf1131
commit 4170d51cf5
14 changed files with 105 additions and 115 deletions
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -306,11 +306,11 @@ dependencies = [

 [[package]]
 name = "serde_spanned"
-version = "0.6.9"
+version = "1.1.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "bf41e0cfaf7226dca15e8197172c295a782857fcb97fad1808a166870dee75a3"
+checksum = "6662b5879511e06e8999a8a235d848113e942c9124f211511b16466ee2995f26"
 dependencies = [
- "serde",
+ "serde_core",
 ]

 [[package]]
@@ -352,44 +352,42 @@ dependencies = [

 [[package]]
 name = "toml"
-version = "0.8.23"
+version = "1.1.4+spec-1.1.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "dc1beb996b9d83529a9e75c17a1686767d148d70663143c7854d8b4a09ced362"
-dependencies = [
- "serde",
- "serde_spanned",
- "toml_datetime",
- "toml_edit",
-]
-
-[[package]]
-name = "toml_datetime"
-version = "0.6.11"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "22cddaf88f4fbc13c51aebbf5f8eceb5c7c5a9da2ac40a13519eb5b0a0e8f11c"
-dependencies = [
- "serde",
-]
-
-[[package]]
-name = "toml_edit"
-version = "0.22.27"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "41fe8c660ae4257887cf66394862d21dbca4a6ddd26f04a3560410406a2f819a"
+checksum = "3aace63f4bbcdfc2c965b059de67119c89c4017a70d633be6c104910f67056f5"
 dependencies = [
 "indexmap",
- "serde",
+ "serde_core",
 "serde_spanned",
 "toml_datetime",
- "toml_write",
+ "toml_parser",
+ "toml_writer",
 "winnow",
 ]

 [[package]]
-name = "toml_write"
-version = "0.1.2"
+name = "toml_datetime"
+version = "1.1.1+spec-1.1.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "5d99f8c9a7727884afe522e9bd5edbfc91a3312b36a77b5fb8926e4c31a41801"
+checksum = "3165f65f62e28e0115a00b2ebdd37eb6f3b641855f9d636d3cd4103767159ad7"
+dependencies = [
+ "serde_core",
+]
+
+[[package]]
+name = "toml_parser"
+version = "1.1.3+spec-1.1.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "1d38ac1cf9b95face32296c0a3ede1fdc270627c9d9c02a7274dd6d960dc4d56"
+dependencies = [
+ "winnow",
+]
+
+[[package]]
+name = "toml_writer"
+version = "1.1.2+spec-1.1.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "7d56353a2a665ad0f41a421187180aab746c8c325620617ad883a99a1cbe66d2"

 [[package]]
 name = "unicode-ident"
@@ -426,12 +424,9 @@ dependencies = [

 [[package]]
 name = "winnow"
-version = "0.7.15"
+version = "1.0.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "df79d97927682d2fd8adb29682d1140b343be4ac0f08fd68b7765d9c059d3945"
-dependencies = [
- "memchr",
-]
+checksum = "23b97319f7b8343df12cc98938e5c3eb436064524c8d2b4e30a1d3a36eecdf81"

 [[package]]
 name = "zmij"
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -18,7 +18,7 @@ dirs      = "6"

 serde      = { version = "1", features = ["derive"] }
 serde_json = "1"
-toml       = "0.8"
+toml       = "^1"

 libc = "0.2"

--- a/spec/porthole-spec.md
+++ b/spec/porthole-spec.md
@@ -204,7 +204,7 @@ Saves a new profile. Does **not** open it.
 | `-l, --local`      | `[bind:]port:host:hostport`   | one of `-l/-r/-d` | —                         | Local forward: your machine → remote   |
 | `-r, --remote`     | `[bind:]port:host:hostport`   | one of `-l/-r/-d` | —                         | Remote forward: remote → your machine  |
 | `-d, --dynamic`    | `[bind:]port`                 | one of `-l/-r/-d` | —                         | Dynamic forward (SOCKS proxy)          |
-| `--via`            | `[user@]host[:port][,...]`    | **yes**            | —                         | Comma-separated hop chain; the *last* hop is the actual `ssh` connection target, any before it are `-J` jumps (§3.1) |
+| `--via`            | `[user@]host[:port]`          | **yes**            | —                         | One hop chain entry; repeatable (`--via a --via b`) and/or comma-separated (`--via a,b`) - the last hop is the `ssh` connection target, any before it are `-J` jumps (§3.1) |
 | `-u, --user`       | `user`                        | no                 | current user / ssh_config | Default user for the final target and any `--via` hop that doesn't specify its own |
 | `-i, --identity`   | `path`                        | no                 | ssh_config default        |                                        |
 | `-p, --port`       | `port`                        | no                 | `22`                      | SSH port on the final target only — a `--via` hop needs its own inline `:port` if it isn't 22 |
--- a/src/atomic.rs
+++ b/src/atomic.rs
@@ -7,7 +7,8 @@
 use std::io::Write;
 use std::path::Path;

-pub fn write(path: &Path, contents: &[u8]) -> std::io::Result<()> {
+pub fn write(path: &Path, contents: &[u8]) -> std::io::Result<()>
+{
    let tmp = path.with_extension(format!(
        "{}.tmp.{}",
        path.extension().and_then(|e| e.to_str()).unwrap_or(""),
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -54,24 +54,23 @@ pub enum Commands {
 /// struct (`#[command(flatten)]`ed into both) so the two can never drift.
 #[derive(Args, Default)]
 pub struct MappingArgs {
-    /// Local forward: your machine -> remote. [bind:]port:host:hostport
-    #[arg(short, long, value_name = "SPEC")]
+    /// Local forward: your machine -> remote.
+    #[arg(short, long, value_name = "[BIND:]PORT:HOST:PORT")]
    pub local: Option<String>,

-    /// Remote forward: remote -> your machine. [bind:]port:host:hostport
-    #[arg(short, long, value_name = "SPEC")]
+    /// Remote forward: remote -> your machine.
+    #[arg(short, long, value_name = "[BIND:]PORT:HOST:PORT")]
    pub remote: Option<String>,

-    /// Dynamic forward (SOCKS proxy). [bind:]port
-    #[arg(short, long, value_name = "SPEC")]
+    /// Dynamic forward (SOCKS proxy).
+    #[arg(short, long, value_name = "[BIND:]PORT")]
    pub dynamic: Option<String>,

-    /// SSH hop chain, comma-separated: [user@]host[:port][,...]. The last
-    /// hop is the actual connection target, any before it are -J jumps.
-    #[arg(long, value_name = "HOPS")]
-    pub via: Option<String>,
+    /// SSH hop chain: jump hosts plus the final connection target.
+    #[arg(long, value_name = "[USER@]HOST[:PORT]", value_delimiter = ',')]
+    pub via: Vec<String>,

-    /// Default user for the target and any --via hop without its own.
+    /// Default user for the target and any hop without one.
    #[arg(short, long, value_name = "USER")]
    pub user: Option<String>,

--- a/src/commands/mod.rs
+++ b/src/commands/mod.rs
@@ -11,12 +11,13 @@ pub mod wipe;
 use crate::cli::MappingArgs;
 use crate::profile::ProfileEdits;

-/// Turns clap's `MappingArgs` into a `ProfileEdits`, splitting `--via`'s
-/// comma-separated raw string into the ordered hop list (spec §5.1).
+/// Turns clap's `MappingArgs` into a `ProfileEdits` (spec §5.1). `--via` is
+/// collected by clap itself: `value_delimiter = ','` splits each
+/// occurrence on commas, and the field being a `Vec` allows repeated
+/// `--via` flags, so both `--via a,b` and `--via a --via b` reach here as
+/// `["a", "b"]`.
 pub fn edits_from_mapping(m: &MappingArgs) -> ProfileEdits {
-    let via = m.via.as_deref().map(|raw| {
-        raw.split(',').map(str::trim).filter(|s| !s.is_empty()).map(str::to_string).collect::<Vec<_>>()
-    });
+    let via = (!m.via.is_empty()).then(|| m.via.clone());
    ProfileEdits {
        local: m.local.clone(),
        remote: m.remote.clone(),
@@ -33,12 +34,12 @@ pub fn edits_from_mapping(m: &MappingArgs) -> ProfileEdits {
 }

 /// `true` if `MappingArgs` carries no edits at all - used by `edit` to
-/// reject a no-op invocation the same way vmic's commands do.
+/// reject a no-op invocation.
 pub fn mapping_is_empty(m: &MappingArgs) -> bool {
    m.local.is_none()
        && m.remote.is_none()
        && m.dynamic.is_none()
-        && m.via.is_none()
+        && m.via.is_empty()
        && m.user.is_none()
        && m.identity.is_none()
        && m.port.is_none()
--- a/src/commands/open.rs
+++ b/src/commands/open.rs
@@ -56,9 +56,9 @@ fn open_one(name: &str, foreground: bool, once: bool) -> Result<()> {
        ui::info(&format!("'{name}' is already open (pid {pid})."));
        return Ok(());
    }
-    // A stale instance file left by a crash shouldn't linger through a
-    // fresh spawn - the lock is the real authority (spec §5.2), this just
-    // keeps `status` from showing ghost state mid-spawn.
+    // Clear a stale instance file left by a crash before spawning. The
+    // lock, not this file, is the authority on "already open" (spec
+    // §5.2) - this just keeps `status` from reading stale state mid-spawn.
    instance::delete(name)?;

    if foreground {
@@ -100,12 +100,12 @@ fn spawn_detached(name: &str, once: bool) -> Result<()> {
 /// Blocks briefly for the detached supervisor to reach a conclusive state,
 /// so an immediate failure (bad auth, bind conflict, unresolvable host) is
 /// reported with a non-zero exit instead of `open` appearing to succeed
-/// (spec §5.2). The instance file's *initial* write is always
-/// `State::Reconnecting` (the first attempt hasn't concluded yet) - that's
-/// indistinguishable from "already failed once, backing off" by state
-/// alone, so this must wait specifically for `Up` or `Error`, not just
-/// "anything other than Error", or it would report success before the
-/// first connection attempt has even had a chance to run.
+/// (spec §5.2). The instance file's initial write is always
+/// `State::Reconnecting`, since the first attempt has not concluded yet;
+/// that value is indistinguishable from "already failed once, backing
+/// off". This function waits specifically for `Up` or `Error`, not merely
+/// for a state other than `Error`, so it does not report success before
+/// the first connection attempt has run.
 fn wait_for_confirmation(name: &str) -> Result<()> {
    let deadline = Instant::now() + CONFIRM_TIMEOUT;
    loop {
--- a/src/commands/wipe.rs
+++ b/src/commands/wipe.rs
@@ -41,10 +41,10 @@ pub fn run(args: WipeArgs) -> Result<()> {
    Ok(())
 }

-/// Kills any supervisor process not backed by a tracked profile (e.g.
-/// orphaned after a crash), matched by cmdline rather than tracked state -
-/// same spirit as vmic's `wipe` (spec §5.8). SIGTERM to the whole process
-/// group, same reasoning as `close_instance`'s force path.
+/// Kills any supervisor process not backed by a tracked profile (e.g. one
+/// orphaned after a crash), matched by cmdline rather than tracked state
+/// (spec §5.8). Sends SIGTERM to each supervisor's process group so its
+/// `ssh` child is included.
 fn kill_orphaned_supervisors() -> u32 {
    let mut killed = 0;
    let Ok(entries) = std::fs::read_dir("/proc") else { return 0 };
--- a/src/instance.rs
+++ b/src/instance.rs
@@ -33,7 +33,7 @@ pub struct Instance {
    pub state: State,
    /// Anchor for "session uptime" (spec §5.5) - set once, when `open` starts.
    pub opened_at: i64,
-    /// Start of the *current* unbroken connection; resets each reconnect.
+    /// Start of the current unbroken connection; resets each reconnect.
    pub connected_at: Option<i64>,
    pub last_error: Option<String>,
    pub reconnect_count: u32,
@@ -77,8 +77,8 @@ pub fn log_path(name: &str) -> PathBuf {
    state_dir().join(format!("{name}.log"))
 }

-/// Loads the instance file for `name`, if any. `None` means "closed" - the
-/// absence of this file *is* the closed state (spec §2.2); there's no
+/// Loads the instance file for `name`, if any. `None` means closed - the
+/// absence of this file is the closed state (spec §2.2); there is no
 /// separate enum value for it.
 pub fn load(name: &str) -> Result<Option<Instance>> {
    let path = instance_path(name);
@@ -109,9 +109,8 @@ pub fn process_alive(pid: i32) -> bool {
    unsafe { libc::kill(pid, 0) == 0 }
 }

-/// True only if `pid` is a live process whose cmdline looks like our own
-/// supervisor for `name` - insurance against a stale/reused pid, same
-/// spirit as vmic's `pid_matches` check on `pw-loopback` processes.
+/// True only if `pid` is a live process whose cmdline identifies it as the
+/// supervisor for `name` - guards against a stale or reused pid.
 pub fn supervisor_alive(pid: i32, name: &str) -> bool {
    if !process_alive(pid) {
        return false;
@@ -134,12 +133,10 @@ pub fn running_pid(name: &str) -> Result<Option<i32>> {
    }
 }

-/// Advisory `flock` held for the supervisor's entire lifetime (spec §2.3) -
-/// releases automatically when the holding process exits or is killed, no
-/// matter how abruptly, which is what makes it a reliable "is a supervisor
-/// actually alive for this profile" primitive even across a crash: the OS
-/// drops the lock the instant the fd closes, no stale-lock cleanup needed
-/// the way a plain pidfile would require.
+/// Advisory `flock` held for the supervisor's entire lifetime (spec §2.3).
+/// The OS releases it the instant the holding process's file descriptors
+/// close, including on a crash or SIGKILL, so it needs no stale-lock
+/// cleanup and reliably answers "is a supervisor running for this profile."
 pub struct Lock {
    _file: File,
 }
--- a/src/main.rs
+++ b/src/main.rs
@@ -16,8 +16,8 @@ fn main() {
    let args: Vec<String> = std::env::args().collect();

    // Plain `porthole`, `-h`/`--help`, or `help` at the top level: show
-    // every subcommand's own flags inline instead of making the user drill
-    // into each one with its own `--help` - same convention as vmic.
+    // every subcommand's own flags inline instead of requiring a
+    // per-subcommand `--help`.
    if wants_top_level_help(&args) {
        print_full_help();
        std::process::exit(if args.len() <= 1 { 2 } else { 0 });
@@ -65,11 +65,9 @@ fn wants_top_level_help(args: &[String]) -> bool {
    }
 }

-/// Prints one screen of help: one short summary line per subcommand (name,
-/// positional args, about), followed by an indented line per flag with its
-/// own help text - so nothing requires drilling into a subcommand's own
-/// `--help` just to see what it takes. Ported from vmic's `print_full_help`
-/// so the two tools present identically.
+/// Prints one screen of help: one summary line per subcommand (name,
+/// positional args, description), followed by an indented line per flag
+/// with its own help text.
 fn print_full_help() {
    let mut cmd = Cli::command();
    cmd.build(); // resolve default value names etc. before introspecting
@@ -149,8 +147,8 @@ fn positional_args(cmd: &clap::Command) -> String {
 }

 /// `(flag display, help text)` for each of `cmd`'s non-positional, non-help
-/// args, e.g. `("-l/--local <SPEC>", "Local forward: your machine ->
-/// remote")`. The flag display is colored yellow.
+/// args, e.g. `("-l/--local <[BIND:]PORT:HOST:PORT>", "Local forward: your
+/// machine -> remote")`. The flag display is colored yellow.
 fn flag_rows(cmd: &clap::Command) -> Vec<(String, String)> {
    cmd.get_arguments()
        .filter(|a| !a.is_positional() && a.get_id().as_str() != "help")
--- a/src/profile.rs
+++ b/src/profile.rs
@@ -237,10 +237,9 @@ impl Profile {
    }

    /// Splits `via` into the `-J` jump-chain value (comma-joined, all but
-    /// the last hop - `None` for a single-hop `via`) and the final `ssh`
-    /// connection target - see spec §3.1. `via` is validated non-empty by
-    /// every path that constructs a `Profile`, so the empty case here is
-    /// unreachable in practice.
+    /// the last hop; `None` for a single-hop `via`) and the final `ssh`
+    /// connection target - see spec §3.1. Every path that constructs a
+    /// `Profile` validates `via` as non-empty.
    pub fn ssh_target(&self) -> (Option<String>, &str) {
        match self.via.split_last() {
            Some((target, jumps)) if !jumps.is_empty() => (Some(jumps.join(",")), target.as_str()),
--- a/src/supervisor.rs
+++ b/src/supervisor.rs
@@ -19,8 +19,8 @@ const STABLE_THRESHOLD_SECS: i64 = 60;
 /// Consecutive unrecognized (not pattern-matched) failures before porthole
 /// gives up on an apparently-permanently-broken profile - spec §4.2.
 const MAX_UNRECOGNIZED_STREAK: u32 = 10;
-/// How long `ssh` must stay alive before porthole calls it "connected" -
-/// see `run_ssh_once`'s doc comment for why this heuristic is used at all.
+/// How long `ssh` must stay alive before porthole treats it as connected;
+/// see `run_ssh_once` for the heuristic this backs.
 const CONNECT_GRACE: Duration = Duration::from_secs(2);
 const POLL_INTERVAL: Duration = Duration::from_millis(200);
 const LOG_ROTATE_BYTES: u64 = 10 * 1024 * 1024;
@@ -31,12 +31,12 @@ extern "C" fn handle_sigterm(_sig: libc::c_int) {
    SHUTDOWN.store(true, Ordering::SeqCst);
 }

-/// Traps SIGTERM (and SIGINT, for `-f/--foreground`'s Ctrl-C - spec §5.2)
-/// into a flag instead of the default terminate-immediately behavior, so
-/// `close` is distinguished from a dropped `ssh` connection by *why* the
-/// loop is unwinding, not by guessing from `ssh`'s exit status - which is
-/// not a reliable signal either way. In foreground mode this function runs
-/// in the same process the terminal sends Ctrl-C's SIGINT to, since
+/// Traps SIGTERM and SIGINT into a flag instead of the default
+/// terminate-immediately behavior. This is how `close` (SIGTERM) and
+/// `-f/--foreground`'s Ctrl-C (SIGINT, spec §5.2) are distinguished from a
+/// dropped `ssh` connection: by which signal arrived, not by inferring
+/// intent from `ssh`'s exit status. In foreground mode this function runs
+/// in the process the terminal sends Ctrl-C to directly, since
 /// `commands::open` calls `supervisor::run` inline rather than detaching.
 fn install_signal_handler() {
    unsafe {
@@ -56,8 +56,9 @@ enum Outcome {
    Failed { class: Class, message: String },
 }

-/// Entry point for `porthole __supervise <name>`. Runs until told to stop
-/// (SIGTERM) or gives up per §4 - this *is* the supervisor process.
+/// Entry point for `porthole __supervise <name>`. This function is the
+/// supervisor process: it runs until told to stop (SIGTERM/SIGINT) or
+/// gives up per §4.
 pub fn run(name: &str) -> Result<()> {
    install_signal_handler();

@@ -141,10 +142,10 @@ fn sleep_or_shutdown(dur: Duration) -> bool {

 /// Spawns one `ssh` attempt and supervises it until it exits or shutdown is
 /// requested. Marks `inst` as `State::Up` once the process has survived
-/// `CONNECT_GRACE` - `ssh` gives no more reliable "the forward is actually
-/// bound" signal than that without parsing `-v` debug output, and a real
-/// failure exits near-instantly under `ExitOnForwardFailure=yes` (§3.1), so
-/// staying alive past the grace window is a reasonable proxy for connected.
+/// `CONNECT_GRACE`. `ssh` does not report "the forward is bound" directly
+/// without parsing `-v` debug output; a real failure exits near-instantly
+/// under `ExitOnForwardFailure=yes` (§3.1), so staying alive past the grace
+/// window is used as a proxy for connected.
 fn run_ssh_once(name: &str, profile: &Profile, inst: &mut Instance) -> Outcome {
    rotate_log_if_large(name);

--- a/src/timefmt.rs
+++ b/src/timefmt.rs
@@ -1,7 +1,7 @@
-//! Minimal UTC timestamp/duration formatting - no `chrono`/`time` dependency,
-//! matching vmic's minimal-dependency footprint. Timestamps are stored as
-//! Unix seconds (`i64`) everywhere in profile/instance state; this module
-//! only turns them into text for `status`/`list` output.
+//! Minimal UTC timestamp/duration formatting, without a `chrono`/`time`
+//! dependency. Timestamps are stored as Unix seconds (`i64`) everywhere in
+//! profile/instance state; this module only turns them into text for
+//! `status`/`list` output.

 use std::time::{SystemTime, UNIX_EPOCH};

--- a/src/ui.rs
+++ b/src/ui.rs
@@ -1,5 +1,4 @@
-//! Colorized status output (NO_COLOR-aware). Same conventions as vmic's
-//! `ui.rs` - kept identical on purpose so the two tools feel like one family.
+//! Colorized status output, honoring `NO_COLOR` and terminal detection.

 use std::io::IsTerminal;
 use std::sync::OnceLock;