Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/tools/internal/diff: non-minimal diff #59232

Closed
adonovan opened this issue Mar 24, 2023 · 2 comments
Closed

x/tools/internal/diff: non-minimal diff #59232

adonovan opened this issue Mar 24, 2023 · 2 comments
Assignees
@pjweinb
Labels
Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@adonovan
Copy link
Member

The x/tools diff algorithm reports correct but non-minimal diffs even for the trivial case where a new line is inserted in the middle of a two-line file. For example:

	got := "aaa\nccc\n"
	want := "aaa\nbbb\nccc\n"
	delta := diff.Unified("got", "want", got, want)

GNU diff -u:

--- want
+++ got
@@ -1,2 +1,3 @@
 aaa
+bbb
 ccc

diff.Unified:

--- got
+++ want
@@ -1,2 +1,3 @@
-aaa
+aaa
+bbb
 ccc

We should match the GNU diff behavior in cases this simple.
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Mar 24, 2023
@gopherbot gopherbot added this to the Unreleased milestone Mar 24, 2023
@gopherbot
Copy link

Change https://go.dev/cl/489695 mentions this issue: gopls/diff/unified: remove redundant information

@gopherbot
Copy link

Change https://go.dev/cl/499377 mentions this issue: internal/diff: fix LineEdits bug in slow path

gopherbot pushed a commit to golang/tools that referenced this issue Jun 1, 2023
@adonovan
internal/diff: fix LineEdits bug in slow path
0b4461b 
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 <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pjweinb pjweinb

Labels
Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@pjweinb @adonovan @gopherbot