fix: get correct location data for comments efficiently

README.md

@@ -91,7 +91,7 @@ And with <a href="https://github.com/lewis6991/pckr.nvim">pckr.nvim</a>:
 
 ### Notes on dependencies
 
-`gitlab.nvim` uses the `diffview.nvim` plugin for showing the diffs in a MR. We recommend using `dlyongemallo`'s [diffview+](https://github.com/dlyongemallo/diffview-plus.nvim) fork which is the de-facto maintained version of the plugin with many fixes and improvements (e.g., marking files as viewed). The original [sindrets/diffview.nvim](https://github.com/sindrets/diffview.nvim) plugin will be supported by `gitlab.nvim` as long as the maintenance remains feasible.
+`gitlab.nvim` uses the `diffview.nvim` plugin for showing the diffs in a MR. We recommend using `dlyongemallo`'s [diffview+](https://github.com/dlyongemallo/diffview-plus.nvim) fork which is an actively maintained version of the plugin with many fixes and improvements (e.g., marking files as viewed). Importantly, it allows setting the same similarity threshold for detecting renamed files as is used by Gitlab (30%). When using the original [sindrets/diffview.nvim](https://github.com/sindrets/diffview.nvim) plugin, file renames may not be detected correctly and comments created on such files will contain incorrect metadata or may fail. Nevertheless, the original `sindrets/diffview.nvim` plugin will be supported by `gitlab.nvim` as long as the maintenance remains feasible.
 
 Some plugin actions use Neovim’s `vim.ui.select()` picker, which looks much nicer if you use `dressing.nvim` or a similar UI plugin. To use Dressing with `gitlab.nvim`, enable it for `vim.ui.select()` like this:
 ```lua

cmd/app/comment_helpers.go

@@ -7,17 +7,27 @@ import (
 	gitlab "gitlab.com/gitlab-org/api/client-go"
 )
 
-/* LinePosition represents a position in a line range. Unlike the Gitlab struct, this does not contain LineCode with a sha1 of the filename */
-type LinePosition struct {
+/* PositionInfo represents one endpoint (start or end) of a line range, as sent by the Lua
+* plugin. Unlike the Gitlab struct, it has no LineCode - Lua can't compute a sha1, so
+* buildCommentPosition computes one below from OldLine and NewLine.
+*
+* OldLine and NewLine are always real, non-nil integers, even when Type is "old" or "new"
+* and only one side actually has a line. On the side that doesn't, the value is a position
+* marker, not a claim that a line exists there: it's wherever that side's cursor was
+* sitting when the other side's line was found. LineCode is always built from this
+* unzeroed pair; buildCommentPosition separately zeroes the inapplicable side before
+* setting it on the request's LineRange.{Start,End}.{OldLine,NewLine} - see
+* zeroInapplicableLine. */
+type PositionInfo struct {
 	Type    string `json:"type"`
 	OldLine int64  `json:"old_line"`
 	NewLine int64  `json:"new_line"`
 }
 
 /* LineRange represents the range of a note. */
 type LineRange struct {
-	StartRange *LinePosition `json:"start"`
-	EndRange   *LinePosition `json:"end"`
+	Start *PositionInfo `json:"start" validate:"required"`
+	End   *PositionInfo `json:"end" validate:"required"`
 }
 
 /* PositionData represents the position of a comment or note (relative to a file diff) */
@@ -30,7 +40,7 @@ type PositionData struct {
 	BaseCommitSHA  string     `json:"base_commit_sha"`
 	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"`
 }
 
 /* RequestWithPosition is an interface that abstracts the handling of position data for a comment or a draft comment */
@@ -42,48 +52,65 @@ type RequestWithPosition interface {
 func buildCommentPosition(commentWithPositionData RequestWithPosition) *gitlab.PositionOptions {
 	positionData := commentWithPositionData.GetPositionData()
 
-	// If the file has been renamed, then this is a relevant part of the payload
-	oldFileName := positionData.OldFileName
-	if oldFileName == "" {
-		oldFileName = positionData.FileName
-	}
-
 	opt := &gitlab.PositionOptions{
 		PositionType: &positionData.Type,
 		StartSHA:     &positionData.StartCommitSHA,
 		HeadSHA:      &positionData.HeadCommitSHA,
 		BaseSHA:      &positionData.BaseCommitSHA,
 		NewPath:      &positionData.FileName,
-		OldPath:      &oldFileName,
+		OldPath:      &positionData.OldFileName,
 		NewLine:      positionData.NewLine,
 		OldLine:      positionData.OldLine,
 	}
 
-	if positionData.LineRange != nil {
-		shaFormat := "%x_%d_%d"
-		startFilenameSha := fmt.Sprintf(
-			shaFormat,
-			sha1.Sum([]byte(positionData.FileName)),
-			positionData.LineRange.StartRange.OldLine,
-			positionData.LineRange.StartRange.NewLine,
-		)
-		endFilenameSha := fmt.Sprintf(
-			shaFormat,
-			sha1.Sum([]byte(positionData.FileName)),
-			positionData.LineRange.EndRange.OldLine,
-			positionData.LineRange.EndRange.NewLine,
-		)
-		opt.LineRange = &gitlab.LineRangeOptions{
-			Start: &gitlab.LinePositionOptions{
-				Type:     &positionData.LineRange.StartRange.Type,
-				LineCode: &startFilenameSha,
-			},
-			End: &gitlab.LinePositionOptions{
-				Type:     &positionData.LineRange.EndRange.Type,
-				LineCode: &endFilenameSha,
-			},
-		}
+	shaFormat := "%x_%d_%d"
+	startFilenameSha := fmt.Sprintf(
+		shaFormat,
+		sha1.Sum([]byte(positionData.FileName)),
+		positionData.LineRange.Start.OldLine,
+		positionData.LineRange.Start.NewLine,
+	)
+	endFilenameSha := fmt.Sprintf(
+		shaFormat,
+		sha1.Sum([]byte(positionData.FileName)),
+		positionData.LineRange.End.OldLine,
+		positionData.LineRange.End.NewLine,
+	)
+
+	startOldLine, startNewLine := zeroInapplicableLine(positionData.LineRange.Start)
+	endOldLine, endNewLine := zeroInapplicableLine(positionData.LineRange.End)
+
+	opt.LineRange = &gitlab.LineRangeOptions{
+		Start: &gitlab.LinePositionOptions{
+			Type:     &positionData.LineRange.Start.Type,
+			LineCode: &startFilenameSha,
+			OldLine:  &startOldLine,
+			NewLine:  &startNewLine,
+		},
+		End: &gitlab.LinePositionOptions{
+			Type:     &positionData.LineRange.End.Type,
+			LineCode: &endFilenameSha,
+			OldLine:  &endOldLine,
+			NewLine:  &endNewLine,
+		},
 	}
 
 	return opt
 }
+
+/* zeroInapplicableLine returns a line_range endpoint's OldLine/NewLine with the side
+* that its Type doesn't apply to zeroed out: NewLine for a deleted ("old") line, OldLine
+* for an added ("new") line. Both stay real for an unmodified ("") or "expanded" line.
+* The unzeroed pair is still what the LineCode hash above is computed from - Gitlab
+* expects LineCode to encode the real old/new correspondence even when the displayed
+* OldLine or NewLine is zeroed. */
+func zeroInapplicableLine(position *PositionInfo) (oldLine int64, newLine int64) {
+	oldLine, newLine = position.OldLine, position.NewLine
+	switch position.Type {
+	case "old":
+		newLine = 0
+	case "new":
+		oldLine = 0
+	}
+	return oldLine, newLine
+}

cmd/app/comment_helpers_test.go

@@ -0,0 +1,65 @@
+package app
+
+import (
+	"testing"
+)
+
+func TestBuildCommentPosition(t *testing.T) {
+	makePositionData := func(startType string, startOld, startNew int64, endType string, endOld, endNew int64) PositionData {
+		return PositionData{
+			FileName:       "file.txt",
+			HeadCommitSHA:  "head-sha",
+			BaseCommitSHA:  "base-sha",
+			StartCommitSHA: "start-sha",
+			Type:           "text",
+			LineRange: &LineRange{
+				Start: &PositionInfo{Type: startType, OldLine: startOld, NewLine: startNew},
+				End:   &PositionInfo{Type: endType, OldLine: endOld, NewLine: endNew},
+			},
+		}
+	}
+
+	t.Run("zeroes NewLine for a deleted (\"old\") line, keeping LineCode's real pair", func(t *testing.T) {
+		positionData := makePositionData("", 4, 4, "old", 5, 5)
+		opt := buildCommentPosition(CommentWithPosition{PositionData: positionData})
+
+		assert(t, *opt.LineRange.End.OldLine, int64(5))
+		assert(t, *opt.LineRange.End.NewLine, int64(0))
+		assert(t, *opt.LineRange.End.LineCode, "5436437fa01a7d3e41d46741da54b451446774ca_5_5")
+	})
+
+	t.Run("zeroes OldLine for an added (\"new\") line, keeping LineCode's real pair", func(t *testing.T) {
+		positionData := makePositionData("", 4, 4, "new", 5, 5)
+		opt := buildCommentPosition(CommentWithPosition{PositionData: positionData})
+
+		assert(t, *opt.LineRange.End.OldLine, int64(0))
+		assert(t, *opt.LineRange.End.NewLine, int64(5))
+		assert(t, *opt.LineRange.End.LineCode, "5436437fa01a7d3e41d46741da54b451446774ca_5_5")
+	})
+
+	t.Run("keeps both lines real for an unmodified (\"\") line", func(t *testing.T) {
+		positionData := makePositionData("", 4, 4, "", 5, 6)
+		opt := buildCommentPosition(CommentWithPosition{PositionData: positionData})
+
+		assert(t, *opt.LineRange.End.OldLine, int64(5))
+		assert(t, *opt.LineRange.End.NewLine, int64(6))
+	})
+
+	t.Run("keeps both lines real for an expanded line", func(t *testing.T) {
+		positionData := makePositionData("", 4, 4, "expanded", 59, 61)
+		opt := buildCommentPosition(CommentWithPosition{PositionData: positionData})
+
+		assert(t, *opt.LineRange.End.OldLine, int64(59))
+		assert(t, *opt.LineRange.End.NewLine, int64(61))
+	})
+
+	t.Run("zeroes the start and end independently", func(t *testing.T) {
+		positionData := makePositionData("new", 0, 50, "", 60, 62)
+		opt := buildCommentPosition(CommentWithPosition{PositionData: positionData})
+
+		assert(t, *opt.LineRange.Start.OldLine, int64(0))
+		assert(t, *opt.LineRange.Start.NewLine, int64(50))
+		assert(t, *opt.LineRange.End.OldLine, int64(60))
+		assert(t, *opt.LineRange.End.NewLine, int64(62))
+	})
+}

cmd/app/comment_test.go

@@ -59,6 +59,10 @@ func TestPostComment(t *testing.T) {
 			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)

cmd/app/middleware_test.go

@@ -111,4 +111,59 @@ func TestValidatorMiddleware(t *testing.T) {
 		), request)
 		assert(t, data.Message, "Some message")
 	})
+	t.Run("Should reject a line_range with a missing endpoint instead of panicking", func(t *testing.T) {
+		payload := PostCommentRequest{
+			Comment: "Some comment",
+			PositionData: PositionData{
+				FileName:  "file.txt",
+				LineRange: &LineRange{}, // Start and End left nil
+			},
+		}
+		request := makeRequest(t, http.MethodPost, "/mr/comment", payload)
+		svc := middleware(
+			commentService{testProjectData, fakeCommentClient{}},
+			withMr(testProjectData, fakeMergeRequestLister{}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[PostCommentRequest]}),
+			withMethodCheck(http.MethodPost),
+		)
+		data, status := getFailData(t, svc, request)
+		assert(t, data.Message, "Invalid payload")
+		assert(t, data.Details, "Start is required; End is required")
+		assert(t, status, http.StatusBadRequest)
+	})
+	t.Run("Should reject a missing line_range when FileName is set", func(t *testing.T) {
+		payload := PostCommentRequest{
+			Comment: "Some comment",
+			PositionData: PositionData{
+				FileName: "file.txt",
+				// LineRange left nil entirely (not just an empty struct).
+			},
+		}
+		request := makeRequest(t, http.MethodPost, "/mr/comment", payload)
+		svc := middleware(
+			commentService{testProjectData, fakeCommentClient{}},
+			withMr(testProjectData, fakeMergeRequestLister{}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[PostCommentRequest]}),
+			withMethodCheck(http.MethodPost),
+		)
+		data, status := getFailData(t, svc, request)
+		assert(t, data.Message, "Invalid payload")
+		assert(t, data.Details, "The field 'LineRange' failed on validation on the 'required_with' tag")
+		assert(t, status, http.StatusBadRequest)
+	})
+	t.Run("Should allow a missing line_range when there is no FileName (unlinked comment)", func(t *testing.T) {
+		payload := PostCommentRequest{
+			Comment: "Some comment",
+			// PositionData is left zero-valued: no FileName, no LineRange.
+		}
+		request := makeRequest(t, http.MethodPost, "/mr/comment", payload)
+		svc := middleware(
+			commentService{testProjectData, fakeCommentClient{}},
+			withMr(testProjectData, fakeMergeRequestLister{}),
+			withPayloadValidation(methodToPayload{http.MethodPost: newPayload[PostCommentRequest]}),
+			withMethodCheck(http.MethodPost),
+		)
+		data := getSuccessData(t, svc, request)
+		assert(t, data.Message, "Comment created successfully")
+	})
 }

doc/gitlab.nvim.txt

@@ -127,10 +127,16 @@ And with pckr.nvim:
 NOTES ON DEPENDENCIES                               *gitlab.nvim.dependencies*
 
 `gitlab.nvim` uses the `diffview.nvim` plugin for showing the diffs in a MR.
-We recommend using `dlyongemallo`'s `diffview+` fork which is the de-facto
+We recommend using `dlyongemallo`'s `diffview+` fork which is an actively
 maintained version of the plugin with many fixes and improvements (e.g.,
-marking files as viewed). The original `sindrets/diffview.nvim` plugin will be
-supported by `gitlab.nvim` as long as the maintenance remains feasible.
+marking files as viewed). Importantly, it allows setting the same similarity
+threshold for detecting renamed files as is used by Gitlab (30%). When using
+the original
+[sindrets/diffview.nvim](https://github.com/sindrets/diffview.nvim) plugin,
+file renames may not be detected correctly and comments created on such files
+will contain incorrect metadata or may fail. Nevertheless the original
+`sindrets/diffview.nvim` plugin will be supported by `gitlab.nvim` as long as
+the maintenance remains feasible.
 
 Some plugin actions use Neovim’s |vim.ui.select()| picker, which looks much
 nicer if you use `dressing.nvim` or a similar UI plugin. To use Dressing,