Skip to content

Commit c95e869

Browse files
committed
diff: add option to display complex color-words diffs without inlining
In this patch, I use the number of adds<->removes alternation as a threshold, which approximates the visual complexity of diff hunks. I don't think user can choose the threshold intuitively, but we need a config knob to try out some. I set `max-inline-alternation = 3` locally. 0 and 1 mean "disable inlining" and "inline adds-only/removes-only lines" respectively. I've added "diff.<format>" config namespace assuming "ui.diff" will be reorganized as "ui.diff-formatter" or something. jj-vcs#3327 Some other metrics I've tried: ``` // Per-line alternation. This also works well, but can't measure complexity of // changes across lines. fn count_max_diff_alternation_per_line(diff_lines: &[DiffLine]) -> usize { diff_lines .iter() .map(|line| { let sides = line.hunks.iter().map(|&(side, _)| side); sides .filter(|&side| side != DiffLineHunkSide::Both) .dedup() // omit e.g. left->both->left .count() }) .max() .unwrap_or(0) } // Per-line occupancy of changes. Large diffs don't always look complex. fn max_diff_token_ratio_per_line(diff_lines: &[DiffLine]) -> f32 { diff_lines .iter() .filter_map(|line| { let [both_len, left_len, right_len] = line.hunks.iter().fold([0, 0, 0], |mut acc, (side, data)| { let index = match side { DiffLineHunkSide::Both => 0, DiffLineHunkSide::Left => 1, DiffLineHunkSide::Right => 2, }; acc[index] += data.len(); acc }); // left/right-only change is readable (left_len != 0 && right_len != 0).then(|| { let diff_len = left_len + right_len; let total_len = both_len + left_len + right_len; (diff_len as f32) / (total_len as f32) }) }) .reduce(f32::max) .unwrap_or(0.0) } // Total occupancy of changes. Large diffs don't always look complex. fn total_change_ratio(diff_lines: &[DiffLine]) -> f32 { let (diff_len, total_len) = diff_lines .iter() .flat_map(|line| &line.hunks) .fold((0, 0), |(diff_len, total_len), (side, data)| { let l = data.len(); match side { DiffLineHunkSide::Both => (diff_len, total_len + l), DiffLineHunkSide::Left => (diff_len + l, total_len + l), DiffLineHunkSide::Right => (diff_len + l, total_len + l), } }); (diff_len as f32) / (total_len as f32) } ```
1 parent 306bfb3 commit c95e869

File tree

7 files changed

+723
-7
lines changed

7 files changed

+723
-7
lines changed

CHANGELOG.md

+3
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
1818
`--color-words`, `--git`, `--stat`, `--summary`, `--types`, and external diff
1919
tools in file-by-file mode.
2020

21+
* Color-words diff has gained [an option to display complex changes as separate
22+
lines](docs/config.md#color-words-diff-options).
23+
2124
* A tilde (`~`) at the start of the path will now be expanded to the user's home
2225
directory when configuring a `signing.key` for SSH commit signing.
2326

cli/src/commit_templater.rs

+2
Original file line numberDiff line numberDiff line change
@@ -1343,8 +1343,10 @@ fn builtin_tree_diff_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, T
13431343
let path_converter = language.path_converter;
13441344
let template = (self_property, context_property)
13451345
.map(move |(diff, context)| {
1346+
// TODO: load defaults from UserSettings?
13461347
let options = diff_util::ColorWordsOptions {
13471348
context: context.unwrap_or(diff_util::DEFAULT_CONTEXT_LINES),
1349+
max_inline_alternation: None,
13481350
};
13491351
diff.into_formatted(move |formatter, store, tree_diff| {
13501352
diff_util::show_color_words_diff(

cli/src/config-schema.json

+16
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,22 @@
276276
]
277277
}
278278
},
279+
"diff": {
280+
"type": "object",
281+
"description": "Builtin diff formats settings",
282+
"properties": {
283+
"color-words": {
284+
"type": "object",
285+
"description": "Options for color-words diffs",
286+
"properties": {
287+
"max-inline-alternation": {
288+
"type": "integer",
289+
"description": "Maximum number of removed/added word alternation to inline"
290+
}
291+
}
292+
}
293+
}
294+
},
279295
"git": {
280296
"type": "object",
281297
"description": "Settings for git behavior (when using git backend)",

cli/src/config/misc.toml

+3
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@ amend = ["squash"]
55
co = ["checkout"]
66
unamend = ["unsquash"]
77

8+
[diff.color-words]
9+
max-inline-alternation = -1
10+
811
[format]
912
tree-level-conflicts = true
1013

cli/src/diff_util.rs

+93-7
Original file line numberDiff line numberDiff line change
@@ -401,15 +401,28 @@ fn collect_copied_sources<'a>(
401401
pub struct ColorWordsOptions {
402402
/// Number of context lines to show.
403403
pub context: usize,
404+
/// Maximum number of removed/added word alternation to inline.
405+
pub max_inline_alternation: Option<usize>,
404406
}
405407

406408
impl ColorWordsOptions {
407409
fn from_settings_and_args(
408-
_settings: &UserSettings,
410+
settings: &UserSettings,
409411
args: &DiffFormatArgs,
410412
) -> Result<Self, config::ConfigError> {
413+
let config = settings.config();
414+
let max_inline_alternation = {
415+
let key = "diff.color-words.max-inline-alternation";
416+
match config.get_int(key)? {
417+
-1 => None, // unlimited
418+
n => Some(usize::try_from(n).map_err(|err| {
419+
config::ConfigError::Message(format!("invalid {key}: {err}"))
420+
})?),
421+
}
422+
};
411423
Ok(ColorWordsOptions {
412424
context: args.context.unwrap_or(DEFAULT_CONTEXT_LINES),
425+
max_inline_alternation,
413426
})
414427
}
415428
}
@@ -461,13 +474,35 @@ fn show_color_words_diff_hunks(
461474
)?;
462475
}
463476
DiffHunk::Different(contents) => {
464-
let word_diff = Diff::by_word(&contents);
465-
let mut diff_line_iter =
466-
DiffLineIterator::with_line_number(word_diff.hunks(), line_number);
467-
for diff_line in diff_line_iter.by_ref() {
468-
show_color_words_diff_line(formatter, &diff_line)?;
477+
let word_diff_hunks = Diff::by_word(&contents).hunks().collect_vec();
478+
let can_inline = match options.max_inline_alternation {
479+
None => true, // unlimited
480+
Some(0) => false, // no need to count alternation
481+
Some(max_num) => {
482+
let groups = split_diff_hunks_by_matching_newline(&word_diff_hunks);
483+
groups.map(count_diff_alternation).max().unwrap_or(0) <= max_num
484+
}
485+
};
486+
if can_inline {
487+
let mut diff_line_iter =
488+
DiffLineIterator::with_line_number(word_diff_hunks.iter(), line_number);
489+
for diff_line in diff_line_iter.by_ref() {
490+
show_color_words_diff_line(formatter, &diff_line)?;
491+
}
492+
line_number = diff_line_iter.next_line_number();
493+
} else {
494+
let (left_lines, right_lines) = unzip_diff_hunks_to_lines(&word_diff_hunks);
495+
for tokens in &left_lines {
496+
show_color_words_line_number(formatter, Some(line_number.left), None)?;
497+
show_color_words_single_sided_line(formatter, tokens, "removed")?;
498+
line_number.left += 1;
499+
}
500+
for tokens in &right_lines {
501+
show_color_words_line_number(formatter, None, Some(line_number.right))?;
502+
show_color_words_single_sided_line(formatter, tokens, "added")?;
503+
line_number.right += 1;
504+
}
469505
}
470-
line_number = diff_line_iter.next_line_number();
471506
}
472507
}
473508
}
@@ -538,6 +573,7 @@ fn show_color_words_line_number(
538573
Ok(())
539574
}
540575

576+
/// Prints `diff_line` which may contain tokens originating from both sides.
541577
fn show_color_words_diff_line(
542578
formatter: &mut dyn Formatter,
543579
diff_line: &DiffLine,
@@ -572,6 +608,56 @@ fn show_color_words_diff_line(
572608
Ok(())
573609
}
574610

611+
/// Prints left/right-only line tokens with the given label.
612+
fn show_color_words_single_sided_line(
613+
formatter: &mut dyn Formatter,
614+
tokens: &[(DiffTokenType, &[u8])],
615+
label: &str,
616+
) -> io::Result<()> {
617+
formatter.with_label(label, |formatter| show_diff_line_tokens(formatter, tokens))?;
618+
let (_, data) = tokens.last().expect("diff line must not be empty");
619+
if !data.ends_with(b"\n") {
620+
writeln!(formatter)?;
621+
};
622+
Ok(())
623+
}
624+
625+
/// Counts number of diff-side alternation, ignoring matching hunks.
626+
///
627+
/// This function is meant to measure visual complexity of diff hunks. It's easy
628+
/// to read hunks containing some removed or added words, but is getting harder
629+
/// as more removes and adds interleaved.
630+
///
631+
/// For example,
632+
/// - `[matching]` -> 0
633+
/// - `[left]` -> 1
634+
/// - `[left, matching, left]` -> 1
635+
/// - `[matching, left, right, matching, right]` -> 2
636+
/// - `[left, right, matching, right, left]` -> 3
637+
fn count_diff_alternation(diff_hunks: &[DiffHunk]) -> usize {
638+
diff_hunks
639+
.iter()
640+
.filter_map(|hunk| match hunk {
641+
DiffHunk::Matching(_) => None,
642+
DiffHunk::Different(contents) => Some(contents),
643+
})
644+
// Map non-empty diff side to index (0: left, 1: right)
645+
.flat_map(|contents| contents.iter().positions(|content| !content.is_empty()))
646+
// Omit e.g. left->(matching->)*left
647+
.dedup()
648+
.count()
649+
}
650+
651+
/// Splits hunks into slices of contiguous changed lines.
652+
fn split_diff_hunks_by_matching_newline<'a, 'b>(
653+
diff_hunks: &'a [DiffHunk<'b>],
654+
) -> impl Iterator<Item = &'a [DiffHunk<'b>]> {
655+
diff_hunks.split_inclusive(|hunk| match hunk {
656+
DiffHunk::Matching(content) => content.contains(&b'\n'),
657+
DiffHunk::Different(_) => false,
658+
})
659+
}
660+
575661
struct FileContent {
576662
/// false if this file is likely text; true if it is likely binary.
577663
is_binary: bool,

0 commit comments

Comments
 (0)