Repository: reviewdog/reviewdog
Pull request #538 (2020-05-04 13:47:54)

gitlab: support -filter-mode=[file/diff_context]
GitLab returns an error if old path/line is not specified where
 appropriate.


Patch:
diff --git a/.github/workflows/reviewdog.yml b/.github/workflows/reviewdog.yml
index 7d7b4b79..e3baac09 100644
--- a/.github/workflows/reviewdog.yml
+++ b/.github/workflows/reviewdog.yml
@@ -120,7 +120,7 @@ jobs:
         env:
           REVIEWDOG_GITHUB_API_TOKEN: ${{ secrets.GITHUB_TOKEN }}
         run: |
-          golint | reviewdog -f=golint -name=golint-pr-check-filter-mode-file -reporter=github-pr-check -filter-mode=file
+          golint | reviewdog -f=golint -name=golint-pr-check-filter-mode-file -reporter=github-pr-check -filter-mode=file -level=warning
 
   golangci-lint:
     if: github.event_name == 'pull_request'
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 09ec31e8..dc485724 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1,3 +1,6 @@
+# Go to https://gitlab.com/reviewdog/reviewdog/-/settings/repository#js-push-remote-settings
+# to sync the repository manually.
+
 # Open merge request automatically and run reviewdog against created MR.
 stages:
   - openMr
@@ -37,6 +40,6 @@ Reviewdog:
     - go install ./cmd/reviewdog
     - ( cd linters && go get golang.org/x/lint/golint )
   script:
-    - golint ./... | reviewdog -f=golint -name=golint-discussion -reporter=gitlab-mr-discussion
+    - golint ./... | reviewdog -f=golint -name=golint-discussion -reporter=gitlab-mr-discussion -filter-mode=file
     - golint ./... | reviewdog -f=golint -name=golint-commit -reporter=gitlab-mr-commit
-    - cd ./_testdata/ && golint ./... | reviewdog -f=golint -name=golint-discussion -reporter=gitlab-mr-discussion
+    - cd ./_testdata/ && golint ./... | reviewdog -f=golint -name=golint-discussion-subdir -reporter=gitlab-mr-discussion
diff --git a/README.md b/README.md
index c543b411..e26b94ce 100644
--- a/README.md
+++ b/README.md
@@ -489,8 +489,6 @@ You can use public GitHub Actions to start using reviewdog with ease! :tada: :ar
   - [reviewdog/action-brakeman](https://github.com/reviewdog/action-brakeman) - Run [brakeman](https://github.com/presidentbeef/brakeman).
   - [reviewdog/action-rubocop](https://github.com/reviewdog/action-rubocop) - Run [rubocop](https://github.com/rubocop-hq/rubocop).
   - [vk26/action-fasterer](https://github.com/vk26/action-fasterer) - Run [fasterer](https://github.com/DamirSvrtan/fasterer).
-  - [SennaLabs/action-standardrb](https://github.com/SennaLabs/action-standardrb) - Run [standardrb](https://github.com/testdouble/standard).
-
 - Python
   - [wemake-python-styleguide](https://github.com/wemake-services/wemake-python-styleguide) - Run wemake-python-styleguide
 - Kotlin
diff --git a/difffilter/filter.go b/difffilter/filter.go
index bd3169f4..1104b89f 100644
--- a/difffilter/filter.go
+++ b/difffilter/filter.go
@@ -104,22 +104,20 @@ func (df *DiffFilter) addDiff(filediffs []*diff.FileDiff) {
 }
 
 // InDiff returns true, if the given path is in diff depending on the filter
-// Mode. It also optionally return LnumDiff[1].
-//
-// [1]: https://github.com/reviewdog/reviewdog/blob/73c40e69d937033b2cf20f2d6085fb7ef202e770/diff/diff.go#L81-L88
-func (df *DiffFilter) InDiff(path string, lnum int) (yes bool, lnumdiff int) {
+// Mode. It also optionally return diff line.
+func (df *DiffFilter) InDiff(path string, lnum int) (bool, *diff.Line) {
 	lines, ok := df.difflines[df.normalizePath(path)]
 	if !ok {
-		return false, 0
+		return false, nil
 	}
 	line, ok := lines[lnum]
 	if !ok {
 		if df.mode == ModeFile {
-			return true, 0
+			return true, nil
 		}
-		return false, 0
+		return false, nil
 	}
-	return true, line.LnumDiff
+	return true, line
 }
 
 func (df *DiffFilter) isSignificantLine(line *diff.Line) bool {
@@ -172,14 +170,23 @@ func contains(path, base string) bool {
 // `git diff --relative` can returns relative path to current workdir, so we
 // ask users not to use it for reviewdog command.
 func (df *DiffFilter) normalizeDiffPath(filediff *diff.FileDiff) normalizedPath {
-	path := filediff.PathNew
-	if df.strip > 0 {
-		ps := splitPathList(filediff.PathNew)
-		if len(ps) > df.strip {
-			path = filepath.Join(ps[df.strip:]...)
+	return normalizedPath{p: NormalizeDiffPath(filediff.PathNew, df.strip)}
+}
+
+// NormalizeDiffPath return path normalized path from given path in diff with
+// strip.
+func NormalizeDiffPath(diffpath string, strip int) string {
+	if diffpath == "/dev/null" {
+		return ""
+	}
+	path := diffpath
+	if strip > 0 {
+		ps := splitPathList(path)
+		if len(ps) > strip {
+			path = filepath.Join(ps[strip:]...)
 		}
 	}
-	return normalizedPath{p: filepath.ToSlash(filepath.Clean(path))}
+	return filepath.ToSlash(filepath.Clean(path))
 }
 
 func splitPathList(path string) []string {
diff --git a/filter.go b/filter.go
index 30212a21..3bc76a01 100644
--- a/filter.go
+++ b/filter.go
@@ -12,6 +12,8 @@ type FilteredCheck struct {
 	*CheckResult
 	InDiff   bool
 	LnumDiff int
+	OldPath  string
+	OldLine  int
 }
 
 // FilterCheck filters check results by diff. It doesn't drop check which
@@ -22,11 +24,14 @@ func FilterCheck(results []*CheckResult, diff []*diff.FileDiff, strip int,
 	df := difffilter.New(diff, strip, cwd, mode)
 	for _, result := range results {
 		check := &FilteredCheck{CheckResult: result}
-		if yes, lnumdiff := df.InDiff(result.Path, result.Lnum); yes {
+		if yes, diffline := df.InDiff(result.Path, result.Lnum); yes {
 			check.InDiff = true
-			check.LnumDiff = lnumdiff
+			if diffline != nil {
+				check.LnumDiff = diffline.LnumDiff
+			}
 		}
 		result.Path = CleanPath(result.Path, cwd)
+		check.OldPath, check.OldLine = getOldPosition(diff, strip, result.Path, result.Lnum)
 		checks = append(checks, check)
 	}
 	return checks
@@ -48,3 +53,26 @@ func CleanPath(path, workdir string) string {
 	}
 	return filepath.ToSlash(p)
 }
+
+func getOldPosition(filediffs []*diff.FileDiff, strip int, newPath string, newLine int) (oldPath string, oldLine int) {
+	for _, filediff := range filediffs {
+		if difffilter.NormalizeDiffPath(filediff.PathNew, strip) != newPath {
+			continue
+		}
+		oldPath = difffilter.NormalizeDiffPath(filediff.PathOld, strip)
+		delta := 0
+		for _, hunk := range filediff.Hunks {
+			if newLine < hunk.StartLineNew {
+				break
+			}
+			delta += hunk.LineLengthOld - hunk.LineLengthNew
+			for _, line := range hunk.Lines {
+				if line.LnumNew == newLine {
+					return oldPath, line.LnumOld
+				}
+			}
+		}
+		return oldPath, newLine + delta
+	}
+	return "", 0
+}
diff --git a/reviewdog.go b/reviewdog.go
index f472696c..2d1e9077 100644
--- a/reviewdog.go
+++ b/reviewdog.go
@@ -53,9 +53,12 @@ type Parser interface {
 // Comment represents a reported result as a comment.
 type Comment struct {
 	*CheckResult
+	ToolName string
 	Body     string
 	LnumDiff int
-	ToolName string
+	DiffLine *diff.Line
+	OldPath  string
+	OldLine  int
 }
 
 // CommentService is an interface which posts Comment.
@@ -92,8 +95,10 @@ func (w *Reviewdog) runFromResult(ctx context.Context, results []*CheckResult,
 		}
 		comment := &Comment{
 			CheckResult: check.CheckResult,
-			Body:        check.Message, // TODO: format message
+			Body:        check.Message,
 			LnumDiff:    check.LnumDiff,
+			OldPath:     check.OldPath,
+			OldLine:     check.OldLine,
 			ToolName:    w.toolname,
 		}
 		if err := w.c.Post(ctx, comment); err != nil {
diff --git a/service/commentutil/commentutil.go b/service/commentutil/commentutil.go
index 0ad213d7..da374079 100644
--- a/service/commentutil/commentutil.go
+++ b/service/commentutil/commentutil.go
@@ -2,6 +2,7 @@ package commentutil
 
 import (
 	"fmt"
+	"log"
 
 	"github.com/reviewdog/reviewdog"
 )
@@ -39,6 +40,15 @@ func (p PostedComments) AddPostedComment(path string, lineNum int, body string)
 	p[path][lineNum] = append(p[path][lineNum], body)
 }
 
+// DebugLog outputs posted comments as log for debugging.
+func (p PostedComments) DebugLog() {
+	for filename, f := range p {
+		for line := range f {
+			log.Printf("[debug] posted: %s:%d", filename, line)
+		}
+	}
+}
+
 // BodyPrefix is prefix text of comment body.
 const BodyPrefix = `<sub>reported by [reviewdog](https://github.com/reviewdog/reviewdog) :dog:</sub>`
 
diff --git a/service/gitlab/gitlab_mr_discussion.go b/service/gitlab/gitlab_mr_discussion.go
index 06a492e1..1f587ab2 100644
--- a/service/gitlab/gitlab_mr_discussion.go
+++ b/service/gitlab/gitlab_mr_discussion.go
@@ -104,16 +104,21 @@ func (g *GitLabMergeRequestDiscussionCommenter) postCommentsForEach(ctx context.
 			continue
 		}
 		eg.Go(func() error {
+			pos := &gitlab.NotePosition{
+				StartSHA:     targetBranch.Commit.ID,
+				HeadSHA:      g.sha,
+				BaseSHA:      targetBranch.Commit.ID,
+				PositionType: "text",
+				NewPath:      comment.Path,
+				NewLine:      comment.Lnum,
+			}
+			if comment.OldPath != "" && comment.OldLine != 0 {
+				pos.OldPath = comment.OldPath
+				pos.OldLine = comment.OldLine
+			}
 			discussion := &gitlab.CreateMergeRequestDiscussionOptions{
-				Body: gitlab.String(commentutil.CommentBody(comment)),
-				Position: &gitlab.NotePosition{
-					StartSHA:     targetBranch.Commit.ID,
-					HeadSHA:      g.sha,
-					BaseSHA:      targetBranch.Commit.ID,
-					PositionType: "text",
-					NewPath:      comment.Path,
-					NewLine:      comment.Lnum,
-				},
+				Body:     gitlab.String(commentutil.CommentBody(comment)),
+				Position: pos,
 			}
 			_, _, err := g.cli.Discussions.CreateMergeRequestDiscussion(g.projects, g.pr, discussion)
 			if err != nil {


Test patch:
diff --git a/difffilter/filter_test.go b/difffilter/filter_test.go
index 20378a5f..aee9ccda 100644
--- a/difffilter/filter_test.go
+++ b/difffilter/filter_test.go
@@ -129,7 +129,11 @@ func TestDiffFilter_root(t *testing.T) {
 	}
 	for _, tt := range tests {
 		df := New(files, 1, getCwd(), tt.mode)
-		if got, gotLnumDiff := df.InDiff(tt.path, tt.lnum); got != tt.want {
+		if got, gotLine := df.InDiff(tt.path, tt.lnum); got != tt.want {
+			gotLnumDiff := 0
+			if gotLine != nil {
+				gotLnumDiff = gotLine.LnumDiff
+			}
 			t.Errorf("InDiff(%q, %d) = (%v, %d), want (%v, %d)",
 				tt.path, tt.lnum, got, gotLnumDiff, tt.want, tt.wantLnumDiff)
 		}
@@ -172,7 +176,11 @@ func TestDiffFilter_subdir(t *testing.T) {
 	}
 	for _, tt := range tests {
 		df := New(files, 1, getCwd(), tt.mode)
-		if got, gotLnumDiff := df.InDiff(tt.path, tt.lnum); got != tt.want {
+		if got, gotLine := df.InDiff(tt.path, tt.lnum); got != tt.want {
+			gotLnumDiff := 0
+			if gotLine != nil {
+				gotLnumDiff = gotLine.LnumDiff
+			}
 			t.Errorf("InDiff(%q, %d) = (%v, %d), want (%v, %d)",
 				tt.path, tt.lnum, got, gotLnumDiff, tt.want, tt.wantLnumDiff)
 		}
diff --git a/filter_test.go b/filter_test.go
index b46cd59e..7c4dbe25 100644
--- a/filter_test.go
+++ b/filter_test.go
@@ -31,6 +31,17 @@ const diffContent = `--- sample.old.txt	2016-10-13 05:09:35.820791185 +0900
 \ No newline at end of file
 `
 
+const diffContentAddedStrip = `diff --git a/test_added.go b/test_added.go
+new file mode 100644
+index 0000000..264c67e
+--- /dev/null
++++ b/test_added.go
+@@ -0,0 +1,3 @@
++package reviewdog
++
++var TestAdded = 14
+`
+
 func TestFilterCheckByAddedLines(t *testing.T) {
 	results := []*CheckResult{
 		{
@@ -56,7 +67,9 @@ func TestFilterCheckByAddedLines(t *testing.T) {
 				Path: "sample.new.txt",
 				Lnum: 1,
 			},
-			InDiff: false,
+			InDiff:  false,
+			OldPath: "sample.old.txt",
+			OldLine: 1,
 		},
 		{
 			CheckResult: &CheckResult{
@@ -65,13 +78,17 @@ func TestFilterCheckByAddedLines(t *testing.T) {
 			},
 			InDiff:   true,
 			LnumDiff: 3,
+			OldPath:  "sample.old.txt",
+			OldLine:  0,
 		},
 		{
 			CheckResult: &CheckResult{
 				Path: "nonewline.new.txt",
 				Lnum: 1,
 			},
-			InDiff: false,
+			InDiff:  false,
+			OldPath: "nonewline.old.txt",
+			OldLine: 1,
 		},
 		{
 			CheckResult: &CheckResult{
@@ -80,6 +97,8 @@ func TestFilterCheckByAddedLines(t *testing.T) {
 			},
 			InDiff:   true,
 			LnumDiff: 5,
+			OldPath:  "nonewline.old.txt",
+			OldLine:  0,
 		},
 	}
 	filediffs, _ := diff.ParseMultiFile(strings.NewReader(diffContent))
@@ -113,6 +132,8 @@ func TestFilterCheckByDiffContext(t *testing.T) {
 			},
 			InDiff:   true,
 			LnumDiff: 1,
+			OldPath:  "sample.old.txt",
+			OldLine:  1,
 		},
 		{
 			CheckResult: &CheckResult{
@@ -121,6 +142,8 @@ func TestFilterCheckByDiffContext(t *testing.T) {
 			},
 			InDiff:   true,
 			LnumDiff: 3,
+			OldPath:  "sample.old.txt",
+			OldLine:  0,
 		},
 		{
 			CheckResult: &CheckResult{
@@ -129,6 +152,8 @@ func TestFilterCheckByDiffContext(t *testing.T) {
 			},
 			InDiff:   true,
 			LnumDiff: 4,
+			OldPath:  "sample.old.txt",
+			OldLine:  0,
 		},
 	}
 	filediffs, _ := diff.ParseMultiFile(strings.NewReader(diffContent))
@@ -137,3 +162,61 @@ func TestFilterCheckByDiffContext(t *testing.T) {
 		t.Error(diff)
 	}
 }
+
+func TestGetOldPosition(t *testing.T) {
+	const strip = 0
+	filediffs, _ := diff.ParseMultiFile(strings.NewReader(diffContent))
+	tests := []struct {
+		newPath     string
+		newLine     int
+		wantOldPath string
+		wantOldLine int
+	}{
+		{
+			newPath:     "sample.new.txt",
+			newLine:     1,
+			wantOldPath: "sample.old.txt",
+			wantOldLine: 1,
+		},
+		{
+			newPath:     "sample.new.txt",
+			newLine:     2,
+			wantOldPath: "sample.old.txt",
+			wantOldLine: 0,
+		},
+		{
+			newPath:     "sample.new.txt",
+			newLine:     3,
+			wantOldPath: "sample.old.txt",
+			wantOldLine: 0,
+		},
+		{
+			newPath:     "sample.new.txt",
+			newLine:     14,
+			wantOldPath: "sample.old.txt",
+			wantOldLine: 13,
+		},
+		{
+			newPath:     "not_found",
+			newLine:     14,
+			wantOldPath: "",
+			wantOldLine: 0,
+		},
+	}
+	for _, tt := range tests {
+		gotPath, gotLine := getOldPosition(filediffs, strip, tt.newPath, tt.newLine)
+		if !(gotPath == tt.wantOldPath && gotLine == tt.wantOldLine) {
+			t.Errorf("getOldPosition(..., %s, %d) = (%s, %d), want (%s, %d)",
+				tt.newPath, tt.newLine, gotPath, gotLine, tt.wantOldPath, tt.wantOldLine)
+		}
+	}
+}
+
+func TestGetOldPosition_added(t *testing.T) {
+	const strip = 1
+	filediffs, _ := diff.ParseMultiFile(strings.NewReader(diffContentAddedStrip))
+	gotPath, _ := getOldPosition(filediffs, strip, "test_added.go", 1)
+	if gotPath != "" {
+		t.Errorf("got %q as old path for addedd diff file, want empty", gotPath)
+	}
+}
diff --git a/service/gitlab/gitlab_mr_discussion_test.go b/service/gitlab/gitlab_mr_discussion_test.go
index a3c219fa..6dc0e123 100644
--- a/service/gitlab/gitlab_mr_discussion_test.go
+++ b/service/gitlab/gitlab_mr_discussion_test.go
@@ -48,12 +48,22 @@ func TestGitLabMergeRequestDiscussionCommenter_Post_Flush_review_api(t *testing.
 		},
 		Body: "new comment 2",
 	}
+	newComment3 := &reviewdog.Comment{
+		CheckResult: &reviewdog.CheckResult{
+			Path: "new_file.go",
+			Lnum: 14,
+		},
+		Body:    "new comment 3",
+		OldPath: "old_file.go",
+		OldLine: 7,
+	}
 
 	comments := []*reviewdog.Comment{
 		alreadyCommented1,
 		alreadyCommented2,
 		newComment1,
 		newComment2,
+		newComment3,
 	}
 
 	mux := http.NewServeMux()
@@ -129,6 +139,18 @@ func TestGitLabMergeRequestDiscussionCommenter_Post_Flush_review_api(t *testing.
 				if diff := cmp.Diff(got, want); diff != "" {
 					t.Error(diff)
 				}
+			case "new_file.go":
+				want := &gitlab.CreateMergeRequestDiscussionOptions{
+					Body: gitlab.String(commentutil.CommentBody(newComment3)),
+					Position: &gitlab.NotePosition{
+						BaseSHA: "xxx", StartSHA: "xxx", HeadSHA: "sha", PositionType: "text",
+						NewPath: "new_file.go", NewLine: 14,
+						OldPath: "old_file.go", OldLine: 7,
+					},
+				}
+				if diff := cmp.Diff(got, want); diff != "" {
+					t.Error(diff)
+				}
 			default:
 				t.Errorf("got unexpected discussion: %#v", got)
 			}