mr discussion: allow to comment on specific lines in a diff

GitLab allows to comment on lines within a commit's diff [1] (or even
ranges of lines[2]).  This is standard practise when using the web
interface, and arguably friendlier to MR authors because it gives
context information in a standardized way.

Teach "lab mr discussion" a a new parameter "--position" that takes
the file, line type (added, deleted, context) and two line numbers
(before and after the diff). It looks like this:

	lab mr discussion --commit=my-commit --position=README.md:+101,103

For added and deleted lines we only need one line number (the one
in the "new" and the "old" version of the file, respectively), but
due to a deficiency in the GitLab API[3] we need to pass both line
numbers for context lines.

[1]: https://docs.gitlab.com/ee/api/discussions.html#create-new-merge-request-thread
[2]: commenting on ranges of lines is not yet supported.
     That can be added in future supplementing the single-line syntax
     +<old_line>,<new_line>
     with a range variant
     +<start_old_line>,<start_new_line>_+<end_old_line>,<end_new_line>
     (where + can be - or space of course).
[3]: https://gitlab.com/gitlab-org/gitlab/-/issues/325161

For reference, I've been testing this with
[Tig](https://github.com/jonas/tig), using the following binding in
my ~/.tigrc:

	bind diff C !tiglab %(remote) %(branch) %(commit) %(file) %(file_old) %(text) %(lineno) %(lineno_old)

where "tiglab" is this small wrapper script:

	#!/bin/sh
	set -eu

	remote=$1
	branch=$2
	commit=$3
	file=$4
	file_old=$5
	text=$6
	lineno=$7
	lineno_old=$8

	line_type=${text%"${text#?}"} # First character of the diff line.
	# Use the new filename, except if we are commenting on a deleted file.
	if [ -z "$file" ] || [ "$file" = /dev/null ]; then
		file=$file_old
	fi

	lab mr discussion "$remote" "$branch" --commit="$commit" --position="${file}:${line_type}${lineno_old},${lineno}"

cmd/mr_discussion.go

@@ -1,6 +1,7 @@
 package cmd
 
 import (
+	"bytes"
 	"fmt"
 	"io/ioutil"
 	"runtime"
@@ -29,7 +30,7 @@ var mrCreateDiscussionCmd = &cobra.Command{
 		lab mr discussion my-topic-branch
 		lab mr discussion origin 123
 		lab mr discussion origin my-topic-branch
-		`),
+		lab mr discussion --commit abcdef123456 --position=main.c:+100,100`),
 	PersistentPreRun: labPersistentPreRun,
 	Run: func(cmd *cobra.Command, args []string) {
 		rn, mrNum, err := parseArgsWithGitBranchMR(args)
@@ -52,6 +53,47 @@ var mrCreateDiscussionCmd = &cobra.Command{
 			log.Fatal(err)
 		}
 
+		position, err := cmd.Flags().GetString("position")
+		if err != nil {
+			log.Fatal(err)
+		}
+		var posFile string
+		var posLineType byte
+		var posLineNumberNew, posLineNumberOld uint64
+		if position != "" {
+			colonOffset := strings.LastIndex(position, ":")
+			positionUserError := "argument to --position must match <file>:[+- ]<old_line>,<new_line>"
+			if colonOffset == -1 {
+				log.Fatal(positionUserError + `: missing ":"`)
+			}
+			posFile = position[:colonOffset]
+			lineTypeOffset := colonOffset + 1
+			if lineTypeOffset == len(position) {
+				log.Fatal(positionUserError + `: expected one of "+- ", found end of string`)
+			}
+			posLineType = position[lineTypeOffset]
+			if bytes.IndexByte([]byte("+- "), posLineType) == -1 {
+				log.Fatal(positionUserError + fmt.Sprintf(`: expected one of "+- ", found %q`, posLineType))
+			}
+			oldLineOffset := colonOffset + 2
+			if oldLineOffset == len(position) {
+				log.Fatal(positionUserError + ": missing line numbers")
+			}
+			commaOffset := strings.LastIndex(position, ",")
+			if commaOffset == -1 || commaOffset < colonOffset {
+				log.Fatal(positionUserError + `: missing "," to separate line numbers`)
+			}
+			posLineNumberOld, err = strconv.ParseUint(position[oldLineOffset:commaOffset], 10, 32)
+			if err != nil {
+				log.Fatal(positionUserError + ":error parsing <old_line>: " + err.Error())
+			}
+			newNumberOffset := commaOffset + 1
+			posLineNumberNew, err = strconv.ParseUint(position[newNumberOffset:], 10, 32)
+			if err != nil {
+				log.Fatal(positionUserError + ":error parsing <new_line>: " + err.Error())
+			}
+		}
+
 		state := noteGetState(rn, true, int(mrNum))
 
 		body := ""
@@ -61,7 +103,8 @@ var mrCreateDiscussionCmd = &cobra.Command{
 				log.Fatal(err)
 			}
 			body = string(content)
-		} else if commit == "" {
+		} else if position != "" || commit == "" {
+			// TODO If we are commenting on a specific position in the diff, we should include some context in the template.
 			body, err = mrDiscussionMsg(int(mrNum), state, commit, msgs, "\n")
 			if err != nil {
 				_, f, l, _ := runtime.Caller(0)
@@ -78,12 +121,54 @@ var mrCreateDiscussionCmd = &cobra.Command{
 			return
 		}
 
+		var notePos *gitlab.NotePosition
+		if position != "" {
+			if commit == "" {
+				// We currently only support "--position" when commenting on individual commits within an MR.
+				// GitLab also allows to comment on the sum of all changes of an MR.
+				// To do so, we'd need to fill the NotePosition below with the target branch as "base SHA" and the source branch as "head SHA".
+				// However, commenting on individual commits within an MR is superior since it gives more information to the MR author.
+				// Additionally, commenting on the sum of all changes is only useful when the changes come with a messy history.
+				// We shouldn't encourage that  - GitLab already does ;).
+				log.Fatal("--position currently requires --commit")
+			}
+			parentSHA, err := git.RevParse(commit + "~")
+			if err != nil {
+				log.Fatal(err)
+			}
+			// WORKAROUND For added (-) and deleted (+) lines we only need one line number parameter, but for context lines we need both. https://gitlab.com/gitlab-org/gitlab/-/issues/325161
+			newLine := posLineNumberNew
+			if posLineType == '-' {
+				newLine = 0
+			}
+			oldLine := posLineNumberOld
+			if posLineType == '+' {
+				oldLine = 0
+			}
+			notePos = &gitlab.NotePosition{
+				BaseSHA:      parentSHA,
+				StartSHA:     parentSHA,
+				HeadSHA:      commit,
+				PositionType: "text",
+				NewPath:      posFile,
+				NewLine:      int(newLine),
+				OldPath:      posFile,
+				OldLine:      int(oldLine),
+			}
+		}
+
 		if body == "" {
 			log.Fatal("aborting discussion due to empty discussion msg")
 		}
+		var commitID *string
+		if commit != "" {
+			commitID = &commit
+		}
 
 		discussionURL, err := lab.MRCreateDiscussion(rn, int(mrNum), &gitlab.CreateMergeRequestDiscussionOptions{
-			Body: &body,
+			Body:     &body,
+			CommitID: commitID,
+			Position: notePos,
 		})
 		if err != nil {
 			log.Fatal(err)
@@ -124,6 +209,33 @@ func init() {
 	mrCreateDiscussionCmd.Flags().StringP("file", "F", "", "use the given file as the message")
 	mrCreateDiscussionCmd.Flags().StringP("commit", "c", "", "start a thread on a commit")
 
+	mrCreateDiscussionCmd.Flags().StringP("position", "", "", heredoc.Doc(`
+		start a thread on a specific line of the diff
+		argument must be of the form <file>":"["+" | "-" | " "]<old_line>","<new_line>
+		that is, the file name, followed by the line type - one of "+" (added line),
+		"-" (deleted line) or a space character (context line) - followed by
+		the line number in the old version of the file, a ",", and finally
+		the line number in the new version of the file. If the line type is "+", then
+		<old_line> is ignored. If the line type is "-", then <new_line> is ignored.
+
+		Here's an example diff that explains how to determine the old/new line numbers:
+	    
+			--- a/README.md		old	new
+			+++ b/README.md
+			@@ -100,3 +100,4 @@
+			 pre-context line	100	100
+			-deleted line		101	101
+			+added line 1		101	102
+			+added line 2		101	103
+			 post-context line	102	104
+
+		# Comment on "deleted line":
+		lab mr discussion --commit=commit-id --position=README.md:-101,101
+		# Comment on "added line 2":
+		lab mr discussion --commit=commit-id --position=README.md:+101,103
+		# Comment on the "post-context line":
+		lab mr discussion --commit=commit-id --position=README.md:\ 102,104`))
+
 	mrCmd.AddCommand(mrCreateDiscussionCmd)
 	carapace.Gen(mrCreateDiscussionCmd).PositionalCompletion(
 		action.Remotes(),

internal/git/git.go

@@ -153,6 +153,17 @@ func CurrentBranch() (string, error) {
 	return strings.TrimSpace(string(branch)), nil
 }
 
+// RevParse returns the output of "git rev-parse".
+func RevParse(args ...string) (string, error) {
+	cmd := New(append([]string{"rev-parse"}, args...)...)
+	cmd.Stdout = nil
+	d, err := cmd.Output()
+	if err != nil {
+		return "", err
+	}
+	return strings.TrimSpace(string(d)), nil
+}
+
 // UpstreamBranch returns the upstream of the specified branch
 func UpstreamBranch(branch string) (string, error) {
 	upstreamBranch, err := gitconfig.Local("branch." + branch + ".merge")