--new-from-rev gets confused by merge commits

[mering]

Welcome

• 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 read the typecheck section of the FAQ (https://golangci-lint.run/usage/faq/#why-do-you-have-typecheck-errors).
• Yes, I've tried with the standalone linter if available (e.g., gocritic, go vet, etc.). (https://golangci-lint.run/usage/linters/)

Description of the problem

Adding a reproduction example to demonstrate breakage of #4047.

Version of golangci-lint

$ ./golangci-lint --version
# 1.54.2

Configuration

No configuration file

--new-from-rev <sha of 1 commit before HEAD>

Go environment

Using binary

Verbose output of running

Running with --new-from-rev HEAD~1 doesn't show anything even though an unused variable was introduced in the last commit when an error already existed before a merge commit. When adding changes in unrelated files it shows all problems even though some of them have been introduced before.

A minimal reproducible example or link to a public repository

#!/bin/sh
set -x

REPO="rev-binary"
git init "$REPO" && pushd $_
echo golangci-lint > .gitignore
GOLANGCI_LINT_VERSION="1.54.2"
curl -Lf https://github.com/golangci/golangci-lint/releases/download/v${GOLANGCI_LINT_VERSION}/golangci-lint-${GOLANGCI_LINT_VERSION}-linux-amd64.tar.gz | tar xz -C . --strip-components=1 && chmod +x ./golangci-lint
git add .
git commit -m "chore: initial empty commit."

go mod init github.com/gcl/example 
git add .
git commit -m "chore: setup project"

cat <<EOF > foo.go
package pkg

func foo() {
    unusedVariableInFoo := 0
}
EOF
git add .
git commit -m "introduced unused var"

git diff HEAD~1
./golangci-lint run
./golangci-lint run --new-from-rev HEAD~1

git checkout -b b1
git commit -m "empty b1" --allow-empty
git switch -
git merge b1 --no-ff --no-edit

cat <<EOF >> foo.go

func foo2() {
    unusedVariableInFoo2 := 0
}
EOF
git add .
git commit -m "another prob"

git diff HEAD~1
./golangci-lint run
# NOTE the following command doesn't print anything although it should print the problem within bar()
./golangci-lint run --new-from-rev HEAD~1

cat <<EOF > bar.go
package pkg

func bar() {
    unusedVariableInBar := 0
}
EOF
git add .
git commit -m "another prob"

git diff HEAD~1
./golangci-lint run
# NOTE the following command also prints the problems in foo.go although this file didn't change in the last commit
./golangci-lint run --new-from-rev HEAD~1

echo "Cleanup"
popd
rm -rf "$REPO"

Validation

• Yes, I've included all information above (version, config, etc.).

[boring-cyborg]

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

[ldez]

Hello,

[x] Yes, I'm using a binary release within 2 latest major releases.

Have you tried with previous versions?

is there a difference?

[mering]

Which version do you want me to try? I am not aware of any previous working version. I tried 1.50.0 for example with the same result.
You can easily change the version string in the reproduction script and just run it yourself.

[udaya2899]

Hi @ldez, friendly reminder. Can you help us with this please?

[ldez]

This is how git works, so I have no magic solution.

[mering]

This is how git works, so I have no magic solution.

Can you elaborate on which part of Git workings you are referring to? To me the git diff looks as expected but golangci-lint doesn't interpret the diff correctly.

[janwytze]

I think I'm currently running in the same problem when using golangci-lint in Gitlab merge result pipielines. This is my CI stage:

lint:
  image: golangci/golangci-lint:latest-alpine
  script:
    - git fetch origin $CI_MERGE_REQUEST_TARGET_BRANCH_NAME && git branch $CI_MERGE_REQUEST_TARGET_BRANCH_NAME -f remotes/origin/$CI_MERGE_REQUEST_TARGET_BRANCH_NAME
    - git diff HEAD $CI_MERGE_REQUEST_TARGET_BRANCH_NAME # Works, and gives back files and lines that do contain linting errors.
    - golangci-lint run --new-from-rev $CI_MERGE_REQUEST_TARGET_BRANCH_NAME # No result, this same command works locally.

Locally it works fine, but within merge result pipelines it does not work :(

[mering]

@janwytze we discovered this in the GitHub CI which by default also creates a merge commit against the base branch. So golangci-lint is pretty much unusable for GitHub CI at the moment as well.

[janwytze]

@mering Hmm, but git diff does detect the difference, so how can it than be unusable?

golangci-lint should check the difference between the current HEAD, and the provided rev. In my case that is the target branch, which is set to the origin.

I've also added these commands:

- go install github.com/golangci/revgrep/cmd/revgrep@latest
- revgrep -d $CI_MERGE_REQUEST_TARGET_BRANCH_NAME HEAD

golangci-lint internally uses revgrep. This works well, also in the Gitlab CI merge result pipeline.

So both git diff and revgrep do detect the correct diff, also in the merge result pipeline. I just don't understand how golangci-lint run --new-from-rev never sees the changes.

[mering]

@janwytze I mean golangci-lint without this fixed is unusable. I updated my previous comment to clarify this.

[janwytze]

@mering Sorry I was also talking about golangci-lint. I don't get why both diff and revgrep work, but golangci-lint, that uses revgrep internally, does not work.

---

The hardest part about it, is that I am not able to reproduce this locally in any way. When I have some spare time I will try to find a way to reproduce it, and hopefully fix it.

Do you maybe have a way to reproduce it locally? I've tried creating a new branch of the target branch, merging it source branch in it and then running it with the origin target branch as rev. This works perfectly... This is the same as what a result pipeline does right?

[mering]

@janwytze yes I included a minimal reproduction example in the issue description. Just unfold the "Details".

[PiotrZakrzewski-instruqt]

What makes this issue more confusing is that there is a difference between how git installed via homebrew on MacOS behaves (it gets the diff mostly correctly) and how git from macos dev toosl behaves (this bug reproduces there for me).

[janwytze]

@PiotrZakrzewski-instruqt I noticed a lot in differences with my local golangci-lint installation, which seems to work perfect. And running the Docker image (v1.55.2, both normal and alpine), which behaves very weird.

I really wanted to get it working, so I created a CI stage which would soft reset all changes between the target branch and source branch, and make them unstaged. The --new flag should only check the unstaged changes when there are unstaged changes. But even then it somehow failed to detect them (found 38000 issues, filtered to 0).

Then I tried to replicate it with a clean repo, but then everything seems to work just fine. But most of our Gitlab repo's seem to break golangci-lint, but I'm still not able to fully figure out why.

So shortly, there are differences between my local installation and the docker image (maybe git version). And fresh repo's seem to work fine, but the Gitlab repo's we want to apply the linting to don't seem to cooperate.

[mering]

@janwytze did you try the minimal reproduction example from the issue description?

[janwytze]

@mering Yes I did, your example gives the same results on my local installation and in Docker. I've also debugged through the source code with your example, but making a fix is not that easy :(

I don't know if my issue where unstaged changes with the --new flag is related to your issue.

[ldez]

The example (from the description) is not really a good example because the reports are only related to typecheck and typecheck is not a linter.

If you look closely at the output:

foo.go:1: : # github.com/gcl/example
./foo.go:4:5: unusedVariableInFoo declared and not used
./foo.go:8:5: unusedVariableInFoo2 declared and not used (typecheck)

You can see that the report is on foo.go:1: and not ./foo.go:4:5: or ./foo.go:8:5:.
All the text after foo.go:1: : is just the content of the message and not several reports.

As the error is a compilation error, it's not possible to skip it.

The 2nd case (after the merge with the comment # NOTE the following command doesn't print anything although it should print the problem within bar()) is a bug because typecheck errors should never be ignored/skipped.

If someone can create a reproducible example with a real linter, it can help.

[ldez]

I modified the script to be based on a real linter:

modified script

#!/bin/sh
set -x

REPO="rev-binary"
rm -rf $REPO

git init "$REPO" && pushd $_
echo golangci-lint > .gitignore

GOLANGCI_LINT_VERSION="1.57.2"
curl -Lf https://github.com/golangci/golangci-lint/releases/download/v${GOLANGCI_LINT_VERSION}/golangci-lint-${GOLANGCI_LINT_VERSION}-linux-amd64.tar.gz | tar xz -C . --strip-components=1 && chmod +x ./golangci-lint

git add .
git commit -m "chore: initial commit."

go mod init github.com/gcl/example
git add .
git commit -m "chore: setup project"

cat <<EOF > foo.go
package example

import "errors"

func foo(a string) error {
	if a == "a" {
		return nil
	}

	return errors.New("OOPS")
}

func Bar1() {
	foo("b")
}
EOF

git add .
git commit -m "introduced problem"

git diff HEAD~1
# 1 issue expected.
./golangci-lint run
# 1 issue expected.
./golangci-lint run --new-from-rev HEAD~1

git checkout -b b1
git commit -m "empty b1" --allow-empty
git switch -
git merge b1 --no-ff --no-edit

cat <<EOF >> foo.go

func Bar2() {
	foo("b")
}
EOF

git add .
git commit -m "another prob"

git diff HEAD~1
# 2 issue expected.
./golangci-lint run
# 1 issue expected.
./golangci-lint run --new-from-rev HEAD~1

cat <<EOF > bar.go
package example

func BarBar() {
	foo("b")
}
EOF
git add .
git commit -m "another prob"

git diff HEAD~1
# 3 issue expected.
./golangci-lint run
# 1 issue expected.
./golangci-lint run --new-from-rev HEAD~1

echo "Cleanup"
popd
rm -rf "$REPO"

Everything works as expected.

I think the root of the confusion was only because the text of the typecheck report contains information related to files information (path and position).

The other behavior is related to the fact that the diff processor, in some unexpected cases, ignored/skipped typecheck errors, this will be fixed in #4674.