[bug] DiffviewFileHistory break in some special repository

NGPONG commented 1 year ago

Description

Hey guys, I found that there will be some errors when opening the DiffViewFilehistory panel from some special repository.

stacktrace

``` Error executing luv callback: ...e/pack/packer/start/diffview.nvim/lua/diffview/async.lua:179: The coroutine failed with this message: context: cur_thread=main co_thread= co_func=...art/diffview.nvim/lua/diffview/vcs/adapters/git/init.lua:852 ...art/diffview.nvim/lua/diffview/vcs/adapters/git/init.lua:1044: attempt to index local 'name' (a nil value) stack traceback: ...art/diffview.nvim/lua/diffview/vcs/adapters/git/init.lua:1044: in function 'parse_fh_data' ...art/diffview.nvim/lua/diffview/vcs/adapters/git/init.lua:1013: in function <...art/diffview.nvim/lua/diffview/vcs/adapters/git/init.lua:852> [C]: in function 'xpcall' ...e/pack/packer/start/diffview.nvim/lua/diffview/async.lua:354: in function <...e/pack/packer/start/diffview.nvim/lua/diffview/async.lua:351> stack traceback: [C]: in function 'error' ...e/pack/packer/start/diffview.nvim/lua/diffview/async.lua:362: in function <...e/pack/packer/start/diffview.nvim/lua/diffview/async.lua:351> stack traceback: [C]: in function 'error' ...e/pack/packer/start/diffview.nvim/lua/diffview/async.lua:179: in function 'raise' ...e/pack/packer/start/diffview.nvim/lua/diffview/async.lua:207: in function 'step' ...e/pack/packer/start/diffview.nvim/lua/diffview/async.lua:239: in function 'notify_all' ...e/pack/packer/start/diffview.nvim/lua/diffview/async.lua:214: in function 'step' ...e/pack/packer/start/diffview.nvim/lua/diffview/async.lua:392: in function 'temp' ...art/diffview.nvim/lua/diffview/vcs/adapters/git/init.lua:894: in function 'callback' ...art/diffview.nvim/lua/diffview/vcs/adapters/git/init.lua:462: in function 'listener' ...ite/pack/packer/start/diffview.nvim/lua/diffview/job.lua:195: in function <...ite/pack/packer/start/diffview.nvim/lua/diffview/job.lua:173> ```

After some deeper digging, I found some possible problems in the following code snippet

https://github.com/sindrets/diffview.nvim/blob/b3a763f8c7810b352226c95faa7d3ac9fb93b8d9/lua/diffview/job.lua#L170-L210

In this function we can clearly see that line_buffer is used to handle some text truncation(maybe because the single read exceeds the buffer limit?). But the problem is that line_buffer is defined as local, and the next time to call the line_reader() is still in a nil state, which may conflict with the original purpose of the logic.

So, I tried to do some small tests and try to fix this problem, which modified like this:

+ do
+ local line_buffer

function Job:line_reader(pipe, out, line_listeners, err, data)
-  local line_buffer

  -- balabala...

end
+ end

This modification solves my problem, and it work well in several large repositories (e.g. kernel). Here is a one question I have is that I'm not quite sure if I'm in the right direction to solve this problem, or maybe there's a better way to do it.

Expected behavior

No response

Actual behavior

Please see the trackback in description.

Steps to reproduce

I can only reproduce the problem in one repository, Please try

git clone https://github.com/NGPONG/code.git
nvim
:DiffviewFileHistory

Health check

Output of :checkhealth diffview

``` diffview: require("diffview.health").check() Checking plugin dependencies - OK nvim-web-devicons installed. Checking VCS tools - The plugin requires at least one of the supported VCS tools to be valid. - OK Git found. - OK Git is up-to-date. (2.34.1) - WARNING Configured hg_cmd is not executable: 'hg' ```

Log info

Relevant info from :DiffviewLog

``` ############################ ### PUT LOG CONTENT HERE ### ############################ ```

Neovim version

NVIM v0.10.0-dev
Build type: RelWithDebInfo
LuaJIT 2.1.0-beta3

Operating system and version

wsl2 ubuntu-22.04

Minimal config

require("diffview").setup({})

sindrets commented 1 year ago

Thank you so much for the detailed bug report and for debugging this!

On the topic of fixing this: simply declaring line_buffer in a new closure won't quite suffice, as then all jobs and pipes will share the same line buffer. I think the best fix here would be to replace line_reader() with this:

---@private
---@param pipe uv_pipe_t
---@param out string[]
---@param line_listeners? diffview.Job.OnOutCallback[]
function Job:new_line_reader(pipe, out, line_listeners)
  local line_buffer

  ---@param err? string
  ---@param data? string
  return function(err, data)
    --- ...
  end
end

And then update all the call sites of line_reader(). I believe it's just one place. Would you care to submit a PR?

NGPONG commented 1 year ago

Would you care to submit a PR?

Absolutely sure! It was a pleasure to work with you and I'll submit it later.

sindrets / diffview.nvim