--new-from-rev doesn't work on Windows

[onobrod]

TL;DR: revgrep tool uses git diff command to collect changed lines. On Windows git diff output contains file paths with forward slashes. But linters output contains file paths with backslashes. When revgrep searches changed lines for a linter warning it cannot find a match because paths are not equal

I found this issue when I tried to run golangci-lint in our project. Without --new-from-rev option it produces the same output on macOS and Windows. With the option it produces correct output on macOS and empty output on Windows

Verbose output of running on macOS

$ golangci-lint run ./... -v
INFO [config_reader] Config search paths: [./ /Users/onobrod/Go/src/cella/backend /Users/onobrod/Go/src/cella /Users/onobrod/Go/src /Users/onobrod/Go /Users/onobrod /Users /]
INFO [config_reader] Used config file .golangci.yml
INFO [lintersdb] Active 1 linters: [errcheck]
INFO [lintersdb] Active 1 linters: [errcheck]
INFO [loader] Go packages loading at mode 575 (deps|files|name|compiled_files|exports_file|imports|types_sizes) took 8.560138042s
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 98.946851ms
INFO [runner/errcheck/goanalysis] analyzers took 98.18298ms with top 10 stages: errcheck: 98.18298ms
INFO [runner] Issues before processing: 34, after processing: 14
INFO [runner] Processors filtering stat (out/in): filename_unadjuster: 34/34, skip_files: 34/34, exclude: 31/31, max_per_file_from_linter: 14/14, max_from_linter: 14/14, source_code: 14/14, skip_dirs: 34/34, identifier_marker: 31/31, nolint: 14/14, exclude-rules: 14/31, uniq_by_line: 14/14, diff: 14/14, max_same_issues: 14/14, path_shortener: 14/14, cgo: 34/34, path_prettifier: 34/34, autogenerated_exclude: 31/34
INFO [runner] processing took 5.082052ms with stages: nolint: 2.038093ms, path_prettifier: 992.173µs, autogenerated_exclude: 484.996µs, identifier_marker: 475.898µs, cgo: 295.779µs, skip_dirs: 293.261µs, exclude-rules: 288.154µs, source_code: 199.403µs, filename_unadjuster: 4.459µs, uniq_by_line: 3.883µs, path_shortener: 3.1µs, max_per_file_from_linter: 953ns, max_same_issues: 769ns, diff: 322ns, max_from_linter: 314ns, skip_files: 264ns, exclude: 231ns
INFO [runner] linters took 3.345272873s with stages: errcheck: 3.340075563s
src/appcontext/context.go:103:3: Error return value of session.AbortTransaction is not checked (errcheck)
                _ = session.AbortTransaction(context.TODO())
                ^
src/appcontext/context.go:106:3: Error return value of session.CommitTransaction is not checked (errcheck)
                _ = session.CommitTransaction(context.TODO())
                ^
 ... 12 more warnings here I need to skip because it is a commercial project
INFO File cache stats: 8 entries of total size 35.6KiB
INFO Memory: 122 samples, avg is 77.0MB, max is 202.2MB
INFO Execution took 12.029032819s

$ golangci-lint run ./... --new-from-rev=origin/master -v
INFO [config_reader] Config search paths: [./ /Users/onobrod/Go/src/cella/backend /Users/onobrod/Go/src/cella /Users/onobrod/Go/src /Users/onobrod/Go /Users/onobrod /Users /]
INFO [config_reader] Used config file .golangci.yml
INFO [lintersdb] Active 1 linters: [errcheck]
INFO [lintersdb] Active 1 linters: [errcheck]
INFO [loader] Go packages loading at mode 575 (compiled_files|deps|exports_file|types_sizes|files|imports|name) took 984.796717ms
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 102.007603ms
INFO [runner/errcheck/goanalysis] analyzers took 0s with no stages
INFO [runner] Issues before processing: 34, after processing: 1
INFO [runner] Processors filtering stat (out/in): identifier_marker: 31/31, nolint: 14/14, diff: 1/14, max_from_linter: 1/1, source_code: 1/1, cgo: 34/34, skip_files: 34/34, autogenerated_exclude: 31/34, exclude: 31/31, uniq_by_line: 14/14, path_shortener: 1/1, max_same_issues: 1/1, filename_unadjuster: 34/34, path_prettifier: 34/34, skip_dirs: 34/34, exclude-rules: 14/31, max_per_file_from_linter: 1/1
INFO [runner] processing took 161.670869ms with stages: diff: 138.85356ms, identifier_marker: 14.012748ms, nolint: 3.507455ms, path_prettifier: 2.912361ms, autogenerated_exclude: 1.026596ms, exclude-rules: 656.367µs, skip_dirs: 515.885µs, source_code: 129.498µs, cgo: 29.879µs, filename_unadjuster: 12.965µs, uniq_by_line: 4.862µs, max_same_issues: 2.975µs, path_shortener: 2.506µs, max_per_file_from_linter: 1.635µs, skip_files: 627ns, max_from_linter: 510ns, exclude: 440ns
INFO [runner] linters took 694.548533ms with stages: errcheck: 532.653928ms
src/appcontext/context.go:106:3: Error return value of session.CommitTransaction is not checked (errcheck)
                _ = session.CommitTransaction(context.TODO())
                ^
INFO File cache stats: 1 entries of total size 4.5KiB
INFO Memory: 19 samples, avg is 69.0MB, max is 69.1MB
INFO Execution took 1.796431411s

Verbose output of running on Windows

>golangci-lint run ./... -v
level=info msg="[config_reader] Config search paths: [./ C:\\Users\\onobrod\\go\\src\\cella\\backend C:\\Users\\onobrod\\go\\src\\cella C:\\Users\\onobrod\\go\\src C:\\Users\\onobrod\\go C:\\Users\\onobrod C:\\Users C:\\]"
level=info msg="[config_reader] Used config file .golangci.yml"
level=info msg="[lintersdb] Active 1 linters: [errcheck]"
level=info msg="[loader] Go packages loading at mode 575 (types_sizes|files|imports|name|compiled_files|deps|exports_file) took 5.838028s"
level=info msg="[runner/filename_unadjuster] Pre-built 0 adjustments in 47.001ms"
level=info msg="[runner] Issues before processing: 34, after processing: 14"
level=info msg="[runner] Processors filtering stat (out/in): skip_files: 34/34, exclude: 31/31, diff: 14/14, max_same_issues: 14/14, source_code: 14/14, path_prettifier: 34/34, identifier_marker: 31/31, nolint: 14/14, uniq_by_line: 14/14, filename_unadjuster: 34/34, autogenerated_exclude: 31/34, exclude-rules: 14/31, max_per_file_from_linter: 14/14, max_from_linter: 14/14, cgo: 34/34, skip_dirs: 34/34, path_shortener: 14/14"
level=info msg="[runner] processing took 16.9993ms with stages: path_prettifier: 11.9992ms, nolint: 2ms, identifier_marker: 1.0002ms, autogenerated_exclude: 1.0001ms, source_code: 999.8µs, path_shortener: 0s, cgo: 0s, skip_files: 0s, exclude: 0s, exclude-rules: 0s, uniq_by_line: 0s, diff: 0s, max_per_file_from_linter: 0s, filename_unadjuster: 0s, skip_dirs: 0s, max_same_issues: 0s, max_from_linter: 0s"
level=info msg="[runner] linters took 443.9987ms with stages: errcheck: 398.9991ms"
src\appcontext\context.go:103:3: Error return value of session.AbortTransaction is not checked (errcheck)
                _ = session.AbortTransaction(context.TODO())
                ^
src\appcontext\context.go:106:3: Error return value of session.CommitTransaction is not checked (errcheck)
                _ = session.CommitTransaction(context.TODO())
                ^
 ... 12 more warnings here I need to skip because it is a commercial project
level=info msg="File cache stats: 8 entries of total size 35.6KiB"
level=info msg="Memory: 70 samples, avg is 31.6MB, max is 165.3MB"
level=info msg="Execution took 6.8159681s"

>golangci-lint run ./... --new-from-rev=origin/master -v
level=info msg="[config_reader] Config search paths: [./ C:\\Users\\onobrod\\go\\src\\cella\\backend C:\\Users\\onobrod\\go\\src\\cella C:\\Users\\onobrod\\go\\src C:\\Users\\onobrod\\go C:\\Users\\onobrod C:\\Users C:\\]"
level=info msg="[config_reader] Used config file .golangci.yml"
level=info msg="[lintersdb] Active 1 linters: [errcheck]"
level=info msg="[loader] Go packages loading at mode 575 (exports_file|imports|types_sizes|compiled_files|deps|files|name) took 968.0251ms"
level=info msg="[runner/filename_unadjuster] Pre-built 0 adjustments in 45.0006ms"
level=info msg="[runner] Issues before processing: 34, after processing: 0"
level=info msg="[runner] Processors filtering stat (out/in): cgo: 34/34, autogenerated_exclude: 31/34, path_prettifier: 34/34, skip_files: 34/34, nolint: 14/14, diff: 0/14, identifier_marker: 31/31, exclude-rules: 14/31, filename_unadjuster: 34/34, skip_dirs: 34/34, exclude: 31/31, uniq_by_line: 14/14"
level=info msg="[runner] processing took 1.3980103s with stages: diff: 1.3820107s, path_prettifier: 12.0005ms, nolint: 1.9999ms, identifier_marker: 1ms, skip_dirs: 999.2µs, path_shortener: 0s, max_from_linter: 0s, exclude: 0s, max_per_file_from_linter: 0s, cgo: 0s, exclude-rules: 0s, source_code: 0s, uniq_by_line: 0s, max_same_issues: 0s, filename_unadjuster: 0s, skip_files: 0s, autogenerated_exclude: 0s"
level=info msg="[runner] linters took 1.8199993s with stages: errcheck: 389.0003ms"
level=info msg="File cache stats: 0 entries of total size 0B"
level=info msg="Memory: 30 samples, avg is 108.6MB, max is 169.5MB"
level=info msg="Execution took 2.8989741s"

Diff on macOS

$ git diff origin/master
diff --git a/backend/src/appcontext/context.go b/backend/src/appcontext/context.go
index a1d6d19732..3ef4ef13e4 100644
--- a/backend/src/appcontext/context.go
+++ b/backend/src/appcontext/context.go
@@ -103,8 +103,10 @@ func (ctx *appContext) DoWithTransaction(fn func() error) error {
                _ = session.AbortTransaction(context.TODO())
                return errSession
        } else {
-               return session.CommitTransaction(context.TODO())
+               _ = session.CommitTransaction(context.TODO())
        }
+
+       return nil
 }

Diff on Windows

>git diff origin/master
diff --git a/backend/src/appcontext/context.go b/backend/src/appcontext/context.go
index a1d6d1973..3ef4ef13e 100644
--- a/backend/src/appcontext/context.go
+++ b/backend/src/appcontext/context.go
@@ -103,8 +103,10 @@ func (ctx *appContext) DoWithTransaction(fn func() error) error {
                _ = session.AbortTransaction(context.TODO())
                return errSession
        } else {
-               return session.CommitTransaction(context.TODO())
+               _ = session.CommitTransaction(context.TODO())
        }
+
+       return nil
 }

Processors filtering stat helps to narrow the circle: macOS output has diff: 1/14 when Windows output has diff: 0/14. Since golangci-lint has no debug option to debug diff processor, I've installed revgrep tool and executed it separately on errcheck results. Seems it searches changed lines for a linter warning but cannot find a match because path from linter output has backslashes and path from git diff has forward slashes

Output on macOS

$ errcheck -blank ./backend/src/appcontext/...
backend/src/appcontext/context.go:103:3:        _ = session.AbortTransaction(context.TODO())
backend/src/appcontext/context.go:106:3:        _ = session.CommitTransaction(context.TODO())

$ errcheck -blank ./backend/src/appcontext/... |& revgrep origin/master
backend/src/appcontext/context.go:106:3:        _ = session.CommitTransaction(context.TODO())

$ errcheck -blank ./backend/src/appcontext/... |& revgrep -d origin/master
 ... skipping a lot of lines
DEBUG: path: "backend/src/appcontext/context.go", lineNo: 103, colNo: 3, msg: "_ = session.AbortTransaction(context.TODO())"
DEBUG: unchanged: backend/src/appcontext/context.go:103:3:      _ = session.AbortTransaction(context.TODO())
DEBUG: path: "backend/src/appcontext/context.go", lineNo: 106, colNo: 3, msg: "_ = session.CommitTransaction(context.TODO())"
backend/src/appcontext/context.go:106:3:        _ = session.CommitTransaction(context.TODO())

Output on Windows

>errcheck -blank ./backend/src/appcontext/...
backend\src\appcontext\context.go:103:3:        _ = session.AbortTransaction(context.TODO())
backend\src\appcontext\context.go:106:3:        _ = session.CommitTransaction(context.TODO())

>powershell -Command "& {errcheck -blank ./backend/src/appcontext/... |& revgrep origin/master}"

>powershell -Command "& {errcheck -blank ./backend/src/appcontext/... |& revgrep -d origin/master}"
 ... skipping a lot of lines
DEBUG: path: "backend\\src\\appcontext\\context.go", lineNo: 103, colNo: 3, msg: "_ = session.AbortTransaction(context.TODO())"
DEBUG: unchanged: backend\src\appcontext\context.go:103:3:      _ = session.AbortTransaction(context.TODO())
DEBUG: path: "backend\\src\\appcontext\\context.go", lineNo: 106, colNo: 3, msg: "_ = session.CommitTransaction(context.TODO())"
DEBUG: unchanged: backend\src\appcontext\context.go:106:3:      _ = session.CommitTransaction(context.TODO())

Sorry I cannot paste the entire debug output of revgrep since it is too large. Hope the info I provided can help. If not, fill free to ask me

---

Thank you for creating the issue!

• Yes, I'm using a binary release within 2 latest major releases. Only such installations are supported.
• Yes, I've searched similar issues on GitHub and didn't find any.
• Yes, I've included all information below (version, config, etc).

Please include the following information:

Version of golangci-lint

$ golangci-lint --version
golangci-lint has version 1.23.6 built from b9eef79 on 2020-02-10T17:51:52Z

Config file

$ cat .golangci.yml
run:
  deadline: 5m
  tests: true

linters:
  disable-all: true
  enable:
    - errcheck

Go environment

$ go version && go env
go version go1.13 darwin/amd64
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/onobrod/Library/Caches/go-build"
GOENV="/Users/onobrod/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/onobrod/Go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/z5/d7vy89t907z9p0gbx69xxqb80000gn/T/go-build666921686=/tmp/go-build -gno-record-gcc-switches -fno-common"

[lhyqy5]

I have the same problem, And seems there is a PR already fix this but doesn't merge.

[de1acr0ix]

@ernado @ldez Can the team get the fix in? I encounter the same problem and the fix PR seems to quite straightforward.

I have the same problem, And seems there is a PR already fix this but doesn't merge.

[ldez]

It's open-source, if you think that the fix is quite straightforward, you can fix it, PRs are welcome.

[de1acr0ix]

@ldez The PR is already raised in revgrep long ago, as mentioned in my previous comment. That one has to be merged before it is taken into golangci-lint, which I believe is only to update the Go modules...

[ldez]

I missed the link to the PR.

The changes can be seen as trivial but evaluating the possible regression is complex and time-consuming.