diff --git a/src/cli/link.rs b/src/cli/link.rs index aa05edf..1d3f1c8 100644 --- a/src/cli/link.rs +++ b/src/cli/link.rs @@ -20,6 +20,10 @@ pub enum Link { /// Move paths in the store back into the repository 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, /// Restore every path this repository has in the store diff --git a/src/cmd/argv.rs b/src/cmd/argv.rs index e6d58b7..06c1573 100644 --- a/src/cmd/argv.rs +++ b/src/cmd/argv.rs @@ -9,6 +9,7 @@ use std::{ use anyhow::{Context, Result, anyhow}; use crate::ctx::Ctx; +use crate::output::write_err; // a program and its arguments: the only thing in ahab that knows what argv looks like #[derive(Default, Clone)] @@ -138,7 +139,7 @@ impl Argv { fn command(&self, ctx: &Ctx) -> Result { if ctx.verbose { - eprintln!("running `{self}`"); + write_err(format_args!("running `{self}`")); } let (program, rest) = self.0.split_first().context("empty command")?; @@ -150,7 +151,8 @@ impl Argv { fn skipped(&self, ctx: &Ctx) -> bool { if ctx.dry_run { - eprintln!("would run `{self}`"); + // the plan is what a dry run was asked for, so --quiet keeps it + write_err(format_args!("would run `{self}`")); return true; } diff --git a/src/cmd/compose.rs b/src/cmd/compose.rs index d06313f..2b7daae 100644 --- a/src/cmd/compose.rs +++ b/src/cmd/compose.rs @@ -15,6 +15,7 @@ fn compose(quiet: bool) -> Argv { pub struct Run { service: String, inner: Argv, + quiet: bool, } impl Run { @@ -22,13 +23,21 @@ impl Run { Self { service: service.to_string(), inner, + quiet: false, } } + + // compose narrates the container it creates before handing over, which is + // progress along the way rather than what was asked for + pub fn quiet(mut self, quiet: bool) -> Self { + self.quiet = quiet; + self + } } impl Cmd for Run { fn argv(&self) -> Argv { - compose(false) + compose(self.quiet) .arg("run") .arg("--rm") .arg(&self.service) diff --git a/src/commands/django.rs b/src/commands/django.rs index 0c241e2..e9795ca 100644 --- a/src/commands/django.rs +++ b/src/commands/django.rs @@ -1,4 +1,4 @@ -use std::path::{Path, PathBuf}; +use std::path::Path; use anyhow::{Result, anyhow}; @@ -16,9 +16,20 @@ class Command(BaseCommand): "#; -pub fn make_command(ctx: &Ctx, app: &PathBuf, name: &str) -> Result<()> { +pub fn make_command(ctx: &Ctx, app: &Path, name: &str) -> Result<()> { let app_name = app.to_string_lossy(); - let app_dir = Path::new(&app); + let app_dir = app; + + // it becomes `.py` under the app, and django imports it by this name, + // so anything that is not an identifier would land the file somewhere else + // or somewhere django will never look for it + if !is_module_name(name) { + return Err(anyhow!( + "{name:?} is not a usable command name; \ + django imports it as a python module, so it can hold only \ + letters, digits and underscores" + )); + } if !app_dir.is_dir() { return Err(anyhow!("directory {app_name} does not exist")); @@ -54,16 +65,30 @@ pub fn make_command(ctx: &Ctx, app: &PathBuf, name: &str) -> Result<()> { Ok(()) } +fn is_module_name(name: &str) -> bool { + !name.is_empty() + && !name.starts_with(|c: char| c.is_ascii_digit()) + && name.chars().all(|c| c.is_ascii_alphanumeric() || c == '_') +} + pub fn bash(ctx: &Ctx) -> Result<()> { - Bash.in_service(&service(ctx)?).replace(ctx) + Bash.in_service(&service(ctx)?) + .quiet(ctx.quiet) + .replace(ctx) } pub fn run(ctx: &Ctx, rest: &[String]) -> Result<()> { - Words::new(rest).in_service(&service(ctx)?).replace(ctx) + Words::new(rest) + .in_service(&service(ctx)?) + .quiet(ctx.quiet) + .replace(ctx) } pub fn manage(ctx: &Ctx, rest: &[String]) -> Result<()> { - Manage::new(rest).in_service(&service(ctx)?).replace(ctx) + Manage::new(rest) + .in_service(&service(ctx)?) + .quiet(ctx.quiet) + .replace(ctx) } // shortcuts @@ -85,3 +110,20 @@ pub fn shell(ctx: &Ctx) -> Result<()> { fn service(ctx: &Ctx) -> Result { Project::resolve(ctx)?.django() } + +#[cfg(test)] +mod tests { + use super::is_module_name; + + #[test] + fn a_command_name_has_to_be_a_python_module_name() { + for name in ["report", "send_mail", "_private", "sync2"] { + assert!(is_module_name(name), "should be usable: {name}"); + } + + // a path would put the file somewhere other than the app + for name in ["../../../etc/cron.d/x", "a/b", "with space", "dash-ed", ""] { + assert!(!is_module_name(name), "should be refused: {name}"); + } + } +} diff --git a/src/commands/link.rs b/src/commands/link.rs index 7b1b7b6..c5b85be 100644 --- a/src/commands/link.rs +++ b/src/commands/link.rs @@ -28,7 +28,7 @@ pub fn add(ctx: &Ctx, paths: &[PathBuf], force: bool, store: Option<&Path>) -> R let mut failed = 0; for path in paths { if let Err(e) = link_one(ctx, &repo, path, force, &report) { - note!(ctx, "error: {e:#}"); + warning!("{e:#}"); failed += 1; } } @@ -44,11 +44,11 @@ pub fn restore(ctx: &Ctx, paths: &[PathBuf], all: bool, store: Option<&Path>) -> let repo = Repo::discover(ctx, store)?; let report = Report::new(&repo); - let stored = match (all, paths) { - (true, []) => stored_paths(&repo, &repo.store)?, - (true, _) => bail!("--all restores everything, so it takes no paths"), - (false, []) => bail!("name a path to restore, or pass --all"), - (false, paths) => paths.iter().cloned().map(|p| (p, Stored::Linked)).collect(), + // clap requires one or the other and refuses both, so only the two real + // cases are left here + let stored = match all { + true => stored_paths(&repo, &repo.store)?, + false => paths.iter().cloned().map(|p| (p, Stored::Linked)).collect(), }; // only a linked path can be moved back; the store can hold orphans too @@ -88,7 +88,7 @@ pub fn restore(ctx: &Ctx, paths: &[PathBuf], all: bool, store: Option<&Path>) -> let mut failed = 0; for path in &linked { if let Err(e) = restore_one(ctx, &repo, path, &report) { - note!(ctx, "error: {e:#}"); + warning!("{e:#}"); failed += 1; } } diff --git a/src/main.rs b/src/main.rs index 1ab7e53..b0795c7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -9,10 +9,10 @@ mod output; mod project; use anyhow::Result; -use clap::Parser; +use clap::{CommandFactory, Parser}; use crate::ctx::Ctx; -use crate::output::note; +use crate::output::{note, write_err}; // 0 ran with nothing to report, 1 could not finish, 2 bad arguments, 4 found something const FINDINGS: u8 = 4; @@ -32,15 +32,24 @@ fn main() -> ExitCode { note!(ctx, "dry run, nothing will be changed"); } - match run(&ctx, args.command) { + // the command's own verdict first, then whether it managed to say it + match run(&ctx, args.command).and_then(|code| output::delivered().map(|()| code)) { Ok(code) => code, Err(e) => { - eprintln!("Error: {e:#}"); + write_err(format_args!("Error: {e:#}")); ExitCode::FAILURE } } } +// reported the way clap reports its own argument errors, which is what makes it +// exit 2 rather than 1 +fn usage(message: &str) -> ! { + cli::Ahab::command() + .error(clap::error::ErrorKind::ArgumentConflict, message) + .exit() +} + fn run(ctx: &Ctx, command: cli::Commands) -> Result { let done = ExitCode::SUCCESS; @@ -65,6 +74,16 @@ fn run(ctx: &Ctx, command: cli::Commands) -> Result { cli::Postgres::Import { path } => commands::postgres::import(ctx, &path), cli::Postgres::Psql { rest } => commands::postgres::psql(ctx, &rest), cli::Postgres::Dump { path, format, gzip } => { + // 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 { + usage( + "a directory dump is a directory of already \ + compressed files, not a stream", + ); + } + commands::postgres::dump(ctx, &path, format, gzip) } }?; diff --git a/src/output.rs b/src/output.rs index 10448f1..57710d0 100644 --- a/src/output.rs +++ b/src/output.rs @@ -1,18 +1,24 @@ use std::fmt::Arguments; use std::io::{self, Write}; +use std::sync::OnceLock; +use std::sync::atomic::{AtomicU8, Ordering}; + +use anyhow::{Result, anyhow}; // progress, on stderr and only when it was asked for macro_rules! note { ($ctx:expr, $($arg:tt)*) => { if !$ctx.quiet { - eprintln!($($arg)*) + $crate::output::write_err(format_args!($($arg)*)) } }; } // not progress: it speaks about the result, so --quiet keeps it macro_rules! warning { - ($($arg:tt)*) => { eprintln!("warning: {}", format_args!($($arg)*)) }; + ($($arg:tt)*) => { + $crate::output::write_err(format_args!("warning: {}", format_args!($($arg)*))) + }; } // what a command was asked for, on stdout @@ -25,19 +31,72 @@ macro_rules! text { ($($arg:tt)*) => { $crate::output::write_text(format_args!($($arg)*)) }; } +// stdout is open, or it is not and why +const OPEN: u8 = 0; +// the reader left: nothing is wrong, there is just nowhere to write +const CLOSED: u8 = 1; +// the write itself failed, so what was asked for was never delivered +const FAILED: u8 = 2; + +static STDOUT: AtomicU8 = AtomicU8::new(OPEN); +static REASON: OnceLock = OnceLock::new(); + pub(crate) fn write_line(args: Arguments) { - finish(writeln!(io::stdout(), "{args}")); -} - -pub(crate) fn write_text(args: Arguments) { - finish(write!(io::stdout(), "{args}")); -} - -// a reader leaving early ends the pipe; println! would panic instead -fn finish(written: io::Result<()>) { - if written.is_err() { - std::process::exit(0); + if writable() { + finish(writeln!(io::stdout(), "{args}")); } } +pub(crate) fn write_text(args: Arguments) { + if writable() { + finish(write!(io::stdout(), "{args}")); + } +} + +// stderr is commentary about the work, not the work itself. there is nowhere to +// report that it could not be written, so the error goes nowhere -- eprintln! +// panics instead, which turns a closed pipe into a crash +pub(crate) fn write_err(args: Arguments) { + let _ = writeln!(io::stderr(), "{args}"); +} + +fn writable() -> bool { + STDOUT.load(Ordering::Relaxed) == OPEN +} + +// a failed write must not end the process: a command halfway through moving +// files would leave the rest undone and still report the success it had planned +// on. writing stops, the command runs to its end, and main asks how it went +fn finish(written: io::Result<()>) { + let Err(e) = written else { + return; + }; + + match e.kind() { + io::ErrorKind::BrokenPipe => STDOUT.store(CLOSED, Ordering::Relaxed), + _ => { + STDOUT.store(FAILED, Ordering::Relaxed); + let _ = REASON.set(e.to_string()); + } + } +} + +// whether everything a command was asked for actually reached stdout. the +// buffered writes text! makes are flushed here rather than by the runtime after +// main returns, which discards the error and truncates the output in silence +pub(crate) fn delivered() -> Result<()> { + if writable() { + finish(io::stdout().flush()); + } + + if STDOUT.load(Ordering::Relaxed) != FAILED { + return Ok(()); + } + + Err(match REASON.get() { + Some(reason) => anyhow!("writing to stdout failed: {reason}"), + None => anyhow!("writing to stdout failed"), + }) +} + pub(crate) use {line, note, text, warning};