rhstatus: Add commit comment functionality

The GitLab WebUI allows users to comment directly on commits.  These comments
appear both in the comment view and the MR view.  This functionality can be
implemented via the lab.Commits.PostCommitComment() function, however, the
function only supports comments on "new" lines of code.  After discussions
with GitLab they have suggested using lab.Discussions.CreateCommitDiscussion()
as a workaround [1].  This solution has a minor issue in that it will not work
with commits that have multiple parents.

Implement the ability for users to comment directly on commits and to
block MRs with commit comments.

[1] https://gitlab.com/gitlab-org/gitlab/-/issues/335337#note_643291819

Signed-off-by: Prarit Bhargava <prarit@redhat.com>

cmd/issue_note.go

@@ -28,6 +28,8 @@ func init() {
 	issueNoteCmd.Flags().Bool("quote", false, "quote note in reply")
 	issueNoteCmd.Flags().Bool("resolve", false, "[unused in issue note command]")
 	issueNoteCmd.Flags().MarkHidden("resolve")
+	issueNoteCmd.Flags().StringP("commit", "", "", "[unused in issue note command]")
+	issueNoteCmd.Flags().MarkHidden("commit")
 
 	issueCmd.AddCommand(issueNoteCmd)
 	carapace.Gen(issueNoteCmd).PositionalCompletion(

cmd/mr_approve.go

@@ -65,7 +65,7 @@ var mrApproveCmd = &cobra.Command{
 				log.Fatal(err)
 			}
 
-			createNote(rn, true, int(id), msgs, filename, linebreak)
+			createNote(rn, true, int(id), msgs, filename, linebreak, "")
 		}
 
 		fmt.Printf("Merge Request !%d approved\n", id)

cmd/mr_discussion.go

@@ -25,7 +25,8 @@ var mrCreateDiscussionCmd = &cobra.Command{
 	Example: heredoc.Doc(`
 		lab mr discussion origin
 		lab mr discussion my_remote -m "discussion comment"
-		lab mr discussion upstream -F test_file.txt`),
+		lab mr discussion upstream -F test_file.txt
+		lab mr discussion --commit abcdef123456`),
 	PersistentPreRun: labPersistentPreRun,
 	Run: func(cmd *cobra.Command, args []string) {
 		rn, mrNum, err := parseArgsWithGitBranchMR(args)
@@ -43,19 +44,33 @@ var mrCreateDiscussionCmd = &cobra.Command{
 			log.Fatal(err)
 		}
 
+		commit, err := cmd.Flags().GetString("commit")
+		if err != nil {
+			log.Fatal(err)
+		}
+
 		body := ""
 		if filename != "" {
 			content, err := ioutil.ReadFile(filename)
 			if err != nil {
 				log.Fatal(err)
 			}
 			body = string(content)
-		} else {
+		} else if commit == "" {
 			body, err = mrDiscussionMsg(msgs)
 			if err != nil {
 				_, f, l, _ := runtime.Caller(0)
 				log.Fatal(f+":"+strconv.Itoa(l)+" ", err)
 			}
+		} else {
+			body = getCommitBody(rn, commit)
+			body, err = noteMsg(nil, true, body)
+			if err != nil {
+				_, f, l, _ := runtime.Caller(0)
+				log.Fatal(f+":"+strconv.Itoa(l)+" ", err)
+			}
+			createCommitComments(rn, int(mrNum), commit, body, true)
+			return
 		}
 
 		if body == "" {
@@ -117,6 +132,7 @@ func mrDiscussionText() (string, error) {
 func init() {
 	mrCreateDiscussionCmd.Flags().StringSliceP("message", "m", []string{}, "use the given <msg>; multiple -m are concatenated as separate paragraphs")
 	mrCreateDiscussionCmd.Flags().StringP("file", "F", "", "use the given file as the message")
+	mrCreateDiscussionCmd.Flags().StringP("commit", "c", "", "start a thread on a commit")
 
 	mrCmd.AddCommand(mrCreateDiscussionCmd)
 	carapace.Gen(mrCreateDiscussionCmd).PositionalCompletion(

cmd/mr_note.go

@@ -17,7 +17,8 @@ var mrNoteCmd = &cobra.Command{
 		lab mr note a_remote -F test_file --force-linebreak
 		lab mr note upstream -m "A helpfull comment"
 		lab mr note upstream:613278106 --quote
-		lab mr note upstream:613278107 --resolve`),
+		lab mr note upstream:613278107 --resolve
+		lab mr note upstream:613278108 --commit abcdef123456`),
 	PersistentPreRun: labPersistentPreRun,
 	Run:              noteRunFn,
 }
@@ -28,6 +29,7 @@ func init() {
 	mrNoteCmd.Flags().Bool("force-linebreak", false, "append 2 spaces to the end of each line to force markdown linebreaks")
 	mrNoteCmd.Flags().Bool("quote", false, "quote note in reply")
 	mrNoteCmd.Flags().Bool("resolve", false, "mark thread resolved")
+	mrNoteCmd.Flags().StringP("commit", "c", "", "start a thread on a commit")
 
 	mrCmd.AddCommand(mrNoteCmd)
 	carapace.Gen(mrNoteCmd).PositionalCompletion(

cmd/mr_unapprove.go

@@ -65,7 +65,7 @@ var mrUnapproveCmd = &cobra.Command{
 				log.Fatal(err)
 			}
 
-			createNote(rn, true, int(id), msgs, filename, linebreak)
+			createNote(rn, true, int(id), msgs, filename, linebreak, "")
 		}
 
 		fmt.Printf("Merge Request !%d unapproved\n", id)

cmd/note_common.go

@@ -1,6 +1,7 @@
 package cmd
 
 import (
+	"bufio"
 	"bytes"
 	"fmt"
 	"io/ioutil"
@@ -66,6 +67,11 @@ func noteRunFn(cmd *cobra.Command, args []string) {
 		log.Fatal(err)
 	}
 
+	commit, err := cmd.Flags().GetString("commit")
+	if err != nil {
+		log.Fatal(err)
+	}
+
 	if reply != 0 {
 		resolve, err := cmd.Flags().GetBool("resolve")
 		if err != nil {
@@ -85,10 +91,119 @@ func noteRunFn(cmd *cobra.Command, args []string) {
 		return
 	}
 
-	createNote(rn, isMR, int(idNum), msgs, filename, linebreak)
+	createNote(rn, isMR, int(idNum), msgs, filename, linebreak, commit)
 }
 
-func createNote(rn string, isMR bool, idNum int, msgs []string, filename string, linebreak bool) {
+func createCommitNote(rn string, mrID int, sha string, newFile string, oldFile string, oldline int, newline int, comment string, block bool) {
+	linetype := "old"
+	line := oldline
+	if oldline == -1 {
+		linetype = "new"
+		line = newline
+	}
+
+	if block {
+		webURL, err := lab.CreateMergeRequestCommitDiscussion(rn, mrID, sha, newFile, oldFile, line, linetype, comment)
+		if err != nil {
+			log.Fatal(err)
+		}
+		fmt.Println(webURL)
+		return
+	}
+
+	webURL, err := lab.CreateCommitComment(rn, sha, newFile, oldFile, line, linetype, comment)
+	if err != nil {
+		log.Fatal(err)
+	}
+	fmt.Println(webURL)
+}
+
+func getCommitBody(project string, commit string) (body string) {
+	//body is going to be the commit diff
+	ds, err := lab.GetCommitDiff(project, commit)
+	if err != nil {
+		fmt.Printf("    Could not get diff for commit %s.\n", commit)
+		log.Fatal(err)
+	}
+
+	if len(ds) == 0 {
+		log.Fatal("    No diff found for %s.", commit)
+	}
+
+	for _, d := range ds {
+		body = body + fmt.Sprintf("| newfile: %s oldfile: %s\n", d.NewPath, d.OldPath)
+		body = body + displayDiff(d.Diff, 0, 0, true)
+	}
+
+	return body
+}
+
+func createCommitComments(project string, mrID int, commit string, body string, block bool) {
+	// Go through the body line-by-line and find lines that do not
+	// begin with |.  These lines are comments that have been made
+	// on the patch.  The lines that begin with | contain patch
+	// tracking information (new line & old line number pairs,
+	// and file information)
+	scanner := bufio.NewScanner(strings.NewReader(body))
+	newfile := ""
+	oldfile := ""
+	oldLineNum := -1
+	newLineNum := -1
+	comments := ""
+	for scanner.Scan() {
+		if strings.HasPrefix(scanner.Text(), "| newfile:") {
+			if comments != "" {
+				createCommitNote(project, mrID, commit, newfile, oldfile, oldLineNum, newLineNum, comments, block)
+				comments = ""
+			}
+			// read filename
+			f := strings.Split(scanner.Text(), " ")
+			newfile = f[2]
+			if len(f) < 5 {
+				oldfile = ""
+			} else {
+				oldfile = f[4]
+			}
+		} else if strings.HasPrefix(scanner.Text(), "|") {
+			if comments != "" {
+				createCommitNote(project, mrID, commit, newfile, oldfile, oldLineNum, newLineNum, comments, block)
+				comments = ""
+			}
+			// read line numbers
+			fs := strings.Split(scanner.Text(), " ")
+
+			oldLineNum = -1
+			newLineNum = -1
+			for _, f := range fs {
+				if f == "" || f == "|" || f == "@@" {
+					continue
+				}
+				val, err := strconv.Atoi(f)
+				if err != nil {
+					// NaN
+					if strings.HasPrefix(f, "+") {
+						newLineNum = oldLineNum
+						oldLineNum = -1
+					}
+					break
+				} else {
+					// Number
+					if oldLineNum == -1 {
+						oldLineNum = val
+					} else {
+						newLineNum = val
+						break
+					}
+				}
+			}
+		} else {
+			// this is a comment (combine for a filename)
+			comments = comments + "\n" + scanner.Text()
+		}
+	}
+
+}
+func createNote(rn string, isMR bool, idNum int, msgs []string, filename string, linebreak bool, commit string) {
 	var err error
 
 	body := ""
@@ -111,7 +226,12 @@ func createNote(rn string, isMR bool, idNum int, msgs []string, filename string,
 				"merged": "MERGED",
 			}[mr.State]
 
-			body = fmt.Sprintf("\n# This comment is being applied to %s Merge Request %d.", state, idNum)
+			if commit != "" {
+				body = getCommitBody(rn, commit)
+				body += fmt.Sprintf("\n# This comment is being applied to %s Merge Request %d commit %s.\n# Do not delete patch tracking lines that begin with '|'.", state, idNum, commit[:len(commit)])
+			} else {
+				body += fmt.Sprintf("\n# This comment is being applied to %s Merge Request %d.", state, idNum)
+			}
 		} else {
 			issue, err := lab.IssueGet(rn, idNum)
 			if err != nil {
@@ -137,7 +257,7 @@ func createNote(rn string, isMR bool, idNum int, msgs []string, filename string,
 		log.Fatal("aborting note due to empty note msg")
 	}
 
-	if linebreak {
+	if linebreak && commit == "" {
 		body = textToMarkdown(body)
 	}
 
@@ -146,9 +266,13 @@ func createNote(rn string, isMR bool, idNum int, msgs []string, filename string,
 	)
 
 	if isMR {
-		noteURL, err = lab.MRCreateNote(rn, idNum, &gitlab.CreateMergeRequestNoteOptions{
-			Body: &body,
-		})
+		if commit != "" {
+			createCommitComments(rn, int(idNum), commit, body, false)
+		} else {
+			noteURL, err = lab.MRCreateNote(rn, idNum, &gitlab.CreateMergeRequestNoteOptions{
+				Body: &body,
+			})
+		}
 	} else {
 		noteURL, err = lab.IssueCreateNote(rn, idNum, &gitlab.CreateIssueNoteOptions{
 			Body: &body,

cmd/show_common.go

@@ -34,25 +34,25 @@ func maxPadding(x int, y int) int {
 
 // printDiffLine does a color print of a diff lines.  Red lines are removals
 // and green lines are additions.
-func printDiffLine(strColor string, maxChars int, sOld string, sNew string, ltext string) {
+func printDiffLine(strColor string, maxChars int, sOld string, sNew string, ltext string) string {
 
 	switch strColor {
-	case "":
-		fmt.Printf("|%*s %*s %s\n", maxChars, sOld, maxChars, sNew, ltext)
 	case "green":
-		color.Green("|%*s %*s %s\n", maxChars, sOld, maxChars, sNew, ltext)
+		return color.GreenString("|%*s %*s %s\n", maxChars, sOld, maxChars, sNew, ltext)
 	case "red":
-		color.Red("|%*s %*s %s\n", maxChars, sOld, maxChars, sNew, ltext)
+		return color.RedString("|%*s %*s %s\n", maxChars, sOld, maxChars, sNew, ltext)
 	}
+	return fmt.Sprintf("|%*s %*s %s\n", maxChars, sOld, maxChars, sNew, ltext)
 }
 
 // displayDiff displays the diff referenced in a discussion
-func displayDiff(diff string, newLine int, oldLine int) {
+func displayDiff(diff string, newLine int, oldLine int, outputAll bool) string {
 	var (
-		oldLineNum   int = 0
-		newLineNum   int = 0
-		maxChars     int
-		output       bool = false
+		oldLineNum int = 0
+		newLineNum int = 0
+		maxChars   int
+		output     bool   = false
+		diffOutput string = ""
 	)
 
 	scanner := bufio.NewScanner(strings.NewReader(diff))
@@ -98,6 +98,11 @@ func displayDiff(diff string, newLine int, oldLine int) {
 				output = false
 			}
 
+			if outputAll {
+				mrShowNoColorDiff = true
+				output = true
+			}
+
 			// padding to align diff output (depends on the line numbers' length)
 			// The patch can have, for example, either
 			// @@ -1 +1 @@
@@ -141,14 +146,17 @@ func displayDiff(diff string, newLine int, oldLine int) {
 		if mrShowNoColorDiff {
 			strColor = ""
 		}
-		if newLine != 0 {
+		if outputAll {
+			diffOutput += printDiffLine(strColor, maxChars, sOld, sNew, ltext)
+		} else if newLine != 0 {
 			if newLineNum <= newLine && newLineNum >= (newLine-4) {
-				printDiffLine(strColor, maxChars, sOld, sNew, ltext)
+				diffOutput += printDiffLine(strColor, maxChars, sOld, sNew, ltext)
 			}
 		} else if oldLineNum <= oldLine && oldLineNum >= (oldLine-4) {
-			printDiffLine(strColor, maxChars, sOld, sNew, ltext)
+			diffOutput += printDiffLine(strColor, maxChars, sOld, sNew, ltext)
 		}
 	}
+	return diffOutput
 }
 
 func displayCommitDiscussion(project string, idNum int, note *gitlab.Note) {
@@ -168,7 +176,7 @@ func displayCommitDiscussion(project string, idNum int, note *gitlab.Note) {
 	// Get a unified diff for the entire file
 	ds, err := lab.GetCommitDiff(project, note.CommitID)
 	if err != nil {
-		fmt.Printf("    Could not get diff for commit %d.\n", note.CommitID)
+		fmt.Printf("    Could not get diff for commit %s.\n", note.CommitID)
 		return
 	}
 
@@ -188,11 +196,14 @@ func displayCommitDiscussion(project string, idNum int, note *gitlab.Note) {
 
 	for _, d := range ds {
 		if note.Position.NewPath == d.NewPath && note.Position.OldPath == d.OldPath {
-			if note.Position.LineRange == nil {
-				displayDiff(d.Diff, note.Position.NewLine, note.Position.OldLine)
-			} else {
-				displayDiff(d.Diff, note.Position.LineRange.StartRange.NewLine, note.Position.LineRange.StartRange.OldLine)
+			newLine := note.Position.NewLine
+			oldLine := note.Position.OldLine
+			if note.Position.LineRange != nil {
+				newLine = note.Position.LineRange.StartRange.NewLine
+				oldLine = note.Position.LineRange.StartRange.OldLine
 			}
+			diffstring := displayDiff(d.Diff, newLine, oldLine, false)
+			fmt.Printf(diffstring)
 		}
 	}
 	fmt.Println("")