fix: dumps and imports that quietly did the wrong thing

src/cmd/argv.rs

@@ -2,7 +2,7 @@ use std::{
     ffi::{OsStr, OsString},
     fmt::Display,
     fs::File,
-    os::unix::process::CommandExt,
+    os::unix::process::{CommandExt, ExitStatusExt},
     path::Path,
     process::{Command, ExitStatus, Output, Stdio},
 };
@@ -16,6 +16,12 @@ use crate::output::write_err;
 #[derive(Default, Clone)]
 pub struct Argv(Vec<OsString>);
 
+// a dry run spawned nothing, so there is no status; nothing ran and nothing
+// failed, which is what every caller checking one of these wants to hear
+fn planned() -> ExitStatus {
+    ExitStatus::from_raw(0)
+}
+
 impl Display for Argv {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         f.write_str(&self.quoted())
@@ -135,10 +141,18 @@ impl Argv {
 
     // the status rather than an error, for callers with something better to say
     pub fn status(&self, ctx: &Ctx) -> Result<ExitStatus> {
+        if self.skipped(ctx) {
+            return Ok(planned());
+        }
+
         Ok(self.command(ctx)?.spawn()?.wait()?)
     }
 
     pub fn stdin_from(&self, ctx: &Ctx, input: &Path) -> Result<ExitStatus> {
+        if self.skipped(ctx) {
+            return Ok(planned());
+        }
+
         let stdin = self.opened(input)?;
 
         Ok(self.command(ctx)?.stdin(stdin).spawn()?.wait()?)
@@ -146,6 +160,22 @@ impl Argv {
 
     // both streams held back, for callers that read the command's complaints
     pub fn stdin_from_captured(&self, ctx: &Ctx, input: &Path) -> Result<Output> {
+        if self.skipped(ctx) {
+            return Ok(Output {
+                status: planned(),
+                stdout: Vec::new(),
+                stderr: Vec::new(),
+            });
+        }
+
+        let stdin = self.opened(input)?;
+
+        Ok(self.command(ctx)?.stdin(stdin).output()?)
+    }
+
+    // a probe that reads its input: like capture, it is asking rather than
+    // changing, so a dry run gets the real answer and plans with it
+    pub fn probe_with_stdin(&self, ctx: &Ctx, input: &Path) -> Result<Output> {
         let stdin = self.opened(input)?;
 
         Ok(self.command(ctx)?.stdin(stdin).output()?)

src/cmd/mod.rs

@@ -80,6 +80,10 @@ pub trait Cmd {
         self.argv().stdin_from_captured(ctx, input)
     }
 
+    fn probe_with_stdin(&self, ctx: &Ctx, input: &Path) -> Result<Output> {
+        self.argv().probe_with_stdin(ctx, input)
+    }
+
     fn quietly_succeeds(&self, ctx: &Ctx) -> Result<bool> {
         self.argv().quietly_succeeds(ctx)
     }

src/cmd/postgres.rs

@@ -25,6 +25,9 @@ impl Cmd for DropDb<'_> {
         Argv::new("dropdb")
             .flag("--username", self.username)
             .arg("--if-exists")
+            // a database name is a positional, and getopt would read one
+            // beginning with a dash as an option instead
+            .arg("--")
             .arg(self.dbname)
     }
 }
@@ -42,6 +45,7 @@ impl Cmd for CreateDb<'_> {
             .flag("--username", self.username)
             .flag("--encoding", "utf8")
             .flag("--template", "template0")
+            .arg("--")
             .arg(self.dbname)
     }
 }
@@ -76,7 +80,7 @@ impl Cmd for PgRestore<'_> {
             .flag("--dbname", self.dbname);
 
         match self.from {
-            Some(path) => argv.arg(path),
+            Some(path) => argv.arg("--").arg(path),
             None => argv,
         }
     }
@@ -179,6 +183,7 @@ impl Cmd for PgDump<'_> {
             Some(path) => argv.flag("--file", path),
             None => argv,
         }
+        .arg("--")
         .arg(self.dbname)
     }
 }
@@ -208,7 +213,7 @@ mod tests {
             }
             .argv()
             .quoted(),
-            "dropdb --username u --if-exists db"
+            "dropdb --username u --if-exists -- db"
         );
     }
 
@@ -216,11 +221,11 @@ mod tests {
     fn a_directory_dump_names_the_file_it_writes() {
         assert_eq!(
             PgDump::new("u", "db", "d").to("/tmp/dump").argv().quoted(),
-            "pg_dump --username u --format d --file /tmp/dump db"
+            "pg_dump --username u --format d --file /tmp/dump -- db"
         );
         assert_eq!(
             PgDump::new("u", "db", "c").argv().quoted(),
-            "pg_dump --username u --format c db"
+            "pg_dump --username u --format c -- db"
         );
     }
 

src/commands/postgres.rs

@@ -1,9 +1,8 @@
 mod server;
 mod shape;
 
-use fs_err::File;
 use std::io::{self, IsTerminal, Write};
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::process::Stdio;
 
 use anyhow::{Context, Result, bail};
@@ -16,15 +15,29 @@ use crate::cmd::{
     Stop, Up,
 };
 use crate::ctx::Ctx;
-use crate::fsops::{remove_file, rename, suffixed};
+use crate::fsops::{create_private_new, remove_file, rename, suffixed};
-use crate::output::note;
+use crate::output::{note, 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
 fn remote_dump() -> String {
-    format!("/tmp/ahab-dump-{}", std::process::id())
+    // the container's /tmp is shared with whatever else runs in it, and a name
+    // that can be worked out in advance is one a symlink can be planted at
+    let spun = std::time::SystemTime::now()
+        .duration_since(std::time::UNIX_EPOCH)
+        .map_or(0, |since| since.subsec_nanos());
+
+    format!("/tmp/ahab-dump-{}-{spun:09}", std::process::id())
 }
 
+// beside the target but unique to this run: a fixed name is a path a second
+// dump would share, and one a symlink can be planted at ahead of time
+fn partial_path(file: &Path) -> PathBuf {
+    suffixed(file, &format!(".{}.partial", std::process::id()))
+}
+
+// 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 {
     line.starts_with("ERROR:") && line.contains("role \"") && line.ends_with("already exists")
 }
@@ -39,13 +52,23 @@ fn restore_cluster(ctx: &Ctx, db: &Database, restore: &dyn Cmd, file: &Path) ->
     io::stdout().write_all(&out.stdout).ok();
 
     let mut existing = 0;
+    let mut failed = 0;
+
     for line in String::from_utf8_lossy(&out.stderr).lines() {
         if is_existing_role_error(line) {
             existing += 1;
             continue;
         }
 
-        note!(ctx, "{line}");
+        // this psql runs without ON_ERROR_STOP, so it exits 0 whatever the sql
+        // did: these lines are the only account of what happened, which makes
+        // them the result rather than progress, and --quiet has to keep them
+        if line.starts_with("ERROR:") {
+            failed += 1;
+            warning!("{line}");
+        } else {
+            note!(ctx, "{line}");
+        }
     }
 
     if existing > 0 {
@@ -59,17 +82,82 @@ fn restore_cluster(ctx: &Ctx, db: &Database, restore: &dyn Cmd, file: &Path) ->
     if !out.status.success() {
         bail!("psql failed, the database is left empty");
     }
+    if failed > 0 {
+        bail!(
+            "psql reported {failed} error{}, so the cluster restored only in part",
+            if failed == 1 { "" } else { "s" }
+        );
+    }
 
     Ok(())
 }
 
+// which shape is inside the compression, read through the container's own gunzip
+// rather than assuming the host has one. this only reads, so it is a probe and
+// runs even in a dry run, where the answer decides what the plan says
+fn gzip_kind(ctx: &Ctx, db: &Database, file: &Path) -> Result<Kind> {
+    wait_until_ready(ctx, db)?;
+
+    let out = Gunzip
+        .pipe(&Head::bytes(HEADER_LEN))
+        // head closes the pipe once it has its bytes, which kills gunzip
+        .allow_early_close()
+        .in_container(&db.container)
+        .interactive()
+        .probe_with_stdin(ctx, file)
+        .context("reading the compressed dump's header")?;
+
+    // head exits 0 whatever gunzip did, so an empty header is the only
+    // sign that decompression failed
+    if !out.status.success() || out.stdout.is_empty() {
+        let reason = String::from_utf8_lossy(&out.stderr);
+        let reason = reason.trim();
+
+        bail!(
+            "could not decompress {}: {}",
+            file.display(),
+            if reason.is_empty() {
+                "no data came out"
+            } else {
+                reason
+            }
+        );
+    }
+
+    Ok(Kind::of(&out.stdout))
+}
+
 pub fn import(ctx: &Ctx, file: &Path) -> Result<()> {
     let dump = Dump::of(file)?;
     let db = Database::resolve(ctx)?;
 
+    if matches!(dump, Dump::Directory) {
+        local_path_for_docker(file)?;
+    }
+
     note!(ctx, "stopping all containers");
     Stop { quiet: ctx.quiet }.run(ctx)?;
 
+    // everything from here runs with the project down, so an error leaves it
+    // that way and the user has no reason to guess as much
+    imported(ctx, &db, &dump, file)
+        .map_err(|e| e.context("the project is left stopped, `docker compose up` starts it again"))
+}
+
+// docker cp reads `container:path`, splitting on the first colon, so a local
+// path holding one is read as a container name and something else entirely
+fn local_path_for_docker(file: &Path) -> Result<()> {
+    match file.to_string_lossy().contains(':') {
+        true => bail!(
+            "{} has a colon in it, which docker cp reads as a container name; \
+             rename it to copy it in or out",
+            file.display()
+        ),
+        false => Ok(()),
+    }
+}
+
+fn imported(ctx: &Ctx, db: &Database, dump: &Dump, file: &Path) -> Result<()> {
     note!(ctx, "starting db container");
     Start {
         service: &db.service,
@@ -81,42 +169,23 @@ pub fn import(ctx: &Ctx, file: &Path) -> Result<()> {
 
     // a directory cannot be streamed, so it is the one shape that gets copied in
     if matches!(dump, Dump::Directory) {
-        when_ready(ctx, &db, &Cp::into_container(file, &db.container, &remote))?;
+        when_ready(ctx, db, &Cp::into_container(file, &db.container, &remote))?;
     }
 
     let kind = match &dump {
         Dump::Directory => Kind::Archive,
         Dump::Header(header) => Kind::of(header),
-        Dump::Gzip => {
+        Dump::Gzip => match gzip_kind(ctx, db, file) {
-            wait_until_ready(ctx, &db)?;
+            Ok(kind) => kind,
-            let out = Gunzip
+            // looking inside needs a container to decompress with, and a dry run
-                .pipe(&Head::bytes(HEADER_LEN))
+            // is worth printing with the project down, which is when it is most
-                // head closes the pipe once it has its bytes, which kills gunzip
+            // likely to be asked for
-                .allow_early_close()
+            Err(e) if ctx.dry_run => {
-                .in_container(&db.container)
+                note!(ctx, "planning for a single database dump: {e:#}");
-                .interactive()
+                Kind::Sql
-                .stdin_from_captured(ctx, file)
-                .context("reading the compressed dump's header")?;
-
-            // head exits 0 whatever gunzip did, so an empty header is the only
-            // sign that decompression failed
-            if !out.status.success() || out.stdout.is_empty() {
-                let reason = String::from_utf8_lossy(&out.stderr);
-                let reason = reason.trim();
-
-                bail!(
-                    "could not decompress {}: {}",
-                    file.display(),
-                    if reason.is_empty() {
-                        "no data came out"
-                    } else {
-                        reason
-                    }
-                );
             }
-
+            Err(e) => return Err(e),
-            Kind::of(&out.stdout)
+        },
-        }
     };
 
     let restore = db.restore_with(kind);
@@ -126,7 +195,7 @@ pub fn import(ctx: &Ctx, file: &Path) -> Result<()> {
 
     when_ready(
         ctx,
-        &db,
+        db,
         &DropDb {
             username: &db.user,
             dbname: &db.name,
@@ -139,7 +208,7 @@ pub fn import(ctx: &Ctx, file: &Path) -> Result<()> {
     if kind != Kind::Cluster {
         when_ready(
             ctx,
-            &db,
+            db,
             &CreateDb {
                 username: &db.user,
                 dbname: &db.name,
@@ -148,48 +217,16 @@ pub fn import(ctx: &Ctx, file: &Path) -> Result<()> {
         )?;
     }
 
-    wait_until_ready(ctx, &db)?;
+    wait_until_ready(ctx, db)?;
-    if ctx.dry_run {
+    let restored = restore_dump(ctx, db, dump, kind, file, &remote, &tool, restore);
-        note!(ctx, "would restore with {tool}");
-    } else {
-        // a directory dump was copied in whole, so pg_restore reads it from the
-        // container; every other shape is fed in on stdin, through gunzip when it
-        // arrives compressed
-        if matches!(dump, Dump::Directory) {
-            let status = PgRestore::new(&db.user, &db.name)
-                .from(&remote)
-                .in_container(&db.container)
-                .status(ctx)?;
-
-            if !status.success() {
-                bail!("{tool} failed, the database is left empty");
-            }
-        } else {
-            let restore: Box<dyn Cmd> = match dump {
-                Dump::Gzip => Box::new(Gunzip.pipe(&*restore)),
-                _ => restore,
-            };
-
-            if kind == Kind::Cluster {
-                // psql's output is read rather than streamed here, to keep the
-                // expected role errors out of the way
-                restore_cluster(ctx, &db, &*restore, file)?;
-            } else {
-                let status = restore
-                    .in_container(&db.container)
-                    .interactive()
-                    .stdin_from(ctx, file)?;
-
-                if !status.success() {
-                    bail!("{tool} failed, the database is left empty");
-                }
-            }
-        }
-    }
 
+    // the copy inside the container holds the whole database, and its /tmp
+    // outlives the command: it goes whether or not the restore worked, which is
+    // exactly when it used to be left behind
     if matches!(dump, Dump::Directory) {
         let _ = Rm::recursive(&remote).in_container(&db.container).run(ctx);
     }
+    restored?;
 
     note!(ctx, "restarting containers");
     Stop { quiet: ctx.quiet }.run(ctx)?;
@@ -198,6 +235,61 @@ pub fn import(ctx: &Ctx, file: &Path) -> Result<()> {
     Ok(())
 }
 
+#[allow(clippy::too_many_arguments)]
+fn restore_dump(
+    ctx: &Ctx,
+    db: &Database,
+    dump: &Dump,
+    kind: Kind,
+    file: &Path,
+    remote: &str,
+    tool: &str,
+    restore: Box<dyn Cmd + '_>,
+) -> Result<()> {
+    if ctx.dry_run {
+        note!(ctx, "would restore with {tool}");
+        return Ok(());
+    }
+
+    // a directory dump was copied in whole, so pg_restore reads it from the
+    // container; every other shape is fed in on stdin, through gunzip when it
+    // arrives compressed
+    if matches!(dump, Dump::Directory) {
+        let status = PgRestore::new(&db.user, &db.name)
+            .from(remote)
+            .in_container(&db.container)
+            .status(ctx)?;
+
+        if !status.success() {
+            bail!("{tool} failed, the database is left empty");
+        }
+
+        return Ok(());
+    }
+
+    let restore: Box<dyn Cmd> = match dump {
+        Dump::Gzip => Box::new(Gunzip.pipe(&*restore)),
+        _ => restore,
+    };
+
+    if kind == Kind::Cluster {
+        // psql's output is read rather than streamed here, to keep the
+        // expected role errors out of the way
+        return restore_cluster(ctx, db, &*restore, file);
+    }
+
+    let status = restore
+        .in_container(&db.container)
+        .interactive()
+        .stdin_from(ctx, file)?;
+
+    if !status.success() {
+        bail!("{tool} failed, the database is left empty");
+    }
+
+    Ok(())
+}
+
 pub fn psql(ctx: &Ctx, rest: &[String]) -> Result<()> {
     let db = Database::resolve(ctx)?;
 
@@ -225,12 +317,12 @@ pub fn dump(ctx: &Ctx, file: &Path, format: Format, gzip: bool) -> Result<()> {
 
     // written beside the target and renamed once the dump succeeds, so a failure
     // cannot destroy the dump that is already there
-    let partial = suffixed(file, ".partial");
+    let partial = partial_path(file);
     // a dry run produces no dump, so it must not lay a hand on the target either
     let stdout = if ctx.dry_run {
         Stdio::null()
     } else {
-        Stdio::from(std::fs::File::from(File::create(&partial)?))
+        Stdio::from(create_private_new(&partial)?)
     };
 
     let dumping = dump_command(&db, format);
@@ -265,7 +357,9 @@ fn dump_command(db: &Database, format: Format) -> Box<dyn Cmd + '_> {
 // pg_dump writes a directory format dump itself rather than to stdout, so it lands
 // in the container and comes back with docker cp
 fn dump_directory(ctx: &Ctx, db: &Database, target: &Path) -> Result<()> {
-    if target.exists() {
+    // symlink_metadata rather than exists(), which follows the link and so is
+    // false for a dangling one: docker cp would then write through it
+    if std::fs::symlink_metadata(target).is_ok() {
         bail!(
             "{} already exists; a directory dump will not be written over it",
             target.display()

src/commands/postgres/server.rs

@@ -1,7 +1,7 @@
 use std::thread;
 use std::time::{Duration, Instant};
 
-use anyhow::{Result, anyhow, bail};
+use anyhow::{Result, bail};
 
 use super::shape::Kind;
 use crate::cmd::{Cmd, PgIsReady, PgRestore, Ps, Psql};
@@ -10,6 +10,8 @@ use crate::project::Project;
 
 const READY_TIMEOUT: Duration = Duration::from_secs(60);
 const POLL_INTERVAL: Duration = Duration::from_secs(1);
+// stands in for an id a dry run has no running container to look up
+const PLANNED_CONTAINER: &str = "<db container>";
 
 pub(super) struct Database {
     pub(super) service: String,
@@ -22,13 +24,24 @@ impl Database {
     pub(super) fn resolve(ctx: &Ctx) -> Result<Self> {
         let compose = Project::resolve(ctx)?;
         let service = compose.postgres()?;
-        let (user, name) = compose.postgres_credentials(&service);
+        let (user, name) = compose.postgres_credentials(&service)?;
 
-        let container = Ps::id_of(&service).capture(ctx)?.trim().to_string();
+        let listed = Ps::id_of(&service).capture(ctx)?;
+        let mut ids = listed.lines().map(str::trim).filter(|id| !id.is_empty());
 
-        if container.is_empty() {
+        let container = match (ids.next(), ids.next()) {
-            return Err(anyhow!("service {service} has no running container"));
+            (Some(id), None) => id.to_string(),
-        }
+            // one id per line: a scaled service has several, and the whole
+            // listing would go to docker exec as though it were a single id
+            (Some(_), Some(_)) => bail!(
+                "service {service} has more than one container running; \
+                 scale it to one first"
+            ),
+            // a dry run only prints a plan, and it is worth printing with the
+            // project down, which is when it is most likely to be asked for
+            (None, _) if ctx.dry_run => PLANNED_CONTAINER.to_string(),
+            (None, _) => bail!("service {service} has no running container"),
+        };
 
         Ok(Self {
             service,

src/commands/postgres/shape.rs

@@ -55,14 +55,14 @@ impl Kind {
     }
 }
 
-// pg_dumpall recreates roles the cluster already has, so this is the expected
-
 fn read_header(path: &Path) -> Result<Vec<u8>> {
-    let mut file = File::open(path)?;
+    let file = File::open(path)?;
-    let mut header = vec![0; HEADER_LEN];
+    let mut header = Vec::with_capacity(HEADER_LEN);
 
-    let read = file.read(&mut header)?;
+    // read fills what it can rather than what was asked for, and a header that
-    header.truncate(read);
+    // arrives in more than one piece would lose the cluster marker sitting a few
+    // bytes in, leaving a whole-cluster dump looking like a single database
+    file.take(HEADER_LEN as u64).read_to_end(&mut header)?;
 
     Ok(header)
 }

src/commands/status.rs

@@ -69,14 +69,20 @@ fn role(ctx: &Ctx, role: &str, detected: Result<String>, project: &Project) {
     }
 
     if role == "postgres" {
-        let (user, database) = project.postgres_credentials(&service);
+        // status reports what it cannot work out rather than stopping, so a name
-        let source = |key: &str| match project.env(&service, key) {
+        // the postgres tools would refuse is something to say, not to fail on
-            Some(_) => "",
+        match project.postgres_credentials(&service) {
-            None => " (default, nothing in the environment)",
+            Ok((user, database)) => {
-        };
+                let source = |key: &str| match project.env(&service, key) {
+                    Some(_) => "",
+                    None => " (default, nothing in the environment)",
+                };
 
-        line!("\tuser:      {user}{}", source("POSTGRES_USER"));
+                line!("\tuser:      {user}{}", source("POSTGRES_USER"));
-        line!("\tdatabase:  {database}{}", source("POSTGRES_DB"));
+                line!("\tdatabase:  {database}{}", source("POSTGRES_DB"));
+            }
+            Err(e) => line!("\tcredentials: {e:#}"),
+        }
     }
 }
 

src/fsops.rs

@@ -1,5 +1,5 @@
 use std::io::Write;
-use std::os::unix::fs::DirBuilderExt;
+use std::os::unix::fs::{DirBuilderExt, OpenOptionsExt};
 use std::path::{Path, PathBuf};
 
 use anyhow::{Context, Result, anyhow};
@@ -74,6 +74,19 @@ pub fn place_link(ctx: &Ctx, link_path: &Path, target: &Path) -> Result<()> {
     Ok(fs::rename(&tmp, link_path)?)
 }
 
+// 0600 and only where nothing is yet. a dump is the whole database, and
+// pg_dumpall's is every role's password hash, so it is not the owner's to share
+// by default; create_new also refuses to follow a symlink planted at the path,
+// which File::create would open and truncate
+pub fn create_private_new(path: &Path) -> Result<std::fs::File> {
+    std::fs::OpenOptions::new()
+        .write(true)
+        .create_new(true)
+        .mode(0o600)
+        .open(path)
+        .with_context(|| format!("creating {}", path.display()))
+}
+
 // the store holds what should not be reachable from the repository, so the
 // directories it is kept in are the owner's alone. only the tree at or below
 // `base` is created here; what is above it is the user's own business

src/project.rs

@@ -47,10 +47,23 @@ impl Project {
 
                 match serving.as_slice() {
                     [only] => Ok(only.to_string()),
+                    // publishing ports is the tie-break, so say which way it
+                    // failed: none of them serving is the opposite complaint
+                    // from all of them serving
+                    [] => Err(anyhow!(
+                        "cannot tell which service runs django, {} all build an image \
+                         and set {DJANGO_SETTINGS_MODULE}, and none publishes ports \
+                         to tell them apart",
+                        several.join(", ")
+                    )),
                     _ => Err(anyhow!(
                         "cannot tell which service runs django, {} all build an image, set \
                          {DJANGO_SETTINGS_MODULE} and publish ports",
-                        several.join(", ")
+                        serving
+                            .iter()
+                            .map(|name| **name)
+                            .collect::<Vec<&str>>()
+                            .join(", ")
                     )),
                 }
             }
@@ -82,13 +95,16 @@ impl Project {
         }
     }
 
-    pub fn postgres_credentials(&self, service: &str) -> (String, String) {
+    pub fn postgres_credentials(&self, service: &str) -> Result<(String, String)> {
-        let service = &self.services[service];
+        let entry = &self.services[service];
 
-        let user = env_var(service, "POSTGRES_USER").unwrap_or_else(|| "db".to_string());
+        let user = env_var(entry, "POSTGRES_USER").unwrap_or_else(|| "db".to_string());
-        let database = env_var(service, "POSTGRES_DB").unwrap_or_else(|| "db".to_string());
+        let database = env_var(entry, "POSTGRES_DB").unwrap_or_else(|| "db".to_string());
 
-        (user, database)
+        Ok((
+            usable_name(service, "POSTGRES_USER", user)?,
+            usable_name(service, "POSTGRES_DB", database)?,
+        ))
     }
 
     pub fn names(&self) -> Vec<&str> {
@@ -128,6 +144,35 @@ fn env_var(service: &Value, key: &str) -> Option<String> {
         .map(str::to_string)
 }
 
+// the compose file chooses these, and every postgres tool ahab runs takes them
+// as a role or database name. libpq reads a name holding `=` or a uri as a whole
+// connection string, so one could send a dump to another server entirely, and a
+// leading dash is read as an option however the argv is quoted
+fn usable_name(service: &str, key: &str, value: String) -> Result<String> {
+    let wrong = if value.is_empty() {
+        Some("is empty")
+    } else if value.starts_with('-') {
+        Some("starts with a dash, which the postgres tools read as an option")
+    } else if value.contains('=') {
+        Some("holds an `=`, which libpq reads as a connection string")
+    } else if value.contains("://") {
+        Some("holds a url, which libpq reads as a connection string")
+    } else if value.chars().any(|c| c.is_whitespace() || c.is_control()) {
+        Some("holds whitespace")
+    } else {
+        None
+    };
+
+    match wrong {
+        None => Ok(value),
+        Some(wrong) => Err(anyhow!(
+            "{key} of service {service} {wrong}: {value:?}. \
+             ahab passes it to psql, pg_dump and dropdb as a name, \
+             so it has to be one"
+        )),
+    }
+}
+
 fn is_postgres_image(image: &str) -> bool {
     let image = image.to_lowercase();
     POSTGRES_IMAGES.iter().any(|kind| image.contains(kind))
@@ -135,13 +180,43 @@ fn is_postgres_image(image: &str) -> bool {
 
 #[cfg(test)]
 mod tests {
-    use super::{Project, is_postgres_image};
+    use super::{Project, is_postgres_image, usable_name};
     use serde_json::json;
 
     fn compose(services: serde_json::Value) -> Project {
         Project { services }
     }
 
+    #[test]
+    fn a_name_the_postgres_tools_would_read_as_something_else_is_refused() {
+        // libpq takes a dbname holding `=` or a url as a whole conninfo string,
+        // so a compose file could otherwise pick the server a dump goes to
+        let refused = [
+            "postgresql://myproject:pw@evil.example.net:5432/loot",
+            "host=evil.example.net dbname=loot",
+            "--file=/var/lib/postgresql/data/pg_hba.conf",
+            "-h evil.example.net",
+            "my db",
+            "",
+        ];
+
+        for value in refused {
+            assert!(
+                usable_name("db", "POSTGRES_DB", value.to_string()).is_err(),
+                "should have been refused: {value:?}"
+            );
+        }
+
+        // and the names real projects actually use are left alone
+        for value in ["myproject_db", "my-db", "db", "app.prod", "DB_2"] {
+            assert_eq!(
+                usable_name("db", "POSTGRES_DB", value.to_string()).unwrap(),
+                value,
+                "should have been accepted: {value:?}"
+            );
+        }
+    }
+
     #[test]
     fn recognises_postgres_flavours() {
         for image in [
@@ -242,7 +317,7 @@ mod tests {
             },
         }));
 
-        let (user, database) = compose.postgres_credentials("db");
+        let (user, database) = compose.postgres_credentials("db").unwrap();
         assert_eq!(
             (user.as_str(), database.as_str()),
             ("myproject", "myproject_db")
@@ -253,7 +328,7 @@ mod tests {
     fn falls_back_to_db_when_the_service_says_nothing() {
         let compose = compose(json!({"db": {"image": "postgres:18"}}));
 
-        let (user, database) = compose.postgres_credentials("db");
+        let (user, database) = compose.postgres_credentials("db").unwrap();
         assert_eq!((user.as_str(), database.as_str()), ("db", "db"));
     }
 }