Fix: Off by 1 Error in Old Sha (#202)

Fixes an issue in the old sha where the index when iterating through the lines of the hunk in the old SHA was off by one

Author: harrisoncramer

lua/gitlab/hunks/init.lua

@@ -35,28 +35,31 @@ end
 ---@param linnr number
 ---@param hunk Hunk
 ---@param all_diff_output table
+---@return boolean
 local line_was_removed = function(linnr, 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, 1 do
+      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 == linnr then
           if string.match(line_content, "^%-") then
-            return "deleted"
+            return true
           end
         end
       end
     end
   end
+  return false
 end
 
 ---@param linnr number
 ---@param hunk Hunk
 ---@param all_diff_output table
+---@return boolean
 local line_was_added = function(linnr, hunk, all_diff_output)
   for matching_line_index, line in ipairs(all_diff_output) do
     local found_hunk = M.parse_possible_hunk_headers(line)
@@ -71,13 +74,14 @@ local line_was_added = function(linnr, hunk, all_diff_output)
         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"
+            return true
           end
         end
         i = i + 1
       end
     end
   end
+  return false
 end
 
 ---Parse git diff hunks.
@@ -173,63 +177,58 @@ local count_changes = function(lines)
   return total
 end
 
+---@param new_line number|nil
+---@param hunks Hunk[]
+---@param all_diff_output table
+---@return string|nil
+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
+    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"
+end
+
+---@param old_line number|nil
+---@param new_line number|nil
+---@param hunks Hunk[]
+---@param all_diff_output table
+---@return string|nil
+local function get_modification_type_from_old_sha(old_line, new_line, hunks, all_diff_output)
+  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
+    local new_line_end = hunk.new_line + hunk.new_range
+    local in_old_range = old_line >= hunk.old_line and old_line <= old_line_end
+    local in_new_range = old_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)
+  end) and "deleted" or "unmodified"
+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 old_line number|nil
 ---@param new_line number|nil
 ---@param current_file string
+---@param is_current_sha boolean
 ---@return string|nil
-function M.get_modification_type(old_line, new_line, current_file)
+function M.get_modification_type(old_line, new_line, current_file, is_current_sha)
   local hunk_and_diff_data = parse_hunks_and_diff(current_file, state.INFO.target_branch)
   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
-
-  local is_current_sha = require("gitlab.reviewer").is_current_sha()
-
-  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 new_line >= hunk.new_line and new_line <= 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 line_was_added(new_line, 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 old_line >= hunk.old_line and old_line <= old_line_end and hunk.old_range == 0 then
-        return "deleted"
-      end
-      if
-        (old_line >= hunk.old_line and old_line <= old_line_end)
-        or (old_line >= hunk.new_line and new_line <= new_line_end)
-      then
-        if line_was_removed(old_line, 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"
+  return is_current_sha 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)
 end
 
 ---Returns the matching line number of a line in the new/old version of the file compared to the current SHA.

lua/gitlab/reviewer/init.lua

@@ -167,18 +167,20 @@ M.get_reviewer_data = function()
 
   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 modification_type = hunks.get_modification_type(old_line, new_line, current_file)
+
+  local is_current_sha = M.is_current_sha()
+  local modification_type = hunks.get_modification_type(old_line, new_line, current_file, is_current_sha)
   if modification_type == nil then
-    u.notify("Error parsing hunks for modification type", vim.log.levels.ERROR)
+    u.notify("Error getting modification type", vim.log.levels.ERROR)
     return
   end
 
   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 = M.is_current_sha() and layout.b.file.bufnr or layout.a.file.bufnr
-  local opposite_bufnr = M.is_current_sha() and layout.a.file.bufnr or layout.b.file.bufnr
+  local current_bufnr = is_current_sha and layout.b.file.bufnr or layout.a.file.bufnr
+  local opposite_bufnr = is_current_sha and layout.a.file.bufnr or layout.b.file.bufnr
   local old_sha_win_id = u.get_window_id_by_buffer_id(layout.a.file.bufnr)
   local new_sha_win_id = u.get_window_id_by_buffer_id(layout.b.file.bufnr)
 

lua/gitlab/reviewer/location.lua

@@ -141,7 +141,8 @@ function Location:set_start_range(visual_range)
   end
 
   local reviewer = require("gitlab.reviewer")
-  local win_id = reviewer.is_current_sha() and self.reviewer_data.new_sha_win_id or self.reviewer_data.old_sha_win_id
+  local is_current_sha = reviewer.is_current_sha()
+  local win_id = is_current_sha and self.reviewer_data.new_sha_win_id or self.reviewer_data.old_sha_win_id
   if win_id == nil then
     u.notify("Error getting window number of SHA for start range", vim.log.levels.ERROR)
     return
@@ -163,7 +164,11 @@ function Location:set_start_range(visual_range)
     return
   end
 
-  local modification_type = hunks.get_modification_type(old_line, new_line, current_file)
+  local modification_type = hunks.get_modification_type(old_line, new_line, current_file, is_current_sha)
+  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,
@@ -199,7 +204,14 @@ function Location:set_end_range(visual_range)
     return
   end
 
-  local modification_type = hunks.get_modification_type(old_line, new_line, current_file)
+  local reviewer = require("gitlab.reviewer")
+  local is_current_sha = reviewer.is_current_sha()
+  local modification_type = hunks.get_modification_type(old_line, new_line, current_file, is_current_sha)
+  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,