refactor: one helper each for the repeated blocks

src/cmd/argv.rs

@@ -65,9 +65,8 @@ impl Argv {
         &self.0
     }
 
-    // one word list for `sh -c`, quoted so a value with a space survives the
+    // for reading: the -v echo, the dry run's plan, and the command named in an
-    // shell. lossy: this is for reading, and for scripts built only out of the
+    // error. lossy, because a message about a path is still worth printing
-    // ascii and compose-derived words that reach a pipeline
     pub fn quoted(&self) -> String {
         let words: Vec<String> = self
             .0
@@ -82,6 +81,28 @@ impl Argv {
         shlex::try_join(words.iter().map(String::as_str)).unwrap_or_else(|_| words.join(" "))
     }
 
+    // one word list for a shell to read, which is a string and so cannot carry
+    // a word that is not text. only ascii and compose-derived words reach a
+    // pipeline today; this is what keeps it that way, rather than a comment
+    // saying so while to_string_lossy quietly rewrites a path
+    pub fn for_shell(&self) -> Result<String> {
+        let mut words = Vec::with_capacity(self.0.len());
+
+        for word in &self.0 {
+            let word = word.to_str().ok_or_else(|| {
+                anyhow!(
+                    "`{}` cannot go in a shell pipeline: {:?} is not text",
+                    self.quoted(),
+                    word
+                )
+            })?;
+
+            words.push(word);
+        }
+
+        shlex::try_join(words).map_err(|e| anyhow!("quoting `{}`: {e}", self.quoted()))
+    }
+
     pub fn run(&self, ctx: &Ctx) -> Result<()> {
         if self.skipped(ctx) {
             return Ok(());

src/cmd/mod.rs

@@ -30,13 +30,13 @@ pub trait Cmd {
     fn argv(&self) -> Argv;
 
     // this command as one word list, for a shell to read
-    fn shell(&self) -> String {
+    fn shell(&self) -> Result<String> {
-        self.argv().quoted()
+        self.argv().for_shell()
     }
 
     // adapters
-    fn pipe(&self, next: &dyn Cmd) -> Pipeline {
+    fn pipe(&self, next: &dyn Cmd) -> Result<Pipeline> {
-        Pipeline::starting(self.shell()).pipe(next)
+        Pipeline::starting(self.shell()?).pipe(next)
     }
 
     fn in_container(&self, container: &str) -> Exec {

src/cmd/shell.rs

@@ -1,3 +1,5 @@
+use anyhow::Result;
+
 use super::{Argv, Cmd};
 
 // sh -c, the only way to reach a shell feature inside a container
@@ -30,9 +32,9 @@ impl Pipeline {
         }
     }
 
-    pub fn pipe(mut self, next: &dyn Cmd) -> Self {
+    pub fn pipe(mut self, next: &dyn Cmd) -> Result<Self> {
-        self.stages.push(next.shell());
+        self.stages.push(next.shell()?);
-        self
+        Ok(self)
     }
 
     // for a pipeline whose last stage closes the pipe on purpose, where the
@@ -119,21 +121,26 @@ mod tests {
 
     #[test]
     fn a_pipeline_reports_a_stage_that_dies_mid_stream() {
-        let script = Gunzip.pipe(&Gzip).shell();
+        let script = Gunzip.pipe(&Gzip).unwrap().shell().unwrap();
 
         assert_eq!(script, "sh -c 'set -o pipefail; gunzip -c | gzip'");
     }
 
     #[test]
     fn a_pipeline_that_closes_the_pipe_on_purpose_keeps_the_default() {
-        let script = Gunzip.pipe(&Head::bytes(512)).allow_early_close().shell();
+        let script = Gunzip
+            .pipe(&Head::bytes(512))
+            .unwrap()
+            .allow_early_close()
+            .shell()
+            .unwrap();
 
         assert_eq!(script, "sh -c 'gunzip -c | head -c 512'");
     }
 
     #[test]
     fn a_pipeline_reaches_a_container_as_one_argument() {
-        let argv = Gunzip.pipe(&Gzip).in_container("abc123").argv();
+        let argv = Gunzip.pipe(&Gzip).unwrap().in_container("abc123").argv();
 
         assert_eq!(
             argv.words(),

src/commands/link.rs

@@ -13,47 +13,59 @@ use self::store::{Leads, Repo, ignored, leads, resolve, symlink_metadata_opt, tr
 use crate::cli::link as cli;
 use crate::ctx::Ctx;
 use crate::fsops::suffixed;
-use crate::output::{line, note, warning};
+use crate::output::{line, note, plural, warning};
 
 const BACKUP_SUFFIX: &str = ".ahab-bak";
 // where the link waits while the payload comes back out of the store
 const RESTORING_SUFFIX: &str = ".ahab-restoring";
 
-// move untracked paths out of the repo and symlink them back
+// one path's failure is its own; several are counted, so a run over a list says
-pub fn add(ctx: &Ctx, args: &cli::Add) -> Result<()> {
+// what happened to each and then how the run as a whole went. a single path has
-    let (paths, force) = (args.paths.as_slice(), args.force);
+// nothing to count, so its error is simply the command's
-    let repo = Repo::discover(ctx, args.store.root())?;
+fn each(paths: &[PathBuf], mut act: impl FnMut(&Path) -> Result<()>) -> Result<()> {
-    let report = Report::new(&repo);
+    if let [only] = paths {
-
+        return act(only);
-    if let [path] = paths {
-        return link_one(ctx, &repo, path, force, &report);
     }
 
     let mut failed = 0;
     for path in paths {
-        if let Err(e) = link_one(ctx, &repo, path, force, &report) {
+        if let Err(e) = act(path) {
             warning!("{e:#}");
             failed += 1;
         }
     }
 
-    if failed > 0 {
+    match failed {
-        return Err(anyhow!("{failed} of {} paths failed", paths.len()));
+        0 => Ok(()),
+        failed => Err(anyhow!("{failed} of {} paths failed", paths.len())),
     }
-    Ok(())
+}
+
+// move untracked paths out of the repo and symlink them back
+pub fn add(ctx: &Ctx, args: &cli::Add) -> Result<()> {
+    let repo = Repo::discover(ctx, args.store.root())?;
+    let report = Report::new(&repo);
+
+    each(&args.paths, |path| {
+        link_one(ctx, &repo, path, args.force, &report)
+    })
 }
 
 // move paths in the store back into the repo, the inverse of add
 pub fn restore(ctx: &Ctx, args: &cli::Restore) -> Result<()> {
-    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
     // cases are left here
-    let stored = match all {
+    let stored = match args.all {
         true => stored_paths(&repo, &repo.store)?,
-        false => paths.iter().cloned().map(|p| (p, Stored::Linked)).collect(),
+        false => args
+            .paths
+            .iter()
+            .cloned()
+            .map(|path| (path, Stored::Linked))
+            .collect(),
     };
 
     // only a linked path can be moved back; the store can hold orphans too
@@ -70,7 +82,7 @@ pub fn restore(ctx: &Ctx, args: &cli::Restore) -> Result<()> {
             n => note!(
                 ctx,
                 "nothing to restore: {n} path{} in the store {} not linked, see `ahab link list`",
-                if n == 1 { "" } else { "s" },
+                plural(n),
                 if n == 1 { "is" } else { "are" }
             ),
         }
@@ -82,26 +94,11 @@ pub fn restore(ctx: &Ctx, args: &cli::Restore) -> Result<()> {
         note!(
             ctx,
             "leaving {skipped} path{} that the store holds but nothing links to",
-            if skipped == 1 { "" } else { "s" }
+            plural(skipped)
         );
     }
 
-    if let [path] = linked.as_slice() {
+    each(&linked, |path| restore_one(ctx, &repo, path, &report))
-        return restore_one(ctx, &repo, path, &report);
-    }
-
-    let mut failed = 0;
-    for path in &linked {
-        if let Err(e) = restore_one(ctx, &repo, path, &report) {
-            warning!("{e:#}");
-            failed += 1;
-        }
-    }
-
-    if failed > 0 {
-        return Err(anyhow!("{failed} of {} paths failed", linked.len()));
-    }
-    Ok(())
 }
 
 fn restore_one(ctx: &Ctx, repo: &Repo, path: &Path, report: &Report) -> Result<()> {
@@ -247,12 +244,18 @@ pub fn list(ctx: &Ctx, args: &cli::List) -> Result<()> {
             Stored::Taken => "shadowed",
         };
 
-        line!("\t{:<11}{}", format!("{verb}:"), rel.display());
+        entry(verb, rel);
     }
 
     Ok(())
 }
 
+// one line of what the store did with a path, in the column width `status`
+// prints too: the two used to set it separately and had to be kept in step
+fn entry(verb: &str, path: &Path) {
+    line!("\t{:<11}{}", format!("{verb}:"), path.display());
+}
+
 struct Report {
     store: PathBuf,
     named: Cell<bool>,
@@ -272,7 +275,7 @@ impl Report {
             line!("store: {}", self.store.display());
         }
 
-        line!("\t{:<11}{}", format!("{verb}:"), path.display());
+        entry(verb, path);
     }
 }
 

src/commands/postgres.rs

@@ -16,7 +16,7 @@ use crate::cmd::{
 };
 use crate::ctx::Ctx;
 use crate::fsops::{create_private_new, suffixed};
-use crate::output::{note, warning};
+use crate::output::{note, plural, warning};
 
 // unique per run: docker cp will not copy a directory over an existing path, and
 // quietly leaves whatever was there for pg_restore to read instead
@@ -36,6 +36,13 @@ fn partial_path(file: &Path) -> PathBuf {
     suffixed(file, &format!(".{}.partial", std::process::id()))
 }
 
+// the database was dropped before the restore was attempted, so a restore that
+// failed leaves nothing behind: said the same way wherever it happens, since
+// what the user has to do about it does not depend on which tool it was
+fn left_empty(tool: &str) -> anyhow::Error {
+    anyhow::anyhow!("{tool} failed, the database is left empty")
+}
+
 // pg_dumpall recreates roles the cluster already has, so this is the expected
 // complaint rather than a failure
 fn is_existing_role_error(line: &str) -> bool {
@@ -75,17 +82,17 @@ fn restore_cluster(ctx: &Ctx, db: &Database, restore: &dyn Cmd, file: &Path) ->
         note!(
             ctx,
             "left {existing} existing role{} alone",
-            if existing == 1 { "" } else { "s" }
+            plural(existing)
         );
     }
 
     if !out.status.success() {
-        bail!("psql failed, the database is left empty");
+        return Err(left_empty("psql"));
     }
     if failed > 0 {
         bail!(
             "psql reported {failed} error{}, so the cluster restored only in part",
-            if failed == 1 { "" } else { "s" }
+            plural(failed)
         );
     }
 
@@ -99,7 +106,7 @@ fn gzip_kind(ctx: &Ctx, db: &Database, file: &Path) -> Result<Kind> {
     wait_until_ready(ctx, db)?;
 
     let out = Gunzip
-        .pipe(&Head::bytes(HEADER_LEN))
+        .pipe(&Head::bytes(HEADER_LEN))?
         // head closes the pipe once it has its bytes, which kills gunzip
         .allow_early_close()
         .in_container(&db.container)
@@ -257,14 +264,14 @@ fn restore_dump(
             .status(ctx)?;
 
         if !status.success() {
-            bail!("{tool} failed, the database is left empty");
+            return Err(left_empty(tool));
         }
 
         return Ok(());
     }
 
     let restore: Box<dyn Cmd> = match dump {
-        Dump::Gzip => Box::new(Gunzip.pipe(&*restore)),
+        Dump::Gzip => Box::new(Gunzip.pipe(&*restore)?),
         _ => restore,
     };
 
@@ -280,7 +287,7 @@ fn restore_dump(
         .stdin_from(ctx, file)?;
 
     if !status.success() {
-        bail!("{tool} failed, the database is left empty");
+        return Err(left_empty(tool));
     }
 
     Ok(())
@@ -328,7 +335,7 @@ pub fn dump(ctx: &Ctx, args: &cli::Dump) -> Result<()> {
 
     let dumped = if gzip {
         dumping
-            .pipe(&Gzip)
+            .pipe(&Gzip)?
             .in_container(&db.container)
             .stream_to(ctx, stdout)
     } else {

src/output.rs

@@ -107,3 +107,11 @@ pub(crate) fn delivered() -> Result<()> {
 }
 
 pub(crate) use {line, note, text, warning};
+
+// the `s` on a counted noun, so a message reads right for one and for many
+pub(crate) fn plural(n: usize) -> &'static str {
+    match n {
+        1 => "",
+        _ => "s",
+    }
+}