Fix: Line Comment Refactor (#180)
This MR re-implements and fixes logic that better handles single-line comments. It does this by parsing the hunk headers and examining the actual changes they contain in order to form the Gitlab payload correctly. Addresses #128
This commit is contained in:
Harrison (Harry) Cramer
committed by
GitHub
GitHub
parent
baee20b279
commit
faf2a25dc4
@@ -112,7 +112,7 @@ end
|
||||
---@field file_name string
|
||||
---@field old_line integer | nil
|
||||
---@field new_line integer | nil
|
||||
---@field range_info ReviewerRangeInfo
|
||||
---@field range_info ReviewerRangeInfo|nil
|
||||
|
||||
---This function (settings.popup.perform_action) will send the comment to the Go server
|
||||
---@param text string comment text
|
||||
|
||||
243
lua/gitlab/reviewer/diffview.lua
Normal file → Executable file
243
lua/gitlab/reviewer/diffview.lua
Normal file → Executable file
@@ -126,106 +126,117 @@ M.get_location = function(range)
|
||||
return
|
||||
end
|
||||
|
||||
local bufnr = vim.api.nvim_get_current_buf()
|
||||
|
||||
-- If there's a range, use the start of the visual selection, not the current line
|
||||
local current_line = range and range.start_line or vim.api.nvim_win_get_cursor(0)[1]
|
||||
|
||||
-- check if we are in the diffview tab
|
||||
-- Check if we are in the diffview tab
|
||||
local tabnr = vim.api.nvim_get_current_tabpage()
|
||||
if tabnr ~= M.tabnr then
|
||||
u.notify("Line location can only be determined within reviewer window", vim.log.levels.ERROR)
|
||||
return
|
||||
end
|
||||
|
||||
-- check if we are in the diffview buffer
|
||||
-- Check if we are in the diffview buffer
|
||||
local view = diffview_lib.get_current_view()
|
||||
if view == nil then
|
||||
u.notify("Could not find Diffview view", vim.log.levels.ERROR)
|
||||
return
|
||||
end
|
||||
local layout = view.cur_layout
|
||||
local result = {}
|
||||
local type
|
||||
local is_new
|
||||
|
||||
if
|
||||
layout.a.file.bufnr == bufnr
|
||||
or (M.lines_are_same(view.cur_layout) and layout.b.file.bufnr == bufnr and range == nil)
|
||||
then
|
||||
result.file_name = layout.a.file.path
|
||||
result.old_line = current_line
|
||||
type = "old"
|
||||
is_new = false
|
||||
elseif layout.b.file.bufnr == bufnr then
|
||||
result.file_name = layout.b.file.path
|
||||
result.new_line = current_line
|
||||
type = "new"
|
||||
is_new = true
|
||||
else
|
||||
u.notify("Line location can only be determined within reviewer window")
|
||||
local layout = view.cur_layout
|
||||
|
||||
---@type ReviewerInfo
|
||||
local reviewer_info = {
|
||||
file_name = layout.a.file.path,
|
||||
new_line = nil,
|
||||
old_line = nil,
|
||||
range_info = nil,
|
||||
}
|
||||
|
||||
local a_win = u.get_window_id_by_buffer_id(layout.a.file.bufnr)
|
||||
local b_win = u.get_window_id_by_buffer_id(layout.b.file.bufnr)
|
||||
local current_win = vim.fn.win_getid()
|
||||
local is_current_sha = current_win == b_win
|
||||
|
||||
if a_win == nil or b_win == nil then
|
||||
u.notify("Error retrieving window IDs for current files", vim.log.levels.ERROR)
|
||||
return
|
||||
end
|
||||
|
||||
local hunks = u.parse_hunk_headers(result.file_name, state.INFO.target_branch)
|
||||
if hunks == nil then
|
||||
local current_file = M.get_current_file()
|
||||
if current_file == nil then
|
||||
u.notify("Error retrieving current file from Diffview", vim.log.levels.ERROR)
|
||||
return
|
||||
end
|
||||
|
||||
local a_linenr = vim.api.nvim_win_get_cursor(a_win)[1]
|
||||
local b_linenr = vim.api.nvim_win_get_cursor(b_win)[1]
|
||||
|
||||
local data = u.parse_hunk_headers(current_file, state.INFO.target_branch)
|
||||
|
||||
if data.hunks == nil then
|
||||
u.notify("Could not parse hunks", vim.log.levels.ERROR)
|
||||
return
|
||||
end
|
||||
|
||||
local current_line_info
|
||||
if is_new then
|
||||
current_line_info = u.get_lines_from_hunks(hunks, result.new_line, is_new)
|
||||
else
|
||||
current_line_info = u.get_lines_from_hunks(hunks, result.old_line, is_new)
|
||||
-- Will be different depending on focused window.
|
||||
local modification_type =
|
||||
M.get_modification_type(a_linenr, b_linenr, is_current_sha, data.hunks, data.all_diff_output)
|
||||
|
||||
if modification_type == "bad_file_unmodified" then
|
||||
u.notify("Comments on unmodified lines will be placed in the old file", vim.log.levels.WARN)
|
||||
end
|
||||
|
||||
-- If single line comment is outside of changed lines then we need to specify both new line and old line
|
||||
-- otherwise the API returns error.
|
||||
-- https://docs.gitlab.com/ee/api/discussions.html#create-a-new-thread-in-the-merge-request-diff
|
||||
if not current_line_info.in_hunk then
|
||||
result.old_line = current_line_info.old_line
|
||||
result.new_line = current_line_info.new_line
|
||||
end
|
||||
|
||||
-- If users leave single-line comments in the new buffer that should be in the old buffer, we can
|
||||
-- tell because the line will not have changed. Send the correct payload.
|
||||
if M.lines_are_same(view.cur_layout) and layout.b.file.bufnr == bufnr and range == nil then
|
||||
local a_win = u.get_win_from_buf(layout.a.file.bufnr)
|
||||
local a_cursor = vim.api.nvim_win_get_cursor(a_win)[1]
|
||||
result.old_line = a_cursor
|
||||
result.new_line = a_cursor
|
||||
type = "old"
|
||||
-- Comment on new line: Include only new_line in payload.
|
||||
if modification_type == "added" then
|
||||
reviewer_info.old_line = nil
|
||||
reviewer_info.new_line = b_linenr
|
||||
-- Comment on deleted line: Include only new_line in payload.
|
||||
elseif modification_type == "deleted" then
|
||||
reviewer_info.old_line = a_linenr
|
||||
reviewer_info.new_line = nil
|
||||
-- The line was not found in any hunks, only send the old line number
|
||||
elseif modification_type == "unmodified" or modification_type == "bad_file_unmodified" then
|
||||
reviewer_info.old_line = a_linenr
|
||||
reviewer_info.new_line = b_linenr
|
||||
end
|
||||
|
||||
if range == nil then
|
||||
return result
|
||||
return reviewer_info
|
||||
end
|
||||
|
||||
result.range_info = { start = {}, ["end"] = {} }
|
||||
-- If leaving a multi-line comment, we want to also add range_info to the payload.
|
||||
local is_new = reviewer_info.new_line ~= nil
|
||||
local current_line_info = is_new and u.get_lines_from_hunks(data.hunks, reviewer_info.new_line, is_new)
|
||||
or u.get_lines_from_hunks(data.hunks, reviewer_info.old_line, is_new)
|
||||
local type = is_new and "new" or "old"
|
||||
|
||||
---@type ReviewerRangeInfo
|
||||
local range_info = { start = {}, ["end"] = {} }
|
||||
|
||||
if current_line == range.start_line then
|
||||
result.range_info.start.old_line = current_line_info.old_line
|
||||
result.range_info.start.new_line = current_line_info.new_line
|
||||
result.range_info.start.type = type
|
||||
range_info.start.old_line = current_line_info.old_line
|
||||
range_info.start.new_line = current_line_info.new_line
|
||||
range_info.start.type = type
|
||||
else
|
||||
local start_line_info = u.get_lines_from_hunks(hunks, range.start_line, is_new)
|
||||
result.range_info.start.old_line = start_line_info.old_line
|
||||
result.range_info.start.new_line = start_line_info.new_line
|
||||
result.range_info.start.type = type
|
||||
local start_line_info = u.get_lines_from_hunks(data.hunks, range.start_line, is_new)
|
||||
range_info.start.old_line = start_line_info.old_line
|
||||
range_info.start.new_line = start_line_info.new_line
|
||||
range_info.start.type = type
|
||||
end
|
||||
|
||||
if current_line == range.end_line then
|
||||
result.range_info["end"].old_line = current_line_info.old_line
|
||||
result.range_info["end"].new_line = current_line_info.new_line
|
||||
result.range_info["end"].type = type
|
||||
range_info["end"].old_line = current_line_info.old_line
|
||||
range_info["end"].new_line = current_line_info.new_line
|
||||
range_info["end"].type = type
|
||||
else
|
||||
local end_line_info = u.get_lines_from_hunks(hunks, range.end_line, is_new)
|
||||
result.range_info["end"].old_line = end_line_info.old_line
|
||||
result.range_info["end"].new_line = end_line_info.new_line
|
||||
result.range_info["end"].type = type
|
||||
local end_line_info = u.get_lines_from_hunks(data.hunks, range.end_line, is_new)
|
||||
range_info["end"].old_line = end_line_info.old_line
|
||||
range_info["end"].new_line = end_line_info.new_line
|
||||
range_info["end"].type = type
|
||||
end
|
||||
|
||||
return result
|
||||
reviewer_info.range_info = range_info
|
||||
return reviewer_info
|
||||
end
|
||||
|
||||
---Return content between start_line and end_line
|
||||
@@ -236,12 +247,9 @@ M.get_lines = function(start_line, end_line)
|
||||
return vim.api.nvim_buf_get_lines(0, start_line - 1, end_line, false)
|
||||
end
|
||||
|
||||
---Checks whether the lines in the two buffers are the same
|
||||
---@return boolean
|
||||
M.lines_are_same = function(layout)
|
||||
local a_win = u.get_win_from_buf(layout.a.file.bufnr)
|
||||
local b_win = u.get_win_from_buf(layout.b.file.bufnr)
|
||||
local a_cursor = vim.api.nvim_win_get_cursor(a_win)[1]
|
||||
local b_cursor = vim.api.nvim_win_get_cursor(b_win)[1]
|
||||
M.lines_are_same = function(layout, a_cursor, b_cursor)
|
||||
local line_a = u.get_line_content(layout.a.file.bufnr, a_cursor)
|
||||
local line_b = u.get_line_content(layout.b.file.bufnr, b_cursor)
|
||||
return line_a == line_b
|
||||
@@ -324,4 +332,101 @@ M.set_callback_for_reviewer_leave = function(callback)
|
||||
})
|
||||
end
|
||||
|
||||
---Returns whether the comment is on a deleted line, added line, or unmodified line.
|
||||
---This is in order to build the payload for Gitlab correctly by setting the old line and new line.
|
||||
---@param a_linenr number
|
||||
---@param b_linenr number
|
||||
---@param is_current_sha boolean
|
||||
---@param hunks Hunk[] A list of hunks
|
||||
---@param all_diff_output table The raw diff output
|
||||
function M.get_modification_type(a_linenr, b_linenr, is_current_sha, hunks, all_diff_output)
|
||||
for _, hunk in ipairs(hunks) do
|
||||
local old_line_end = hunk.old_line + hunk.old_range
|
||||
local new_line_end = hunk.new_line + hunk.new_range
|
||||
|
||||
if is_current_sha then
|
||||
-- If it is a single line change and neither hunk has a range, then it's added
|
||||
if b_linenr >= hunk.new_line and b_linenr <= new_line_end then
|
||||
if hunk.new_range == 0 and hunk.old_range == 0 then
|
||||
return "added"
|
||||
end
|
||||
-- If leaving a comment on the new window, we may be commenting on an added line
|
||||
-- or on an unmodified line. To tell, we have to check whether the line itself is
|
||||
-- prefixed with "+" and only return "added" if it is.
|
||||
if M.line_was_added(b_linenr, hunk, all_diff_output) then
|
||||
return "added"
|
||||
end
|
||||
end
|
||||
else
|
||||
-- It's a deletion if it's in the range of the hunks and the new
|
||||
-- range is zero, since that is only a deletion hunk, or if we find
|
||||
-- a match in another hunk with a range, and the corresponding line is prefixed
|
||||
-- with a "-" only. If it is, then it's a deletion.
|
||||
if a_linenr >= hunk.old_line and a_linenr <= old_line_end and hunk.old_range == 0 then
|
||||
return "deleted"
|
||||
end
|
||||
if
|
||||
(a_linenr >= hunk.old_line and a_linenr <= old_line_end)
|
||||
or (a_linenr >= hunk.new_line and b_linenr <= new_line_end)
|
||||
then
|
||||
if M.line_was_removed(a_linenr, hunk, all_diff_output) then
|
||||
return "deleted"
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
-- If we can't find the line, this means the user is either trying to leave
|
||||
-- a comment on an unchanged line in the new or old file SHA. This is only
|
||||
-- allowed in the old file
|
||||
return is_current_sha and "bad_file_unmodified" or "unmodified"
|
||||
end
|
||||
|
||||
---@param linnr number
|
||||
---@param hunk Hunk
|
||||
---@param all_diff_output table
|
||||
M.line_was_removed = function(linnr, hunk, all_diff_output)
|
||||
for matching_line_index, line in ipairs(all_diff_output) do
|
||||
local found_hunk = u.parse_possible_hunk_headers(line)
|
||||
if found_hunk ~= nil and vim.deep_equal(found_hunk, hunk) then
|
||||
-- We found a matching hunk, now we need to iterate over the lines from the raw diff output
|
||||
-- at that hunk until we reach the line we are looking for. When the indexes match we check
|
||||
-- to see if that line is deleted or not.
|
||||
for hunk_line_index = found_hunk.old_line, hunk.old_line + hunk.old_range - 1, 1 do
|
||||
local line_content = all_diff_output[matching_line_index + 1]
|
||||
if hunk_line_index == linnr then
|
||||
if string.match(line_content, "^%-") then
|
||||
return "deleted"
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
---@param linnr number
|
||||
---@param hunk Hunk
|
||||
---@param all_diff_output table
|
||||
M.line_was_added = function(linnr, hunk, all_diff_output)
|
||||
for matching_line_index, line in ipairs(all_diff_output) do
|
||||
local found_hunk = u.parse_possible_hunk_headers(line)
|
||||
if found_hunk ~= nil and vim.deep_equal(found_hunk, hunk) then
|
||||
-- For added lines, we only want to iterate over the part of the diff that has has new lines,
|
||||
-- so we skip over the old range. We then keep track of the increment to the original new line index,
|
||||
-- and iterate until we reach the end of the total range of this hunk. If we arrive at the matching
|
||||
-- index for the line number, we check to see if the line was added.
|
||||
local i = 0
|
||||
local old_range = (found_hunk.old_range == 0 and found_hunk.old_line ~= 0) and 1 or found_hunk.old_range
|
||||
for hunk_line_index = matching_line_index + old_range + 1, matching_line_index + old_range + found_hunk.new_range, 1 do
|
||||
local line_content = all_diff_output[hunk_line_index]
|
||||
if (found_hunk.new_line + i) == linnr then
|
||||
if string.match(line_content, "^%+") then
|
||||
return "added"
|
||||
end
|
||||
end
|
||||
i = i + 1
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
return M
|
||||
|
||||
@@ -472,19 +472,28 @@ M.get_line_content = function(bufnr, start)
|
||||
return lines[1]
|
||||
end
|
||||
|
||||
M.get_win_from_buf = function(bufnr)
|
||||
for _, win in ipairs(vim.api.nvim_list_wins()) do
|
||||
if vim.fn.winbufnr(win) == bufnr then
|
||||
return win
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
M.switch_can_edit_buf = function(buf, bool)
|
||||
vim.api.nvim_set_option_value("modifiable", bool, { buf = buf })
|
||||
vim.api.nvim_set_option_value("readonly", not bool, { buf = buf })
|
||||
end
|
||||
|
||||
-- Gets the window holding a buffer in the current tab page
|
||||
---@param buffer_id number Id of a buffer
|
||||
---@return integer|nil
|
||||
M.get_window_id_by_buffer_id = function(buffer_id)
|
||||
local tabpage = vim.api.nvim_get_current_tabpage()
|
||||
local windows = vim.api.nvim_tabpage_list_wins(tabpage)
|
||||
|
||||
for _, win_id in ipairs(windows) do
|
||||
local buf_id = vim.api.nvim_win_get_buf(win_id)
|
||||
if buf_id == buffer_id then
|
||||
return win_id
|
||||
end
|
||||
end
|
||||
|
||||
return nil -- Buffer ID not found in any window
|
||||
end
|
||||
|
||||
M.list_files_in_folder = function(folder_path)
|
||||
if vim.fn.isdirectory(folder_path) == 0 then
|
||||
return nil
|
||||
@@ -524,12 +533,37 @@ end
|
||||
---@field new_line integer
|
||||
---@field new_range integer
|
||||
|
||||
---@class HunksAndDiff
|
||||
---@field hunks Hunk[] list of hunks
|
||||
---@field all_diff_output table The data from the git diff command
|
||||
|
||||
---Turn hunk line into Lua table
|
||||
---@param line table
|
||||
---@return Hunk|nil
|
||||
M.parse_possible_hunk_headers = function(line)
|
||||
if line:sub(1, 2) == "@@" then
|
||||
-- match:
|
||||
-- @@ -23 +23 @@ ...
|
||||
-- @@ -23,0 +23 @@ ...
|
||||
-- @@ -41,0 +42,4 @@ ...
|
||||
local old_start, old_range, new_start, new_range = line:match("@@+ %-(%d+),?(%d*) %+(%d+),?(%d*) @@+")
|
||||
|
||||
return {
|
||||
old_line = tonumber(old_start),
|
||||
old_range = tonumber(old_range) or 0,
|
||||
new_line = tonumber(new_start),
|
||||
new_range = tonumber(new_range) or 0,
|
||||
}
|
||||
end
|
||||
end
|
||||
|
||||
---Parse git diff hunks.
|
||||
---@param file_path string Path to file.
|
||||
---@param base_branch string Git base branch of merge request.
|
||||
---@return Hunk[] list of hunks.
|
||||
---@return HunksAndDiff
|
||||
M.parse_hunk_headers = function(file_path, base_branch)
|
||||
local hunks = {}
|
||||
local all_diff_output = {}
|
||||
|
||||
local Job = require("plenary.job")
|
||||
|
||||
@@ -538,20 +572,11 @@ M.parse_hunk_headers = function(file_path, base_branch)
|
||||
args = { "diff", "--minimal", "--unified=0", "--no-color", base_branch, "--", file_path },
|
||||
on_exit = function(j, return_code)
|
||||
if return_code == 0 then
|
||||
for _, line in ipairs(j:result()) do
|
||||
if line:sub(1, 2) == "@@" then
|
||||
-- match:
|
||||
-- @@ -23 +23 @@ ...
|
||||
-- @@ -23,0 +23 @@ ...
|
||||
-- @@ -41,0 +42,4 @@ ...
|
||||
local old_start, old_range, new_start, new_range = line:match("@@+ %-(%d+),?(%d*) %+(%d+),?(%d*) @@+")
|
||||
|
||||
table.insert(hunks, {
|
||||
old_line = tonumber(old_start),
|
||||
old_range = tonumber(old_range) or 0,
|
||||
new_line = tonumber(new_start),
|
||||
new_range = tonumber(new_range) or 0,
|
||||
})
|
||||
all_diff_output = j:result()
|
||||
for _, line in ipairs(all_diff_output) do
|
||||
local hunk = M.parse_possible_hunk_headers(line)
|
||||
if hunk ~= nil then
|
||||
table.insert(hunks, hunk)
|
||||
end
|
||||
end
|
||||
else
|
||||
@@ -562,7 +587,7 @@ M.parse_hunk_headers = function(file_path, base_branch)
|
||||
|
||||
diff_job:sync()
|
||||
|
||||
return hunks
|
||||
return { hunks = hunks, all_diff_output = all_diff_output }
|
||||
end
|
||||
|
||||
---@class LineDiffInfo
|
||||
|
||||
Reference in New Issue
Block a user