Fix E816 malformed diffpatch output issue#287
Conversation
Mainly stemmed from incorrect filepath handling
|
Thanks for taking a crack at this! I'm a bit confused - why would this affect the |
|
I could be wrong but I think its due to this from the Note that {patchfile} should only contain a diff for one file, I think because its trying to diff with a file that doesn't exist, the error is being thrown. At least this is my guess for now. |
|
Ah I see, thanks for explaining! So basically OpenCode might send a filepath that's incorrect relative to how Neovim searches for the file to open it? And resolving it as an absolute path fixes that? Why do we sometimes make it relative to the home directory? I don't see when the absolute path would fail and need to fallback to that.
Ah thanks for figuring that out! It puzzled me. Does this change fix it too? |
|
Given we have the |
|
from what I remember the home directory branch was added because just making it absolute didn't completely solve. I might be misremembering but
doesn't solve it either. It has the same path issue because open code doesn't create paths wrt server.cwd.
Nope, it doesn't, just the malformed issue. I will spend sometime to see if I can find a better fix to the malformed issue and also fix the single line issue. Though the latter is a bit confusing in the sense that it opens the correct file, but the patch is not for that file? It doesn't make complete sense to me but I suspect it to be a similar file path issue. Sucks that diff patch doesn't have the greatest error handling but what can you do. |
|
for the e816 error, I think this is the fix for now. Opencode sometimes sends the full path and sometimes everything but home, so I dont think there is a better solution. I managed to figure out the bug with the single line thing, bad news is that there's nothing that can be done about it. So turns out that what opencode exposes in the diff, is a pretty printed version of the actual diff. It uses it just for display. It calls |
|
/review |
|
I've reviewed the changes. Here's my assessment: What the code does: Resolves Correctness: The logic is sound — Nit — silent failure when neither strategy works: If neither Style — extraction: The resolution logic could be pulled into a small helper function for readability, e.g.: local function resolve_filepath(filepath)
local abs = vim.fn.fnamemodify(filepath, ":p")
if vim.fn.filereadable(abs) == 1 then return vim.fn.fnameescape(abs) end
local home = vim.fs.normalize(vim.fs.joinpath(vim.env.HOME or "", filepath))
if vim.env.HOME and vim.fn.filereadable(home) == 1 then return vim.fn.fnameescape(home) end
return nil
endBut this is purely stylistic — the current code works fine and is only 8 lines. PR health: Good size (+14/-1), single file, focused on one issue. The discussion shows good collaboration between maintainer and contributor. The description could be stronger (clearer "why") but the conversation fills in the gaps. |
|
addressed the comments |
| elseif vim.env.HOME and vim.env.HOME ~= "" then | ||
| local home_filepath = vim.fs.normalize(vim.fs.joinpath(vim.env.HOME, filepath)) | ||
| if vim.fn.filereadable(home_filepath) == 1 then | ||
| filepath = home_filepath | ||
| end | ||
| else |
There was a problem hiding this comment.
It's unlikely, but I see a small bug here: the home_filepath readability check is in a nested if, so if it fails, we'll miss the outer else w/ the error notification.
Feel free to restructure how you like, but maybe after the absolute/home resolution attempts to update filepath, we put a standalone vim.fn.filereadable(filepath) check? Then no matter the code path, we check the final filepath before attempting to use it.
I think we also want an early return after the notification, so we don't get E816. And maybe vim.log.levels.ERROR.
| end, { buffer = true, desc = "Close opencode edit diff" }) | ||
| elseif event.type == "permission.replied" and current_edit_request_id == event.properties.requestID then | ||
| -- Entire edit was accepted or rejected, either in the plugin or TUI; close the diff | ||
| -- #NOTE: i don't think it works when accepting/rejecting from the TUI |
There was a problem hiding this comment.
This worked last I tested, has that changed? Feel free to open a new bug report if so, but let's keep this PR focused 🙂


Tasks
Description
Issue basically was because of some incorrect file path handling. The current solution does work, but might not be the best, so let me know. Below is what the logs captured
Testing
Related Issue(s)
Screenshots/Videos