feat(reviewer): browse MR commit history - #577
Conversation
|
Hi Sebastian, I think this will be a welcome improvement to the plugin (I believe there was already the issue #450 that tracked the feature request, so I guess you can close your new issue #576). However, I don't know whether Harrison, the repo owner, has the time and motivation to review any PRs right now. I certainly want to have a look at it some time, but at the moment my priority is fixing #386 which has been around for almost two years and I believe the fix will also influence the your PR - I would like to radically simplify and fix the way how the Location data are calculated for normal comments, as currently the plugin does too much work (like parsing diff hunks three times instead of once) and it doesn't even produce the correct data (line codes, modification types, and line numbers in ranged comments). I've just briefly had a look at your PR and I have some general suggestions. I believe it would be more user-friendly if the commenting worked similar to how comments can be created in the standard reviewer and how comments on individual commits can be created in Gitlab online: Users should be able to create ranged comments on any commit, irrespective of whether the code was changed later, otherwise I don't see the point of commenting on individual commits if I can't really comment on them. If it's currently impossible to create comments on lines that are changed in later commits, then we should find out how to make it possible instead of forcing the user to comment on a different commit. I suggest you investigate what payload Gitlab creates when creating commit-bound comments in the browser - I've briefly had a look and it sends different "base_sha", "head_sha", "start_sha" values, and I think the way to go will be to also send the correct SHAs. I would like to fix #386 as soon as possible and then I will be open to reviewing your PR but I can't promise anything. |
|
@jakubbortlik thanks for raising this. Going by the docs, the discussions position is always built from the MR's diff refs, so I assumed comments could only target the MR head. I just ran some tests against a live instance and got it working. It's the same endpoint we already use. With the position's I'd still like to keep two things. Walking through a file's versions is useful on its own, independent of commenting, so I'd leave it in as a browsing feature. And when a line commented on an earlier commit gets overwritten later in the same MR, the user should notice. GitLab already marks such comments outdated, and I'd rather show it than hide the case. As for #386, I'll keep going in the meantime and rebase once it's merged. |
f6c0806 to
0991df1
Compare
0991df1 to
b049dca
Compare
ef6dfcf to
375415d
Compare
|
I continued to work on the implementation. This is what I have achieved now:
To show the discussion window in both tabs (reviewer tab and commit-history tab) I had to add a window registry. There used to be exactly one discussion window: its window id, its two buffers and the current view type were single fields in I did one refactoring to make the code easier to read. It was not necessary for the implementation, but it helped me understand it. I also fixed a small thing: jumping from a line without a comment warned about missing diagnostics, which confused me. It now says there is no comment on this line. The last point is an idea, not implemented yet. Commit comments and MR comments currently live in the same list, told apart only by the hash. I am thinking about separating them with a third winbar tab for the commit-specific comments. Typing The code sits on top of #581. |
b7e8318 to
0800554
Compare
|
Hi Sebastian. Please have a look at #582 - I'd be grateful if you could do a genuine review and also if you told me if that PR will be useful for your current PR or I could do something differently to make your work on this easier. |
0800554 to
bd944a6
Compare
bd944a6 to
5677200
Compare
c comments on the line under the cursor while browsing a MR commit by commit. The comment is anchored to the commit being viewed: the position's head_sha is that commit and a top-level commit_id is sent alongside it, which binds the note to that commit's diff while keeping it MR-scoped. Every line of the commit's new side is commentable. GitLab overwrites base_sha and start_sha with the MR base whatever is sent, so a line the commit deletes cannot be positioned from here: its old_line would be numbered against the MR base, while the browser shows the file at the commit's parent. Commenting from the old side is refused rather than guessed at. Such a note is marked in the browser and nowhere else. Its lines are relative to that commit's own diff, so in the MR diff the marker would sit on an unrelated line. a jumps from the marker into the discussion tree, as it does in the reviewer. ]v and [v follow the line under the cursor to the next or previous commit that changes it, via git log -L. That is navigation only, and independent of commenting. refresh_diagnostics() no longer errors when no regular reviewer view is open, which commenting from the browser would otherwise trigger on an otherwise successful comment. It still errors when a reviewer is open, where a missing view is a broken state.
Buffers and trees are shared, so the tabs show the same content; only the window, its view type and its cursor position are per tab. Closing the reviewer takes down every discussion window, since one can now sit in a tab the reviewer does not own.
5677200 to
8498c68
Compare
|
Hi Sebastian. I'm having a look at your PR and I definitely want to make the feature part of the plugin, but the PR is huge and I would like to propose some changes to limit the scope, so that it becomes manageable. I'd like to also keep working on some other fixes/features in the meantime :) I realize that a large part of the additions are tests, which is great, but tests are code too and need to be reviewed and maintained, so this doesn't make the review any simpler :) Here are some suggestions to make this more likely to get merged. I hope these comments make sense and will be actionable. Split the PRPlease pull out commits not directly related to the this feature into separate PRs if possible (mainly jumping to the discussion). Consider if other parts (e.g., the window registry and allowing the discussion split in several tabs) could also be pulled out. Consider upstreaming some features to diffview+The commit navigation feature: Commenting on commitsI need to understand the use case or your intended workflow better, as I don't use the commit view in Gitlab too often (but that might change if it's available in I believe it should be possible to reuse the existing way of getting the location data - modify Outdated commentsYou wrote before that you'd like to keep this behaviour:
Does it mean A) the user should notice when creating the comment or B) we should somehow mark such comments in the discussion tree? Regarding A: I agree that a notification would be useful, maybe there could be two kinds of commenting keybindings: one to comment on this exact commit and another one to comment on the last commit that touches this line (I believe this is closer to how you intended the commenting to work). Regarding B: Outdated comments often have incorrectly placed diagnostics, which is something we should fix regardless of whether the comment was on a specific commit or just in an older version of the MR (comment in the regular reviewer). Maybe we could also implement something like "jump to the diff version on which this comment was originally created" which is conceptually similar to your feature, except for the distinction between "versions" and "commits" - comparing the various versions of v MR would be a useful feature that could be a followup for this PR. Just FYI, my long-lived draft #504 enables something similar - to edit a comment with a preview diff in a new tab that uses the original head sha, but that's a workaround, not a proper solution for the use case and I'm not particularly pressed to finish that PR since I currently prioritize fixing bugs and use that suggestion-preview branch locally for my own daily work... Other commentsBugs
- The spinner in the two tabs is broken because both winbars update the same [spinner_index](https://github.com/seflue/gitlab.nvim/blob/86bf3bcbf7622dadb7bad3aa4cdbcecf3b27c67a/lua/gitlab/actions/discussions/winbar.lua#L60) - that's not a bug you introduced, it's part of a faulty implementation on my side and I'm working on a fix so you don't need to worry about it (a fix definitely should not be part of this PR 😁).
Separate winbar tab
In #463 I proposed something related - use a third tab to show the MR activity - I miss this kind of information in |
There was a problem hiding this comment.
I'm working on a re-implementation (#584) of the discussion tree to use virtual text for indentation instead of literal spaces and as a part of that I would like to change the syntax highlighting of the discussion tree header nodes to extmarks so this file will almost completely be gone. But adding the commit_ref will be very easy in the new approach.
The commit browser shelled out to git diff itself and so missed the hardening the location refactor added: no external diff driver, no suppressed blank lines. diff_files takes the context width now, since the browser's range arithmetic needs zero context. Position building stays hand-rolled. Location.new derives old_line from the hunks it is given, and for parent..commit that number is against a base Gitlab replaces with the MR base on storage.
Commenting from the browser on a line the browsed commit did not change sent a position typed "new" with no old_line, whose line_code encoded old line 0. Gitlab answers 500 to that. Measured against a live instance: it validates the position in the commit's own parent..commit diff and renumbers old_line to the MR base itself. Location.new fed with that diff's hunks builds exactly what it accepts, which also removes the hand-rolled position.
The old side was refused because its old_line looked unanchorable. It is not: the position is numbered in the commit's own diff, the rule the new side follows too, so the side only decides how the line is typed. Measured against a live instance: a deleted line that came from the MR base is stored against the MR base with old_line renumbered, and one an earlier commit in the MR added is stored against the commit's own refs unchanged. Gitlab resolves the position where it can and keeps what we sent where it cannot.
The jump refused the old side because a comment stored against the MR base is numbered in a diff the browser does not show. Its line range is not: Gitlab renumbers the top-level old_line where it can and leaves the range in the commit's own numbering, which is what the browser shows. So the range is where the line comes from, and the cursor lands in the old window.
A comment on a line neither side changed carries both line numbers, and old_line alone read it as a deletion, so the jump landed in the old window. The range's own type says what the line is, which is what the FIXME on this function asked for. Only reachable since commenting on such a line works at all; before that the position carried no old_line.
2 participants
Closes #576.
On larger MRs I like to review commit by commit. My teammates put real effort into structuring their history, and getting value out of that means the tooling has to support reviewing it that way.
So I added a commit browser:
glhopens the MR's history so it can be read one commit at a time.I also added commenting from that view:
ccomments on the line under the cursor while browsing. A commit shows its owncommit^..commitdiff while GitLab positions comments against the MR's base..head, so a line is only commentable when it maps cleanly onto base..head; when a later commit rewrote it,]v/[vmove to a version where it does. I went with "comment where it maps, navigate otherwise", but I'm open to other takes.The regular reviewer is untouched. Line math comes from
git diffandgit log -Lagainst the MR's own SHAs, so it ignores the working tree, and the parsers have unit tests.One dependency outside the browser:
refresh_diagnostics()now returns quietly when no regular reviewer view is open. Commenting routes through the shared comment path, which rebuilds the views; with no reviewer open that previously errored on an otherwise successful comment.