From b2c587546edf48cbf69c28e17b19d5591ea6cc6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Sat, 26 Sep 2026 02:28:13 -0400 Subject: [PATCH] T19 round 2 fixes - regions read their lines through TextMerge's accessors; conflicts and resolve split with similar's tokenize_lines, so a lone return ends a line everywhere - the merged ending and line breaks know a lone return - taking both sides separates them only when both hold text - merge views split rows the same way --- TASKS.md | 3 ++- src/git/merge.rs | 8 +++++-- src/git/merging.rs | 56 +++++++++++++++++++++++++++++++++------------- 3 files changed, 48 insertions(+), 19 deletions(-) diff --git a/TASKS.md b/TASKS.md index 2e74934..804e05e 100644 --- a/TASKS.md +++ b/TASKS.md @@ -243,4 +243,5 @@ Per-component lines live in `tasks/`. Tags there: | 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 | 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 | +| T19 | 2 | FAIL | a lone return split lines for similar but not for regions, so a change was lost; taking both sides added a newline to an empty side | +| T19 | 3 | — | pending review | diff --git a/src/git/merge.rs b/src/git/merge.rs index 839142a..5fd4549 100644 --- a/src/git/merge.rs +++ b/src/git/merge.rs @@ -5,6 +5,8 @@ use gpui::{ SharedString, Styled, Window, div, prelude::*, relative, }; +use similar::DiffableStr; + use super::merging::{Region, RegionKind, Take, conflicts, resolve, result}; use crate::{ buttons::{Button, ButtonVariant}, @@ -238,7 +240,8 @@ impl RenderOnce for ThreeWayMerge { .font_family(theme.mono_family.clone()) .text_size(theme.text_size(TextSize::Xs)) .text_color(colors.fg) - .children(merged.lines().map(|line| { + .children(merged.tokenize_lines().into_iter().map(|line| { + let line = line.trim_end_matches(['\r', '\n']); let marker = ["<<<<<<<", "=======", ">>>>>>>"] .iter() .any(|mark| line.starts_with(mark)); @@ -298,7 +301,8 @@ impl RenderOnce for ConflictResolver { }); let (current, incoming) = (colors.success.opacity(0.1), colors.info.opacity(0.1)); let mut rows: Vec = Vec::new(); - for (ix, line) in self.text.lines().enumerate() { + for (ix, line) in self.text.tokenize_lines().into_iter().enumerate() { + let line = line.trim_end_matches(['\r', '\n']); if let Some(at) = found.iter().position(|conflict| conflict.start == ix) { rows.push( takes( diff --git a/src/git/merging.rs b/src/git/merging.rs index efee2cc..c61e4fc 100644 --- a/src/git/merging.rs +++ b/src/git/merging.rs @@ -1,4 +1,4 @@ -use similar::{MergeResolution, TextMerge}; +use similar::{DiffableStr, MergeResolution, TextMerge}; /// Which side a conflict takes. #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -28,12 +28,17 @@ pub struct Region { pub theirs: Vec, } -/// Lines with their own endings, so a merge gives back each byte it keeps. -fn lines(text: &str, range: std::ops::Range) -> Vec { - text.split_inclusive('\n') - .skip(range.start) - .take(range.len()) - .map(str::to_string) +/// A region's lines as similar split them, each with its ending. +fn lines<'a>( + range: std::ops::Range, + line: impl Fn(usize) -> Option<&'a str>, +) -> Vec { + range + .map(|ix| { + line(ix) + .expect("a region's lines are in its text") + .to_string() + }) .collect() } @@ -42,23 +47,25 @@ fn ending(regions: &[Region]) -> &'static str { let line = regions .iter() .flat_map(|region| region.base.iter().chain(®ion.ours).chain(®ion.theirs)) - .find(|line| line.ends_with('\n')); + .find(|line| line.as_str().ends_with_newline()); match line { Some(line) if line.ends_with("\r\n") => "\r\n", + Some(line) if line.ends_with('\r') => "\r", _ => "\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') { + if !out.is_empty() && !out.as_str().ends_with_newline() { out.push_str(ending); } } /// `ours` and `theirs` merged against `base`, region by region. pub fn regions(base: &str, ours: &str, theirs: &str) -> Vec { - TextMerge::from_lines(base, ours, theirs) + let merge = TextMerge::from_lines(base, ours, theirs); + merge .regions() .iter() .map(|region| Region { @@ -70,9 +77,9 @@ pub fn regions(base: &str, ours: &str, theirs: &str) -> Vec { MergeResolution::Conflict => RegionKind::Conflict, other => unreachable!("a merge resolution this crate does not know: {other:?}"), }, - base: lines(base, region.base_range()), - ours: lines(ours, region.ours_range()), - theirs: lines(theirs, region.theirs_range()), + base: lines(region.base_range(), |ix| merge.base_line(ix)), + ours: lines(region.ours_range(), |ix| merge.ours_line(ix)), + theirs: lines(region.theirs_range(), |ix| merge.theirs_line(ix)), }) .collect() } @@ -94,7 +101,9 @@ pub fn result(regions: &[Region], takes: &[Option]) -> (String, usize) { Some(Take::Theirs) => out.push_str(&theirs), Some(Take::Both) => { out.push_str(&ours); - break_line(&mut out, ending); + if !theirs.is_empty() { + break_line(&mut out, ending); + } out.push_str(&theirs); } None => { @@ -129,7 +138,7 @@ pub struct Conflict { pub fn conflicts(text: &str) -> Vec { let mut found = Vec::new(); let (mut start, mut middle) = (None, None); - for (ix, line) in text.lines().enumerate() { + for (ix, line) in text.tokenize_lines().into_iter().enumerate() { if line.starts_with("<<<<<<<") { (start, middle) = (Some(ix), None); } else if line.starts_with("=======") && start.is_some() { @@ -150,7 +159,7 @@ pub fn conflicts(text: &str) -> Vec { /// `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.split_inclusive('\n').collect(); + let lines = text.tokenize_lines(); let ours = lines[conflict.start + 1..conflict.middle].concat(); let theirs = lines[conflict.middle + 1..conflict.end].concat(); let kept = match take { @@ -204,6 +213,21 @@ mod tests { assert_eq!(resolve(marked, 0, Take::Ours), "a\r\nours\r\nz"); } + #[test] + fn a_bare_return_ends_a_line_as_it_does_for_similar() { + let changed = regions("a\rb\n", "a\rb\n", "a\rB\n"); + assert_eq!(result(&changed, &[]).0, "a\rB\n"); + let held = result(®ions("a\r", "b\r", "c\r"), &[]).0; + assert_eq!(held, "<<<<<<< ours\rb\r=======\rc\r>>>>>>> theirs\r"); + assert_eq!(resolve(&held, 0, Take::Theirs), "c\r"); + } + + #[test] + fn taking_both_adds_no_line_to_an_empty_side() { + assert_eq!(result(®ions("a", "b", ""), &[Some(Take::Both)]).0, "b"); + assert_eq!(result(®ions("a", "", "c"), &[Some(Take::Both)]).0, "c"); + } + #[test] fn markers_resolve_to_one_side_or_both() { let marked = "a\n<<<<<<< HEAD\nours\n=======\ntheirs\n>>>>>>> topic\nz\n";