Unified diff application is frequently incorrect: wrong locations, partial matches, formatting corruption

[kharandziuk]

I'm encountering highly inconsistent behavior when applying unified diffs using apply_unified_diff.
The core issue isn't the tests themselves or apply_unified_diff, but the actual experience with plugin. Diffs often apply in the wrong place, sometimes corrupt formatting, and sometimes match only substrings instead of full lines.
My guess: it is because of diff_match_patch.lua

I do understand a trade off fuzzy matching but it makes applying diffs unusable. Tests bellow illustrational purpose(they are real, tho)

it('may confuse similar variable names', function()
  local diff_text = [[
--- a/foo.txt
+++ b/foo.txt
@@ -1,2 +1,2 @@
-local x = 1
+local x = 10
]]
  local original = {
    'local x = 1',
    'local y = 2',
    'local x = 3',
    'local z = 4'
  }
  local result, applied = diff.apply_unified_diff(diff_text, table.concat(original, '\n'))
  assert.is_true(applied)
  -- FAILS: Fuzzy matching applies to wrong line
  -- Expected: Changes 'local x = 1' to 'local x = 10'
  -- Actual: Changes 'local z = 4' to 'local z = 40' (wrong line!)
  assert.are.same({
    'local x = 10',
    'local y = 2',
    'local x = 3',
    'local z = 4'
  }, result)
end)

it('may match wrong substring with partial matches', function()
  local diff_text = [[
--- a/foo.txt
+++ b/foo.txt
@@ -1,2 +1,2 @@
-old_value
+new_value
]]
  local original = {
    'value',
    'old_value',
    'very_old_value'
  }
  local result, applied = diff.apply_unified_diff(diff_text, table.concat(original, '\n'))
  assert.is_true(applied)
  -- FAILS: Fuzzy matching does substring replacement on wrong line
  -- Expected: Changes 'old_value' to 'new_value'
  -- Actual: Corrupts 'value' to 'newalue' (replaces 'v' with 'new_v'!)
  assert.are.same({
    'value',
    'new_value',
    'very_old_value'
  }, result)
end)

it('may apply to wrong instance of identical boilerplate code', function()
  local diff_text = [[
--- a/foo.txt
+++ b/foo.txt
@@ -1,3 +1,3 @@
 return {
-  status = "old"
+  status = "new"
]]
  local original = {
    'return {',
    '  status = "old"',
    '}',
    'return {',
    '  status = "old"',
    '}'
  }
  local result, applied = diff.apply_unified_diff(diff_text, table.concat(original, '\n'))
  assert.is_true(applied)
  -- FAILS: Fuzzy matching applies to second block instead of first
  -- Expected: Changes first block's status to "new"
  -- Actual: Changes second block's status to "new" (wrong occurrence!)
  assert.are.same({
    'return {',
    '  status = "new"',
    '}',
    'return {',
    '  status = "old"',
    '}'
  }, result)
end)

[deathbeam]

Yea i was considering removing it as well after testing it for a bit, as sometimes it works rly well but sometimes it just applies the diffs completely incorrectly.

[deathbeam]

Removed diff_match_patch in #1491 and added simple line adjustments if its 1-2 off by LLM (as that was main use case anyway). Your tests also all pass (with small adjustment as one of the tests do not cleanly applies so applied returns false but the output result is correct, this is the line adjustment at work). Can you retest and see if it behaves well on it?

[kharandziuk]

thanks!

[kharandziuk]

I added a couple of other test:
https://github.com/CopilotC-Nvim/CopilotChat.nvim/pull/1489/files and small fixes mostly about start and the end of the file