internal/diff: fix LineEdits bug in slow path

Previously, the expandEdit operation would expand to
the end of the line unconditionally, but this caused it
to gulp an extra line if it was already line-aligned.

This change causes it to do the expansion only if
the end is not line-aligned, or the replacement text
doesn't end with a newline.

Now, removing the fast path no longer causes tests to fail.
This also allows us to remove the logic added in CL 489695
to work around issue golang/go#59232.

Fixes golang/go#60379
Fixes golang/go#59232

Change-Id: Ia40e4e3bb714d75acb95103a38e8c49a8ef456de
Reviewed-on: https://go-review.googlesource.com/c/tools/+/499377
Run-TryBot: Alan Donovan <[email protected]>
gopls-CI: kokoro <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Peter Weinberger <[email protected]>

Author: adonovan

internal/diff/diff.go

@@ -18,7 +18,7 @@ type Edit struct {
 }
 
 func (e Edit) String() string {
-	return fmt.Sprintf("{Start:%d,End:%d,New:%s}", e.Start, e.End, e.New)
+	return fmt.Sprintf("{Start:%d,End:%d,New:%q}", e.Start, e.End, e.New)
 }
 
 // Apply applies a sequence of edits to the src buffer and returns the
@@ -116,19 +116,21 @@ func lineEdits(src string, edits []Edit) ([]Edit, error) {
 
 	// Do all deletions begin and end at the start of a line,
 	// and all insertions end with a newline?
-	// TODO(adonovan, pjw): why does omitting this 'optimization'
-	// cause tests to fail? (TestDiff/insert-line,extra_newline)
+	// (This is merely a fast path.)
 	for _, edit := range edits {
 		if edit.Start >= len(src) || // insertion at EOF
 			edit.Start > 0 && src[edit.Start-1] != '\n' || // not at line start
 			edit.End > 0 && src[edit.End-1] != '\n' || // not at line start
 			edit.New != "" && edit.New[len(edit.New)-1] != '\n' { // partial insert
-			goto expand
+			goto expand // slow path
 		}
 	}
 	return edits, nil // aligned
 
 expand:
+	if len(edits) == 0 {
+		return edits, nil // no edits (unreachable due to fast path)
+	}
 	expanded := make([]Edit, 0, len(edits)) // a guess
 	prev := edits[0]
 	// TODO(adonovan): opt: start from the first misaligned edit.
@@ -160,10 +162,13 @@ func expandEdit(edit Edit, src string) Edit {
 
 	// Expand end right to end of line.
 	end := edit.End
-	if nl := strings.IndexByte(src[end:], '\n'); nl < 0 {
-		edit.End = len(src) // extend to EOF
-	} else {
-		edit.End = end + nl + 1 // extend beyond \n
+	if end > 0 && src[end-1] != '\n' ||
+		edit.New != "" && edit.New[len(edit.New)-1] != '\n' {
+		if nl := strings.IndexByte(src[end:], '\n'); nl < 0 {
+			edit.End = len(src) // extend to EOF
+		} else {
+			edit.End = end + nl + 1 // extend beyond \n
+		}
 	}
 	edit.New += src[end:edit.End]
 

internal/diff/diff_test.go

@@ -95,17 +95,17 @@ func FuzzRoundTrip(f *testing.F) {
 func TestLineEdits(t *testing.T) {
 	for _, tc := range difftest.TestCases {
 		t.Run(tc.Name, func(t *testing.T) {
-			// if line edits not specified, it is the same as edits
-			edits := tc.LineEdits
-			if edits == nil {
-				edits = tc.Edits
+			want := tc.LineEdits
+			if want == nil {
+				want = tc.Edits // already line-aligned
 			}
 			got, err := diff.LineEdits(tc.In, tc.Edits)
 			if err != nil {
 				t.Fatalf("LineEdits: %v", err)
 			}
-			if !reflect.DeepEqual(got, edits) {
-				t.Errorf("LineEdits got\n%q, want\n%q\n%#v", got, edits, tc)
+			if !reflect.DeepEqual(got, want) {
+				t.Errorf("in=<<%s>>\nout=<<%s>>\nraw  edits=%s\nline edits=%s\nwant: %s",
+					tc.In, tc.Out, tc.Edits, got, want)
 			}
 			// make sure that applying the edits gives the expected result
 			fixed, err := diff.Apply(tc.In, got)

internal/diff/difftest/difftest.go

@@ -28,7 +28,7 @@ const (
 
 var TestCases = []struct {
 	Name, In, Out, Unified string
-	Edits, LineEdits       []diff.Edit
+	Edits, LineEdits       []diff.Edit // expectation (LineEdits=nil => already line-aligned)
 	NoDiff                 bool
 }{{
 	Name: "empty",
@@ -220,9 +220,9 @@ var TestCases = []struct {
 			{Start: 14, End: 14, New: "C\n"},
 		},
 		LineEdits: []diff.Edit{
-			{Start: 0, End: 6, New: "C\n"},
-			{Start: 6, End: 8, New: "B\nA\n"},
-			{Start: 10, End: 14, New: "A\n"},
+			{Start: 0, End: 4, New: ""},
+			{Start: 6, End: 6, New: "B\n"},
+			{Start: 10, End: 12, New: ""},
 			{Start: 14, End: 14, New: "C\n"},
 		},
 	}, {

internal/diff/unified.go

@@ -155,18 +155,9 @@ func toUnified(fromName, toName string, content string, edits []Edit) (unified,
 			last++
 		}
 		if edit.New != "" {
-			for i, content := range splitLines(edit.New) {
-				toLine++
-				// Merge identical Delete+Insert.
-				// This is an unwanted output of converting diffs to line diffs
-				// that is easiest to fix by postprocessing.
-				// e.g.  issue #59232: ("aaa\nccc\n", "aaa\nbbb\nccc")
-				// -> [Delete "aaa\n", Insert "aaa\n", Insert "bbb\n", ...].
-				if i == 0 && last > start && h.lines[len(h.lines)-1].content == content {
-					h.lines[len(h.lines)-1].kind = opEqual
-					continue
-				}
+			for _, content := range splitLines(edit.New) {
 				h.lines = append(h.lines, line{kind: opInsert, content: content})
+				toLine++
 			}
 		}
 	}