diff --git a/README.md b/README.md index ca64337f..7921f023 100644 --- a/README.md +++ b/README.md @@ -91,7 +91,7 @@ And with pckr.nvim: ### Notes on dependencies -`gitlab.nvim` uses the `diffview.nvim` plugin for showing the diffs in a MR. We recommend using `dlyongemallo`'s [diffview+](https://github.com/dlyongemallo/diffview-plus.nvim) fork which is the de-facto maintained version of the plugin with many fixes and improvements (e.g., marking files as viewed). The original [sindrets/diffview.nvim](https://github.com/sindrets/diffview.nvim) plugin will be supported by `gitlab.nvim` as long as the maintenance remains feasible. +`gitlab.nvim` uses the `diffview.nvim` plugin for showing the diffs in a MR. We recommend using `dlyongemallo`'s [diffview+](https://github.com/dlyongemallo/diffview-plus.nvim) fork which is an actively maintained version of the plugin with many fixes and improvements (e.g., marking files as viewed). Importantly, it allows setting the same similarity threshold for detecting renamed files as is used by Gitlab (30%). When using the original [sindrets/diffview.nvim](https://github.com/sindrets/diffview.nvim) plugin, file renames may not be detected correctly and comments created on such files will contain incorrect metadata or may fail. Nevertheless, the original `sindrets/diffview.nvim` plugin will be supported by `gitlab.nvim` as long as the maintenance remains feasible. Some plugin actions use Neovim’s `vim.ui.select()` picker, which looks much nicer if you use `dressing.nvim` or a similar UI plugin. To use Dressing with `gitlab.nvim`, enable it for `vim.ui.select()` like this: ```lua diff --git a/cmd/app/comment_helpers.go b/cmd/app/comment_helpers.go index 05881da2..9cc189cd 100644 --- a/cmd/app/comment_helpers.go +++ b/cmd/app/comment_helpers.go @@ -7,8 +7,18 @@ import ( gitlab "gitlab.com/gitlab-org/api/client-go" ) -/* LinePosition represents a position in a line range. Unlike the Gitlab struct, this does not contain LineCode with a sha1 of the filename */ -type LinePosition struct { +/* PositionInfo represents one endpoint (start or end) of a line range, as sent by the Lua +* plugin. Unlike the Gitlab struct, it has no LineCode - Lua can't compute a sha1, so +* buildCommentPosition computes one below from OldLine and NewLine. +* +* OldLine and NewLine are always real, non-nil integers, even when Type is "old" or "new" +* and only one side actually has a line. On the side that doesn't, the value is a position +* marker, not a claim that a line exists there: it's wherever that side's cursor was +* sitting when the other side's line was found. LineCode is always built from this +* unzeroed pair; buildCommentPosition separately zeroes the inapplicable side before +* setting it on the request's LineRange.{Start,End}.{OldLine,NewLine} - see +* zeroInapplicableLine. */ +type PositionInfo struct { Type string `json:"type"` OldLine int64 `json:"old_line"` NewLine int64 `json:"new_line"` @@ -16,8 +26,8 @@ type LinePosition struct { /* LineRange represents the range of a note. */ type LineRange struct { - StartRange *LinePosition `json:"start"` - EndRange *LinePosition `json:"end"` + Start *PositionInfo `json:"start" validate:"required"` + End *PositionInfo `json:"end" validate:"required"` } /* PositionData represents the position of a comment or note (relative to a file diff) */ @@ -30,7 +40,7 @@ type PositionData struct { BaseCommitSHA string `json:"base_commit_sha"` StartCommitSHA string `json:"start_commit_sha"` Type string `json:"type"` - LineRange *LineRange `json:"line_range,omitempty"` + LineRange *LineRange `json:"line_range" validate:"required_with=FileName"` } /* RequestWithPosition is an interface that abstracts the handling of position data for a comment or a draft comment */ @@ -42,48 +52,65 @@ type RequestWithPosition interface { func buildCommentPosition(commentWithPositionData RequestWithPosition) *gitlab.PositionOptions { positionData := commentWithPositionData.GetPositionData() - // If the file has been renamed, then this is a relevant part of the payload - oldFileName := positionData.OldFileName - if oldFileName == "" { - oldFileName = positionData.FileName - } - opt := &gitlab.PositionOptions{ PositionType: &positionData.Type, StartSHA: &positionData.StartCommitSHA, HeadSHA: &positionData.HeadCommitSHA, BaseSHA: &positionData.BaseCommitSHA, NewPath: &positionData.FileName, - OldPath: &oldFileName, + OldPath: &positionData.OldFileName, NewLine: positionData.NewLine, OldLine: positionData.OldLine, } - if positionData.LineRange != nil { - shaFormat := "%x_%d_%d" - startFilenameSha := fmt.Sprintf( - shaFormat, - sha1.Sum([]byte(positionData.FileName)), - positionData.LineRange.StartRange.OldLine, - positionData.LineRange.StartRange.NewLine, - ) - endFilenameSha := fmt.Sprintf( - shaFormat, - sha1.Sum([]byte(positionData.FileName)), - positionData.LineRange.EndRange.OldLine, - positionData.LineRange.EndRange.NewLine, - ) - opt.LineRange = &gitlab.LineRangeOptions{ - Start: &gitlab.LinePositionOptions{ - Type: &positionData.LineRange.StartRange.Type, - LineCode: &startFilenameSha, - }, - End: &gitlab.LinePositionOptions{ - Type: &positionData.LineRange.EndRange.Type, - LineCode: &endFilenameSha, - }, - } + shaFormat := "%x_%d_%d" + startFilenameSha := fmt.Sprintf( + shaFormat, + sha1.Sum([]byte(positionData.FileName)), + positionData.LineRange.Start.OldLine, + positionData.LineRange.Start.NewLine, + ) + endFilenameSha := fmt.Sprintf( + shaFormat, + sha1.Sum([]byte(positionData.FileName)), + positionData.LineRange.End.OldLine, + positionData.LineRange.End.NewLine, + ) + + startOldLine, startNewLine := zeroInapplicableLine(positionData.LineRange.Start) + endOldLine, endNewLine := zeroInapplicableLine(positionData.LineRange.End) + + opt.LineRange = &gitlab.LineRangeOptions{ + Start: &gitlab.LinePositionOptions{ + Type: &positionData.LineRange.Start.Type, + LineCode: &startFilenameSha, + OldLine: &startOldLine, + NewLine: &startNewLine, + }, + End: &gitlab.LinePositionOptions{ + Type: &positionData.LineRange.End.Type, + LineCode: &endFilenameSha, + OldLine: &endOldLine, + NewLine: &endNewLine, + }, } return opt } + +/* zeroInapplicableLine returns a line_range endpoint's OldLine/NewLine with the side +* that its Type doesn't apply to zeroed out: NewLine for a deleted ("old") line, OldLine +* for an added ("new") line. Both stay real for an unmodified ("") or "expanded" line. +* The unzeroed pair is still what the LineCode hash above is computed from - Gitlab +* expects LineCode to encode the real old/new correspondence even when the displayed +* OldLine or NewLine is zeroed. */ +func zeroInapplicableLine(position *PositionInfo) (oldLine int64, newLine int64) { + oldLine, newLine = position.OldLine, position.NewLine + switch position.Type { + case "old": + newLine = 0 + case "new": + oldLine = 0 + } + return oldLine, newLine +} diff --git a/cmd/app/comment_helpers_test.go b/cmd/app/comment_helpers_test.go new file mode 100644 index 00000000..50f0cefe --- /dev/null +++ b/cmd/app/comment_helpers_test.go @@ -0,0 +1,65 @@ +package app + +import ( + "testing" +) + +func TestBuildCommentPosition(t *testing.T) { + makePositionData := func(startType string, startOld, startNew int64, endType string, endOld, endNew int64) PositionData { + return PositionData{ + FileName: "file.txt", + HeadCommitSHA: "head-sha", + BaseCommitSHA: "base-sha", + StartCommitSHA: "start-sha", + Type: "text", + LineRange: &LineRange{ + Start: &PositionInfo{Type: startType, OldLine: startOld, NewLine: startNew}, + End: &PositionInfo{Type: endType, OldLine: endOld, NewLine: endNew}, + }, + } + } + + t.Run("zeroes NewLine for a deleted (\"old\") line, keeping LineCode's real pair", func(t *testing.T) { + positionData := makePositionData("", 4, 4, "old", 5, 5) + opt := buildCommentPosition(CommentWithPosition{PositionData: positionData}) + + assert(t, *opt.LineRange.End.OldLine, int64(5)) + assert(t, *opt.LineRange.End.NewLine, int64(0)) + assert(t, *opt.LineRange.End.LineCode, "5436437fa01a7d3e41d46741da54b451446774ca_5_5") + }) + + t.Run("zeroes OldLine for an added (\"new\") line, keeping LineCode's real pair", func(t *testing.T) { + positionData := makePositionData("", 4, 4, "new", 5, 5) + opt := buildCommentPosition(CommentWithPosition{PositionData: positionData}) + + assert(t, *opt.LineRange.End.OldLine, int64(0)) + assert(t, *opt.LineRange.End.NewLine, int64(5)) + assert(t, *opt.LineRange.End.LineCode, "5436437fa01a7d3e41d46741da54b451446774ca_5_5") + }) + + t.Run("keeps both lines real for an unmodified (\"\") line", func(t *testing.T) { + positionData := makePositionData("", 4, 4, "", 5, 6) + opt := buildCommentPosition(CommentWithPosition{PositionData: positionData}) + + assert(t, *opt.LineRange.End.OldLine, int64(5)) + assert(t, *opt.LineRange.End.NewLine, int64(6)) + }) + + t.Run("keeps both lines real for an expanded line", func(t *testing.T) { + positionData := makePositionData("", 4, 4, "expanded", 59, 61) + opt := buildCommentPosition(CommentWithPosition{PositionData: positionData}) + + assert(t, *opt.LineRange.End.OldLine, int64(59)) + assert(t, *opt.LineRange.End.NewLine, int64(61)) + }) + + t.Run("zeroes the start and end independently", func(t *testing.T) { + positionData := makePositionData("new", 0, 50, "", 60, 62) + opt := buildCommentPosition(CommentWithPosition{PositionData: positionData}) + + assert(t, *opt.LineRange.Start.OldLine, int64(0)) + assert(t, *opt.LineRange.Start.NewLine, int64(50)) + assert(t, *opt.LineRange.End.OldLine, int64(60)) + assert(t, *opt.LineRange.End.NewLine, int64(62)) + }) +} diff --git a/cmd/app/comment_test.go b/cmd/app/comment_test.go index a10ef5fc..947eb359 100644 --- a/cmd/app/comment_test.go +++ b/cmd/app/comment_test.go @@ -59,6 +59,10 @@ func TestPostComment(t *testing.T) { Comment: "Some comment", PositionData: PositionData{ FileName: "file.txt", + LineRange: &LineRange{ + Start: &PositionInfo{Type: "", OldLine: 4, NewLine: 4}, + End: &PositionInfo{Type: "", OldLine: 4, NewLine: 4}, + }, }, } request := makeRequest(t, http.MethodPost, "/mr/comment", testCommentCreationData) diff --git a/cmd/app/middleware_test.go b/cmd/app/middleware_test.go index 598c3d59..853f8afe 100644 --- a/cmd/app/middleware_test.go +++ b/cmd/app/middleware_test.go @@ -111,4 +111,59 @@ func TestValidatorMiddleware(t *testing.T) { ), request) assert(t, data.Message, "Some message") }) + t.Run("Should reject a line_range with a missing endpoint instead of panicking", func(t *testing.T) { + payload := PostCommentRequest{ + Comment: "Some comment", + PositionData: PositionData{ + FileName: "file.txt", + LineRange: &LineRange{}, // Start and End left nil + }, + } + request := makeRequest(t, http.MethodPost, "/mr/comment", payload) + svc := middleware( + commentService{testProjectData, fakeCommentClient{}}, + withMr(testProjectData, fakeMergeRequestLister{}), + withPayloadValidation(methodToPayload{http.MethodPost: newPayload[PostCommentRequest]}), + withMethodCheck(http.MethodPost), + ) + data, status := getFailData(t, svc, request) + assert(t, data.Message, "Invalid payload") + assert(t, data.Details, "Start is required; End is required") + assert(t, status, http.StatusBadRequest) + }) + t.Run("Should reject a missing line_range when FileName is set", func(t *testing.T) { + payload := PostCommentRequest{ + Comment: "Some comment", + PositionData: PositionData{ + FileName: "file.txt", + // LineRange left nil entirely (not just an empty struct). + }, + } + request := makeRequest(t, http.MethodPost, "/mr/comment", payload) + svc := middleware( + commentService{testProjectData, fakeCommentClient{}}, + withMr(testProjectData, fakeMergeRequestLister{}), + withPayloadValidation(methodToPayload{http.MethodPost: newPayload[PostCommentRequest]}), + withMethodCheck(http.MethodPost), + ) + data, status := getFailData(t, svc, request) + assert(t, data.Message, "Invalid payload") + assert(t, data.Details, "The field 'LineRange' failed on validation on the 'required_with' tag") + assert(t, status, http.StatusBadRequest) + }) + t.Run("Should allow a missing line_range when there is no FileName (unlinked comment)", func(t *testing.T) { + payload := PostCommentRequest{ + Comment: "Some comment", + // PositionData is left zero-valued: no FileName, no LineRange. + } + request := makeRequest(t, http.MethodPost, "/mr/comment", payload) + svc := middleware( + commentService{testProjectData, fakeCommentClient{}}, + withMr(testProjectData, fakeMergeRequestLister{}), + withPayloadValidation(methodToPayload{http.MethodPost: newPayload[PostCommentRequest]}), + withMethodCheck(http.MethodPost), + ) + data := getSuccessData(t, svc, request) + assert(t, data.Message, "Comment created successfully") + }) } diff --git a/doc/gitlab.nvim.txt b/doc/gitlab.nvim.txt index 1868e5df..f5d1c9ec 100644 --- a/doc/gitlab.nvim.txt +++ b/doc/gitlab.nvim.txt @@ -127,10 +127,16 @@ And with pckr.nvim: NOTES ON DEPENDENCIES *gitlab.nvim.dependencies* `gitlab.nvim` uses the `diffview.nvim` plugin for showing the diffs in a MR. -We recommend using `dlyongemallo`'s `diffview+` fork which is the de-facto +We recommend using `dlyongemallo`'s `diffview+` fork which is an actively maintained version of the plugin with many fixes and improvements (e.g., -marking files as viewed). The original `sindrets/diffview.nvim` plugin will be -supported by `gitlab.nvim` as long as the maintenance remains feasible. +marking files as viewed). Importantly, it allows setting the same similarity +threshold for detecting renamed files as is used by Gitlab (30%). When using +the original +[sindrets/diffview.nvim](https://github.com/sindrets/diffview.nvim) plugin, +file renames may not be detected correctly and comments created on such files +will contain incorrect metadata or may fail. Nevertheless the original +`sindrets/diffview.nvim` plugin will be supported by `gitlab.nvim` as long as +the maintenance remains feasible. Some plugin actions use Neovim’s |vim.ui.select()| picker, which looks much nicer if you use `dressing.nvim` or a similar UI plugin. To use Dressing, diff --git a/lua/gitlab/actions/comment.lua b/lua/gitlab/actions/comment.lua index 862521af..dc3a1e3b 100644 --- a/lua/gitlab/actions/comment.lua +++ b/lua/gitlab/actions/comment.lua @@ -8,6 +8,7 @@ local job = require("gitlab.job") local u = require("gitlab.utils") local popup = require("gitlab.popup") local git = require("gitlab.git") +local hunks = require("gitlab.hunks") local discussions = require("gitlab.actions.discussions") local draft_notes = require("gitlab.actions.draft_notes") local miscellaneous = require("gitlab.actions.miscellaneous") @@ -21,6 +22,19 @@ local M = { comment_popup = nil, } +---Build a Location from the live reviewer state (the active Diffview session). +---Return nil when the location cannot be built due to missing reviewer data. +---@return Location? +M.new_location_from_reviewer = function() + local reviewer_data = reviewer.get_reviewer_data() + if reviewer_data == nil then + return nil + end + local diff_hunks = + hunks.get_hunks(reviewer_data.old_sha, reviewer_data.new_sha, reviewer_data.old_file_name, reviewer_data.file_name) + return Location.new(reviewer_data, diff_hunks) +end + ---Fire the API to send the comment data to the Go server. ---@param text string comment text ---@param unlinked boolean if true, the comment is not linked to a line @@ -151,11 +165,11 @@ M.create_comment_layout = function(opts) title = "Note" user_settings = popup_settings.note else - local file_name = (M.location.reviewer_data.new_sha_focused or M.location.reviewer_data.old_file_name == "") + local file_name = (M.location.reviewer_data.new_file_focused or M.location.reviewer_data.old_file_name == "") and M.location.reviewer_data.file_name or M.location.reviewer_data.old_file_name title = - popup.create_title("Comment", file_name, M.location.visual_range.start_line, M.location.visual_range.end_line) + popup.create_title("Comment", file_name, M.location.reviewer_data.start_line, M.location.reviewer_data.end_line) user_settings = popup_settings.comment end local settings = u.merge(popup_settings, user_settings or {}) @@ -207,7 +221,7 @@ end ---Open a comment popup in order to create a comment on the changed/updated line in the ---current MR. M.create_comment = function() - M.location = Location.new() + M.location = M.new_location_from_reviewer() if not M.can_create_comment(false) then return end @@ -219,7 +233,7 @@ end ---Open a multi-line comment popup in order to create a multi-line comment on the ---changed/updated line in the current MR. M.create_multiline_comment = function() - M.location = Location.new() + M.location = M.new_location_from_reviewer() if not M.can_create_comment(true) then u.press_escape() return @@ -238,12 +252,12 @@ end ---Given the current visually selected area of text, builds text to fill in the ---comment popup with a suggested change ----@return LineRange? +---@return string[]? local build_suggestion = function() local current_line = vim.api.nvim_win_get_cursor(0)[1] - local range_length = M.location.visual_range.end_line - M.location.visual_range.start_line + local range_length = M.location.reviewer_data.end_line - M.location.reviewer_data.start_line local backticks = "```" - local selected_lines = u.get_lines(M.location.visual_range.start_line, M.location.visual_range.end_line) + local selected_lines = u.get_lines(M.location.reviewer_data.start_line, M.location.reviewer_data.end_line) for _, line in ipairs(selected_lines) do if string.match(line, "^```%S*$") then @@ -253,9 +267,9 @@ local build_suggestion = function() end local suggestion_start - if M.location.visual_range.start_line == current_line then + if M.location.reviewer_data.start_line == current_line then suggestion_start = backticks .. "suggestion:-0+" .. range_length - elseif M.location.visual_range.end_line == current_line then + elseif M.location.reviewer_data.end_line == current_line then suggestion_start = backticks .. "suggestion:-" .. range_length .. "+0" else --- This should never happen afaik @@ -274,7 +288,7 @@ end ---Open a popup to create a suggestion comment on the changed/updated line in the current MR ---See: https://docs.gitlab.com/ee/user/project/merge_requests/reviews/suggestions.html M.create_comment_suggestion = function() - M.location = Location.new() + M.location = M.new_location_from_reviewer() if not M.can_create_comment(true) then u.press_escape() return @@ -355,7 +369,7 @@ M.can_create_comment = function(must_be_visual) return false end - if M.location == nil or M.location.location_data == nil then + if M.location == nil then u.notify("Error getting location information", vim.log.levels.ERROR) return false end diff --git a/lua/gitlab/actions/common.lua b/lua/gitlab/actions/common.lua index ff346332..03ca63a0 100644 --- a/lua/gitlab/actions/common.lua +++ b/lua/gitlab/actions/common.lua @@ -267,6 +267,7 @@ M.get_line_numbers_for_range = function(old_line, new_line, start_line_code, end elseif new_line ~= nil then local range = new_end_line - new_start_line -- Force start_line to be greater than 0 + -- TODO: use `math.max(new_line - range, 1)` instead local start_line = (new_line - range > 0) and (new_line - range) or 1 return start_line, new_line, true else diff --git a/lua/gitlab/actions/discussions/init.lua b/lua/gitlab/actions/discussions/init.lua index c3600f7d..81e23247 100644 --- a/lua/gitlab/actions/discussions/init.lua +++ b/lua/gitlab/actions/discussions/init.lua @@ -25,9 +25,9 @@ local emoji = require("gitlab.emoji") local M = { split_visible = false, split = nil, - ---@type number + ---@type integer linked_bufnr = nil, - ---@type number + ---@type integer unlinked_bufnr = nil, ---@type NuiTree? discussion_tree = nil, @@ -36,7 +36,7 @@ local M = { } ---Delete discussion buffers to prevent leaked buffers on each M.open/M.close cycle. ----@param split_bufnr number? Passed in because `unmount` has already nil'd `M.split.bufnr`. +---@param split_bufnr integer? Passed in because `unmount` has already nil'd `M.split.bufnr`. local function delete_bufs(split_bufnr) -- pairs, because any of these might be nil for _, bufnr in pairs({ split_bufnr, M.linked_bufnr, M.unlinked_bufnr }) do diff --git a/lua/gitlab/annotations.lua b/lua/gitlab/annotations.lua index 7aee76cf..7d66129c 100644 --- a/lua/gitlab/annotations.lua +++ b/lua/gitlab/annotations.lua @@ -11,13 +11,22 @@ ---@field avatar_url string ---@field web_url string ----@class LinePosition ----@field line_code string ----@field type string +---The modification of a line in a diff. +---@alias ModificationType +---| "old" A deleted line +---| "new" An added line +---| "" An unmodified line adjacent to a hunk +---| "expanded" A line more than 3 lines away from any change (i.e. one that Gitlab only shows upon manually expanding the diff) + +---@class PositionInfo +---@field line_code? string SHA of the file name with old and new line numbers, e.g., 3f454a98e586d1aa0d322e19afd5e67e08f2d3c8_1_1 +---@field old_line integer The corresponding line in the old version of the file. For added lines it is the line after the last common unchanged line +---@field new_line integer The corresponding line in the new version of the file. For deleted lines it is the line after the last common unchanged line +---@field type ModificationType ----@class GitlabLineRange ----@field start LinePosition ----@field end LinePosition +---@class LineRange +---@field start PositionInfo +---@field end PositionInfo ---@class NotePosition ---@field base_sha string @@ -28,7 +37,7 @@ ---@field new_line integer? ---@field old_path string? ---@field old_line integer? ----@field line_range GitlabLineRange? +---@field line_range LineRange? ---@class Note ---@field id integer @@ -98,21 +107,14 @@ ---@field lnum number ---@field buffer number? ----@class LineRange +---@class ReviewerData +---@field old_file_name string +---@field file_name string +---@field old_sha string +---@field new_sha string ---@field start_line integer ---@field end_line integer - ----@class DiffviewInfo ----@field modification_type string ----@field file_name string ----Relevant for renamed files only, the name of the file in the previous commit ----@field old_file_name string ----@field current_bufnr integer ----@field opposite_bufnr integer ----@field new_line_from_buf integer ----@field old_line_from_buf integer ----@field new_sha_focused boolean ----@field current_win_id integer +---@field new_file_focused boolean ---@class DraftNote ---@field note string diff --git a/lua/gitlab/git.lua b/lua/gitlab/git.lua index d3a5bb42..a37e8073 100644 --- a/lua/gitlab/git.lua +++ b/lua/gitlab/git.lua @@ -8,7 +8,7 @@ local M = {} ---@return string? result The result of the command as a string. Nil if the command failed ---@return string? error The error the command failed with. Nil if the command succeeded local run_system = function(command) - local result = vim.fn.trim(vim.fn.system(command)) + local result = vim.fn.trim(vim.fn.system(command), "\r\n") if vim.v.shell_error ~= 0 then require("gitlab.utils").notify(result, vim.log.levels.ERROR) return nil, result @@ -241,21 +241,27 @@ M.check_mr_in_good_condition = function() end end ----Return the full diff between the local working tree relative to the named `base_sha`, ----for the given file(s). ----@param base_sha string Base SHA to diff against +---Return the diff between two commits for the given file(s). +---Diffs the two commit trees directly rather than against the working tree, so it's +---correct regardless of what's currently checked out. +---@param old_sha string SHA to diff from +---@param new_sha string SHA to diff to ---@param old_path? string Old file name ---@param new_path? string New file name - relevant for renamed files, ignored if same as old_path ---@return string? diff, string? err -M.diff_files = function(base_sha, old_path, new_path) +M.diff_files = function(old_sha, new_sha, old_path, new_path) return run_system({ "git", + "-c", + "diff.suppressBlankEmpty=false", "diff", "--minimal", - "--unified=0", + "--find-renames=30%", + "--unified=3", "--no-color", "--no-ext-diff", - base_sha, + old_sha, + new_sha, "--", old_path, new_path, diff --git a/lua/gitlab/hunks.lua b/lua/gitlab/hunks.lua index 046502bd..7d532de4 100644 --- a/lua/gitlab/hunks.lua +++ b/lua/gitlab/hunks.lua @@ -1,7 +1,3 @@ -local List = require("gitlab.utils.list") -local u = require("gitlab.utils") -local state = require("gitlab.state") - local M = {} ---@class Hunk @@ -9,24 +5,22 @@ local M = {} ---@field old_range integer ---@field new_line integer ---@field new_range integer +---@field lines? string[] The hunk's body lines: context, added, and removed; prefixed with " ", "+", and "-", respectively. ----@class HunksAndDiff ----@field hunks Hunk[] List of hunks ----@field all_diff_output string[] The data from the git diff command - ----Parse hunk header line into a Lua table. Return nil, if line is not a hunk header. +---Parse a diff line into a Lua table if it's a hunk header, otherwise return nil. ---@param line string ---@return Hunk? M.parse_possible_hunk_headers = function(line) - if line:sub(1, 2) == "@@" then + if line:match("^@@") 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*) @@+") - -- Git omits the ",N" count when it is exactly 1, so an empty capture means 1, - -- while a captured "0" means a genuine zero-length range (pure insertion/deletion). + -- The unified diff format omits the ",N" count when it is exactly 1, so an empty + -- capture means 1, while a captured "0" means a genuine zero-length range (pure + -- insertion/deletion). return { old_line = tonumber(old_start), old_range = tonumber(old_range) or 1, @@ -36,249 +30,88 @@ M.parse_possible_hunk_headers = function(line) end end ----Return true if given line was removed in the MR. ----The diff comes from `git.diff_files`, which runs with `--unified=0`. A hunk ----therefore carries no context lines and its old range holds removed lines only, ----so membership in that range already answers the question. ----@param linenr integer Line number in the old version of the file ----@param hunk Hunk A hunk candidate from the file's diff ----@return boolean -local line_was_removed = function(linenr, hunk) - return linenr >= hunk.old_line and linenr < hunk.old_line + hunk.old_range -end - ----Return true if given line was added in the MR. ----@param linenr integer Line number in the new version of the file ----@param hunk Hunk A hunk candidate from the file's diff ----@param all_diff_output string[] ----@return boolean -local line_was_added = function(linenr, hunk, all_diff_output) - for matching_line_index, line in ipairs(all_diff_output) do - local found_hunk = M.parse_possible_hunk_headers(line) - if found_hunk ~= nil and vim.deep_equal(found_hunk, hunk) then - -- Parse the lines from the hunk and return only the added lines - local hunk_lines = {} - local i = 1 - local line_content = all_diff_output[matching_line_index + i] - while line_content ~= nil and line_content:sub(1, 2) ~= "@@" do - if string.match(line_content, "^%+") then - table.insert(hunk_lines, line_content) - end - i = i + 1 - line_content = all_diff_output[matching_line_index + i] - end - - -- We are only looking at added lines in the changed hunk to see if their index - -- matches the index of a line that was added - local starting_index = found_hunk.new_line - 1 -- The "+j" will add one - for j, _ in ipairs(hunk_lines) do - if (starting_index + j) == linenr then - return true - end - end - end - end - return false -end - ----Parse the diff of the current file against the base SHA of the MR. ----@param base_sha string Git base SHA of the merge request ----@return HunksAndDiff hunks_and_diff The hunk headers and full diff of the file -local parse_hunks_and_diff = function(base_sha) +---Parse the diff between two commits for a file into a list of hunks. +---Each hunk carries its own header info and body lines. +---@param old_sha string SHA to diff from +---@param new_sha string SHA to diff to +---@param old_path string Old file name +---@param new_path string New file name +---@return Hunk[] hunks +M.get_hunks = function(old_sha, new_sha, old_path, new_path) local hunks = {} - local all_diff_output = {} local git = require("gitlab.git") - local reviewer = require("gitlab.reviewer") - - local diff, _ = git.diff_files(base_sha, reviewer.get_current_file_oldpath(), reviewer.get_current_file_path()) - if diff ~= nil then - for line in diff:gmatch("[^\r\n]+") do - table.insert(all_diff_output, line) - local hunk = M.parse_possible_hunk_headers(line) - if hunk ~= nil then - table.insert(hunks, hunk) - end - end + local diff, _ = git.diff_files(old_sha, new_sha, old_path, new_path) + if diff == nil then + return hunks end - return { hunks = hunks, all_diff_output = all_diff_output } -end - ----Parse the lines from a diff and return the index of the next hunk, when provided an ----initial index. ----@param lines string[] ----@param i integer ----@return integer? -local next_hunk_index = function(lines, i) - for j, line in ipairs(lines) do + local current_hunk = nil + for line in diff:gmatch("[^\r\n]+") do local hunk = M.parse_possible_hunk_headers(line) - if hunk ~= nil and j > i then - return j - end - end - return nil -end - ----Process the number of changes until the target is reached. ----This returns a negative or positive number indicating the number of lines in ----the hunk that have been added or removed prior to the target line. ----@param linenr integer ----@param hunk Hunk ----@param lines string[] ----@return integer -local net_changed_in_hunk_before_line = function(linenr, hunk, lines) - local net_lines = 0 - local current_line_old = hunk.old_line - - for _, line in ipairs(lines) do - if line:sub(1, 1) == "-" then - if current_line_old < linenr then - net_lines = net_lines - 1 - end - current_line_old = current_line_old + 1 - elseif line:sub(1, 1) == "+" then - if current_line_old < linenr then - net_lines = net_lines + 1 + if hunk ~= nil then + hunk.lines = {} + table.insert(hunks, hunk) + current_hunk = hunk + elseif current_hunk ~= nil then + local prefix = line:sub(1, 1) + if prefix == " " or prefix == "+" or prefix == "-" then + table.insert(current_hunk.lines, line) end - else - current_line_old = current_line_old + 1 - end - end - - return net_lines -end - ----Count the total number of changes in a set of lines, positive if added lines and ----negative if removed lines. ----@param lines string[] ----@return integer -local count_changes = function(lines) - local total = 0 - for _, line in ipairs(lines) do - if line:match("^%+") then - total = total + 1 - else - total = total - 1 end end - return total -end ----Return the modification type for the selected line. ----@param new_line? integer The starting or ending line of the current selection in the new version ----@param hunks Hunk[] ----@param all_diff_output string[] ----@return ("added"|"bad_file_unmodified")? -local function get_modification_type_from_new_sha(new_line, hunks, all_diff_output) - if new_line == nil then - return nil - end - return List.new(hunks):find(function(hunk) - local new_line_end = hunk.new_line + hunk.new_range - (hunk.new_range > 0 and 1 or 0) - local in_new_range = new_line >= hunk.new_line and new_line <= new_line_end - local is_range_zero = hunk.new_range == 0 and hunk.old_range == 0 - return in_new_range and (is_range_zero or line_was_added(new_line, hunk, all_diff_output)) - end) and "added" or "bad_file_unmodified" + return hunks end ----Return the modification type for the selected line. ----@param old_line? integer The starting or ending line of the current selection in the old version ----@param new_line? integer The starting or ending line of the current selection in the new version +---Return the line position (old_line, new_line, type) for a queried line number. +---Walk the hunk list once. Callers use this for both the start and end of a line range. ---@param hunks Hunk[] ----@return ("deleted"|"unmodified")? -local function get_modification_type_from_old_sha(old_line, new_line, hunks) - if old_line == nil then - return nil - end - - return List.new(hunks):find(function(hunk) - local old_line_end = hunk.old_line + hunk.old_range - (hunk.old_range > 0 and 1 or 0) - local new_line_end = hunk.new_line + hunk.new_range - (hunk.new_range > 0 and 1 or 0) - local in_old_range = old_line >= hunk.old_line and old_line <= old_line_end - local in_new_range = new_line >= hunk.new_line and new_line <= new_line_end - return (in_old_range or in_new_range) and line_was_removed(old_line, hunk) - end) and "deleted" or "unmodified" -end - ----Return the modification type of the line for which the comment is created. ----This is in order to build the payload for Gitlab correctly by setting the old line ----and new line. ----FIXME: This misses the fact that Gitlab also uses the type "expanded" (when ----commenting on lines that are more than 3 lines away from any change, thus are on ----folded lines that the user expanded manually). ----FIXME: This function is called three times when creating a ranged comment - this ----means three `git diff` calls, six times parsing the same diff output. This should ----only be done once! ----@param old_line? integer ----@param new_line? integer ----@param new_sha_focused boolean ----@return ("added"|"bad_file_unmodified"|"deleted"|"unmodified")? -function M.get_modification_type(old_line, new_line, new_sha_focused) - local hunk_and_diff_data = parse_hunks_and_diff(state.INFO.diff_refs.base_sha) - if hunk_and_diff_data.hunks == nil then - return - end - - local hunks = hunk_and_diff_data.hunks - local all_diff_output = hunk_and_diff_data.all_diff_output - return new_sha_focused and get_modification_type_from_new_sha(new_line, hunks, all_diff_output) - or get_modification_type_from_old_sha(old_line, new_line, hunks) -end - ----Return the matching line number of a line in the new/old version of the file compared ----to the currently selected version. ----@param old_sha string The base SHA of the MR when getting matching line in the old version, otherwise the head SHA when getting matching line in the new version ----@param new_sha string The head SHA of the MR when getting matching line in the old version, otherwise the base SHA when getting matching line in the new version ----@param file_path string The file name after change ----@param old_file_path string The file name before change (different from file_path for renamed/moved files) ----@param linenr integer The starting or ending line of the current selection ----@return integer? -M.calculate_matching_line_new = function(old_sha, new_sha, file_path, old_file_path, linenr) - local net_change = 0 - local diff_cmd = string.format( - "git diff --minimal --unified=0 --no-color %s %s -- %s %s", - old_sha, - new_sha, - old_file_path, - file_path - ) - - local handle = io.popen(diff_cmd) - if handle == nil then - u.notify(string.format("Error running git diff command for %s", file_path), vim.log.levels.ERROR) - return nil - end - - local all_lines = List.new({}) - for line in handle:lines() do - table.insert(all_lines, line) - end - - for i, line in ipairs(all_lines) do - local hunk = M.parse_possible_hunk_headers(line) - if hunk ~= nil then - if linenr <= hunk.old_line then - -- We have reached a hunk which starts after our target, return the changed total lines - return linenr + net_change - end - - local n = next_hunk_index(all_lines, i) or #all_lines - local diff_lines = all_lines:slice(i + 1, n - 1) - - -- If the line is IN the hunk, process the hunk and return the change until that line - if linenr >= hunk.old_line and linenr < hunk.old_line + hunk.old_range then - net_change = linenr + net_change + net_changed_in_hunk_before_line(linenr, hunk, diff_lines) - return net_change +---@param linenr integer Line number on the focused side +---@param new_file_focused boolean Whether linenr is a line number in the new version of the file +---@return PositionInfo +M.get_line_position = function(hunks, linenr, new_file_focused) + local net_change_before = 0 + + for _, hunk in ipairs(hunks) do + local hunk_start = new_file_focused and hunk.new_line or hunk.old_line + local hunk_end = new_file_focused and (hunk.new_line + hunk.new_range - 1) or (hunk.old_line + hunk.old_range - 1) + + -- Inside the hunk + if linenr >= hunk_start and linenr <= hunk_end then + local old_cursor, new_cursor = hunk.old_line, hunk.new_line + for _, line in ipairs(hunk.lines) do + local prefix = line:sub(1, 1) + if prefix == " " then + if (new_file_focused and new_cursor == linenr) or (not new_file_focused and old_cursor == linenr) then + return { old_line = old_cursor, new_line = new_cursor, type = "" } + end + old_cursor, new_cursor = old_cursor + 1, new_cursor + 1 + elseif prefix == "-" then + if not new_file_focused and old_cursor == linenr then + return { old_line = old_cursor, new_line = new_cursor, type = "old" } + end + old_cursor = old_cursor + 1 + elseif prefix == "+" then + if new_file_focused and new_cursor == linenr then + return { old_line = old_cursor, new_line = new_cursor, type = "new" } + end + new_cursor = new_cursor + 1 + end end - - -- If it's not it's after this hunk, just add all the changes and keep iterating - net_change = net_change + count_changes(diff_lines) + -- Past the hunk + elseif linenr > hunk_end then + net_change_before = net_change_before + (hunk.new_range - hunk.old_range) end end - -- TODO: Possibly handle lines that are out of range in the new files - return linenr + net_change + 1 + -- When linenr falls outside every hunk (including each hunk's 3-line context, since + -- M.get_hunks fetches the diff with --unified=3) the cursor was in unmodified content + -- that Gitlab collapses - the type is "expanded". + if new_file_focused then + return { old_line = linenr - net_change_before, new_line = linenr, type = "expanded" } + end + return { old_line = linenr, new_line = linenr + net_change_before, type = "expanded" } end return M diff --git a/lua/gitlab/indicators/common.lua b/lua/gitlab/indicators/common.lua index 649b13d1..5324a463 100644 --- a/lua/gitlab/indicators/common.lua +++ b/lua/gitlab/indicators/common.lua @@ -70,6 +70,11 @@ end M.is_old_sha = function(d_or_n) local position = M.get_first_note(d_or_n).position local old_start_line = position.line_range ~= nil and M.parse_line_code(position.line_range.start.line_code) or nil + -- FIXME: Update how `old_start_line ~= 0` is evaluated. After the Location refactor, + -- the numbers in line codes never are set to 0, but we should support the old way of + -- determining "is_old_sha" at least for some time because users will encounter the + -- old values in existing discussion nodes created with the old version of the plugin. + -- This should also check if the type of the range location(s) is "old". return position.old_line ~= nil and old_start_line ~= 0 end diff --git a/lua/gitlab/reviewer/init.lua b/lua/gitlab/reviewer/init.lua index 0336bf4b..45b097fb 100644 --- a/lua/gitlab/reviewer/init.lua +++ b/lua/gitlab/reviewer/init.lua @@ -6,7 +6,6 @@ local List = require("gitlab.utils.list") local u = require("gitlab.utils") local state = require("gitlab.state") -local hunks = require("gitlab.hunks") local async = require("diffview.async") local M = { @@ -34,7 +33,10 @@ M.open = function() require("gitlab.git_async").check_current_branch_up_to_date_on_remote() local git = require("gitlab.git") - local diffview_open_command = "DiffviewOpen" + -- The rename threshold used by Gitlab (through Gitaly) is 30%, see + -- https://gitlab.com/gitlab-org/gitaly/-/blob/db39e26f8f8a8da62e2c2db00325cf51315c89db/internal/gitaly/service/diff/commit_diff.go#L64-64 + -- https://gitlab.com/gitlab-org/gitaly/-/blob/0e81e24ae1f650c242670eb7bf66c4b4b91b7813/internal/gitaly/service/diff/find_changed_paths.go#L116-116 + local diffview_open_command = "DiffviewOpen --rename-threshold=30" if state.settings.reviewer_settings.diffview.imply_local then local has_clean_tree, err = git.has_clean_tree() @@ -172,66 +174,44 @@ M.jump = function(file_name, old_file_name, linenr, new_buffer) vim.cmd("normal! zz") end ----Get the data from diffview, such as line information and file name. ----To be used by other modules such as the comment module to create line codes or set ----diagnostics. ----@param current_win integer The ID of the currently focused window ----@return DiffviewInfo? -M.get_reviewer_data = function(current_win) - if M.diffview == nil then - return - end - local old_win = u.get_window_id_by_buffer_id(M.diffview_layout.a.file.bufnr) - local new_win = u.get_window_id_by_buffer_id(M.diffview_layout.b.file.bufnr) - - if old_win == nil or new_win == nil then - u.notify("Error getting window IDs for current files", vim.log.levels.ERROR) - return +---Return start line and end line of visual selection. +---@return integer +---@return integer +local get_visual_selection_boundaries = function() + local start_line = vim.fn.line("v") + local end_line = vim.fn.line(".") + if start_line > end_line then + start_line, end_line = end_line, start_line end + return start_line, end_line +end - local current_file = M.get_current_file_path() - if current_file == nil then - u.notify("Error getting current file from Diffview", vim.log.levels.ERROR) - return - end - - local new_line = vim.api.nvim_win_get_cursor(new_win)[1] - local old_line = vim.api.nvim_win_get_cursor(old_win)[1] - - local new_sha_focused = M.is_new_sha_focused(current_win) - - local modification_type = hunks.get_modification_type(old_line, new_line, new_sha_focused) - if modification_type == nil then - u.notify("Error getting modification type", vim.log.levels.ERROR) +---Get the data from the reviewer: file names, line information, and cursor focus. +---@return ReviewerData? +M.get_reviewer_data = function() + if M.diffview_layout == nil then return end - -- FIXME: This causes false positive warnings when the selection range spans a - -- modified line but the new_line itself is on an unmodified line. - 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 - - local current_bufnr = new_sha_focused and M.diffview_layout.b.file.bufnr or M.diffview_layout.a.file.bufnr - local opposite_bufnr = new_sha_focused and M.diffview_layout.a.file.bufnr or M.diffview_layout.b.file.bufnr + local start_line, end_line = get_visual_selection_boundaries() + local new_file_focused = M.is_new_file_focused(vim.api.nvim_get_current_win()) + local diff_refs = state.INFO.diff_refs return { - old_file_name = M.is_file_renamed() and M.diffview_layout.a.file.path or "", + old_file_name = M.is_file_renamed() and M.diffview_layout.a.file.path or M.diffview_layout.b.file.path, file_name = M.diffview_layout.b.file.path, - old_line_from_buf = old_line, - new_line_from_buf = new_line, - modification_type = modification_type, - current_bufnr = current_bufnr, - opposite_bufnr = opposite_bufnr, - new_sha_focused = new_sha_focused, - current_win_id = current_win, + old_sha = diff_refs.base_sha, + new_sha = diff_refs.head_sha, + start_line = start_line, + end_line = end_line, + new_file_focused = new_file_focused, } end ---Return true if user is focused on the new version of the file, otherwise false. ---@param current_win integer The ID of the currently focused window ---@return boolean -M.is_new_sha_focused = function(current_win) +M.is_new_file_focused = function(current_win) local b_win = u.get_window_id_by_buffer_id(M.diffview_layout.b.file.bufnr) local a_win = u.get_window_id_by_buffer_id(M.diffview_layout.a.file.bufnr) if a_win ~= current_win and b_win ~= current_win then diff --git a/lua/gitlab/reviewer/location.lua b/lua/gitlab/reviewer/location.lua index 87d65172..725fc104 100755 --- a/lua/gitlab/reviewer/location.lua +++ b/lua/gitlab/reviewer/location.lua @@ -1,243 +1,54 @@ -local u = require("gitlab.utils") local hunks = require("gitlab.hunks") -local state = require("gitlab.state") - ----@class ReviewerLineInfo ----@field old_line? integer ----@field new_line? integer ----@field type "new"|"old" - ----@class ReviewerRangeInfo ----@field start ReviewerLineInfo ----@field end ReviewerLineInfo ---@class LocationData ---@field old_line? integer ---@field new_line? integer ----@field line_range? ReviewerRangeInfo +---@field line_range? LineRange ---@class Location ---@field location_data LocationData ----@field reviewer_data DiffviewInfo ----@field run function ----@field build_location_data function ----@field visual_range table +---@field reviewer_data ReviewerData +---@field new fun(reviewer_data: ReviewerData, diff_hunks: Hunk[]): Location +---@field build_location_data fun() local Location = {} Location.__index = Location ----Return information about the selection in the reviewer. ----Return nil when the location cannot be created due to missing reviewer data. ----@return Location? -function Location.new() - local current_win = vim.api.nvim_get_current_win() - local reviewer_data = require("gitlab.reviewer").get_reviewer_data(current_win) - if reviewer_data == nil then - return nil - end - local location = {} - local instance = setmetatable(location, Location) +---Build a Location from already-resolved reviewer data and diff hunks. +---@param reviewer_data ReviewerData +---@param diff_hunks Hunk[] +---@return Location +function Location.new(reviewer_data, diff_hunks) + local instance = setmetatable({}, Location) instance.reviewer_data = reviewer_data - instance.base_sha = state.INFO.diff_refs.base_sha - instance.head_sha = state.INFO.diff_refs.head_sha - instance:build_location_data() + instance:build_location_data(diff_hunks) return instance end ----Build the payload for creating a comment based on the file name, modification type of ----the diff, and line numbers. -function Location:build_location_data() - ---@type DiffviewInfo - local reviewer_data = self.reviewer_data - - local start_line, end_line = u.get_visual_selection_boundaries() - ---@type LineRange - self.visual_range = { start_line = start_line, end_line = end_line } - +---Build the payload for creating a comment. +---@param diff_hunks Hunk[] +function Location:build_location_data(diff_hunks) + local line_range = { + start = hunks.get_line_position(diff_hunks, self.reviewer_data.start_line, self.reviewer_data.new_file_focused), + ["end"] = hunks.get_line_position(diff_hunks, self.reviewer_data.end_line, self.reviewer_data.new_file_focused), + } ---@type LocationData self.location_data = { - old_line = nil, - new_line = nil, - line_range = nil, - } - - -- Comment on new line: Include only new_line in payload. - -- Comment on deleted line: Include only old_line in payload. - -- The line was not found in any hunks, send both lines. - if reviewer_data.modification_type == "added" then - self.location_data.old_line = nil - self.location_data.new_line = reviewer_data.new_line_from_buf - elseif reviewer_data.modification_type == "deleted" then - self.location_data.old_line = reviewer_data.old_line_from_buf - self.location_data.new_line = nil - elseif - reviewer_data.modification_type == "unmodified" or reviewer_data.modification_type == "bad_file_unmodified" - then - self.location_data.old_line = reviewer_data.old_line_from_buf - self.location_data.new_line = reviewer_data.new_line_from_buf - end - - -- TODO: Don't skip line_range for single-line comments (Gitlab doesn't skip them either). - if end_line > start_line then - self.location_data.line_range = { - start = {}, - ["end"] = {}, - } - else - return - end - - self:set_range_start() - self:set_range_end() - - -- Ranged comments should always use the end of the range. - -- Otherwise they will not highlight the full comment in Gitlab. - self.location_data.old_line = self.location_data.line_range["end"].old_line - self.location_data.new_line = self.location_data.line_range["end"].new_line -end - --- Helper methods 🤝 - ----Return the matching line number from the new version of the file. ----For instance, line 12 in the new version may be scroll-linked to line 10 in the old ----version. ----@param linenr integer The starting or ending line of the current selection ----@return integer? -function Location:get_line_number_from_new_sha(linenr) - if self.reviewer_data.new_sha_focused then - return linenr - end - -- Otherwise we want to get the matching line in the opposite buffer - return hunks.calculate_matching_line_new( - self.base_sha, - self.head_sha, - self.reviewer_data.file_name, - self.reviewer_data.old_file_name, - linenr - ) -end - ----Return the matching line number from the old version of the file. ----For instance, line 12 in the new version may be scroll-linked to line 10 in the old ----version. ----@param linenr integer The starting or ending line of the current selection ----@return integer? -function Location:get_line_number_from_old_sha(linenr) - if not self.reviewer_data.new_sha_focused then - return linenr - end - - -- Otherwise we want to get the matching line in the opposite buffer - return hunks.calculate_matching_line_new( - self.head_sha, - self.base_sha, - self.reviewer_data.file_name, - self.reviewer_data.old_file_name, - linenr - ) -end - ----Return the current line number from whatever version (new or old) the reviewer is ----focused in. ----@return integer? -function Location:get_current_line() - if self.reviewer_data.current_win_id == nil then - return - end - - local current_line = vim.api.nvim_win_get_cursor(self.reviewer_data.current_win_id)[1] - return current_line -end - ----Set the range start to the location_data for the Gitlab payload based on the ----modification type, visual selection range, and the hunk data. -function Location:set_range_start() - local current_file = require("gitlab.reviewer").get_current_file_path() - if current_file == nil then - u.notify("Error getting current file from Diffview", vim.log.levels.ERROR) - return - end - - if self.reviewer_data.current_win_id == nil then - u.notify("Error getting window number of SHA for start of range", vim.log.levels.ERROR) - return - end - - local current_line = self:get_current_line() - if current_line == nil then - u.notify("Error getting current line for start of range", vim.log.levels.ERROR) - return - end - - local new_line = self:get_line_number_from_new_sha(self.visual_range.start_line) - local old_line = self:get_line_number_from_old_sha(self.visual_range.start_line) - if - (new_line == nil and self.reviewer_data.modification_type ~= "deleted") - or (old_line == nil and self.reviewer_data.modification_type ~= "added") - then - u.notify("Error getting new or old line for start of range", vim.log.levels.ERROR) - return - end - - local modification_type = hunks.get_modification_type(old_line, new_line, self.reviewer_data.new_sha_focused) - if modification_type == nil then - u.notify("Error getting modification type for start of range", vim.log.levels.ERROR) - return - end - - self.location_data.line_range.start = { - new_line = modification_type ~= "deleted" and new_line or nil, - old_line = modification_type ~= "added" and old_line or nil, - -- FIXME: The type should only be "old" explicitly for comments on deleted lines. - -- For unchanged lines this should be empty. Apart from that, the modification type - -- can also be "expanded" (when commenting on lines that are more than 3 lines away - -- from any change, thus are on folded lines that the user expanded manually. - type = modification_type == "added" and "new" or "old", - } -end - ----Set the range end to the location_data for the Gitlab payload based on the ----modification type, visual selection range, and the hunk data. -function Location:set_range_end() - local current_file = require("gitlab.reviewer").get_current_file_path() - if current_file == nil then - u.notify("Error getting current file from Diffview", vim.log.levels.ERROR) - return - end - - if self.reviewer_data.current_win_id == nil then - u.notify("Error getting window number of SHA for end of range", vim.log.levels.ERROR) - return - end - - local current_line = self:get_current_line() - if current_line == nil then - u.notify("Error getting current line for end of range", vim.log.levels.ERROR) - return - end - - local new_line = self:get_line_number_from_new_sha(self.visual_range.end_line) - local old_line = self:get_line_number_from_old_sha(self.visual_range.end_line) - - if - (new_line == nil and self.reviewer_data.modification_type ~= "deleted") - or (old_line == nil and self.reviewer_data.modification_type ~= "added") - then - u.notify("Error getting new or old line for end of range", vim.log.levels.ERROR) - return - end - - local modification_type = hunks.get_modification_type(old_line, new_line, self.reviewer_data.new_sha_focused) - if modification_type == nil then - u.notify("Error getting modification type for end of range", vim.log.levels.ERROR) - return - end - - self.location_data.line_range["end"] = { - new_line = modification_type ~= "deleted" and new_line or nil, - old_line = modification_type ~= "added" and old_line or nil, - type = modification_type == "added" and "new" or "old", + -- Top-level old_line and new_line must correspond to the end of the range to be + -- placed correctly in Gitlab. They are only set if they match the modification + -- type. + old_line = line_range["end"].type ~= "new" and line_range["end"].old_line or nil, + new_line = line_range["end"].type ~= "old" and line_range["end"].new_line or nil, + line_range = line_range, } + -- TODO: Warn the user when position.type == "" (unmodified) while their selection was + -- made on the "wrong" side, since gitlab.nvim may render such a comment's diagnostic + -- on the other side of the diff than the one the user was looking at. This used to + -- exist (see the removed "bad_file_unmodified" modification type) but was lost in a + -- refactor; reviving it needs its own design, since which side gitlab.nvim actually + -- picks incorrectly doesn't reflect the start of the comment range and can show a + -- comment spanning "new-unchanged" lines on the old file even if it belongs to the + -- new file. end return Location diff --git a/lua/gitlab/utils/init.lua b/lua/gitlab/utils/init.lua index 366a097c..56c08a22 100644 --- a/lua/gitlab/utils/init.lua +++ b/lua/gitlab/utils/init.lua @@ -557,19 +557,6 @@ M.check_visual_mode = function() return true end ----Return start line and end line of visual selection. ----TODO: Move to `lua/gitlab/reviewer/location.lua` ----@return integer ----@return integer -M.get_visual_selection_boundaries = function() - local start_line = vim.fn.line("v") - local end_line = vim.fn.line(".") - if start_line > end_line then - start_line, end_line = end_line, start_line - end - return start_line, end_line -end - ---Get icon for filename if nvim-web-devicons plugin is available, otherwise return ---empty string. ---@return string? diff --git a/tests/spec/comment_spec.lua b/tests/spec/comment_spec.lua new file mode 100644 index 00000000..2d06992d --- /dev/null +++ b/tests/spec/comment_spec.lua @@ -0,0 +1,59 @@ +describe("gitlab/actions/comment.lua", function() + describe("new_location_from_reviewer", function() + -- comment.lua captures `reviewer` via a top-level require, so stubbing + -- gitlab.reviewer only takes effect if comment.lua is required again afterwards. + local function load_comment() + package.loaded["gitlab.actions.comment"] = nil + return require("gitlab.actions.comment") + end + + after_each(function() + package.loaded["gitlab.reviewer"] = nil + package.loaded["gitlab.git"] = nil + package.loaded["gitlab.actions.comment"] = nil + end) + + it("returns nil when the reviewer has no data to give", function() + package.loaded["gitlab.reviewer"] = { + get_reviewer_data = function() + return nil + end, + } + + local comment = load_comment() + assert.is_nil(comment.new_location_from_reviewer()) + end) + + it("threads reviewer_data's shas and file names into hunks.get_hunks in the right order", function() + package.loaded["gitlab.reviewer"] = { + get_reviewer_data = function() + return { + old_file_name = "old_name.txt", + file_name = "new_name.txt", + old_sha = "old-sha", + new_sha = "new-sha", + start_line = 1, + end_line = 1, + new_file_focused = true, + } + end, + } + + local seen_old_sha, seen_new_sha, seen_old_path, seen_new_path + package.loaded["gitlab.git"] = { + diff_files = function(old_sha, new_sha, old_path, new_path) + seen_old_sha, seen_new_sha, seen_old_path, seen_new_path = old_sha, new_sha, old_path, new_path + return nil, nil + end, + } + + local comment = load_comment() + comment.new_location_from_reviewer() + + assert.are.same("old-sha", seen_old_sha) + assert.are.same("new-sha", seen_new_sha) + assert.are.same("old_name.txt", seen_old_path) + assert.are.same("new_name.txt", seen_new_path) + end) + end) +end) diff --git a/tests/spec/hunks_spec.lua b/tests/spec/hunks_spec.lua index 4195ed73..eb8c0527 100644 --- a/tests/spec/hunks_spec.lua +++ b/tests/spec/hunks_spec.lua @@ -27,103 +27,142 @@ describe("gitlab/hunks.lua", function() end) end) - describe("get_modification_type", function() - local state = require("gitlab.state") - + describe("get_hunks", function() local function stub_diff(diff_text) package.loaded["gitlab.git"] = { diff_files = function() return diff_text, nil end, } - package.loaded["gitlab.reviewer"] = { - get_current_file_oldpath = function() - return "file.txt" - end, - get_current_file_path = function() - return "file.txt" - end, - } end - before_each(function() - state.INFO = { diff_refs = { base_sha = "base-sha" } } - end) - after_each(function() - state.INFO = nil package.loaded["gitlab.git"] = nil - package.loaded["gitlab.reviewer"] = nil end) - it("does not classify the unmodified context line above a single-line deletion as added", function() + it("attaches each hunk's own body lines instead of a flat diff blob", function() stub_diff([[ diff --git a/file.txt b/file.txt index 1111111..2222222 100644 --- a/file.txt +++ b/file.txt -@@ -5 +4,0 @@ --old content that was removed +@@ -2,7 +2,6 @@ + line 2 + line 3 + line 4 +-line 5 + line 6 + line 7 + line 8 ]]) - local got = hunks.get_modification_type(4, 4, true) - assert.are_not.same("added", got) - assert.are.same("bad_file_unmodified", got) + local got = hunks.get_hunks("old-sha", "new-sha", "file.txt", "file.txt") + assert.are.same(1, #got) + assert.are.same({ old_line = 2, old_range = 7, new_line = 2, new_range = 6 }, { + old_line = got[1].old_line, + old_range = got[1].old_range, + new_line = got[1].new_line, + new_range = got[1].new_range, + }) + assert.are.same({ " line 2", " line 3", " line 4", "-line 5", " line 6", " line 7", " line 8" }, got[1].lines) end) - it("keeps classifying the context line above a two-line deletion as bad_file_unmodified", function() + it("splits body lines into separate hunks when the diff has more than one", function() stub_diff([[ diff --git a/file.txt b/file.txt index 1111111..2222222 100644 --- a/file.txt +++ b/file.txt -@@ -5,2 +4,0 @@ --old line 5 --old line 6 +@@ -2,4 +2,4 @@ + line 2 +-line 3 ++line three + line 4 +@@ -40,3 +40,4 @@ + line 40 ++line 41 + line 42 ]]) - local got = hunks.get_modification_type(4, 4, true) - assert.are.same("bad_file_unmodified", got) + local got = hunks.get_hunks("old-sha", "base-sha", "file.txt", "file.txt") + assert.are.same(2, #got) + assert.are.same({ " line 2", "-line 3", "+line three", " line 4" }, got[1].lines) + assert.are.same({ " line 40", "+line 41", " line 42" }, got[2].lines) end) - it("treats a deleted line as deleted", function() - stub_diff([[ -diff --git a/file.txt b/file.txt -index 1111111..2222222 100644 ---- a/file.txt -+++ b/file.txt -@@ -5 +4,0 @@ --old line 5 -]]) + it("returns no hunks when there is no diff", function() + stub_diff(nil) + local got = hunks.get_hunks("old-sha", "base-sha", "file.txt", "file.txt") + assert.are.same({}, got) + end) + end) + + describe("get_line_position", function() + it("classifies a deleted line and shifts the unmodified lines that follow it", function() + local hunk = { + old_line = 2, + old_range = 7, + new_line = 2, + new_range = 6, + lines = { " a", " b", " c", "-removed", " d", " e", " f" }, + } - assert.are.same("deleted", hunks.get_modification_type(5, 4, false)) + assert.are.same({ old_line = 5, new_line = 5, type = "old" }, hunks.get_line_position({ hunk }, 5, false)) + assert.are.same({ old_line = 6, new_line = 5, type = "" }, hunks.get_line_position({ hunk }, 6, false)) + assert.are.same({ old_line = 6, new_line = 5, type = "" }, hunks.get_line_position({ hunk }, 5, true)) end) - it("treats the line below a single-line deletion as unmodified", function() - stub_diff([[ -diff --git a/file.txt b/file.txt -index 1111111..2222222 100644 ---- a/file.txt -+++ b/file.txt -@@ -5 +4,0 @@ --old line 5 -]]) + it("classifies an added line and shifts the unmodified lines that follow it", function() + local hunk = { + old_line = 2, + old_range = 6, + new_line = 2, + new_range = 7, + lines = { " a", " b", " c", "+added", " d", " e", " f" }, + } - assert.are.same("unmodified", hunks.get_modification_type(6, 5, false)) + assert.are.same({ old_line = 5, new_line = 5, type = "new" }, hunks.get_line_position({ hunk }, 5, true)) + assert.are.same({ old_line = 5, new_line = 6, type = "" }, hunks.get_line_position({ hunk }, 5, false)) end) - it("treats the line below a multi-line deletion as unmodified", function() - stub_diff([[ -diff --git a/file.txt b/file.txt -index 1111111..2222222 100644 ---- a/file.txt -+++ b/file.txt -@@ -5,2 +4,0 @@ --old line 5 --old line 6 -]]) + it("treats a line beyond any hunk's context as expanded, shifted by earlier hunks", function() + local hunks_list = { + { old_line = 2, old_range = 2, new_line = 2, new_range = 5, lines = { " a", "+b", "+c", " d", "+e" } }, + } + + -- net change introduced by the hunk: new_range(5) - old_range(2) = +3 + assert.are.same( + { old_line = 50, new_line = 53, type = "expanded" }, + hunks.get_line_position(hunks_list, 50, false) + ) + assert.are.same( + { old_line = 47, new_line = 50, type = "expanded" }, + hunks.get_line_position(hunks_list, 50, true) + ) + end) + + it("treats a line before any hunk as expanded with no shift", function() + local hunks_list = { + { old_line = 20, old_range = 1, new_line = 20, new_range = 0, lines = { "-x" } }, + } + + assert.are.same({ old_line = 5, new_line = 5, type = "expanded" }, hunks.get_line_position(hunks_list, 5, false)) + end) + + it("treats a line between two distant hunks as expanded, shifted only by the earlier one", function() + local hunks_list = { + { old_line = 2, old_range = 2, new_line = 2, new_range = 5, lines = { " a", "+b", "+c", " d", "+e" } }, + { old_line = 100, old_range = 5, new_line = 103, new_range = 3, lines = { " a", "-b", "-c", " d", " e" } }, + } + + assert.are.same( + { old_line = 50, new_line = 53, type = "expanded" }, + hunks.get_line_position(hunks_list, 50, false) + ) + end) - assert.are.same("unmodified", hunks.get_modification_type(7, 5, false)) + it("returns an empty hunk list result as expanded with no shift", function() + assert.are.same({ old_line = 10, new_line = 10, type = "expanded" }, hunks.get_line_position({}, 10, false)) end) end) end) diff --git a/tests/spec/location_spec.lua b/tests/spec/location_spec.lua new file mode 100644 index 00000000..cb660806 --- /dev/null +++ b/tests/spec/location_spec.lua @@ -0,0 +1,104 @@ +local hunks = require("gitlab.hunks") +local Location = require("gitlab.reviewer.location") + +describe("gitlab/reviewer/location.lua", function() + local function hunks_from_diff(diff_text) + package.loaded["gitlab.git"] = { + diff_files = function() + return diff_text, nil + end, + } + local file_hunks = hunks.get_hunks("old-sha", "new-sha", "file.txt", "file.txt") + package.loaded["gitlab.git"] = nil + return file_hunks + end + + local function reviewer_data(overrides) + return vim.tbl_extend("force", { + old_file_name = "file.txt", + file_name = "file.txt", + old_sha = "old-sha", + new_sha = "new-sha", + start_line = 1, + end_line = 1, + new_file_focused = true, + }, overrides or {}) + end + + -- A single-line deletion at old line 5, surrounded by --unified=3 context (old 2-8, new 2-7). + -- Each context line is labeled "old new" so the expected shift is visible in the fixture itself. + local DELETION_HUNKS = hunks_from_diff([[ +diff --git a/file.txt b/file.txt +index 1111111..2222222 100644 +--- a/file.txt ++++ b/file.txt +@@ -2,7 +2,6 @@ + line 2 2 + line 3 3 + line 4 4 +-line 5 + line 6 5 + line 7 6 + line 8 7 +]]) + + -- A single-line insertion after old line 4, surrounded by --unified=3 context (old 2-7, new 2-8). + -- Each context line is labeled "old new" so the expected shift is visible in the fixture itself. + local ADDITION_HUNKS = hunks_from_diff([[ +diff --git a/file.txt b/file.txt +index 1111111..2222222 100644 +--- a/file.txt ++++ b/file.txt +@@ -2,6 +2,7 @@ + line 2 2 + line 3 3 + line 4 4 ++line 5 + line 5 6 + line 6 7 + line 7 8 +]]) + + it("builds a range on unmodified lines with real, matching old/new line numbers", function() + local location = + Location.new(reviewer_data({ start_line = 2, end_line = 4, new_file_focused = true }), DELETION_HUNKS) + + assert.are.same({ old_line = 2, new_line = 2, type = "" }, location.location_data.line_range.start) + assert.are.same({ old_line = 4, new_line = 4, type = "" }, location.location_data.line_range["end"]) + assert.are.same(4, location.location_data.old_line) + assert.are.same(4, location.location_data.new_line) + end) + + it("nils out the top-level old_line when the range ends on an added line", function() + -- new-side selection from unmodified line 2 to the added line (new_line 5) + local location = + Location.new(reviewer_data({ start_line = 2, end_line = 5, new_file_focused = true }), ADDITION_HUNKS) + + assert.are.same({ old_line = 2, new_line = 2, type = "" }, location.location_data.line_range.start) + assert.are.same({ old_line = 5, new_line = 5, type = "new" }, location.location_data.line_range["end"]) + assert.is_nil(location.location_data.old_line) + assert.are.same(5, location.location_data.new_line) + end) + + it("nils out the top-level new_line when the range ends on a deleted line", function() + -- old-side selection from unmodified line 2 to the deleted line (old_line 5) + local location = + Location.new(reviewer_data({ start_line = 2, end_line = 5, new_file_focused = false }), DELETION_HUNKS) + + assert.are.same({ old_line = 2, new_line = 2, type = "" }, location.location_data.line_range.start) + assert.are.same({ old_line = 5, new_line = 5, type = "old" }, location.location_data.line_range["end"]) + assert.are.same(5, location.location_data.old_line) + assert.is_nil(location.location_data.new_line) + end) + + it("keeps both top-level line numbers real for an expanded (far-from-any-change) range", function() + -- Line 500 is nowhere near the hunk (old 2-8 / new 2-7), so it falls outside every + -- hunk's --unified=3 context and is classified as "expanded", not "". + local location = + Location.new(reviewer_data({ start_line = 500, end_line = 500, new_file_focused = false }), DELETION_HUNKS) + + assert.are.same({ old_line = 500, new_line = 499, type = "expanded" }, location.location_data.line_range["end"]) + assert.are.same(500, location.location_data.old_line) + assert.are.same(499, location.location_data.new_line) + end) +end)