Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
</p>

<p align="center">
A portable, efficeint tool for reviewing code
A portable, efficient tool for reviewing code

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[praise] I like how you fixed this grammar

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank yoU!

</p>

## Vision
Expand Down Expand Up @@ -31,3 +31,4 @@ brew install sdavisde/tap/redquill
## Documentation

- [`docs/forge-setup.md`](docs/forge-setup.md) — Pull Requests tab: supported providers, zero-config detection, hosted-instance setup, troubleshooting.
- [`docs/diff-summary.md`](docs/diff-summary.md) — the review-wide summary model: what counts as churn, how binary and rename-only files are treated, and where the numbers come from.
49 changes: 49 additions & 0 deletions docs/diff-summary.md
Comment thread
sdavisde marked this conversation as resolved.
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# Review summary model

`diff::summarize` rolls a whole review up into one `ReviewSummary`, so the
question "how big is this, and where should I start?" is answerable without
walking hunks at a render surface.

## What it reports

| Field | Meaning |
| --- | --- |
| `files` | Every file in the review — binary and rename-only included. |
| `binary_files` | How many of those are binary. |
| `content_files` | Files whose content actually changed (added, deleted, modified) and that carry at least one changed line. |
| `stat` | Added/removed lines summed across the review. |
| `largest_file` | The file with the most changed lines, as a `Hotspot { path, stat }`. |
| `largest_hunk` | The biggest single hunk's changed-line count, across all files. |

## Counting rules

**Churn, not net.** `DiffStat::total()` is `added + removed`, so a line
rewritten in place counts twice — once on each side. That's deliberate: the
measure exists to estimate how much there is to *read*, and rewriting a line
means reading two. `DiffStat::net()` is there for the cases that want the
signed difference instead.

**Context lines never count.** A hunk with two changed lines and forty lines
of context is a two-line hunk as far as the summary is concerned. This
matches `Hunk::stats`, which has always excluded context.

**Binary files count as files, never as lines.** A line count over binary
content is meaningless, so binary files land in `files` and `binary_files`
and are skipped entirely for `stat`, `largest_file`, and `largest_hunk`.
This is the same call `stat_display` makes when it renders `bin` instead of
`+0 -0`.

**Rename-only changes count as files, not content changes.** A pure rename
carries no hunks, so it contributes to `files` but not to `content_files` —
`FileChangeKind::is_content_change` draws that line.

**Ties keep the earlier file.** When two files carry identical churn,
`largest_file` reports whichever came first in the input. Callers keep the
file list path-sorted, so the tiebreak is stable and path-ordered rather
than arbitrary.

## Where the numbers come from

`build_review` computes the summary once, on the background snapshot build,
and hands it to the UI on `ReviewSnapshot`. Render surfaces read the
precomputed value; nothing recomputes it per frame.
10 changes: 10 additions & 0 deletions src/diff/file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,16 @@ impl FileChangeKind {
}
}

/// Whether this kind implies the file's content changed. A rename or
/// copy may carry hunks or be path-only; every other kind always
/// carries content.
pub fn is_content_change(self) -> bool {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[issue] What is this function doing?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing important. why?

matches!(
self,
FileChangeKind::Added | FileChangeKind::Deleted | FileChangeKind::Modified
)
}

/// Derives the change kind from a raw patch's header text and metadata.
fn from_raw(patch: &RawFilePatch) -> FileChangeKind {
if patch.raw.contains("\nnew file mode ") {
Expand Down
4 changes: 4 additions & 0 deletions src/diff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,21 @@
//! - [`FileDiff::stats`]/[`Hunk::stats`] count added/removed lines, and
//! [`stat_display`] decides how a file's counts should render (real
//! counts, binary, or omitted).
//! - [`summarize`] rolls a whole review's files up into a [`ReviewSummary`]:
//! file/binary counts, total churn, and the largest file and hunk.

mod error;
mod file;
mod hunk;
mod line;
mod stat;
mod summary;
mod word;

pub use error::DiffParseError;
pub use file::{FileChangeKind, FileDiff};
pub use hunk::{Hunk, parse_hunks};
pub use line::{DiffLine, LineOrigin};
pub use stat::{DiffStat, StatDisplay, stat_display};
pub use summary::{Hotspot, ReviewSummary, summarize};
pub use word::{WordSpan, pair_hunk_lines, word_diff};
78 changes: 77 additions & 1 deletion src/diff/stat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,25 @@ pub struct DiffStat {
pub removed: usize,
}

impl DiffStat {
/// Total changed lines — added plus removed. The churn measure: a line
/// rewritten in place counts twice, once on each side, which is what
/// makes it a reasonable proxy for how much there is to read.
pub fn total(self) -> usize {
self.added + self.removed
}

/// Lines gained minus lines lost. Negative for a net deletion.
pub fn net(self) -> isize {
self.added as isize - self.removed as isize
}

/// Whether nothing changed on either side.
pub fn is_empty(self) -> bool {
self.added == 0 && self.removed == 0
}
}

impl std::ops::AddAssign for DiffStat {
fn add_assign(&mut self, other: DiffStat) {
self.added += other.added;
Expand Down Expand Up @@ -47,6 +66,12 @@ impl Hunk {
acc
})
}

/// How many of this hunk's lines actually changed — its churn, context
/// excluded. Distinct from [`Hunk::new_count`], which spans context too.
pub fn changed_lines(&self) -> usize {
self.stats().total()
}
}

impl FileDiff {
Expand Down Expand Up @@ -87,7 +112,7 @@ pub enum StatDisplay {
pub fn stat_display(file: &FileDiff, stat: DiffStat) -> StatDisplay {
if file.is_binary {
StatDisplay::Binary
} else if file.hunks.is_empty() || (stat.added == 0 && stat.removed == 0) {
} else if file.hunks.is_empty() || stat.is_empty() {
StatDisplay::Omitted
} else {
StatDisplay::Counts(stat)
Expand Down Expand Up @@ -141,6 +166,57 @@ mod tests {
);
}

#[test]
fn hunk_changed_lines_excludes_context() {
let h = hunk(vec![
line(LineOrigin::Context),
line(LineOrigin::Context),
line(LineOrigin::Added),
line(LineOrigin::Removed),
]);
assert_eq!(h.changed_lines(), 2);
}

// -- DiffStat arithmetic --

#[test]
fn total_and_net_treat_a_rewritten_line_differently() {
let stat = DiffStat {
added: 4,
removed: 4,
};
assert_eq!(stat.total(), 8);
assert_eq!(stat.net(), 0);
}

#[test]
fn net_goes_negative_for_a_deletion_heavy_stat() {
let stat = DiffStat {
added: 1,
removed: 6,
};
assert_eq!(stat.net(), -5);
}

#[test]
fn is_empty_only_when_neither_side_changed() {
assert!(DiffStat::default().is_empty());
assert!(
!DiffStat {
added: 0,
removed: 1
}
.is_empty()
);
assert!(
!DiffStat {
added: 1,
removed: 0
}
.is_empty()
);
}

// -- FileDiff::stats --

#[test]
Expand Down
Loading
Loading