fix: consider path prefix when matching path patterns #3571

pohly · 2023-02-06T19:42:52Z

Problem

When someone invokes golangci-lint in a sub-directory, they can set the path prefix to make the output look like the invocation had been done in the root.
But this prefix was ignored when checking path patterns of exclude, severity, and skip files/dirs entries, so those with matching for directories only worked when golangci-lint was always invoked in the same directory.

Solution

The underlying problem is that the rules from the configuration get checked before updating the path associated with the issues in the path fixer.
To make the prefix work, it now gets passed down into processors and added there to the issue path before checking against the path pattern.

For exclude and severity rules, this could have been done through a separate parameter, but bundling it in a new fsutils helper struct made the change a bit smaller.

boring-cyborg · 2023-02-06T19:42:56Z

Hey, thank you for opening your first Pull Request !

CLAassistant · 2023-02-06T19:42:58Z

All committers have signed the CLA.

pohly · 2023-02-06T19:46:53Z

pkg/lint/runner.go

@@ -83,7 +83,7 @@ func NewRunner(cfg *config.Config, log logutils.Log, goenv *goutil.Env, es *lint
 			processors.NewIdentifierMarker(),

 			getExcludeProcessor(&cfg.Issues),
-			getExcludeRulesProcessor(&cfg.Issues, log, lineCache),
+			getExcludeRulesProcessor(&cfg.Issues, log, cfg.Output.PathPrefix, lineCache),


It looks like path prefix and line cache get passed through different functions together. To minimize changes I had originally made "path prefix" a field in the line cache (or rather, the file cache, with an accessor in the line cache).

That didn't work because the caches got created before parsing the configuration. I would have had to add a SetPathPrefix to add that later, which didn't feel right.

Do you have any preference?

In the end I went for an approach where line cache and path prefix get combined. This keeps the number of parameters that need to be passed through the same.

Here's a diff against the earlier approach with two parameters: https://github.com/golangci/golangci-lint/compare/9664d89f477ef60739fe6ec54792b50d43099e71..d7853eaed135202c0aff13ccd6db22abe8e27d56

pohly · 2023-02-06T19:49:45Z

pkg/result/processors/base_rule.go

 	if r.isEmpty() {
 		return false
 	}
 	if r.text != nil && !r.text.MatchString(issue.Text) {
 		return false
 	}
-	if r.path != nil && !r.path.MatchString(issue.FilePath()) {
+	if r.path != nil && !r.path.MatchString(path.Join(pathPrefix, issue.FilePath())) {


If pathPrefix is empty, pathPrefix will ignore it. But the result is "cleaned" - I am not sure whether that is potentially harmful (performance, Windows separator?).

If "path prefix" was an attribute in lineCache, then a LineCache.WithPathPrefix(p string) method could be added to do the extension of the path in a consistent way.

path.Join is correct. The path prefixer does the same. To handle Windows, the path regex gets mangled to match the separator used by the platform.

pohly · 2023-02-06T19:50:32Z

pkg/result/processors/exclude_rules_test.go

@@ -37,7 +37,7 @@ func TestExcludeRulesMultiple(t *testing.T) {
 				Linters: []string{"lll"},
 			},
 		},
-	}, lineCache, nil)
+	}, "" /* path prefix */, lineCache, nil)


I'll add unit tests once I know how this is supposed to be handled.

I've added them.

pohly · 2023-02-09T10:29:20Z

@ldez: I also looked at skip-files and skip-dirs. Both had the same problem, so I extended the PR to also fix those.

I think this is ready for review now.

pohly · 2023-02-09T10:34:10Z

In case someone wonders: the reason why the directory has to be changed in the Kubernetes repo is that staging/src/k8s.io/* are all separate modules, so golangci-lint ./staging/src/k8s.io/... wouldn't work. But we want to maintain a single golangci.yaml for all modules with path matches for certain directories, hence this PR...

When someone invokes golangci-lint in a sub-directory, they can set the path prefix to make the output look like the invocation had been done in the root. But this prefix was ignored when checking path patterns of exclude, severity, and skip files/dirs entries, so those with matching for directories only worked when golangci-lint was always invoked in the same directory. The underlying problem is that the rules from the configuration get checked before updating the path associated with the issues in the path fixer. To make the prefix work, it now gets passed down into processors and added there to the issue path before checking against the path pattern. For exclude and severity rules, this could have been done through a separate parameter, but bundling it in a new fsutils helper struct made the change a bit smaller.

pohly · 2023-02-26T20:03:52Z

The path prefixer test failed on Windows because I was using "path.Join", not "path/filepath.Join" as before. This change should fix that:

diff --git a/pkg/fsutils/files.go b/pkg/fsutils/files.go
index 542af531..3d369a23 100644
--- a/pkg/fsutils/files.go
+++ b/pkg/fsutils/files.go
@@ -1,6 +1,6 @@
 package fsutils
 
-import "path"
+import "path/filepath"
 
 // Files combines different operations related to handling file paths and
 // content.
@@ -32,5 +32,5 @@ func WithPathPrefix(pathPrefix, relativePath string) string {
        if pathPrefix == "" {
                return relativePath
        }
-       return path.Join(pathPrefix, relativePath)
+       return filepath.Join(pathPrefix, relativePath)
 }

ldez

I think your approach can be extended to the Fixer processor, can try to fix the Fixer?

docs/src/docs/usage/false-positives.mdx

pkg/fsutils/files.go

pkg/lint/runner.go

Co-authored-by: Ludovic Fernandez <ldez@users.noreply.github.com>

pohly · 2023-03-16T15:25:42Z

I think your approach can be extended to the Fixer processor, can try to fix the Fixer?

The problem with the fixer processor seems to be that it runs after the path prefixer (#3626).

That leads to the inverse problem of what I am solving here: the fixer sees the path with prefix which doesn't match the correct path relative to the current working directory, whereas severity and exclude processors (before this PR) see the path without the prefix when they need it with prefix.

The fix for the fixer in the PR above is to strip the prefix again. That doesn't seem ideal. Perhaps a simpler solution is to move the path prefixing and sorting out of the normal set of processors and do that after fixing?

Either way, such a fix doesn't need to be in this PR.

Thanks for the comment fixes, I included all of them in this commit - I prefer to keep the commit history clean. Let me know if you prefer it otherwise and I can restore them as a separate commit.

ldez · 2023-03-16T15:31:14Z

Thanks for the comment fixes, I included all of them in this commit - I prefer to keep the commit history clean. Let me know if you prefer it otherwise and I can restore them as a separate commit.

I prefer to have fixes as commits, it's easier to follow the review.

pohly · 2023-03-16T17:11:41Z

Okay, I added the second commit with the comment changes.

BTW, should I also rebase each time I do a push to avoid having to merge with master?

pohly · 2023-03-16T17:14:37Z

I prepared a separate PR with my idea of fixing the fixer. It's unrelated to the changes in this PR, but there will be a slight merge conflict.

I can also add those other changes here, if you prefer a single PR.

ldez

LTGM

This patch enables the golangci-lint checks for csi-wrapper. The version of golangci-lint is bumped to v1.52.2, since the old version has a bug when processing the --prefix-path option, which is necessary to run the lint tool in sub modules. golangci/golangci-lint#3571 Signed-off-by: Yohei Ueda <yohei@jp.ibm.com>

pohly commented Feb 6, 2023

View reviewed changes

pohly changed the title ~~WIP: consider path prefix when matching exclude rules~~ RFC: consider path prefix when matching exclude rules Feb 6, 2023

ldez changed the title ~~RFC: consider path prefix when matching exclude rules~~ Consider path prefix when matching exclude rules Feb 6, 2023

ldez marked this pull request as draft February 6, 2023 21:49

ldez added the area: config Related to .golangci.yml and/or cli options label Feb 6, 2023

pohly force-pushed the path-filtering branch from 9664d89 to d7853ea Compare February 8, 2023 20:29

pohly marked this pull request as ready for review February 8, 2023 20:33

pohly force-pushed the path-filtering branch from d7853ea to faf5560 Compare February 8, 2023 20:33

pohly changed the title ~~Consider path prefix when matching exclude rules~~ Consider path prefix when matching exclude and severity rules Feb 8, 2023

pohly force-pushed the path-filtering branch from faf5560 to cdbc500 Compare February 9, 2023 06:56

pohly mentioned this pull request Feb 9, 2023

per-file patterns ashanbrown/forbidigo#22

Closed

pohly force-pushed the path-filtering branch from cdbc500 to 33826ae Compare February 9, 2023 10:26

pohly changed the title ~~Consider path prefix when matching exclude and severity rules~~ Consider path prefix when matching path patterns Feb 9, 2023

ldez force-pushed the path-filtering branch from 933ba6f to 3ae5acb Compare February 25, 2023 02:55

pohly force-pushed the path-filtering branch from 3ae5acb to 3107130 Compare February 26, 2023 20:02

pohly mentioned this pull request Mar 1, 2023

writing-good-e2e-tests.md: polling, contexts and failure messages kubernetes/community#7021

Merged

ldez self-requested a review March 6, 2023 02:30

ldez added the enhancement New feature or improvement label Mar 15, 2023

ldez requested changes Mar 15, 2023

View reviewed changes

ldez added the feedback required Requires additional feedback label Mar 15, 2023

Apply suggestions from code review

29f1516

Co-authored-by: Ludovic Fernandez <ldez@users.noreply.github.com>

pohly force-pushed the path-filtering branch from 29f1516 to 91e4b2e Compare March 16, 2023 15:25

pohly force-pushed the path-filtering branch from 91e4b2e to 29f1516 Compare March 16, 2023 17:10

ldez removed the feedback required Requires additional feedback label Mar 16, 2023

ldez approved these changes Mar 17, 2023

View reviewed changes

ldez added bug Something isn't working and removed enhancement New feature or improvement labels Mar 17, 2023

ldez changed the title ~~Consider path prefix when matching path patterns~~ fix: consider path prefix when matching path patterns Mar 17, 2023

ldez merged commit b40a544 into golangci:master Mar 17, 2023

pohly mentioned this pull request Mar 18, 2023

fix: combination of --fix and --path-prefix #3700

Merged

SeigeC pushed a commit to SeigeC/golangci-lint that referenced this pull request Apr 4, 2023

fix: consider path prefix when matching path patterns (golangci#3571)

eeaf105

ldez added this to the v1.52 milestone Mar 6, 2024

alexandear mentioned this pull request Mar 15, 2024

docs fix broken links alexandear/golangci-lint#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: consider path prefix when matching path patterns #3571

fix: consider path prefix when matching path patterns #3571

pohly commented Feb 6, 2023 •

edited by ldez

boring-cyborg bot commented Feb 6, 2023

CLAassistant commented Feb 6, 2023 •

edited

pohly Feb 6, 2023

pohly Feb 8, 2023

pohly Feb 9, 2023

pohly Feb 6, 2023

pohly Feb 8, 2023

pohly Feb 6, 2023

pohly Feb 8, 2023

pohly commented Feb 9, 2023

pohly commented Feb 9, 2023

pohly commented Feb 26, 2023

ldez left a comment

pohly commented Mar 16, 2023

ldez commented Mar 16, 2023 •

edited

pohly commented Mar 16, 2023

pohly commented Mar 16, 2023 •

edited

ldez left a comment

fix: consider path prefix when matching path patterns #3571

fix: consider path prefix when matching path patterns #3571

Conversation

pohly commented Feb 6, 2023 • edited by ldez

Problem

Solution

boring-cyborg bot commented Feb 6, 2023

CLAassistant commented Feb 6, 2023 • edited

pohly Feb 6, 2023

Choose a reason for hiding this comment

pohly Feb 8, 2023

Choose a reason for hiding this comment

pohly Feb 9, 2023

Choose a reason for hiding this comment

pohly Feb 6, 2023

Choose a reason for hiding this comment

pohly Feb 8, 2023

Choose a reason for hiding this comment

pohly Feb 6, 2023

Choose a reason for hiding this comment

pohly Feb 8, 2023

Choose a reason for hiding this comment

pohly commented Feb 9, 2023

pohly commented Feb 9, 2023

pohly commented Feb 26, 2023

ldez left a comment

Choose a reason for hiding this comment

pohly commented Mar 16, 2023

ldez commented Mar 16, 2023 • edited

pohly commented Mar 16, 2023

pohly commented Mar 16, 2023 • edited

ldez left a comment

Choose a reason for hiding this comment

pohly commented Feb 6, 2023 •

edited by ldez

CLAassistant commented Feb 6, 2023 •

edited

ldez commented Mar 16, 2023 •

edited

pohly commented Mar 16, 2023 •

edited