From e5c1c1f99926079b5354fecaa3f5d521ad25fc0c Mon Sep 17 00:00:00 2001 From: Prarit Bhargava Date: Fri, 10 Dec 2021 13:28:02 -0500 Subject: [PATCH] cmd/note-common.go: Fix context comments on diffs krobelus noticed that it wasnt possible to comment on the context portion of a diff. For example, when commenting on line 6 of | newfile: Makefile oldfile: Makefile | @@ -5,6 +5,8 @@ SUBLEVEL = 0 | 5 5 EXTRAVERSION = | 6 6 NAME = Merciless Moray | 7 7 | 8 +# space | 9 + | 8 10 # | 9 11 # DRM backport version | 10 12 # it was not possible to comment on line 6 and lab would output an error. This is fixed by reworking the code to detect context diffs, however, this only appears to work on code shown before the difflines. There are several open GitLab API issues that report errors when commenting on Merge Request commits and it is likely that one of these is causing the error. ex) https://gitlab.com/gitlab-org/gitlab-foss/-/issues/28599 Rework and cleanup the code to allow for commenting on the context of diffs. Signed-off-by: Prarit Bhargava --- cmd/note_common.go | 133 +++++++++++++++++++++++++------------- internal/gitlab/gitlab.go | 18 ++++-- 2 files changed, 101 insertions(+), 50 deletions(-) diff --git a/cmd/note_common.go b/cmd/note_common.go index 9b649626..ebaed410 100644 --- a/cmd/note_common.go +++ b/cmd/note_common.go @@ -94,11 +94,9 @@ func noteRunFn(cmd *cobra.Command, args []string) { createNote(rn, isMR, int(idNum), msgs, filename, linebreak, commit, true) } -func createCommitNote(rn string, mrID int, sha string, newFile string, oldFile string, oldline int, newline int, comment string, block bool) { - linetype := "old" +func createCommitNote(rn string, mrID int, sha string, newFile string, oldFile string, linetype string, oldline int, newline int, comment string, block bool) { line := oldline if oldline == -1 { - linetype = "new" line = newline } @@ -145,63 +143,106 @@ func createCommitComments(project string, mrID int, commit string, body string, // tracking information (new line & old line number pairs, // and file information) scanner := bufio.NewScanner(strings.NewReader(body)) + lastDiffLine := "" + comments := "" newfile := "" oldfile := "" - oldLineNum := -1 - newLineNum := -1 - comments := "" + diffCut := 1 + for scanner.Scan() { - if strings.HasPrefix(scanner.Text(), "| newfile:") { - if comments != "" { - createCommitNote(project, mrID, commit, newfile, oldfile, oldLineNum, newLineNum, comments, block) - comments = "" + line := scanner.Text() + + if !strings.HasPrefix(line, "| ") { + comments += "\n" + line + continue + } + + if comments != "" && lastDiffLine == "" { + log.Fatal("Cannot comment on first line of commit (context unknown).") + } + + if comments != "" { + linetype := "" + oldLineNum := -1 + newLineNum := -1 + + // parse lastDiffLine + f := strings.Fields(strings.TrimSpace(lastDiffLine)) + + // The output can be, for example: + // | # # [no +/-] < context comment + // | # # + < newline comment + // | # # - < oldline comment + // | # - < oldline comment + // | # + < newline comment + + // f[0] is always a "|" + // f[1] will always be a number + val1, _ := strconv.Atoi(f[1]) + val2, err := strconv.Atoi(f[2]) + + if err == nil { // f[2] is a number + if len(f) <= 3 { // f[3] does not exist + oldLineNum = val1 + newLineNum = val2 + linetype = "context" + } else { + newLineNum = val2 + switch { + case strings.HasPrefix(f[3], "+"): + linetype = "new" + case strings.HasPrefix(f[3], "-"): + linetype = "old" + default: + linetype = "context" + } + } + } else { // f[2] is not a number + switch { + case strings.HasPrefix(f[2], "+"): + newLineNum = val1 + linetype = "new" + case strings.HasPrefix(f[2], "-"): + oldLineNum = val1 + linetype = "old" + default: + panic("unknown string in diff") + } } + + createCommitNote(project, mrID, commit, newfile, oldfile, linetype, oldLineNum, newLineNum, comments, block) + comments = "" + } + + f := strings.Fields(strings.TrimSpace(line)) + if f[1] == "@@" { + // In GitLab diff output, the leading "@" symbol indicates where + // the metadata ends. This is true even if passing over a digit + // boundary (ie going from line 99 to 100). This location can + // be used to truncate the lines to only include the metadata. + diffCut = strings.Index(line, "@") + 1 + lastDiffLine = "" + continue + } + + if f[1] == "newfile:" { // read filename - f := strings.Split(scanner.Text(), " ") + f := strings.Split(line, " ") 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 - } - } - } + continue + } + + if len(line) > diffCut { + lastDiffLine = line[0:diffCut] } else { - // this is a comment (combine for a filename) - comments = comments + "\n" + scanner.Text() + lastDiffLine = line } } - } func noteGetState(rn string, isMR bool, idNum int) (state string) { diff --git a/internal/gitlab/gitlab.go b/internal/gitlab/gitlab.go index 55f7483b..3793c3f9 100644 --- a/internal/gitlab/gitlab.go +++ b/internal/gitlab/gitlab.go @@ -1718,12 +1718,18 @@ func CreateCommitComment(projID string, sha string, newFile string, oldFile stri PositionType: "text", } - if linetype == "old" { + switch linetype { + case "new": + position.NewPath = newFile + position.NewLine = line + case "old": position.OldPath = oldFile position.OldLine = line - } else { + case "context": position.NewPath = newFile position.NewLine = line + position.OldPath = oldFile + position.OldLine = line } opt := &gitlab.CreateCommitDiscussionOptions{ @@ -1760,9 +1766,13 @@ func CreateMergeRequestCommitDiscussion(projID string, id int, sha string, newFi PositionType: "text", } - if linetype == "new" { + switch linetype { + case "new": + position.NewLine = line + case "old": + position.OldLine = line + case "context": position.NewLine = line - } else { position.OldLine = line }