From 443fd3da664056b5c8608c160c1dff7530658e29 Mon Sep 17 00:00:00 2001 From: Prarit Bhargava Date: Sat, 3 Jul 2021 16:57:46 -0400 Subject: [PATCH] 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 --- cmd/issue_note.go | 2 + cmd/mr_approve.go | 2 +- cmd/mr_discussion.go | 20 +++++- cmd/mr_note.go | 4 +- cmd/mr_unapprove.go | 2 +- cmd/note_common.go | 138 ++++++++++++++++++++++++++++++++++++-- cmd/show_common.go | 47 ++++++++----- internal/gitlab/gitlab.go | 100 ++++++++++++++++++++++++++- 8 files changed, 284 insertions(+), 31 deletions(-) diff --git a/cmd/issue_note.go b/cmd/issue_note.go index 8e6cc135..526c0dc7 100644 --- a/cmd/issue_note.go +++ b/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( diff --git a/cmd/mr_approve.go b/cmd/mr_approve.go index 6d898d7f..cd6a9323 100644 --- a/cmd/mr_approve.go +++ b/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) diff --git a/cmd/mr_discussion.go b/cmd/mr_discussion.go index efbc1d72..830f64ea 100644 --- a/cmd/mr_discussion.go +++ b/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,6 +44,11 @@ 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) @@ -50,12 +56,21 @@ var mrCreateDiscussionCmd = &cobra.Command{ 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 ; 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( diff --git a/cmd/mr_note.go b/cmd/mr_note.go index 784c46e1..5b237ed0 100644 --- a/cmd/mr_note.go +++ b/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( diff --git a/cmd/mr_unapprove.go b/cmd/mr_unapprove.go index 61fc51dd..e84d88cf 100644 --- a/cmd/mr_unapprove.go +++ b/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) diff --git a/cmd/note_common.go b/cmd/note_common.go index 198dda0b..65424cb5 100644 --- a/cmd/note_common.go +++ b/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, diff --git a/cmd/show_common.go b/cmd/show_common.go index 283c7881..11b659ef 100644 --- a/cmd/show_common.go +++ b/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("") diff --git a/internal/gitlab/gitlab.go b/internal/gitlab/gitlab.go index 6456c44d..ba125d2c 100644 --- a/internal/gitlab/gitlab.go +++ b/internal/gitlab/gitlab.go @@ -1784,7 +1784,9 @@ func GetCommitDiff(project string, sha string) ([]*gitlab.Diff, error) { for { ds, resp, err := lab.Commits.GetCommitDiff(p.ID, sha, opt) if err != nil { - fmt.Println(err) + if resp.StatusCode == 404 { + log.Fatalf("Cannot find diff for commit %s. Verify the commit ID or add more characters to the commit ID.", sha) + } return nil, err } @@ -1801,3 +1803,99 @@ func GetCommitDiff(project string, sha string) ([]*gitlab.Diff, error) { return diffs, nil } + +func CreateCommitComment(project string, sha string, newFile string, oldFile string, line int, linetype string, comment string) (string, error) { + p, err := FindProject(project) + if err != nil { + return "", err + } + + // Ideally want to use lab.Commits.PostCommitComment, however, + // that API only support comments on linetype=new. + // + // https://gitlab.com/gitlab-org/gitlab/-/issues/335337 + + commitInfo, err := GetCommit(p.ID, sha) + if err != nil { + fmt.Printf("Could not get diff for commit %s.\n", sha) + return "", err + } + + if len(commitInfo.ParentIDs) > 1 { + log.Fatalf("Commit %s has mulitple parents. This interface cannot be used for comments.\n", sha) + return "", err + } + + position := gitlab.NotePosition{ + BaseSHA: commitInfo.ParentIDs[0], + StartSHA: commitInfo.ParentIDs[0], + HeadSHA: sha, + PositionType: "text", + } + + if linetype == "old" { + position.OldPath = oldFile + position.OldLine = line + } else { + position.NewPath = newFile + position.NewLine = line + } + + opt := &gitlab.CreateCommitDiscussionOptions{ + Body: &comment, + Position: &position, + } + + commitDiscussion, _, err := lab.Discussions.CreateCommitDiscussion(p.ID, sha, opt) + if err != nil { + return "", err + } + + return fmt.Sprintf("%s#note_%d", commitInfo.WebURL, commitDiscussion.Notes[0].ID), nil +} + +func CreateMergeRequestCommitDiscussion(project string, mrID int, sha string, newFile string, oldFile string, line int, linetype string, comment string) (string, error) { + p, err := FindProject(project) + if err != nil { + return "", err + } + + commitInfo, err := GetCommit(p.ID, sha) + if err != nil { + fmt.Printf("Could not get diff for commit %s.\n", sha) + return "", err + } + + if len(commitInfo.ParentIDs) > 1 { + log.Fatalf("Commit %s has mulitple parents. This interface cannot be used for comments.\n", sha) + return "", err + } + + position := gitlab.NotePosition{ + NewPath: newFile, + OldPath: oldFile, + BaseSHA: commitInfo.ParentIDs[0], + StartSHA: commitInfo.ParentIDs[0], + HeadSHA: sha, + PositionType: "text", + } + + if linetype == "new" { + position.NewLine = line + } else { + position.OldLine = line + } + + opt := &gitlab.CreateMergeRequestDiscussionOptions{ + Body: &comment, + Position: &position, + } + + discussion, _, err := lab.Discussions.CreateMergeRequestDiscussion(p.ID, mrID, opt) + if err != nil { + return "", err + } + + note := discussion.Notes[0] + return fmt.Sprintf("%s/merge_requests/%d#note_%d", p.WebURL, note.NoteableIID, note.ID), nil +}