feat(reviewer): browse MR commit history

.github/workflows/lua.yaml

@@ -6,6 +6,7 @@ on:
       - develop
     paths:
       - 'lua/**' # Ignore changes to the Go code
+      - 'tests/**'
 jobs:
   lua_lint:
     name: Lint Lua 💅

README.md

@@ -178,6 +178,7 @@ These keymaps are available globally (i.e., in any buffer).
 | `glC`     | Create a new MR for currently checked-out feature branch                                          |
 | `glc`     | Chose MR for review                                                                               |
 | `glS`     | Start review for the currently checked-out branch                                                 |
+| `glh`     | Browse the MR's commit history, one commit at a time                                              |
 | `gl<C-R>` | Load new MR state from Gitlab and apply new diff refs to the diff view                            |
 | `gls`     | Show the editable summary of the MR                                                               |
 | `glu`     | Copy the URL of the MR to the system clipboard                                                    |
@@ -239,6 +240,22 @@ These `keymaps` are active in the reviewer window (the diff view).
 | `s`  | Create a suggestion for the lines that the following {motion} moves over |
 | `a`  | Jump to the comment in the discussion tree                               |
 
+#### Commit Browser Keymaps
+
+These `keymaps` are active in the commit browser (`glh`), which steps through the
+MR one commit at a time.
+
+| Keys | Action                                                                          |
+| ---- | -------------------------------------------------------------------------------- |
+| `c`  | Comment on the current line, anchored to the commit being viewed                 |
+| `a`  | Jump to the comment in the discussion tree                                        |
+| `g?` | Show these keymaps                                                               |
+
+`c` works on both sides of the diff, including a line the commit deletes. To
+follow the line under the cursor to the next commit that changes it, bind
+`select_next_change_here` / `select_prev_change_here` in your diffview config.
+Those actions exist only in the `diffview+` fork.
+
 ## Contributing
 
 Contributions to the plugin are welcome. Please read [.github/CONTRIBUTING.md](.github/CONTRIBUTING.md) before you start working on a pull request.

after/syntax/gitlab.vim

@@ -11,10 +11,13 @@ let formatted_date = '\w\+ \{1,2}\d\{1,2}, \d\{4}'
 let absolute_time = '\d\{2}/\d\{2}/\d\{4} at \d\{2}:\d\{2}'
 let date = '\%(' . time_ago . '\|' . formatted_date . '\|' . absolute_time . '\|just now\)'
 
-let published = date . ' \%(' . g:gitlab_discussion_tree_resolved . '\|' . g:gitlab_discussion_tree_unresolved . '\|' . g:gitlab_discussion_tree_unlinked . '\)\?'
+" Commits are referenced by the first 7 characters of their SHA, e.g. '1a2b3c4'
+let commit_ref = '[0-9a-f]\{7}'
+
+let published = date . '\%( ' . commit_ref . '\)\?' . ' \%(' . g:gitlab_discussion_tree_resolved . '\|' . g:gitlab_discussion_tree_unresolved . '\|' . g:gitlab_discussion_tree_unlinked . '\)\?'
 let state = ' \%(' . published . '\|' . g:gitlab_discussion_tree_draft . '\)'
 
-execute 'syntax match GitlabNoteHeader "' . expanders . username . state . '" contains=GitlabDate,GitlabUnresolved,GitlabUnlinked,GitlabResolved,GitlabExpander,GitlabDraft,GitlabUsername'
+execute 'syntax match GitlabNoteHeader "' . expanders . username . state . '" contains=GitlabDate,GitlabUnresolved,GitlabUnlinked,GitlabResolved,GitlabExpander,GitlabDraft,GitlabUsername,GitlabCommit'
 
 execute 'syntax match GitlabDate "' . date . '" contained'
 execute 'syntax match GitlabUnresolved "' . g:gitlab_discussion_tree_unresolved . '" contained'
@@ -24,5 +27,6 @@ execute 'syntax match GitlabExpander "' . expanders . '" contained'
 execute 'syntax match GitlabDraft "' . g:gitlab_discussion_tree_draft . '" contained'
 execute 'syntax match GitlabUsername "' . username . '" contained'
 execute 'syntax match GitlabMention "' . username . '"'
+execute 'syntax match GitlabCommit "' . commit_ref . '" contained'
 
 let b:current_syntax = 'gitlab'

cmd/app/comment.go

@@ -97,6 +97,10 @@ func (a commentService) postComment(w http.ResponseWriter, r *http.Request) {
 		opt.Position = buildCommentPosition(commentWithPositionData)
 	}
 
+	if payload.CommitID != "" {
+		opt.CommitID = &payload.CommitID
+	}
+
 	discussion, res, err := a.client.CreateMergeRequestDiscussion(a.projectInfo.ProjectId, a.projectInfo.MergeId, &opt)
 
 	if err != nil {

cmd/app/comment_helpers.go

@@ -41,6 +41,7 @@ type PositionData struct {
 	StartCommitSHA string     `json:"start_commit_sha"`
 	Type           string     `json:"type"`
 	LineRange      *LineRange `json:"line_range" validate:"required_with=FileName"`
+	CommitID       string     `json:"commit_id,omitempty"`
 }
 
 /* RequestWithPosition is an interface that abstracts the handling of position data for a comment or a draft comment */

cmd/app/comment_test.go

@@ -9,6 +9,7 @@ import (
 
 type fakeCommentClient struct {
 	testBase
+	capturedOpt **gitlab.CreateMergeRequestDiscussionOptions
 }
 
 func (f fakeCommentClient) CreateMergeRequestDiscussion(pid interface{}, mergeRequest int64, opt *gitlab.CreateMergeRequestDiscussionOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Discussion, *gitlab.Response, error) {
@@ -17,6 +18,10 @@ func (f fakeCommentClient) CreateMergeRequestDiscussion(pid interface{}, mergeRe
 		return nil, nil, err
 	}
 
+	if f.capturedOpt != nil {
+		*f.capturedOpt = opt
+	}
+
 	return &gitlab.Discussion{Notes: []*gitlab.Note{{}}}, resp, err
 }
 func (f fakeCommentClient) UpdateMergeRequestDiscussionNote(pid interface{}, mergeRequest int64, discussion string, note int64, opt *gitlab.UpdateMergeRequestDiscussionNoteOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Note, *gitlab.Response, error) {
@@ -80,10 +85,70 @@ func TestPostComment(t *testing.T) {
 		assert(t, data.Message, "Comment created successfully")
 	})
 
+	t.Run("Passes commit_id through to the Gitlab client when provided", func(t *testing.T) {
+		testCommentCreationData := PostCommentRequest{
+			Comment: "Some comment",
+			PositionData: PositionData{
+				FileName: "file.txt",
+				CommitID: "abc123",
+				LineRange: &LineRange{
+					Start: &PositionInfo{Type: "", OldLine: 4, NewLine: 4},
+					End:   &PositionInfo{Type: "", OldLine: 4, NewLine: 4},
+				},
+			},
+		}
+		request := makeRequest(t, http.MethodPost, "/mr/comment", testCommentCreationData)
+		var capturedOpt *gitlab.CreateMergeRequestDiscussionOptions
+		svc := middleware(
+			commentService{testProjectData, fakeCommentClient{capturedOpt: &capturedOpt}},
+			withMr(testProjectData, fakeMergeRequestLister{}),
+			withPayloadValidation(methodToPayload{
+				http.MethodPost:   newPayload[PostCommentRequest],
+				http.MethodDelete: newPayload[DeleteCommentRequest],
+				http.MethodPatch:  newPayload[EditCommentRequest],
+			}),
+			withMethodCheck(http.MethodPost, http.MethodDelete, http.MethodPatch),
+		)
+		getSuccessData(t, svc, request)
+		if capturedOpt.CommitID == nil {
+			t.Fatal("expected CommitID to be set")
+		}
+		assert(t, *capturedOpt.CommitID, "abc123")
+	})
+
+	t.Run("Leaves commit_id unset when not provided", func(t *testing.T) {
+		testCommentCreationData := PostCommentRequest{
+			Comment: "Some comment",
+			PositionData: PositionData{
+				FileName: "file.txt",
+				LineRange: &LineRange{
+					Start: &PositionInfo{Type: "", OldLine: 4, NewLine: 4},
+					End:   &PositionInfo{Type: "", OldLine: 4, NewLine: 4},
+				},
+			},
+		}
+		request := makeRequest(t, http.MethodPost, "/mr/comment", testCommentCreationData)
+		var capturedOpt *gitlab.CreateMergeRequestDiscussionOptions
+		svc := middleware(
+			commentService{testProjectData, fakeCommentClient{capturedOpt: &capturedOpt}},
+			withMr(testProjectData, fakeMergeRequestLister{}),
+			withPayloadValidation(methodToPayload{
+				http.MethodPost:   newPayload[PostCommentRequest],
+				http.MethodDelete: newPayload[DeleteCommentRequest],
+				http.MethodPatch:  newPayload[EditCommentRequest],
+			}),
+			withMethodCheck(http.MethodPost, http.MethodDelete, http.MethodPatch),
+		)
+		getSuccessData(t, svc, request)
+		if capturedOpt.CommitID != nil {
+			t.Fatalf("expected CommitID to be nil, got %q", *capturedOpt.CommitID)
+		}
+	})
+
 	t.Run("Handles errors from Gitlab client", func(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/mr/comment", testCommentCreationData)
 		svc := middleware(
-			commentService{testProjectData, fakeCommentClient{testBase{errFromGitlab: true}}},
+			commentService{testProjectData, fakeCommentClient{testBase: testBase{errFromGitlab: true}}},
 			withMr(testProjectData, fakeMergeRequestLister{}),
 			withPayloadValidation(methodToPayload{
 				http.MethodPost:   newPayload[PostCommentRequest],
@@ -99,7 +164,7 @@ func TestPostComment(t *testing.T) {
 	t.Run("Handles non-200s from Gitlab client", func(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/mr/comment", testCommentCreationData)
 		svc := middleware(
-			commentService{testProjectData, fakeCommentClient{testBase{status: http.StatusSeeOther}}},
+			commentService{testProjectData, fakeCommentClient{testBase: testBase{status: http.StatusSeeOther}}},
 			withMr(testProjectData, fakeMergeRequestLister{}),
 			withPayloadValidation(methodToPayload{
 				http.MethodPost:   newPayload[PostCommentRequest],

cmd/app/draft_notes.go

@@ -112,6 +112,10 @@ func (a draftNoteService) postDraftNote(w http.ResponseWriter, r *http.Request)
 		opt.Position = buildCommentPosition(draftNoteWithPosition)
 	}
 
+	if payload.CommitID != "" {
+		opt.CommitID = &payload.CommitID
+	}
+
 	draftNote, res, err := a.client.CreateDraftNote(a.projectInfo.ProjectId, a.projectInfo.MergeId, &opt)
 
 	if err != nil {

cmd/app/draft_notes_test.go

@@ -9,6 +9,7 @@ import (
 
 type fakeDraftNoteManager struct {
 	testBase
+	capturedOpt **gitlab.CreateDraftNoteOptions
 }
 
 func (f fakeDraftNoteManager) ListDraftNotes(pid interface{}, mergeRequest int64, opt *gitlab.ListDraftNotesOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.DraftNote, *gitlab.Response, error) {
@@ -24,6 +25,9 @@ func (f fakeDraftNoteManager) CreateDraftNote(pid interface{}, mergeRequest int6
 	if err != nil {
 		return nil, nil, err
 	}
+	if f.capturedOpt != nil {
+		*f.capturedOpt = opt
+	}
 	return &gitlab.DraftNote{}, resp, err
 }
 
@@ -104,6 +108,54 @@ func TestPostDraftNote(t *testing.T) {
 		data := getSuccessData(t, svc, request)
 		assert(t, data.Message, "Draft note created successfully")
 	})
+
+	t.Run("Passes commit_id through to the Gitlab client when provided", func(t *testing.T) {
+		testData := PostDraftNoteRequest{
+			Comment: "Some comment",
+			PositionData: PositionData{
+				FileName: "file.txt",
+				CommitID: "abc123",
+				LineRange: &LineRange{
+					Start: &PositionInfo{Type: "", OldLine: 4, NewLine: 4},
+					End:   &PositionInfo{Type: "", OldLine: 4, NewLine: 4},
+				},
+			},
+		}
+		request := makeRequest(t, http.MethodPost, "/mr/draft_notes/", testData)
+		var capturedOpt *gitlab.CreateDraftNoteOptions
+		svc := middleware(
+			draftNoteService{testProjectData, fakeDraftNoteManager{capturedOpt: &capturedOpt}},
+			withMr(testProjectData, fakeMergeRequestLister{}),
+			withPayloadValidation(methodToPayload{
+				http.MethodPost:  newPayload[PostDraftNoteRequest],
+				http.MethodPatch: newPayload[UpdateDraftNoteRequest],
+			}),
+			withMethodCheck(http.MethodGet, http.MethodPost, http.MethodPatch, http.MethodDelete),
+		)
+		getSuccessData(t, svc, request)
+		if capturedOpt.CommitID == nil {
+			t.Fatal("expected CommitID to be set")
+		}
+		assert(t, *capturedOpt.CommitID, "abc123")
+	})
+
+	t.Run("Leaves commit_id unset when not provided", func(t *testing.T) {
+		request := makeRequest(t, http.MethodPost, "/mr/draft_notes/", testPostDraftNoteRequestData)
+		var capturedOpt *gitlab.CreateDraftNoteOptions
+		svc := middleware(
+			draftNoteService{testProjectData, fakeDraftNoteManager{capturedOpt: &capturedOpt}},
+			withMr(testProjectData, fakeMergeRequestLister{}),
+			withPayloadValidation(methodToPayload{
+				http.MethodPost:  newPayload[PostDraftNoteRequest],
+				http.MethodPatch: newPayload[UpdateDraftNoteRequest],
+			}),
+			withMethodCheck(http.MethodGet, http.MethodPost, http.MethodPatch, http.MethodDelete),
+		)
+		getSuccessData(t, svc, request)
+		if capturedOpt.CommitID != nil {
+			t.Fatalf("expected CommitID to be nil, got %q", *capturedOpt.CommitID)
+		}
+	})
 }
 
 func TestDeleteDraftNote(t *testing.T) {

doc/gitlab.nvim.txt

@@ -255,6 +255,7 @@ you call this function with no values the defaults will be used:
         create_mr = "glC", -- Create a new MR for currently checked-out feature branch
         choose_merge_request = "glc", -- Chose MR for review (if necessary check out the feature branch)
         start_review = "glS", -- Start review for the currently checked-out branch
+        browse_commits = "glh", -- Browse the MR's commit history, one commit at a time
         reload_review = "gl<C-R>", -- Load new MR state from Gitlab and apply new diff refs to the diff view
         summary = "gls", -- Show the editable summary of the MR
         copy_mr_url = "glu", -- Copy the URL of the MR to the system clipboard
@@ -482,6 +483,7 @@ you call this function with no values the defaults will be used:
         file_name = "Normal",
         resolved = "DiagnosticSignOk",
         unresolved = "DiagnosticSignWarn",
+        commit = "DiagnosticSignInfo",
         draft = "DiffviewNonText",
         draft_mode = "DiagnosticWarn",
         live_mode = "DiagnosticOk",
@@ -921,6 +923,24 @@ Opens the reviewer pane. Can be used from anywhere within Neovim after the
 plugin is loaded. If run twice, will open a second reviewer pane.
 >lua
   require("gitlab").review()
+<
+                                                  *gitlab.nvim.browse_commits*
+gitlab.browse_commits() ~
+
+Opens a view of the MR's commit history, letting you step through it one commit
+at a time. Each entry shows a single commit's isolated diff, to understand how
+the MR was built up.
+
+Press `c` to comment on a line, anchored to the commit being viewed. This works
+on both sides of the diff, including a line the commit deletes. Press `a` on a
+commented line to jump to the comment in the discussion tree. `g?` lists these
+keymaps.
+
+To follow the line under the cursor to the next commit that changes it, bind
+`select_next_change_here` / `select_prev_change_here` in your diffview config.
+Those actions exist only in the `diffview+` fork.
+>lua
+  require("gitlab").browse_commits()
 <
                                                    *gitlab.nvim.reload_review*
 gitlab.reload_review() ~

lua/gitlab/actions/comment.lua

@@ -35,6 +35,26 @@ M.new_location_from_reviewer = function()
   return Location.new(reviewer_data, diff_hunks)
 end
 
+---Build the position_data payload for a positioned comment, anchored to the MR's current
+---revision or, with `M.location.commit_override` set, to that single commit. Gitlab
+---rejects a commit_id whose position refs do not describe the commit's own diff.
+---@return table
+M.build_position_data = function()
+  local revision = state.MR_REVISIONS[1]
+  local override = M.location.commit_override
+  return {
+    file_name = M.location.reviewer_data.file_name,
+    old_file_name = M.location.reviewer_data.old_file_name,
+    base_commit_sha = override and override.base_sha or revision.base_commit_sha,
+    start_commit_sha = override and override.start_sha or revision.start_commit_sha,
+    head_commit_sha = override and override.head_sha or revision.head_commit_sha,
+    old_line = M.location.location_data.old_line,
+    new_line = M.location.location_data.new_line,
+    line_range = M.location.location_data.line_range,
+    commit_id = override and override.commit_id,
+  }
+end
+
 ---Fire the API to send the comment data to the Go server.
 ---@param text string comment text
 ---@param unlinked boolean if true, the comment is not linked to a line
@@ -86,17 +106,7 @@ local confirm_create_comment = function(text, unlinked, discussion_id)
     return
   end
 
-  local revision = state.MR_REVISIONS[1]
-  local position_data = {
-    file_name = M.location.reviewer_data.file_name,
-    old_file_name = M.location.reviewer_data.old_file_name,
-    base_commit_sha = revision.base_commit_sha,
-    start_commit_sha = revision.start_commit_sha,
-    head_commit_sha = revision.head_commit_sha,
-    old_line = M.location.location_data.old_line,
-    new_line = M.location.location_data.new_line,
-    line_range = M.location.location_data.line_range,
-  }
+  local position_data = M.build_position_data()
 
   -- Creating a new comment (linked to specific changes)
   local body = u.merge({ type = "text", comment = text }, position_data)
@@ -250,6 +260,17 @@ M.create_note = function()
   layout:mount()
 end
 
+---Open a comment popup for a `location` the caller resolved itself, instead of reading the
+---live reviewer.
+---@param location table reviewer_data (file_name, old_file_name, new_file_focused,
+---start_line, end_line), location_data (old_line, new_line, line_range), and optionally
+---commit_override (base_sha, start_sha, head_sha, commit_id)
+M.create_comment_for_location = function(location)
+  M.location = location
+  local layout = M.create_comment_layout({ unlinked = false })
+  layout:mount()
+end
+
 ---Given the current visually selected area of text, builds text to fill in the
 ---comment popup with a suggested change
 ---@return string[]?