From fd532ebfe36d0cea7a83ede608fbe4f99a0a9cc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Fl=C3=BCgge?= <952313+seflue@users.noreply.github.com> Date: Sun, 2 Aug 2026 00:14:53 +0200 Subject: [PATCH 1/2] fix: comment placement above a one-line deletion Commenting on the unchanged line directly above a deletion of exactly one line sent it as an added line, anchored to the new side, and without the warning that comments on unmodified lines are placed in the old file. It now goes to the old file with both line numbers, the way it already did above a deletion of two or more lines. --- lua/gitlab/hunks.lua | 6 ++- tests/spec/hunks_spec.lua | 89 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 2 deletions(-) create mode 100644 tests/spec/hunks_spec.lua diff --git a/lua/gitlab/hunks.lua b/lua/gitlab/hunks.lua index 01df7160..044282ee 100644 --- a/lua/gitlab/hunks.lua +++ b/lua/gitlab/hunks.lua @@ -25,11 +25,13 @@ M.parse_possible_hunk_headers = function(line) -- @@ -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). return { old_line = tonumber(old_start), - old_range = tonumber(old_range) or 0, + old_range = tonumber(old_range) or 1, new_line = tonumber(new_start), - new_range = tonumber(new_range) or 0, + new_range = tonumber(new_range) or 1, } end end diff --git a/tests/spec/hunks_spec.lua b/tests/spec/hunks_spec.lua new file mode 100644 index 00000000..aa7b860d --- /dev/null +++ b/tests/spec/hunks_spec.lua @@ -0,0 +1,89 @@ +local hunks = require("gitlab.hunks") + +describe("gitlab/hunks.lua", function() + describe("parse_possible_hunk_headers", function() + it("treats an omitted old count as 1, keeping an explicit new count", function() + local got = hunks.parse_possible_hunk_headers("@@ -5 +5,3 @@") + local want = { old_line = 5, old_range = 1, new_line = 5, new_range = 3 } + assert.are.same(want, got) + end) + + it("keeps an explicit old count of 0 as a genuine pure insertion", function() + local got = hunks.parse_possible_hunk_headers("@@ -5,0 +5,3 @@") + local want = { old_line = 5, old_range = 0, new_line = 5, new_range = 3 } + assert.are.same(want, got) + end) + + it("treats both omitted counts as 1", function() + local got = hunks.parse_possible_hunk_headers("@@ -5 +5 @@") + local want = { old_line = 5, old_range = 1, new_line = 5, new_range = 1 } + assert.are.same(want, got) + end) + + it("treats an omitted new count as 1, keeping an explicit old count", function() + local got = hunks.parse_possible_hunk_headers("@@ -5,3 +5 @@") + local want = { old_line = 5, old_range = 3, new_line = 5, new_range = 1 } + assert.are.same(want, got) + end) + end) + + describe("get_modification_type", function() + local state = require("gitlab.state") + + 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() + 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 +]]) + + local got = hunks.get_modification_type(4, 4, true) + assert.are_not.same("added", got) + assert.are.same("bad_file_unmodified", got) + end) + + it("keeps classifying the context line above a two-line deletion as bad_file_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 +]]) + + local got = hunks.get_modification_type(4, 4, true) + assert.are.same("bad_file_unmodified", got) + end) + end) +end) From 1b17edd8ac51b9d48ee093be7331e0a068f1c946 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Fl=C3=BCgge?= <952313+seflue@users.noreply.github.com> Date: Sun, 2 Aug 2026 16:42:45 +0200 Subject: [PATCH 2/2] refactor: simplify the deleted-line check The check implemented the general algorithm for arbitrary diffs, including context lines, and its range carried an off-by-one that no caller could reach. The only diff it ever sees comes from git.diff_files, which runs with --unified=0 and has no context lines, so a line is removed exactly when it falls in the hunk's old range. Simplify to that rather than fix the bound. --- lua/gitlab/hunks.lua | 31 ++++++++---------------------- tests/spec/hunks_spec.lua | 40 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 23 deletions(-) diff --git a/lua/gitlab/hunks.lua b/lua/gitlab/hunks.lua index 044282ee..046502bd 100644 --- a/lua/gitlab/hunks.lua +++ b/lua/gitlab/hunks.lua @@ -37,28 +37,14 @@ M.parse_possible_hunk_headers = function(line) 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 ----@param all_diff_output string[] ---@return boolean -local line_was_removed = 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 - -- 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 do - local line_content = all_diff_output[matching_line_index + 1] - if hunk_line_index == linenr then - if string.match(line_content, "^%-") then - return true - end - end - end - end - end - return false +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. @@ -200,9 +186,8 @@ end ---@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 ---@param hunks Hunk[] ----@param all_diff_output string[] ---@return ("deleted"|"unmodified")? -local function get_modification_type_from_old_sha(old_line, new_line, hunks, all_diff_output) +local function get_modification_type_from_old_sha(old_line, new_line, hunks) if old_line == nil then return nil end @@ -212,7 +197,7 @@ local function get_modification_type_from_old_sha(old_line, new_line, hunks, all 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, all_diff_output) + return (in_old_range or in_new_range) and line_was_removed(old_line, hunk) end) and "deleted" or "unmodified" end @@ -238,7 +223,7 @@ function M.get_modification_type(old_line, new_line, new_sha_focused) 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, 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 diff --git a/tests/spec/hunks_spec.lua b/tests/spec/hunks_spec.lua index aa7b860d..4195ed73 100644 --- a/tests/spec/hunks_spec.lua +++ b/tests/spec/hunks_spec.lua @@ -85,5 +85,45 @@ index 1111111..2222222 100644 local got = hunks.get_modification_type(4, 4, true) assert.are.same("bad_file_unmodified", got) 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 +]]) + + assert.are.same("deleted", hunks.get_modification_type(5, 4, false)) + 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 +]]) + + assert.are.same("unmodified", hunks.get_modification_type(6, 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 +]]) + + assert.are.same("unmodified", hunks.get_modification_type(7, 5, false)) + end) end) end)