diff --git a/.gitignore b/.gitignore index 73fab07..c4a91a9 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,35 @@ target/ # MSVC Windows builds of rustc generate these, which store debugging information *.pdb + +# the cargo home CI keeps inside the project so it lands in the cache +.cargo/ + +# where the completions build writes when it is not given a directory +completions/ + +# os +.DS_Store + +# editors +.vscode +.idea +.ignore + +# ai +CLAUDE.local.md +.claude + +# languages +.python-version + +# nix +.nix-venv +.envrc +.direnv/ + +# project-local +TODO.md +# database dumps, which this tool exists to keep out of a repository +dumps +*.partial diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 9e549e6..37d1bf6 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -5,30 +5,38 @@ default: files: - Cargo.lock paths: + # no .cargo/bin here: the audit job runs what is in it, and this cache is + # shared by every branch and merge request, so a pipeline could otherwise + # leave behind the binary a later scheduled audit executes - .cargo/registry - # so the audit job does not rebuild cargo-audit on every schedule - - .cargo/bin - target variables: # keep the registry inside the project so it lands in the cache CARGO_HOME: $CI_PROJECT_DIR/.cargo CARGO_TERM_COLOR: always + # pinned, rather than whatever version exists on the night this runs + CARGO_AUDIT_VERSION: "0.21.2" # without this a push to a branch with an open merge request runs twice workflow: rules: + # first, because a scheduled pipeline sets CI_COMMIT_BRANCH as well and so + # would be matched by the branch rule below, or vetoed by the one above it + - if: $CI_PIPELINE_SOURCE == "schedule" - if: $CI_PIPELINE_SOURCE == "merge_request_event" - if: $CI_COMMIT_BRANCH && $CI_OPEN_MERGE_REQUESTS when: never - if: $CI_COMMIT_BRANCH - - if: $CI_PIPELINE_SOURCE == "schedule" check: script: - rustup component add rustfmt clippy - cargo fmt --check - - cargo clippy --all-targets -- -D warnings + # --locked on the first cargo invocation too: without it clippy resolves the + # dependencies and rewrites a Cargo.lock that has drifted from Cargo.toml, + # and the --locked commands after it then pass against what it just wrote + - cargo clippy --locked --all-targets -- -D warnings - cargo test --locked - cargo build --release --locked rules: @@ -39,19 +47,33 @@ check: # runs only from a pipeline schedule, so set one up in the project settings audit: script: - - cargo install cargo-audit --locked + - cargo install cargo-audit --version $CARGO_AUDIT_VERSION --locked - cargo audit # reports dependencies that have drifted behind, without changing the lockfile - cargo update --dry-run + # its own cache, so the audit tool is not rebuilt on every schedule while + # still being written only by this schedule-only job. the pinned version is + # part of the key, so a bump fetches rather than reusing the old binary + cache: + key: audit-tools-$CARGO_AUDIT_VERSION + paths: + - .cargo/bin + - .cargo/registry rules: - if: $CI_PIPELINE_SOURCE == "schedule" - allow_failure: true # the msrv declared in Cargo.toml, so it fails when something needs a newer rustc msrv: image: rust:1.85 script: - cargo build --locked + # its own key: artifacts built by another rustc are of no use to this job, and + # sharing one only has the two toolchains taking turns overwriting it + cache: + key: msrv-$CI_COMMIT_REF_SLUG + paths: + - .cargo/registry + - target rules: - if: $CI_PIPELINE_SOURCE == "schedule" when: never diff --git a/README.md b/README.md index 21aa507..5d5aab6 100644 --- a/README.md +++ b/README.md @@ -95,15 +95,21 @@ named anything: - **postgres**: the service whose image is a postgres flavour, matching `postg`, `timescale`, `pgvector` or `citus` - **django**: the service that both builds an image and sets - `DJANGO_SETTINGS_MODULE`. A worker sharing the same build and `env_file` - matches too, so the one publishing ports wins + `DJANGO_SETTINGS_MODULE`. Where more than one does — a worker beside the web + service, say — the one publishing ports wins If nothing matches, or two candidates cannot be told apart, `ahab` says so and -lists the services it looked at rather than guessing. +lists the services it looked at rather than guessing. Two services that both +build and set the settings module are ambiguous unless exactly one of them +publishes ports. `POSTGRES_USER` and `POSTGRES_DB` are read off the detected postgres service, so `dropdb`, `createdb`, `pg_restore` and `pg_dump` use the role and database the -project declares, falling back to `db` when it declares neither. +project declares, falling back to `db` when it declares neither. Both have to +be names: `ahab` hands them to those tools as a role and a database, and libpq +reads a database name holding an `=` or a url as a whole connection string, +which would send a dump to whatever server it names. A value that could be read +as something other than a name is refused rather than passed on. ## django @@ -138,6 +144,10 @@ ahab postgres import # drop, create, then restore ahab postgres psql # psql in the database container ``` +A dump is written `0600` and under a name of its own until it is complete, then +renamed over the target: a cluster dump carries every role's password hash, and +the default `0644` would hand it to anyone else with an account on the machine. + `psql` passes its arguments through and only asks docker for a terminal when it has one to hand over, so both of these work: @@ -155,9 +165,14 @@ A whole cluster dump from `pg_dumpall` is recognised by its header and handled differently again: it creates its own databases and carries role statements, so the database is dropped but not recreated, the dump goes to `psql` connected to `postgres`, and it runs without `ON_ERROR_STOP` because roles that already exist -report errors that are expected. Gzipped dumps are decompressed on the way in, -whichever of the three they hold. A cluster that has never held the database -yet is a valid target either way, so importing into a fresh one works. +report errors that are expected. Any error it does not expect is reported and +fails the import, since psql without `ON_ERROR_STOP` exits 0 having applied +only part of the dump. Gzipped dumps are decompressed on the way in, whichever +of the three they hold. A cluster that has never held the database yet is a +valid target either way, so importing into a fresh one works. + +`import` stops the project before it starts, so an import that fails leaves it +stopped and says so: `docker compose up` brings it back. ## link @@ -200,10 +215,24 @@ there is nothing to undo for a path whose symlink is gone. end with a NUL, and the two paths of a symlink leading elsewhere are separated by one as well, the way `git status -z` reports a rename. +`check` looks at tracked paths too, since git tracks symlinks and one can lead +out of the repository without appearing in any untracked listing. `add` cannot +externalize a tracked path, so all `check` can do is report it, under the code +`T>`. + +A symlink already leading out of the repository is not something `add` will +take: moving the link would move the pointer and leave the contents where they +are, so the store would hold a way back out while `check`, seeing a link into +the store, called the repository clean. + The store lives under `${XDG_DATA_HOME:-$HOME/.local/share}/ahab////`, derived from the git `origin` remote, or `_local/` when there is no remote to -name it after. +name it after. Its directories are created `0700`: it exists to hold what +should not be readable from the repository, and on a shared machine the default +`0755` would leave that to whoever else has an account. A store path derived +from a remote, or a checkout name, that needed characters replacing carries a +short fingerprint of the original, so two of them cannot land on one directory. ## configuration diff --git a/build.rs b/build.rs index 0d99eb1..50d1b71 100644 --- a/build.rs +++ b/build.rs @@ -25,6 +25,14 @@ fn install_dir(shell: Shell) -> Option<(PathBuf, bool)> { } fn main() -> Result<(), Error> { + // naming any rerun-if condition replaces cargo's default of re-running the + // script whenever anything in the package changed, so everything that + // shapes a completion script has to be named here. without them an + // installed script goes stale against the binary it completes + println!("cargo::rerun-if-changed=build.rs"); + println!("cargo::rerun-if-changed=src/cli"); + println!("cargo::rerun-if-changed=Cargo.toml"); + println!("cargo::rerun-if-env-changed=SHELL_COMPLETIONS_DIR"); for shell in Shell::value_variants() { println!( @@ -40,7 +48,18 @@ fn main() -> Result<(), Error> { continue; }; - fs::create_dir_all(&dir)?; + // a build script's working directory is the package root, so a relative + // dir would quietly install into the source tree and one holding `..` + // somewhere else again. only a path that says where it means is taken + if requested && !dir.is_absolute() { + return Err(Error::other(format!( + "the completions directory must be an absolute path, not {}", + dir.display() + ))); + } + + fs::create_dir_all(&dir) + .map_err(|e| Error::other(format!("creating {}: {e}", dir.display())))?; let path = generate_to(*shell, &mut cmd, env!("CARGO_PKG_NAME"), &dir)?; if requested { diff --git a/src/cli/django.rs b/src/cli/django.rs index 0a4c8ad..5790ebd 100644 --- a/src/cli/django.rs +++ b/src/cli/django.rs @@ -9,7 +9,7 @@ pub enum Django { /// Start a bash session in a fresh django container Bash, - /// Prepare empty management command 'command' in app 'app' + /// Prepare an empty management command NAME in the app at APP MakeCommand { app: PathBuf, name: String }, /// Run Django's manage.py makemigrations 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..ed44190 100644 --- a/src/cmd/argv.rs +++ b/src/cmd/argv.rs @@ -1,7 +1,8 @@ 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}, }; @@ -9,10 +10,17 @@ 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)] -pub struct Argv(Vec); +pub struct Argv(Vec); + +// 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 { @@ -21,42 +29,57 @@ impl Display for Argv { } impl Argv { - pub fn new(program: &str) -> Self { - Self(vec![program.to_string()]) + pub fn new(program: impl AsRef) -> Self { + Self(vec![program.as_ref().to_owned()]) } - pub fn arg(mut self, arg: impl AsRef) -> Self { - self.0.push(arg.as_ref().to_string()); + pub fn arg(mut self, arg: impl AsRef) -> Self { + self.0.push(arg.as_ref().to_owned()); self } pub fn args(mut self, args: I) -> Self where I: IntoIterator, - S: AsRef, + S: AsRef, { self.0 - .extend(args.into_iter().map(|arg| arg.as_ref().to_string())); + .extend(args.into_iter().map(|arg| arg.as_ref().to_owned())); self } // a long option and the value it takes - pub fn flag(self, name: &str, value: impl AsRef) -> Self { + pub fn flag(self, name: &str, value: impl AsRef) -> Self { self.arg(name).arg(value) } - pub fn program(&self) -> &str { - self.0.first().map(String::as_str).unwrap_or_default() + // for a message that names the tool being run, so lossy is what is wanted + pub fn program(&self) -> String { + self.0 + .first() + .map(|word| word.to_string_lossy().into_owned()) + .unwrap_or_default() } - pub fn words(&self) -> &[String] { + pub fn words(&self) -> &[OsString] { &self.0 } - // one word list for `sh -c`, quoted so a value with a space survives the shell + // 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 pub fn quoted(&self) -> String { - // a nul byte is the only thing shlex refuses, and no argv can hold one - shlex::try_join(self.0.iter().map(String::as_str)).unwrap_or_default() + let words: Vec = self + .0 + .iter() + .map(|word| word.to_string_lossy().into_owned()) + .collect(); + + // a nul byte is the only thing shlex refuses, and no argv can hold one, + // so this only happens for a word that could never have been run. it + // still has to be readable: an empty string would name no command at + // all in the very message explaining what went wrong + shlex::try_join(words.iter().map(String::as_str)).unwrap_or_else(|_| words.join(" ")) } pub fn run(&self, ctx: &Ctx) -> Result<()> { @@ -71,6 +94,14 @@ impl Argv { // reading changes nothing, so a dry run answers the question for real pub fn capture(&self, ctx: &Ctx) -> Result { + let stdout = self.capture_bytes(ctx)?; + + String::from_utf8(stdout).with_context(|| format!("reading the output of `{self}`")) + } + + // git's -z listings are raw bytes: a path on unix is bytes, not text, and + // one that is not utf-8 would otherwise fail the whole listing it appears in + pub fn capture_bytes(&self, ctx: &Ctx) -> Result> { let out = self.command(ctx)?.output()?; if !out.status.success() { @@ -84,7 +115,7 @@ impl Argv { }); } - Ok(String::from_utf8(out.stdout)?) + Ok(out.stdout) } // replaces this process, so the command's exit code and signals become ours @@ -110,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 { + if self.skipped(ctx) { + return Ok(planned()); + } + Ok(self.command(ctx)?.spawn()?.wait()?) } pub fn stdin_from(&self, ctx: &Ctx, input: &Path) -> Result { + if self.skipped(ctx) { + return Ok(planned()); + } + let stdin = self.opened(input)?; Ok(self.command(ctx)?.stdin(stdin).spawn()?.wait()?) @@ -121,24 +160,41 @@ 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 { + 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 { + let stdin = self.opened(input)?; + + Ok(self.command(ctx)?.stdin(stdin).output()?) + } + + // a probe: it asks a question rather than changing anything, so a dry run + // answers it for real, and the status is the answer instead of an error + pub fn probe_status(&self, ctx: &Ctx) -> Result { + Ok(self.command(ctx)?.stdout(Stdio::null()).spawn()?.wait()?) + } + // whether it succeeded, without failure being an error pub fn quietly_succeeds(&self, ctx: &Ctx) -> Result { - Ok(self - .command(ctx)? - .stdout(Stdio::null()) - .spawn()? - .wait()? - .success()) + Ok(self.probe_status(ctx)?.success()) } 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 +206,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/cmd/docker.rs b/src/cmd/docker.rs index b57d9b1..5962de9 100644 --- a/src/cmd/docker.rs +++ b/src/cmd/docker.rs @@ -1,3 +1,6 @@ +use std::ffi::OsString; +use std::path::Path; + use super::{Argv, Cmd}; // docker exec, around a command that runs inside the container @@ -47,24 +50,25 @@ impl Cmd for Exec { } } -// docker cp, in either direction +// docker cp, in either direction. the local side stays an OsString: it is a +// path the user named, and a lossy one would copy something else pub struct Cp { - from: String, - to: String, + from: OsString, + to: OsString, } impl Cp { - pub fn into_container(local: &str, container: &str, remote: &str) -> Self { + pub fn into_container(local: &Path, container: &str, remote: &str) -> Self { Self { - from: local.to_string(), - to: format!("{container}:{remote}"), + from: local.as_os_str().to_owned(), + to: OsString::from(format!("{container}:{remote}")), } } - pub fn out_of_container(container: &str, remote: &str, local: &str) -> Self { + pub fn out_of_container(container: &str, remote: &str, local: &Path) -> Self { Self { - from: format!("{container}:{remote}"), - to: local.to_string(), + from: OsString::from(format!("{container}:{remote}")), + to: local.as_os_str().to_owned(), } } } diff --git a/src/cmd/git.rs b/src/cmd/git.rs index cf7d211..a3f0d07 100644 --- a/src/cmd/git.rs +++ b/src/cmd/git.rs @@ -1,10 +1,22 @@ +use std::ffi::OsString; use std::path::Path; use super::{Argv, Cmd}; // -C has no long form; it runs git as though from that directory fn git(root: &Path) -> Argv { - Argv::new("git").arg("-C").arg(root.to_string_lossy()) + Argv::new("git").arg("-C").arg(root) +} + +// `--` stops git reading a leading dash as an option, but pathspec magic is +// read after it too, so a file actually named `:(exclude)x` or `:!x` would ask +// about a different set of paths than the one in hand. built as an OsString: a +// path is bytes, and a lossy one asks git about a name nothing on disk has +fn literal(path: &Path) -> OsString { + let mut spec = OsString::from(":(literal)"); + spec.push(path); + + spec } // the root of the repository the working directory is in @@ -31,7 +43,7 @@ pub struct LsFiles<'a> { root: &'a Path, tracked: bool, ignored: bool, - pathspecs: Vec, + pathspecs: Vec, } impl<'a> LsFiles<'a> { @@ -64,7 +76,7 @@ impl<'a> LsFiles<'a> { pub fn limited_to>(mut self, pathspecs: &[P]) -> Self { self.pathspecs = pathspecs .iter() - .map(|path| path.as_ref().to_string_lossy().to_string()) + .map(|path| literal(path.as_ref())) .collect(); self } @@ -72,15 +84,14 @@ impl<'a> LsFiles<'a> { impl Cmd for LsFiles<'_> { fn argv(&self) -> Argv { - let mut argv = git(self.root).arg("ls-files"); + // -z has no long form; it separates the paths with NUL, which is the + // only separator a filename cannot contain + let mut argv = git(self.root).arg("ls-files").arg("-z"); if self.tracked { argv = argv.arg("--cached"); } else { - // -z has no long form; it separates the paths with NUL, which is the - // only separator a filename cannot contain argv = argv - .arg("-z") .arg("--others") .arg("--exclude-standard") .arg("--directory") @@ -113,7 +124,7 @@ impl Cmd for CheckIgnore<'_> { .arg("check-ignore") .arg("--quiet") .arg("--") - .arg(self.path.to_string_lossy()) + .arg(self.path) } } @@ -134,7 +145,7 @@ mod tests { assert_eq!( argv.quoted(), "git -C /repo ls-files -z --others --exclude-standard --directory \ - --no-empty-directory --ignored -- 'a b'" + --no-empty-directory --ignored -- ':(literal)a b'", ); } @@ -144,7 +155,25 @@ mod tests { .limited_to(&[PathBuf::from(".env")]) .argv(); - assert_eq!(argv.quoted(), "git -C /repo ls-files --cached -- .env"); + assert_eq!( + argv.quoted(), + "git -C /repo ls-files -z --cached -- ':(literal).env'" + ); + } + + #[test] + fn a_pathspec_is_asked_for_literally_however_it_is_spelled() { + // git reads magic after `--` too, so these names must not become one + for name in [":(exclude)secret", ":!secret", ":(glob)**"] { + let argv = LsFiles::tracked(Path::new("/repo")) + .limited_to(&[PathBuf::from(name)]) + .argv(); + + assert_eq!( + argv.words().last().unwrap(), + format!(":(literal){name}").as_str() + ); + } } #[test] diff --git a/src/cmd/mod.rs b/src/cmd/mod.rs index 1c13afa..a88486f 100644 --- a/src/cmd/mod.rs +++ b/src/cmd/mod.rs @@ -55,6 +55,10 @@ pub trait Cmd { self.argv().capture(ctx) } + fn capture_bytes(&self, ctx: &Ctx) -> Result> { + self.argv().capture_bytes(ctx) + } + fn replace(&self, ctx: &Ctx) -> Result<()> { self.argv().replace(ctx) } @@ -76,7 +80,15 @@ pub trait Cmd { self.argv().stdin_from_captured(ctx, input) } + fn probe_with_stdin(&self, ctx: &Ctx, input: &Path) -> Result { + self.argv().probe_with_stdin(ctx, input) + } + fn quietly_succeeds(&self, ctx: &Ctx) -> Result { self.argv().quietly_succeeds(ctx) } + + fn probe_status(&self, ctx: &Ctx) -> Result { + self.argv().probe_status(ctx) + } } diff --git a/src/cmd/postgres.rs b/src/cmd/postgres.rs index 74b9aea..e2dbe7d 100644 --- a/src/cmd/postgres.rs +++ b/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" ); } diff --git a/src/commands/completions.rs b/src/commands/completions.rs index b7ffb0d..a8c1913 100644 --- a/src/commands/completions.rs +++ b/src/commands/completions.rs @@ -27,15 +27,24 @@ fn script(shell: Shell) -> Vec { #[cfg(test)] mod tests { - use super::{Shell, script}; - use clap::ValueEnum; + use super::{Ahab, Shell, script}; + use clap::{CommandFactory, ValueEnum}; #[test] fn every_shell_gets_a_script_covering_the_subcommands() { + // asked of the parser rather than listed here, which is a list that + // silently stops covering the newest command + let subcommands: Vec = Ahab::command() + .get_subcommands() + .map(|sub| sub.get_name().to_string()) + .collect(); + + assert!(subcommands.len() > 1, "{subcommands:?}"); + for shell in Shell::value_variants() { let out = String::from_utf8(script(*shell)).expect("script is utf8"); - for subcommand in ["django", "postgres", "link", "completions"] { + for subcommand in &subcommands { assert!( out.contains(subcommand), "{shell:?} script never mentions {subcommand}" 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..5a725be 100644 --- a/src/commands/link.rs +++ b/src/commands/link.rs @@ -7,14 +7,18 @@ use fs_err::{read_dir, read_link}; use std::cell::Cell; use std::path::{Path, PathBuf}; -use anyhow::{Result, anyhow, bail}; +use anyhow::{Context, Result, anyhow, bail}; -use self::store::{Repo, ignored, resolve, symlink_metadata_opt, tracked}; +use self::store::{Leads, Repo, ignored, leads, resolve, symlink_metadata_opt, tracked}; use crate::ctx::Ctx; -use crate::fsops::{move_path, place_link, prune_empty, remove_file, rename, suffixed}; +use crate::fsops::{ + ensure_private_parent, move_path, place_link, prune_empty, remove_file, rename, suffixed, +}; use crate::output::{line, note, 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, paths: &[PathBuf], force: bool, store: Option<&Path>) -> Result<()> { @@ -28,7 +32,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 +48,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 +92,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; } } @@ -126,8 +130,27 @@ fn restore_one(ctx: &Ctx, repo: &Repo, path: &Path, report: &Report) -> Result<( bail!("{} is missing from the store", rel.display()); } - remove_file(ctx, &src)?; - move_path(ctx, &stored, &src)?; + // the link is moved aside rather than removed: if the payload cannot come + // back out of the store, the repository is left pointing at where it still is + let aside = suffixed(&src, RESTORING_SUFFIX); + if symlink_metadata_opt(&aside)?.is_some() { + bail!("{} is in the way; move it aside", aside.display()); + } + + rename(ctx, &src, &aside)?; + + if let Err(e) = move_path(ctx, &stored, &src) { + rename(ctx, &aside, &src).with_context(|| { + format!( + "could not put the link at {} back after failing to restore it", + src.display() + ) + })?; + + return Err(e); + } + + remove_file(ctx, &aside)?; prune_empty(ctx, stored.parent(), &repo.base); report.line("restored", &rel); @@ -158,9 +181,13 @@ fn stored_paths(repo: &Repo, dir: &Path) -> Result> { for entry in entries { let stored = entry?.path(); - let rel = stored - .strip_prefix(&repo.store) - .expect("walked out of the store"); + let rel = stored.strip_prefix(&repo.store).with_context(|| { + format!( + "{} is not under the store {}", + stored.display(), + repo.store.display() + ) + })?; let src = repo.root.join(rel); let state = match symlink_metadata_opt(&src)? { @@ -170,8 +197,12 @@ fn stored_paths(repo: &Repo, dir: &Path) -> Result> { Some(_) => Stored::Taken, }; + // a symlink the store happens to hold is a leaf, never a directory to + // walk into: is_dir would follow it and list whatever it points at + let holds_dir = symlink_metadata_opt(&stored)?.is_some_and(|meta| meta.is_dir()); + // a linked directory is one entry; otherwise the paths inside it are - if state == Stored::Linked || !stored.is_dir() { + if state == Stored::Linked || !holds_dir { found.push((src, state)); } else { found.extend(stored_paths(repo, &stored)?); @@ -249,22 +280,37 @@ fn link_one(ctx: &Ctx, repo: &Repo, path: &Path, force: bool, report: &Report) - let rel = repo.relative(&src)?; let target = repo.store.join(&rel); - // a target inside the repo would be readable from the sandbox anyway - if target.starts_with(&repo.root) { + // a target inside the repo would be readable from the sandbox anyway. the + // base is resolved as well as compared: one symlinked into the checkout + // passes a prefix test while landing the file straight back inside it + if target.starts_with(&repo.root) || matches!(leads(&repo.base, &repo.root), Leads::Inside) { return Err(anyhow!( - "target {} is inside the repository; point AHAB_LINK_ROOT elsewhere", - target.display() + "the store at {} is inside the repository {}; point --store or \ + AHAB_LINK_ROOT somewhere else", + repo.base.display(), + repo.root.display() )); } + stays_in_store(repo, &rel)?; + // before anything is moved in, so the tree it lands in is never briefly + // readable by anyone else + ensure_private_parent(ctx, &repo.base, &target)?; + if tracked(ctx, repo, &rel)? { return Err(anyhow!( "{} is tracked by git; only untracked or ignored paths can be externalized", rel.display() )); } - if !ignored(ctx, repo, &rel) { - warning!("{} is not gitignored", rel.display()); + match ignored(ctx, repo, &rel) { + Ok(true) => {} + Ok(false) => warning!("{} is not gitignored", rel.display()), + // saying "not gitignored" here would be an answer git never gave + Err(e) => warning!( + "could not tell whether {} is gitignored: {e:#}", + rel.display() + ), } let src_meta = symlink_metadata_opt(&src)?; @@ -288,6 +334,19 @@ fn link_one(ctx: &Ctx, repo: &Repo, path: &Path, force: bool, report: &Report) - // nothing in the store to adopt, so the symlink itself moves out if !target_taken { + // moving the link moves the pointer and leaves the contents + // where they are, so the store would hold a way back out and + // check, seeing a link into the store, would call it clean + if let Leads::Outside(end) = leads(&src, &repo.root) { + return Err(anyhow!( + "{} is a symlink to {}, outside the repository; \ + externalizing it would move the link and leave its \ + contents there, so repoint or remove it instead", + rel.display(), + end.display() + )); + } + if !src.exists() { warning!( "{} is a broken symlink to {}", @@ -295,8 +354,7 @@ fn link_one(ctx: &Ctx, repo: &Repo, path: &Path, force: bool, report: &Report) - dest.display() ); } - move_path(ctx, &src, &target)?; - place_link(ctx, &src, &target)?; + move_and_link(ctx, &src, &target)?; report.line("moved", &rel); return Ok(()); } @@ -332,8 +390,7 @@ fn link_one(ctx: &Ctx, repo: &Repo, path: &Path, force: bool, report: &Report) - } Some(_) => { - move_path(ctx, &src, &target)?; - place_link(ctx, &src, &target)?; + move_and_link(ctx, &src, &target)?; report.line("moved", &rel); Ok(()) } @@ -355,6 +412,57 @@ fn link_one(ctx: &Ctx, repo: &Repo, path: &Path, force: bool, report: &Report) - } } +// the two halves have to end up looking like one step: with the payload moved +// but no link placed, the store holds a path nothing points at and the working +// tree has lost it altogether, which is the one outcome worse than failing +fn move_and_link(ctx: &Ctx, src: &Path, target: &Path) -> Result<()> { + move_path(ctx, src, target)?; + + if let Err(e) = place_link(ctx, src, target) { + move_path(ctx, target, src).with_context(|| { + format!( + "could not put {} back after failing to link it to {}", + src.display(), + target.display() + ) + })?; + + return Err(e); + } + + Ok(()) +} + +// creating the store directories follows any symlink already standing in them, +// so a store that holds one would take the move somewhere else entirely; only +// the components at or below the store are examined, since everything above it +// is outside by definition +fn stays_in_store(repo: &Repo, rel: &Path) -> Result<()> { + let mut path = repo.store.clone(); + + for part in rel.components() { + path.push(part); + + match symlink_metadata_opt(&path)? { + // nothing here yet, so nothing below it can be followed either + None => return Ok(()), + Some(meta) if meta.is_symlink() => { + if let Leads::Outside(end) = leads(&path, &repo.store) { + return Err(anyhow!( + "the store holds {} as a symlink to {}, outside the store; \ + refusing to write through it", + path.display(), + end.display() + )); + } + } + Some(_) => {} + } + } + + Ok(()) +} + fn needs_force(target: &Path) -> anyhow::Error { anyhow!( "{} already exists; pass --force to link to it", diff --git a/src/commands/link/check.rs b/src/commands/link/check.rs index d3c3791..7b4f980 100644 --- a/src/commands/link/check.rs +++ b/src/commands/link/check.rs @@ -1,12 +1,14 @@ use fs_err::{read_dir, read_link}; +use std::ffi::OsString; +use std::os::unix::ffi::{OsStrExt, OsStringExt}; use std::path::{Path, PathBuf}; use anyhow::{Result, anyhow}; -use super::store::{Repo, resolve, symlink_metadata_opt}; +use super::store::{Leads, Repo, leads, resolve, symlink_metadata_opt}; use crate::cmd::{Cmd, LsFiles}; use crate::ctx::Ctx; -use crate::output::{line, text}; +use crate::output::{line, text, write_bytes}; // whether anything is outside the store, which main turns into an exit code pub fn check( @@ -23,17 +25,19 @@ pub fn check( // git lists untracked and ignored separately for (mark, ignored) in [(UNTRACKED, false), (IGNORED, true)] { for entry in list_others(ctx, &repo, ignored, &pathspecs)? { - let rel = PathBuf::from(entry.trim_end_matches('/')); - // --directory collapses a wholly untracked dir into `dir/` - if entry.ends_with('/') { - exposed.extend(walk(&repo, &rel, mark)?.1); - } else { - exposed.extend(classify(&repo, &rel, mark)?); + match entry.strip_suffix(b"/") { + Some(dir) => exposed.extend(walk(&repo, &path_from(dir), mark)?.1), + None => exposed.extend(classify(&repo, &path_from(&entry), mark)?), } } } + // and says nothing about tracked paths, where a symlink out still counts + for entry in list_tracked(ctx, &repo, &pathspecs)? { + exposed.extend(classify_tracked(&repo, &path_from(&entry))?); + } + exposed.sort_by(|a, b| a.name.cmp(&b.name)); if porcelain || null { @@ -46,19 +50,36 @@ pub fn check( } fn print_porcelain(exposed: &[Exposed], null: bool) { - let end = if null { '\0' } else { '\n' }; - // a filename can hold an arrow but not a NUL, as `git status -z` also assumes - let between = if null { "\0" } else { " -> " }; + // -z is the format for scripts that must survive any filename, so its + // records are written as the bytes a path actually is. a filename can hold + // an arrow but not a NUL, as `git status -z` also assumes + if null { + for item in exposed { + let mut record = item.code().into_bytes(); + record.push(b' '); + record.extend_from_slice(item.name.as_os_str().as_bytes()); + + if let Some(dest) = &item.dest { + record.push(0); + record.extend_from_slice(dest.as_os_str().as_bytes()); + } + + record.push(0); + write_bytes(&record); + } + + return; + } for item in exposed { match &item.dest { Some(dest) => text!( - "{} {}{between}{}{end}", + "{} {} -> {}\n", item.code(), - item.name, + item.name.display(), dest.display() ), - None => text!("{} {}{end}", item.code(), item.name), + None => text!("{} {}\n", item.code(), item.name.display()), } } } @@ -98,8 +119,8 @@ fn print_listing(repo: &Repo, exposed: &[Exposed]) { line!("\n{heading}\n{hint}"); for item in items { match &item.dest { - Some(dest) => line!("\t{} -> {}", item.name, dest.display()), - None => line!("\t{}", item.name), + Some(dest) => line!("\t{} -> {}", item.name.display(), dest.display()), + None => line!("\t{}", item.name.display()), } } } @@ -123,15 +144,17 @@ impl Section { const UNTRACKED: char = '?'; const IGNORED: char = '!'; const ELSEWHERE: char = '>'; +// no git equivalent: a tracked path, which only ever appears as a symlink out +const TRACKED: char = 'T'; struct Exposed { mark: char, - name: String, + name: PathBuf, dest: Option, } impl Exposed { - fn content(mark: char, name: String) -> Self { + fn content(mark: char, name: PathBuf) -> Self { Self { mark, name, @@ -151,7 +174,7 @@ impl Exposed { fn classify(repo: &Repo, rel: &Path, mark: char) -> Result> { let src = repo.root.join(rel); - let name = rel.display().to_string(); + let name = rel.to_path_buf(); let Some(meta) = symlink_metadata_opt(&src)? else { return Ok(None); @@ -162,7 +185,16 @@ fn classify(repo: &Repo, rel: &Path, mark: char) -> Result> { let dest = read_link(&src)?; if dest == repo.store.join(rel) { - return Ok(None); + // it names the store, but what the store holds there can be a symlink of + // its own leading straight back out, which is not being held at all + return Ok(match leads(&src, &repo.store) { + Leads::Inside | Leads::Dangling => None, + Leads::Outside(end) => Some(Exposed { + mark, + name, + dest: Some(end), + }), + }); } Ok(Some(Exposed { @@ -172,6 +204,29 @@ fn classify(repo: &Repo, rel: &Path, mark: char) -> Result> { })) } +// git tracks symlinks, so one can lead out of the repository without ever +// showing up in an untracked or ignored listing; `add` cannot externalize it +// either, so all check can do is say it is there +fn classify_tracked(repo: &Repo, rel: &Path) -> Result> { + let src = repo.root.join(rel); + + let Some(meta) = symlink_metadata_opt(&src)? else { + return Ok(None); + }; + if !meta.is_symlink() { + return Ok(None); + } + + Ok(match leads(&src, &repo.root) { + Leads::Inside | Leads::Dangling => None, + Leads::Outside(end) => Some(Exposed { + mark: TRACKED, + name: rel.to_path_buf(), + dest: Some(end), + }), + }) +} + fn walk(repo: &Repo, rel: &Path, mark: char) -> Result<(usize, Vec)> { let dir = repo.root.join(rel); let mut handled = 0; @@ -194,10 +249,16 @@ fn walk(repo: &Repo, rel: &Path, mark: char) -> Result<(usize, Vec)> { } } - // nothing below is in the store, so collapse to one line - if handled == 0 && !exposed.is_empty() { - let name = format!("{}/", rel.display()); - return Ok((0, vec![Exposed::content(mark, name)])); + // nothing below is in the store, so collapse to one line -- unless some of + // it leads out of the repository, which is a different thing to report and + // carries a destination the one line would drop + let all_content = exposed.iter().all(|item| item.dest.is_none()); + + if handled == 0 && !exposed.is_empty() && all_content { + let mut name = rel.as_os_str().to_owned(); + name.push("/"); + + return Ok((0, vec![Exposed::content(mark, PathBuf::from(name))])); } Ok((handled, exposed)) @@ -230,16 +291,31 @@ fn list_others( repo: &Repo, ignored: bool, pathspecs: &[PathBuf], -) -> Result> { +) -> Result>> { let mut listing = LsFiles::untracked(&repo.root).limited_to(pathspecs); if ignored { listing = listing.ignored(); } - Ok(listing - .capture(ctx)? - .split('\0') - .filter(|p| !p.is_empty()) - .map(String::from) - .collect()) + Ok(split_nul(&listing.capture_bytes(ctx)?)) +} + +fn list_tracked(ctx: &Ctx, repo: &Repo, pathspecs: &[PathBuf]) -> Result>> { + let listing = LsFiles::tracked(&repo.root).limited_to(pathspecs); + + Ok(split_nul(&listing.capture_bytes(ctx)?)) +} + +// every listing check reads is asked for with -z, and kept as the bytes git +// wrote: a path is not obliged to be utf-8, and one that is not used to fail +// the whole listing rather than the one entry +fn split_nul(out: &[u8]) -> Vec> { + out.split(|byte| *byte == 0) + .filter(|record| !record.is_empty()) + .map(<[u8]>::to_vec) + .collect() +} + +fn path_from(bytes: &[u8]) -> PathBuf { + PathBuf::from(OsString::from_vec(bytes.to_vec())) } diff --git a/src/commands/link/store.rs b/src/commands/link/store.rs index ef7c89b..7be74c8 100644 --- a/src/commands/link/store.rs +++ b/src/commands/link/store.rs @@ -1,7 +1,8 @@ use fs_err as fs; use std::env; -use std::ffi::OsString; -use std::path::{Path, PathBuf}; +use std::ffi::{OsStr, OsString}; +use std::os::unix::ffi::{OsStrExt, OsStringExt}; +use std::path::{Component, Path, PathBuf}; use anyhow::{Context, Result, anyhow}; @@ -22,10 +23,10 @@ pub(super) struct Repo { impl Repo { pub(super) fn discover(ctx: &Ctx, store: Option<&Path>) -> Result { let root = git_root(ctx)?; - let base = match store { + let base = normalized(match store { Some(store) => store.to_path_buf(), None => store_root()?, - }; + })?; Ok(Self { store: base.join(repo_components(ctx, &root)?), @@ -68,17 +69,44 @@ pub(super) fn resolve(path: &Path) -> Result { Ok(parent.join(name)) } -fn git_root(ctx: &Ctx) -> Result { - let root = RevParse - .capture(ctx) - .map_err(|_| anyhow!("not inside a git repository"))?; +// absolute, with `.` and `..` folded out. the store path is written into every +// symlink `add` creates, where a relative one would resolve from the link's own +// directory rather than the working one, and it is compared against the +// repository root, which a `..` would slip past +fn normalized(path: PathBuf) -> Result { + let absolute = std::path::absolute(&path) + .with_context(|| format!("resolving absolute path of {}", path.display()))?; - let root = root.trim().to_string(); + let mut out = PathBuf::new(); + for part in absolute.components() { + match part { + Component::CurDir => {} + Component::ParentDir => { + out.pop(); + } + part => out.push(part), + } + } + + Ok(out) +} + +fn git_root(ctx: &Ctx) -> Result { + // bytes, since the checkout can live at a path that is not utf-8, and with + // the cause kept: git not being installed and the directory not being a + // repository are different problems with the same one-line answer otherwise + let root = RevParse + .capture_bytes(ctx) + .context("asking git for the repository root")?; + + let root = root.strip_suffix(b"\n").unwrap_or(&root); if root.is_empty() { return Err(anyhow!("git reported an empty repository root")); } - Ok(fs::canonicalize(&root)?) + Ok(fs::canonicalize(PathBuf::from(OsString::from_vec( + root.to_vec(), + )))?) } fn repo_components(ctx: &Ctx, root: &Path) -> Result { @@ -95,7 +123,7 @@ fn repo_components(ctx: &Ctx, root: &Path) -> Result { let name = root .file_name() .ok_or_else(|| anyhow!("cannot derive a store path for {}", root.display()))?; - Ok(Path::new(LOCAL_NAMESPACE).join(sanitize(&name.to_string_lossy()))) + Ok(Path::new(LOCAL_NAMESPACE).join(sanitize_name(name))) } fn git_origin_url(ctx: &Ctx) -> Option { @@ -135,7 +163,7 @@ fn components_from_remote(url: &str) -> Option { (depth > 0).then_some(components) } -fn sanitize(s: &str) -> String { +fn cleaned(s: &str) -> String { let out: String = s .chars() .map(|c| { @@ -148,10 +176,51 @@ fn sanitize(s: &str) -> String { .collect(); // `.` and `..` are legal characters but not legal components - if out.chars().all(|c| c == '.') { - return "_".repeat(out.len()); + match out.chars().all(|c| c == '.') { + true => "_".repeat(out.len()), + false => out, } - out +} + +fn sanitize(s: &str) -> String { + let out = cleaned(s); + + // every replaced character maps to the same `_`, so `my~api` and `my:api` + // would otherwise share one directory with the plain `my_api`. a component + // that came through untouched keeps its name, so the common remote keeps + // the store path it already has + match out == s { + true => out, + false => format!("{out}-{}", fingerprint(s.as_bytes())), + } +} + +// a checkout name is bytes like any other path. a lossy rendering turns every +// byte it cannot read into the same replacement character, so sanitize would +// see two different names as one and fingerprint them identically: the bytes +// themselves are what has to be fingerprinted +fn sanitize_name(name: &OsStr) -> String { + match name.to_str() { + Some(text) => sanitize(text), + None => format!( + "{}-{}", + cleaned(&name.to_string_lossy()), + fingerprint(name.as_bytes()) + ), + } +} + +// fnv-1a: the store path has to stay put across rust releases, which the hashers +// in std explicitly do not promise +fn fingerprint(bytes: &[u8]) -> String { + let mut hash: u64 = 0xcbf2_9ce4_8422_2325; + + for byte in bytes { + hash ^= u64::from(*byte); + hash = hash.wrapping_mul(0x100_0000_01b3); + } + + format!("{:08x}", hash as u32) } fn store_root() -> Result { @@ -170,17 +239,28 @@ fn non_empty_var(name: &str) -> Option { } pub(super) fn tracked(ctx: &Ctx, repo: &Repo, rel: &Path) -> Result { + // bytes: the listing echoes back the path asked about, which is not obliged + // to be utf-8, and failing to read it would refuse the path for the wrong + // reason rather than answering whether git tracks it let listed = LsFiles::tracked(&repo.root) .limited_to(&[rel]) - .capture(ctx)?; + .capture_bytes(ctx)?; Ok(!listed.is_empty()) } -pub(super) fn ignored(ctx: &Ctx, repo: &Repo, rel: &Path) -> bool { - CheckIgnore::new(&repo.root, rel) - .quietly_succeeds(ctx) - .unwrap_or(false) +// check-ignore says 0 for ignored and 1 for not; anything else means it could +// not answer at all, which is not the same as "not ignored". git cannot even be +// asked about a path whose name looks like pathspec magic, since it rejects the +// magic rather than the name, and `:(literal)` is not accepted by this command +pub(super) fn ignored(ctx: &Ctx, repo: &Repo, rel: &Path) -> Result { + let status = CheckIgnore::new(&repo.root, rel).probe_status(ctx)?; + + match status.code() { + Some(0) => Ok(true), + Some(1) => Ok(false), + _ => Err(anyhow!("`git check-ignore` exited with {status}")), + } } pub(super) fn symlink_metadata_opt(path: &Path) -> Result> { @@ -192,9 +272,40 @@ pub(super) fn symlink_metadata_opt(path: &Path) -> Result Leads { + let Ok(end) = fs::canonicalize(path) else { + return Leads::Dangling; + }; + + // the root can be reached through a symlink of its own, so resolve it too + // rather than comparing a resolved path against an unresolved prefix + let root = fs::canonicalize(root).unwrap_or_else(|_| root.to_path_buf()); + + match end.starts_with(&root) { + true => Leads::Inside, + false => Leads::Outside(end), + } +} + #[cfg(test)] mod tests { - use super::{components_from_remote, sanitize}; + use super::{components_from_remote, fingerprint, normalized, sanitize, sanitize_name}; + use std::collections::HashSet; + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; use std::path::PathBuf; #[test] @@ -249,9 +360,70 @@ mod tests { #[test] fn sanitize_never_yields_a_traversal() { - assert_eq!(sanitize(".."), "__"); - assert_eq!(sanitize("."), "_"); - assert_eq!(sanitize("a/b"), "a_b"); + for name in ["..", ".", "a/b", "../..", "a/../b"] { + let out = sanitize(name); + + assert!(!out.contains('/'), "{name} -> {out}"); + assert!(out != "." && out != "..", "{name} -> {out}"); + } + + // a name that needed no replacing keeps the store path it already has assert_eq!(sanitize(".env"), ".env"); + assert_eq!(sanitize("afurnik"), "afurnik"); + assert_eq!(sanitize("git.aflabs.org"), "git.aflabs.org"); + } + + #[test] + fn sanitize_keeps_names_apart_that_replacing_would_collapse() { + // every disallowed character maps to `_`, so without the fingerprint + // these would all share one store directory with a plain `my_api` + let names = ["my~api", "my:api", "my api", "my/api", "my%api"]; + + for name in names { + assert_ne!(sanitize(name), sanitize("my_api"), "name: {name}"); + } + + let distinct: HashSet = names.iter().map(|name| sanitize(name)).collect(); + assert_eq!(distinct.len(), names.len(), "{distinct:?}"); + } + + #[test] + fn a_fingerprint_does_not_drift_with_the_toolchain() { + // std's hashers make no such promise, and a moved store loses the files + assert_eq!(fingerprint(b""), "84222325"); + assert_eq!(fingerprint(b"my~api"), fingerprint(b"my~api")); + assert_ne!(fingerprint(b"my~api"), fingerprint(b"my:api")); + } + + #[test] + fn a_checkout_name_that_is_not_utf_8_keeps_its_own_directory() { + // both render to the same replacement character, so a fingerprint taken + // of the rendering rather than the bytes cannot tell them apart + let one = OsStr::from_bytes(b"proj\xe9"); + let two = OsStr::from_bytes(b"proj\xff"); + + assert_ne!(sanitize_name(one), sanitize_name(two)); + // and a name that is utf-8 is keyed exactly as before + assert_eq!(sanitize_name(OsStr::new("afurnik")), "afurnik"); + } + + #[test] + fn a_store_root_is_absolute_with_the_dots_folded_out() { + // it is written into every symlink add creates, and compared against the + // repository root, so it needs exactly one spelling + let cases = [ + ("/a/b/../c", "/a/c"), + ("/a/./b", "/a/b"), + ("/a/b/../../c", "/c"), + ("/../..", "/"), + ]; + + for (from, want) in cases { + assert_eq!( + normalized(PathBuf::from(from)).unwrap(), + PathBuf::from(want), + "from: {from}" + ); + } } } diff --git a/src/commands/postgres.rs b/src/commands/postgres.rs index 17c11a5..735cf05 100644 --- a/src/commands/postgres.rs +++ b/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::output::note; +use crate::fsops::{create_private_new, remove_file, rename, suffixed}; +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 { + 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,46 +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.to_string_lossy(), &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 => { - 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() - .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 - } - ); + Dump::Gzip => match gzip_kind(ctx, db, file) { + Ok(kind) => kind, + // looking inside needs a container to decompress with, and a dry run + // is worth printing with the project down, which is when it is most + // likely to be asked for + Err(e) if ctx.dry_run => { + note!(ctx, "planning for a single database dump: {e:#}"); + Kind::Sql } - - Kind::of(&out.stdout) - } + Err(e) => return Err(e), + }, }; let restore = db.restore_with(kind); @@ -130,7 +195,7 @@ pub fn import(ctx: &Ctx, file: &Path) -> Result<()> { when_ready( ctx, - &db, + db, &DropDb { username: &db.user, dbname: &db.name, @@ -143,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, @@ -152,48 +217,16 @@ pub fn import(ctx: &Ctx, file: &Path) -> Result<()> { )?; } - wait_until_ready(ctx, &db)?; - if ctx.dry_run { - 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 = 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"); - } - } - } - } + wait_until_ready(ctx, db)?; + let restored = restore_dump(ctx, db, dump, kind, file, &remote, &tool, restore); + // 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)?; @@ -202,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, +) -> 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 = 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)?; @@ -229,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); @@ -269,7 +357,9 @@ fn dump_command(db: &Database, format: Format) -> Box { // 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() @@ -284,7 +374,7 @@ fn dump_directory(ctx: &Ctx, db: &Database, target: &Path) -> Result<()> { .in_container(&db.container) .run(ctx)?; - let copied = Cp::out_of_container(&db.container, &remote, &target.to_string_lossy()).run(ctx); + let copied = Cp::out_of_container(&db.container, &remote, target).run(ctx); let _ = Rm::recursive(&remote).in_container(&db.container).run(ctx); diff --git a/src/commands/postgres/server.rs b/src/commands/postgres/server.rs index 7e840cd..0f92b73 100644 --- a/src/commands/postgres/server.rs +++ b/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 = ""; pub(super) struct Database { pub(super) service: String, @@ -22,13 +24,24 @@ impl Database { pub(super) fn resolve(ctx: &Ctx) -> Result { 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() { - return Err(anyhow!("service {service} has no running container")); - } + let container = match (ids.next(), ids.next()) { + (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, diff --git a/src/commands/postgres/shape.rs b/src/commands/postgres/shape.rs index 7508fc4..5add983 100644 --- a/src/commands/postgres/shape.rs +++ b/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> { - let mut file = File::open(path)?; - let mut header = vec![0; HEADER_LEN]; + let file = File::open(path)?; + let mut header = Vec::with_capacity(HEADER_LEN); - let read = file.read(&mut header)?; - header.truncate(read); + // read fills what it can rather than what was asked for, and a header that + // 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) } diff --git a/src/commands/status.rs b/src/commands/status.rs index 0da1340..05651e2 100644 --- a/src/commands/status.rs +++ b/src/commands/status.rs @@ -69,14 +69,20 @@ fn role(ctx: &Ctx, role: &str, detected: Result, project: &Project) { } if role == "postgres" { - let (user, database) = project.postgres_credentials(&service); - let source = |key: &str| match project.env(&service, key) { - Some(_) => "", - None => " (default, nothing in the environment)", - }; + // status reports what it cannot work out rather than stopping, so a name + // the postgres tools would refuse is something to say, not to fail on + match project.postgres_credentials(&service) { + 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!("\tdatabase: {database}{}", source("POSTGRES_DB")); + line!("\tuser: {user}{}", source("POSTGRES_USER")); + line!("\tdatabase: {database}{}", source("POSTGRES_DB")); + } + Err(e) => line!("\tcredentials: {e:#}"), + } } } diff --git a/src/fsops.rs b/src/fsops.rs index 9edbd98..c97bd63 100644 --- a/src/fsops.rs +++ b/src/fsops.rs @@ -1,7 +1,8 @@ use std::io::Write; +use std::os::unix::fs::{DirBuilderExt, OpenOptionsExt}; use std::path::{Path, PathBuf}; -use anyhow::{Context, Result}; +use anyhow::{Context, Result, anyhow}; // std::fs with the path and the operation already in the error use fs_err as fs; use fs_err::os::unix::fs::symlink; @@ -47,14 +48,65 @@ pub fn place_link(ctx: &Ctx, link_path: &Path, target: &Path) -> Result<()> { return Ok(()); } + // the parent can be missing when the store holds a path the repository does + // not have any more, and symlink would only report the bare ENOENT + ensure_parent(ctx, link_path)?; + // symlink under a temp name and rename over the path: the rename is atomic let tmp = suffixed(link_path, ".ahab-tmp"); - let _ = fs::remove_file(&tmp); + + // only ahab's own leftover is cleared away: anything else here belongs to + // the project, and silently unlinking it would lose it + match fs::symlink_metadata(&tmp) { + Ok(meta) if meta.is_symlink() => fs::remove_file(&tmp)?, + Ok(_) => { + return Err(anyhow!( + "{} is in the way and is not a symlink ahab left behind; \ + move it aside", + tmp.display() + )); + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => return Err(e.into()), + } symlink(target, &tmp)?; 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::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 +pub fn ensure_private_parent(ctx: &Ctx, base: &Path, target: &Path) -> Result<()> { + if ctx.dry_run { + return Ok(()); + } + + let Some(parent) = target.parent().filter(|dir| dir.starts_with(base)) else { + return Ok(()); + }; + + // a recursive create applies the mode to every directory it makes + std::fs::DirBuilder::new() + .recursive(true) + .mode(0o700) + .create(parent) + .with_context(|| format!("creating {}", parent.display())) +} + pub fn remove_file(ctx: &Ctx, path: &Path) -> Result<()> { if ctx.dry_run { return Ok(()); 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..14086f7 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,79 @@ 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}")); + } +} + +// a path on unix is bytes, not text, so a record naming one is written as bytes +pub(crate) fn write_bytes(bytes: &[u8]) { + if writable() { + finish(io::stdout().write_all(bytes)); + } +} + +// 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}; diff --git a/src/project.rs b/src/project.rs index 239df00..124c936 100644 --- a/src/project.rs +++ b/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::>() + .join(", ") )), } } @@ -82,13 +95,16 @@ impl Project { } } - pub fn postgres_credentials(&self, service: &str) -> (String, String) { - let service = &self.services[service]; + pub fn postgres_credentials(&self, service: &str) -> Result<(String, String)> { + let entry = &self.services[service]; - let user = env_var(service, "POSTGRES_USER").unwrap_or_else(|| "db".to_string()); - let database = env_var(service, "POSTGRES_DB").unwrap_or_else(|| "db".to_string()); + let user = env_var(entry, "POSTGRES_USER").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 { .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 { + 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")); } }