Merge pull request #617 from GuillaumeLagrange/review-from-anywhere

feat: allow entering review mode without being in an octo pr buffer

README.md

@@ -440,8 +440,7 @@ Octo search assignee:pwntester is:pr
 
 ## 📋 PR reviews
 
-- Open the PR (e.g. `Octo <PR url>` or `Octo pr list` or `Octo pr edit <PR number>`)
-- Start a review with `Octo review start` or resume a pending review with `Octo review resume`
+- Enter review mode for the current branch with `Octo review`. Alternatively open the PR (e.g. `Octo <PR url>` or `Octo pr list` or `Octo pr edit <PR number>`) then use `Octo review` in the PR buffer to enter review mode for a specific PR.
 - A new tab will show a panel with changed files and two windows showing the diff on any of them.
 - Change panel entries with `]q` and `[q` or by selecting an entry in the window
 - Add comments with `<leader>ca` or suggestions with `<leader>sa` on single or multiple visual-selected lines

lua/octo/gh/graphql.lua

@@ -1536,7 +1536,7 @@ query($endCursor: String) {
       closedAt
       updatedAt
       url
-      repository { nameWithOwner }
+      headRepository { nameWithOwner }
       files(first:100) {
         nodes {
           path

lua/octo/model/octo-buffer.lua

@@ -908,6 +908,8 @@ function OctoBuffer:get_pr()
   return PullRequest:new {
     bufnr = bufnr,
     repo = self.repo,
+    head_repo = self.node.headRepository.nameWithOwner,
+    head_ref_name = self.node.headRefName,
     number = self.number,
     id = self.node.id,
     left = Rev:new(self.node.baseRefOid),

lua/octo/model/pull-request.lua

@@ -5,6 +5,8 @@ local M = {}
 
 ---@class PullRequest
 ---@field repo string
+---@field head_repo string
+---@field head_ref_name string
 ---@field owner string
 ---@field name string
 ---@field number integer
@@ -23,8 +25,9 @@ PullRequest.__index = PullRequest
 ---@return PullRequest
 function PullRequest:new(opts)
   local this = {
-    -- TODO: rename to nwo
     repo = opts.repo,
+    head_repo = opts.head_repo,
+    head_ref_name = opts.head_ref_name,
     number = opts.number,
     owner = "",
     name = "",
@@ -57,7 +60,8 @@ end
 
 M.PullRequest = PullRequest
 
----Fetch the diff of the PR
+--- Fetch the diff of the PR
+--- @param pr PullRequest
 function PullRequest:get_diff(pr)
   local url = string.format("repos/%s/pulls/%d", pr.repo, pr.number)
   gh.run {

lua/octo/reviews/file-entry.lua

@@ -238,7 +238,7 @@ function FileEntry:load_buffers(left_winid, right_winid)
   for _, split in ipairs(splits) do
     if not split.bufid or not vim.api.nvim_buf_is_loaded(split.bufid) then
       local conf = config.values
-      local use_local = conf.use_local_fs and split.pos == "right" and utils.in_pr_branch(self.pull_request.bufnr)
+      local use_local = conf.use_local_fs and split.pos == "right" and utils.in_pr_branch(self.pull_request)
 
       -- create buffer
       split.bufid = M._create_buffer {

lua/octo/reviews/init.lua

@@ -152,7 +152,7 @@ function Review:initiate(opts)
   opts = opts or {}
   local pr = self.pull_request
   local conf = config.values
-  if conf.use_local_fs and not utils.in_pr_branch(pr.bufnr) then
+  if conf.use_local_fs and not utils.in_pr_branch(pr) then
     local choice = vim.fn.confirm("Currently not in PR branch, would you like to checkout?", "&Yes\n&No", 2)
     if choice == 1 then
       utils.checkout_pr_sync(pr.number)
@@ -520,41 +520,43 @@ function M.close(tabpage)
   end
 end
 
---- Get the pull request associated with current buffer
+--- Get the pull request associated with current buffer.
+--- Fall back to pull request associated with the current branch if not in an Octo buffer.
 --- @param cb function
-local function get_pr_from_buffer(cb)
+local function get_pr_from_buffer_or_current_branch(cb)
   local bufnr = vim.api.nvim_get_current_buf()
   local buffer = octo_buffers[bufnr]
+
   if not buffer then
-    utils.error "No Octo buffer found"
+    -- We are not in an octo buffer, try and fallback to the current branch's pr
+    utils.get_pull_request_for_current_branch(cb)
     return
   end
+
   local pull_request = buffer:get_pr()
   if pull_request then
     cb(pull_request)
   else
-    pull_request = utils.get_pull_request_for_current_branch(function(pr)
-      cb(pr)
-    end)
+    pull_request = utils.get_pull_request_for_current_branch(cb)
   end
 end
 
 function M.start_review()
-  get_pr_from_buffer(function(pull_request)
+  get_pr_from_buffer_or_current_branch(function(pull_request)
     local current_review = Review:new(pull_request)
     current_review:start()
   end)
 end
 
 function M.resume_review()
-  get_pr_from_buffer(function(pull_request)
+  get_pr_from_buffer_or_current_branch(function(pull_request)
     local current_review = Review:new(pull_request)
     current_review:resume()
   end)
 end
 
 function M.start_or_resume_review()
-  get_pr_from_buffer(function(pull_request)
+  get_pr_from_buffer_or_current_branch(function(pull_request)
     local current_review = Review:new(pull_request)
     current_review:start_or_resume()
   end)

lua/octo/utils.lua

@@ -254,10 +254,13 @@ function M.parse_git_remote()
   return remotes
 end
 
-function M.get_remote()
+--- Returns first host and repo information found in a list of remote values
+--- If no argument is provided, defaults to matching against config's default remote
+--- @param remote table | nil list of local remotes to match against
+function M.get_remote(remote)
   local conf = config.values
   local remotes = M.parse_git_remote()
-  for _, name in ipairs(conf.default_remote) do
+  for _, name in ipairs(remote or conf.default_remote) do
     if remotes[name] then
       return remotes[name]
     end
@@ -283,12 +286,12 @@ function M.get_all_remotes()
   return vim.tbl_values(M.parse_git_remote())
 end
 
-function M.get_remote_name()
-  return M.get_remote().repo
+function M.get_remote_name(remote)
+  return M.get_remote(remote).repo
 end
 
-function M.get_remote_host()
-  return M.get_remote().host
+function M.get_remote_host(remote)
+  return M.get_remote(remote).host
 end
 
 function M.commit_exists(commit, cb)
@@ -363,37 +366,22 @@ function M.in_pr_repo()
   end
 end
 
-function M.in_pr_branch(bufnr)
-  bufnr = bufnr or vim.api.nvim_get_current_buf()
-  local buffer = octo_buffers[bufnr]
-  if not buffer then
-    return
-  end
-  if not buffer:isPullRequest() then
-    --M.error("Not in Octo PR buffer")
-    return false
-  end
+--- Determines if we are locally are in a branch matching the pr head ref
+--- @param pr PullRequest
+--- @return boolean
+function M.in_pr_branch(pr)
+  local cmd = "git rev-parse --abbrev-ref --symbolic-full-name @{u}"
+  local local_branch_with_local_remote = vim.split(string.gsub(vim.fn.system(cmd), "%s+", ""), "/")
+  local local_remote = local_branch_with_local_remote[1]
+  local local_branch = local_branch_with_local_remote[2]
 
-  local cmd = "git rev-parse --abbrev-ref HEAD"
-  local local_branch = string.gsub(vim.fn.system(cmd), "%s+", "")
-  if local_branch == string.format("%s/%s", buffer.node.headRepoName, buffer.node.headRefName) then
-    -- for PRs submitted from master, local_branch will get something like other_repo/master
-    local_branch = vim.split(local_branch, "/")[2]
-  end
+  local local_repo = M.get_remote_name { local_remote }
 
-  local local_repo = M.get_remote_name()
-  if buffer.node.baseRepository.nameWithOwner == local_repo and buffer.node.headRefName == local_branch then
+  if local_repo == pr.head_repo and local_branch == pr.head_ref_name then
     return true
-  elseif buffer.node.baseRepository.nameWithOwner ~= local_repo then
-    --M.error(string.format("Not in PR repo. Expected %s, got %s", buffer.node.baseRepository.nameWithOwner, local_repo))
-    return false
-  elseif buffer.node.headRefName ~= local_branch then
-    -- TODO: suggest to checkout the branch
-    --M.error(string.format("Not in PR branch. Expected %s, got %s", buffer.node.headRefName, local_branch))
-    return false
-  else
-    return false
   end
+
+  return false
 end
 
 ---Checks out a PR b number
@@ -1249,8 +1237,8 @@ function M.get_pull_request_for_current_branch(cb)
         return
       end
       local pr = vim.fn.json_decode(out)
-      local owner
-      local name
+      local base_owner
+      local base_name
       if pr.number then
         if pr.isCrossRepository then
           -- Parsing the pr url is the only way to get the target repo owner if the pr is cross repo
@@ -1264,15 +1252,15 @@ function M.get_pull_request_for_current_branch(cb)
             return
           end
           local iter = url_suffix:gmatch "[^/]+/"
-          owner = vim.print(iter():sub(1, -2))
-          name = vim.print(iter():sub(1, -2))
+          base_owner = iter():sub(1, -2)
+          base_name = iter():sub(1, -2)
         else
-          owner = pr.headRepositoryOwner.login
-          name = pr.headRepository.name
+          base_owner = pr.headRepositoryOwner.login
+          base_name = pr.headRepository.name
         end
         local number = pr.number
         local id = pr.id
-        local query = graphql("pull_request_query", owner, name, number, _G.octo_pv2_fragment)
+        local query = graphql("pull_request_query", base_owner, base_name, number, _G.octo_pv2_fragment)
         gh.run {
           args = { "api", "graphql", "--paginate", "--jq", ".", "-f", string.format("query=%s", query) },
           cb = function(output, stderr)
@@ -1284,9 +1272,11 @@ function M.get_pull_request_for_current_branch(cb)
               local Rev = require("octo.reviews.rev").Rev
               local PullRequest = require("octo.model.pull-request").PullRequest
               local pull_request = PullRequest:new {
-                repo = owner .. "/" .. name,
+                repo = base_owner .. "/" .. base_name,
+                head_repo = obj.headRepository.nameWithOwner,
                 number = number,
                 id = id,
+                head_ref_name = obj.headRefName,
                 left = Rev:new(obj.baseRefOid),
                 right = Rev:new(obj.headRefOid),
                 files = obj.files.nodes,