diff --git a/src/cli.rs b/src/cli.rs index 5292808..ae5abf2 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -30,6 +30,26 @@ pub enum Command { #[command(subcommand)] command: PatchCommand, }, + /// Check the tree, the scripts, the mail setup and the paths + Doctor, + /// Point the config at a kernel tree, verifying or cloning one + Init(InitArgs), +} + +#[derive(Debug, Args)] +pub struct InitArgs { + /// A kernel tree you already have + #[arg(long, value_name = "PATH")] + pub tree: Option, + /// Clone a tree from here instead + #[arg(long, value_name = "URL")] + pub clone: Option, + /// Clone only this many commits deep + #[arg(long = "depth", value_name = "N", requires = "clone")] + pub depth: Option, + /// Overwrite a config that already points somewhere else + #[arg(long)] + pub force: bool, } #[derive(Debug, Subcommand)] @@ -71,11 +91,11 @@ pub enum PatchCommand { Format(FormatArgs), /// Commit the work in progress with a kernel-style message Commit(CommitArgs), - /// Write the diff against the base branch out to a .patch file + /// Write the change out as a patch file, or as a series for a range Create(CreateArgs), - /// Send a patch, with To/CC taken from get_maintainer.pl + /// Send a patch or a series, with To/CC taken from get_maintainer.pl Submit(SubmitArgs), - /// Re-roll a patch as vN, renaming the file to match + /// Re-roll a patch, or a whole series, as vN Update(UpdateArgs), } @@ -114,22 +134,28 @@ pub struct CommitArgs { #[derive(Debug, Args)] pub struct CreateArgs { - /// Patch name; `.patch` is appended if you leave it off + /// Patch name, or the series directory name when --range is given #[arg(value_name = "NAME")] - pub name: String, + pub name: Option, /// Branch or revision to diff against #[arg(long, value_name = "REV", default_value = "master")] pub base: String, - /// Directory to write the patch into; defaults to the patch dir + /// Directory to write into; defaults to the patch dir #[arg(long, value_name = "DIR")] pub out: Option, + /// Write a series for these commits, e.g. HEAD~3..HEAD + #[arg(long, value_name = "RANGE")] + pub range: Option, + /// Cover letter text; its first line becomes the subject + #[arg(long, value_name = "BLURB")] + pub cover_letter: Option, } #[derive(Debug, Args)] pub struct SubmitArgs { - /// Patch file to send - #[arg(value_name = "PATCH")] - pub patch: PathBuf, + /// Patch file to send, or a directory holding a series + #[arg(value_name = "PATCH", num_args = 1..)] + pub patches: Vec, /// Print the recipients and command instead of sending #[arg(long)] pub dry_run: bool, @@ -143,7 +169,7 @@ pub struct SubmitArgs { #[derive(Debug, Args)] pub struct UpdateArgs { - /// Patch file to re-roll + /// Patch file to re-roll, or the directory holding a series #[arg(value_name = "PATCH")] pub patch: PathBuf, /// Revision to re-roll as; defaults to one past the file's current vN diff --git a/src/config.rs b/src/config.rs index f43a0ee..49c187d 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,48 +1,130 @@ -//! Where the kernel tree and the patches live. +//! Where the kernel tree and the patches live, and the file that can say so. //! -//! Everything else asks this module for paths, so adding a config file or a -//! `spectral init` only touches this file. - -#![allow(dead_code)] // accessors are read once the patch verbs stop being stubs +//! Everything else asks this module for paths, so the config file and +//! `spectral init` touch nothing else. The precedence is the one the roadmap +//! promised: the environment wins over the file, and the file wins over the +//! built-in default. use std::env; use std::path::{Path, PathBuf}; +use std::process::Command; -use crate::error::{Error, Result}; +use serde::{Deserialize, Serialize}; + +use crate::cli::InitArgs; +use crate::error::{Error, Result, exit_code}; /// Environment variable that points at the kernel checkout. pub const ENV_KERNEL_TREE: &str = "SPECTRAL_KERNEL"; -/// Tree used when that variable is unset, relative to `$HOME`. +/// Environment variable that points at the patch directory. +pub const ENV_PATCH_DIR: &str = "SPECTRAL_PATCH_DIR"; + +/// Tree used when nothing else says otherwise, relative to `$HOME`. const DEFAULT_KERNEL_TREE: &str = ".spectral/linux"; -/// Where written patches land, relative to `$HOME`. +/// Where written patches land by default, relative to `$HOME`. const DEFAULT_PATCH_DIR: &str = ".spectral/patches"; +/// The file, relative to the config directory. +const CONFIG_FILE: &str = "spectral/config.toml"; + +/// What the config file holds. +/// +/// Unknown keys are refused rather than ignored: a mistyped key that silently +/// does nothing is worse than one that stops the run and says so. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct ConfigFile { + /// Where the kernel tree is. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub kernel_tree: Option, + + /// Where written patches go. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub patch_dir: Option, + + /// Addresses every patch is copied to, on top of what get_maintainer says. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub always_cc: Vec, + + /// Who a patch is sent as. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub send_email: Option, +} + +/// The `[send_email]` table. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct SendEmail { + /// `git send-email --from`, when it should differ from git's own identity. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub from: Option, +} + +/// Where a resolved path came from. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Origin { + /// One of the two environment variables. + Environment(&'static str), + /// The config file. + File, + /// The built-in default, relative to `$HOME`. + Default, +} + +impl Origin { + /// How the report names it. + #[must_use] + pub fn describe(self) -> String { + match self { + Self::Environment(variable) => format!("from ${variable}"), + Self::File => "from the config file".to_owned(), + Self::Default => "the built-in default".to_owned(), + } + } +} + #[derive(Debug, Clone)] pub struct Config { kernel_tree: PathBuf, patch_dir: PathBuf, + tree_origin: Origin, + patch_origin: Origin, + file: ConfigFile, + file_path: PathBuf, } impl Config { - /// Resolve the tree from `$SPECTRAL_KERNEL`, falling back to - /// `~/.spectral/linux`. + /// Resolve both paths: environment first, then the config file, then the + /// default under `$HOME`. /// /// The tree is not checked for existence here. Commands that need it call /// [`Config::require_kernel_tree`], which leaves `kernel quest` working /// before anything is cloned. - /// - /// TODO: also read `~/.config/spectral/config.toml` once `spectral init` - /// exists. The environment variable should keep winning over the file. pub fn load() -> Result { let home = home_dir(); - let kernel_tree = env::var_os(ENV_KERNEL_TREE) - .map_or_else(|| home.join(DEFAULT_KERNEL_TREE), PathBuf::from); + let file_path = config_path(&home); + let file = read_file(&file_path)?; + + let (kernel_tree, tree_origin) = resolve( + ENV_KERNEL_TREE, + file.kernel_tree.clone(), + home.join(DEFAULT_KERNEL_TREE), + )?; + let (patch_dir, patch_origin) = resolve( + ENV_PATCH_DIR, + file.patch_dir.clone(), + home.join(DEFAULT_PATCH_DIR), + )?; Ok(Self { kernel_tree, - patch_dir: home.join(DEFAULT_PATCH_DIR), + patch_dir, + tree_origin, + patch_origin, + file, + file_path, }) } @@ -56,6 +138,42 @@ impl Config { &self.patch_dir } + /// Where the tree path came from, for `doctor` to print. + /// + /// A path that was resolved for the user and silently turns out to be the + /// wrong one is a worse report than one that says which knob set it. + #[must_use] + pub fn kernel_tree_origin(&self) -> String { + self.tree_origin.describe() + } + + /// Where the patch directory came from, for `doctor` to print. + #[must_use] + pub fn patch_dir_origin(&self) -> String { + self.patch_origin.describe() + } + + /// The identity a patch is sent as, when the config file names one. + #[must_use] + pub fn send_email_from(&self) -> Option { + self.file + .send_email + .as_ref() + .and_then(|send_email| send_email.from.clone()) + } + + /// Addresses every patch is copied to. + #[must_use] + pub fn always_cc(&self) -> &[String] { + &self.file.always_cc + } + + /// The config file this run read, whether or not it was there. + #[must_use] + pub fn file_path(&self) -> &Path { + &self.file_path + } + /// Resolve the tree and check that it really is a kernel source tree. pub fn require_kernel_tree(&self) -> Result<&Path> { if self.kernel_tree.join("scripts/checkpatch.pl").is_file() { @@ -73,6 +191,177 @@ impl Config { } } +/// What `spectral init` did. +#[derive(Debug, Clone)] +pub struct Init { + /// The file that was written, or that already said this. + pub config_file: PathBuf, + /// The tree the config now names. + pub kernel_tree: PathBuf, + /// Whether anything was written. + pub wrote: bool, +} + +/// `spectral init`: point the config at a tree, verifying or cloning one. +/// +/// Idempotent on purpose. A second run that would change nothing says so and +/// writes nothing; a run that would change an existing answer is refused until +/// `--force` says it meant it. +pub fn init(args: &InitArgs) -> Result { + let home = home_dir(); + let config_file = config_path(&home); + let current = read_file(&config_file)?; + + let tree = match (&args.tree, &args.clone) { + // The destination is named first: the refusal below has to happen + // before a clone runs, not after one has already landed. + (tree, Some(_)) => tree + .clone() + .unwrap_or_else(|| home.join(DEFAULT_KERNEL_TREE)), + (Some(tree), None) => tree.clone(), + (None, None) => Config::load()?.kernel_tree().to_path_buf(), + }; + + let mut next = current.clone(); + next.kernel_tree = Some(tree.clone()); + let changed = next != current; + if changed && config_file.is_file() && !args.force { + return Err(Error::ConfigSaysSomethingElse { path: config_file }); + } + + if let Some(url) = &args.clone { + clone_tree(url, &tree, args.depth)?; + } + verify_tree(&tree)?; + + if changed { + write_file(&config_file, &next)?; + } + + Ok(Init { + config_file, + kernel_tree: tree, + wrote: changed, + }) +} + +/// Clone `url` into `into`. +/// +/// A tree already sitting there is left alone: `init` run twice should not +/// re-clone anything. +fn clone_tree(url: &str, into: &Path, depth: Option) -> Result<()> { + if into.join("scripts/checkpatch.pl").is_file() { + return Ok(()); + } + if let Some(parent) = into.parent() { + std::fs::create_dir_all(parent)?; + } + + let mut args = vec!["clone".to_owned()]; + if let Some(depth) = depth { + args.push("--depth".to_owned()); + args.push(depth.to_string()); + } + args.push(url.to_owned()); + args.push(into.display().to_string()); + + let output = + Command::new("git") + .args(&args) + .output() + .map_err(|source| Error::CommandSpawn { + program: "git clone".to_owned(), + source, + })?; + if !output.status.success() { + return Err(Error::ExternalCommand { + program: "git".to_owned(), + args: args.join(" "), + code: exit_code(&output.status), + stderr: String::from_utf8_lossy(&output.stderr) + .trim_end() + .to_owned(), + }); + } + + Ok(()) +} + +/// Check that a path really is a kernel source tree. +fn verify_tree(tree: &Path) -> Result<()> { + if tree.join("scripts/checkpatch.pl").is_file() { + Ok(()) + } else if tree.exists() { + Err(Error::NotAKernelTree { + path: tree.to_path_buf(), + }) + } else { + Err(Error::KernelTreeMissing { + path: tree.to_path_buf(), + env: ENV_KERNEL_TREE, + }) + } +} + +/// Read the config file, or hand back an empty one when it is not there. +fn read_file(path: &Path) -> Result { + let text = match std::fs::read_to_string(path) { + Ok(text) => text, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + return Ok(ConfigFile::default()); + } + Err(error) => return Err(Error::Io(error)), + }; + + toml::from_str(&text).map_err(|source| Error::ConfigUnreadable { + path: path.to_path_buf(), + source, + }) +} + +/// Write the config file, making its directory first. +fn write_file(path: &Path, file: &ConfigFile) -> Result<()> { + let text = toml::to_string(file).map_err(|source| Error::ConfigUnwritable { source })?; + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + } + std::fs::write(path, text)?; + + Ok(()) +} + +/// Where the config file lives: `$XDG_CONFIG_HOME/spectral/config.toml`, or +/// `~/.config/spectral/config.toml` when that variable is unset. +#[must_use] +pub fn config_path(home: &Path) -> PathBuf { + env::var_os("XDG_CONFIG_HOME") + .map_or_else(|| home.join(".config"), PathBuf::from) + .join(CONFIG_FILE) +} + +/// Pick one path out of the environment, the file, and the default. +/// +/// A relative path in the file is refused rather than resolved against +/// whichever directory the command happened to run in. +fn resolve( + variable: &'static str, + from_file: Option, + fallback: PathBuf, +) -> Result<(PathBuf, Origin)> { + if let Some(value) = env::var_os(variable) { + return Ok((PathBuf::from(value), Origin::Environment(variable))); + } + if let Some(path) = from_file { + if !path.is_absolute() { + return Err(Error::ConfigPathRelative { variable, path }); + } + + return Ok((path, Origin::File)); + } + + Ok((fallback, Origin::Default)) +} + fn home_dir() -> PathBuf { env::var_os("HOME").map_or_else(|| PathBuf::from("."), PathBuf::from) } diff --git a/src/doctor.rs b/src/doctor.rs new file mode 100644 index 0000000..c9799d6 --- /dev/null +++ b/src/doctor.rs @@ -0,0 +1,436 @@ +//! `spectral doctor`: what is missing before the loop can run. +//! +//! The checks run in the order a first attempt hits them, and the summary names +//! the first hard failure, because that is the one worth fixing first. A +//! warning is something spectral works around or reports without stopping: a +//! host with no SMTP route still exits 0, because `--dry-run` is what the +//! README tells a reader to trust. + +use std::io::Write as _; +use std::path::{Path, PathBuf}; +use std::process::Command; + +use crate::config::Config; + +/// How one check ended. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Verdict { + /// Anything that needs it can go ahead. + Ok, + /// Worth knowing, not worth stopping for. + Warn, + /// Something spectral cannot work without. + Fail, + /// Not looked at, because something it depends on failed first. + Skipped, +} + +impl Verdict { + fn label(self) -> &'static str { + match self { + Self::Ok => "ok", + Self::Warn => "warn", + Self::Fail => "fail", + Self::Skipped => "skip", + } + } + + fn is_failure(self) -> bool { + matches!(self, Self::Fail) + } +} + +/// One check and what it found. +#[derive(Debug, Clone)] +pub struct Check { + /// Short name, so the summary can name the one that failed. + pub name: &'static str, + pub verdict: Verdict, + pub detail: String, + /// What to do about it, printed under the line. + pub fix: Option, +} + +impl Check { + fn ok(name: &'static str, detail: impl Into) -> Self { + Self { + name, + verdict: Verdict::Ok, + detail: detail.into(), + fix: None, + } + } + + fn warn(name: &'static str, detail: impl Into, fix: impl Into) -> Self { + Self { + name, + verdict: Verdict::Warn, + detail: detail.into(), + fix: Some(fix.into()), + } + } + + fn fail(name: &'static str, detail: impl Into, fix: impl Into) -> Self { + Self { + name, + verdict: Verdict::Fail, + detail: detail.into(), + fix: Some(fix.into()), + } + } + + fn skipped(name: &'static str, detail: impl Into) -> Self { + Self { + name, + verdict: Verdict::Skipped, + detail: detail.into(), + fix: None, + } + } +} + +/// Everything the checks found. +#[derive(Debug, Clone)] +pub struct Report { + pub checks: Vec, +} + +impl Report { + /// Whether anything hard failed. Warnings do not count. + #[must_use] + pub fn ok(&self) -> bool { + !self.checks.iter().any(|check| check.verdict.is_failure()) + } + + /// The first hard failure, which is the one worth naming. + #[must_use] + pub fn first_failure(&self) -> Option<&Check> { + self.checks.iter().find(|check| check.verdict.is_failure()) + } + + fn count(&self, verdict: Verdict) -> usize { + self.checks + .iter() + .filter(|check| check.verdict == verdict) + .count() + } + + /// The report as `main` prints it: one line per check, the fix under it, + /// and a summary that names the first failure. + #[must_use] + pub fn render(&self) -> String { + let mut out = String::new(); + for check in &self.checks { + out.push_str(&format!( + "{:<4} {:<23} {}\n", + check.verdict.label(), + check.name, + check.detail + )); + if let Some(fix) = &check.fix { + out.push_str(&format!(" {:<23} -> {fix}\n", "")); + } + } + + let failures = self.count(Verdict::Fail); + let warnings = self.count(Verdict::Warn); + match self.first_failure() { + Some(first) => out.push_str(&format!( + "doctor: {failures} failed, {warnings} warned; first failure: {}\n", + first.name + )), + None if warnings > 0 => { + out.push_str(&format!("doctor: nothing failed, {warnings} warned\n")); + } + None => out.push_str("doctor: everything checked out\n"), + } + + out + } +} + +/// Run every check, in the order a first attempt hits them. +#[must_use] +pub fn run(config: &Config) -> Report { + let tree = config.require_kernel_tree().ok().map(Path::to_path_buf); + let checks = vec![ + tree_path(config), + tree_shape(config), + tree_file( + tree.as_deref(), + "scripts/checkpatch.pl", + "checkpatch.pl", + "the only thing that decides kernel style", + ), + tree_file( + tree.as_deref(), + "scripts/get_maintainer.pl", + "get_maintainer.pl", + "the only thing that knows who to mail", + ), + tree_file( + tree.as_deref(), + "MAINTAINERS", + "MAINTAINERS", + "the maintainer map the script reads", + ), + git_runs(), + send_email_invocable(), + sender_identity(config, tree.as_deref()), + mail_route(tree.as_deref()), + qemu_on_path(), + patch_dir_writable(config), + config_file(config), + ]; + + Report { checks } +} + +/// Whether a config file is there, and where it is. +/// +/// A warning: everything works without one, and the paths in the report above +/// say where the settings came from. The file is what `spectral init` writes, +/// so a missing one is a pointer rather than a problem. +fn config_file(config: &Config) -> Check { + let path = config.file_path(); + if path.is_file() { + Check::ok("config file", path.display().to_string()) + } else { + Check::warn( + "config file", + format!( + "no config file at {}; the paths above are defaults", + path.display() + ), + "run `spectral init --tree ` to write one", + ) + } +} + +/// Where spectral thinks the tree is, and where it got that from. +fn tree_path(config: &Config) -> Check { + let path = config.kernel_tree(); + if path.is_dir() { + Check::ok( + "kernel tree", + format!("{} ({})", path.display(), config.kernel_tree_origin()), + ) + } else { + Check::fail( + "kernel tree", + format!("{} ({})", path.display(), config.kernel_tree_origin()), + "clone a tree there, or point the path at one you have", + ) + } +} + +/// Whether the path really is a kernel source tree. +fn tree_shape(config: &Config) -> Check { + if !config.kernel_tree().exists() { + return Check::skipped( + "kernel tree shape", + "not looked at: there is nothing at that path", + ); + } + + match config.require_kernel_tree() { + Ok(_) => Check::ok("kernel tree shape", "scripts/checkpatch.pl is there"), + Err(error) => Check::fail( + "kernel tree shape", + error.to_string(), + "point the tree path at a kernel checkout", + ), + } +} + +/// One file inside the tree. +fn tree_file(tree: Option<&Path>, rel: &'static str, name: &'static str, why: &str) -> Check { + let Some(tree) = tree else { + return Check::skipped(name, "not looked at: there is no tree"); + }; + let path = tree.join(rel); + + if path.is_file() { + Check::ok(name, format!("{rel} ({why})")) + } else { + Check::fail( + name, + format!("no {rel} in {}", tree.display()), + "use a full kernel checkout, not a build directory", + ) + } +} + +/// Whether git runs at all. +fn git_runs() -> Check { + match Command::new("git").arg("--version").output() { + Ok(output) if output.status.success() => { + let version = String::from_utf8_lossy(&output.stdout).trim().to_owned(); + Check::ok("git", version) + } + Ok(output) => Check::fail( + "git", + format!( + "`git --version` failed ({})", + crate::error::exit_code(&output.status) + ), + "fix git on this machine before spectral can do anything", + ), + Err(error) => Check::fail("git", format!("could not run git: {error}"), "install git"), + } +} + +/// Whether `git send-email` is installed. +/// +/// `--dump-aliases` is the cheapest call that proves the script runs: it reads +/// the same config send-email would and needs no patch and no mail host. +fn send_email_invocable() -> Check { + let output = Command::new("git") + .arg("send-email") + .arg("--dump-aliases") + .output(); + + match output { + Ok(output) if output.status.success() => Check::ok("git send-email", "runs"), + Ok(output) => Check::fail( + "git send-email", + format!( + "`git send-email --dump-aliases` failed ({}): {}", + crate::error::exit_code(&output.status), + String::from_utf8_lossy(&output.stderr).trim_end() + ), + "install git's send-email support (on Arch: git is not enough on its own)", + ), + Err(error) => Check::fail( + "git send-email", + format!("could not run git send-email: {error}"), + "install git", + ), + } +} + +/// Who a patch is sent as. +/// +/// A hard failure: `git send-email` refuses a patch with no sender, and finding +/// that out at the end of the loop is the worst time. +fn sender_identity(config: &Config, tree: Option<&Path>) -> Check { + if let Some(from) = config.send_email_from() { + return Check::ok("sender identity", format!("{from} (from the config file)")); + } + let Some(tree) = tree else { + return Check::skipped("sender identity", "not looked at: there is no tree"); + }; + let email = git_config(tree, "user.email"); + + match email { + Some(email) if !email.trim().is_empty() => Check::ok("sender identity", email), + _ => Check::fail( + "sender identity", + "git has no user.email in this tree", + "set `git config --global user.email you@example.com`, or an identity in the config file", + ), + } +} + +/// Whether anything would carry mail out. +/// +/// A warning, not a failure: this host has no `sendemail.*` config at all and +/// still runs `git send-email --dry-run` to completion, which is the half of +/// the loop spectral itself relies on. +fn mail_route(tree: Option<&Path>) -> Check { + let Some(tree) = tree else { + return Check::skipped("mail route", "not looked at: there is no tree"); + }; + let configured = Command::new("git") + .arg("-C") + .arg(tree) + .args(["config", "--get-regexp", r"^sendemail\."]) + .output(); + + match configured { + Ok(output) if !String::from_utf8_lossy(&output.stdout).trim().is_empty() => { + Check::ok("mail route", "sendemail.* is configured") + } + _ => Check::warn( + "mail route", + "no sendemail.* config: --dry-run works, a real send will not", + "configure `git send-email` before dropping --dry-run", + ), + } +} + +/// Whether qemu is there to boot with. +/// +/// A warning: `kernel test` cannot run without it, and nothing else cares. +fn qemu_on_path() -> Check { + match Command::new("qemu-system-x86_64").arg("--version").output() { + Ok(output) if output.status.success() => { + let version = String::from_utf8_lossy(&output.stdout) + .lines() + .next() + .unwrap_or_default() + .trim() + .to_owned(); + Check::ok("qemu", version) + } + _ => Check::warn( + "qemu", + "qemu-system-x86_64 is not on $PATH, so `kernel test` cannot boot anything", + "install qemu (qemu-system-x86 on Arch)", + ), + } +} + +/// Whether patches can be written where spectral wants them. +/// +/// This is the one check with a side effect: it makes the directory when it is +/// missing, which is what the patch verbs would do anyway, and says which of +/// the two happened. +fn patch_dir_writable(config: &Config) -> Check { + let dir: PathBuf = config.patch_dir().to_path_buf(); + let existed = dir.is_dir(); + if let Err(error) = std::fs::create_dir_all(&dir) { + return Check::fail( + "patch directory", + format!("{} cannot be created: {error}", dir.display()), + "point the patch directory somewhere writable", + ); + } + + let probe = dir.join(format!(".spectral-doctor-{}", std::process::id())); + let wrote = std::fs::File::create(&probe).and_then(|mut file| file.write_all(b"probe\n")); + let _ = std::fs::remove_file(&probe); + + let state = if existed { "exists" } else { "created" }; + match wrote { + Ok(()) => Check::ok( + "patch directory", + format!("{} ({state}, {})", dir.display(), config.patch_dir_origin()), + ), + Err(error) => Check::fail( + "patch directory", + format!("{} is not writable: {error}", dir.display()), + "point the patch directory somewhere writable", + ), + } +} + +/// One key out of git's configuration, or nothing when it is unset. +fn git_config(tree: &Path, key: &str) -> Option { + let output = Command::new("git") + .arg("-C") + .arg(tree) + .args(["config", key]) + .output() + .ok()?; + + if output.status.success() { + Some( + String::from_utf8_lossy(&output.stdout) + .trim_end() + .to_owned(), + ) + } else { + None + } +} diff --git a/src/error.rs b/src/error.rs index 1c06e64..2ce2cd9 100644 --- a/src/error.rs +++ b/src/error.rs @@ -10,10 +10,24 @@ use std::process::ExitStatus; pub type Result = std::result::Result; /// How a failed process is described in [`Error::ExternalCommand`]. +/// +/// A process killed by a signal has no exit code, and the bare word "signal" +/// throws away which one: a SIGTERM from a `timeout` and a SIGSEGV from the +/// program under test are different answers to why it stopped. pub(crate) fn exit_code(status: &ExitStatus) -> String { - status - .code() - .map_or_else(|| "signal".to_owned(), |code| code.to_string()) + if let Some(code) = status.code() { + return code.to_string(); + } + + #[cfg(unix)] + { + use std::os::unix::process::ExitStatusExt as _; + if let Some(signal) = status.signal() { + return format!("signal {signal}"); + } + } + + "a signal".to_owned() } #[derive(Debug, thiserror::Error)] @@ -36,13 +50,21 @@ pub enum Error { }, /// An external tool could not even be started. - #[error("could not run `{program}`")] + #[error("could not run `{program}`: {source}")] CommandSpawn { program: String, #[source] source: std::io::Error, }, + /// A patch file the user named could not be read. + #[error("`{path}` could not be read: {source}")] + PatchUnreadable { + path: PathBuf, + #[source] + source: std::io::Error, + }, + /// A patch file the user named does not exist. #[error("no patch named `{name}` in `{dir}`")] PatchNotFound { name: String, dir: PathBuf }, @@ -57,7 +79,7 @@ pub enum Error { /// One patch file was asked to carry a whole set. #[error( - "`{base}..HEAD` holds {count} commits; one patch file cannot carry a set (patch series support is not built yet)" + "`{base}..HEAD` holds {count} commits; one file cannot carry a set, so write it as a series with `--range {base}..HEAD`" )] RangeHoldsSeveralCommits { base: String, count: usize }, @@ -75,11 +97,120 @@ pub enum Error { #[error("`{path}` already exists; delete it or pick another revision")] RerollWouldOverwrite { path: PathBuf }, + /// Bugzilla answered, but with something that is not a bug list. + #[error("bugzilla answered with something that is not a bug list: {source}")] + BugzillaNotABugList { + #[source] + source: serde_json::Error, + }, + + /// Bugzilla answered with an empty set of open bugs. + #[error("bugzilla reports no open bugs at all; try again later")] + BugzillaEmpty, + + /// `--filter` matched nothing, which is a dead end rather than a pick. + #[error("no open bug matched `{filter}`; drop --filter or try another word")] + NoQuestCandidates { filter: String }, + + /// The tree built, or was expected to have built, and holds no image. + #[error("no bootable image at `{path}`; build the tree first or drop --no-build")] + ImageMissing { path: PathBuf }, + + /// `make` ran and failed. + #[error("`make` failed (exit {code}); the tree did not build")] + BuildFailed { code: String }, + + /// The thread the build was moved onto did not come back. + #[error("the build stopped before it finished: {source}")] + BuildTask { + #[source] + source: tokio::task::JoinError, + }, + + /// `patch create` was given neither a name nor a range. + #[error("`patch create` wants a NAME, or --range for a series")] + CreateNeedsName, + + /// `--range` was given something that is not a range. + #[error("`{range}` is not a range; `--range` wants `..`, e.g. HEAD~3..HEAD")] + RangeNotARange { range: String }, + + /// `--cover-letter` without a series to put it in. + #[error("`--cover-letter` belongs to a series; add `--range ..`")] + CoverLetterNeedsSeries, + + /// A blurb with nothing in it would leave the template subject behind. + #[error("the cover letter blurb is empty; give it a first line, which becomes the subject")] + CoverLetterBlurbEmpty, + + /// format-patch wrote no cover letter where one was expected. + #[error("no cover letter was written into `{dir}`; a cover letter was asked for")] + CoverLetterMissing { dir: PathBuf }, + + /// A directory was used as a series but carries no sidecar. + #[error("`{path}` holds no series.toml; create the series with `patch create --range`")] + SeriesManifestMissing { path: PathBuf }, + + /// The sidecar is there and cannot be read. + #[error("`{path}` is not a readable series manifest: {source}")] + SeriesManifestUnreadable { + path: PathBuf, + #[source] + source: toml::de::Error, + }, + + /// The sidecar could not be written. + #[error("the series manifest could not be written: {source}")] + SeriesManifestUnwritable { + #[source] + source: toml::ser::Error, + }, + + /// A superseded revision would not go, so the directory holds two of them. + #[error( + "these files could not be removed: {names}; `submit` on the directory would send both revisions" + )] + StaleSeriesFiles { names: String }, + + /// A directory was handed to `submit` and holds no patches. + #[error("`{dir}` holds no `.patch` files")] + NoPatchesInDirectory { dir: PathBuf }, + + /// `git format-patch` was asked for a set and the range held nothing. + #[error("`{range}` holds no commits; a series needs at least one")] + RangeHoldsNoCommits { range: String }, + + /// The config file is there and is not readable as TOML. + #[error("`{path}` is not a readable config file: {source}")] + ConfigUnreadable { + path: PathBuf, + #[source] + source: toml::de::Error, + }, + + /// The config file could not be written. + #[error("the config file could not be written: {source}")] + ConfigUnwritable { + #[source] + source: toml::ser::Error, + }, + + /// A path in the config file would resolve against the working directory. + #[error( + "`{path}` in the config file is not an absolute path; ${variable} and the default are, so this one is refused" + )] + ConfigPathRelative { + variable: &'static str, + path: PathBuf, + }, + + /// The config already answers the question `init` was asked. + #[error("`{path}` already points somewhere else; pass --force to overwrite it")] + ConfigSaysSomethingElse { path: PathBuf }, + #[error(transparent)] Io(#[from] std::io::Error), - // Reached once `kernel quest` stops being a stub. - #[allow(dead_code)] #[error(transparent)] Http(#[from] reqwest::Error), } diff --git a/src/git.rs b/src/git.rs index 2394e5b..d7897e3 100644 --- a/src/git.rs +++ b/src/git.rs @@ -67,9 +67,7 @@ impl Git { program: "git".to_owned(), args: format!("apply {}", args.join(" ")), code: exit_code(&output.status), - stderr: String::from_utf8_lossy(&output.stderr) - .trim_end() - .to_owned(), + stderr: told_off(&output), }); } @@ -149,12 +147,49 @@ impl Git { Ok(()) } - /// Resolve a revision to a commit hash. + /// Resolve a revision, or `None` when there is no such revision. /// - /// Callers want the hash: a branch name can move under a command that is - /// about to build a range out of it. - pub fn rev_parse(&self, rev: &str) -> Result { - self.run(&["rev-parse", "--verify", "--quiet", rev]) + /// A revision that does not resolve and a repository that is not there are + /// both failures to git, and they are not the same answer: git says so on + /// stderr when the repository is missing, and `--quiet` keeps a missing + /// revision silent. Reading that difference is what keeps "not a git + /// repository" from being reported as "no revision `master`". + pub fn rev_parse_optional(&self, rev: &str) -> Result> { + let args = ["rev-parse", "--verify", "--quiet", rev]; + let output = self.attempt(&args)?; + if output.status.success() { + return Ok(Some( + String::from_utf8_lossy(&output.stdout) + .trim_end() + .to_owned(), + )); + } + if output.stdout.is_empty() && output.stderr.is_empty() { + return Ok(None); + } + + Err(Error::ExternalCommand { + program: "git".to_owned(), + args: args.join(" "), + code: exit_code(&output.status), + stderr: told_off(&output), + }) + } + + /// Run git and hand back what happened, failure included. + /// + /// The one caller is [`Git::rev_parse_optional`], which has to look at a + /// non-zero exit rather than turn it into an error immediately. + fn attempt(&self, args: &[&str]) -> Result { + Command::new("git") + .arg("-C") + .arg(&self.repo) + .args(args) + .output() + .map_err(|source| Error::CommandSpawn { + program: format!("git {}", args.join(" ")), + source, + }) } fn output(&self, args: &[&str]) -> Result { @@ -173,12 +208,26 @@ impl Git { program: "git".to_owned(), args: args.join(" "), code: exit_code(&output.status), - stderr: String::from_utf8_lossy(&output.stderr) - .trim_end() - .to_owned(), + stderr: told_off(&output), }); } Ok(output) } } + +/// What a failed git said, wherever it said it. +/// +/// git puts some failures on stdout rather than stderr. `git commit` with +/// nothing staged explains itself there, and a reader handed an empty message +/// cannot tell what went wrong. +fn told_off(output: &std::process::Output) -> String { + for stream in [&output.stderr, &output.stdout] { + let said = String::from_utf8_lossy(stream); + if !said.trim().is_empty() { + return said.trim_end().to_owned(); + } + } + + "nothing was captured".to_owned() +} diff --git a/src/kernel/qemu.rs b/src/kernel/qemu.rs index a3a45c6..a640714 100644 --- a/src/kernel/qemu.rs +++ b/src/kernel/qemu.rs @@ -1,13 +1,26 @@ //! `spectral kernel test`: build the tree, then boot it under qemu. -#![allow(dead_code)] // nothing is reachable until `run` stops being a stub - use std::path::{Path, PathBuf}; -use std::process::ExitStatus; +use std::process::{Command, ExitStatus, Stdio}; + +use tokio::io::{AsyncBufReadExt as _, BufReader}; +use tokio::process::Command as AsyncCommand; use crate::cli::TestArgs; use crate::config::Config; -use crate::error::Result; +use crate::error::{Error, Result, exit_code}; + +/// Where an x86 kernel leaves the image qemu boots. +const IMAGE: &str = "arch/x86/boot/bzImage"; + +/// The console the kernel is told to use. +/// +/// It is the one that comes back on a terminal, which is the difference +/// between a boot hang someone can read and one they cannot. +const CONSOLE: &str = "console=ttyS0"; + +/// The emulator, by the name the README's prerequisites list. +const QEMU: &str = "qemu-system-x86_64"; /// One qemu run. #[derive(Debug, Clone)] @@ -20,25 +33,219 @@ pub struct Boot { pub extra_args: Vec, } +/// What one `kernel test` did. +/// +/// The status is carried rather than flattened into a `()`: a boot that failed +/// must not print the same thing as one that succeeded. +#[derive(Debug, Clone)] +pub struct Outcome { + /// The image qemu was given. + pub image: PathBuf, + /// How qemu ended. A kernel that panics and stays running ends in a signal + /// rather than in an exit code, and that is worth reporting as itself. + pub status: ExitStatus, +} + +impl Outcome { + /// Whether qemu ended well. + #[must_use] + pub fn succeeded(&self) -> bool { + self.status.success() + } +} + /// Build the tree and hand back the path to the bootable image. +/// +/// `make` inherits the terminal, because a kernel build that says nothing for +/// ten minutes is indistinguishable from one that has hung. pub fn build(kernel_tree: &Path) -> Result { - todo!( - "make -j$(nproc) in {}, then locate arch/x86/boot/bzImage", - kernel_tree.display() - ) + let jobs = std::thread::available_parallelism().map_or(1, std::num::NonZero::get); + let status = Command::new("make") + .arg(format!("-j{jobs}")) + .current_dir(kernel_tree) + .status() + .map_err(|source| Error::CommandSpawn { + program: "make".to_owned(), + source, + })?; + + if !status.success() { + return Err(Error::BuildFailed { + code: exit_code(&status), + }); + } + + existing_image(kernel_tree) +} + +/// The qemu arguments for one boot, without the emulator itself. +/// +/// Split out so the shape of the command line is a pure function a test can +/// assert on, which a spawned qemu is not. +fn boot_args(boot: &Boot) -> Vec { + let mut args = vec![ + "-kernel".to_owned(), + boot.kernel.display().to_string(), + "-append".to_owned(), + CONSOLE.to_owned(), + "-nographic".to_owned(), + ]; + + if let Some(rootfs) = &boot.rootfs { + args.push("-initrd".to_owned()); + args.push(rootfs.display().to_string()); + } + args.extend(boot.extra_args.iter().cloned()); + + args } /// Boot an image and wait for qemu to exit. +/// +/// The serial console is streamed line by line rather than buffered until qemu +/// exits: a boot that hangs halfway is exactly the case where the output that +/// arrived is the whole diagnosis. pub async fn boot(kernel_tree: &Path, boot: &Boot) -> Result { - // TODO: qemu-system-x86_64 -kernel {boot.kernel} -append "console=ttyS0" \ - // -nographic, with -initrd when `rootfs` is set and `extra_args` on - // the end. Stream serial output instead of buffering it. - let _ = (kernel_tree, boot); - todo!("boot under qemu and stream the console") + let mut child = AsyncCommand::new(QEMU) + .current_dir(kernel_tree) + .args(boot_args(boot)) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + // A qemu left behind by a failed run would hold the image and the + // terminal, so it goes when this does. + .kill_on_drop(true) + .spawn() + .map_err(|source| Error::CommandSpawn { + program: QEMU.to_owned(), + source, + })?; + + let console = child + .stdout + .take() + .map(|pipe| tokio::spawn(drain(pipe, false))); + let complaints = child + .stderr + .take() + .map(|pipe| tokio::spawn(drain(pipe, true))); + let status = child.wait().await?; + if let Some(console) = console { + let _ = console.await; + } + if let Some(complaints) = complaints { + let _ = complaints.await; + } + + Ok(status) } /// `spectral kernel test`: build unless told not to, then boot. -pub async fn run(config: &Config, args: TestArgs) -> Result<()> { - let _ = (config, args); - todo!("build + boot") +pub async fn run(config: &Config, args: TestArgs) -> Result { + let tree = config.require_kernel_tree()?.to_path_buf(); + let image = if args.no_build { + existing_image(&tree)? + } else { + // A kernel build blocks for minutes, and blocking the async runtime + // for that long stalls everything else it is holding. + let building = tree.clone(); + tokio::task::spawn_blocking(move || build(&building)) + .await + .map_err(|source| Error::BuildTask { source })?? + }; + + let planned = Boot { + kernel: image.clone(), + rootfs: args.rootfs, + extra_args: args.qemu_args, + }; + let status = boot(&tree, &planned).await?; + + Ok(Outcome { image, status }) +} + +/// The image a `--no-build` run was asked to boot. +fn existing_image(kernel_tree: &Path) -> Result { + let image = kernel_tree.join(IMAGE); + + if image.is_file() { + Ok(image) + } else { + Err(Error::ImageMissing { path: image }) + } +} + +/// Copy one of qemu's pipes to this process's own, line by line. +/// +/// A read that fails ends the stream with a note rather than silently: the +/// last lines of a console are often the reason the run is being read at all. +async fn drain(pipe: R, to_stderr: bool) +where + R: tokio::io::AsyncRead + Unpin, +{ + let mut lines = BufReader::new(pipe).lines(); + + loop { + match lines.next_line().await { + Ok(Some(line)) if to_stderr => eprintln!("{line}"), + Ok(Some(line)) => println!("{line}"), + Ok(None) => break, + Err(error) => { + eprintln!("spectral: qemu's console stopped early: {error}"); + break; + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn boot(rootfs: Option<&str>, extra_args: &[&str]) -> Boot { + Boot { + kernel: PathBuf::from("/tree/arch/x86/boot/bzImage"), + rootfs: rootfs.map(PathBuf::from), + extra_args: extra_args.iter().map(|arg| (*arg).to_owned()).collect(), + } + } + + #[test] + fn a_boot_names_the_image_and_the_serial_console() { + assert_eq!( + boot_args(&boot(None, &[])), + vec![ + "-kernel", + "/tree/arch/x86/boot/bzImage", + "-append", + "console=ttyS0", + "-nographic", + ] + ); + } + + #[test] + fn a_rootfs_becomes_an_initrd_after_the_kernel() { + assert_eq!( + boot_args(&boot(Some("/img/initramfs.img"), &[])), + vec![ + "-kernel", + "/tree/arch/x86/boot/bzImage", + "-append", + "console=ttyS0", + "-nographic", + "-initrd", + "/img/initramfs.img", + ] + ); + } + + #[test] + fn extra_arguments_go_on_the_end_verbatim() { + assert_eq!( + boot_args(&boot(None, &["-smp", "2", "-m", "1G"]))[5..], + ["-smp", "2", "-m", "1G"], + "the pass-through arguments are the caller's, in the order it gave them" + ); + } } diff --git a/src/kernel/quest.rs b/src/kernel/quest.rs index 7f7e8b5..76e5d5a 100644 --- a/src/kernel/quest.rs +++ b/src/kernel/quest.rs @@ -2,14 +2,29 @@ //! //! The source sits behind [`QuestSource`], so adding a local TODO file or a //! syzbot scraper later means one new impl and no change to the command. - -#![allow(dead_code)] // nothing is reachable until `run` stops being a stub +//! +//! The data comes from bugzilla.kernel.org's REST API rather than its HTML: +//! `buglist.cgi` caps at 200 rows and would have to be paginated, and the CSV +//! export needs a quote-aware parser for nothing JSON does not already give. use std::future::Future; +use std::time::{Duration, SystemTime, UNIX_EPOCH}; + +use serde::Deserialize; use crate::cli::QuestArgs; use crate::config::Config; -use crate::error::Result; +use crate::error::{Error, Result}; + +/// The bug fields the REST call asks for, which are the ones an [`Issue`] +/// needs. `limit=0` means everything rather than nothing. +const INCLUDE_FIELDS: &str = "id,summary,product,component"; + +/// How long bugzilla gets before the command gives up. +/// +/// Without one, a route that drops packets rather than refusing the connection +/// hangs the command with nothing on screen and no way out but Ctrl-C. +const REQUEST_TIMEOUT: Duration = Duration::from_secs(30); /// One piece of work, normalised across whatever source it came from. #[derive(Debug, Clone)] @@ -23,6 +38,31 @@ pub struct Issue { pub url: String, } +impl Issue { + /// What `main` prints: the JSON form when `--json` asked for it, otherwise + /// the readable one. + #[must_use] + pub fn render(&self, json: bool) -> String { + if json { + let value = serde_json::json!({ + "id": &self.id, + "title": &self.title, + "component": &self.component, + "url": &self.url, + }); + if let Ok(rendered) = serde_json::to_string_pretty(&value) { + return rendered; + } + } + + let component = self.component.as_deref().unwrap_or("unspecified"); + format!( + "{} {}\ncomponent: {component}\n{}", + self.id, self.title, self.url + ) + } +} + /// Anything spectral can pull open work from. /// /// Written as `impl Future` rather than a bare `async fn` so the `Send` bound @@ -46,29 +86,311 @@ impl Default for Bugzilla { } } +impl Bugzilla { + /// The REST call that returns every open bug and the four fields wanted. + fn open_bugs_url(&self) -> String { + format!( + "{}/rest/bug?status=NEW&limit=0&include_fields={INCLUDE_FIELDS}", + self.base_url.trim_end_matches('/') + ) + } + + /// Where a human reads the bug. + fn issue_url(&self, id: u64) -> String { + format!( + "{}/show_bug.cgi?id={id}", + self.base_url.trim_end_matches('/') + ) + } +} + impl QuestSource for Bugzilla { async fn fetch_open(&self) -> Result> { - // TODO: GET `{base_url}/buglist.cgi` with status=NEW/ASSIGNED/REOPENED, - // walk the paginated result tables, and pull id + summary + product out - // of each row with `scraper`. - let _ = &self.base_url; - todo!("scrape open bugs from bugzilla.kernel.org") + let body = reqwest::Client::builder() + .timeout(REQUEST_TIMEOUT) + .build()? + .get(self.open_bugs_url()) + .send() + .await? + .text() + .await?; + + parse_open_bugs(self, &body) } } +/// What the REST call answered with, in the shape it answered with it. +#[derive(Debug, Deserialize)] +struct BugList { + bugs: Vec, +} + +/// One bug, with only the fields that were asked for. +#[derive(Debug, Deserialize)] +struct Bug { + id: u64, + summary: String, + product: Option, + component: Option, +} + +/// Read a `/rest/bug` answer into issues. +/// +/// Kept separate from the call so a recorded answer can be asserted against +/// without a network: what a live bugzilla returns is not something a test can +/// pin, but the shape is. +fn parse_open_bugs(source: &Bugzilla, body: &str) -> Result> { + let list: BugList = + serde_json::from_str(body).map_err(|error| Error::BugzillaNotABugList { source: error })?; + + Ok(list + .bugs + .into_iter() + .map(|bug| { + // Read the shared fields before `summary` moves out of the bug. + let component = place(&bug); + let url = source.issue_url(bug.id); + + Issue { + id: bug.id.to_string(), + title: bug.summary, + component, + url, + } + }) + .collect()) +} + +/// Where a bug lives, as far as the API says: `product/component` when it +/// names both, whichever one it names otherwise. +fn place(bug: &Bug) -> Option { + match (&bug.product, &bug.component) { + (Some(product), Some(component)) => Some(format!("{product}/{component}")), + (Some(only), None) | (None, Some(only)) => Some(only.clone()), + (None, None) => None, + } +} + +/// Whether an issue answers to a `--filter` word. +/// +/// The title or the component containing it is a match, case-insensitively: +/// the words someone remembers from a bug are rarely the ones with the right +/// capitalisation. +fn matches(issue: &Issue, filter: &str) -> bool { + let needle = filter.to_lowercase(); + + issue.title.to_lowercase().contains(&needle) + || issue + .component + .as_ref() + .is_some_and(|component| component.to_lowercase().contains(&needle)) +} + /// Fetch the open issues and pick one. /// -/// TODO: apply `args.filter` to the title or component before picking, and -/// print JSON when `args.json` is set. -pub async fn run(config: &Config, args: QuestArgs) -> Result { - let _ = (config, args); - todo!("pick one open bug at random") +/// The config is deliberately not consulted: a quest is a question about +/// bugzilla, not about a kernel tree, so this verb works before anything has +/// been cloned. +pub async fn run(_config: &Config, args: QuestArgs) -> Result { + let source = Bugzilla::default(); + let mut issues = source.fetch_open().await?; + + if let Some(filter) = &args.filter { + issues.retain(|issue| matches(issue, filter)); + } + + let picked = pick_random(&issues, seed()).ok_or_else(|| match &args.filter { + Some(filter) => Error::NoQuestCandidates { + filter: filter.clone(), + }, + None => Error::BugzillaEmpty, + })?; + + Ok(picked.clone()) +} + +/// A seed that changes between runs, for a pick nobody can rerun by accident. +/// +/// The low bits of the clock are the ones that move, and those are the ones a +/// caller of [`pick_random`] folds through the xorshift. +fn seed() -> u64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .map_or(0, |elapsed| elapsed.as_nanos() as u64) } /// Choose one issue at random. `seed` keeps the pick reproducible in tests. pub fn pick_random(issues: &[Issue], seed: u64) -> Option<&Issue> { - todo!( - "index into {} candidate issues with seed {seed}", - issues.len() - ) + if issues.is_empty() { + return None; + } + + let index = (xorshift(seed) % issues.len() as u64) as usize; + + issues.get(index) +} + +/// One step of xorshift64, so a clock seed becomes a spread-out index without +/// a `rand` dependency. +fn xorshift(mut state: u64) -> u64 { + state ^= state << 13; + state ^= state >> 7; + state ^= state << 17; + + state +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A `/rest/bug` answer, recorded from the live API during planning and + /// trimmed to three bugs: one with both place fields, one with only a + /// component, and one with only a product. + const RECORDED: &str = r#"{ + "bugs": [ + { + "id": 219123, + "summary": "usb: xhci: device does not enumerate after resume", + "product": "Drivers", + "component": "USB" + }, + { + "id": 218001, + "summary": "amdgpu: ring timeout on resume", + "component": "Video(AMDGPU/R600)" + }, + { + "id": 215000, + "summary": "ppc64: build failure with gcc 15", + "product": "Platform Specific/Hardware" + } + ] + }"#; + + fn issues() -> Vec { + parse_open_bugs(&Bugzilla::default(), RECORDED).expect("the recorded answer parses") + } + + #[test] + fn a_recorded_answer_becomes_issues() { + let issues = issues(); + + assert_eq!(issues.len(), 3); + assert_eq!(issues[0].id, "219123"); + assert_eq!( + issues[0].title, + "usb: xhci: device does not enumerate after resume" + ); + assert_eq!( + issues[0].url, + "https://bugzilla.kernel.org/show_bug.cgi?id=219123" + ); + } + + #[test] + fn a_bug_keeps_whichever_place_fields_it_has() { + let issues = issues(); + + assert_eq!(issues[0].component.as_deref(), Some("Drivers/USB")); + assert_eq!( + issues[1].component.as_deref(), + Some("Video(AMDGPU/R600)"), + "a component with no product is still worth showing" + ); + assert_eq!( + issues[2].component.as_deref(), + Some("Platform Specific/Hardware") + ); + } + + #[test] + fn an_answer_that_is_not_a_bug_list_is_a_named_error() { + let error = parse_open_bugs(&Bugzilla::default(), "nope") + .expect_err("html is not JSON"); + + assert!(error.to_string().contains("is not a bug list"), "{error}"); + } + + #[test] + fn an_answer_with_no_bugs_parses_as_none() { + assert_eq!( + parse_open_bugs(&Bugzilla::default(), r#"{"bugs": []}"#) + .expect("an empty list") + .len(), + 0 + ); + } + + #[test] + fn a_filter_matches_the_title_or_the_component_whatever_the_case() { + let issues = issues(); + + assert!(matches(&issues[0], "xhci")); + assert!(matches(&issues[0], "XHCI")); + assert!(matches(&issues[1], "amdgpu")); + assert!(matches(&issues[2], "platform specific")); + assert!(!matches(&issues[0], "amdgpu")); + } + + #[test] + fn the_same_seed_picks_the_same_issue() { + let issues = issues(); + + for seed in [0, 1, 7, 42, 12345, u64::MAX] { + let first = pick_random(&issues, seed).expect("a pick"); + let again = pick_random(&issues, seed).expect("the same pick"); + + assert_eq!(first.id, again.id, "seed {seed} was not stable"); + } + } + + #[test] + fn there_is_nothing_to_pick_from_an_empty_list() { + assert!(pick_random(&[], 1).is_none()); + } + + #[test] + fn the_seed_spreads_across_the_list() { + // Not a distribution assertion: only that consecutive seeds do not all + // land on the same issue, which is what folding nothing would do, and that + // every pick is one of the candidates. + let issues = issues(); + let picked: std::collections::HashSet<&str> = (0..64) + .filter_map(|seed| pick_random(&issues, seed).map(|issue| issue.id.as_str())) + .collect(); + + assert!(picked.len() > 1, "every seed picked the same issue"); + assert!( + picked + .iter() + .all(|id| issues.iter().any(|issue| issue.id == *id)) + ); + } + + #[test] + fn the_json_form_is_json_and_holds_every_field() { + let rendered = issues()[0].render(true); + let parsed: serde_json::Value = + serde_json::from_str(&rendered).expect("the JSON form parses"); + + assert_eq!(parsed["id"], "219123"); + assert_eq!(parsed["component"], "Drivers/USB"); + assert_eq!( + parsed["url"], + "https://bugzilla.kernel.org/show_bug.cgi?id=219123" + ); + assert!(parsed["title"].as_str().is_some_and(|t| t.contains("xhci"))); + } + + #[test] + fn the_readable_form_names_the_component_and_the_url() { + let rendered = issues()[1].render(false); + + assert!( + rendered.contains("component: Video(AMDGPU/R600)"), + "{rendered}" + ); + assert!(rendered.contains("show_bug.cgi?id=218001"), "{rendered}"); + } } diff --git a/src/main.rs b/src/main.rs index 94008aa..73d1f6b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -6,6 +6,7 @@ mod cli; mod config; +mod doctor; mod error; mod git; mod kernel; @@ -40,14 +41,27 @@ async fn run(cli: Cli) -> Result { match cli.command { Command::Kernel { command } => match command { KernelCommand::Quest(args) => { + let json = args.json; let issue = kernel::quest::run(&config, args).await?; - println!("{} {}", issue.id, issue.title); - println!("{}", issue.url); + println!("{}", issue.render(json)); Ok(ExitCode::SUCCESS) } - KernelCommand::Test(args) => kernel::qemu::run(&config, args) - .await - .map(|()| ExitCode::SUCCESS), + KernelCommand::Test(args) => { + let outcome = kernel::qemu::run(&config, args).await?; + println!("image: {}", outcome.image.display()); + if outcome.succeeded() { + println!("qemu exited cleanly"); + Ok(ExitCode::SUCCESS) + } else { + // A boot that failed is not a crash of spectral, so this + // reads as a result rather than as an error. + println!( + "qemu exited with status {}", + crate::error::exit_code(&outcome.status) + ); + Ok(ExitCode::FAILURE) + } + } }, Command::Patch { command } => match command { @@ -89,6 +103,27 @@ async fn run(cli: Cli) -> Result { Ok(ExitCode::SUCCESS) } }, + + Command::Doctor => { + let report = doctor::run(&config); + print!("{}", report.render()); + Ok(if report.ok() { + ExitCode::SUCCESS + } else { + ExitCode::FAILURE + }) + } + + Command::Init(args) => { + let init = config::init(&args)?; + if init.wrote { + println!("wrote {}", init.config_file.display()); + } else { + println!("{} already says this", init.config_file.display()); + } + println!("tree: {}", init.kernel_tree.display()); + Ok(ExitCode::SUCCESS) + } } } diff --git a/src/patch/maintainers.rs b/src/patch/maintainers.rs index dc25494..6127368 100644 --- a/src/patch/maintainers.rs +++ b/src/patch/maintainers.rs @@ -26,29 +26,42 @@ pub struct Recipients { pub touched: Vec, } -/// Look up recipients for a patch. +/// The arguments one lookup runs with. /// -/// One invocation over the patch. `--no-tree` because the script's own tree -/// check wants a dozen marker files a working tree need not have, and -/// `--pattern-depth=0` so a file matches every `F:` entry covering it rather -/// than only the closest one. -pub fn lookup(kernel_tree: &Path, patch: &Path) -> Result { - let patch_arg = patch.to_string_lossy().into_owned(); - let output = run( - kernel_tree, - &[ - "--no-tree", - "--git", - "--roles", - "--no-rolestats", - "--pattern-depth=0", - &patch_arg, - ], - )?; +/// `--no-tree` because the script's own tree check wants a dozen marker files a +/// working tree need not have, and `--pattern-depth=0` so a file matches every +/// `F:` entry covering it rather than only the closest one. +const LOOKUP_ARGS: [&str; 5] = [ + "--no-tree", + "--git", + "--roles", + "--no-rolestats", + "--pattern-depth=0", +]; + +/// Look up recipients for a patch, or for every patch in a set. +/// +/// One invocation over all the files: a series has one recipient list, and +/// asking per patch would only repeat the same answer. +pub fn lookup(kernel_tree: &Path, patches: &[PathBuf]) -> Result { + let mut args: Vec = LOOKUP_ARGS.iter().map(|arg| (*arg).to_owned()).collect(); + for patch in patches { + args.push(patch.to_string_lossy().into_owned()); + } + let borrowed: Vec<&str> = args.iter().map(String::as_str).collect(); + let output = run(kernel_tree, &borrowed)?; let entries: Vec = output.lines().filter_map(parse_entry).collect(); + let mut touched: Vec = Vec::new(); + for patch in patches { + for file in touched_files(patch)? { + if !touched.contains(&file) { + touched.push(file); + } + } + } - Ok(split(&entries, touched_files(patch)?)) + Ok(split(&entries, touched)) } /// Add extra addresses to the CC list, skipping ones already there. @@ -164,9 +177,12 @@ fn parse_entry(line: &str) -> Option { /// Paths git felt the need to quote are skipped rather than mangled. They are /// for reporting only; the lookup itself reads the patch. fn touched_files(patch: &Path) -> Result> { - let patch = std::fs::read(patch)?; + let patch_bytes = std::fs::read(patch).map_err(|source| Error::PatchUnreadable { + path: patch.to_path_buf(), + source, + })?; - Ok(String::from_utf8_lossy(&patch) + Ok(String::from_utf8_lossy(&patch_bytes) .lines() .filter_map(|line| line.strip_prefix("diff --git ")) .filter_map(|rest| rest.rsplit_once(" b/")) diff --git a/src/patch/mod.rs b/src/patch/mod.rs index a14d26d..0f43881 100644 --- a/src/patch/mod.rs +++ b/src/patch/mod.rs @@ -15,6 +15,7 @@ pub mod checkpatch; pub mod maintainers; +pub mod series; use std::path::{Path, PathBuf}; @@ -62,17 +63,20 @@ pub fn commit(config: &Config, args: CommitArgs) -> Result<()> { /// Write the change against `--base` out as a patch file. /// -/// The name is normalised to end in `.patch` and the file lands in -/// [`Config::patch_dir`] unless `--out` says otherwise. Returns the path -/// written. +/// One file, one commit: a range holding more than one commit is refused and +/// pointed at `--range`, because a single file cannot carry a set. pub fn create(config: &Config, args: CreateArgs) -> Result { + if args.cover_letter.is_some() && args.range.is_none() { + return Err(Error::CoverLetterNeedsSeries); + } + if let Some(range) = args.range.clone() { + return create_series(config, &args, &range); + } + + let name = args.name.clone().ok_or(Error::CreateNeedsName)?; let tree = config.require_kernel_tree()?; let git = Git::new(tree); - let base = git - .rev_parse(&args.base) - .map_err(|_| Error::UnknownRevision { - rev: args.base.clone(), - })?; + let base = resolve_revision(&git, &args.base)?; // The patch is mail-formatted rather than a bare diff: `git send-email` // refuses the bare one with "No subject line". @@ -90,12 +94,95 @@ pub fn create(config: &Config, args: CreateArgs) -> Result { let dir = args.out.unwrap_or_else(|| config.patch_dir().to_path_buf()); std::fs::create_dir_all(&dir)?; - let path = dir.join(patch_name(&args.name)); + let path = dir.join(patch_name(&name)); std::fs::write(&path, set)?; Ok(path) } +/// Write a whole set, with a cover letter, into a directory of its own. +/// +/// The directory carries a sidecar saying what was generated and at which +/// revision, which is what lets `patch update` re-roll the set without the +/// range being retyped. +fn create_series(config: &Config, args: &CreateArgs, range: &str) -> Result { + // Checked before anything is written: a blurb with no first line would + // otherwise leave generated files on disk with no sidecar beside them. + if let Some(blurb) = &args.cover_letter + && blurb.trim().is_empty() + { + return Err(Error::CoverLetterBlurbEmpty); + } + + let tree = config.require_kernel_tree()?; + let git = Git::new(tree); + let resolved = resolve_range(&git, range)?; + let (base, head) = resolved.split_once("..").map_or_else( + || (resolved.clone(), resolved.clone()), + |(base, head)| (base.to_owned(), head.to_owned()), + ); + let dir = series_dir(config, args); + std::fs::create_dir_all(&dir)?; + + let files = series::generate(&git, &dir, &resolved, 1, args.cover_letter.as_deref())?; + series::Series { + base, + head, + version: 1, + files, + cover_letter: args.cover_letter.clone(), + } + .write(&dir)?; + + Ok(dir) +} + +/// The directory a series is written into. +/// +/// `NAME` names a subdirectory of the patch directory, which keeps a set from +/// mixing with the single patches beside it. Without a name the patch +/// directory itself is the series directory. +fn series_dir(config: &Config, args: &CreateArgs) -> PathBuf { + match (&args.out, &args.name) { + (Some(out), Some(name)) => out.join(name), + (Some(out), None) => out.clone(), + (None, Some(name)) => config.patch_dir().join(name), + (None, None) => config.patch_dir().to_path_buf(), + } +} + +/// Turn `..` into hashes. +/// +/// Both ends are resolved before format-patch runs: a branch that moves +/// between resolving and generating would otherwise change which commits the +/// set holds. +fn resolve_range(git: &Git, range: &str) -> Result { + let (base, head) = range + .split_once("..") + .ok_or_else(|| Error::RangeNotARange { + range: range.to_owned(), + })?; + let head = head.strip_prefix('.').unwrap_or(head); + + let base = if base.is_empty() { "HEAD" } else { base }; + let head = if head.is_empty() { "HEAD" } else { head }; + let base_hash = resolve_revision(git, base)?; + let head_hash = resolve_revision(git, head)?; + + Ok(format!("{base_hash}..{head_hash}")) +} + +/// One end of a range, or the named error saying which revision was missing. +/// +/// A tree that is not a git repository is reported as itself rather than as a +/// missing revision: the two failures want different fixes. +fn resolve_revision(git: &Git, rev: &str) -> Result { + git.rev_parse_optional(rev)? + .ok_or_else(|| Error::UnknownRevision { + rev: rev.to_owned(), + }) +} + /// What a `submit` run decided, so `main` can print it. #[derive(Debug, Clone)] pub struct Submission { @@ -108,7 +195,7 @@ pub struct Submission { pub output: String, } -/// Send a patch to whoever `get_maintainer.pl` names. +/// Send a patch, or a whole series, to whoever `get_maintainer.pl` names. /// /// With `--dry-run` nothing leaves the machine: send-email prints the plan and /// nothing else happens. send-email's own refusals, a cover letter still @@ -116,10 +203,13 @@ pub struct Submission { /// swallowed, because the exit code is what a script around this reads. pub fn submit(config: &Config, args: SubmitArgs) -> Result { let tree = config.require_kernel_tree()?; - let patch = resolve_patch(config, &args.patch)?; + let patches = resolve_patches(config, &args.patches)?; - let mut recipients = maintainers::lookup(tree, &patch)?; - maintainers::add_cc(&mut recipients, &args.cc); + let mut recipients = maintainers::lookup(tree, &patches)?; + // `--cc` on the command line, then whatever the config file always copies. + let mut extras: Vec = args.cc.clone(); + extras.extend(config.always_cc().iter().cloned()); + maintainers::add_cc(&mut recipients, &extras); let mut argv: Vec = vec!["send-email".to_owned()]; if args.dry_run { @@ -128,6 +218,10 @@ pub fn submit(config: &Config, args: SubmitArgs) -> Result { // an answer to send-email's confirmation question. argv.push("--confirm=never".to_owned()); } + if let Some(from) = config.send_email_from() { + argv.push("--from".to_owned()); + argv.push(from); + } for address in &recipients.to { argv.push("--to".to_owned()); argv.push(address.clone()); @@ -140,7 +234,11 @@ pub fn submit(config: &Config, args: SubmitArgs) -> Result { argv.push("--in-reply-to".to_owned()); argv.push(message_id.clone()); } - argv.push(patch.to_string_lossy().into_owned()); + // A set goes out in one invocation, which is what threads the patches + // together and what send-email already knows how to do. + for patch in &patches { + argv.push(patch.to_string_lossy().into_owned()); + } let borrowed: Vec<&str> = argv.iter().map(String::as_str).collect(); let git = Git::new(tree); @@ -153,33 +251,93 @@ pub fn submit(config: &Config, args: SubmitArgs) -> Result { Ok(Submission { recipients, - command: format!("git -C {} {}", tree.display(), shell_join(&argv)), + command: format!( + "git -C {} {}", + shell_quote(&tree.display().to_string()), + shell_join(&argv) + ), output, }) } -/// A command line a reader can paste, with the awkward arguments quoted. +/// The patch files to send, in the order they will be sent. +/// +/// A directory stands for a series: its sidecar decides the order when there +/// is one, and otherwise the patch names sorted, which is the order +/// format-patch numbered them in. +fn resolve_patches(config: &Config, names: &[PathBuf]) -> Result> { + let mut files: Vec = Vec::new(); + + for name in names { + let resolved = resolve_patch(config, name)?; + if !resolved.is_dir() { + files.push(resolved); + continue; + } + + let inside = match series::Series::read(&resolved) { + Ok(series) => series.paths(&resolved), + Err(Error::SeriesManifestMissing { .. }) => series::patches_in(&resolved)? + .into_iter() + .map(|name| resolved.join(name)) + .collect(), + Err(other) => return Err(other), + }; + if inside.is_empty() { + return Err(Error::NoPatchesInDirectory { dir: resolved }); + } + files.extend(inside); + } + + Ok(files) +} + +/// A command line a reader can paste. +/// +/// Quoting an argument only when it holds a space is not enough to make that +/// true: `--in-reply-to ` is a redirection in bash, and an address +/// with an apostrophe swallows the opening quote and arrives mangled. Anything +/// outside a conservative safe set is single-quoted, and an apostrophe inside is +/// escaped the way a shell reads it. fn shell_join(argv: &[String]) -> String { argv.iter() - .map(|arg| { - if arg.contains(' ') { - format!("'{arg}'") - } else { - arg.clone() - } - }) + .map(|arg| shell_quote(arg)) .collect::>() .join(" ") } -/// Re-roll a patch as `v`. +/// One argument, quoted if a shell would otherwise read something into it. +fn shell_quote(arg: &str) -> String { + let plain = !arg.is_empty() + && arg.bytes().all(|byte| { + byte.is_ascii_alphanumeric() + || matches!( + byte, + b'/' | b'.' | b'-' | b'_' | b'+' | b':' | b'=' | b'@' | b',' + ) + }); + + if plain { + arg.to_owned() + } else { + format!("'{}'", arg.replace('\'', r"'\''")) + } +} + +/// Re-roll a patch as `v`, or a whole series when handed a directory. /// -/// `000-kernel-patch.patch` becomes `v2-000-kernel-patch.patch`; an existing -/// `vN-` prefix is replaced rather than stacked, so re-running it at the same -/// revision is a no-op. `args.revision` overrides the inferred next number. -/// Returns the path of the renamed file. +/// A single patch is renamed: `000-kernel-patch.patch` becomes +/// `v2-000-kernel-patch.patch`, and an existing `vN-` prefix is replaced +/// rather than stacked, so re-running it at the same revision is a no-op. +/// A directory is regenerated through `git format-patch --reroll-count`, so +/// every file in it moves to the same revision. `args.revision` overrides the +/// inferred next number. Returns the path of what moved. pub fn update(config: &Config, args: UpdateArgs) -> Result { let patch = resolve_patch(config, &args.patch)?; + if patch.is_dir() { + return update_series(config, &patch, args.revision); + } + let revision = match args.revision { Some(revision) => revision, None => current_revision(&patch) + 1, @@ -198,6 +356,37 @@ pub fn update(config: &Config, args: UpdateArgs) -> Result { Ok(renamed) } +/// Re-roll every file in a series directory to `revision`. +/// +/// The range comes out of the sidecar rather than being rebuilt as `base..HEAD`: +/// a commit that landed on the branch after `create` is a different set, and +/// picking it up silently would mail commits nobody asked to send. The +/// superseded revision is removed only once the new one is entirely on disk, so +/// a re-roll that fails halfway leaves the previous revision usable. +fn update_series(config: &Config, dir: &Path, revision: Option) -> Result { + let series = series::Series::read(dir)?; + let version = revision.unwrap_or(series.version + 1); + if version == series.version { + return Ok(dir.to_path_buf()); + } + + let tree = config.require_kernel_tree()?; + let git = Git::new(tree); + let range = format!("{}..{}", series.base, series.head); + let files = series::generate(&git, dir, &range, version, series.cover_letter.as_deref())?; + series::remove_superseded(dir, &series.files, &files)?; + series::Series { + base: series.base, + head: series.head, + version, + files, + cover_letter: series.cover_letter, + } + .write(dir)?; + + Ok(dir.to_path_buf()) +} + /// The path `patch` should have once it is revision `revision`. /// /// Kept separate from [`update`] because it is the rule worth having a test @@ -229,11 +418,13 @@ fn strip_revision(name: &str) -> &str { /// Find the patch the user named. /// -/// A path that is there is used as given. Anything else is looked for in the -/// patch directory, which is what the README's examples assume: `patch submit -/// 000-xhci-port-rearm.patch` from anywhere, without the path in front. +/// A path that is there is used as given, a directory included: a series is a +/// directory, and `submit` and `update` both take one. Anything else is looked +/// for in the patch directory, which is what the README's examples assume: +/// `patch submit 000-xhci-port-rearm.patch` from anywhere, without the path in +/// front. fn resolve_patch(config: &Config, name: &Path) -> Result { - if name.is_file() { + if name.is_file() || name.is_dir() { return Ok(name.to_path_buf()); } @@ -242,7 +433,7 @@ fn resolve_patch(config: &Config, name: &Path) -> Result { |name| name.to_string_lossy().into_owned(), ); let in_patch_dir = config.patch_dir().join(&file_name); - if in_patch_dir.is_file() { + if in_patch_dir.is_file() || in_patch_dir.is_dir() { return Ok(in_patch_dir); } diff --git a/src/patch/series.rs b/src/patch/series.rs new file mode 100644 index 0000000..21405cb --- /dev/null +++ b/src/patch/series.rs @@ -0,0 +1,222 @@ +//! Patch sets: what a series directory is, and the `git format-patch` calls +//! that write one. +//! +//! The naming is not spectral's: `git format-patch` already numbers the files, +//! prefixes a re-roll with `vN`, and writes the cover letter. What is spectral's +//! is the sidecar that lets a re-roll know what it is re-rolling, and the +//! refusal to leave two revisions in one directory. + +use std::path::{Path, PathBuf}; + +use serde::{Deserialize, Serialize}; + +use crate::error::{Error, Result}; +use crate::git::Git; + +/// The file a series directory carries. +pub const MANIFEST: &str = "series.toml"; + +/// The placeholder git leaves in a cover letter's subject line. +const SUBJECT_TEMPLATE: &str = "*** SUBJECT HERE ***"; + +/// The placeholder git leaves where the blurb belongs. +const BLURB_TEMPLATE: &str = "*** BLURB HERE ***"; + +/// What a series directory holds. +/// +/// Minimal on purpose: the range, the revision, the files, and the cover letter +/// blurb. Where sent patches are tracked afterwards is a later design question +/// and is deliberately not answered here. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Series { + /// Commit the set was generated against, resolved to a hash so a branch + /// moving under a re-roll cannot change what the range means. + pub base: String, + /// The other end of that range, also a hash. + /// + /// Without it a re-roll would regenerate `base..HEAD`, which quietly picks + /// up every commit that landed on the branch since `create` and mails it + /// with the set. + pub head: String, + /// The revision every file in the directory is at. + pub version: u32, + /// The files, in the order they are sent: cover letter first. + pub files: Vec, + /// The cover letter blurb, kept so a re-roll does not throw it away and + /// regenerate the template that `git send-email` refuses. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub cover_letter: Option, +} + +impl Series { + /// Read the sidecar out of a series directory. + pub fn read(dir: &Path) -> Result { + let path = dir.join(MANIFEST); + let text = match std::fs::read_to_string(&path) { + Ok(text) => text, + // Only "there is no manifest here" means "this is not a series". + // Anything else is a real read failure and is reported as one. + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + return Err(Error::SeriesManifestMissing { path }); + } + Err(error) => return Err(Error::Io(error)), + }; + + toml::from_str(&text).map_err(|source| Error::SeriesManifestUnreadable { path, source }) + } + + /// Write the sidecar into a series directory. + pub fn write(&self, dir: &Path) -> Result<()> { + let text = + toml::to_string(self).map_err(|source| Error::SeriesManifestUnwritable { source })?; + std::fs::write(dir.join(MANIFEST), text)?; + + Ok(()) + } + + /// The patches this series holds, in the order they are sent. + #[must_use] + pub fn paths(&self, dir: &Path) -> Vec { + self.files.iter().map(|name| dir.join(name)).collect() + } +} + +/// Generate the set for `range` into `dir`, and return the file names written. +/// +/// `version` above 1 goes through `--reroll-count`, which is what puts the same +/// `vN-` prefix on every file including the cover letter. The names come from +/// what format-patch says it wrote rather than from listing the directory: on a +/// re-roll the directory still holds the revision being replaced, and a listing +/// would read those back as part of the new set. +pub fn generate( + git: &Git, + dir: &Path, + range: &str, + version: u32, + cover_letter: Option<&str>, +) -> Result> { + let mut args = vec!["format-patch".to_owned(), "--cover-letter".to_owned()]; + if version > 1 { + args.push(format!("--reroll-count={version}")); + } + args.push("-o".to_owned()); + args.push(dir.display().to_string()); + args.push(range.to_owned()); + + let borrowed: Vec<&str> = args.iter().map(String::as_str).collect(); + let written = git.run(&borrowed)?; + let files: Vec = written + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()) + .map(|line| { + Path::new(line).file_name().map_or_else( + || line.to_owned(), + |name| name.to_string_lossy().into_owned(), + ) + }) + .collect(); + + if files.is_empty() { + return Err(Error::RangeHoldsNoCommits { + range: range.to_owned(), + }); + } + if let Some(blurb) = cover_letter { + fill_cover_letter(dir, &files, blurb)?; + } + + Ok(files) +} + +/// Every patch in `dir`, cover letter first. +/// +/// format-patch numbers the files, so their names sort into the order they are +/// meant to be sent in. +pub fn patches_in(dir: &Path) -> Result> { + let entries = std::fs::read_dir(dir)?; + let mut names: Vec = Vec::new(); + + for entry in entries { + let entry = entry?; + let name = entry.file_name().to_string_lossy().into_owned(); + if name.ends_with(".patch") && entry.path().is_file() { + names.push(name); + } + } + names.sort(); + + Ok(names) +} + +/// Put the blurb into a freshly generated cover letter. +/// +/// git leaves `*** SUBJECT HERE ***` and `*** BLURB HERE ***` behind, and +/// `git send-email` refuses to send a patch carrying either, so a series +/// created without this stays unsendable on purpose: the refusal is louder than +/// a cover letter nobody wrote. +fn fill_cover_letter(dir: &Path, files: &[String], blurb: &str) -> Result<()> { + let Some(cover) = files.iter().find(|name| name.contains("cover-letter")) else { + return Err(Error::CoverLetterMissing { + dir: dir.to_path_buf(), + }); + }; + let path = dir.join(cover); + let original = std::fs::read_to_string(&path)?; + let subject = blurb.lines().next().unwrap_or_default().trim().to_owned(); + if subject.is_empty() { + return Err(Error::CoverLetterBlurbEmpty); + } + + let mut filled = String::with_capacity(original.len() + blurb.len()); + for line in original.lines() { + match line + .strip_prefix("Subject: ") + .and_then(|subject| subject.rsplit_once(SUBJECT_TEMPLATE)) + { + Some((head, _)) => { + filled.push_str("Subject: "); + filled.push_str(head); + filled.push_str(&subject); + } + None => filled.push_str(line), + } + filled.push('\n'); + } + + let filled = filled.replace(BLURB_TEMPLATE, blurb.trim_end()); + std::fs::write(&path, filled)?; + + Ok(()) +} + +/// Delete the files of the revision this one replaces. +/// +/// Called only once every new file is on disk, so a failed re-roll cannot cost +/// the previous revision. A file that is already gone is not a failure, because +/// the directory is then in the state the re-roll wanted; a file that will not +/// go is named rather than swallowed, since two revisions in one directory is +/// what `submit` would send. +pub fn remove_superseded(dir: &Path, old: &[String], new: &[String]) -> Result<()> { + let mut stuck: Vec = Vec::new(); + + for name in old { + if new.contains(name) || name == MANIFEST { + continue; + } + if let Err(error) = std::fs::remove_file(dir.join(name)) + && error.kind() != std::io::ErrorKind::NotFound + { + stuck.push(format!("{name} ({error})")); + } + } + + if stuck.is_empty() { + Ok(()) + } else { + Err(Error::StaleSeriesFiles { + names: stuck.join(", "), + }) + } +} diff --git a/tests/cli_smoke.rs b/tests/cli_smoke.rs index 7689b51..41ace05 100644 --- a/tests/cli_smoke.rs +++ b/tests/cli_smoke.rs @@ -29,6 +29,8 @@ fn every_verb_has_help_of_its_own() { &["patch", "create"], &["patch", "submit"], &["patch", "update"], + &["doctor"], + &["init"], ] { let mut args = verb.to_vec(); args.push("--help"); @@ -67,16 +69,3 @@ fn a_leaf_missing_its_argument_exits_two() { let stderr = String::from_utf8_lossy(&output.stderr); assert!(stderr.contains("MESSAGE"), "{stderr}"); } - -#[test] -fn a_verb_that_is_still_a_stub_panics_where_it_says_it_does() { - // `kernel quest` is P5 and `kernel test` is P6. Until they land, this is - // the proof that the dispatch is wired end to end: exit 101 with the - // panic's own location in it. - let fixture = Fixture::kernel_tree(); - let output = fixture.cli(&["kernel", "quest"]); - - assert_eq!(output.status.code(), Some(101)); - let stderr = String::from_utf8_lossy(&output.stderr); - assert!(stderr.contains("kernel/quest.rs"), "{stderr}"); -} diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 4dd4666..63cb4ed 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -161,6 +161,17 @@ impl Fixture { std::fs::set_permissions(&path, permissions).expect("make the script executable"); } + /// Write an executable script into a directory of its own. + fn write_script_in(&self, dir: &Path, name: &str, body: &str) { + let path = dir.join(name); + std::fs::write(&path, body).expect("write the script"); + let mut permissions = std::fs::metadata(&path) + .expect("stat the script") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&path, permissions).expect("make the script executable"); + } + pub fn read(&self, rel: &str) -> String { std::fs::read_to_string(self.join(rel)).expect("read the file") } @@ -214,6 +225,40 @@ impl Fixture { self.cli_env(args, &[]) } + /// Run the built binary without `$SPECTRAL_KERNEL`, so the config file and + /// the built-in default are the only things left to resolve a tree from. + /// + /// `XDG_CONFIG_HOME` points into the fixture, which is where the test's + /// config file lives. + pub fn cli_without_the_tree_variable(&self, args: &[&str], env: &[(&str, &str)]) -> Output { + let mut command = Command::new(env!("CARGO_BIN_EXE_spectral")); + command + .args(args) + .current_dir(self.root.path()) + .env_clear() + .env("PATH", std::env::var("PATH").unwrap_or_default()) + .env("HOME", self.home.path()) + .env("XDG_CONFIG_HOME", self.home.join("config")) + .env("GIT_CONFIG_GLOBAL", self.home.join("gitconfig")) + .env("GIT_CONFIG_SYSTEM", "/dev/null"); + + for (key, value) in env { + command.env(key, value); + } + + command.output().expect("run spectral") + } + + /// Write a config file into the fixture's own config directory. + pub fn write_config(&self, contents: &str) -> PathBuf { + let path = self.home.join("config/spectral/config.toml"); + std::fs::create_dir_all(path.parent().expect("the config directory")) + .expect("create the config directory"); + std::fs::write(&path, contents).expect("write the config file"); + + path + } + /// Run the built binary with extra environment on top of the fixture's. pub fn cli_env(&self, args: &[&str], env: &[(&str, &str)]) -> Output { let mut command = Command::new(env!("CARGO_BIN_EXE_spectral")); @@ -246,4 +291,52 @@ impl Fixture { command } + + /// A `PATH` holding symlinks to exactly these programs, found on the + /// machine's own path. + /// + /// This is how a test takes a tool away: uninstalling qemu to prove the + /// missing-qemu path would be absurd, and pointing `PATH` somewhere narrow + /// asserts the same thing without touching the host. + pub fn restricted_path(&self, programs: &[&str]) -> PathBuf { + let bin = self.home.join("bin"); + std::fs::create_dir_all(&bin).expect("create the restricted bin directory"); + + for program in programs { + let found = self.program(program); + std::os::unix::fs::symlink(found, bin.join(program)).expect("link the program"); + } + + bin + } + + /// A `PATH` whose `git` is the machine's, except that `send-email` answers + /// with `answer` instead of running. + /// + /// The harness controls `PATH` and git's configuration but not git's own + /// exec-path, so whether the host has git's send-email support installed is + /// otherwise inherited from the machine. This makes the answer the test's, + /// in both directions. + pub fn path_with_send_email(&self, answer: u32) -> PathBuf { + let bin = self.home.join("gitbin"); + std::fs::create_dir_all(&bin).expect("create the git bin directory"); + let real = self.program("git"); + let wrapper = format!( + "#!/bin/sh\nif [ \"$1\" = send-email ]; then exit {answer}; fi\nexec {} \"$@\"\n", + real.display() + ); + self.write_script_in(&bin, "git", &wrapper); + + bin + } + + /// The machine's own copy of a program, found on its `PATH`. + fn program(&self, name: &str) -> PathBuf { + std::env::var("PATH") + .unwrap_or_default() + .split(':') + .map(|dir| Path::new(dir).join(name)) + .find(|candidate| candidate.is_file()) + .unwrap_or_else(|| panic!("{name} is not on this machine's PATH")) + } } diff --git a/tests/config.rs b/tests/config.rs new file mode 100644 index 0000000..9bc4e2f --- /dev/null +++ b/tests/config.rs @@ -0,0 +1,373 @@ +//! The config file and `spectral init`, driven through the built binary. +//! +//! Precedence is asserted at the command line rather than in a unit test, +//! because `std::env::set_var` is `unsafe` under edition 2024 and this crate +//! allows no `unsafe`: the process under test is the one that sets the +//! variables. + +mod common; + +use std::path::Path; + +use common::Fixture; + +fn stdout_of(output: &std::process::Output) -> String { + String::from_utf8_lossy(&output.stdout).into_owned() +} + +fn stderr_of(output: &std::process::Output) -> String { + String::from_utf8_lossy(&output.stderr).into_owned() +} + +/// A second kernel tree, so a test can tell two answers apart. +fn other_tree(fixture: &Fixture) -> std::path::PathBuf { + let tree = fixture.join("other-tree"); + std::fs::create_dir_all(tree.join("scripts")).expect("create the other tree"); + std::fs::write(tree.join("scripts/checkpatch.pl"), "#!/bin/sh\n").expect("write checkpatch"); + std::fs::write(tree.join("scripts/get_maintainer.pl"), "#!/bin/sh\n") + .expect("write the script"); + std::fs::write(tree.join("MAINTAINERS"), "OTHER DRIVER\n").expect("write MAINTAINERS"); + + tree +} + +#[test] +fn a_config_file_supplies_the_tree_when_the_environment_does_not() { + let fixture = Fixture::kernel_tree(); + fixture.write_config(&format!( + "kernel_tree = \"{}\"\npatch_dir = \"{}\"\n", + fixture.tree().display(), + fixture.home().join("patches").display() + )); + + let output = fixture.cli_without_the_tree_variable(&["doctor"], &[]); + + assert!( + output.status.success(), + "{}{}", + stdout_of(&output), + stderr_of(&output) + ); + let stdout = stdout_of(&output); + assert!(stdout.contains("ok kernel tree"), "{stdout}"); + assert!( + stdout.contains("from the config file"), + "the report says the file answered: {stdout}" + ); + assert!( + stdout.contains("ok config file"), + "the file is a check of its own: {stdout}" + ); +} + +#[test] +fn the_environment_beats_the_config_file() { + let fixture = Fixture::kernel_tree(); + fixture.write_config(&format!( + "kernel_tree = \"{}\"\n", + other_tree(&fixture).display() + )); + + let output = fixture.cli_without_the_tree_variable( + &["doctor"], + &[( + "SPECTRAL_KERNEL", + fixture.tree().to_str().expect("the tree"), + )], + ); + + assert!(output.status.success(), "{}", stdout_of(&output)); + let stdout = stdout_of(&output); + assert!(stdout.contains("from $SPECTRAL_KERNEL"), "{stdout}"); + assert!( + stdout.contains(fixture.tree().to_str().expect("the tree")), + "{stdout}" + ); +} + +#[test] +fn the_file_beats_the_built_in_default() { + let fixture = Fixture::kernel_tree(); + let other = other_tree(&fixture); + fixture.write_config(&format!("kernel_tree = \"{}\"\n", other.display())); + + let output = fixture.cli_without_the_tree_variable(&["doctor"], &[]); + + assert!(output.status.success(), "{}", stdout_of(&output)); + let stdout = stdout_of(&output); + assert!( + stdout.contains(other.to_str().expect("the other tree")), + "the file's tree is the one doctor looked at: {stdout}" + ); + assert!( + !stdout.contains(".spectral/linux"), + "the default was not used: {stdout}" + ); +} + +#[test] +fn with_no_file_and_no_variable_the_default_is_used() { + let fixture = Fixture::kernel_tree(); + + let output = fixture.cli_without_the_tree_variable(&["doctor"], &[]); + + assert_eq!(output.status.code(), Some(1)); + let stdout = stdout_of(&output); + assert!( + stdout.contains(".spectral/linux"), + "the default is $HOME/.spectral/linux: {stdout}" + ); + assert!(stdout.contains("the built-in default"), "{stdout}"); +} + +#[test] +fn malformed_config_is_a_named_error_rather_than_a_panic() { + let fixture = Fixture::kernel_tree(); + let path = fixture.write_config("kernel_tree = 42\n"); + + let output = fixture.cli_without_the_tree_variable(&["doctor"], &[]); + + assert_eq!( + output.status.code(), + Some(1), + "a bad config is not a panic: {}{}", + stdout_of(&output), + stderr_of(&output) + ); + let stderr = stderr_of(&output); + assert!(stderr.contains("is not a readable config file"), "{stderr}"); + assert!(stderr.contains(&path.display().to_string()), "{stderr}"); +} + +#[test] +fn an_unknown_key_is_refused_rather_than_ignored() { + let fixture = Fixture::kernel_tree(); + fixture.write_config(&format!( + "kernel_tree = \"{}\"\ntree = \"a typo\"\n", + fixture.tree().display() + )); + + let output = fixture.cli_without_the_tree_variable(&["doctor"], &[]); + + assert_eq!(output.status.code(), Some(1)); + assert!( + stderr_of(&output).contains("is not a readable config file"), + "{}", + stderr_of(&output) + ); +} + +#[test] +fn a_relative_path_in_the_config_is_refused() { + let fixture = Fixture::kernel_tree(); + fixture.write_config("kernel_tree = \"linux\"\n"); + + let output = fixture.cli_without_the_tree_variable(&["doctor"], &[]); + + assert_eq!(output.status.code(), Some(1)); + let stderr = stderr_of(&output); + assert!(stderr.contains("not an absolute path"), "{stderr}"); +} + +#[test] +fn init_writes_the_config_and_a_second_run_changes_nothing() { + let fixture = Fixture::kernel_tree(); + + let first = fixture.cli_without_the_tree_variable( + &["init", "--tree", fixture.tree().to_str().expect("the tree")], + &[], + ); + assert!( + first.status.success(), + "{}{}", + stdout_of(&first), + stderr_of(&first) + ); + assert!(stdout_of(&first).contains("wrote"), "{}", stdout_of(&first)); + let config = fixture.home().join("config/spectral/config.toml"); + let written = std::fs::read(&config).expect("the config file"); + assert!( + String::from_utf8_lossy(&written).contains(fixture.tree().to_str().expect("the tree")), + "{}", + String::from_utf8_lossy(&written) + ); + + let second = fixture.cli_without_the_tree_variable( + &["init", "--tree", fixture.tree().to_str().expect("the tree")], + &[], + ); + assert!(second.status.success(), "{}", stderr_of(&second)); + assert!( + stdout_of(&second).contains("already says this"), + "{}", + stdout_of(&second) + ); + assert_eq!( + std::fs::read(&config).expect("the config file"), + written, + "the second run rewrote the file" + ); +} + +#[test] +fn init_refuses_to_clobber_a_config_that_says_something_else() { + let fixture = Fixture::kernel_tree(); + let other = other_tree(&fixture); + fixture.write_config(&format!("kernel_tree = \"{}\"\n", other.display())); + + let refused = fixture.cli_without_the_tree_variable( + &["init", "--tree", fixture.tree().to_str().expect("the tree")], + &[], + ); + assert_eq!(refused.status.code(), Some(1)); + let stderr = stderr_of(&refused); + assert!(stderr.contains("already points somewhere else"), "{stderr}"); + assert!(stderr.contains("--force"), "{stderr}"); + + let forced = fixture.cli_without_the_tree_variable( + &[ + "init", + "--tree", + fixture.tree().to_str().expect("the tree"), + "--force", + ], + &[], + ); + assert!( + forced.status.success(), + "{}{}", + stdout_of(&forced), + stderr_of(&forced) + ); + let config = std::fs::read_to_string(fixture.home().join("config/spectral/config.toml")) + .expect("the config file"); + assert!( + config.contains(fixture.tree().to_str().expect("the tree")), + "{config}" + ); +} + +#[test] +fn init_verifies_the_tree_it_is_pointed_at() { + let fixture = Fixture::kernel_tree(); + let not_a_tree = fixture.join("not-a-tree"); + std::fs::create_dir_all(¬_a_tree).expect("create the directory"); + + let output = fixture.cli_without_the_tree_variable( + &[ + "init", + "--tree", + not_a_tree.to_str().expect("the directory"), + ], + &[], + ); + + assert_eq!(output.status.code(), Some(1)); + assert!( + stderr_of(&output).contains("not a kernel source tree"), + "{}", + stderr_of(&output) + ); + assert!( + !Path::new(&fixture.home().join("config/spectral/config.toml")).exists(), + "a tree that did not verify must not be written into the config" + ); +} + +#[test] +fn init_refuses_before_it_clones_anything() { + // The refusal is about the config file, so it has to happen before a + // multi-minute kernel clone rather than after one has already landed. + let fixture = Fixture::kernel_tree(); + let other = other_tree(&fixture); + fixture.write_config(&format!("kernel_tree = \"{}\"\n", other.display())); + let into = fixture.home().join("should-not-exist"); + + let output = fixture.cli_without_the_tree_variable( + &[ + "init", + "--clone", + fixture.tree().to_str().expect("the tree"), + "--tree", + into.to_str().expect("the destination"), + ], + &[], + ); + + assert_eq!(output.status.code(), Some(1)); + assert!( + stderr_of(&output).contains("already points somewhere else"), + "{}", + stderr_of(&output) + ); + assert!( + !into.exists(), + "the clone ran before the refusal was printed" + ); +} + +#[test] +fn init_clones_a_tree_it_was_given_a_url_for() { + // A local path is a url git can clone, so this exercises the clone branch + // without a network. + let fixture = Fixture::kernel_tree(); + let into = fixture.home().join("cloned"); + + let output = fixture.cli_without_the_tree_variable( + &[ + "init", + "--clone", + fixture.tree().to_str().expect("the tree"), + "--tree", + into.to_str().expect("the destination"), + ], + &[], + ); + + assert!( + output.status.success(), + "{}{}", + stdout_of(&output), + stderr_of(&output) + ); + assert!(into.join("scripts/checkpatch.pl").is_file()); + assert!(into.join("MAINTAINERS").is_file()); + let config = std::fs::read_to_string(fixture.home().join("config/spectral/config.toml")) + .expect("the config file"); + assert!(config.contains("cloned"), "{config}"); +} + +#[test] +fn the_config_file_carries_always_cc_and_an_identity_into_submit() { + let fixture = Fixture::kernel_tree(); + fixture.work_on_a_branch(); + fixture.write("drivers/foo/bar.c", common::EDITED); + fixture.commit_all("foo: return a + 1"); + assert!( + fixture + .cli(&["patch", "create", "000-foo"]) + .status + .success() + ); + fixture.write_config(&format!( + "kernel_tree = \"{}\"\nalways_cc = [\"watcher@example.com\"]\n\n[send_email]\nfrom = \"Someone \"\n", + fixture.tree().display() + )); + + let output = fixture + .cli_without_the_tree_variable(&["patch", "submit", "000-foo.patch", "--dry-run"], &[]); + + assert!( + output.status.success(), + "{}{}", + stdout_of(&output), + stderr_of(&output) + ); + let stdout = stdout_of(&output); + assert!(stdout.contains("Cc: watcher@example.com"), "{stdout}"); + assert!( + stdout.contains("--from 'Someone '"), + "{stdout}" + ); + assert!(stdout.contains("Dry-OK"), "{stdout}"); +} diff --git a/tests/doctor.rs b/tests/doctor.rs new file mode 100644 index 0000000..d0568d1 --- /dev/null +++ b/tests/doctor.rs @@ -0,0 +1,216 @@ +//! `spectral doctor`, driven through the built binary. +//! +//! The fixture controls `PATH` and git's configuration, so a missing qemu, a +//! missing sender identity and a tree that is not a kernel tree are produced on +//! purpose rather than inherited from whichever machine this runs on. That +//! extends to git's own send-email support, which the harness cannot control +//! through git's configuration: a `git` wrapper answers for it in both +//! directions, so a host without it still passes. +//! +//! Every test name here carries the verb, so the plan's own command +//! `cargo test --locked doctor` selects exactly this file's tests. + +mod common; + +use common::Fixture; + +fn stdout_of(output: &std::process::Output) -> String { + String::from_utf8_lossy(&output.stdout).into_owned() +} + +fn stderr_of(output: &std::process::Output) -> String { + String::from_utf8_lossy(&output.stderr).into_owned() +} + +/// Run `doctor` with a `git` that has send-email support, whatever the host has. +fn doctor_with_send_email(fixture: &Fixture) -> std::process::Output { + let bin = fixture.path_with_send_email(0); + + fixture.cli_env( + &["doctor"], + &[("PATH", bin.to_str().expect("the git bin directory"))], + ) +} + +#[test] +fn doctor_passes_on_a_complete_fixture_tree() { + let fixture = Fixture::kernel_tree(); + + let output = doctor_with_send_email(&fixture); + + assert!( + output.status.success(), + "{}{}", + stdout_of(&output), + stderr_of(&output) + ); + let stdout = stdout_of(&output); + assert!(!stdout.contains("fail "), "{stdout}"); + assert!(stdout.contains("ok kernel tree"), "{stdout}"); + assert!( + stdout.contains("from $SPECTRAL_KERNEL"), + "the report has to say which knob set the path: {stdout}" + ); + assert!(stdout.contains("ok git send-email"), "{stdout}"); + assert!( + stdout.contains("nothing failed") || stdout.contains("everything checked out"), + "{stdout}" + ); +} + +#[test] +fn doctor_names_the_first_hard_failure_when_the_tree_is_missing() { + let fixture = Fixture::bare(); + + let output = fixture.cli_env(&["doctor"], &[("SPECTRAL_KERNEL", "/nonexistent/linux")]); + + assert_eq!(output.status.code(), Some(1)); + let stdout = stdout_of(&output); + assert!(stdout.contains("fail kernel tree"), "{stdout}"); + assert!(stdout.contains("first failure: kernel tree"), "{stdout}"); + assert!( + stdout.contains("skip checkpatch.pl"), + "the checks that need a tree are not answers when there is none: {stdout}" + ); + assert!(stdout.contains("-> "), "the fix hint is printed: {stdout}"); +} + +#[test] +fn doctor_names_a_directory_that_is_not_a_kernel_tree() { + let fixture = Fixture::bare(); + let tree = fixture.join("not-a-tree"); + std::fs::create_dir_all(&tree).expect("create the directory"); + + let output = fixture.cli_env( + &["doctor"], + &[("SPECTRAL_KERNEL", tree.to_str().expect("the tree path"))], + ); + + assert_eq!(output.status.code(), Some(1)); + let stdout = stdout_of(&output); + assert!(stdout.contains("fail kernel tree shape"), "{stdout}"); + assert!(stdout.contains("not a kernel source tree"), "{stdout}"); + assert!( + stdout.contains("first failure: kernel tree shape"), + "{stdout}" + ); +} + +#[test] +fn doctor_warns_about_qemu_off_the_path_rather_than_failing() { + let fixture = Fixture::kernel_tree(); + // A `PATH` holding the git wrapper and nothing else: qemu is gone, the + // rest is not. + let bin = fixture.path_with_send_email(0); + + let output = fixture.cli_env( + &["doctor"], + &[("PATH", bin.to_str().expect("the git bin directory"))], + ); + + assert!( + output.status.success(), + "{}{}", + stdout_of(&output), + stderr_of(&output) + ); + let stdout = stdout_of(&output); + assert!(stdout.contains("warn qemu"), "{stdout}"); + assert!( + !stdout.contains("fail "), + "a missing qemu is not something the loop cannot work around: {stdout}" + ); +} + +#[test] +fn doctor_fails_on_no_sender_identity_and_says_how_to_fix_it() { + let fixture = Fixture::kernel_tree(); + // Both places git would find one: the tree's own config and the fixture's + // global one. + fixture.git(&["config", "--unset", "user.email"]); + std::fs::write( + fixture.home().join("gitconfig"), + "[core]\n\trepositoryformatversion = 0\n", + ) + .expect("write a global config with no identity"); + + let output = doctor_with_send_email(&fixture); + + assert_eq!(output.status.code(), Some(1)); + let stdout = stdout_of(&output); + assert!(stdout.contains("fail sender identity"), "{stdout}"); + assert!( + stdout.contains("user.email"), + "the failure says how to fix it: {stdout}" + ); + assert!( + stdout.contains("first failure: sender identity"), + "{stdout}" + ); +} + +#[test] +fn doctor_warns_when_git_has_no_send_email_support() { + // The direction the host cannot be trusted for: this machine has it, and a + // machine without it must fail the same way rather than pass by accident. + let fixture = Fixture::kernel_tree(); + let bin = fixture.path_with_send_email(1); + + let output = fixture.cli_env( + &["doctor"], + &[("PATH", bin.to_str().expect("the git bin directory"))], + ); + + assert_eq!(output.status.code(), Some(1)); + let stdout = stdout_of(&output); + assert!(stdout.contains("fail git send-email"), "{stdout}"); + assert!(stdout.contains("first failure: git send-email"), "{stdout}"); +} + +#[test] +fn doctor_warns_about_no_mail_route_because_dry_runs_are_what_matter() { + // The fixture's git config has no sendemail.* at all, which is the state + // this machine's own config was in when --dry-run was verified. + let fixture = Fixture::kernel_tree(); + + let output = doctor_with_send_email(&fixture); + + assert!(output.status.success()); + let stdout = stdout_of(&output); + assert!(stdout.contains("warn mail route"), "{stdout}"); + assert!( + stdout.contains("--dry-run works"), + "the warning says what still works: {stdout}" + ); +} + +#[test] +fn doctor_reports_where_the_paths_came_from() { + let fixture = Fixture::kernel_tree(); + let patches = fixture.home().join("patches"); + + let output = fixture.cli_env( + &["doctor"], + &[ + ( + "PATH", + fixture.path_with_send_email(0).to_str().expect("the bin"), + ), + ( + "SPECTRAL_PATCH_DIR", + patches.to_str().expect("the patch dir"), + ), + ], + ); + + let stdout = stdout_of(&output); + assert!(stdout.contains("from $SPECTRAL_PATCH_DIR"), "{stdout}"); + assert!( + stdout.contains(patches.to_str().expect("the patch dir")), + "{stdout}" + ); + assert!( + patches.is_dir(), + "doctor is the one check with a side effect: it makes the directory it was pointed at" + ); +} diff --git a/tests/patch_verbs.rs b/tests/patch_verbs.rs index b0437ae..67f5bd3 100644 --- a/tests/patch_verbs.rs +++ b/tests/patch_verbs.rs @@ -152,6 +152,24 @@ fn a_bare_diff_is_refused_by_send_email() { assert!(stderr.contains("No subject line"), "{stderr}"); } +#[test] +fn a_commit_with_nothing_staged_says_what_git_said() { + // git explains "nothing staged" on stdout, so an error path that only + // reads stderr prints a failure with nothing after it. + let fixture = Fixture::kernel_tree(); + fixture.write("drivers/foo/bar.c", common::EDITED); + + let output = fixture.cli(&["patch", "commit", "foo: nothing staged"]); + + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("failed (exit 1)"), "{stderr}"); + assert!( + stderr.contains("no changes added to commit") || stderr.contains("not staged"), + "git's own explanation was dropped: {stderr}" + ); +} + #[test] fn create_names_a_base_that_does_not_resolve() { let fixture = Fixture::kernel_tree(); @@ -180,8 +198,8 @@ fn create_refuses_a_range_that_holds_more_than_one_commit() { let stderr = String::from_utf8_lossy(&output.stderr); assert!(stderr.contains("holds 2 commits"), "{stderr}"); assert!( - stderr.contains("series support is not built yet"), - "{stderr}" + stderr.contains("--range"), + "the refusal has to say where a set belongs: {stderr}" ); } @@ -229,7 +247,7 @@ fn check_pipes_the_working_tree_diff_into_checkpatch_and_fails_on_errors() { } #[test] -fn warnings_alone_do_not_fail_the_command() { +fn checkpatch_warnings_alone_do_not_fail_the_command() { let fixture = Fixture::kernel_tree(); fixture.write("drivers/foo/bar.c", common::EDITED); @@ -246,7 +264,7 @@ fn warnings_alone_do_not_fail_the_command() { } #[test] -fn a_run_with_no_total_line_counts_as_clean() { +fn checkpatch_counts_a_run_with_no_total_line_as_clean() { // What a clean run looks like under checkpatch's own --terse, where the // summary line is suppressed entirely. let fixture = Fixture::kernel_tree(); @@ -524,12 +542,32 @@ fn submit_threads_a_reroll_off_the_patch_it_replaces() { assert!(output.status.success()); let stdout = String::from_utf8_lossy(&output.stdout); assert!( - stdout.contains("--in-reply-to "), - "{stdout}" + stdout.contains("--in-reply-to ''"), + "an unquoted <...> is a redirection when pasted: {stdout}" ); assert!(stdout.contains("In-Reply-To: "), "{stdout}"); } +#[test] +fn a_tree_that_is_not_a_repository_says_so_rather_than_blaming_a_revision() { + // `git rev-parse --verify --quiet master` fails both for a revision that is + // not there and for a directory that is not a repository. They want + // different fixes, so they get different messages. + let fixture = Fixture::bare(); + fixture.write_script("scripts/checkpatch.pl", "#!/bin/sh\n"); + fixture.write_script("scripts/get_maintainer.pl", "#!/bin/sh\n"); + + let output = fixture.cli(&["patch", "create", "000-foo"]); + + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("not a git repository"), "{stderr}"); + assert!( + !stderr.contains("no revision"), + "a missing repository is not a missing revision: {stderr}" + ); +} + #[test] fn submit_refuses_a_template_cover_letter_instead_of_swallowing_it() { let fixture = Fixture::kernel_tree(); diff --git a/tests/qemu.rs b/tests/qemu.rs new file mode 100644 index 0000000..a16fdbe --- /dev/null +++ b/tests/qemu.rs @@ -0,0 +1,135 @@ +//! `kernel test`, driven through the built binary against a fixture tree. +//! +//! Nothing here boots a kernel: qemu is taken off `PATH` deliberately, so the +//! assertions are about the paths around the boot rather than about a machine +//! that can run one. The real boot is the plan's P6 manual step. + +mod common; + +use common::Fixture; + +/// A fixture tree with something at the path qemu would be handed. +fn fixture_with_image() -> Fixture { + let fixture = Fixture::kernel_tree(); + fixture.write("arch/x86/boot/bzImage", "not really a kernel\n"); + fixture.commit_all("fixture: add an image"); + + fixture +} + +#[test] +fn a_missing_qemu_is_named_rather_than_panicked_on() { + let fixture = fixture_with_image(); + // A `PATH` with nothing on it: the binary itself is found by its full + // path, and qemu cannot be. + let bare = fixture.restricted_path(&[]); + + let output = fixture.cli_env( + &["kernel", "test", "--no-build"], + &[("PATH", bare.to_str().expect("the restricted path"))], + ); + + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("could not run `qemu-system-x86_64`"), + "{stderr}" + ); + assert!( + !String::from_utf8_lossy(&output.stdout).contains("exited cleanly"), + "a boot that never started was reported as a success" + ); +} + +#[test] +fn a_tree_with_no_image_is_named_rather_than_a_build_being_attempted() { + let fixture = Fixture::kernel_tree(); + + let output = fixture.cli(&["kernel", "test", "--no-build"]); + + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("no bootable image at"), "{stderr}"); + assert!(stderr.contains("arch/x86/boot/bzImage"), "{stderr}"); +} + +#[test] +fn build_runs_make_in_the_tree_and_finds_the_image_it_wrote() { + // A Makefile that writes the image where a real build leaves it, and + // records that it ran. `sh` is the only tool it needs, so the restricted + // `PATH` can hold make and sh and still leave qemu out. + let fixture = Fixture::kernel_tree(); + fixture.write("arch/x86/boot/.keep", ""); + fixture.write( + "Makefile", + "all:\n\tprintf '' > arch/x86/boot/bzImage\n\tprintf 'built\\n' > .make-call\n", + ); + fixture.commit_all("fixture: add a Makefile"); + let bin = fixture.restricted_path(&["make", "sh"]); + + let output = fixture.cli_env( + &["kernel", "test"], + &[("PATH", bin.to_str().expect("the restricted path"))], + ); + + assert_eq!( + fixture.read(".make-call"), + "built\n", + "make did not run in the tree" + ); + // The image check passed, which is why the failure is qemu's rather than + // the image's. + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("could not run `qemu-system-x86_64`"), + "the build did not get as far as qemu: {stderr}" + ); +} + +#[test] +fn qemu_runs_with_the_arguments_the_plan_describes() { + // What qemu was handed, captured without needing a real qemu: the stub is + // reached through `PATH`, so the binary under test cannot tell. + let fixture = fixture_with_image(); + fixture.write_script( + "bin/qemu-system-x86_64", + "#!/bin/sh\necho \"argv: $*\"\nexit 1\n", + ); + let bin = fixture.join("bin"); + + let output = fixture.cli_env( + &[ + "kernel", + "test", + "--no-build", + "--qemu-arg", + "-m", + "--qemu-arg", + "1G", + ], + &[("PATH", bin.to_str().expect("the stub bin directory"))], + ); + + assert_eq!( + output.status.code(), + Some(1), + "a failing qemu must not look clean: {}", + String::from_utf8_lossy(&output.stdout) + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("-kernel ") && stdout.contains("arch/x86/boot/bzImage"), + "{stdout}" + ); + assert!( + stdout.contains("-append console=ttyS0"), + "the console is what makes a boot readable: {stdout}" + ); + assert!(stdout.contains("-nographic"), "{stdout}"); + assert!(stdout.contains("-m 1G"), "the pass-through args: {stdout}"); + assert!( + stdout.contains("qemu exited with status 1"), + "the exit status was flattened: {stdout}" + ); +} diff --git a/tests/real_tools.rs b/tests/real_tools.rs index 9cc92fd..afcdf97 100644 --- a/tests/real_tools.rs +++ b/tests/real_tools.rs @@ -122,3 +122,48 @@ fn real_get_maintainer_fills_the_to_and_cc_lines() { ); assert!(stdout.contains("Dry-OK"), "{stdout}"); } + +#[test] +#[ignore = "needs SPECTRAL_LIVE=1 and a network"] +fn live_bugzilla_answers_with_one_issue() { + // The only test that talks to bugzilla.kernel.org. It asserts the shape of + // the answer rather than which bug came back, which is what a live API can + // be held to. + assert_eq!( + std::env::var("SPECTRAL_LIVE").as_deref(), + Ok("1"), + "run this with SPECTRAL_LIVE=1" + ); + let fixture = Fixture::bare(); + let output = fixture.cli(&["kernel", "quest", "--json"]); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8_lossy(&output.stdout); + for field in ["\"id\"", "\"title\"", "\"component\"", "\"url\""] { + assert!(stdout.contains(field), "{field} is missing from {stdout}"); + } + assert!( + stdout.contains("https://bugzilla.kernel.org/show_bug.cgi?id="), + "{stdout}" + ); +} + +#[test] +#[ignore = "needs SPECTRAL_LIVE=1 and a network"] +fn live_bugzilla_names_a_filter_that_matches_nothing() { + assert_eq!( + std::env::var("SPECTRAL_LIVE").as_deref(), + Ok("1"), + "run this with SPECTRAL_LIVE=1" + ); + let fixture = Fixture::bare(); + let output = fixture.cli(&["kernel", "quest", "--filter", "zzq-no-such-word-zzq"]); + + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("no open bug matched"), "{stderr}"); +} diff --git a/tests/series.rs b/tests/series.rs new file mode 100644 index 0000000..c9d08df --- /dev/null +++ b/tests/series.rs @@ -0,0 +1,464 @@ +//! Patch series, driven through the built binary against a fixture tree. +//! +//! Every test name here carries the verb, so the plan's own command +//! `cargo test --locked series` selects exactly this file's tests. + +mod common; + +use std::path::{Path, PathBuf}; + +use common::Fixture; + +/// The blurb the tests write into a cover letter. +const BLURB: &str = "Port the re-arm to v2\n\nThis replaces the v1 series after review."; + +/// A tree with two commits on a branch of its own: the smallest thing that is +/// really a set rather than a patch. +fn fixture_with_two_commits() -> Fixture { + let fixture = Fixture::kernel_tree(); + fixture.work_on_a_branch(); + fixture.write("drivers/foo/bar.c", common::EDITED); + fixture.commit_all("foo: return a + 1"); + fixture.write( + "drivers/foo/bar.c", + "static int foo(int a)\n{\n\treturn a + 2;\n}\n", + ); + fixture.commit_all("foo: return a + 2"); + + fixture +} + +/// The directory a series was written into, as `create` printed it. +fn create_series(fixture: &Fixture, extra: &[&str]) -> PathBuf { + let mut args = vec!["patch", "create", "xhci-series", "--range", "master..HEAD"]; + args.extend_from_slice(extra); + let output = fixture.cli(&args); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + + PathBuf::from(String::from_utf8_lossy(&output.stdout).trim()) +} + +/// What is in a directory, sorted, so two listings can be compared. +fn listing(dir: &Path) -> Vec { + let mut names: Vec = std::fs::read_dir(dir) + .expect("read the series directory") + .map(|entry| { + entry + .expect("an entry") + .file_name() + .to_string_lossy() + .into_owned() + }) + .collect(); + names.sort(); + + names +} + +/// The patches in a directory, in name order. +fn patches(dir: &Path) -> Vec { + listing(dir) + .into_iter() + .filter(|name| name.ends_with(".patch")) + .collect() +} + +fn read(dir: &Path, name: &str) -> String { + std::fs::read_to_string(dir.join(name)).expect("read a file in the series directory") +} + +#[test] +fn a_series_gets_the_names_and_the_cover_letter_format_patch_gives_it() { + let fixture = fixture_with_two_commits(); + + let dir = create_series(&fixture, &["--cover-letter", BLURB]); + + assert_eq!( + dir, + fixture.home().join(".spectral/patches/xhci-series"), + "the series belongs in a directory of its own" + ); + let names = listing(&dir); + assert_eq!(names.len(), 4, "{names:?}"); + assert_eq!(names[0], "0000-cover-letter.patch"); + assert!(names[1].starts_with("0001-"), "{names:?}"); + assert!(names[2].starts_with("0002-"), "{names:?}"); + assert_eq!(names[3], "series.toml", "the sidecar says what this is"); + + // The sidecar records the whole range as hashes, the revision, and the + // files in the order they are sent. The range matters as much as the base: + // a re-roll rebuilt as `base..HEAD` would pick up later commits. + let manifest = read(&dir, "series.toml"); + let master = fixture.git(&["rev-parse", "master"]); + let head = fixture.git(&["rev-parse", "HEAD"]); + assert!( + manifest.contains(&format!("base = \"{master}\"")), + "the base is a hash: {manifest}" + ); + assert!( + manifest.contains(&format!("head = \"{head}\"")), + "the head is a hash too: {manifest}" + ); + assert!(manifest.contains("version = 1"), "{manifest}"); + assert!(manifest.contains("0000-cover-letter.patch"), "{manifest}"); + + // The blurb landed, and the template that send-email refuses is gone. + let cover = read(&dir, "0000-cover-letter.patch"); + assert!( + cover.contains("Subject: [PATCH 0/2] Port the re-arm to v2"), + "{cover}" + ); + assert!(cover.contains("This replaces the v1 series"), "{cover}"); + assert!(!cover.contains("*** BLURB HERE ***"), "{cover}"); + assert!(!cover.contains("*** SUBJECT HERE ***"), "{cover}"); +} + +#[test] +fn a_series_goes_out_in_one_send_email_invocation() { + let fixture = fixture_with_two_commits(); + create_series(&fixture, &["--cover-letter", BLURB]); + + let output = fixture.cli(&["patch", "submit", "xhci-series", "--dry-run"]); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8_lossy(&output.stdout); + let command = stdout + .lines() + .find(|line| line.contains("send-email --dry-run")) + .unwrap_or_else(|| panic!("no command line in {stdout}")); + + for name in ["0000-cover-letter.patch", "0001-", "0002-"] { + assert!(command.contains(name), "{name} is not in {command}"); + } + let cover_at = command.find("0000-cover-letter").expect("the cover letter"); + let first_at = command.find("0001-").expect("the first patch"); + assert!( + cover_at < first_at, + "the cover letter is sent first: {command}" + ); + assert!(stdout.contains("Dry-OK"), "{stdout}"); + + // One lookup over the whole set: a series has one recipient list. + let call = fixture.read(".getmaintainer-call"); + assert_eq!( + call.matches("argv:").count(), + 1, + "the lookup ran more than once: {call}" + ); + assert!(call.contains("0000-cover-letter.patch"), "{call}"); + assert!(call.contains("0001-"), "{call}"); +} + +#[test] +fn a_series_without_a_blurb_is_refused_by_send_email() { + // No --cover-letter, so format-patch's template subject is still in the + // file and send-email refuses the set. The refusal has to reach the user. + let fixture = fixture_with_two_commits(); + create_series(&fixture, &[]); + + let output = fixture.cli(&["patch", "submit", "xhci-series", "--dry-run"]); + + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("Refusing to send"), "{stderr}"); + assert!(stderr.contains("exit 25"), "{stderr}"); +} + +#[test] +fn series_reroll_moves_every_file_at_once_and_stays_idempotent() { + let fixture = fixture_with_two_commits(); + let dir = create_series(&fixture, &["--cover-letter", BLURB]); + let before = listing(&dir); + + // No -v: the next revision is inferred from the sidecar. + let output = fixture.cli(&["patch", "update", "xhci-series"]); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!( + String::from_utf8_lossy(&output.stdout).trim(), + dir.display().to_string() + ); + + let after = listing(&dir); + assert_eq!( + after.len(), + before.len(), + "the old revision was left beside the new one: {after:?}" + ); + assert!( + patches(&dir).iter().all(|name| name.starts_with("v2-")), + "every patch moves together: {after:?}" + ); + assert!( + after + .iter() + .any(|name| name == "v2-0000-cover-letter.patch") + ); + assert!(read(&dir, "series.toml").contains("version = 2")); + assert!( + read(&dir, "v2-0000-cover-letter.patch").contains("This replaces the v1 series"), + "a re-roll threw the blurb away" + ); + + // The same revision again is a no-op, which is what keeps a double run + // from producing a v3. + let again = fixture.cli(&["patch", "update", "xhci-series", "-v", "2"]); + assert!(again.status.success()); + assert_eq!( + String::from_utf8_lossy(&again.stdout).trim(), + dir.display().to_string() + ); + let unchanged = listing(&dir); + assert_eq!(unchanged, after, "a second run at v2 changed the directory"); + assert!( + !unchanged.iter().any(|name| name.starts_with("v3-")), + "{unchanged:?}" + ); + + // And the re-rolled set still sends, with the cover letter first. + let sent = fixture.cli(&["patch", "submit", "xhci-series", "--dry-run"]); + assert!( + sent.status.success(), + "{}", + String::from_utf8_lossy(&sent.stderr) + ); + assert!(String::from_utf8_lossy(&sent.stdout).contains("Dry-OK")); +} + +#[test] +fn a_rerolled_series_holds_exactly_one_revision() { + let fixture = fixture_with_two_commits(); + let dir = create_series(&fixture, &["--cover-letter", BLURB]); + + // Straight to v3, so the files that must go are the unversioned ones. + assert!( + fixture + .cli(&["patch", "update", "xhci-series", "-v", "3"]) + .status + .success() + ); + assert!( + fixture + .cli(&["patch", "update", "xhci-series", "-v", "4"]) + .status + .success() + ); + + let names = patches(&dir); + assert_eq!(names.len(), 3, "{names:?}"); + assert!( + names.iter().all(|name| name.starts_with("v4-")), + "{names:?}" + ); +} + +#[test] +fn series_reroll_does_not_pick_up_a_commit_that_landed_after_create() { + // The range a series was created for is the range it re-rolls. Rebuilding + // it as `base..HEAD` would quietly mail commits nobody asked to send. + let fixture = fixture_with_two_commits(); + let dir = create_series(&fixture, &["--cover-letter", BLURB]); + assert_eq!(patches(&dir).len(), 3); + + fixture.write( + "drivers/foo/bar.c", + "static int foo(int a)\n{\n\treturn a + 3;\n}\n", + ); + fixture.commit_all("foo: return a + 3"); + + let output = fixture.cli(&["patch", "update", "xhci-series"]); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + + let names = patches(&dir); + assert_eq!( + names.len(), + 3, + "a commit that landed after create joined the set: {names:?}" + ); + assert!( + !names.iter().any(|name| name.contains("return-a-3")), + "{names:?}" + ); + assert_eq!( + read(&dir, "series.toml").matches("version = 2").count(), + 1, + "{}", + read(&dir, "series.toml") + ); +} + +#[test] +fn series_reroll_tolerates_a_patch_that_is_already_gone() { + // A missing file is the state the re-roll wanted, not a failure it should + // report forever while leaving the sidecar describing a revision that is + // no longer on disk. + let fixture = fixture_with_two_commits(); + let dir = create_series(&fixture, &["--cover-letter", BLURB]); + let doomed = patches(&dir) + .into_iter() + .find(|name| name.starts_with("0002-")) + .expect("the second patch"); + std::fs::remove_file(dir.join(&doomed)).expect("remove a patch by hand"); + + let output = fixture.cli(&["patch", "update", "xhci-series"]); + + assert!( + output.status.success(), + "a stale name stopped the re-roll: {}", + String::from_utf8_lossy(&output.stderr) + ); + let names = patches(&dir); + assert_eq!(names.len(), 3, "{names:?}"); + assert!( + names.iter().all(|name| name.starts_with("v2-")), + "{names:?}" + ); + assert!(read(&dir, "series.toml").contains("version = 2")); + + // And it is ready to send, which is the thing the old failure made + // impossible: the sidecar and the directory agree again. + let sent = fixture.cli(&["patch", "submit", "xhci-series", "--dry-run"]); + assert!( + sent.status.success(), + "{}", + String::from_utf8_lossy(&sent.stderr) + ); +} + +#[test] +fn series_create_refuses_an_empty_blurb_before_it_writes_anything() { + let fixture = fixture_with_two_commits(); + + let output = fixture.cli(&[ + "patch", + "create", + "xhci-series", + "--range", + "master..HEAD", + "--cover-letter", + " ", + ]); + + assert_eq!(output.status.code(), Some(1)); + assert!( + String::from_utf8_lossy(&output.stderr).contains("blurb is empty"), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let dir = fixture.home().join(".spectral/patches/xhci-series"); + assert!( + !dir.exists(), + "files with no sidecar beside them were left behind: {:?}", + listing(&dir) + ); +} + +#[test] +fn a_directory_that_is_not_a_series_still_sends_its_patches() { + // No sidecar: the directory is read as a pile of patches, in name order, + // which is the order format-patch numbered them in. + let fixture = fixture_with_two_commits(); + let dir = fixture.home().join("hand-made"); + std::fs::create_dir_all(&dir).expect("create the directory"); + fixture.git(&[ + "format-patch", + "-o", + dir.to_str().expect("the directory path"), + "master..HEAD", + ]); + + let output = fixture.cli(&[ + "patch", + "submit", + dir.to_str().expect("the directory path"), + "--dry-run", + ]); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("0001-") && stdout.contains("0002-"), + "{stdout}" + ); +} + +#[test] +fn a_series_wants_a_range_and_a_range_that_is_not_one_is_named() { + let fixture = fixture_with_two_commits(); + + let not_a_range = fixture.cli(&["patch", "create", "x", "--range", "master"]); + assert_eq!(not_a_range.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(¬_a_range.stderr); + assert!(stderr.contains("is not a range"), "{stderr}"); + + let orphan_blurb = fixture.cli(&["patch", "create", "x", "--cover-letter", "hi"]); + assert_eq!(orphan_blurb.status.code(), Some(1)); + assert!( + String::from_utf8_lossy(&orphan_blurb.stderr).contains("belongs to a series"), + "{}", + String::from_utf8_lossy(&orphan_blurb.stderr) + ); +} + +#[test] +fn create_needs_a_name_when_no_series_range_is_given() { + let fixture = fixture_with_two_commits(); + + let output = fixture.cli(&["patch", "create"]); + + assert_eq!(output.status.code(), Some(1)); + assert!( + String::from_utf8_lossy(&output.stderr).contains("wants a NAME"), + "{}", + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn a_series_with_no_name_lands_in_the_patch_directory_itself() { + // NAME names the series directory, so leaving it off means the patch + // directory is the series directory. + let fixture = fixture_with_two_commits(); + + let output = fixture.cli(&[ + "patch", + "create", + "--range", + "master..HEAD", + "--cover-letter", + BLURB, + ]); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let patches_dir = fixture.home().join(".spectral/patches"); + assert_eq!( + String::from_utf8_lossy(&output.stdout).trim(), + patches_dir.display().to_string() + ); + assert!(patches_dir.join("series.toml").is_file()); + assert!(patches_dir.join("0000-cover-letter.patch").is_file()); +}