返回 CodeWhale
review_hunks.rs
根目录 / crates / tui / src / tools / review_hunks.rs
1 //! Unified-diff hunk parsing for GitHub review anchors.
2 //!
3 //! GitHub's "create a review" API rejects the *entire* review with a 422 when
4 //! any single inline comment anchors to a line that is not part of the diff.
5 //! Before this module existed the reviewer only checked that the *file* was
6 //! touched, so one model-estimated line number could make the whole review
7 //! request fail after every other inline comment had already been prepared.
8 //!
9 //! [`DiffHunks::parse`] turns a unified diff into the exact set of RIGHT-side
10 //! (post-image) line numbers GitHub will accept per file, so a bad anchor
11 //! drops one comment instead of the whole review.
12
13 use std::collections::BTreeMap;
14
15 /// A contiguous run of RIGHT-side line numbers, inclusive on both ends.
16 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
17 struct LineRange {
18 start: u32,
19 end: u32,
20 }
21
22 /// The RIGHT-side lines a unified diff exposes, keyed by post-image path.
23 #[derive(Debug, Clone, Default, PartialEq, Eq)]
24 pub struct DiffHunks {
25 files: BTreeMap<String, FileHunks>,
26 }
27
28 #[derive(Debug, Clone, Default, PartialEq, Eq)]
29 struct FileHunks {
30 /// Every RIGHT-side line inside a hunk: context lines plus added lines.
31 /// These are exactly the lines GitHub accepts with `side: "RIGHT"`.
32 commentable: Vec<LineRange>,
33 }
34
35 /// Cursor state while walking a hunk body.
36 #[derive(Debug, Clone, Copy, Default)]
37 struct HunkCursor {
38 right_line: u32,
39 remaining_old: u32,
40 remaining_new: u32,
41 }
42
43 impl HunkCursor {
44 fn exhausted(self) -> bool {
45 self.remaining_old == 0 && self.remaining_new == 0
46 }
47 }
48
49 impl DiffHunks {
50 /// Parse a unified diff (`git diff`, `gh pr diff`) into per-file
51 /// RIGHT-side line coverage.
52 ///
53 /// Handles multiple hunks per file, added-only and deleted files,
54 /// renames (the post-image path wins), `\ No newline at end of file`
55 /// markers, CRLF line endings, and hunk headers with omitted counts
56 /// (`@@ -1 +1 @@`). Hunk bodies are bounded by the counts in the header
57 /// so a file whose *content* contains `--- `, `+++ `, or `diff --git`
58 /// lines cannot be mistaken for a new file header.
59 #[must_use]
60 pub fn parse(diff: &str) -> Self {
61 let mut files: BTreeMap<String, FileHunks> = BTreeMap::new();
62 let mut current: Option<String> = None;
63 let mut cursor = HunkCursor::default();
64 let mut in_hunk = false;
65
66 for raw in diff.split('\n') {
67 let line = raw.strip_suffix('\r').unwrap_or(raw);
68
69 if in_hunk && !cursor.exhausted() {
70 match consume_hunk_line(line, &mut cursor, current.as_deref(), &mut files) {
71 BodyStep::Consumed => continue,
72 BodyStep::NotABodyLine => in_hunk = false,
73 }
74 }
75
76 if line.starts_with("diff --git ") {
77 current = None;
78 in_hunk = false;
79 } else if let Some(rest) = line.strip_prefix("+++ ") {
80 in_hunk = false;
81 current = post_image_path(rest);
82 if let Some(path) = current.as_ref() {
83 files.entry(path.clone()).or_default();
84 }
85 } else if line.starts_with("--- ") {
86 // Pre-image header; the `+++` that follows decides the path we
87 // anchor against (which is what makes renames work).
88 in_hunk = false;
89 } else if let Some((right_start, old_count, new_count)) = parse_hunk_header(line) {
90 cursor = HunkCursor {
91 right_line: right_start,
92 remaining_old: old_count,
93 remaining_new: new_count,
94 };
95 in_hunk = true;
96 }
97 }
98
99 Self { files }
100 }
101
102 /// True when the diff touches `path` at all (post-image path match).
103 #[must_use]
104 pub fn touches_path(&self, path: &str) -> bool {
105 self.files.contains_key(path)
106 }
107
108 /// Post-image paths and hunk ranges from the same parser used to validate
109 /// comments. Context collection must not invent a second diff parser.
110 pub(crate) fn paths(&self) -> impl Iterator<Item = &str> {
111 self.files.keys().map(String::as_str)
112 }
113
114 pub(crate) fn ranges(&self, path: &str) -> impl Iterator<Item = (u32, u32)> + '_ {
115 self.files.get(path).into_iter().flat_map(|file| {
116 file.commentable
117 .iter()
118 .map(|range| (range.start, range.end))
119 })
120 }
121
122 /// True when `line` is a RIGHT-side line GitHub will accept an inline
123 /// comment on for `path` — a context line or an added line inside a hunk.
124 #[must_use]
125 pub fn contains_line(&self, path: &str, line: u32) -> bool {
126 self.files
127 .get(path)
128 .is_some_and(|file| covers(&file.commentable, line))
129 }
130
131 /// True when every line in `start..=end` is a valid RIGHT-side anchor.
132 /// Gates multi-line committable suggestions: GitHub rejects a
133 /// `start_line`/`line` span that leaves the diff.
134 #[must_use]
135 pub fn contains_span(&self, path: &str, start: u32, end: u32) -> bool {
136 if start > end {
137 return false;
138 }
139 let Some(file) = self.files.get(path) else {
140 return false;
141 };
142 (start..=end).all(|line| covers(&file.commentable, line))
143 }
144 }
145
146 enum BodyStep {
147 Consumed,
148 NotABodyLine,
149 }
150
151 fn consume_hunk_line(
152 line: &str,
153 cursor: &mut HunkCursor,
154 current: Option<&str>,
155 files: &mut BTreeMap<String, FileHunks>,
156 ) -> BodyStep {
157 // `\ No newline at end of file` annotates the previous line and consumes
158 // no line number on either side.
159 if line.starts_with('\\') {
160 return BodyStep::Consumed;
161 }
162 match line.as_bytes().first() {
163 Some(b'+') => {
164 if let Some(path) = current {
165 let entry = files.entry(path.to_string()).or_default();
166 push_line(&mut entry.commentable, cursor.right_line);
167 }
168 cursor.right_line = cursor.right_line.saturating_add(1);
169 cursor.remaining_new = cursor.remaining_new.saturating_sub(1);
170 BodyStep::Consumed
171 }
172 Some(b'-') => {
173 // Deleted: LEFT side only, never a valid RIGHT anchor.
174 cursor.remaining_old = cursor.remaining_old.saturating_sub(1);
175 BodyStep::Consumed
176 }
177 // A context line is " text"; some transports strip the trailing space
178 // from an empty context line, leaving a bare empty line.
179 Some(b' ') | None => {
180 if let Some(path) = current {
181 let entry = files.entry(path.to_string()).or_default();
182 push_line(&mut entry.commentable, cursor.right_line);
183 }
184 cursor.right_line = cursor.right_line.saturating_add(1);
185 cursor.remaining_old = cursor.remaining_old.saturating_sub(1);
186 cursor.remaining_new = cursor.remaining_new.saturating_sub(1);
187 BodyStep::Consumed
188 }
189 Some(_) => BodyStep::NotABodyLine,
190 }
191 }
192
193 fn covers(ranges: &[LineRange], line: u32) -> bool {
194 ranges
195 .iter()
196 .any(|range| line >= range.start && line <= range.end)
197 }
198
199 fn push_line(ranges: &mut Vec<LineRange>, line: u32) {
200 if let Some(last) = ranges.last_mut()
201 && last.end.saturating_add(1) == line
202 {
203 last.end = line;
204 return;
205 }
206 ranges.push(LineRange {
207 start: line,
208 end: line,
209 });
210 }
211
212 /// Extract the post-image path from a `+++ ` header body. Returns `None` for
213 /// `/dev/null` (a deleted file has no RIGHT side to comment on).
214 fn post_image_path(rest: &str) -> Option<String> {
215 // Some diff formats append a tab plus timestamp.
216 let rest = rest.split('\t').next().unwrap_or(rest).trim_end();
217 if rest.is_empty() || rest == "/dev/null" {
218 return None;
219 }
220 // Quoted paths (`"b/we\tird.rs"`) carry C-style escapes; anchoring on a
221 // mangled path would 422 the review, so decline rather than guess.
222 if rest.starts_with('"') {
223 return None;
224 }
225 let path = rest.strip_prefix("b/").unwrap_or(rest);
226 if path.is_empty() || path == "/dev/null" {
227 return None;
228 }
229 Some(path.to_string())
230 }
231
232 /// Parse `@@ -a,b +c,d @@ optional context` into
233 /// `(right_start, old_count, new_count)`. Counts may be omitted
234 /// (`@@ -1 +1 @@` means one line on each side).
235 pub(super) fn parse_hunk_header(line: &str) -> Option<(u32, u32, u32)> {
236 let rest = line.strip_prefix("@@ ")?;
237 let end = rest.find(" @@")?;
238 let ranges = &rest[..end];
239 let mut old_count = None;
240 let mut new_count = None;
241 let mut right_start = None;
242 for part in ranges.split(' ') {
243 if let Some(spec) = part.strip_prefix('-') {
244 old_count = Some(parse_range_count(spec)?);
245 } else if let Some(spec) = part.strip_prefix('+') {
246 let (start, count) = parse_range(spec)?;
247 right_start = Some(start);
248 new_count = Some(count);
249 }
250 }
251 Some((right_start?, old_count?, new_count?))
252 }
253
254 fn parse_range(spec: &str) -> Option<(u32, u32)> {
255 let mut parts = spec.split(',');
256 let start = parts.next()?.parse::<u32>().ok()?;
257 let count = match parts.next() {
258 Some(count) => count.parse::<u32>().ok()?,
259 None => 1,
260 };
261 Some((start, count))
262 }
263
264 fn parse_range_count(spec: &str) -> Option<u32> {
265 parse_range(spec).map(|(_, count)| count)
266 }
267
268 #[cfg(test)]
269 mod tests {
270 use super::*;
271
272 /// Join diff lines verbatim. Written as a slice rather than one string
273 /// literal because a `\` continuation would strip the leading space that
274 /// marks a context line — which is exactly the byte this parser reads.
275 fn diff(lines: &[&str]) -> String {
276 let mut joined = lines.join("\n");
277 joined.push('\n');
278 joined
279 }
280
281 #[test]
282 fn parses_multiple_hunks_in_one_file() {
283 let diff = diff(&[
284 "diff --git a/src/lib.rs b/src/lib.rs",
285 "index 1111111..2222222 100644",
286 "--- a/src/lib.rs",
287 "+++ b/src/lib.rs",
288 "@@ -10,3 +10,4 @@ fn one() {",
289 " context_a",
290 "-removed",
291 "+added_one",
292 "+added_two",
293 " context_b",
294 "@@ -80,2 +81,2 @@ fn two() {",
295 " ctx",
296 "-old",
297 "+new",
298 ]);
299 let hunks = DiffHunks::parse(&diff);
300 assert!(hunks.touches_path("src/lib.rs"));
301 // First hunk covers right lines 10..=13 (ctx, +, +, ctx).
302 for line in 10..=13 {
303 assert!(hunks.contains_line("src/lib.rs", line), "line {line}");
304 }
305 assert!(!hunks.contains_line("src/lib.rs", 9));
306 assert!(!hunks.contains_line("src/lib.rs", 14));
307 // Second hunk: ctx at 81, `-old` consumes no right line, `+new` at 82.
308 assert!(hunks.contains_line("src/lib.rs", 81));
309 assert!(hunks.contains_line("src/lib.rs", 82));
310 assert!(!hunks.contains_line("src/lib.rs", 83));
311 }
312
313 #[test]
314 fn parses_added_only_file() {
315 let diff = diff(&[
316 "diff --git a/new.rs b/new.rs",
317 "new file mode 100644",
318 "index 0000000..3333333",
319 "--- /dev/null",
320 "+++ b/new.rs",
321 "@@ -0,0 +1,3 @@",
322 "+fn a() {}",
323 "+fn b() {}",
324 "+fn c() {}",
325 ]);
326 let hunks = DiffHunks::parse(&diff);
327 assert!(hunks.contains_span("new.rs", 1, 3));
328 assert!(!hunks.contains_line("new.rs", 4));
329 }
330
331 #[test]
332 fn deleted_file_has_no_right_side_anchor() {
333 let diff = diff(&[
334 "diff --git a/gone.rs b/gone.rs",
335 "deleted file mode 100644",
336 "index 3333333..0000000",
337 "--- a/gone.rs",
338 "+++ /dev/null",
339 "@@ -1,2 +0,0 @@",
340 "-fn a() {}",
341 "-fn b() {}",
342 ]);
343 let hunks = DiffHunks::parse(&diff);
344 assert!(!hunks.touches_path("gone.rs"));
345 assert!(!hunks.contains_line("gone.rs", 1));
346 }
347
348 #[test]
349 fn pure_deletion_hunk_exposes_no_right_lines() {
350 let diff = diff(&[
351 "diff --git a/src/a.rs b/src/a.rs",
352 "--- a/src/a.rs",
353 "+++ b/src/a.rs",
354 "@@ -5,3 +4,0 @@",
355 "-one",
356 "-two",
357 "-three",
358 ]);
359 let hunks = DiffHunks::parse(&diff);
360 assert!(hunks.touches_path("src/a.rs"));
361 assert!(!hunks.contains_line("src/a.rs", 4));
362 assert!(!hunks.contains_line("src/a.rs", 5));
363 }
364
365 #[test]
366 fn rename_anchors_on_the_post_image_path() {
367 let diff = diff(&[
368 "diff --git a/old/name.rs b/new/name.rs",
369 "similarity index 92%",
370 "rename from old/name.rs",
371 "rename to new/name.rs",
372 "--- a/old/name.rs",
373 "+++ b/new/name.rs",
374 "@@ -1,2 +1,2 @@",
375 " kept",
376 "-old",
377 "+new",
378 ]);
379 let hunks = DiffHunks::parse(&diff);
380 assert!(hunks.touches_path("new/name.rs"));
381 assert!(!hunks.touches_path("old/name.rs"));
382 assert!(hunks.contains_span("new/name.rs", 1, 2));
383 assert!(!hunks.contains_line("new/name.rs", 3));
384 }
385
386 #[test]
387 fn rename_without_content_change_has_no_anchors() {
388 let diff = diff(&[
389 "diff --git a/old.rs b/new.rs",
390 "similarity index 100%",
391 "rename from old.rs",
392 "rename to new.rs",
393 ]);
394 let hunks = DiffHunks::parse(&diff);
395 // No `+++` header and no hunk: nothing is commentable, and we must not
396 // invent an anchor.
397 assert!(!hunks.touches_path("new.rs"));
398 assert!(!hunks.contains_line("new.rs", 1));
399 }
400
401 #[test]
402 fn no_newline_marker_consumes_no_line_number() {
403 let diff = diff(&[
404 "diff --git a/eof.rs b/eof.rs",
405 "--- a/eof.rs",
406 "+++ b/eof.rs",
407 "@@ -1,2 +1,2 @@",
408 " first",
409 "-last",
410 "\\ No newline at end of file",
411 "+last!",
412 "\\ No newline at end of file",
413 ]);
414 let hunks = DiffHunks::parse(&diff);
415 assert!(hunks.contains_span("eof.rs", 1, 2));
416 assert!(!hunks.contains_line("eof.rs", 3));
417 }
418
419 #[test]
420 fn crlf_diff_parses_paths_and_lines() {
421 let diff = [
422 "diff --git a/crlf.rs b/crlf.rs",
423 "index 1111111..2222222 100644",
424 "--- a/crlf.rs",
425 "+++ b/crlf.rs",
426 "@@ -1,2 +1,3 @@",
427 " ctx",
428 "+added",
429 " tail",
430 ]
431 .join("\r\n")
432 + "\r\n";
433 let hunks = DiffHunks::parse(&diff);
434 assert!(
435 hunks.touches_path("crlf.rs"),
436 "CRLF path must not keep the carriage return"
437 );
438 assert!(hunks.contains_span("crlf.rs", 1, 3));
439 assert!(!hunks.contains_line("crlf.rs", 4));
440 }
441
442 #[test]
443 fn hunk_header_without_counts_is_supported() {
444 let diff = diff(&[
445 "diff --git a/one.rs b/one.rs",
446 "--- a/one.rs",
447 "+++ b/one.rs",
448 "@@ -7 +9 @@",
449 "-old",
450 "+new",
451 ]);
452 let hunks = DiffHunks::parse(&diff);
453 assert!(hunks.contains_line("one.rs", 9));
454 assert!(!hunks.contains_line("one.rs", 8));
455 }
456
457 #[test]
458 fn multi_file_diff_keeps_files_independent() {
459 let diff = diff(&[
460 "diff --git a/a.rs b/a.rs",
461 "--- a/a.rs",
462 "+++ b/a.rs",
463 "@@ -1,0 +1,1 @@",
464 "+alpha",
465 "diff --git a/b.rs b/b.rs",
466 "--- a/b.rs",
467 "+++ b/b.rs",
468 "@@ -50,0 +50,1 @@",
469 "+beta",
470 ]);
471 let hunks = DiffHunks::parse(&diff);
472 assert!(hunks.contains_line("a.rs", 1));
473 assert!(!hunks.contains_line("a.rs", 50));
474 assert!(hunks.contains_line("b.rs", 50));
475 assert!(!hunks.contains_line("b.rs", 1));
476 }
477
478 #[test]
479 fn file_content_that_looks_like_a_diff_header_stays_in_the_hunk() {
480 // A patch that itself edits a stored diff fixture. Naive parsers read
481 // the ` --- a/inner.rs` / ` +++ b/inner.rs` context lines as a new file
482 // header and mis-anchor every later comment.
483 let diff = diff(&[
484 "diff --git a/fixtures/sample.diff b/fixtures/sample.diff",
485 "--- a/fixtures/sample.diff",
486 "+++ b/fixtures/sample.diff",
487 "@@ -1,4 +1,4 @@",
488 " --- a/inner.rs",
489 " +++ b/inner.rs",
490 "-@@ -1 +1 @@",
491 "+@@ -2 +2 @@",
492 " tail",
493 ]);
494 let hunks = DiffHunks::parse(&diff);
495 assert!(hunks.contains_span("fixtures/sample.diff", 1, 4));
496 assert!(!hunks.touches_path("inner.rs"));
497 }
498
499 #[test]
500 fn quoted_path_is_declined_rather_than_mangled() {
501 let diff = diff(&[
502 "diff --git \"a/we\\tird.rs\" \"b/we\\tird.rs\"",
503 "--- \"a/we\\tird.rs\"",
504 "+++ \"b/we\\tird.rs\"",
505 "@@ -1,0 +1,1 @@",
506 "+x",
507 ]);
508 let hunks = DiffHunks::parse(&diff);
509 assert!(hunks.files.is_empty());
510 }
511
512 #[test]
513 fn empty_context_line_still_advances_the_cursor() {
514 // Some transports strip the trailing space from an empty context line.
515 let diff = diff(&[
516 "diff --git a/s.rs b/s.rs",
517 "--- a/s.rs",
518 "+++ b/s.rs",
519 "@@ -1,3 +1,4 @@",
520 " one",
521 "",
522 "+three",
523 " four",
524 ]);
525 let hunks = DiffHunks::parse(&diff);
526 assert!(hunks.contains_span("s.rs", 1, 4));
527 }
528
529 #[test]
530 fn contains_span_rejects_partial_and_inverted_ranges() {
531 let diff = diff(&[
532 "diff --git a/x.rs b/x.rs",
533 "--- a/x.rs",
534 "+++ b/x.rs",
535 "@@ -1,1 +1,2 @@",
536 " a",
537 "+b",
538 ]);
539 let hunks = DiffHunks::parse(&diff);
540 assert!(hunks.contains_span("x.rs", 1, 2));
541 assert!(!hunks.contains_span("x.rs", 1, 3));
542 assert!(!hunks.contains_span("x.rs", 2, 1));
543 assert!(!hunks.contains_span("missing.rs", 1, 1));
544 }
545
546 #[test]
547 fn empty_diff_yields_no_anchors() {
548 let hunks = DiffHunks::parse("");
549 assert!(!hunks.touches_path("anything.rs"));
550 assert!(!hunks.contains_line("anything.rs", 1));
551 }
552 }
553
553 lines RUST