diff --git a/src/cli.rs b/src/cli.rs index 6bb9820..5292808 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -84,6 +84,9 @@ pub struct CheckArgs { /// Check this patch file instead of the working tree #[arg(value_name = "PATCH")] pub patch: Option, + /// Check this revision instead of the working tree + #[arg(long, value_name = "REV", conflicts_with = "patch")] + pub rev: Option, /// Pass --strict to checkpatch.pl #[arg(long)] pub strict: bool, diff --git a/src/error.rs b/src/error.rs index d950211..1c06e64 100644 --- a/src/error.rs +++ b/src/error.rs @@ -4,13 +4,18 @@ //! call site, and is printed once by `main`. use std::path::PathBuf; +use std::process::ExitStatus; /// Shorthand for the crate's result type. pub type Result = std::result::Result; -// Variants that no command constructs yet, but that the command paths below -// are written to return. Safe to drop the allow once they are all reached. -#[allow(dead_code)] +/// How a failed process is described in [`Error::ExternalCommand`]. +pub(crate) fn exit_code(status: &ExitStatus) -> String { + status + .code() + .map_or_else(|| "signal".to_owned(), |code| code.to_string()) +} + #[derive(Debug, thiserror::Error)] pub enum Error { /// Nothing at the configured path at all. @@ -42,9 +47,39 @@ pub enum Error { #[error("no patch named `{name}` in `{dir}`")] PatchNotFound { name: String, dir: PathBuf }, + /// A revision named on the command line does not resolve. + #[error("no revision `{rev}` in this tree; `--base` wants a branch or commit that exists")] + UnknownRevision { rev: String }, + + /// The base is already at the tip, so there is no change to send. + #[error("nothing to send: `{base}` and HEAD hold no commits; commit the change first")] + NoCommitsToSend { base: String }, + + /// 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)" + )] + RangeHoldsSeveralCommits { base: String, count: usize }, + + /// `--fix` was pointed at something that is not a patch file. + #[error( + "checkpatch only fixes a patch file; give `patch format` a patch instead of a revision" + )] + FixNeedsPatchFile, + + /// checkpatch's fix could not be put into the working tree. + #[error("the fix could not be applied ({reason}); the working tree was left as it was")] + FixApplyFailed { reason: String }, + + /// Re-rolling would land on a file that is already there. + #[error("`{path}` already exists; delete it or pick another revision")] + RerollWouldOverwrite { 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 66aff86..2394e5b 100644 --- a/src/git.rs +++ b/src/git.rs @@ -3,12 +3,11 @@ //! Nothing in here knows what a kernel is. It starts processes, returns //! stdout, and turns a non-zero exit into an [`Error`]. -#![allow(dead_code)] // reachable as soon as the patch verbs stop being stubs +use std::io::Write as _; +use std::path::PathBuf; +use std::process::{Command, Stdio}; -use std::path::{Path, PathBuf}; -use std::process::Command; - -use crate::error::{Error, Result}; +use crate::error::{Error, Result, exit_code}; /// A git invocation rooted at one repository. #[derive(Debug, Clone)] @@ -21,16 +20,144 @@ impl Git { Self { repo: repo.into() } } - #[must_use] - pub fn repo(&self) -> &Path { - &self.repo - } - /// Run git in the repository and return its trimmed stdout. /// /// This is the only place a git process is started. Every other method /// here goes through it. pub fn run(&self, args: &[&str]) -> Result { + let stdout = self.run_bytes(args)?; + Ok(String::from_utf8_lossy(&stdout).trim_end().to_owned()) + } + + /// Run git and hand back stdout byte for byte. + /// + /// Patches come through here. A patch whose last line lost its newline is + /// one checkpatch and `git apply` can both misread, so nothing trims it. + pub fn run_bytes(&self, args: &[&str]) -> Result> { + Ok(self.output(args)?.stdout) + } + + /// Apply `patch`, which arrives on stdin, with `args` in front of the `-`. + /// + /// Reading the patch from a pipe keeps it in memory until it is known to + /// apply, and leaves nothing on disk to clean up. + pub fn apply(&self, args: &[&str], patch: &[u8]) -> Result<()> { + let mut child = Command::new("git") + .arg("-C") + .arg(&self.repo) + .arg("apply") + .args(args) + .arg("-") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .map_err(|source| Error::CommandSpawn { + program: format!("git apply {}", args.join(" ")), + source, + })?; + + if let Some(mut stdin) = child.stdin.take() { + stdin.write_all(patch)?; + } + + let output = child.wait_with_output()?; + if !output.status.success() { + return Err(Error::ExternalCommand { + 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(), + }); + } + + Ok(()) + } + + /// Run git and hand back everything it said, stdout then stderr. + /// + /// `send-email` is the caller: its plan goes to stdout and its complaints + /// to stderr, and a reader wants both. A non-zero exit is still an error. + pub fn run_all(&self, args: &[&str]) -> Result { + let output = self.output(args)?; + + Ok(format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + )) + } + + /// Run git with the terminal attached, for a subcommand that may want to + /// ask the user something. + /// + /// `git send-email` confirms before it sends, and answering that needs the + /// terminal this process was started from. What it printed went there + /// already, so only a failure comes back. + pub fn run_interactive(&self, args: &[&str]) -> Result<()> { + let status = Command::new("git") + .arg("-C") + .arg(&self.repo) + .args(args) + .status() + .map_err(|source| Error::CommandSpawn { + program: format!("git {}", args.join(" ")), + source, + })?; + + if !status.success() { + return Err(Error::ExternalCommand { + program: "git".to_owned(), + args: args.join(" "), + code: exit_code(&status), + stderr: "nothing was captured; read the output above".to_owned(), + }); + } + + Ok(()) + } + + /// The uncommitted diff, exactly as `git diff` writes it. + /// + /// Staged-but-uncommitted work is not in here; that is what a revision + /// range is for. + pub fn diff_unstaged(&self) -> Result> { + self.run_bytes(&["diff"]) + } + + /// The patches for `range`, mail-formatted. + /// + /// `git send-email` refuses a bare diff with "No subject line in ...?", so + /// the mail form is the only form worth handing it. + pub fn format_patch(&self, range: &str) -> Result> { + self.run_bytes(&["format-patch", "--stdout", range]) + } + + /// Commit the index, optionally signing off and optionally amending. + pub fn commit(&self, message: &str, signoff: bool, amend: bool) -> Result<()> { + let mut args = vec!["commit", "-m", message]; + if signoff { + args.push("--signoff"); + } + if amend { + args.push("--amend"); + } + self.run(&args)?; + + Ok(()) + } + + /// Resolve a revision to a commit hash. + /// + /// 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]) + } + + fn output(&self, args: &[&str]) -> Result { let output = Command::new("git") .arg("-C") .arg(&self.repo) @@ -45,40 +172,13 @@ impl Git { return Err(Error::ExternalCommand { program: "git".to_owned(), args: args.join(" "), - code: output - .status - .code() - .map_or_else(|| "signal".to_owned(), |code| code.to_string()), + code: exit_code(&output.status), stderr: String::from_utf8_lossy(&output.stderr) .trim_end() .to_owned(), }); } - Ok(String::from_utf8_lossy(&output.stdout) - .trim_end() - .to_owned()) - } - - // TODO: each of these is a few lines over `run`, still unwritten. - - /// The branch currently checked out, or `HEAD` when detached. - pub fn current_branch(&self) -> Result { - todo!("git rev-parse --abbrev-ref HEAD") - } - - /// The working tree diff against `base`, for piping into a patch file. - pub fn diff_against(&self, base: &str) -> Result { - todo!("git diff {base}: needs --stat/--no-prefix decisions before it is real") - } - - /// Commit the index, optionally signing off and optionally amending. - pub fn commit(&self, message: &str, signoff: bool, amend: bool) -> Result<()> { - todo!("git commit -m {message:?} -s={signoff} --amend={amend}") - } - - /// Resolve a revision to a commit hash. - pub fn rev_parse(&self, rev: &str) -> Result { - todo!("git rev-parse {rev}") + Ok(output) } } diff --git a/src/main.rs b/src/main.rs index f3fec19..94008aa 100644 --- a/src/main.rs +++ b/src/main.rs @@ -24,7 +24,7 @@ async fn main() -> ExitCode { let cli = Cli::parse(); match run(cli).await { - Ok(()) => ExitCode::SUCCESS, + Ok(code) => code, Err(error) => { eprintln!("spectral: {error}"); ExitCode::FAILURE @@ -32,9 +32,9 @@ async fn main() -> ExitCode { } } -/// Command modules do the work and return what they produced. Printing it is -/// `main`'s job. -async fn run(cli: Cli) -> Result<()> { +/// Command modules do the work and return what they produced. Printing it, and +/// deciding the exit code, is `main`'s job. +async fn run(cli: Cli) -> Result { let config = Config::load()?; match cli.command { @@ -43,32 +43,62 @@ async fn run(cli: Cli) -> Result<()> { let issue = kernel::quest::run(&config, args).await?; println!("{} {}", issue.id, issue.title); println!("{}", issue.url); - Ok(()) + Ok(ExitCode::SUCCESS) } - KernelCommand::Test(args) => kernel::qemu::run(&config, args).await, + KernelCommand::Test(args) => kernel::qemu::run(&config, args) + .await + .map(|()| ExitCode::SUCCESS), }, Command::Patch { command } => match command { PatchCommand::Check(args) => { let report = patch::check(&config, args)?; print!("{}", report.output); - Ok(()) + Ok(exit_code(&report)) } PatchCommand::Format(args) => { let report = patch::format(&config, args)?; print!("{}", report.output); - Ok(()) + if !report.is_clean() { + eprintln!("spectral: checkpatch still reports problems"); + } + Ok(exit_code(&report)) } - PatchCommand::Commit(args) => patch::commit(&config, args), + PatchCommand::Commit(args) => patch::commit(&config, args).map(|()| ExitCode::SUCCESS), PatchCommand::Create(args) => { println!("{}", patch::create(&config, args)?.display()); - Ok(()) + Ok(ExitCode::SUCCESS) + } + PatchCommand::Submit(args) => { + let submission = patch::submit(&config, args)?; + for path in &submission.recipients.touched { + println!("Files: {}", path.display()); + } + for address in &submission.recipients.to { + println!("To: {address}"); + } + for address in &submission.recipients.cc { + println!("Cc: {address}"); + } + println!("{}", submission.command); + print!("{}", submission.output); + Ok(ExitCode::SUCCESS) } - PatchCommand::Submit(args) => patch::submit(&config, args), PatchCommand::Update(args) => { println!("{}", patch::update(&config, args)?.display()); - Ok(()) + Ok(ExitCode::SUCCESS) } }, } } + +/// A patch with errors in it fails the command; one with only warnings does +/// not, because a warning is a suggestion rather than something to stop the +/// loop for. +fn exit_code(report: &patch::checkpatch::Report) -> ExitCode { + if report.errors == 0 { + ExitCode::SUCCESS + } else { + ExitCode::FAILURE + } +} diff --git a/src/patch/checkpatch.rs b/src/patch/checkpatch.rs index 08ddb88..ff651a5 100644 --- a/src/patch/checkpatch.rs +++ b/src/patch/checkpatch.rs @@ -1,19 +1,27 @@ //! `scripts/checkpatch.pl`, from the kernel tree you are working in. -#![allow(dead_code)] // reachable as soon as the patch verbs stop being stubs - +use std::io::Write as _; use std::path::{Path, PathBuf}; +use std::process::{Command, Stdio}; -use crate::error::Result; +use crate::error::{Error, Result}; +use crate::git::Git; /// What checkpatch.pl should look at. #[derive(Debug, Clone)] pub enum Target { - /// The uncommitted working tree, via `--git`. + /// The uncommitted working tree, piped in as a diff. + /// + /// checkpatch cannot read a working tree by itself: `--git` with no + /// revision exits with "no git commits after extraction". WorkingTree, /// A committed range, e.g. `HEAD~1`. Rev(String), - /// A patch file, via `--file`. + /// A patch file, which checkpatch reads as a patch by default. + /// + /// `--file` is not the flag for this. checkpatch reads that as "the + /// argument is a C source file", and reports a patch as clean without ever + /// looking at the diff. File(PathBuf), } @@ -36,12 +44,97 @@ impl Report { /// Run checkpatch.pl over `target`. /// -/// `fix` adds `--fix`, which rewrites the patch file in place. Only -/// `Target::File` supports it, and the caller has to have the change -/// committed first so a bad fix can be reverted. +/// `fix` adds `--fix-inplace`, which rewrites a patch file in place, so only +/// [`Target::File`] can take it. checkpatch refuses `--git` together with +/// `--fix` anyway, and this rejects the combination before the process starts. pub fn run(kernel_tree: &Path, target: &Target, strict: bool, fix: bool) -> Result { - todo!( - "scripts/checkpatch.pl --no-tree in {} on {target:?} (strict={strict}, fix={fix})", - kernel_tree.display() - ) + if fix && matches!(target, Target::Rev(_)) { + return Err(Error::FixNeedsPatchFile); + } + + let mut command = Command::new(kernel_tree.join("scripts/checkpatch.pl")); + command + .current_dir(kernel_tree) + .arg("--no-tree") + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + + if strict { + command.arg("--strict"); + } + if fix { + command.arg("--fix-inplace"); + } + + let diff = match target { + Target::WorkingTree => { + let diff = Git::new(kernel_tree).diff_unstaged()?; + command.arg("-").stdin(Stdio::piped()); + Some(diff) + } + Target::Rev(rev) => { + command.args(["--git", rev]); + None + } + Target::File(path) => { + command.arg(path); + None + } + }; + + let output = match diff { + Some(diff) => { + let mut child = command.spawn().map_err(spawn_failed)?; + if let Some(mut stdin) = child.stdin.take() { + stdin.write_all(&diff)?; + } + child.wait_with_output()? + } + None => command.output().map_err(spawn_failed)?, + }; + + let text = format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + let (errors, warnings) = parse_totals(&text); + + Ok(Report { + errors, + warnings, + output: text, + }) +} + +fn spawn_failed(source: std::io::Error) -> Error { + Error::CommandSpawn { + program: "scripts/checkpatch.pl".to_owned(), + source, + } +} + +/// The error and warning counts from checkpatch's `total:` line. +/// +/// The line is the only reliable source. A clean run under `--terse`, which +/// implies `--quiet`, prints no total line at all, so a run with nothing to +/// report counts as clean. The exit code is deliberately not consulted: +/// checkpatch fails a run it considers merely unfashionable too. +fn parse_totals(output: &str) -> (usize, usize) { + for line in output.lines().rev() { + let Some(rest) = line.trim().strip_prefix("total:") else { + continue; + }; + + // `total: 4 errors, 2 warnings, 0 checks, 9 lines checked`, with the + // checks count present only under --strict. + let mut fields = rest.split_whitespace(); + let errors = fields.next().and_then(|n| n.parse().ok()).unwrap_or(0); + let _unit = fields.next(); + let warnings = fields.next().and_then(|n| n.parse().ok()).unwrap_or(0); + + return (errors, warnings); + } + + (0, 0) } diff --git a/src/patch/maintainers.rs b/src/patch/maintainers.rs index 9cd3cb8..dc25494 100644 --- a/src/patch/maintainers.rs +++ b/src/patch/maintainers.rs @@ -1,17 +1,26 @@ //! `scripts/get_maintainer.pl`, so `patch submit` knows who to mail. - -#![allow(dead_code)] // reachable as soon as the patch verbs stop being stubs +//! +//! The script is the kernel's own, taken from the tree being worked in, and it +//! answers with the roles it knows: maintainers, reviewers, the lists, and +//! whoever git remembers signing off on those files. Sorting those roles into +//! the `To:` and `Cc:` a patch needs is the only judgement this module makes. use std::path::{Path, PathBuf}; +use std::process::Command; -use crate::error::Result; +use crate::error::{Error, Result, exit_code}; /// Who a patch goes to, already split into the two headers git send-email wants. +/// +/// Entries keep the spelling the script used, `Name
` when it knows a +/// name and the bare address for a list, because a maintainer reading their own +/// name in the header is the point. Nothing is added twice: two spellings of one +/// address are one recipient. #[derive(Debug, Clone, Default)] pub struct Recipients { - /// Maintainers listed with a `(M)` role, plus the lists they own. + /// Maintainers, and the subsystem's own list. pub to: Vec, - /// Reviewers, lists, and everyone else worth a courtesy copy. + /// Reviewers, other lists, and everyone git remembers signing off. pub cc: Vec, /// Files the patch touches, which is what the lookup runs against. pub touched: Vec, @@ -19,14 +28,329 @@ pub struct Recipients { /// Look up recipients for a patch. /// -/// Runs `get_maintainer.pl --roles=... --git` over the patch's diff. That also -/// gives us the files the patch touches. +/// 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 { - todo!("get_maintainer.pl --git on {patch:?} inside {kernel_tree:?}") + 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, + ], + )?; + + let entries: Vec = output.lines().filter_map(parse_entry).collect(); + + Ok(split(&entries, touched_files(patch)?)) } /// Add extra addresses to the CC list, skipping ones already there. +/// +/// An address the `To:` line already carries is skipped too: a maintainer +/// copied on their own mail reads as a mistake. pub fn add_cc(recipients: &mut Recipients, extra: &[String]) { - let _ = (recipients, extra); - todo!("push the extra addresses onto the CC list, deduped") + for address in extra { + if contains(&recipients.to, address) || contains(&recipients.cc, address) { + continue; + } + recipients.cc.push(address.clone()); + } +} + +/// One line of the script's output, split into a name, an address, and roles. +#[derive(Debug, Clone)] +struct Entry { + /// The display name the script gave, still quoted if the script quoted it. + name: Option, + address: String, + /// Lowercased role words, e.g. `maintainer`, `open list`, `authored`. + roles: Vec, +} + +impl Entry { + /// How the entry reads in a header: `Name
`, or the bare address + /// for a list, which is how the script writes one. + fn recipient(&self) -> String { + match &self.name { + Some(name) => format!("{name} <{}>", self.address), + None => self.address.clone(), + } + } +} + +/// Sort the script's entries into the two headers. +/// +/// Maintainers go to `To:`, along with the first list-family entry for the +/// patch, which is the subsystem's own list. Reviewers, further lists, and +/// everyone git remembers signing off go to `Cc:`. +fn split(entries: &[Entry], touched: Vec) -> Recipients { + let mut recipients = Recipients { + touched, + ..Recipients::default() + }; + let mut primary_list_taken = false; + + for entry in entries { + let list = entry.roles.iter().any(|role| role.ends_with("list")); + let to = + entry.roles.iter().any(|role| role == "maintainer") || (list && !primary_list_taken); + if list && !primary_list_taken { + primary_list_taken = true; + } + + if to { + push_unique(&mut recipients.to, &entry.address, entry.recipient()); + } else { + push_unique(&mut recipients.cc, &entry.address, entry.recipient()); + } + } + + let addressed = recipients.to.clone(); + recipients + .cc + .retain(|recipient| !contains(&addressed, address_of(recipient))); + + recipients +} + +/// Parse `Name
(role:subsystem)` or `address (role:subsystem)`. +/// +/// The role group is whatever sits in the last pair of parentheses, and its +/// first colon-separated field holds the roles, comma-separated for a git +/// signer: `(authored,added_lines)`. +fn parse_entry(line: &str) -> Option { + let line = line.trim(); + let (head, roles) = match line.rfind('(') { + Some(at) if line.ends_with(')') => (line[..at].trim(), &line[at + 1..line.len() - 1]), + _ => (line, ""), + }; + + let (name, address) = match head.rfind('<') { + Some(at) if head.ends_with('>') => ( + Some(head[..at].trim().to_owned()), + head[at + 1..head.len() - 1].to_owned(), + ), + _ => (None, head.to_owned()), + }; + if !address.contains('@') { + return None; + } + + let roles = roles + .split(':') + .next() + .unwrap_or_default() + .split(',') + .map(|role| role.trim().to_ascii_lowercase()) + .filter(|role| !role.is_empty()) + .collect(); + + Some(Entry { + name, + address, + roles, + }) +} + +/// The files a patch touches, read out of its `diff --git` headers. +/// +/// 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)?; + + Ok(String::from_utf8_lossy(&patch) + .lines() + .filter_map(|line| line.strip_prefix("diff --git ")) + .filter_map(|rest| rest.rsplit_once(" b/")) + .map(|(_, path)| PathBuf::from(path.trim_matches('"'))) + .collect()) +} + +/// Add a recipient if its address is not already among `recipients`. +/// +/// The display form is kept; only the address decides whether it is a +/// duplicate, because that is what mail is routed on. +fn push_unique(recipients: &mut Vec, address: &str, recipient: String) { + if !contains(recipients, address) { + recipients.push(recipient); + } +} + +fn contains(recipients: &[String], address: &str) -> bool { + recipients + .iter() + .any(|existing| address_of(existing).eq_ignore_ascii_case(address)) +} + +/// The address inside a recipient, whether it arrived as `Name
` or +/// bare. +fn address_of(recipient: &str) -> &str { + match recipient.rfind('<') { + Some(at) if recipient.ends_with('>') => &recipient[at + 1..recipient.len() - 1], + _ => recipient, + } +} + +/// Run the tree's copy of the script and hand back everything it said. +fn run(kernel_tree: &Path, args: &[&str]) -> Result { + let output = Command::new(kernel_tree.join("scripts/get_maintainer.pl")) + .current_dir(kernel_tree) + .args(args) + .output() + .map_err(|source| Error::CommandSpawn { + program: "scripts/get_maintainer.pl".to_owned(), + source, + })?; + + if !output.status.success() { + return Err(Error::ExternalCommand { + program: "scripts/get_maintainer.pl".to_owned(), + args: args.join(" "), + code: exit_code(&output.status), + stderr: String::from_utf8_lossy(&output.stderr) + .trim_end() + .to_owned(), + }); + } + + // stderr is kept: the script warns about a thin history there, and quietly + // dropping it would hide a reason a role is missing. + Ok(format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + )) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Lines the real script printed for a fixture tree, kept verbatim. + const REAL_LINES: &[&str] = &[ + "Mathias Nyman (maintainer:USB XHCI HOST CONTROLLER DRIVER)", + "linux-usb@vger.kernel.org (open list:USB XHCI HOST CONTROLLER DRIVER)", + "Greg Kroah-Hartman (maintainer:USB SUBSYSTEM)", + "\"GitAuthor: Fixture\" (authored,added_lines)", + "someone@example.com (reviewer:SOMETHING ELSE)", + "another@example.com (moderated list:SOMETHING ELSE)", + ]; + + fn split_lines(lines: &[&str]) -> Recipients { + let entries: Vec = lines.iter().filter_map(|line| parse_entry(line)).collect(); + + split(&entries, Vec::new()) + } + + #[test] + fn lookup_puts_maintainers_and_the_subsystem_list_on_to() { + let recipients = split_lines(REAL_LINES); + + assert_eq!( + recipients.to, + vec![ + "Mathias Nyman ".to_owned(), + "linux-usb@vger.kernel.org".to_owned(), + "Greg Kroah-Hartman ".to_owned(), + ] + ); + } + + #[test] + fn lookup_puts_reviewers_other_lists_and_signers_on_cc() { + let recipients = split_lines(REAL_LINES); + + assert_eq!( + recipients.cc, + vec![ + "\"GitAuthor: Fixture\" ".to_owned(), + "someone@example.com".to_owned(), + "another@example.com".to_owned(), + ] + ); + } + + #[test] + fn lookup_keeps_an_address_off_the_cc_line_once_it_is_on_to() { + let recipients = split_lines(&[ + "Greg Kroah-Hartman (maintainer:USB SUBSYSTEM)", + "Greg Kroah-Hartman (supporter:USB SUBSYSTEM)", + "GREGKH@linuxfoundation.org (contributor:USB SUBSYSTEM)", + ]); + + assert_eq!( + recipients.to, + vec!["Greg Kroah-Hartman ".to_owned()] + ); + assert!(recipients.cc.is_empty(), "{:?}", recipients.cc); + } + + #[test] + fn lookup_keeps_the_name_the_script_gave() { + let recipients = split_lines(REAL_LINES); + + assert_eq!(address_of(&recipients.to[0]), "mathias.nyman@intel.com"); + assert_eq!(address_of(&recipients.to[1]), "linux-usb@vger.kernel.org"); + } + + #[test] + fn lookup_reads_the_roles_out_of_the_last_parenthetical() { + let entry = parse_entry("Uwe (West) Kleine-Konig (maintainer:PWM)") + .expect("one entry"); + + assert_eq!(entry.address, "u@example.com"); + assert_eq!(entry.roles, vec!["maintainer".to_owned()]); + } + + #[test] + fn lookup_ignores_a_line_with_no_address() { + assert!(parse_entry("Bad divisor in main::vcs_assign: 0").is_none()); + assert!(parse_entry("").is_none()); + } + + #[test] + fn add_cc_skips_duplicates_and_anyone_already_on_to() { + let mut recipients = split_lines(REAL_LINES); + + add_cc( + &mut recipients, + &[ + "extra@example.com".to_owned(), + "EXTRA@example.com".to_owned(), + "mathias.nyman@intel.com".to_owned(), + ], + ); + + assert_eq!(recipients.cc.len(), 4, "{:?}", recipients.cc); + assert_eq!(recipients.cc[3], "extra@example.com"); + } + + #[test] + fn touched_files_come_out_of_the_patch_headers() { + let dir = std::env::temp_dir().join(format!("spectral-maintainers-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("temp dir"); + let patch = dir.join("thing.patch"); + std::fs::write( + &patch, + "diff --git a/drivers/foo/bar.c b/drivers/foo/bar.c\nindex 1..2 100644\n--- a/x\n+++ b/x\n@@ -1 +1 @@\n-a\n+b\ndiff --git a/README b/README\n", + ) + .expect("write the patch"); + + let touched = touched_files(&patch).expect("read the patch"); + + assert_eq!( + touched, + vec![PathBuf::from("drivers/foo/bar.c"), PathBuf::from("README")] + ); + + let _ = std::fs::remove_dir_all(&dir); + } } diff --git a/src/patch/mod.rs b/src/patch/mod.rs index 3d7f38c..a14d26d 100644 --- a/src/patch/mod.rs +++ b/src/patch/mod.rs @@ -1,6 +1,6 @@ //! `spectral patch`: carry a change from working tree to mailing list. //! -//! The verbs are all stubs. The order they are meant to be run in is: +//! The order the verbs are meant to be run in: //! //! ```text //! check ─▶ format ─▶ commit ─▶ create ─▶ submit @@ -8,8 +8,10 @@ //! └─ update┘ (v2, v3, …) //! ``` //! -//! Each verb gets the tree from [`Config::require_kernel_tree`] and builds a -//! [`crate::git::Git`] over it, which is how it reaches the plumbing. +//! Every verb is written. Each one that touches the tree gets it from +//! [`Config::require_kernel_tree`] and builds a [`Git`] over it, which is how it +//! reaches the plumbing; the ones that only move a patch file around work +//! without a tree at all. pub mod checkpatch; pub mod maintainers; @@ -18,43 +20,156 @@ use std::path::{Path, PathBuf}; use crate::cli::{CheckArgs, CommitArgs, CreateArgs, FormatArgs, SubmitArgs, UpdateArgs}; use crate::config::Config; -use crate::error::Result; +use crate::error::{Error, Result}; +use crate::git::Git; /// `checkpatch.pl` over the work in progress. pub fn check(config: &Config, args: CheckArgs) -> Result { - let _ = (config, args); - todo!("resolve the target, run checkpatch, report errors and warnings") + let tree = config.require_kernel_tree()?; + let target = match (&args.patch, &args.rev) { + (Some(patch), _) => checkpatch::Target::File(resolve_patch(config, patch)?), + (None, Some(rev)) => checkpatch::Target::Rev(rev.clone()), + (None, None) => checkpatch::Target::WorkingTree, + }; + + checkpatch::run(tree, &target, args.strict, false) } -/// Auto-fix whatever `check` reported. +/// Auto-fix whatever `check` reported, then check again. +/// +/// The report that comes back is the second one, so what it prints is what is +/// left after the fix rather than what prompted it. pub fn format(config: &Config, args: FormatArgs) -> Result { - let _ = (config, args); - todo!("checkpatch --fix, then re-run check to show what is left") + let tree = config.require_kernel_tree()?; + + match &args.patch { + Some(path) => { + let target = checkpatch::Target::File(resolve_patch(config, path)?); + checkpatch::run(tree, &target, false, true)?; + + checkpatch::run(tree, &target, false, false) + } + None => format_working_tree(tree), + } } /// Commit the work in progress with a kernel-style message. pub fn commit(config: &Config, args: CommitArgs) -> Result<()> { - let _ = (config, args); - todo!("git commit, appending Signed-off-by when asked") + let tree = config.require_kernel_tree()?; + + Git::new(tree).commit(&args.message, args.signoff, args.amend) } -/// Write the diff against the base branch out as a patch file. +/// 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. pub fn create(config: &Config, args: CreateArgs) -> Result { - let _ = (config, args); - todo!("git diff > /.patch") + 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(), + })?; + + // The patch is mail-formatted rather than a bare diff: `git send-email` + // refuses the bare one with "No subject line". + let set = git.format_patch(&format!("{base}..HEAD"))?; + let count = patch_count(&set); + if count == 0 { + return Err(Error::NoCommitsToSend { base: args.base }); + } + if count > 1 { + return Err(Error::RangeHoldsSeveralCommits { + base: args.base, + count, + }); + } + + 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)); + std::fs::write(&path, set)?; + + Ok(path) +} + +/// What a `submit` run decided, so `main` can print it. +#[derive(Debug, Clone)] +pub struct Submission { + /// Who the patch is addressed to, after the split and any extra `--cc`. + pub recipients: maintainers::Recipients, + /// The `git send-email` command line that was run. + pub command: String, + /// Everything send-email said, empty for a real send because that went to + /// the terminal send-email had to ask its confirmation question on. + pub output: String, } /// Send a patch to whoever `get_maintainer.pl` names. /// -/// With `--dry-run` it prints the recipients and the exact `git send-email` -/// invocation instead of sending anything. -pub fn submit(config: &Config, args: SubmitArgs) -> Result<()> { - let _ = (config, args); - todo!("look up recipients, then git send-email") +/// 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 +/// holding its template subject for one, come back as errors rather than being +/// 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 mut recipients = maintainers::lookup(tree, &patch)?; + maintainers::add_cc(&mut recipients, &args.cc); + + let mut argv: Vec = vec!["send-email".to_owned()]; + if args.dry_run { + argv.push("--dry-run".to_owned()); + // A dry run is not a decision to send anything, so it never waits for + // an answer to send-email's confirmation question. + argv.push("--confirm=never".to_owned()); + } + for address in &recipients.to { + argv.push("--to".to_owned()); + argv.push(address.clone()); + } + for address in &recipients.cc { + argv.push("--cc".to_owned()); + argv.push(address.clone()); + } + if let Some(message_id) = &args.in_reply_to { + argv.push("--in-reply-to".to_owned()); + argv.push(message_id.clone()); + } + argv.push(patch.to_string_lossy().into_owned()); + + let borrowed: Vec<&str> = argv.iter().map(String::as_str).collect(); + let git = Git::new(tree); + let output = if args.dry_run { + git.run_all(&borrowed)? + } else { + git.run_interactive(&borrowed)?; + String::new() + }; + + Ok(Submission { + recipients, + command: format!("git -C {} {}", tree.display(), shell_join(&argv)), + output, + }) +} + +/// A command line a reader can paste, with the awkward arguments quoted. +fn shell_join(argv: &[String]) -> String { + argv.iter() + .map(|arg| { + if arg.contains(' ') { + format!("'{arg}'") + } else { + arg.clone() + } + }) + .collect::>() + .join(" ") } /// Re-roll a patch as `v`. @@ -64,18 +179,226 @@ pub fn submit(config: &Config, args: SubmitArgs) -> Result<()> { /// revision is a no-op. `args.revision` overrides the inferred next number. /// Returns the path of the renamed file. pub fn update(config: &Config, args: UpdateArgs) -> Result { - let _ = (config, args); - todo!("work out N, rename the file, leave the contents alone") + let patch = resolve_patch(config, &args.patch)?; + let revision = match args.revision { + Some(revision) => revision, + None => current_revision(&patch) + 1, + }; + let renamed = reroll_path(&patch, revision); + + if renamed == patch { + return Ok(renamed); + } + if renamed.exists() { + return Err(Error::RerollWouldOverwrite { path: renamed }); + } + + std::fs::rename(&patch, &renamed)?; + + Ok(renamed) } /// The path `patch` should have once it is revision `revision`. /// /// Kept separate from [`update`] because it is the rule worth having a test /// for: strip one leading `vN-`, then prepend `v-`. -#[allow(dead_code)] // called by `update` once that is written pub fn reroll_path(patch: &Path, revision: u32) -> PathBuf { - todo!( - "strip the vN- prefix from {} if present, then prepend v{revision}-", - patch.display() - ) + let name = patch + .file_name() + .map_or_else(String::new, |name| name.to_string_lossy().into_owned()); + + patch.with_file_name(format!("v{revision}-{}", strip_revision(&name))) +} + +/// Drop one leading `vN-`, if the name has one. +fn strip_revision(name: &str) -> &str { + let Some(rest) = name.strip_prefix('v') else { + return name; + }; + let Some(dash) = rest.find('-') else { + return name; + }; + + match rest.get(..dash) { + Some(digits) if !digits.is_empty() && digits.bytes().all(|b| b.is_ascii_digit()) => { + &rest[dash + 1..] + } + _ => name, + } +} + +/// 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. +fn resolve_patch(config: &Config, name: &Path) -> Result { + if name.is_file() { + return Ok(name.to_path_buf()); + } + + let file_name = name.file_name().map_or_else( + || name.display().to_string(), + |name| name.to_string_lossy().into_owned(), + ); + let in_patch_dir = config.patch_dir().join(&file_name); + if in_patch_dir.is_file() { + return Ok(in_patch_dir); + } + + // A name with a separator was meant as a path, so the complaint names the + // directory the user pointed at rather than the one patches live in. + let dir = match name.parent() { + Some(parent) if !parent.as_os_str().is_empty() => parent.to_path_buf(), + _ => config.patch_dir().to_path_buf(), + }; + + Err(Error::PatchNotFound { + name: file_name, + dir, + }) +} + +/// The revision a patch's name claims, or 0 when it claims none. +fn current_revision(patch: &Path) -> u32 { + patch + .file_name() + .and_then(|name| name.to_str()) + .and_then(|name| name.strip_prefix('v')) + .and_then(|rest| rest.split_once('-')) + .and_then(|(digits, _)| digits.parse().ok()) + .unwrap_or(0) +} + +/// How many patches `git format-patch` put in the set. +/// +/// Each one opens with a `From Mon Sep 17 00:00:00 2001` line, which is +/// cheaper to count than asking git for the commit count all over again. +fn patch_count(set: &[u8]) -> usize { + String::from_utf8_lossy(set) + .lines() + .filter(|line| line.starts_with("From ") && line.ends_with("2001")) + .count() +} + +/// A patch name always ends in `.patch`. +fn patch_name(name: &str) -> String { + if name.ends_with(".patch") { + name.to_owned() + } else { + format!("{name}.patch") + } +} + +/// Put checkpatch's fix into the working tree. +/// +/// `--fix-inplace` rewrites a patch file and a working tree is not one, so the +/// change goes out as a patch, gets fixed, and comes back. The tree drops to +/// the base first, and returns to the user's own unfixed change if the fixed +/// patch turns out not to apply, so a bad fix costs nothing. +fn format_working_tree(tree: &Path) -> Result { + let git = Git::new(tree); + let original = git.diff_unstaged()?; + + if original.is_empty() { + return checkpatch::run(tree, &checkpatch::Target::WorkingTree, false, false); + } + + let scratch = + std::env::temp_dir().join(format!("spectral-format-{}.patch", std::process::id())); + std::fs::write(&scratch, &original)?; + let fixed = checkpatch::run( + tree, + &checkpatch::Target::File(scratch.clone()), + false, + true, + ); + let fixed_patch = std::fs::read(&scratch); + let _ = std::fs::remove_file(&scratch); + fixed?; + let fixed_patch = fixed_patch?; + + if fixed_patch != original { + git.apply(&["--reverse"], &original) + .map_err(|_| Error::FixApplyFailed { + reason: "the uncommitted change could not be set aside; commit or stash it first" + .to_owned(), + })?; + + if git.apply(&[], &fixed_patch).is_err() { + let _ = git.apply(&[], &original); + return Err(Error::FixApplyFailed { + reason: "checkpatch's fixed patch does not apply".to_owned(), + }); + } + } + + checkpatch::run(tree, &checkpatch::Target::WorkingTree, false, false) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn reroll_path_adds_a_revision_to_an_unversioned_name() { + assert_eq!( + reroll_path(Path::new("/tmp/000-kernel-patch.patch"), 2), + PathBuf::from("/tmp/v2-000-kernel-patch.patch") + ); + } + + #[test] + fn reroll_path_replaces_one_revision_rather_than_stacking_them() { + assert_eq!( + reroll_path(Path::new("/tmp/v2-x.patch"), 3), + PathBuf::from("/tmp/v3-x.patch") + ); + assert_eq!( + reroll_path(Path::new("/tmp/v9-some-subject.patch"), 10), + PathBuf::from("/tmp/v10-some-subject.patch") + ); + } + + #[test] + fn reroll_path_at_the_same_revision_is_a_no_op() { + assert_eq!( + reroll_path(Path::new("/tmp/v2-x.patch"), 2), + PathBuf::from("/tmp/v2-x.patch") + ); + } + + #[test] + fn reroll_path_leaves_a_name_that_only_looks_versioned_alone() { + assert_eq!( + reroll_path(Path::new("/tmp/version-2.patch"), 2), + PathBuf::from("/tmp/v2-version-2.patch") + ); + assert_eq!( + reroll_path(Path::new("/tmp/vd-linux.patch"), 2), + PathBuf::from("/tmp/v2-vd-linux.patch") + ); + } + + #[test] + fn current_revision_reads_the_version_a_name_claims() { + assert_eq!(current_revision(Path::new("/tmp/000-foo.patch")), 0); + assert_eq!(current_revision(Path::new("/tmp/v2-000-foo.patch")), 2); + assert_eq!(current_revision(Path::new("/tmp/v10-x.patch")), 10); + assert_eq!(current_revision(Path::new("/tmp/version-2.patch")), 0); + } + + #[test] + fn patch_count_counts_the_patches_in_a_set() { + let one = b"From abcdef1234567890 Mon Sep 17 00:00:00 2001\nSubject: [PATCH] x\n"; + assert_eq!(patch_count(one), 1); + assert_eq!(patch_count(&[one.as_slice(), one.as_slice()].concat()), 2); + assert_eq!(patch_count(b""), 0); + } + + #[test] + fn patch_name_always_ends_in_dot_patch() { + assert_eq!(patch_name("000-foo"), "000-foo.patch"); + assert_eq!(patch_name("000-foo.patch"), "000-foo.patch"); + } } diff --git a/tests/cli_smoke.rs b/tests/cli_smoke.rs new file mode 100644 index 0000000..7689b51 --- /dev/null +++ b/tests/cli_smoke.rs @@ -0,0 +1,82 @@ +//! The command surface parses, and a bad invocation is a usage error rather +//! than a panic. + +mod common; + +use common::Fixture; + +#[test] +fn the_top_level_help_names_both_command_groups() { + let fixture = Fixture::bare(); + let output = fixture.cli(&["--help"]); + + assert!(output.status.success()); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(stdout.contains("kernel"), "{stdout}"); + assert!(stdout.contains("patch"), "{stdout}"); +} + +#[test] +fn every_verb_has_help_of_its_own() { + let fixture = Fixture::bare(); + + for verb in [ + &["kernel", "quest"][..], + &["kernel", "test"], + &["patch", "check"], + &["patch", "format"], + &["patch", "commit"], + &["patch", "create"], + &["patch", "submit"], + &["patch", "update"], + ] { + let mut args = verb.to_vec(); + args.push("--help"); + + let output = fixture.cli(&args); + assert!( + output.status.success(), + "{} --help failed: {}", + verb.join(" "), + String::from_utf8_lossy(&output.stderr) + ); + assert!( + !String::from_utf8_lossy(&output.stdout).is_empty(), + "{} --help printed nothing", + verb.join(" ") + ); + } +} + +#[test] +fn an_unknown_subcommand_exits_two() { + let fixture = Fixture::bare(); + let output = fixture.cli(&["frobnicate"]); + + assert_eq!(output.status.code(), Some(2)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("Usage"), "{stderr}"); +} + +#[test] +fn a_leaf_missing_its_argument_exits_two() { + let fixture = Fixture::bare(); + let output = fixture.cli(&["patch", "commit"]); + + assert_eq!(output.status.code(), Some(2)); + 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 new file mode 100644 index 0000000..4dd4666 --- /dev/null +++ b/tests/common/mod.rs @@ -0,0 +1,249 @@ +//! Fixture helpers shared by the CLI tests. +//! +//! A test never touches the machine it runs on. `HOME`, `SPECTRAL_KERNEL`, and +//! git's global configuration all point into a temp directory that deletes +//! itself when the test ends, so `~/.spectral` is never written and the +//! developer's git config never leaks in. +//! +//! The kernel tree, when a test wants one, is a real git repository holding the +//! two scripts spectral shells out to, a MAINTAINERS entry, and one commit. + +#![allow(dead_code)] // each test binary uses a different part of this set + +use std::os::unix::fs::PermissionsExt as _; +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; +use std::sync::atomic::{AtomicU32, Ordering}; + +/// The checkpatch stand-in. +/// +/// The real one is a kernel script whose output moves with the kernel version, +/// which is not something a test can assert on. This records how it was called, +/// then says what the test asked for through the environment: +/// `FIXTURE_SUMMARY` is printed as checkpatch's summary line, `FIXTURE_EXIT` is +/// the exit status, and `FIXTURE_RECORD` names the file the call is noted in. +const CHECKPATCH: &str = r#"#!/bin/sh +: "${FIXTURE_RECORD:=.checkpatch-call}" +{ + echo "argv: $*" + echo "cwd: $(pwd)" + cat +} >> "$FIXTURE_RECORD" +[ -n "$FIXTURE_SUMMARY" ] && printf '%s\n' "$FIXTURE_SUMMARY" +exit "${FIXTURE_EXIT:-0}" +"#; + +/// The get_maintainer stand-in, for the same reason. +/// +/// One line per role the real script emits, including a git signer whose +/// address is already on the `To:` line, so the dedup is exercised. +const GET_MAINTAINER: &str = r#"#!/bin/sh +echo "argv: $*" >> .getmaintainer-call +echo "Fixture Maintainer (maintainer:FIXTURE DRIVER)" +echo "fixture-list@example.com (open list:FIXTURE DRIVER)" +echo "Fixture Reviewer (reviewer:FIXTURE REVIEWERS)" +echo "another-list@example.com (open list:FIXTURE REVIEWERS)" +echo "\"GitAuthor: Fixture\" (authored,added_lines)" +"#; + +/// The file `return a;` is committed as, and the edit most tests start from. +pub const COMMITTED: &str = "static int foo(int a)\n{\n\treturn a;\n}\n"; +pub const EDITED: &str = "static int foo(int a)\n{\n\treturn a + 1;\n}\n"; + +/// A temp directory that removes itself when the test ends. +#[derive(Debug)] +pub struct TempDir { + path: PathBuf, +} + +impl TempDir { + fn new(label: &str) -> Self { + static NEXT: AtomicU32 = AtomicU32::new(0); + let path = std::env::temp_dir().join(format!( + "spectral-test-{}-{label}-{}", + std::process::id(), + NEXT.fetch_add(1, Ordering::Relaxed) + )); + std::fs::create_dir_all(&path).expect("create the temp directory"); + + Self { path } + } + + pub fn path(&self) -> &Path { + &self.path + } + + pub fn join(&self, rel: &str) -> PathBuf { + self.path.join(rel) + } +} + +impl Drop for TempDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.path); + } +} + +/// A throwaway home directory, and a kernel tree when the test wants one. +pub struct Fixture { + home: TempDir, + root: TempDir, +} + +impl Fixture { + /// A home with nothing in it but a git identity. + pub fn bare() -> Self { + let fixture = Self { + home: TempDir::new("home"), + root: TempDir::new("root"), + }; + std::fs::write( + fixture.home.join("gitconfig"), + "[user]\n\tname = Fixture\n\temail = fixture@example.com\n", + ) + .expect("write the fixture git config"); + + fixture + } + + /// A git repository shaped like a kernel tree. + pub fn kernel_tree() -> Self { + let fixture = Self::bare(); + // Pinned: `create` defaults to `--base master`, the kernel's branch, + // and the fixture should not inherit whatever a machine's git defaults + // to for a new repository. + fixture.git(&["init", "--quiet", "--initial-branch=master"]); + fixture.git(&["config", "user.name", "Fixture"]); + fixture.git(&["config", "user.email", "fixture@example.com"]); + fixture.write( + "MAINTAINERS", + "FIXTURE DRIVER\nM:\tFixture Maintainer \nL:\tfixture-list@example.com\nS:\tMaintained\nF:\tdrivers/foo/*\n\nFIXTURE REVIEWERS\nR:\tFixture Reviewer \nL:\tanother-list@example.com\nS:\tOdd Fixes\nF:\tdrivers/foo/*\n", + ); + fixture.write("drivers/foo/bar.c", COMMITTED); + fixture.write_script("scripts/checkpatch.pl", CHECKPATCH); + fixture.write_script("scripts/get_maintainer.pl", GET_MAINTAINER); + fixture.commit_all("fixture: add bar"); + + fixture + } + + /// The directory the tests treat as `$HOME`. + pub fn home(&self) -> &Path { + self.home.path() + } + + /// The kernel tree, which is also the working directory the binary runs in. + pub fn tree(&self) -> &Path { + self.root.path() + } + + pub fn join(&self, rel: &str) -> PathBuf { + self.root.join(rel) + } + + /// Write a file into the tree, making its directory first. + pub fn write(&self, rel: &str, contents: &str) { + let path = self.join(rel); + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).expect("create the file's directory"); + } + std::fs::write(&path, contents).expect("write the file"); + } + + /// Write an executable script into the tree. + pub fn write_script(&self, rel: &str, body: &str) { + self.write(rel, body); + let path = self.join(rel); + 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") + } + + /// Run git in the tree and return its trimmed stdout. + pub fn git(&self, args: &[&str]) -> String { + let output = self.git_command(args).output().expect("run git"); + assert!( + output.status.success(), + "git {} failed: {}", + args.join(" "), + String::from_utf8_lossy(&output.stderr) + ); + + String::from_utf8_lossy(&output.stdout) + .trim_end() + .to_owned() + } + + /// Run git expecting a failure, and hand back its stderr. + /// + /// Used to pin the things spectral works around, such as send-email + /// refusing a bare diff. + pub fn git_must_fail(&self, args: &[&str]) -> String { + let output = self.git_command(args).output().expect("run git"); + assert!( + !output.status.success(), + "git {} was supposed to fail", + args.join(" ") + ); + + String::from_utf8_lossy(&output.stderr) + .trim_end() + .to_owned() + } + + pub fn commit_all(&self, message: &str) { + self.git(&["add", "-A"]); + self.git(&["commit", "--quiet", "-m", message]); + } + + /// Do the work on a branch of its own, the way the loop expects: `create` + /// diffs the branch against `master`, so committing straight to `master` + /// leaves it nothing to send. + pub fn work_on_a_branch(&self) { + self.git(&["checkout", "--quiet", "-b", "work"]); + } + + /// Run the built binary with the fixture's environment. + pub fn cli(&self, args: &[&str]) -> Output { + self.cli_env(args, &[]) + } + + /// 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")); + 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("SPECTRAL_KERNEL", self.root.path()) + .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") + } + + fn git_command(&self, args: &[&str]) -> Command { + let mut command = Command::new("git"); + command + .arg("-C") + .arg(self.root.path()) + .args(args) + .env("HOME", self.home.path()) + .env("GIT_CONFIG_GLOBAL", self.home.join("gitconfig")) + .env("GIT_CONFIG_SYSTEM", "/dev/null"); + + command + } +} diff --git a/tests/patch_verbs.rs b/tests/patch_verbs.rs new file mode 100644 index 0000000..b0437ae --- /dev/null +++ b/tests/patch_verbs.rs @@ -0,0 +1,708 @@ +//! The patch verbs, driven through the built binary against a fixture tree. + +mod common; + +use common::Fixture; + +/// Every stub records how it was called, so a test can assert the arguments +/// spectral really passed and see the diff that came in on stdin. +const RECORD: &str = r#": "${FIXTURE_RECORD:=.checkpatch-call}" +{ + echo "argv: $*" + echo "cwd: $(pwd)" + cat +} >> "$FIXTURE_RECORD" +"#; + +/// Build a checkpatch stand-in from the part that differs between tests. +fn stub(body: &str) -> String { + format!("#!/bin/sh\n{RECORD}{body}") +} + +/// Rewrites the patch's added line, the way a real fix pass does, then reports +/// clean. +const FIXER: &str = r#"if [ "$2" = "--fix-inplace" ]; then + sed -i 's/return a + 1/return a + 2/' "$3" +fi +echo "total: 0 errors, 0 warnings, 9 lines checked" +exit 0 +"#; + +/// Leaves a mark in the patch file it is given, for the case where nothing has +/// to be applied to a tree afterwards. +const TOUCHER: &str = r#"if [ "$2" = "--fix-inplace" ]; then + printf 'FIXED\n' >> "$3" +fi +echo "total: 0 errors, 0 warnings, 3 lines checked" +exit 0 +"#; + +/// A fix that is useless, for the path that has to undo one. +const RUINER: &str = r#"if [ "$2" = "--fix-inplace" ]; then + printf 'this is not a patch at all\n' > "$3" +fi +echo "total: 1 errors, 0 warnings, 3 lines checked" +exit 1 +"#; + +#[test] +fn commit_signs_off_and_keeps_the_message() { + let fixture = Fixture::kernel_tree(); + fixture.write("drivers/foo/bar.c", common::EDITED); + fixture.git(&["add", "-A"]); + + let output = fixture.cli(&["patch", "commit", "foo: return a + 1", "--signoff"]); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let message = fixture.git(&["log", "-1", "--pretty=%B"]); + assert!(message.contains("foo: return a + 1"), "{message}"); + assert!( + message.contains("Signed-off-by: Fixture "), + "{message}" + ); +} + +#[test] +fn commit_amend_replaces_the_previous_commit() { + let fixture = Fixture::kernel_tree(); + fixture.write("drivers/foo/bar.c", common::EDITED); + fixture.git(&["add", "-A"]); + + let output = fixture.cli(&["patch", "commit", "foo: reworded", "--amend"]); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!(fixture.git(&["rev-list", "--count", "HEAD"]), "1"); + assert!( + fixture + .git(&["log", "-1", "--pretty=%s"]) + .contains("reworded") + ); +} + +#[test] +fn create_writes_a_patch_that_send_email_accepts() { + 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"); + + let output = fixture.cli(&["patch", "create", "000-foo"]); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let printed = String::from_utf8_lossy(&output.stdout).trim().to_owned(); + assert_eq!( + printed, + fixture + .home() + .join(".spectral/patches/000-foo.patch") + .display() + .to_string() + ); + + let patch = std::fs::read_to_string(&printed).expect("the patch create wrote"); + assert!(patch.starts_with("From "), "{patch}"); + assert!( + patch.contains("Subject: [PATCH] foo: return a + 1"), + "{patch}" + ); + assert!(patch.contains("return a + 1;"), "{patch}"); + + let sent = fixture.git(&[ + "send-email", + "--dry-run", + "--confirm=never", + "--to", + "list@example.com", + &printed, + ]); + assert!(sent.contains("Dry-OK"), "{sent}"); +} + +#[test] +fn a_bare_diff_is_refused_by_send_email() { + // This is why `create` goes through format-patch, so the failure is pinned + // rather than rediscovered. + let fixture = Fixture::kernel_tree(); + fixture.write("drivers/foo/bar.c", common::EDITED); + let diff = fixture.git(&["diff"]); + let path = fixture.home().join("bare.patch"); + std::fs::write(&path, format!("{diff}\n")).expect("write the bare diff"); + + let stderr = fixture.git_must_fail(&[ + "send-email", + "--dry-run", + "--confirm=never", + "--to", + "list@example.com", + path.to_str().expect("the patch path"), + ]); + + assert!(stderr.contains("No subject line"), "{stderr}"); +} + +#[test] +fn create_names_a_base_that_does_not_resolve() { + let fixture = Fixture::kernel_tree(); + + let output = fixture.cli(&["patch", "create", "000-foo", "--base", "not-a-branch"]); + + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("no revision `not-a-branch`"), "{stderr}"); +} + +#[test] +fn create_refuses_a_range_that_holds_more_than_one_commit() { + let fixture = Fixture::kernel_tree(); + 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"); + + let output = fixture.cli(&["patch", "create", "000-foo", "--base", "HEAD~2"]); + + assert_eq!(output.status.code(), Some(1)); + 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}" + ); +} + +#[test] +fn create_refuses_when_there_is_nothing_to_send() { + let fixture = Fixture::kernel_tree(); + + let output = fixture.cli(&["patch", "create", "000-foo", "--base", "HEAD"]); + + assert_eq!(output.status.code(), Some(1)); + assert!(String::from_utf8_lossy(&output.stderr).contains("nothing to send")); +} + +#[test] +fn check_pipes_the_working_tree_diff_into_checkpatch_and_fails_on_errors() { + let fixture = Fixture::kernel_tree(); + fixture.write("drivers/foo/bar.c", common::EDITED); + + let output = fixture.cli_env( + &["patch", "check"], + &[ + ( + "FIXTURE_SUMMARY", + "total: 4 errors, 2 warnings, 9 lines checked", + ), + ("FIXTURE_EXIT", "1"), + ], + ); + + assert_eq!(output.status.code(), Some(1)); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("total: 4 errors, 2 warnings, 9 lines checked"), + "{stdout}" + ); + + // The diff really went in, rather than a revision being named. + let call = fixture.read(".checkpatch-call"); + assert!(call.contains("argv: --no-tree -"), "{call}"); + assert!(call.contains("+ return a + 1;"), "{call}"); + assert!( + call.contains(&format!("cwd: {}", fixture.tree().display())), + "{call}" + ); +} + +#[test] +fn warnings_alone_do_not_fail_the_command() { + let fixture = Fixture::kernel_tree(); + fixture.write("drivers/foo/bar.c", common::EDITED); + + let output = fixture.cli_env( + &["patch", "check"], + &[( + "FIXTURE_SUMMARY", + "total: 0 errors, 3 warnings, 9 lines checked", + )], + ); + + assert_eq!(output.status.code(), Some(0)); + assert!(String::from_utf8_lossy(&output.stdout).contains("0 errors, 3 warnings")); +} + +#[test] +fn a_run_with_no_total_line_counts_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(); + fixture.write("drivers/foo/bar.c", common::EDITED); + + let output = fixture.cli(&["patch", "check"]); + + assert_eq!(output.status.code(), Some(0)); + assert!(String::from_utf8_lossy(&output.stdout).trim().is_empty()); +} + +#[test] +fn a_patch_file_and_a_revision_go_in_as_themselves() { + let fixture = Fixture::kernel_tree(); + let patch = fixture.home().join("thing.patch"); + std::fs::write(&patch, "--- a/drivers/foo/bar.c\n+++ b/drivers/foo/bar.c\n").expect("write"); + + let by_file = fixture.cli(&[ + "patch", + "check", + patch.to_str().expect("the patch path"), + "--strict", + ]); + assert!(by_file.status.success()); + let call = fixture.read(".checkpatch-call"); + assert!(call.contains("--strict"), "{call}"); + assert!( + !call.contains("--file"), + "checkpatch reads --file as a source file, not as a patch: {call}" + ); + assert!(call.contains("thing.patch"), "{call}"); + + let by_rev = fixture.cli(&["patch", "check", "--rev", "HEAD"]); + assert!(by_rev.status.success()); + let call = fixture.read(".checkpatch-call"); + assert!(call.contains("--git HEAD"), "{call}"); +} + +#[test] +fn a_patch_file_that_is_not_there_is_named_as_such() { + let fixture = Fixture::kernel_tree(); + + let output = fixture.cli(&["patch", "check", "nope.patch"]); + + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("no patch named `nope.patch`"), "{stderr}"); +} + +#[test] +fn a_missing_tree_names_the_variable_that_would_have_set_it() { + let fixture = Fixture::bare(); + + let output = fixture.cli_env( + &["patch", "check"], + &[("SPECTRAL_KERNEL", "/nonexistent/linux")], + ); + + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("no kernel tree at `/nonexistent/linux`"), + "{stderr}" + ); + assert!(stderr.contains("SPECTRAL_KERNEL"), "{stderr}"); +} + +#[test] +fn format_rewrites_a_patch_file_in_place_and_checks_it_again() { + let fixture = Fixture::kernel_tree(); + fixture.write_script("scripts/checkpatch.pl", &stub(TOUCHER)); + let patch = fixture.home().join("thing.patch"); + std::fs::write(&patch, "--- a/x\n+++ b/x\n").expect("write the patch"); + + let output = fixture.cli(&["patch", "format", patch.to_str().expect("the patch path")]); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!( + std::fs::read_to_string(&patch) + .expect("the patch") + .ends_with("FIXED\n") + ); + + let call = fixture.read(".checkpatch-call"); + assert!(call.contains("argv: --no-tree --fix-inplace "), "{call}"); + assert!(call.contains("thing.patch"), "{call}"); + assert!(!call.contains("--file"), "{call}"); +} + +#[test] +fn format_applies_the_fix_to_the_working_tree() { + let fixture = Fixture::kernel_tree(); + fixture.write_script("scripts/checkpatch.pl", &stub(FIXER)); + fixture.commit_all("fixture: use the fixing checkpatch"); + fixture.write("drivers/foo/bar.c", common::EDITED); + + let output = fixture.cli(&["patch", "format"]); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let file = fixture.read("drivers/foo/bar.c"); + assert!( + file.contains("return a + 2;"), + "the fix did not land: {file}" + ); + assert!(fixture.git(&["diff"]).contains("+ return a + 2;")); + + let call = fixture.read(".checkpatch-call"); + assert!(call.contains("argv: --no-tree --fix-inplace "), "{call}"); + assert!(!call.contains("--file"), "{call}"); + assert!(call.contains("argv: --no-tree -"), "{call}"); +} + +#[test] +fn a_fix_that_does_not_apply_leaves_the_tree_alone() { + let fixture = Fixture::kernel_tree(); + fixture.write_script("scripts/checkpatch.pl", &stub(RUINER)); + fixture.commit_all("fixture: use the useless checkpatch"); + fixture.write("drivers/foo/bar.c", common::EDITED); + + let output = fixture.cli(&["patch", "format"]); + + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("does not apply"), "{stderr}"); + assert!( + fixture.read("drivers/foo/bar.c").contains("return a + 1;"), + "the uncommitted change was not put back" + ); +} + +#[test] +fn submit_dry_run_shows_the_recipients_and_the_command() { + 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"); + let created = fixture.cli(&["patch", "create", "000-foo"]); + assert!( + created.status.success(), + "{}", + String::from_utf8_lossy(&created.stderr) + ); + let patch = fixture.home().join(".spectral/patches/000-foo.patch"); + + let output = fixture.cli(&[ + "patch", + "submit", + patch.to_str().expect("the patch path"), + "--dry-run", + ]); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("Files: drivers/foo/bar.c"), + "the lookup should say what it ran against: {stdout}" + ); + assert!( + stdout.contains("To: Fixture Maintainer "), + "{stdout}" + ); + assert!(stdout.contains("To: fixture-list@example.com"), "{stdout}"); + assert!( + stdout.contains("Cc: Fixture Reviewer "), + "{stdout}" + ); + assert!(stdout.contains("Cc: another-list@example.com"), "{stdout}"); + assert!( + !stdout.contains("Cc: Fixture Maintainer"), + "an address on the To line needs no courtesy copy: {stdout}" + ); + + // The command line is the thing the README tells a reader to look at. + assert!( + stdout.contains("send-email --dry-run --confirm=never"), + "{stdout}" + ); + assert!( + stdout.contains("--to 'Fixture Maintainer '"), + "{stdout}" + ); + assert!(stdout.contains("Dry-OK"), "send-email's own plan: {stdout}"); + + // And the recipients came from the tree's own script, asked the way the + // fixture tree needs to be asked. + let call = fixture.read(".getmaintainer-call"); + assert!( + call.contains("--no-tree --git --roles --no-rolestats --pattern-depth=0"), + "{call}" + ); + assert!(call.contains("000-foo.patch"), "{call}"); +} + +#[test] +fn submit_puts_extra_ccs_on_the_cc_line_once_and_skips_anyone_on_to() { + 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"); + let created = fixture.cli(&["patch", "create", "000-foo"]); + assert!(created.status.success()); + let patch = fixture.home().join(".spectral/patches/000-foo.patch"); + + let output = fixture.cli(&[ + "patch", + "submit", + patch.to_str().expect("the patch path"), + "--dry-run", + "--cc", + "extra@example.com", + "--cc", + "EXTRA@example.com", + "--cc", + "fixture-list@example.com", + ]); + + assert!(output.status.success()); + let stdout = String::from_utf8_lossy(&output.stdout); + + // Only the block above the command line is ours; send-email echoes the + // same addresses back in its own plan. + let ours: Vec<&str> = stdout + .lines() + .take_while(|line| !line.contains("send-email --dry-run")) + .collect(); + let cc_lines: Vec<&str> = ours + .iter() + .copied() + .filter(|line| line.starts_with("Cc: ")) + .collect(); + assert_eq!(cc_lines.len(), 3, "{stdout}"); + assert_eq!(cc_lines[2], "Cc: extra@example.com"); + assert_eq!( + stdout.matches("--cc extra@example.com").count(), + 1, + "{stdout}" + ); + assert!( + !stdout.contains("Cc: fixture-list@example.com"), + "the list is already on the To line: {stdout}" + ); +} + +#[test] +fn submit_threads_a_reroll_off_the_patch_it_replaces() { + 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"); + let created = fixture.cli(&["patch", "create", "000-foo"]); + assert!(created.status.success()); + let patch = fixture.home().join(".spectral/patches/000-foo.patch"); + + let output = fixture.cli(&[ + "patch", + "submit", + patch.to_str().expect("the patch path"), + "--dry-run", + "--in-reply-to", + "", + ]); + + assert!(output.status.success()); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("--in-reply-to "), + "{stdout}" + ); + assert!(stdout.contains("In-Reply-To: "), "{stdout}"); +} + +#[test] +fn submit_refuses_a_template_cover_letter_instead_of_swallowing_it() { + 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.git(&[ + "format-patch", + "--cover-letter", + "-o", + "out", + "master..HEAD", + ]); + let cover = fixture.join("out/0000-cover-letter.patch"); + + let output = fixture.cli(&[ + "patch", + "submit", + cover.to_str().expect("the cover letter path"), + "--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 submit_names_a_patch_that_is_not_there() { + let fixture = Fixture::kernel_tree(); + + let output = fixture.cli(&["patch", "submit", "nope.patch", "--dry-run"]); + + assert_eq!(output.status.code(), Some(1)); + assert!(String::from_utf8_lossy(&output.stderr).contains("no patch named `nope.patch`")); +} + +#[test] +fn update_renames_a_patch_to_the_next_revision_without_rewriting_it() { + 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() + ); + let patches = fixture.home().join(".spectral/patches"); + let before = std::fs::read(patches.join("000-foo.patch")).expect("the created patch"); + + // Named the way the README's examples name it: no path in front. + let output = fixture.cli(&["patch", "update", "000-foo.patch"]); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let printed = String::from_utf8_lossy(&output.stdout).trim().to_owned(); + assert_eq!( + printed, + patches.join("v1-000-foo.patch").display().to_string() + ); + assert_eq!( + std::fs::read(patches.join("v1-000-foo.patch")).expect("the renamed patch"), + before, + "a re-roll moves the file, it does not rewrite it" + ); + assert!(!patches.join("000-foo.patch").exists()); +} + +#[test] +fn update_infers_the_next_revision_and_does_nothing_at_the_same_one() { + 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() + ); + let patches = fixture.home().join(".spectral/patches"); + + assert!( + fixture + .cli(&["patch", "update", "000-foo.patch"]) + .status + .success() + ); + let to_v2 = fixture.cli(&["patch", "update", "v1-000-foo.patch"]); + assert!(to_v2.status.success()); + assert_eq!( + String::from_utf8_lossy(&to_v2.stdout).trim(), + patches.join("v2-000-foo.patch").display().to_string() + ); + + let before = std::fs::read(patches.join("v2-000-foo.patch")).expect("the v2 patch"); + let again = fixture.cli(&["patch", "update", "v2-000-foo.patch", "-v", "2"]); + + assert!(again.status.success()); + assert_eq!( + String::from_utf8_lossy(&again.stdout).trim(), + patches.join("v2-000-foo.patch").display().to_string() + ); + assert_eq!( + std::fs::read(patches.join("v2-000-foo.patch")).expect("the v2 patch"), + before + ); + assert!(!patches.join("v3-000-foo.patch").exists()); +} + +#[test] +fn update_refuses_to_land_on_a_patch_that_is_already_there() { + 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() + ); + assert!( + fixture + .cli(&["patch", "update", "000-foo.patch", "-v", "1"]) + .status + .success() + ); + assert!( + fixture + .cli(&["patch", "create", "000-foo"]) + .status + .success() + ); + + let output = fixture.cli(&["patch", "update", "000-foo.patch", "-v", "1"]); + + assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("already exists"), "{stderr}"); +} + +#[test] +fn a_patch_name_resolves_without_its_path() { + 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() + ); + + // Straight from `create` into `submit`, by name, as the README shows. + let output = fixture.cli(&["patch", "submit", "000-foo.patch", "--dry-run"]); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("To: Fixture Maintainer "), + "{stdout}" + ); + assert!(stdout.contains("Dry-OK"), "{stdout}"); +} diff --git a/tests/real_tools.rs b/tests/real_tools.rs new file mode 100644 index 0000000..9cc92fd --- /dev/null +++ b/tests/real_tools.rs @@ -0,0 +1,124 @@ +//! The real tools, when the machine has them. +//! +//! Everything else in `tests/` drives stubs, because checkpatch's output moves +//! with the kernel version and a test cannot assert on that. These tests are the +//! other half: they run the host's own scripts so a kernel-version change to a +//! summary line, an argument, or an exit code shows up here rather than in +//! someone's patch workflow. +//! +//! They are ignored by default and need both variables: +//! +//! ```console +//! $ SPECTRAL_REAL_CHECKPATCH=1 SPECTRAL_REAL_TREE=/usr/src/linux \ +//! cargo test --locked -- --ignored real_ +//! ``` + +mod common; + +use common::Fixture; + +/// A change checkpatch has opinions about, whatever kernel it comes from. +const BAD: &str = + "static int foo(int a) {\n\tint b = a + 1;\n\tif (a>0)\n\t\tb += 2;\n\treturn b;\n}\n"; + +#[test] +#[ignore = "needs SPECTRAL_REAL_CHECKPATCH=1 and SPECTRAL_REAL_TREE=/path/to/linux"] +fn real_checkpatch_reads_all_three_kinds_of_target() { + let fixture = real_fixture(); + fixture.write("drivers/foo/bar.c", BAD); + + // The working tree goes in on stdin, because `--git` with no revision + // dies with "no git commits after extraction". + let working_tree = fixture.cli(&["patch", "check"]); + let stdout = String::from_utf8_lossy(&working_tree.stdout).into_owned(); + assert_eq!(working_tree.status.code(), Some(1), "{stdout}"); + assert!(stdout.contains("total:"), "{stdout}"); + assert!(stdout.contains("ERROR"), "{stdout}"); + + // A revision goes through --git, and checkpatch names the commit it read. + let by_rev = fixture.cli(&["patch", "check", "--rev", "HEAD"]); + let stdout = String::from_utf8_lossy(&by_rev.stdout).into_owned(); + assert!(stdout.contains("fixture: add bar"), "{stdout}"); + + // A patch file goes through --file. + let patch = fixture.home().join("bad.patch"); + std::fs::write(&patch, format!("{}\n", fixture.git(&["diff"]))).expect("write the patch"); + let by_file = fixture.cli(&["patch", "check", patch.to_str().expect("the patch path")]); + let stdout = String::from_utf8_lossy(&by_file.stdout).into_owned(); + assert_eq!(by_file.status.code(), Some(1), "{stdout}"); + assert!(stdout.contains("ERROR"), "{stdout}"); +} + +/// A fixture whose checkpatch is the host's, copied in. +fn real_fixture() -> Fixture { + assert_eq!( + std::env::var("SPECTRAL_REAL_CHECKPATCH").as_deref(), + Ok("1"), + "run this with SPECTRAL_REAL_CHECKPATCH=1" + ); + let tree = std::env::var("SPECTRAL_REAL_TREE") + .expect("set SPECTRAL_REAL_TREE to a kernel tree holding scripts/checkpatch.pl"); + + let fixture = Fixture::kernel_tree(); + let script = std::path::Path::new(&tree).join("scripts/checkpatch.pl"); + let body = std::fs::read_to_string(&script) + .unwrap_or_else(|error| panic!("read {}: {error}", script.display())); + fixture.write_script("scripts/checkpatch.pl", &body); + + fixture +} + +#[test] +#[ignore = "needs SPECTRAL_REAL_GETMAINTAINER=1 and SPECTRAL_REAL_TREE=/path/to/linux"] +fn real_get_maintainer_fills_the_to_and_cc_lines() { + assert_eq!( + std::env::var("SPECTRAL_REAL_GETMAINTAINER").as_deref(), + Ok("1"), + "run this with SPECTRAL_REAL_GETMAINTAINER=1" + ); + let tree = std::env::var("SPECTRAL_REAL_TREE") + .expect("set SPECTRAL_REAL_TREE to a kernel tree holding scripts/get_maintainer.pl"); + + let fixture = Fixture::kernel_tree(); + let script = std::path::Path::new(&tree).join("scripts/get_maintainer.pl"); + let body = std::fs::read_to_string(&script) + .unwrap_or_else(|error| panic!("read {}: {error}", script.display())); + fixture.write_script("scripts/get_maintainer.pl", &body); + fixture.commit_all("fixture: use the real get_maintainer"); + + fixture.work_on_a_branch(); + fixture.write("drivers/foo/bar.c", common::EDITED); + fixture.commit_all("foo: return a + 1"); + let created = fixture.cli(&["patch", "create", "000-foo"]); + assert!( + created.status.success(), + "{}", + String::from_utf8_lossy(&created.stderr) + ); + let patch = fixture.home().join(".spectral/patches/000-foo.patch"); + + let output = fixture.cli(&[ + "patch", + "submit", + patch.to_str().expect("the patch path"), + "--dry-run", + ]); + + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8_lossy(&output.stdout); + // The script's own words decide the split, so the assertion is about the + // shape rather than about which kernel's MAINTAINERS matched. + assert!( + stdout.contains("To: Fixture Maintainer "), + "{stdout}" + ); + assert!( + stdout.lines().any(|line| line.starts_with("Cc: ")), + "a reviewer and a second list belong on the Cc line: {stdout}" + ); + assert!(stdout.contains("Dry-OK"), "{stdout}"); +}