refactor: hand clap's own structs to the commands

src/cli/link.rs

@@ -2,64 +2,80 @@ use std::path::PathBuf;
 
 use clap::{Args, Subcommand};
 
+// each variant carries its own arguments as a struct rather than as fields
+// spread across the enum. the command that implements it takes that struct, so
+// main does not unpack what clap has already parsed and hand it on as a row of
+// booleans nothing but position tells apart
 #[derive(Subcommand, Debug)]
 pub enum Link {
     /// Move untracked paths into the store and symlink them back
-    Add {
+    Add(Add),
-        /// Untracked or ignored paths inside the repository
-        #[arg(required = true)]
-        paths: Vec<PathBuf>,
-
-        /// Link to a store path that already exists
-        #[arg(long)]
-        force: bool,
-
-        #[command(flatten)]
-        store: Store,
-    },
 
     /// Move paths in the store back into the repository
-    Restore {
+    Restore(Restore),
-        // said here rather than checked at runtime, so a mistake in the
-        // arguments is reported as one, with the usage and the exit code clap
-        // gives every other argument error
-        #[arg(required_unless_present = "all", conflicts_with = "all")]
-        paths: Vec<PathBuf>,
-
-        /// Restore every path this repository has in the store
-        #[arg(long)]
-        all: bool,
-
-        #[command(flatten)]
-        store: Store,
-    },
 
     /// List what this repository keeps in the store
-    List {
+    List(List),
-        #[command(flatten)]
-        store: Store,
-    },
 
     /// List untracked paths a sandbox would still see
-    Check {
+    Check(Check),
-        /// Limit the listing to these paths
+}
-        paths: Vec<PathBuf>,
 
-        /// Print `<code> <path>` for scripts, as `git status --porcelain` does
+#[derive(Args, Debug)]
-        #[arg(long)]
+pub struct Add {
-        porcelain: bool,
+    /// Untracked or ignored paths inside the repository
+    #[arg(required = true)]
+    pub paths: Vec<PathBuf>,
 
-        /// Exit with 4 when anything is outside the store, for scripts
+    /// Link to a store path that already exists
-        #[arg(long)]
+    #[arg(long)]
-        exit_code: bool,
+    pub force: bool,
 
-        /// Terminate porcelain entries with NUL
+    #[command(flatten)]
-        #[arg(short = 'z')]
+    pub store: Store,
-        null: bool,
+}
 
-        #[command(flatten)]
+#[derive(Args, Debug)]
-        store: Store,
+pub struct Restore {
-    },
+    // said here rather than checked at runtime, so a mistake in the
+    // arguments is reported as one, with the usage and the exit code clap
+    // gives every other argument error
+    #[arg(required_unless_present = "all", conflicts_with = "all")]
+    pub paths: Vec<PathBuf>,
+
+    /// Restore every path this repository has in the store
+    #[arg(long)]
+    pub all: bool,
+
+    #[command(flatten)]
+    pub store: Store,
+}
+
+#[derive(Args, Debug)]
+pub struct List {
+    #[command(flatten)]
+    pub store: Store,
+}
+
+#[derive(Args, Debug)]
+pub struct Check {
+    /// Limit the listing to these paths
+    pub paths: Vec<PathBuf>,
+
+    /// Print `<code> <path>` for scripts, as `git status --porcelain` does
+    #[arg(long)]
+    pub porcelain: bool,
+
+    /// Exit with 4 when anything is outside the store, for scripts
+    #[arg(long)]
+    pub exit_code: bool,
+
+    /// Terminate porcelain entries with NUL
+    #[arg(short = 'z')]
+    pub null: bool,
+
+    #[command(flatten)]
+    pub store: Store,
 }
 
 #[derive(Args, Debug)]
@@ -70,3 +86,9 @@ pub struct Store {
     #[arg(long = "store", env = "AHAB_LINK_ROOT", value_name = "DIR")]
     pub root: Option<PathBuf>,
 }
+
+impl Store {
+    pub fn root(&self) -> Option<&std::path::Path> {
+        self.root.as_deref()
+    }
+}

src/cli/mod.rs

@@ -1,9 +1,9 @@
 // build.rs reaches these definitions with include!, so nothing here may refer to
 // the rest of the crate: keep this tree to clap definitions only
-mod ahab;
+pub mod ahab;
-mod django;
+pub mod django;
-mod link;
+pub mod link;
-mod postgres;
+pub mod postgres;
 
 pub use ahab::{Ahab, Commands};
 pub use django::Django;

src/cli/postgres.rs

@@ -1,6 +1,6 @@
 use std::path::PathBuf;
 
-use clap::{Subcommand, ValueEnum};
+use clap::{Args, Subcommand, ValueEnum};
 
 #[derive(Subcommand, Debug)]
 pub enum Postgres {
@@ -15,17 +15,20 @@ pub enum Postgres {
     },
 
     /// Dump via pg_dump, or pg_dumpall for a whole cluster
-    Dump {
+    Dump(Dump),
-        path: PathBuf,
+}
 
-        /// Dump format
+#[derive(Args, Debug)]
-        #[arg(short = 'F', long, value_enum, default_value_t = Format::Custom)]
+pub struct Dump {
-        format: Format,
+    pub path: PathBuf,
 
-        /// Compress the dump with gzip
+    /// Dump format
-        #[arg(short = 'z', long)]
+    #[arg(short = 'F', long, value_enum, default_value_t = Format::Custom)]
-        gzip: bool,
+    pub format: Format,
-    },
+
+    /// Compress the dump with gzip
+    #[arg(short = 'z', long)]
+    pub gzip: bool,
 }
 
 #[derive(ValueEnum, Clone, Copy, Debug, Default, PartialEq)]
@@ -51,16 +54,3 @@ pub enum Format {
     #[value(alias = "all")]
     Cluster,
 }
-
-impl Format {
-    // pg_dump's -F letter, which a cluster dump does not have: that is pg_dumpall
-    pub fn flag(&self) -> Option<&'static str> {
-        match self {
-            Self::Custom => Some("c"),
-            Self::Plain => Some("p"),
-            Self::Tar => Some("t"),
-            Self::Directory => Some("d"),
-            Self::Cluster => None,
-        }
-    }
-}

src/cmd/postgres.rs

@@ -1,4 +1,5 @@
 use super::{Argv, Cmd};
+use crate::cli::Format;
 
 // whether the server is accepting connections yet
 pub struct PgIsReady<'a> {
@@ -156,13 +157,25 @@ pub struct PgDump<'a> {
 }
 
 impl<'a> PgDump<'a> {
-    pub fn new(username: &'a str, dbname: &'a str, format: &'a str) -> Self {
+    // pg_dump's -F letter, which belongs with the rest of what this module
-        Self {
+    // knows about pg_dump's argv rather than with the parser that reads the
+    // user's `-F custom`. a cluster has no letter: that is pg_dumpall, so
+    // there is no PgDump to build for it
+    pub fn of(username: &'a str, dbname: &'a str, format: Format) -> Option<Self> {
+        let format = match format {
+            Format::Custom => "c",
+            Format::Plain => "p",
+            Format::Tar => "t",
+            Format::Directory => "d",
+            Format::Cluster => return None,
+        };
+
+        Some(Self {
             username,
             dbname,
             format,
             to: None,
-        }
+        })
     }
 
     // a path in the container, for the directory format, which pg_dump writes
@@ -201,7 +214,7 @@ impl Cmd for PgDumpAll<'_> {
 
 #[cfg(test)]
 mod tests {
-    use super::{DropDb, PgDump, Psql};
+    use super::{DropDb, Format, PgDump, Psql};
     use crate::cmd::Cmd;
 
     #[test]
@@ -219,16 +232,35 @@ mod tests {
 
     #[test]
     fn a_directory_dump_names_the_file_it_writes() {
+        let directory = PgDump::of("u", "db", Format::Directory).unwrap();
+
         assert_eq!(
-            PgDump::new("u", "db", "d").to("/tmp/dump").argv().quoted(),
+            directory.to("/tmp/dump").argv().quoted(),
             "pg_dump --username u --format d --file /tmp/dump -- db"
         );
         assert_eq!(
-            PgDump::new("u", "db", "c").argv().quoted(),
+            PgDump::of("u", "db", Format::Custom)
+                .unwrap()
+                .argv()
+                .quoted(),
             "pg_dump --username u --format c -- db"
         );
     }
 
+    #[test]
+    fn a_cluster_is_pg_dumpall_rather_than_a_pg_dump_letter() {
+        assert!(PgDump::of("u", "db", Format::Cluster).is_none());
+
+        for format in [
+            Format::Custom,
+            Format::Plain,
+            Format::Tar,
+            Format::Directory,
+        ] {
+            assert!(PgDump::of("u", "db", format).is_some(), "{format:?}");
+        }
+    }
+
     #[test]
     fn only_a_single_database_restore_stops_at_the_first_error() {
         assert!(

src/commands/link.rs

@@ -10,6 +10,7 @@ use std::path::{Path, PathBuf};
 use anyhow::{Context, Result, anyhow, bail};
 
 use self::store::{Leads, Repo, ignored, leads, resolve, symlink_metadata_opt, tracked};
+use crate::cli::link as cli;
 use crate::ctx::Ctx;
 use crate::fsops::suffixed;
 use crate::output::{line, note, warning};
@@ -19,8 +20,9 @@ const BACKUP_SUFFIX: &str = ".ahab-bak";
 const RESTORING_SUFFIX: &str = ".ahab-restoring";
 
 // move untracked paths out of the repo and symlink them back
-pub fn add(ctx: &Ctx, paths: &[PathBuf], force: bool, store: Option<&Path>) -> Result<()> {
+pub fn add(ctx: &Ctx, args: &cli::Add) -> Result<()> {
-    let repo = Repo::discover(ctx, store)?;
+    let (paths, force) = (args.paths.as_slice(), args.force);
+    let repo = Repo::discover(ctx, args.store.root())?;
     let report = Report::new(&repo);
 
     if let [path] = paths {
@@ -42,8 +44,9 @@ pub fn add(ctx: &Ctx, paths: &[PathBuf], force: bool, store: Option<&Path>) -> R
 }
 
 // move paths in the store back into the repo, the inverse of add
-pub fn restore(ctx: &Ctx, paths: &[PathBuf], all: bool, store: Option<&Path>) -> Result<()> {
+pub fn restore(ctx: &Ctx, args: &cli::Restore) -> Result<()> {
-    let repo = Repo::discover(ctx, store)?;
+    let (paths, all) = (args.paths.as_slice(), args.all);
+    let repo = Repo::discover(ctx, args.store.root())?;
     let report = Report::new(&repo);
 
     // clap requires one or the other and refuses both, so only the two real
@@ -225,8 +228,8 @@ pub fn stored_summary(ctx: &Ctx, store: Option<&Path>) -> Result<(PathBuf, usize
 }
 
 // list what the store holds for this repository, the inverse of check
-pub fn list(ctx: &Ctx, store: Option<&Path>) -> Result<()> {
+pub fn list(ctx: &Ctx, args: &cli::List) -> Result<()> {
-    let repo = Repo::discover(ctx, store)?;
+    let repo = Repo::discover(ctx, args.store.root())?;
     let stored = stored_paths(&repo, &repo.store)?;
 
     line!("store: {}", repo.store.display());

src/commands/link/check.rs

@@ -6,20 +6,15 @@ use std::path::{Path, PathBuf};
 use anyhow::{Result, anyhow};
 
 use super::store::{Leads, Repo, leads, resolve, symlink_metadata_opt};
+use crate::cli::link as cli;
 use crate::cmd::{Cmd, LsFiles};
 use crate::ctx::Ctx;
 use crate::output::{line, text, write_bytes};
 
 // whether anything is outside the store, which main turns into an exit code
-pub fn check(
+pub fn check(ctx: &Ctx, args: &cli::Check) -> Result<bool> {
-    ctx: &Ctx,
+    let repo = Repo::discover(ctx, args.store.root())?;
-    paths: &[PathBuf],
+    let pathspecs = relative_pathspecs(&repo, &args.paths)?;
-    porcelain: bool,
-    null: bool,
-    store: Option<&Path>,
-) -> Result<bool> {
-    let repo = Repo::discover(ctx, store)?;
-    let pathspecs = relative_pathspecs(&repo, paths)?;
 
     let mut exposed = Vec::new();
     // git lists untracked and ignored separately
@@ -40,8 +35,8 @@ pub fn check(
 
     exposed.sort_by(|a, b| a.name.cmp(&b.name));
 
-    if porcelain || null {
+    if args.porcelain || args.null {
-        print_porcelain(&exposed, null);
+        print_porcelain(&exposed, args.null);
     } else {
         print_listing(&repo, &exposed);
     }

src/commands/postgres.rs

@@ -10,6 +10,7 @@ use anyhow::{Context, Result, bail};
 use self::server::{Database, wait_until_ready, when_ready};
 use self::shape::{Dump, HEADER_LEN, Kind};
 use crate::cli::Format;
+use crate::cli::postgres as cli;
 use crate::cmd::{
     Cmd, Cp, CreateDb, DropDb, Gunzip, Gzip, Head, PgDump, PgDumpAll, PgRestore, Psql, Rm,
 };
@@ -297,10 +298,13 @@ pub fn psql(ctx: &Ctx, rest: &[String]) -> Result<()> {
         .replace(ctx)
 }
 
-pub fn dump(ctx: &Ctx, file: &Path, format: Format, gzip: bool) -> Result<()> {
+pub fn dump(ctx: &Ctx, args: &cli::Dump) -> Result<()> {
+    let (file, format, gzip) = (args.path.as_path(), args.format, args.gzip);
     let db = Database::resolve(ctx)?;
 
     if format == Format::Directory {
+        // main says this as an argument error before getting here; kept as the
+        // last word in case anything else ever calls dump
         if gzip {
             bail!("a directory dump is a directory of already compressed files, not a stream");
         }
@@ -343,8 +347,8 @@ pub fn dump(ctx: &Ctx, file: &Path, format: Format, gzip: bool) -> Result<()> {
 
 // pg_dump for one database, pg_dumpall for a cluster, which has no format letter
 fn dump_command(db: &Database, format: Format) -> Box<dyn Cmd + '_> {
-    match format.flag() {
+    match PgDump::of(&db.user, &db.name, format) {
-        Some(flag) => Box::new(PgDump::new(&db.user, &db.name, flag)),
+        Some(dump) => Box::new(dump),
         None => Box::new(PgDumpAll { username: &db.user }),
     }
 }
@@ -364,7 +368,8 @@ fn dump_directory(ctx: &Ctx, db: &Database, target: &Path) -> Result<()> {
     note!(ctx, "dumping to local directory {}", target.display());
     let remote = remote_dump();
 
-    PgDump::new(&db.user, &db.name, "d")
+    PgDump::of(&db.user, &db.name, Format::Directory)
+        .expect("a directory dump has a pg_dump letter")
         .to(&remote)
         .in_container(&db.container)
         .run(ctx)?;

src/main.rs

@@ -73,51 +73,32 @@ fn run(ctx: &Ctx, command: cli::Commands) -> Result<ExitCode> {
             match command {
                 cli::Postgres::Import { path } => commands::postgres::import(ctx, &path),
                 cli::Postgres::Psql { rest } => commands::postgres::psql(ctx, &rest),
-                cli::Postgres::Dump { path, format, gzip } => {
+                cli::Postgres::Dump(args) => {
                     // clap cannot say that a flag conflicts with one value of
                     // another, and this is still an argument error: it belongs
                     // with the usage and the exit code the others get
-                    if gzip && format == cli::Format::Directory {
+                    if args.gzip && args.format == cli::Format::Directory {
                         usage(
                             "a directory dump is a directory of already \
                              compressed files, not a stream",
                         );
                     }
 
-                    commands::postgres::dump(ctx, &path, format, gzip)
+                    commands::postgres::dump(ctx, &args)
                 }
             }?;
 
             Ok(done)
         }
         cli::Commands::Link { command } => match command {
-            cli::Link::Add {
+            cli::Link::Add(args) => commands::link::add(ctx, &args).map(|()| done),
-                paths,
+            cli::Link::List(args) => commands::link::list(ctx, &args).map(|()| done),
-                force,
+            cli::Link::Restore(args) => commands::link::restore(ctx, &args).map(|()| done),
-                store,
-            } => commands::link::add(ctx, &paths, force, store.root.as_deref()).map(|()| done),
-            cli::Link::List { store } => {
-                commands::link::list(ctx, store.root.as_deref()).map(|()| done)
-            }
-            cli::Link::Restore { paths, all, store } => {
-                commands::link::restore(ctx, &paths, all, store.root.as_deref()).map(|()| done)
-            }
             // the one command with something to say through its exit code
-            cli::Link::Check {
+            cli::Link::Check(args) => match commands::link::check(ctx, &args)? && args.exit_code {
-                paths,
+                true => Ok(ExitCode::from(FINDINGS)),
-                porcelain,
+                false => Ok(done),
-                null,
+            },
-                exit_code,
-                store,
-            } => {
-                let exposed =
-                    commands::link::check(ctx, &paths, porcelain, null, store.root.as_deref())?;
-
-                match exit_code && exposed {
-                    true => Ok(ExitCode::from(FINDINGS)),
-                    false => Ok(done),
-                }
-            }
         },
         cli::Commands::Status { store } => {
             commands::status::status(ctx, store.root.as_deref())?;