Line up visible lines and include gaps if necessary

CHANGELOG.md

@@ -1,5 +1,7 @@
 ## 0.3 (unreleased)
 
+Diffs are now displayed with unchanged lines aligned to the other side.
+
 Improved Rust parsing to recognise lifetime syntax `'foo`, character
 literals `'x'` and sequences of punctuation.
 

src/lines.rs

@@ -1,7 +1,8 @@
 use crate::positions::SingleLineSpan;
-use crate::syntax::{MatchKind, MatchedPos};
+use crate::syntax::{aligned_lines, MatchKind, MatchedPos};
 use regex::Regex;
 use std::cmp::{max, min, Ordering};
+use std::collections::HashMap;
 use std::fmt;
 use std::ops::RangeInclusive;
 
@@ -43,22 +44,30 @@ impl LineGroup {
         }
     }
 
-    // We can't iterate over a RangeInclusive<LineNumber> in safe, stable Rust. See
-    // https://github.com/rust-lang/rust/issues/42168
-    pub fn iter_lhs_lines(&self) -> RangeInclusive<usize> {
-        let empty_iter = 1usize..=0;
+    fn lhs_lines(&self) -> Vec<LineNumber> {
+        let mut res = vec![];
         match &self.lhs_lines {
-            Some(lhs_lines) => lhs_lines.start().number..=lhs_lines.end().number,
-            None => empty_iter,
+            Some(lhs_lines) => {
+                for line in lhs_lines.start().number..=lhs_lines.end().number {
+                    res.push(line.into());
+                }
+            }
+            None => {}
         }
+        res
     }
 
-    pub fn iter_rhs_lines(&self) -> RangeInclusive<usize> {
-        let empty_iter = 1usize..=0;
+    fn rhs_lines(&self) -> Vec<LineNumber> {
+        let mut res = vec![];
         match &self.rhs_lines {
-            Some(rhs_lines) => rhs_lines.start().number..=rhs_lines.end().number,
-            None => empty_iter,
+            Some(rhs_lines) => {
+                for line in rhs_lines.start().number..=rhs_lines.end().number {
+                    res.push(line.into());
+                }
+            }
+            None => {}
         }
+        res
     }
 
     pub fn pad(&mut self, amount: usize, max_lhs_line: LineNumber, max_rhs_line: LineNumber) {
@@ -393,25 +402,45 @@ fn apply_group(
     lhs_lines: &[&str],
     rhs_lines: &[&str],
     group: &LineGroup,
+    lhs_line_matches: &HashMap<LineNumber, LineNumber>,
     lhs_content_width: usize,
     lhs_column_width: usize,
     rhs_column_width: usize,
 ) -> String {
     let mut lhs_result = String::new();
-    for lhs_line_num in group.iter_lhs_lines() {
-        lhs_result.push_str(&format_line_num_padded(lhs_line_num, lhs_column_width));
+    let mut rhs_result = String::new();
 
-        match lhs_lines.get(lhs_line_num) {
-            Some(line) => lhs_result.push_str(line),
-            None => lhs_result.push_str(&" ".repeat(lhs_content_width)),
+    for (lhs_line_num, rhs_line_num) in
+        aligned_lines(&group.lhs_lines(), &group.rhs_lines(), lhs_line_matches)
+    {
+        // TODO: we could build up a single string rather than
+        // horizontally concatenating afterwards.
+
+        match lhs_line_num {
+            Some(lhs_line_num) => {
+                lhs_result.push_str(&format_line_num_padded(
+                    lhs_line_num.number,
+                    lhs_column_width,
+                ));
+                lhs_result.push_str(lhs_lines[lhs_line_num.number]);
+            }
+            None => {
+                lhs_result.push_str(&" ".repeat(lhs_column_width));
+                lhs_result.push_str(&" ".repeat(lhs_content_width));
+            }
         }
         lhs_result.push('\n');
-    }
 
-    let mut rhs_result = String::new();
-    for rhs_line_num in group.iter_rhs_lines() {
-        rhs_result.push_str(&format_line_num_padded(rhs_line_num, rhs_column_width));
-        rhs_result.push_str(rhs_lines.get(rhs_line_num).unwrap_or(&""));
+        match rhs_line_num {
+            Some(rhs_line_num) => {
+                rhs_result.push_str(&format_line_num_padded(
+                    rhs_line_num.number,
+                    rhs_column_width,
+                ));
+                rhs_result.push_str(rhs_lines[rhs_line_num.number]);
+            }
+            None => {}
+        }
         rhs_result.push('\n');
     }
 
@@ -429,6 +458,7 @@ pub fn apply_groups(
     lhs: &str,
     rhs: &str,
     groups: &[LineGroup],
+    lhs_line_matches: &HashMap<LineNumber, LineNumber>,
     lhs_content_width: usize,
     lhs_column_width: usize,
     rhs_column_width: usize,
@@ -445,6 +475,7 @@ pub fn apply_groups(
             &lhs_lines,
             &rhs_lines,
             group,
+            lhs_line_matches,
             lhs_content_width,
             lhs_column_width,
             rhs_column_width,

src/main.rs

@@ -16,7 +16,7 @@ use crate::lines::{
 };
 use crate::parse::{find_lang, parse, parse_lines, read_or_die, ConfigDir};
 use crate::style::apply_colors;
-use crate::syntax::{change_positions, set_next};
+use crate::syntax::{change_positions, matching_lines, set_next};
 
 fn term_width() -> Option<usize> {
     term_size::dimensions().map(|(w, _)| w)
@@ -91,6 +91,8 @@ fn main() {
     let lhs_positions = change_positions(&lhs_src, &rhs_src, &lhs);
     let rhs_positions = change_positions(&rhs_src, &lhs_src, &rhs);
 
+    let lhs_matched_lines = matching_lines(&lhs);
+
     let mut groups = visible_groups(&lhs_positions, &rhs_positions);
     for group in &mut groups {
         group.pad(3, lhs_src.max_line(), rhs_src.max_line());
@@ -127,6 +129,7 @@ fn main() {
             &lhs_colored,
             &rhs_colored,
             &groups,
+            &lhs_matched_lines,
             lhs_content_width,
             lhs_column_width,
             rhs_column_width,

src/syntax.rs

@@ -3,8 +3,9 @@
 
 use itertools::{EitherOrBoth, Itertools};
 use std::cell::Cell;
+use std::cmp::min;
 use std::collections::hash_map::DefaultHasher;
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 use std::fmt;
 use std::hash::{Hash, Hasher};
 use typed_arena::Arena;
@@ -520,6 +521,22 @@ fn change_positions_<'a>(
     }
 }
 
+fn zip_pad_shorter<Tx: Copy, Ty: Copy>(lhs: &[Tx], rhs: &[Ty]) -> Vec<(Option<Tx>, Option<Ty>)> {
+    let mut res = vec![];
+
+    let mut i = 0;
+    loop {
+        match (lhs.get(i), rhs.get(i)) {
+            (None, None) => break,
+            (x, y) => res.push((x.map(|v| *v), y.map(|v| *v))),
+        }
+
+        i += 1;
+    }
+
+    res
+}
+
 /// Given two slices of line positions, return a list of line number
 /// pairs. If the slices have different lengths, reuse the last item
 /// from the shorter slice.
@@ -555,59 +572,53 @@ pub fn aligned_lines(
     lhs_lines: &[LineNumber],
     rhs_lines: &[LineNumber],
     lhs_line_matches: &HashMap<LineNumber, LineNumber>,
-    rhs_line_matches: &HashMap<LineNumber, LineNumber>,
 ) -> Vec<(Option<LineNumber>, Option<LineNumber>)> {
-    // Find RHS lines that we can match up.
-    let mut lhs_opposite_lines = vec![];
-    for lhs_line in lhs_lines {
-        if let Some(lhs_opposite_line) = lhs_line_matches.get(lhs_line) {
-            lhs_opposite_lines.push(lhs_opposite_line);
-        }
-    }
+    let mut rhs_lines_available: HashSet<_> = rhs_lines.iter().collect();
 
-    // Find LHS lines that we can match up.
-    let mut rhs_opposite_lines = vec![];
-    for rhs_line in rhs_lines {
-        if let Some(rhs_opposite_line) = rhs_line_matches.get(rhs_line) {
-            rhs_opposite_lines.push(rhs_opposite_line);
+    // For every LHS line, if there is a RHS line that included in
+    // `rhs_lines` and hasn't yet been paired up, add it to matched_lines.
+    let mut matched_lines = vec![];
+    for lhs_line in lhs_lines {
+        if let Some(rhs_line) = lhs_line_matches.get(lhs_line) {
+            if rhs_lines_available.remove(&rhs_line) {
+                matched_lines.push((lhs_line, rhs_line));
+            }
         }
     }
 
-    // Sanity check: if LHS X matches RHS Y, then RHS Y should match
-    // LHS X and we should have the same number of opposite lines.
-    assert_eq!(lhs_opposite_lines.len(), rhs_opposite_lines.len());
-
     let mut res = vec![];
 
     let mut lhs_i = 0;
     let mut rhs_i = 0;
 
-    // Build a vec of matched lines, padding the unmatched sequences with None.
-    for (lhs_matched_line, rhs_matched_line) in rhs_opposite_lines
-        .into_iter()
-        .zip(lhs_opposite_lines.into_iter())
-    {
+    // Build a vec of matched line tuples. For lines without matches
+    // (novel lines, empty lines), just match lines up pairwise. Pad
+    // gaps if one side has more lines.
+    for (lhs_matched_line, rhs_matched_line) in matched_lines {
+        let mut lhs_prev_lines = vec![];
         while lhs_i < lhs_lines.len() && lhs_lines[lhs_i] < *lhs_matched_line {
-            res.push((Some(lhs_lines[lhs_i]), None));
+            lhs_prev_lines.push(lhs_lines[lhs_i]);
             lhs_i += 1;
         }
+        let mut rhs_prev_lines = vec![];
         while rhs_i < rhs_lines.len() && rhs_lines[rhs_i] < *rhs_matched_line {
-            res.push((None, Some(rhs_lines[rhs_i])));
+            rhs_prev_lines.push(rhs_lines[rhs_i]);
             rhs_i += 1;
         }
-        res.push((Some(*lhs_matched_line), Some(*rhs_matched_line)));
-    }
 
-    // If we have trailing unmatched lines on other side, add them now.
-    while lhs_i < lhs_lines.len() {
-        res.push((Some(lhs_lines[lhs_i]), None));
+        res.extend(zip_pad_shorter(&lhs_prev_lines, &rhs_prev_lines));
+
+        res.push((Some(*lhs_matched_line), Some(*rhs_matched_line)));
         lhs_i += 1;
-    }
-    while rhs_i < rhs_lines.len() {
-        res.push((None, Some(rhs_lines[rhs_i])));
         rhs_i += 1;
     }
 
+    // Handle unmatched lines after the last match.
+    res.extend(zip_pad_shorter(
+        &lhs_lines[min(lhs_i, lhs_lines.len())..],
+        &rhs_lines[min(rhs_i, rhs_lines.len())..],
+    ));
+
     res
 }
 
@@ -675,6 +686,70 @@ mod tests {
     use pretty_assertions::assert_eq;
     use AtomKind::Other;
 
+    #[test]
+    fn test_aligned_middle() {
+        let lhs_lines: Vec<LineNumber> = vec![1.into(), 2.into()];
+        let rhs_lines: Vec<LineNumber> = vec![12.into(), 13.into()];
+
+        let mut line_matches: HashMap<LineNumber, LineNumber> = HashMap::new();
+        line_matches.insert(2.into(), 12.into());
+
+        assert_eq!(
+            aligned_lines(&lhs_lines, &rhs_lines, &line_matches),
+            vec![
+                (Some(1.into()), None),
+                (Some(2.into()), Some(12.into())),
+                (None, Some(13.into()))
+            ]
+        );
+    }
+
+    #[test]
+    fn test_aligned_all() {
+        let lhs_lines: Vec<LineNumber> = vec![1.into(), 2.into()];
+        let rhs_lines: Vec<LineNumber> = vec![11.into(), 12.into()];
+
+        let mut line_matches: HashMap<LineNumber, LineNumber> = HashMap::new();
+        line_matches.insert(1.into(), 2.into());
+        line_matches.insert(2.into(), 12.into());
+
+        assert_eq!(
+            aligned_lines(&lhs_lines, &rhs_lines, &line_matches),
+            vec![
+                (Some(1.into()), Some(11.into())),
+                (Some(2.into()), Some(12.into())),
+            ]
+        );
+    }
+
+    #[test]
+    fn test_aligned_none() {
+        let lhs_lines: Vec<LineNumber> = vec![1.into()];
+        let rhs_lines: Vec<LineNumber> = vec![11.into()];
+
+        let line_matches: HashMap<LineNumber, LineNumber> = HashMap::new();
+
+        assert_eq!(
+            aligned_lines(&lhs_lines, &rhs_lines, &line_matches),
+            vec![(Some(1.into()), Some(11.into()))]
+        );
+    }
+
+    #[test]
+    fn test_aligned_line_overlap() {
+        let lhs_lines: Vec<LineNumber> = vec![1.into(), 2.into()];
+        let rhs_lines: Vec<LineNumber> = vec![11.into()];
+
+        let mut line_matches: HashMap<LineNumber, LineNumber> = HashMap::new();
+        line_matches.insert(1.into(), 11.into());
+        line_matches.insert(2.into(), 11.into());
+
+        assert_eq!(
+            aligned_lines(&lhs_lines, &rhs_lines, &line_matches),
+            vec![(Some(1.into()), Some(11.into())), (Some(2.into()), None)]
+        );
+    }
+
     /// Ensure that we assign prev_opposite_pos even if the change is on the first node.
     #[test]
     fn test_prev_opposite_pos_first_node() {