From 3eeb6338ae33d3bc89618d4f873f38410fdcfa37 Mon Sep 17 00:00:00 2001 From: Derek Date: Thu, 29 Oct 2026 07:41:17 +1100 Subject: [PATCH] fix: stop -w and -o changing bytes they did not fix -w rebuilt the file from its lines, turning CRLF into LF, adding a final newline and turning an empty file into one newline while reporting 0 fixes. Each line now keeps its own terminator. -o now copies the input's permission bits, so a fixed script stays executable. A file that cannot be read is reported on stderr and the other files are still scanned, exiting 1. The run only aborts when nothing could be read. Text output printed the line number twice (2:2:1). It is now 2:1. --- src/cli.rs | 43 +++++++++++++++++++++++++++++++--------- src/fixer.rs | 48 +++++++++++++++++++++++++++++++++++++-------- src/output.rs | 2 +- src/scanner.rs | 31 +++++++++++++++++++++-------- tests/cli_check.rs | 12 ++++++++++++ tests/cli_errors.rs | 28 ++++++++++++++++++++++++++ tests/cli_fix.rs | 34 ++++++++++++++++++++++++++++++++ 7 files changed, 172 insertions(+), 26 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 65b0910..136334e 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -157,13 +157,23 @@ fn run_inner(mut cli: Cli) -> Result { // just prints "loading rules: loading custom rules: ...". let rules = load_rules(cli.config.as_deref())?; let scanner = Scanner::new(&rules).context("compiling rule patterns")?; - let mut matches = scanner - .scan_files(cli.files.iter().map(PathBuf::as_path)) - .context("scanning files")?; - matches = filter_by_severity(matches, min_severity, &rules); + let mut report = scanner.scan_files(cli.files.iter().map(PathBuf::as_path)); + // One bad file must not hide the others' results. The run only aborts when + // nothing at all could be read, and any unreadable file still exits 1. + let read_failed = !report.failures.is_empty(); + if report.readable.is_empty() && read_failed { + let first = report.failures.swap_remove(0); + return Err(anyhow::Error::new(first).context("scanning files")); + } + for failure in &report.failures { + eprintln!("Error: {}", error_chain(failure)); + } + cli.files = report.readable; + let matches = filter_by_severity(report.matches, min_severity, &rules); if cli.write || cli.output.is_some() { - return run_fix(&cli, &matches, &rules); + let code = run_fix(&cli, &matches, &rules)?; + return Ok(if read_failed { ExitCode::from(1) } else { code }); } let stdout = std::io::stdout(); @@ -174,7 +184,7 @@ fn run_inner(mut cli: Cli) -> Result { "json" => formatter.json(&matches)?, _ => formatter.text(&matches)?, } - if has_errors(&matches, &rules) { + if read_failed || has_errors(&matches, &rules) { return Ok(ExitCode::from(1)); } Ok(ExitCode::SUCCESS) @@ -238,7 +248,7 @@ fn run_fix(cli: &Cli, matches: &[MatchHit], rules: &RuleSet) -> Result } continue; } - write_output(&out_path, &outcome.content) + write_output(&out_path, &outcome.content, input) .with_context(|| format!("writing {}", out_path.display()))?; match (outcome.fixed_count, outcome.unfixed_count) { (f, u) if f > 0 && u > 0 => println!( @@ -309,13 +319,28 @@ fn same_file(input: &Path, out: &Path) -> bool { } } -fn write_output(path: &Path, content: &str) -> std::io::Result<()> { +/// Write `content` to `path`, giving it the permission bits of `mode_from` +/// (the input script) so `-o` keeps a script executable the way `-w` does. +fn write_output(path: &Path, content: &str, mode_from: &Path) -> std::io::Result<()> { if let Some(parent) = path.parent() && !parent.as_os_str().is_empty() { std::fs::create_dir_all(parent)?; } - std::fs::write(path, content) + std::fs::write(path, content)?; + std::fs::set_permissions(path, std::fs::metadata(mode_from)?.permissions()) +} + +/// `err: cause: cause`, the same shape anyhow prints with `{:#}`. +fn error_chain(err: &dyn std::error::Error) -> String { + let mut out = err.to_string(); + let mut cur = err.source(); + while let Some(c) = cur { + out.push_str(": "); + out.push_str(&c.to_string()); + cur = c.source(); + } + out } fn load_rules(custom: Option<&Path>) -> Result { diff --git a/src/fixer.rs b/src/fixer.rs index 346dec8..04b0bda 100644 --- a/src/fixer.rs +++ b/src/fixer.rs @@ -82,23 +82,27 @@ impl Fixer { v.sort_by_key(|m| std::cmp::Reverse(m.column)); } - let mut out_lines: Vec = Vec::new(); let mut outcome = FixOutcome::default(); - let lines: Vec<&str> = text.lines().collect(); - for (idx, line) in lines.iter().enumerate() { + let mut content = String::with_capacity(text.len()); + // Each line keeps its own terminator (LF, CRLF or none on the last + // line), so bytes a fix did not touch are written back unchanged. + for (idx, raw) in text.split_inclusive('\n').enumerate() { let lineno = idx + 1; + let body = raw.strip_suffix('\n').unwrap_or(raw); + let body = body.strip_suffix('\r').unwrap_or(body); + let terminator = &raw[body.len()..]; if let Some(line_matches) = by_line.get(&lineno) { - let (fixed, fixes, unfixed) = self.fix_line(line, line_matches); + let (fixed, fixes, unfixed) = self.fix_line(body, line_matches); outcome.fixed_count += fixes.len(); outcome.unfixed_count += unfixed; outcome.fixes.extend(fixes); - out_lines.push(fixed); + content.push_str(&fixed); } else { - out_lines.push((*line).to_string()); + content.push_str(body); } + content.push_str(terminator); } - outcome.content = out_lines.join("\n"); - outcome.content.push('\n'); + outcome.content = content; outcome } @@ -483,4 +487,32 @@ mod tests { let err = validate_bash_syntax("if then\n").unwrap_err(); assert!(!err.is_empty(), "expected non-empty error from bash -n"); } + + /// Only the fixed line may change. Line endings, a missing final newline + /// and an empty file all come back byte-for-byte. + #[test] + fn fix_text_preserves_bytes_it_did_not_fix() { + let cases: [(&str, &str, usize); 7] = [ + ("", "", 0), + ("\n", "\n", 0), + ("echo hi", "echo hi", 0), + ("echo hi\r\necho there\r\n", "echo hi\r\necho there\r\n", 0), + ( + "echo a\r\ngrep -P '\\d+' f\r\necho b\r\n", + "echo a\r\ngrep -E '[0-9]+' f\r\necho b\r\n", + 1, + ), + ("grep -P '\\d+' f", "grep -E '[0-9]+' f", 1), + ( + "echo a\ngrep -P '\\d+' f\r\necho b", + "echo a\ngrep -E '[0-9]+' f\r\necho b", + 1, + ), + ]; + for (input, want, fixed) in cases { + let out = run_fix(input); + assert_eq!(out.content, want, "input {input:?}"); + assert_eq!(out.fixed_count, fixed, "input {input:?}"); + } + } } diff --git a/src/output.rs b/src/output.rs index 084d57e..b081fd5 100644 --- a/src/output.rs +++ b/src/output.rs @@ -63,7 +63,7 @@ impl<'a, W: Write> Formatter<'a, W> { }; write!(self.w, " ")?; self.write_styled(gray(), &format!("{}", m.line))?; - write!(self.w, ":{}:{}: ", m.line, m.column)?; + write!(self.w, ":{}: ", m.column)?; self.write_styled(severity_style(severity), severity_str)?; writeln!(self.w, " [{}]", m.rule_id)?; if let Some(r) = rule { diff --git a/src/scanner.rs b/src/scanner.rs index f2f2c30..d050255 100644 --- a/src/scanner.rs +++ b/src/scanner.rs @@ -7,7 +7,7 @@ // Copyright: (c) 2025-2026 HYPERI PTY LIMITED use std::collections::BTreeMap; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::sync::OnceLock; use regex::Regex; @@ -183,18 +183,33 @@ impl Scanner { out } - pub fn scan_files<'a>( - &self, - paths: impl IntoIterator, - ) -> Result, ScanError> { - let mut all = Vec::new(); + /// Scan every path, keeping the findings from the files that could be + /// read. A file that cannot be read (missing, a directory, not UTF-8) is + /// recorded in `failures` instead of discarding the other files' results. + pub fn scan_files<'a>(&self, paths: impl IntoIterator) -> ScanReport { + let mut report = ScanReport::default(); for p in paths { - all.extend(self.scan_file(p)?); + match self.scan_file(p) { + Ok(hits) => { + report.matches.extend(hits); + report.readable.push(p.to_path_buf()); + } + Err(e) => report.failures.push(e), + } } - Ok(all) + report } } +/// Result of scanning several files: findings, the files that were read, and +/// the files that could not be. +#[derive(Debug, Default)] +pub struct ScanReport { + pub matches: Vec, + pub readable: Vec, + pub failures: Vec, +} + /// Filter matches to those at or above `min`. The Go implementation uses /// match.Rule.Severity directly; here we look the rule up by id from the /// RuleSet to avoid stuffing a borrowed reference into MatchHit. diff --git a/tests/cli_check.rs b/tests/cli_check.rs index d6a4bf0..0c554a5 100644 --- a/tests/cli_check.rs +++ b/tests/cli_check.rs @@ -240,3 +240,15 @@ fn version_short_flag_matches_the_long_flag() { stdout_of(macbash().arg("--version")) ); } + +#[test] +fn text_output_prefix_is_line_then_column_once() { + let dir = tempfile::tempdir().unwrap(); + write_file(&dir, "probe.sh", "#!/usr/bin/env bash\necho -e \"hi\"\n"); + macbash() + .current_dir(dir.path()) + .arg("probe.sh") + .assert() + .code(0) + .stdout(predicate::str::is_match(r"(?m)^ 2:\d+: (ERROR|WARNING|INFO) ").unwrap()); +} diff --git a/tests/cli_errors.rs b/tests/cli_errors.rs index d869952..eba4928 100644 --- a/tests/cli_errors.rs +++ b/tests/cli_errors.rs @@ -176,3 +176,31 @@ fn help_lists_the_output_modes_and_exits_zero() { .stdout(predicate::str::contains("-o, --output")) .stdout(predicate::str::contains("Exit Codes:")); } + +#[test] +fn unreadable_file_does_not_hide_the_other_files_results() { + let dir = tempfile::tempdir().unwrap(); + let bad = dir.path().join("latin1.sh"); + std::fs::write(&bad, b"echo caf\xe9\n").unwrap(); + macbash() + .arg(&bad) + .arg(fixture("sample-linux.sh")) + .assert() + .code(1) + .stderr(predicate::str::contains("Error: reading")) + .stderr(predicate::str::contains("latin1.sh")) + .stdout(predicate::str::contains("sed-inplace-no-backup")); +} + +#[test] +fn only_unreadable_files_abort_the_run() { + let dir = tempfile::tempdir().unwrap(); + let bad = dir.path().join("latin1.sh"); + std::fs::write(&bad, b"echo caf\xe9\n").unwrap(); + macbash() + .arg(&bad) + .assert() + .code(1) + .stderr(predicate::str::contains("Error: scanning files: reading")) + .stdout(predicate::str::is_empty()); +} diff --git a/tests/cli_fix.rs b/tests/cli_fix.rs index 13dace3..0a7da94 100644 --- a/tests/cli_fix.rs +++ b/tests/cli_fix.rs @@ -216,3 +216,37 @@ fn severity_filter_narrows_what_fix_mode_touches() { "--severity info should fix at least as much as --severity error" ); } + +#[cfg(unix)] +#[test] +fn output_file_keeps_the_input_permissions() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let script = copy_fixture(&dir, "sample-linux.sh"); + std::fs::set_permissions(&script, std::fs::Permissions::from_mode(0o755)).unwrap(); + let out = dir.path().join("fixed").join("out.sh"); + + macbash().arg("-o").arg(&out).arg(&script).assert().code(1); + + let mode = std::fs::metadata(&out).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o755, "-o must keep the executable bit"); +} + +#[test] +fn write_mode_leaves_untouched_bytes_alone() { + let dir = tempfile::tempdir().unwrap(); + for (name, body) in [ + ("crlf.sh", "#!/usr/bin/env bash\r\necho hi\r\n"), + ("no-newline.sh", "#!/usr/bin/env bash\necho hi"), + ("empty.sh", ""), + ] { + let script = write_file(&dir, name, body); + macbash() + .arg("-w") + .arg(&script) + .assert() + .code(0) + .stdout(predicate::str::contains("(0 fixes applied, 0 unfixable)")); + assert_eq!(std::fs::read_to_string(&script).unwrap(), body, "{name}"); + } +}