From c55273d80c60f722d1360b7c3de1ef71d5d12aa8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matej=20Jane=C5=BEi=C4=8D?= Date: Wed, 9 Sep 2026 12:42:23 +0000 Subject: [PATCH] refactor: one helper each for the repeated blocks --- src/cmd/argv.rs | 27 +++++++++++++-- src/cmd/mod.rs | 8 ++--- src/cmd/shell.rs | 19 ++++++---- src/commands/link.rs | 75 +++++++++++++++++++++------------------- src/commands/postgres.rs | 25 +++++++++----- src/output.rs | 8 +++++ 6 files changed, 104 insertions(+), 58 deletions(-) diff --git a/src/cmd/argv.rs b/src/cmd/argv.rs index ed44190..ab9e5bc 100644 --- a/src/cmd/argv.rs +++ b/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 - // shell. lossy: this is for reading, and for scripts built only out of the - // ascii and compose-derived words that reach a pipeline + // for reading: the -v echo, the dry run's plan, and the command named in an + // error. lossy, because a message about a path is still worth printing pub fn quoted(&self) -> String { let words: Vec = 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 { + 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(()); diff --git a/src/cmd/mod.rs b/src/cmd/mod.rs index 43c13f1..5ac7f72 100644 --- a/src/cmd/mod.rs +++ b/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 { - self.argv().quoted() + fn shell(&self) -> Result { + self.argv().for_shell() } // adapters - fn pipe(&self, next: &dyn Cmd) -> Pipeline { - Pipeline::starting(self.shell()).pipe(next) + fn pipe(&self, next: &dyn Cmd) -> Result { + Pipeline::starting(self.shell()?).pipe(next) } fn in_container(&self, container: &str) -> Exec { diff --git a/src/cmd/shell.rs b/src/cmd/shell.rs index 850a590..3857214 100644 --- a/src/cmd/shell.rs +++ b/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 { - self.stages.push(next.shell()); - self + pub fn pipe(mut self, next: &dyn Cmd) -> Result { + self.stages.push(next.shell()?); + 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(), diff --git a/src/commands/link.rs b/src/commands/link.rs index 0dea1fa..2e20f49 100644 --- a/src/commands/link.rs +++ b/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 -pub fn add(ctx: &Ctx, args: &cli::Add) -> Result<()> { - 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 { - return link_one(ctx, &repo, path, force, &report); +// one path's failure is its own; several are counted, so a run over a list says +// what happened to each and then how the run as a whole went. a single path has +// nothing to count, so its error is simply the command's +fn each(paths: &[PathBuf], mut act: impl FnMut(&Path) -> Result<()>) -> Result<()> { + if let [only] = paths { + return act(only); } 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 { - return Err(anyhow!("{failed} of {} paths failed", paths.len())); + match failed { + 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() { - 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(()) + each(&linked, |path| restore_one(ctx, &repo, path, &report)) } 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, @@ -272,7 +275,7 @@ impl Report { line!("store: {}", self.store.display()); } - line!("\t{:<11}{}", format!("{verb}:"), path.display()); + entry(verb, path); } } diff --git a/src/commands/postgres.rs b/src/commands/postgres.rs index 1d29587..79af0eb 100644 --- a/src/commands/postgres.rs +++ b/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 { 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 = 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 { diff --git a/src/output.rs b/src/output.rs index 14086f7..237cdec 100644 --- a/src/output.rs +++ b/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", + } +}