Timeout on PR view due to logic bug(maybe?) in `utils.get_file_at_commit`

[alexjg]

Issue Description

I'm trying out octo.nvim to review this PR. Calling :Octo review resume (having already started a review and encountered this bug) opens the PR window fine with a split showing the diff and a quicklist of the other files in the diff. Selecting another file hangs for 5 seconds and then an error saying "Timeout fetching ".

I did some digging into this. The files are available locally (pull_request.local_{left | right} are not nil in FileEntry:fetch()) so the fetch logic spawns some Plenary jobs to get the local files via git show in utils.get_file_at_commit. The relevant code is:

local last_get_file_job = nil

function M.get_file_at_commit(path, commit, cb)
  if not Job then
    return
  end
  local job = Job:new {
    enable_recording = true,
    command = "git",
    args = { "show", string.format("%s:%s", commit, path) },
    on_exit = vim.schedule_wrap(function(j_self, _, _)
      local output = table.concat(j_self:result(), "\n")
      local stderr = table.concat(j_self:stderr_result(), "\n")
      cb(vim.split(output, "\n"), vim.split(stderr, "\n"))
    end),
  }
  if last_get_file_job ~= nil then
    last_get_file_job:and_then(job)
  else
    job:start()
  end
  last_get_file_job = job
end

If there is a local_last_get_file_job then we just set the new job as the on_exit_callback of the current job. I think the logic of this last_get_file_job may be wrong. I think if the last_get_file_job has already completed when we call and_then then the next job will never get executed (the on exit callback of the previous job was already executed). If I remove the local_last_get_file_job logic via this diff then the problem goes away:

diff --git a/lua/octo/utils.lua b/lua/octo/utils.lua
index 5acf374..23de717 100644
--- a/lua/octo/utils.lua
+++ b/lua/octo/utils.lua
@@ -205,12 +205,7 @@ function M.get_file_at_commit(path, commit, cb)
       cb(vim.split(output, "\n"), vim.split(stderr, "\n"))
     end),
   }
-  if last_get_file_job ~= nil then
-    last_get_file_job:and_then(job)
-  else
-    job:start()
-  end
-  last_get_file_job = job
+  job:start()
 end

 function M.in_pr_repo()

I haven't put in a PR for this because I don't know what the purpose of this logic is, is it to bound concurrency in some manner?

[pwntester]

Thanks! I recently changed the logic to load the PR changed files lazily which means that we are no longer going to try to run too many of these jobs at the same time, and therefore we can get rid of this flawed concurrency control.