huntedbytheirs/spectral
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

patch: fill in the loop verbs, and test them

Browse Source 
Every patch verb outside the series work is real. check reads the working
tree through checkpatch's stdin, which is the only way it can read one,
since --git with no revision dies; format puts --fix-inplace's output back
into the tree after setting the uncommitted change aside, and restores it
if the fix does not apply; commit and create sit on git; create writes a
mail-formatted patch, because git send-email refuses a bare diff with "No
subject line"; submit splits get_maintainer.pl's roles into To and Cc,
prints the command line it is about to run, and only sends when --dry-run
is off; update re-rolls a patch by renaming it and does nothing at the
revision it is already at.

git.rs grows the plumbing those verbs stand on: diff_unstaged,
format_patch, apply, commit, rev_parse. apply feeds the patch in on stdin,
so a fixed patch stays in memory until it is known to apply.

tests/ holds a fixture kernel tree, so the suite needs no kernel tree, no
network, and no configured git send-email. The two tests that do want the
host's own checkpatch and get_maintainer are ignored unless asked for, and
one of them earned its place immediately: checkpatch's --file means "this
argument is source code", so checking a patch with it reported a dirty
patch as clean, 8004 lines and no errors. Patchfile mode is checkpatch's
default and the path now stands alone.

patch check also takes --rev, which is how a committed range gets checked.

Checked with cargo fmt --check, cargo clippy --all-targets -- -D warnings,
cargo test, and cargo doc --no-deps.
This commit is contained in:
huntedbytheirs
2026-09-16 18:41:51 -04:00
parent 33161f2ae4
commit 97664806b7
11 changed files with 2173 additions and 102 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/cli.rs
+3
View File
@@ -84,6 +84,9 @@ pub struct CheckArgs {
/// Check this patch file instead of the working tree
#[arg(value_name = "PATCH")]
pub patch: Option<PathBuf>,
/// Check this revision instead of the working tree
#[arg(long, value_name = "REV", conflicts_with = "patch")]
pub rev: Option<String>,
/// Pass --strict to checkpatch.pl
#[arg(long)]
pub strict: bool,
src/error.rs
+38 -3
View File
@@ -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<T, E = Error> = std::result::Result<T, E>;
// 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),
}
src/git.rs
+139 -39
View File
@@ -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<String> {
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<Vec<u8>> {
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<String> {
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<Vec<u8>> {
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<Vec<u8>> {
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<String> {
self.run(&["rev-parse", "--verify", "--quiet", rev])
}
fn output(&self, args: &[&str]) -> Result<std::process::Output> {
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<String> {
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<String> {
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<String> {
todo!("git rev-parse {rev}")
Ok(output)
}
}
src/main.rs
+42 -12
View File
@@ -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<ExitCode> {
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
}
}
src/patch/checkpatch.rs
+105 -12
View File
@@ -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<Report> {
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)
}
src/patch/maintainers.rs
+334 -10
View File
@@ -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 <address>` 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<String>,
/// Reviewers, lists, and everyone else worth a courtesy copy.
/// Reviewers, other lists, and everyone git remembers signing off.
pub cc: Vec<String>,
/// Files the patch touches, which is what the lookup runs against.
pub touched: Vec<PathBuf>,
@@ -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<Recipients> {
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<Entry> = 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<String>,
address: String,
/// Lowercased role words, e.g. `maintainer`, `open list`, `authored`.
roles: Vec<String>,
}
impl Entry {
/// How the entry reads in a header: `Name <address>`, 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<PathBuf>) -> 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 <address> (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<Entry> {
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<Vec<PathBuf>> {
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<String>, 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 <address>` 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<String> {
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 <[email protected]> (maintainer:USB XHCI HOST CONTROLLER DRIVER)",
"[email protected] (open list:USB XHCI HOST CONTROLLER DRIVER)",
"Greg Kroah-Hartman <[email protected]> (maintainer:USB SUBSYSTEM)",
"\"GitAuthor: Fixture\" <[email protected]> (authored,added_lines)",
"[email protected] (reviewer:SOMETHING ELSE)",
"[email protected] (moderated list:SOMETHING ELSE)",
];
fn split_lines(lines: &[&str]) -> Recipients {
let entries: Vec<Entry> = 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 <[email protected]>".to_owned(),
"[email protected]".to_owned(),
"Greg Kroah-Hartman <[email protected]>".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\" <[email protected]>".to_owned(),
"[email protected]".to_owned(),
"[email protected]".to_owned(),
]
);
}
#[test]
fn lookup_keeps_an_address_off_the_cc_line_once_it_is_on_to() {
let recipients = split_lines(&[
"Greg Kroah-Hartman <[email protected]> (maintainer:USB SUBSYSTEM)",
"Greg Kroah-Hartman <[email protected]> (supporter:USB SUBSYSTEM)",
"[email protected] (contributor:USB SUBSYSTEM)",
]);
assert_eq!(
recipients.to,
vec!["Greg Kroah-Hartman <[email protected]>".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]), "[email protected]");
assert_eq!(address_of(&recipients.to[1]), "[email protected]");
}
#[test]
fn lookup_reads_the_roles_out_of_the_last_parenthetical() {
let entry = parse_entry("Uwe (West) Kleine-Konig <[email protected]> (maintainer:PWM)")
.expect("one entry");
assert_eq!(entry.address, "[email protected]");
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,
&[
"[email protected]".to_owned(),
"[email protected]".to_owned(),
"[email protected]".to_owned(),
],
);
assert_eq!(recipients.cc.len(), 4, "{:?}", recipients.cc);
assert_eq!(recipients.cc[3], "[email protected]");
}
#[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);
}
}
src/patch/mod.rs
+349 -26
View File
@@ -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<checkpatch::Report> {
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<checkpatch::Report> {
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<PathBuf> {
let _ = (config, args);
todo!("git diff <base> > <patch_dir>/<name>.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<Submission> {
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<String> = 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::<Vec<_>>()
.join(" ")
}
/// Re-roll a patch as `v<N>`.
@@ -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<PathBuf> {
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<N>-`.
#[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<PathBuf> {
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 <hash> 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<checkpatch::Report> {
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");
}
}