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}"); + } +}