make --fix work (naively) with --prefix-path

[moitias]

Issue

Currently, using --path-prefix together with --fix does not work with any other path prefix than "".

This is easily reproduced merely by adding --path-prefix=something to the command line when also using --fix (and, of course, actually having something fixable) or adding it to the arguments used in TestFix, which will cause the test to fail.

This is problematic for my use case, a repository with multiple go modules in it (in separate directories) where I'd like to run golangci-lint on all of them with pre-commit before commiting. To do this, I did not figure out other approaches than to run golangci-lint in each of those directories and using --path-prefix to add the module directory to the output which does not work.

Reproduction

matias@kiri ~/c/t/fix_sample> ls
main.go
matias@kiri ~/c/t/fix_sample> cat main.go 
package main

func main() { 
	println(  
"hello") 
}
matias@kiri ~/c/t/fix_sample> golangci-lint version
golangci-lint has version 1.42.0 built from c6142e38 on 2021-08-17T11:47:22Z
matias@kiri ~/c/t/fix_sample> golangci-lint run --fix --path-prefix=foo ./main.go -v --disable-all --enable gofmt
INFO [config_reader] Config search paths: [./ /home/matias/code/temp/fix_sample /home/matias/code/temp /home/matias/code /home/matias /home /] 
INFO [lintersdb] Active 1 linters: [gofmt]        
INFO [loader] Go packages loading at mode 7 (compiled_files|files|name) took 41.533512ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 122.782µs 
INFO [linters context/goanalysis] analyzers took 731.031µs with top 10 stages: gofmt: 731.031µs 
INFO [runner] Processors filtering stat (out/in): max_same_issues: 1/1, max_from_linter: 1/1, severity-rules: 1/1, path_prefixer: 1/1, sort_results: 1/1, identifier_marker: 1/1, path_prettifier: 1/1, autogenerated_exclude: 1/1, exclude: 1/1, nolint: 1/1, uniq_by_line: 1/1, max_per_file_from_linter: 1/1, path_shortener: 1/1, filename_unadjuster: 1/1, diff: 1/1, skip_files: 1/1, skip_dirs: 1/1, exclude-rules: 1/1, source_code: 1/1, cgo: 1/1 
INFO [runner] processing took 123.9µs with stages: identifier_marker: 22.489µs, exclude-rules: 14.388µs, nolint: 13.41µs, path_prettifier: 12.431µs, autogenerated_exclude: 12.153µs, source_code: 9.429µs, skip_dirs: 5.308µs, cgo: 3.841µs, exclude: 3.352µs, skip_files: 2.934µs, sort_results: 2.864µs, uniq_by_line: 2.793µs, diff: 2.445µs, severity-rules: 2.444µs, path_shortener: 2.375µs, path_prefixer: 2.374µs, max_same_issues: 2.305µs, filename_unadjuster: 2.305µs, max_per_file_from_linter: 2.304µs, max_from_linter: 1.956µs 
INFO [runner] linters took 1.433009ms with stages: gofmt: 1.159161ms 
ERRO Failed to fix issues in file foo/main.go: failed to get file bytes for foo/main.go: can't read file foo/main.go: open foo/main.go: no such file or directory 
INFO fixer took 16.623µs with stages: all: 16.623µs 
foo/main.go:3: File is not `gofmt`-ed with `-s` (gofmt)
func main() { 
	println(  
"hello") 
INFO File cache stats: 1 entries of total size 53B 
INFO Memory: 2 samples, avg is 72.7MB, max is 72.8MB 
INFO Execution took 50.472601ms           

(even though this test was ran with not quite the latest version, the issue reproduces with tip of master as well)

The error in the output clearly points out the problem;

ERRO Failed to fix issues in file foo/main.go: failed to get file bytes for foo/main.go: can't read file foo/main.go: open foo/main.go: no such file or directory

That is, the path prefix is prepended to the path of the file to be fixed which does not seem to be what is intended, as the help / documentation states that --prefix-path configures the Path prefix to add to **output**.

Fix

This PR adds a mock --path-prefix argument to TestFix and implements a (very naive) fix for the issue, stripping the configured path prefix from the path passed to Fixer.fixIssuesInFile if the configured path prefix is not "".

I'm happy to create an alternative fix if some other approach is seen to be better, options off the top of my head would include;

• Adding the actual path to token.Position, allowing Fixer to use it instead.
• Adding the actual path to Issue, allowing Issue.FilePath() to return it, as this is how Fixer seems to determine the path to use.
• Adding the path prefix only to (all of?) the output(s) rather than prefixing token.Position.Filename as there could be other needs for the actual path of the file in the future? This seems like the best option to me but the details of how to do this and implications it could have are unclear to me as I'm quite unfamiliar with the codebase, so would need some pointers to implement this.

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[SVilgelm]

Great idea, but please add the tests

[moitias]

Hi, thanks for the review and sorry for the delay!

@SVilgelm This is tested, but it's in the same TestFix test as all other fix-related testing (as far as I can tell?).

I can break it into a separate test but I'm not sure what the value of that would be, as my uneducated gut feeling is that it would be pretty much the exact same test with less validation (and inputs) but more implied maintenance down the road.

Happy to do either so let me know what you'd prefer.

[scop]

@SVilgelm @alexandear sorry about the ping, just making sure you've noticed @moitias' comment above -- if you could let us know what you think so we can get this moving forward. Thanks!

[SVilgelm]

What will happen if the path prefix contains / at the end? --path-prefix=app/
Do we have any checks or are we using something like path.Clean()?

[SVilgelm]

I'm not sure about this. Right now you just replace one test with another.
It would be better to have different tests for the cases without and with path prefix

[scop]

Hi @moitias, others, I took the liberty to submit #3626 which builds on this, resolves conflicts, and addresses remaining open comments here.