fix: report what happened instead of losing it

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<PathBuf>,
 
         /// Restore every path this repository has in the store

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<Command> {
         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;
         }
 

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)

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 `<name>.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<String> {
     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}");
+        }
+    }
+}

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) {
+    // clap requires one or the other and refuses both, so only the two real
-        (true, []) => stored_paths(&repo, &repo.store)?,
+    // cases are left here
-        (true, _) => bail!("--all restores everything, so it takes no paths"),
+    let stored = match all {
-        (false, []) => bail!("name a path to restore, or pass --all"),
+        true => stored_paths(&repo, &repo.store)?,
-        (false, paths) => paths.iter().cloned().map(|p| (p, Stored::Linked)).collect(),
+        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;
         }
     }

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<ExitCode> {
     let done = ExitCode::SUCCESS;
 
@@ -65,6 +74,16 @@ fn run(ctx: &Ctx, command: cli::Commands) -> Result<ExitCode> {
                 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)
                 }
             }?;

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<String> = OnceLock::new();
+
 pub(crate) fn write_line(args: Arguments) {
+    if writable() {
         finish(writeln!(io::stdout(), "{args}"));
     }
+}
 
 pub(crate) fn write_text(args: Arguments) {
+    if writable() {
         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);
 }
+
+// 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};