Fix diagnostic position when sha changes

[jakubbortlik]

This PR is an attempt to improve diagnostic placement and jump_to_reviewer behaviour reported in #158.

It introduces the following changes:

1. The position of a diagnostic and for jump_to_reviewer is calculated with a common function, reducing the potential for incorrect jumps
2. The position for ranged diagnostics is based on the first_note.position.new_line or first_note.position.old_line, rather than the line_code - in my experience, this tends to be the most accurate source of information about where to place diagnostics, see example below. The information in the line_codes is used to calculate the length of the diagnostic range, though.
3. Old SHA detection is improved. Relying on the old_line only is not always reliable, I've come across notes that had a non-nil old_line, but still should be placed in the NEW buffer, since the line code for range.start looked something like this: "a34a68046a50849178410fce4af6182a98526bfc_0_94", -> the start of the diagnostic was not set (was 0) and the code that calculated the diagnostic position lnum = new_start_line - 1 produced a negative value, which caused an error.

I've been using this branch for some time and it looks like it improves the placement of many diagnostics, but it still does not solve all the problems. E.g., diagnostics are still misplaced for code that has been moved to a different file or for files that have been heavily modified. The good thing is, that this PR does not seem to produce worse results than the previous solution.

For the cases when diagnostics are still misplaced, I'd suggest to implement a feature that would open a new DiffviewFileHistory and select the revision on which the original note was made. Then the user will be free to navigate to newer versions of the file and easily see what changed so that the note placement is out of date. I'm going to open a new feature request for this.

Example of diagnostics positions

In this example from a discussions tree node, the correct position is the top new_line, all the information in range (the ["end"].new_line and both line numbers in ["end"].line_code are outdated:

  new_line = 93,
  range = {
    ["end"] = {
      line_code = "a34a68046a50849178410fce4af6182a98526bfc_96_94",
      new_line = 94,
      old_line = 0,
      type = "new"
    },
  },

[harrisoncramer]

Thanks for working on this.

There is one recurring issue I've noticed. This is probably also an issue on main but we can try to address it as part of this MR. In the new buffer, creating a ranged comment that starts on a modified or added line, but ends on an unmodified line, does not set the diagnostic correctly.

The comment is created correctly in Gitlab, but the start line for the diagnostic is not correct. For instance, if I make a ranged comment that starts on line 7 (which is added) and spans to line 9 (in the new buffer), here's the comment's position per Gitlab:

{
  "body": "1",
  "position": {
    "base_sha": "70cf8a54aeaaa4c94802b6b641a1b32447455ca4",
    "head_sha": "b2f138e2e82cdc6fcf7d55ae12b345cbc4f9a5fc",
    "start_sha": "70cf8a54aeaaa4c94802b6b641a1b32447455ca4",
    "new_path": "blah.js",
    "old_path": "blah.js",
    "position_type": "text",
    "new_line": 9,
    "old_line": 8,
    "line_range": {
      "start": {
        "line_code": "766c3895fabeada8abe3a60b1b39e6baf2dd8f03_0_7",
        "type": "new"
      },
      "end": {
        "line_code": "766c3895fabeada8abe3a60b1b39e6baf2dd8f03_8_9",
        "type": "old"
      }
    }
  }
}

If I had to guess, I think in this case the 0 may be intended to indicate that this line does not exist in the old diff, but your parsing function is picking it up as an actual line code.

[jakubbortlik]

Hi @harrisoncramer, thanks for looking at this.

Could you be more specific on the way the diagnostic is not correct in the case you describe? If I try to reproduce it on this PR's branch, I'm actually getting the correct diagnostic if I create a suggestion on lines 3 and 4 of the NEW buffer, ranging from an added line to an old line:

This is part of the node in question:

  new_line = 4,
  old_line = 2,
  range = {
    ["end"] = {
      line_code = "669a2c84d4eb3d540cd258b34af26909f6173cef_2_4",
      new_line = 0,
      old_line = 0,
      type = "old"
    },
    start = {
      line_code = "669a2c84d4eb3d540cd258b34af26909f6173cef_0_3",
      new_line = 0,
      old_line = 0,
      type = "new"
    }

Surprisingly, when I create the same comment in Gitlab (instead of neovim), the diagnostic is wrongly placed in the OLD buffer:

  new_line = 4,
  old_line = 2,
  range = {
    ["end"] = {
      line_code = "669a2c84d4eb3d540cd258b34af26909f6173cef_2_4",
      new_line = 4,
      old_line = 2,
      type = ""
    },
    start = {
      line_code = "669a2c84d4eb3d540cd258b34af26909f6173cef_2_3",
      new_line = 3,
      old_line = 0,
      type = "new"
    }
  },

Maybe the way indicators_common.is_old_sha() works should be turned around from the current:

---@param d_or_n Discussion|DraftNote
---@return boolean
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
  return position.old_line ~= nil and old_start_line ~= 0
end

---@param discussion Discussion|DraftNote
---@return boolean
M.is_new_sha = function(discussion)
  return not M.is_old_sha(discussion)
end

to this:

---@param d_or_n Discussion|DraftNote
---@return boolean
M.is_new_sha = function(d_or_n)
  local position = M.get_first_note(d_or_n).position
  local _, new_start_line = position.line_range ~= nil and M.parse_line_code(position.line_range.start.line_code) or nil, nil
  return position.new_line ~= nil and new_start_line ~= 0
end

---@param discussion Discussion|DraftNote
---@return boolean
M.is_old_sha = function(discussion)
  return not M.is_new_sha(discussion)
end

This would, however, require some more changes, and I don't know whether this would create more problems for other diagnostics. A comprehensive test suite would be really useful for such modifications.

[jakubbortlik]

Hi @harrisoncramer, I've just come back to this PR and I'd like to ask about your opinion on changing the logic of is_old_sha and is_new_sha.

Apart from that, I've upgraded neovim to version 0.10 and I've found out that now, the range diagnostics in gitlab.nvim do not work. I can mostly only see the "I" indicator, but not the ranges:

This is in nvim 0.9:

This is the same file in nvim 0.10:

I'll try to figure out what's wrong with that and fix it in this PR, if you don't mind.

[harrisoncramer]

Hey @jakubbortlik Let's do the issue relating to diagnostics a new MR please, I've also noticed this since switching to .10 and I'm not sure what's causing it, since the breaking changes do not mention anything related to diagnostics. I've opened up a separate issue for it here: #302

And yes, I agree that a comprehensive test suite would be very helpful. I'm not sure I'll ever get there, since it'd be a bit tricky to set up, we'd probably want to basically generate a hunk file that contains a bunch of changes and use it as input to these functions to validate them. Absent that, these are basically the regression checks that I've been doing to check functionality. For unranged comments, check that the following works:

• Place comment on new buffer (no change line)
• Place comment on new buffer (added line)
• Place comment on new buffer (modified line)
• Place comment on old buffer (deleted line)
• Place comment on old buffer (no change line)
• Place comment on old buffer (modified line)

For ranged comments, check that the following works:

• Place comment on new buffer (ending on line w/ no change)
• Place comment on new buffer (ending on added line)
• Place comment on new buffer (ending on changed line)
• Place comment on old buffer (ending on line w/ no change)
• Place comment on old buffer (ending on deleted line)
• Place comment on old buffer (ending on modified line)

I'm open to changing around the is_new_sha and is_old_sha logic if you think that would help, I'd just ask that you try these tests out on both branches and see whether we see strange behavior as a result of that change.

[jakubbortlik]

Hi Harrison. I agree that it's a good idea to fix the ranged diagnostics in a separate PR. I'd prefer to get that fixed first and only then continue work on this PR. Do you think you'll be able to do it in the near future? If not, I can try and find out what has changed in 0.10 that causes this behaviour.

[harrisoncramer]

I tried looking into it for a bit this weekend and didn't see anything obvious. Go for it! I'm not sure why, but it seems like subsequent sign placements are actually removing previous ones? You might try just playing around with signs outside of the plugin entirely, this isn't an area of the editor I have much experience with so it's possible I did something dumb...

[jakubbortlik]

Hi @harrisoncramer, I've finally managed to track down a bug that I was experiencing and that is also present on main (ranged comments in the OLD buffer which ended right after a line that was deleted would fail). And I've added a couple of minor fixes that I encountered along the way. Please, could you review this PR again and ideally test this branch?

You mentioned one more situation where the comment is not created correctly (ranged comment in new buffer, spanning from added line to unmodified line). As far as I can tell from comparing main and this feature branch, this seems fixed now.

There is one remaining issue with range comments, and that is when in the new buffer when the selection starts on a modified/added line and ends on an unmodified line, the warning "Comments on unmodified lines will be placed in the old file" is displayed even if the diagnostic is in fact placed in the new buffer. I have some idea why this is happening but I'm not going to solve this in this PR that has already been open for too long, mainly because it was quite difficult and time-consuming to debug some of the tricky parts of how the diff hunks and cursor positions are analyzed.

[jakubbortlik]

Using this branch right now at work and I've come across another case that is not yet solved, but I believe could be solvable (and I remember you mentioned this topic in the original issue):

• the discussion sometimes contains information about diff changes, e.g., changed this line in [version 8 of the diff](/PATH/TO/PROJECT/-/merge_requests/17/diffs?diff_id=49578&start_sha=b8a21c0e1c976ee83476fd631d5bc2eb7f4e56aa#302157ca84036705aa2105011209ec166168dabd_88_92) - I see right now, that for a misplaced diagnostic (on line 88) the correct position is actually contained at the end of the URL (92).

I'd like to actually try and use this information to further improve diagnostic placement, but I'd prefer to leave that to a separate issue so that the current improvement gets into develop/main quicker.

[harrisoncramer]

Sounds great, I'll test this out this weekend and merge it into develop once I've done so. Thank you for your hard work on this!! 💪

[jakubbortlik]

This annotation should actually be removed. I added it because it helped me get rid of some LSP warnings, but I didn't know the warnings were caused by a bad LSP setting. Now this annotation probably introduces more typing warnings with a properly set up LSP. I can fix this on Monday, or if you @harrisoncramer would like to merge the PR earlier, than please remove the annotation yourself.