T19 round 1 fixes

- merges keep each line's ending and the last newline; resolve keeps every other byte
- the split view numbers each side from its own file
- hunk headers come from similar's UnifiedHunkHeader: an empty side starts before its range
- the commit subject counter sits in the input's suffix
- clipped code rows in diffs, merges, blame and review set their line height
- gallery: scripted split view and a long subject

TASKS.md

@@ -242,4 +242,5 @@ Per-component lines live in `tasks/`. Tags there:
 | T18 | 1 | FAIL | a missing folder ran in the app's own; combining marks dropped; Tab skipped the terminal; its wheel scrolled the page too; clear kept find matches; New bash reused the old shell; the dark script typed into the replay |
 | T18 | 2 | FAIL | a folder it could not enter still started the shell elsewhere; links read cluster cells as spaces |
 | T18 | 3 | PASS | |
-| T19 | 1 | — | pending review |
+| T19 | 1 | FAIL | a merge rewrote line endings and added a last newline; the split view numbered its right side from the old file; an empty side started its hunk a line late; the subject counter covered long subjects; clipped code rows had no line height |
+| T19 | 2 | — | pending review |

examples/gallery/pages/git/diffs.rs

@@ -6,7 +6,10 @@ use ely_gpui_component::{
 };
 use gpui::{App, IntoElement, ParentElement, SharedString, Styled, Window, div, px};
 
-use crate::ui::{keep, section, set};
+use crate::{
+    probe::probe,
+    ui::{keep, section, set},
+};
 
 const OLD: &str = r#"use std::collections::HashMap;
 
@@ -84,7 +87,8 @@ pub fn viewer(window: &mut Window, cx: &mut App) -> impl IntoElement + use<> {
         "One file's changes, unified or side by side: each change in its context with old and new numbers, the words that changed washed, the stretches between folded until pressed. InlineDiff inside an editor is CodeEditor::set_diff.",
         cx,
     )
-    .child(
+    .child(probe(
+        "git-diff",
         div()
             .w(px(840.))
             .h(px(360.))
@@ -103,7 +107,7 @@ pub fn viewer(window: &mut Window, cx: &mut App) -> impl IntoElement + use<> {
                         set(&unfold, next, cx)
                     }),
             ),
-    )
+    ))
 }
 
 const BASE: &str =

examples/gallery/pages/git/mod.rs

@@ -16,7 +16,20 @@ pub const PAGE: Page = Page {
     script: SCRIPT,
 };
 
-const SCRIPT: &[Step] = &[Step::Rest];
+const SCRIPT: &[Step] = &[
+    Step::Rest,
+    Step::DownAt("git-diff", 800.0, 21.0),
+    Step::UpAt("git-diff", 800.0, 21.0),
+    Step::Wait(300),
+    Step::Shot("split"),
+    Step::DownAt("git-commit", 40.0, 20.0),
+    Step::UpAt("git-commit", 40.0, 20.0),
+    Step::Wait(200),
+    Step::Key("cmd-a"),
+    Step::Type("Blend every accent toward white by lift, clamped in gamma"),
+    Step::Wait(300),
+    Step::Shot("subject"),
+];
 
 fn render(window: &mut Window, cx: &mut App) -> AnyElement {
     div()

examples/gallery/pages/git/work.rs

@@ -10,7 +10,10 @@ use ely_gpui_component::{
 };
 use gpui::{App, Entity, IntoElement, ParentElement, SharedString, Styled, Window, div, px};
 
-use crate::ui::{keep, row, section, set};
+use crate::{
+    probe::probe,
+    ui::{keep, row, section, set},
+};
 
 fn changed(path: &str, status: GitStatus, added: usize, removed: usize) -> Changed {
     Changed {
@@ -113,7 +116,8 @@ pub fn changes(window: &mut Window, cx: &mut App) -> impl IntoElement + use<> {
                         }),
                 ),
             )
-            .child(
+            .child(probe(
+                "git-commit",
                 div().w(px(380.)).child(
                     CommitInput::new("git-commit", &subject, &body, "main", count)
                         .amend(now_amend)
@@ -124,7 +128,7 @@ pub fn changes(window: &mut Window, cx: &mut App) -> impl IntoElement + use<> {
                             set(&done, Some(format!("Committed {files} file{}.", if files == 1 { "" } else { "s" }).into()), cx);
                         }),
                 ),
-            ),
+            )),
     )
     .children(told.map(Caption::new))
 }

src/git/blame.rs

@@ -3,14 +3,14 @@ use std::{ops::Range, rc::Rc};
 use gpui::{
     AnyElement, App, ElementId, FontWeight, InteractiveElement, IntoElement, ParentElement,
     RenderOnce, SharedString, StatefulInteractiveElement, Styled, StyledText, Window, div,
-    prelude::*,
+    prelude::*, relative,
 };
 
 use crate::{
     data_display::Avatar,
     editor::code_colors,
     theme::{ActiveTheme, AvatarSize, TextSize},
-    typography::Ellipsis,
+    typography::{Ellipsis, LEADING},
 };
 
 /// Who last changed lines: the commit, its author, when and its subject, and its age from 0, newest, to 1, oldest.
@@ -190,6 +190,7 @@ impl RenderOnce for BlameView {
                     .flex_1()
                     .min_w_0()
                     .overflow_hidden()
+                    .line_height(relative(LEADING))
                     .flex()
                     .flex_col()
                     .children(span.clone().map(|line| {

src/git/changes.rs

@@ -333,9 +333,8 @@ impl RenderOnce for CommitInput {
                 }
             })
             .child(
-                div().relative().child(Input::new(&self.subject)).child(
+                Input::new(&self.subject).suffix(
-                    tabular(div().absolute().top_2().right_2().text_color(count))
+                    tabular(div().text_color(count)).child(format!("{length}/{}", SUBJECT.0)),
-                        .child(format!("{length}/{}", SUBJECT.0)),
                 ),
             )
             .child(Input::new(&self.body))

src/git/diff.rs

@@ -1,7 +1,7 @@
 use std::ops::Range;
 
 use gpui::SharedString;
-use similar::{ChangeTag, TextDiff};
+use similar::{ChangeTag, TextDiff, udiff::UnifiedHunkHeader};
 
 /// Whether a diff line stayed, came in, or went.
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
@@ -95,13 +95,7 @@ pub fn diff(old: &str, new: &str, context: usize) -> Vec<Stretch> {
                 });
             }
         }
-        let header = format!(
+        let header = UnifiedHunkHeader::new(&group).to_string();
-            "@@ -{},{} +{},{} @@",
-            old_start + 1,
-            old_end - old_start,
-            new_start + 1,
-            new_end - new_start
-        );
         out.push(Stretch::Hunk {
             header: header.into(),
             lines,
@@ -157,6 +151,17 @@ pub fn pairs(lines: &[DiffLine]) -> Vec<(Option<&DiffLine>, Option<&DiffLine>)>
     out
 }
 
+/// The numbers a side-by-side row shows: the old file's on the left, the new file's on the right.
+pub(crate) fn side_numbers(
+    left: Option<&DiffLine>,
+    right: Option<&DiffLine>,
+) -> (Option<usize>, Option<usize>) {
+    (
+        left.and_then(|line| line.old),
+        right.and_then(|line| line.new),
+    )
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -214,4 +219,29 @@ mod tests {
             .collect();
         assert_eq!(sides, [(true, true), (true, true), (false, true)]);
     }
+
+    #[test]
+    fn each_side_numbers_its_own_file() {
+        let stretches = diff("a\nb\n", "x\na\nb\n", 3);
+        let [Stretch::Hunk { lines, .. }] = stretches.as_slice() else {
+            panic!("one hunk, not {stretches:?}");
+        };
+        let numbers: Vec<_> = pairs(lines)
+            .into_iter()
+            .map(|(left, right)| side_numbers(left, right))
+            .collect();
+        assert_eq!(
+            numbers,
+            [(None, Some(1)), (Some(1), Some(2)), (Some(2), Some(3))]
+        );
+    }
+
+    #[test]
+    fn an_empty_side_starts_before_its_range() {
+        let stretches = diff("", "a\n", 3);
+        let [Stretch::Hunk { header, .. }] = stretches.as_slice() else {
+            panic!("one hunk, not {stretches:?}");
+        };
+        assert_eq!(header.as_ref(), "@@ -0,0 +1 @@");
+    }
 }

src/git/merge.rs

@@ -2,7 +2,7 @@ use std::rc::Rc;
 
 use gpui::{
     AnyElement, App, Div, ElementId, FontWeight, Hsla, IntoElement, ParentElement, RenderOnce,
-    SharedString, Styled, Window, div, prelude::*,
+    SharedString, Styled, Window, div, prelude::*, relative,
 };
 
 use super::merging::{Region, RegionKind, Take, conflicts, resolve, result};
@@ -10,7 +10,7 @@ use crate::{
     buttons::{Button, ButtonVariant},
     primitives::{Icon, IconName},
     theme::{ActiveTheme, ControlSize, IconSize, Palette, Radius, TextSize},
-    typography::format,
+    typography::{LEADING, format},
 };
 
 type OnTake = Rc<dyn Fn(usize, Take, &mut Window, &mut App)>;
@@ -26,10 +26,14 @@ fn column(lines: &[String], rows: usize, wash: Option<Hsla>) -> Div {
         .px_2()
         .when_some(wash, |column, wash| column.bg(wash))
         .children((0..rows).map(|ix| {
+            let line = lines
+                .get(ix)
+                .map_or("", |line| line.trim_end_matches(['\r', '\n']));
             div()
                 .whitespace_nowrap()
                 .overflow_hidden()
-                .child(lines.get(ix).cloned().unwrap_or_default())
+                .line_height(relative(LEADING))
+                .child(line.to_string())
         }))
 }
 

src/git/merging.rs

@@ -28,14 +28,34 @@ pub struct Region {
     pub theirs: Vec<String>,
 }
 
+/// Lines with their own endings, so a merge gives back each byte it keeps.
 fn lines(text: &str, range: std::ops::Range<usize>) -> Vec<String> {
-    text.lines()
+    text.split_inclusive('\n')
         .skip(range.start)
         .take(range.len())
         .map(str::to_string)
         .collect()
 }
 
+/// The line ending the merged texts use: the first one found, or a newline.
+fn ending(regions: &[Region]) -> &'static str {
+    let line = regions
+        .iter()
+        .flat_map(|region| region.base.iter().chain(&region.ours).chain(&region.theirs))
+        .find(|line| line.ends_with('\n'));
+    match line {
+        Some(line) if line.ends_with("\r\n") => "\r\n",
+        _ => "\n",
+    }
+}
+
+/// Ends the last line when it has no ending, so what follows starts a line.
+fn break_line(out: &mut String, ending: &str) {
+    if !out.is_empty() && !out.ends_with('\n') {
+        out.push_str(ending);
+    }
+}
+
 /// `ours` and `theirs` merged against `base`, region by region.
 pub fn regions(base: &str, ours: &str, theirs: &str) -> Vec<Region> {
     TextMerge::from_lines(base, ours, theirs)
@@ -59,41 +79,42 @@ pub fn regions(base: &str, ours: &str, theirs: &str) -> Vec<Region> {
 
 /// The merged text with each conflict as taken, and how many are still open; an open one keeps its markers.
 pub fn result(regions: &[Region], takes: &[Option<Take>]) -> (String, usize) {
-    let mut out: Vec<String> = Vec::new();
+    let ending = ending(regions);
+    let mut out = String::new();
     let mut conflicts = 0;
     let mut open = 0;
     for region in regions {
+        let (ours, theirs) = (region.ours.concat(), region.theirs.concat());
         match region.kind {
-            RegionKind::Unchanged | RegionKind::Ours | RegionKind::Both => {
+            RegionKind::Unchanged | RegionKind::Ours | RegionKind::Both => out.push_str(&ours),
-                out.extend(region.ours.iter().cloned())
+            RegionKind::Theirs => out.push_str(&theirs),
-            }
-            RegionKind::Theirs => out.extend(region.theirs.iter().cloned()),
             RegionKind::Conflict => {
                 match takes.get(conflicts).copied().flatten() {
-                    Some(Take::Ours) => out.extend(region.ours.iter().cloned()),
+                    Some(Take::Ours) => out.push_str(&ours),
-                    Some(Take::Theirs) => out.extend(region.theirs.iter().cloned()),
+                    Some(Take::Theirs) => out.push_str(&theirs),
                     Some(Take::Both) => {
-                        out.extend(region.ours.iter().cloned());
+                        out.push_str(&ours);
-                        out.extend(region.theirs.iter().cloned());
+                        break_line(&mut out, ending);
+                        out.push_str(&theirs);
                     }
                     None => {
                         open += 1;
-                        out.push("<<<<<<< ours".into());
+                        for (marker, side) in [("<<<<<<< ours", &ours), ("=======", &theirs)] {
-                        out.extend(region.ours.iter().cloned());
+                            break_line(&mut out, ending);
-                        out.push("=======".into());
+                            out.push_str(marker);
-                        out.extend(region.theirs.iter().cloned());
+                            out.push_str(ending);
-                        out.push(">>>>>>> theirs".into());
+                            out.push_str(side);
+                        }
+                        break_line(&mut out, ending);
+                        out.push_str(">>>>>>> theirs");
+                        out.push_str(ending);
                     }
                 }
                 conflicts += 1;
             }
         }
     }
-    let mut text = out.join("\n");
+    (out, open)
-    if !out.is_empty() {
-        text.push('\n');
-    }
-    (text, open)
 }
 
 /// A conflict in marked text: the lines of its markers, from zero.
@@ -126,26 +147,24 @@ pub fn conflicts(text: &str) -> Vec<Conflict> {
     found
 }
 
-/// `text` with conflict `ix` settled as taken.
+/// `text` with conflict `ix` settled as taken; every other byte stays.
 pub fn resolve(text: &str, ix: usize, take: Take) -> String {
     let conflict = conflicts(text)[ix];
-    let lines: Vec<&str> = text.lines().collect();
+    let lines: Vec<&str> = text.split_inclusive('\n').collect();
-    let ours = &lines[conflict.start + 1..conflict.middle];
+    let ours = lines[conflict.start + 1..conflict.middle].concat();
-    let theirs = &lines[conflict.middle + 1..conflict.end];
+    let theirs = lines[conflict.middle + 1..conflict.end].concat();
-    let kept: Vec<&str> = match take {
+    let kept = match take {
-        Take::Ours => ours.to_vec(),
+        Take::Ours => ours,
-        Take::Theirs => theirs.to_vec(),
+        Take::Theirs => theirs,
-        Take::Both => ours.iter().chain(theirs).copied().collect(),
+        Take::Both => ours + &theirs,
     };
-    let mut out: Vec<&str> = lines[..conflict.start].to_vec();
-    out.extend(kept);
-    out.extend(&lines[conflict.end + 1..]);
-    let mut settled = out.join("\n");
-    if text.ends_with('\n') {
-        settled.push('\n');
-    }
     log::info!("conflict {ix} took {take:?}");
-    settled
+    [
+        lines[..conflict.start].concat(),
+        kept,
+        lines[conflict.end + 1..].concat(),
+    ]
+    .concat()
 }
 
 #[cfg(test)]
@@ -169,6 +188,22 @@ mod tests {
         assert_eq!(taken, "fn lift() {\n    0.75\n}\nfn keep() { log() }\n");
     }
 
+    #[test]
+    fn merges_keep_line_endings_and_the_last_newline() {
+        assert_eq!(result(&regions("x", "x", "x"), &[]), ("x".into(), 0));
+        let crlf = regions("a\r\nb\r\n", "a\r\nB\r\n", "a\r\nb\r\n");
+        assert_eq!(result(&crlf, &[]).0, "a\r\nB\r\n");
+        let held = regions("a\r\n", "b\r\n", "c\r\n");
+        assert_eq!(
+            result(&held, &[]).0,
+            "<<<<<<< ours\r\nb\r\n=======\r\nc\r\n>>>>>>> theirs\r\n"
+        );
+        let bare = regions("a", "b", "c");
+        assert_eq!(result(&bare, &[Some(Take::Both)]).0, "b\nc");
+        let marked = "a\r\n<<<<<<< HEAD\r\nours\r\n=======\r\ntheirs\r\n>>>>>>> topic\r\nz";
+        assert_eq!(resolve(marked, 0, Take::Ours), "a\r\nours\r\nz");
+    }
+
     #[test]
     fn markers_resolve_to_one_side_or_both() {
         let marked = "a\n<<<<<<< HEAD\nours\n=======\ntheirs\n>>>>>>> topic\nz\n";

src/git/review.rs

@@ -3,7 +3,7 @@ use std::rc::Rc;
 use gpui::{
     AnyElement, App, ElementId, Entity, FontWeight, Hsla, InteractiveElement, IntoElement,
     ParentElement, RenderOnce, SharedString, StatefulInteractiveElement, Styled, Window, div,
-    prelude::*,
+    prelude::*, relative,
 };
 
 use super::badges::DiffStat;
@@ -13,7 +13,7 @@ use crate::{
     forms::{Input, TextInput},
     primitives::{Icon, IconName},
     theme::{ActiveTheme, AvatarSize, ControlSize, IconSize, Palette, Radius, TextSize},
-    typography::{Ellipsis, format},
+    typography::{Ellipsis, LEADING, format},
 };
 
 type Run = Rc<dyn Fn(&mut Window, &mut App)>;
@@ -359,6 +359,7 @@ impl RenderOnce for ReviewComment {
                     .text_size(theme.text_size(TextSize::Xs))
                     .whitespace_nowrap()
                     .overflow_hidden()
+                    .line_height(relative(LEADING))
                     .child(format!("{}  {}", self.line, self.code)),
             )
             .children(notes)

src/git/viewer.rs

@@ -3,18 +3,18 @@ use std::{ops::Range, rc::Rc};
 use gpui::{
     AnyElement, App, ElementId, FontWeight, HighlightStyle, Hsla, InteractiveElement, IntoElement,
     MouseButton, ParentElement, RenderOnce, SharedString, StatefulInteractiveElement, Styled,
-    StyledText, Window, div, prelude::*, uniform_list,
+    StyledText, Window, div, prelude::*, relative, uniform_list,
 };
 
 use super::{
     badges::DiffStat,
-    diff::{DiffLine, LineKind, Stretch, diff, pairs},
+    diff::{DiffLine, LineKind, Stretch, diff, pairs, side_numbers},
 };
 use crate::{
     buttons::SegmentedControl,
     editor::{code_colors, stack},
     theme::{ActiveTheme, ControlSize, Palette, TextSize},
-    typography::format,
+    typography::{LEADING, format},
 };
 
 /// How a diff lays out.
@@ -210,7 +210,7 @@ impl RenderOnce for DiffViewer {
                         .children(value.map(|value| value.to_string()))
                 };
                 let side =
-                    |line: Option<&DiffLine>, cx: &App| -> AnyElement {
+                    |line: Option<&DiffLine>, shown: Option<usize>, cx: &App| -> AnyElement {
                         let (wash, _) = line
                             .map_or((Some(colors.hover.opacity(0.5)), colors.hover), |line| {
                                 washes(line.kind, &colors)
@@ -222,7 +222,7 @@ impl RenderOnce for DiffViewer {
                             .gap_2()
                             .overflow_hidden()
                             .when_some(wash, |side, wash| side.bg(wash))
-                            .child(number(line.and_then(|line| line.old.or(line.new))))
+                            .child(number(shown))
                             .children(line.map(|line| {
                                 div().whitespace_nowrap().child(code(line, &colors, cx))
                             }))
@@ -230,7 +230,13 @@ impl RenderOnce for DiffViewer {
                     };
                 range
                     .map(|ix| {
-                        let row = div().id(ix).w_full().flex().px_2().whitespace_nowrap();
+                        let row = div()
+                            .id(ix)
+                            .w_full()
+                            .flex()
+                            .px_2()
+                            .whitespace_nowrap()
+                            .line_height(relative(LEADING));
                         match &rows[ix] {
                             Shown::Header(header) => row
                                 .bg(colors.hover)
@@ -260,11 +266,13 @@ impl RenderOnce for DiffViewer {
                                     )
                                     .into_any_element()
                             }
-                            Shown::Pair(left, right) => row
+                            Shown::Pair(left, right) => {
-                                .gap_1()
+                                let (old, new) = side_numbers(left.as_ref(), right.as_ref());
-                                .child(side(left.as_ref(), cx))
+                                row.gap_1()
-                                .child(side(right.as_ref(), cx))
+                                    .child(side(left.as_ref(), old, cx))
-                                .into_any_element(),
+                                    .child(side(right.as_ref(), new, cx))
+                                    .into_any_element()
+                            }
                             Shown::Fold(stretch, count) => {
                                 let (open, stretch) = (on_open.clone(), *stretch);
                                 row.id((id.clone(), format!("fold-{stretch}")))