Repository: VKCOM/noverify
Pull request #591 (2020-07-16 20:51:29)

cmd: add -gitignore filter
 cmd: add -gitignore flag (default=false)

When -gitignore is set, NoVerify tries to respect gitignore
files and exclude ignored files from the analysis.

During the startup, we try to find a git repository root
by going up until we find a folder with .git or encounter
the FS root. We save all gitignore files we find along the way.

gitignore can't be used outside of the valid git repository.

quasilyte: I guess we can use godirwalk PostChildrenCallback to pop gitignore files when all its children folders were traversed to avoid running a list of gitignore matchers over the entire project.

Patch:
diff --git a/go.mod b/go.mod
index c02257e..2f5efa7 100644
--- a/go.mod
+++ b/go.mod
@@ -6,6 +6,7 @@ require (
 	github.com/client9/misspell v0.3.4
 	github.com/google/go-cmp v0.4.1
 	github.com/karrick/godirwalk v1.15.6
+	github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00
 	github.com/quasilyte/regex/syntax v0.0.0-20200419152657-af9db7f4a3ab
 	go.lsp.dev/uri v0.3.0
 )
diff --git a/go.sum b/go.sum
index 5e8c68c..de392f2 100644
--- a/go.sum
+++ b/go.sum
@@ -6,6 +6,8 @@ github.com/google/go-cmp v0.4.1 h1:/exdXoGamhu5ONeUJH0deniYLWYvQwW66yvlfiiKTu0=
 github.com/google/go-cmp v0.4.1/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
 github.com/karrick/godirwalk v1.15.6 h1:Yf2mmR8TJy+8Fa0SuQVto5SYap6IF7lNVX4Jdl8G1qA=
 github.com/karrick/godirwalk v1.15.6/go.mod h1:j4mkqPuvaLI8mp1DroR3P6ad7cyYd4c1qeJ3RV7ULlk=
+github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00 h1:n6/2gBQ3RWajuToeY6ZtZTIKv2v7ThUy5KKusIT0yc0=
+github.com/monochromegane/go-gitignore v0.0.0-20200626010858-205db1a8cc00/go.mod h1:Pm3mSP3c5uWn86xMLZ5Sa7JB9GsEZySvHYXCTK4E9q4=
 github.com/quasilyte/regex/syntax v0.0.0-20200419152657-af9db7f4a3ab h1:rjBjlam2Bbr6Dwp0T8HY2paibXTjMsNQU7vUH8hB+C4=
 github.com/quasilyte/regex/syntax v0.0.0-20200419152657-af9db7f4a3ab/go.mod h1:rlzQ04UMyJXu/aOvhd8qT+hvDrFpiwqp8MRXDY9szc0=
 go.lsp.dev/uri v0.3.0 h1:KcZJmh6nFIBeJzTugn5JTU6OOyG0lDOo3R9KwTxTYbo=
diff --git a/src/cmd/flags.go b/src/cmd/flags.go
index cd81f69..0083561 100644
--- a/src/cmd/flags.go
+++ b/src/cmd/flags.go
@@ -50,6 +50,8 @@ type cmdlineArguments struct {
 
 	phpExtensionsArg string
 
+	gitignore bool
+
 	gitPushArg                 string
 	gitAuthorsWhitelist        string
 	gitWorkTree                string
@@ -127,6 +129,9 @@ func bindFlags(ruleSets []*rules.Set, args *cmdlineArguments) {
 	flag.BoolVar(&args.fix, "fix", false,
 		"Apply a quickfix where possible (updates source files)")
 
+	flag.BoolVar(&args.gitignore, "gitignore", false,
+		"If enabled, noverify tries to use .gitignore files to exclude matched ignored files from the analysis")
+
 	flag.StringVar(&args.gitRepo, "git", "", "Path to git repository to analyze")
 	flag.StringVar(&args.mutable.gitCommitFrom, "git-commit-from", "", "Analyze changes between commits <git-commit-from> and <git-commit-to>")
 	flag.StringVar(&args.mutable.gitCommitTo, "git-commit-to", "", "")
diff --git a/src/cmd/linter_runner.go b/src/cmd/linter_runner.go
index a88f3df..bd1942f 100644
--- a/src/cmd/linter_runner.go
+++ b/src/cmd/linter_runner.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"path/filepath"
 	"regexp"
 	"strings"
 
@@ -19,6 +20,8 @@ type linterRunner struct {
 
 	outputFp io.Writer
 
+	filenameFilter *linter.FilenameFilter
+
 	reportsExcludeChecksSet map[string]bool
 	reportsIncludeChecksSet map[string]bool
 	reportsCriticalSet      map[string]bool
@@ -38,9 +41,52 @@ func (l *linterRunner) IsEnabledByFlags(checkName string) bool {
 	return true
 }
 
+func (l *linterRunner) collectGitIgnoreFiles() error {
+	l.filenameFilter = linter.NewFilenameFilter(linter.ExcludeRegex)
+
+	if !l.args.gitignore {
+		return nil
+	}
+
+	workingDir, err := os.Getwd()
+	if err != nil {
+		return fmt.Errorf("getwd: %v", err)
+	}
+
+	l.filenameFilter.EnableGitignore()
+
+	// Walk wd up until we find .git (good) or FS root (bad).
+	// We collect all gitignore files along the way.
+	dir := workingDir
+	for {
+		m, err := linter.ParseGitignoreFromDir(dir)
+		if err != nil {
+			return fmt.Errorf("read .gitignore: %v", err)
+		}
+		if m != nil {
+			l.filenameFilter.InitialGitignorePush(dir, m)
+		}
+		if _, err := os.Stat(filepath.Join(dir, ".git")); err == nil {
+			linter.DebugMessage("discovered git top level: %s", dir)
+			break
+		}
+		parentDir := filepath.Dir(dir)
+		if parentDir == dir {
+			return fmt.Errorf("not a git repository (don't use -gitignore for non-git projects)")
+		}
+		dir = parentDir
+	}
+
+	return nil
+}
+
 func (l *linterRunner) Init(ruleSets []*rules.Set, args *cmdlineArguments) error {
 	l.args = args
 
+	if err := l.collectGitIgnoreFiles(); err != nil {
+		return fmt.Errorf("collect gitignore files: %v", err)
+	}
+
 	l.outputFp = os.Stderr
 	if args.output != "" {
 		outputFp, err := os.OpenFile(args.output, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0666)
diff --git a/src/cmd/main.go b/src/cmd/main.go
index dd70247..35af0bc 100644
--- a/src/cmd/main.go
+++ b/src/cmd/main.go
@@ -170,14 +170,14 @@ func mainNoExit(ruleSets []*rules.Set, args *cmdlineArguments, cfg *MainConfig)
 	linter.ParseFilenames(linter.ReadFilenames(flag.Args(), nil), l.allowDisableRegex)
 	parseIndexOnlyFiles(&l)
 	meta.SetIndexingComplete(true)
-	log.Printf("Linting")
 
 	filenames := flag.Args()
 	if args.fullAnalysisFiles != "" {
 		filenames = strings.Split(args.fullAnalysisFiles, ",")
 	}
 
-	reports := linter.ParseFilenames(linter.ReadFilenames(filenames, linter.ExcludeRegex), l.allowDisableRegex)
+	log.Printf("Linting")
+	reports := linter.ParseFilenames(linter.ReadFilenames(filenames, l.filenameFilter), l.allowDisableRegex)
 	if args.outputBaseline {
 		if err := createBaseline(&l, cfg, reports); err != nil {
 			return 1, fmt.Errorf("write baseline: %v", err)
diff --git a/src/langsrv/langsrv.go b/src/langsrv/langsrv.go
index 8190b41..949c030 100644
--- a/src/langsrv/langsrv.go
+++ b/src/langsrv/langsrv.go
@@ -169,7 +169,8 @@ func handleInitialize(req *baseRequest) error {
 	go func() {
 		linter.AnalysisFiles = []string{params.RootURI.Filename()}
 
-		linter.ParseFilenames(linter.ReadFilenames(linter.AnalysisFiles, linter.ExcludeRegex), nil)
+		filter := linter.NewFilenameFilter(linter.ExcludeRegex)
+		linter.ParseFilenames(linter.ReadFilenames(linter.AnalysisFiles, filter), nil)
 
 		meta.SetIndexingComplete(true)
 
diff --git a/src/linter/filename_filter.go b/src/linter/filename_filter.go
new file mode 100644
index 0000000..789a631
--- /dev/null
+++ b/src/linter/filename_filter.go
@@ -0,0 +1,79 @@
+package linter
+
+import (
+	"regexp"
+
+	"github.com/monochromegane/go-gitignore"
+)
+
+type FilenameFilter struct {
+	exclude *regexp.Regexp
+
+	gitignoreEnabled bool
+	matchers         []gitignore.IgnoreMatcher
+	initialMatchers  map[string]struct{}
+}
+
+func NewFilenameFilter(exclude *regexp.Regexp) *FilenameFilter {
+	return &FilenameFilter{
+		exclude:         exclude,
+		initialMatchers: make(map[string]struct{}),
+	}
+}
+
+func (filter *FilenameFilter) EnableGitignore() { filter.gitignoreEnabled = true }
+
+func (filter *FilenameFilter) GitignoreIsEnabled() bool { return filter.gitignoreEnabled }
+
+func (filter *FilenameFilter) InitialGitignorePush(path string, matcher gitignore.IgnoreMatcher) {
+	if !filter.gitignoreEnabled {
+		panic("add when gitignore is disabled")
+	}
+	DebugMessage("gitignore: add %s/.gitignore", path)
+	filter.initialMatchers[path] = struct{}{}
+	filter.matchers = append(filter.matchers, matcher)
+}
+
+func (filter *FilenameFilter) GitignorePush(path string, matcher gitignore.IgnoreMatcher) {
+	if !filter.gitignoreEnabled {
+		panic("pop when gitignore is disabled")
+	}
+	if _, ok := filter.initialMatchers[path]; ok {
+		DebugMessage("gitignore: don't push %s/.gitignore", path)
+		return
+	}
+	DebugMessage("gitignore: push %s/.gitignore", path)
+	filter.matchers = append(filter.matchers, matcher)
+}
+
+func (filter *FilenameFilter) GitignorePop(path string) {
+	if !filter.gitignoreEnabled {
+		panic("pop when gitignore is disabled")
+	}
+	if _, ok := filter.initialMatchers[path]; ok {
+		DebugMessage("gitignore: don't pop %s/.gitignore", path)
+		return
+	}
+	DebugMessage("gitignore: pop %s/.gitignore", path)
+	filter.matchers = filter.matchers[:len(filter.matchers)-1]
+}
+
+func (filter *FilenameFilter) IgnoreFile(path string) bool {
+	return filter.ignore(path, false)
+}
+
+func (filter *FilenameFilter) IgnoreDir(path string) bool {
+	return filter.ignore(path, true)
+}
+
+func (filter *FilenameFilter) ignore(path string, isDir bool) bool {
+	if filter.exclude != nil && filter.exclude.MatchString(path) {
+		return true
+	}
+	for _, matcher := range filter.matchers {
+		if matcher.Match(path, isDir) {
+			return true
+		}
+	}
+	return false
+}
diff --git a/src/linter/parser.go b/src/linter/parser.go
index 41d1c27..744e5cd 100644
--- a/src/linter/parser.go
+++ b/src/linter/parser.go
@@ -16,6 +16,7 @@ import (
 	"time"
 
 	"github.com/karrick/godirwalk"
+	"github.com/monochromegane/go-gitignore"
 	"github.com/quasilyte/regex/syntax"
 
 	"github.com/VKCOM/noverify/src/git"
@@ -229,62 +230,121 @@ func DebugMessage(msg string, args ...interface{}) {
 	}
 }
 
-// ReadFilenames returns callback that reads filenames into channel
-func ReadFilenames(filenames []string, ignoreRegex *regexp.Regexp) ReadCallback {
-	return func(ch chan FileInfo) {
-		for _, filename := range filenames {
-			absFilename, err := filepath.Abs(filename)
-			if err == nil {
-				filename = absFilename
-			}
+// ParseGitignoreFromDir tries to parse a gitignore file at path/.gitignore.
+// If no such file exists, <nil, nil> is returned.
+func ParseGitignoreFromDir(path string) (gitignore.IgnoreMatcher, error) {
+	f, err := os.Open(filepath.Join(path, ".gitignore"))
+	switch {
+	case os.IsNotExist(err):
+		return nil, nil // No gitignore file, not an error
+	case err != nil:
+		return nil, err // Some unexpected error (e.g. access failure)
+	}
+	defer f.Close()
+	matcher := gitignore.NewGitIgnoreFromReader(path, f)
+	return matcher, nil
+}
 
-			// If we use stat here, it will return file info of an entry
-			// pointed by a symlink (if filename is a link).
-			// lstat is required for a symlink test below to succeed.
-			// If we ever want to permit top-level (CLI args) symlinks,
-			// caller should resolve them to a files that are pointed by them.
-			st, err := os.Lstat(filename)
-			if err != nil {
-				log.Fatalf("Could not stat file %s: %s", filename, err.Error())
-				continue
-			}
-			if st.Mode()&os.ModeSymlink != 0 {
-				// filepath.Walk does not follow symlinks, but it does
-				// accept it as a root argument without an error.
-				// godirwalk.Walk can traverse symlinks with FollowSymbolicLinks=true,
-				// but we don't use it. It will give an error if root is
-				// a symlink, so we avoid calling Walk() on them.
-				continue
-			}
+func readFilenames(ch chan<- FileInfo, filename string, filter *FilenameFilter) {
+	absFilename, err := filepath.Abs(filename)
+	if err == nil {
+		filename = absFilename
+	}
 
-			if !st.IsDir() {
-				if ignoreRegex != nil && ignoreRegex.MatchString(filename) {
-					continue
-				}
+	if filter == nil {
+		// No-op filter that doesn't track gitignore files.
+		filter = &FilenameFilter{}
+	}
 
-				ch <- FileInfo{Filename: filename}
-				continue
-			}
+	// If we use stat here, it will return file info of an entry
+	// pointed by a symlink (if filename is a link).
+	// lstat is required for a symlink test below to succeed.
+	// If we ever want to permit top-level (CLI args) symlinks,
+	// caller should resolve them to a files that are pointed by them.
+	st, err := os.Lstat(filename)
+	if err != nil {
+		log.Fatalf("Could not stat file %s: %s", filename, err.Error())
+	}
+	if st.Mode()&os.ModeSymlink != 0 {
+		// filepath.Walk does not follow symlinks, but it does
+		// accept it as a root argument without an error.
+		// godirwalk.Walk can traverse symlinks with FollowSymbolicLinks=true,
+		// but we don't use it. It will give an error if root is
+		// a symlink, so we avoid calling Walk() on them.
+		return
+	}
 
-			err = godirwalk.Walk(filename, &godirwalk.Options{
-				Callback: func(path string, de *godirwalk.Dirent) error {
-					if de.IsDir() || !isPHPExtension(path) {
-						return nil
-					}
+	if !st.IsDir() {
+		if filter.IgnoreFile(filename) {
+			return
+		}
 
-					if ignoreRegex != nil && ignoreRegex.MatchString(path) {
-						return nil
-					}
+		ch <- FileInfo{Filename: filename}
+		return
+	}
+
+	// Start with a sentinel "" path to make last(gitignorePaths) safe
+	// without a length check.
+	gitignorePaths := []string{""}
 
-					ch <- FileInfo{Filename: path}
+	walkOptions := &godirwalk.Options{
+		Unsorted: true,
+
+		Callback: func(path string, de *godirwalk.Dirent) error {
+			if de.IsDir() {
+				if filter.IgnoreDir(path) {
+					return filepath.SkipDir
+				}
+				// During indexing phase and with -gitignore=false
+				// we don't want to do extra FS operations.
+				if !filter.GitignoreIsEnabled() {
 					return nil
-				},
-				Unsorted: true,
-			})
+				}
 
-			if err != nil {
-				log.Fatalf("Could not walk filepath %s (%v)", filename, err)
+				matcher, err := ParseGitignoreFromDir(path)
+				if err != nil {
+					linterError(path, "read .gitignore: %v", err)
+				}
+				if matcher != nil {
+					gitignorePaths = append(gitignorePaths, path)
+					filter.GitignorePush(path, matcher)
+				}
+				return nil
 			}
+
+			if !isPHPExtension(path) {
+				return nil
+			}
+			if filter.IgnoreFile(path) {
+				return nil
+			}
+
+			ch <- FileInfo{Filename: path}
+			return nil
+		},
+	}
+
+	if filter.GitignoreIsEnabled() {
+		walkOptions.PostChildrenCallback = func(path string, de *godirwalk.Dirent) error {
+			topGitignorePath := gitignorePaths[len(gitignorePaths)-1]
+			if topGitignorePath == path {
+				gitignorePaths = gitignorePaths[:len(gitignorePaths)-1]
+				filter.GitignorePop(path)
+			}
+			return nil
+		}
+	}
+
+	if err := godirwalk.Walk(filename, walkOptions); err != nil {
+		log.Fatalf("Could not walk filepath %s (%v)", filename, err)
+	}
+}
+
+// ReadFilenames returns callback that reads filenames into channel
+func ReadFilenames(filenames []string, filter *FilenameFilter) ReadCallback {
+	return func(ch chan FileInfo) {
+		for _, filename := range filenames {
+			readFilenames(ch, filename, filter)
 		}
 	}
 }


Test patch:
diff --git a/src/linttest/golden_test.go b/src/linttest/golden_test.go
index a3cc8c6..6d9cde3 100644
--- a/src/linttest/golden_test.go
+++ b/src/linttest/golden_test.go
@@ -23,6 +23,9 @@ type goldenTest struct {
 	deps    []string
 	disable []string
 
+	onlyE2E   bool
+	gitignore bool
+
 	srcDir string
 
 	// want is a golden file contents.
@@ -170,6 +173,12 @@ func TestGolden(t *testing.T) {
 		{
 			name: "output-test",
 		},
+
+		{
+			name:      "gitignore-test",
+			onlyE2E:   true,
+			gitignore: true,
+		},
 	}
 
 	for _, target := range targets {
@@ -191,6 +200,9 @@ func TestGolden(t *testing.T) {
 	// easier, but it can't test whether our linter can work from
 	// the command-line in the same way as it does here.
 	for _, target := range targets {
+		if target.onlyE2E {
+			continue
+		}
 		runGoldenTest(t, target)
 	}
 
@@ -239,6 +251,9 @@ func runGoldenTestsE2E(t *testing.T, targets []*goldenTest) {
 			if len(target.disable) != 0 {
 				args = append(args, "--exclude-checks", strings.Join(target.disable, ","))
 			}
+			if target.gitignore {
+				args = append(args, "--gitignore")
+			}
 			args = append(args, target.srcDir)
 
 			out, err := exec.Command("./phplinter", args...).CombinedOutput()
diff --git a/src/linttest/testdata/gitignore-test/.gitignore b/src/linttest/testdata/gitignore-test/.gitignore
new file mode 100644
index 0000000..0aaca74
--- /dev/null
+++ b/src/linttest/testdata/gitignore-test/.gitignore
@@ -0,0 +1,1 @@
+root-ignore.php
diff --git a/src/linttest/testdata/gitignore-test/golden.txt b/src/linttest/testdata/gitignore-test/golden.txt
new file mode 100644
index 0000000..32664af
--- /dev/null
+++ b/src/linttest/testdata/gitignore-test/golden.txt
@@ -0,0 +1,3 @@
+WARNING discardExpr: expression evaluated but not used at testdata/gitignore-test/src/b/src-a-ignore.php:3
+"not ignored, since it's not under src/a";
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/src/linttest/testdata/gitignore-test/src/.gitignore b/src/linttest/testdata/gitignore-test/src/.gitignore
new file mode 100644
index 0000000..c1f0c4b
--- /dev/null
+++ b/src/linttest/testdata/gitignore-test/src/.gitignore
@@ -0,0 +1,2 @@
+src-ignore1.php
+src-ignore2.php
diff --git a/src/linttest/testdata/gitignore-test/src/a/.gitignore b/src/linttest/testdata/gitignore-test/src/a/.gitignore
new file mode 100644
index 0000000..4fcfe58
--- /dev/null
+++ b/src/linttest/testdata/gitignore-test/src/a/.gitignore
@@ -0,0 +1,1 @@
+src-a-ignore.php
diff --git a/src/linttest/testdata/gitignore-test/src/a/aa/src-a-ignore.php b/src/linttest/testdata/gitignore-test/src/a/aa/src-a-ignore.php
new file mode 100644
index 0000000..2670952
--- /dev/null
+++ b/src/linttest/testdata/gitignore-test/src/a/aa/src-a-ignore.php
@@ -0,0 +1,3 @@
+<?php
+
+"ignored by src/a/.gitignore";
diff --git a/src/linttest/testdata/gitignore-test/src/a/src-a-ignore.php b/src/linttest/testdata/gitignore-test/src/a/src-a-ignore.php
new file mode 100644
index 0000000..2670952
--- /dev/null
+++ b/src/linttest/testdata/gitignore-test/src/a/src-a-ignore.php
@@ -0,0 +1,3 @@
+<?php
+
+"ignored by src/a/.gitignore";
diff --git a/src/linttest/testdata/gitignore-test/src/a/src-ignore2.php b/src/linttest/testdata/gitignore-test/src/a/src-ignore2.php
new file mode 100644
index 0000000..1cb1acb
--- /dev/null
+++ b/src/linttest/testdata/gitignore-test/src/a/src-ignore2.php
@@ -0,0 +1,3 @@
+<?php
+
+"ignored by src/.gitignore";
diff --git a/src/linttest/testdata/gitignore-test/src/b/src-a-ignore.php b/src/linttest/testdata/gitignore-test/src/b/src-a-ignore.php
new file mode 100644
index 0000000..ef569cc
--- /dev/null
+++ b/src/linttest/testdata/gitignore-test/src/b/src-a-ignore.php
@@ -0,0 +1,3 @@
+<?php
+
+"not ignored, since it's not under src/a";
diff --git a/src/linttest/testdata/gitignore-test/src/root-ignore.php b/src/linttest/testdata/gitignore-test/src/root-ignore.php
new file mode 100644
index 0000000..16d6271
--- /dev/null
+++ b/src/linttest/testdata/gitignore-test/src/root-ignore.php
@@ -0,0 +1,3 @@
+<?php
+
+"ignored by root .gitignore";
diff --git a/src/linttest/testdata/gitignore-test/src/src-ignore1.php b/src/linttest/testdata/gitignore-test/src/src-ignore1.php
new file mode 100644
index 0000000..1cb1acb
--- /dev/null
+++ b/src/linttest/testdata/gitignore-test/src/src-ignore1.php
@@ -0,0 +1,3 @@
+<?php
+
+"ignored by src/.gitignore";