huskies: merge 849
This commit is contained in:
dave
@@ -83,6 +83,103 @@ fn adapter_for_ext(ext: &str) -> Option<Box<dyn LanguageAdapter>> {
|
||||
}
|
||||
}
|
||||
|
||||
/// 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<std::ops::RangeInclusive<usize>> {
|
||||
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::<usize>().unwrap_or(0),
|
||||
c.parse::<usize>().unwrap_or(0),
|
||||
)
|
||||
} else {
|
||||
(new_info.parse::<usize>().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<std::ops::RangeInclusive<usize>> {
|
||||
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<String, Vec<&Path>> = 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) {
|
||||
|
||||
@@ -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!(
|
||||
|
||||
Reference in New Issue
Block a user