fix: get correct location data for comments efficiently - #582
fix: get correct location data for comments efficiently#582jakubbortlik wants to merge 5 commits into
Conversation
Follow-ups will include:
|
30bf262 to
67a640e
Compare
seflue
left a comment
seflue
left a comment
There was a problem hiding this comment.
Thanks, Jakub. Calculating the diff the same way GitLab does makes a lot of sense, and it takes a whole class of edge cases out of the code.
Most of what I found is minor and noted inline. The one you should consider the most is the blank-line case in hunks.lua: a comment could end up attached to a different line than the one selected.
| -- 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 | ||
| -- dlyongemallo/diffview.nvim can be configured to respect this: | ||
| M._original_diffview_config = require("diffview.config").get_config() |
There was a problem hiding this comment.
Minor but could surprise users of Diffview: you capture the original config unconditionally and when restoring you keep the captured value in M._original_diffview_config. If the user closes the Review diffview by hand, close is not run, so the config override leaks into the users setting. And because you capture it, a reopen would capture gitlab.nvims override as the users setting and that will be kept for the rest of the session.
If you check it for nil before capturing it and reset to nil after restore in line 111 it would at least become self-healing when reopen and closing the gitlab.nvim's diffview properly.
There was a problem hiding this comment.
Yes, this is a half-baked hack and it doesn't even work 😁. For some reason even with this diffview setup in place just before the DiffviewOpen command is run, the value when running diff_file_list was still set to value in my gitlab.nvim config. So I removed this and replaced with a recommendation for config.
I've also opened an issue in the diffview-plus.nvim repo to allow specifying the rename threshold for each command.
There was a problem hiding this comment.
David, the maintainer of diffview-plus.nvim, has self-assigned the issue. In my experience he's very efficient with implementing features, so I'll keep this PR open for a bit and maybe we'll be able to merge it with the functionality in place which would be preferable to suggesting some user configuration and replacing it with different instructions soon afterwards.
67a640e to
fc0c29c
Compare
fc0c29c to
20a18c2
Compare
seflue
left a comment
seflue
left a comment
There was a problem hiding this comment.
I just left one inline-comment to consider, non-blocking.
| 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"` |
There was a problem hiding this comment.
Thanks for adding this.
What I didn't think of in the initial review: GitLab's position object has a third position_type, file, which carries paths and no line numbers at all. In their generated OpenAPI spec the only required members are:
base_shastart_shahead_shaposition_type
Optional:
new_line,old_lineline_range
That is what a file-level comment looks like on the wire. Since the tag makes file_name imply line_range, we would now reject such a payload. Nothing sends it today - lua/gitlab/actions/comment.lua:102 hardcodes type = "text" - but we should take it into consideration when we add file-level comments.
Minor, while I was in there: it seems no test currently covers file_name set with line_range missing.
2 participants
This PR fixes #386 and #478.
Apart from sending the correct data to Gitlab (correct line codes, line numbers and modification types), this plugin now creates these data more efficiently, with a single git diff call and with a single iteration through the diff. The PR removes many (often nearly identical) helper functions and adds tests for the newly added or modified functions.