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. |
f8cead0 to
b7e8318
Compare
`NuiSplit:unmount` sets an internal loading flag before destroying buffer and window and clears it only at the end, so an error in between leaves the flag set and every later unmount returns early without closing anything. `close` marked the split as gone regardless, so the window can stay on screen while `split_visible` says otherwise and the next toggle opens a second one beside it. No such failure was observed, that part is hardening. Close the window directly when the split does not, and keep `split_visible` set for as long as the window is alive. The WinClosed handler defers the teardown to the next tick. A buffer wiped from inside a WinClosed callback fires no BufWipeout, and the autocmds that reset `linked_bufnr` and `unlinked_bufnr` hang off that event, so a synchronous teardown leaves both fields holding the number of a wiped buffer.
The linked and unlinked buffers are created per open, not per session, so the pair the closing window leaves behind stays listed forever while the next open allocates a fresh one: two leaked buffers per open/close cycle.
A node of type "note" without `is_root` recurses into `tree:get_node(nil)`, which resolves a node from a window's cursor and can hand back the very same node. The call is in tail position, so the recursion never overflows the stack, it freezes Neovim. Give up on a nil parent, the way get_note_node already does.
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.
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. |
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.