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 |
|
/review |
Review of PR: Fix E816 malformed diffpatch output issueWhat changed: Path resolution logic in CorrectnessThe logic is sound. Edge case — empty/missing
|
|
Thank you for investigating this tricky issue! |



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