From 8a98e2fe9b81880b3e31a798349b30eb6d66503a Mon Sep 17 00:00:00 2001 From: dave Date: Wed, 29 Apr 2026 08:10:02 +0000 Subject: [PATCH] huskies: merge 849 --- crates/source-map-gen/src/lib.rs | 230 ++++++++++++++++++++++++++++++ crates/source-map-gen/src/main.rs | 4 +- 2 files changed, 232 insertions(+), 2 deletions(-) diff --git a/crates/source-map-gen/src/lib.rs b/crates/source-map-gen/src/lib.rs index 618bc110..a767fc5e 100644 --- a/crates/source-map-gen/src/lib.rs +++ b/crates/source-map-gen/src/lib.rs @@ -83,6 +83,103 @@ fn adapter_for_ext(ext: &str) -> Option> { } } +/// Parse added line ranges from a unified diff output. +/// +/// Returns the 1-based, inclusive line ranges in the new version of the file +/// that were introduced by the diff. Lines that are context or deletions are +/// not included. +fn parse_added_ranges(diff: &str) -> Vec> { + let mut ranges = Vec::new(); + for line in diff.lines() { + // Unified diff hunk header: @@ -old[,count] +new[,count] @@ + if !line.starts_with("@@") { + continue; + } + let parts: Vec<&str> = line.split_whitespace().collect(); + // Expected: ["@@", "-old[,count]", "+new[,count]", "@@", ...] + if parts.len() < 3 { + continue; + } + let new_part = parts[2]; + let Some(new_info) = new_part.strip_prefix('+') else { + continue; + }; + let (start, count) = if let Some((s, c)) = new_info.split_once(',') { + ( + s.parse::().unwrap_or(0), + c.parse::().unwrap_or(0), + ) + } else { + (new_info.parse::().unwrap_or(0), 1usize) + }; + if count > 0 && start > 0 { + ranges.push(start..=start + count - 1); + } + } + ranges +} + +/// Returns the 1-based line ranges in `file` that were added since `base` in `worktree`. +/// +/// Uses `git diff --unified=0 {base}...HEAD -- {file}` and parses the hunk headers. +/// Returns an empty `Vec` on git errors or when there are no added lines. +pub fn added_line_ranges( + worktree: &Path, + base: &str, + file: &Path, +) -> Vec> { + let rel = file.strip_prefix(worktree).unwrap_or(file); + let output = Command::new("git") + .args([ + "diff", + "--unified=0", + &format!("{base}...HEAD"), + "--", + &rel.to_string_lossy(), + ]) + .current_dir(worktree) + .output(); + match output { + Ok(o) => parse_added_ranges(&String::from_utf8_lossy(&o.stdout)), + Err(_) => Vec::new(), + } +} + +/// Check documentation coverage, reporting only violations in lines added since `base`. +/// +/// Like [`check_files`], but filters each [`CheckFailure`] to items whose declaration +/// line falls within a range added by `git diff {base}...HEAD` against `worktree`. +/// Pre-existing undocumented items whose lines were not touched by the commit are +/// silently ignored. +pub fn check_files_ratcheted(files: &[&Path], worktree: &Path, base: &str) -> CheckResult { + let mut by_ext: HashMap> = HashMap::new(); + for &file in files { + if let Some(ext) = file.extension().and_then(|e| e.to_str()) { + by_ext.entry(ext.to_string()).or_default().push(file); + } + } + let mut all_failures = Vec::new(); + for (ext, ext_files) in &by_ext { + if let Some(adapter) = adapter_for_ext(ext) + && let CheckResult::Failures(failures) = adapter.check(ext_files) + { + for failure in failures { + let added = added_line_ranges(worktree, base, &failure.file_path); + // Only report if the item's declaration line is within an added range. + // If added is empty (no additions or git error), skip — nothing new to blame. + if !added.is_empty() && added.iter().any(|r| r.contains(&failure.line)) { + all_failures.push(failure); + } + } + } + } + if all_failures.is_empty() { + CheckResult::Ok + } else { + CheckResult::Failures(all_failures) + } +} + /// Check documentation coverage for a mixed list of files. /// /// Dispatches each file to the appropriate [`LanguageAdapter`] based on its @@ -412,6 +509,139 @@ mod tests { ); } + // --- Ratchet tests: AC3 / AC4 --- + + /// AC3: a file with N pre-existing undocumented items plus 1 new undocumented item + /// added by the commit reports exactly 1 violation, not N+1. + #[test] + fn ratchet_only_new_undocumented_items_are_flagged() { + let tmp = TempDir::new().unwrap(); + init_git_repo(tmp.path()); + + // Base commit: file with 2 undocumented public fns (pre-existing). + write_rs( + tmp.path(), + "lib.rs", + "//! Module doc.\n\npub fn old_a() {}\npub fn old_b() {}\n", + ); + Command::new("git") + .args(["add", "lib.rs"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "base"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + + // Second commit: append 1 new undocumented fn. + write_rs( + tmp.path(), + "lib.rs", + "//! Module doc.\n\npub fn old_a() {}\npub fn old_b() {}\npub fn new_c() {}\n", + ); + Command::new("git") + .args(["add", "lib.rs"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "add new_c"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + + let file = tmp.path().join("lib.rs"); + let result = check_files_ratcheted(&[file.as_path()], tmp.path(), "HEAD~1"); + match result { + CheckResult::Failures(failures) => { + assert_eq!( + failures.len(), + 1, + "expected exactly 1 failure (new_c), got {failures:?}" + ); + assert_eq!(failures[0].item_name, "new_c"); + } + CheckResult::Ok => panic!("expected 1 failure for new_c, got Ok"), + } + } + + /// AC4: a commit that doesn't change a file does not blame it for pre-existing + /// undocumented items. + #[test] + fn ratchet_unchanged_file_not_blamed() { + let tmp = TempDir::new().unwrap(); + init_git_repo(tmp.path()); + + // Base commit: undocumented file. + write_rs( + tmp.path(), + "untouched.rs", + "//! Module doc.\n\npub fn old_undocumented() {}\n", + ); + Command::new("git") + .args(["add", "untouched.rs"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "base"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + + // Second commit: add a different, fully documented file; untouched.rs unchanged. + write_rs( + tmp.path(), + "new_file.rs", + "//! Module doc.\n\n/// A function.\npub fn documented() {}\n", + ); + Command::new("git") + .args(["add", "new_file.rs"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + Command::new("git") + .args(["commit", "-m", "add new_file"]) + .current_dir(tmp.path()) + .output() + .unwrap(); + + // Simulate passing untouched.rs to the ratcheted check. + // Since it has no added lines in the diff, it should produce no failures. + let file = tmp.path().join("untouched.rs"); + let result = check_files_ratcheted(&[file.as_path()], tmp.path(), "HEAD~1"); + assert_eq!( + result, + CheckResult::Ok, + "file not touched by the commit should not be blamed" + ); + } + + // --- parse_added_ranges unit tests --- + + #[test] + fn parse_added_ranges_single_hunk() { + let diff = "@@ -0,0 +1,3 @@ some context\n+line1\n+line2\n+line3\n"; + let ranges = parse_added_ranges(diff); + assert_eq!(ranges, vec![1..=3]); + } + + #[test] + fn parse_added_ranges_multiple_hunks() { + let diff = + "@@ -1,2 +1,3 @@\n context\n+new\n context\n@@ -10,0 +11,2 @@\n+added1\n+added2\n"; + let ranges = parse_added_ranges(diff); + assert_eq!(ranges, vec![1..=3, 11..=12]); + } + + #[test] + fn parse_added_ranges_empty_diff() { + let ranges = parse_added_ranges(""); + assert!(ranges.is_empty()); + } + // --- Spawn integration: update_for_worktree writes map at expected path --- fn init_git_repo(dir: &Path) { diff --git a/crates/source-map-gen/src/main.rs b/crates/source-map-gen/src/main.rs index 22a5699b..ce827da0 100644 --- a/crates/source-map-gen/src/main.rs +++ b/crates/source-map-gen/src/main.rs @@ -6,7 +6,7 @@ //! missing doc comments. Exits 0 (silently) when all changed files are fully //! documented or when there are no relevant changes to check. -use source_map_gen::{CheckResult, check_files}; +use source_map_gen::{CheckResult, check_files_ratcheted}; use std::path::{Path, PathBuf}; use std::process::Command; @@ -47,7 +47,7 @@ fn main() { let file_refs: Vec<&Path> = changed.iter().map(PathBuf::as_path).collect(); - match check_files(&file_refs) { + match check_files_ratcheted(&file_refs, worktree_path, &base) { CheckResult::Ok => {} CheckResult::Failures(failures) => { eprintln!(