chore: add more linters, address findings (#670)

* chore(golangci-lint): remove gofmt, subset of gofumpt * chore(golangci-lint): enable most gocritic performance checks Many of them also promote clean, idiomatic code. * fix: appending to non-zero length initialized arrays Per makezero linter findings. * chore(typos): accept decorder (the golangci-lint linter) * chore(golangci-lint): add some useful linters with no current findings * chore(golangci-lint): upgrade to 1.56.2 * perf: address perfsprint linter findings Enable the linter, but exclude fmt.Sprintf -> concatenation, as the former is arguably often more readable. * style: sort struct tags alphabetically Per the tagalign linter autofix. * fix: siplify skip_output and output logic --------- Co-authored-by: Valentin Kiselev <[email protected]>
evilmartians · Mar 13, 2024 · 15190c8 · 15190c8
1 parent 96a206e
commit 15190c8
Show file tree

Hide file tree

Showing 16 changed files with 302 additions and 396 deletions.
diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
@@ -18,4 +18,4 @@ jobs:
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v3
         with:
-          version: v1.54.1
+          version: v1.56.2
diff --git a/.golangci.yml b/.golangci.yml
@@ -1,4 +1,9 @@
 linters-settings:
+  gocritic:
+    disabled-checks:
+      - hugeParam
+    enabled-tags:
+      - performance
   govet:
     check-shadowing: true
   misspell:
@@ -10,6 +15,12 @@ linters-settings:
       - name: unused-parameter
         disabled: true
 
+issues:
+  exclude:
+    # https://github.com/catenacyber/perfsprint/issues/21
+    # https://github.com/catenacyber/perfsprint/issues/23
+    - "^fmt\\.Sprintf can be replaced with string (addi|concatena)tion$"
+
 linters:
   disable-all: true
   enable:
@@ -38,7 +49,6 @@ linters:
     - gocyclo
     - godot
     - godox
-    - gofmt
     - gofumpt
     - goheader
     - goimports
@@ -47,14 +57,22 @@ linters:
     - gosimple
     - govet
     - ineffassign
+    - makezero
+    - mirror
     - misspell
     - nestif
     - noctx
     - nolintlint
+    - perfsprint
+    - predeclared
+    - reassign
     - revive
     - staticcheck
+    - tagalign
+    - tenv
     - typecheck
     - unconvert
     - unparam
     - unused
+    - usestdlibvars
     - whitespace
diff --git a/.typos.toml b/.typos.toml
@@ -0,0 +1,3 @@
+[default.extend-words]
+# .golangci.yml
+decorder = "decorder"
diff --git a/Makefile b/Makefile
@@ -20,7 +20,7 @@ bench:
 
 bin/golangci-lint:
 	@test -x $$(go env GOPATH)/bin/golangci-lint || \
-		curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $$(go env GOPATH)/bin v1.54.1
+		curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $$(go env GOPATH)/bin v1.56.2
 
 lint: bin/golangci-lint
 	$$(go env GOPATH)/bin/golangci-lint run --fix

diff --git a/internal/config/command.go b/internal/config/command.go
@@ -12,23 +12,23 @@ import (
 var errFilesIncompatible = errors.New("One of your runners contains incompatible file types")
 
 type Command struct {
-	Run string `mapstructure:"run" yaml:"run" json:"run" toml:"run"`
-
-	Skip  interface{}       `mapstructure:"skip"  yaml:",omitempty"  json:"skip,omitempty"  toml:"skip,omitempty,inline"`
-	Only  interface{}       `mapstructure:"only"  yaml:",omitempty"  json:"only,omitempty"  toml:"only,omitempty,inline"`
-	Tags  []string          `mapstructure:"tags"  yaml:",omitempty"  json:"tags,omitempty"  toml:"tags,omitempty"`
-	Glob  string            `mapstructure:"glob"  yaml:",omitempty"  json:"glob,omitempty"  toml:"glob,omitempty"`
-	Files string            `mapstructure:"files" yaml:",omitempty"  json:"files,omitempty" toml:"files,omitempty"`
-	Env   map[string]string `mapstructure:"env"   yaml:",omitempty"  json:"env,omitempty"   toml:"env,omitempty"`
-
-	Root     string `mapstructure:"root"     yaml:",omitempty" json:"root,omitempty"     toml:"root,omitempty"`
-	Exclude  string `mapstructure:"exclude"  yaml:",omitempty" json:"exclude,omitempty"  toml:"exclude,omitempty"`
-	Priority int    `mapstructure:"priority" yaml:",omitempty" json:"priority,omitempty" toml:"priority,omitempty"`
-
-	FailText    string `mapstructure:"fail_text"   yaml:"fail_text,omitempty"   json:"fail_text,omitempty"   toml:"fail_text,omitempty"`
-	Interactive bool   `mapstructure:"interactive" yaml:",omitempty"            json:"interactive,omitempty" toml:"interactive,omitempty"`
-	UseStdin    bool   `mapstructure:"use_stdin"   yaml:",omitempty"            json:"use_stdin,omitempty"   toml:"use_stdin,omitempty"`
-	StageFixed  bool   `mapstructure:"stage_fixed" yaml:"stage_fixed,omitempty" json:"stage_fixed,omitempty" toml:"stage_fixed,omitempty"`
+	Run string `json:"run" mapstructure:"run" toml:"run" yaml:"run"`
+
+	Skip  interface{}       `json:"skip,omitempty"  mapstructure:"skip"  toml:"skip,omitempty,inline" yaml:",omitempty"`
+	Only  interface{}       `json:"only,omitempty"  mapstructure:"only"  toml:"only,omitempty,inline" yaml:",omitempty"`
+	Tags  []string          `json:"tags,omitempty"  mapstructure:"tags"  toml:"tags,omitempty"        yaml:",omitempty"`
+	Glob  string            `json:"glob,omitempty"  mapstructure:"glob"  toml:"glob,omitempty"        yaml:",omitempty"`
+	Files string            `json:"files,omitempty" mapstructure:"files" toml:"files,omitempty"       yaml:",omitempty"`
+	Env   map[string]string `json:"env,omitempty"   mapstructure:"env"   toml:"env,omitempty"         yaml:",omitempty"`
+
+	Root     string `json:"root,omitempty"     mapstructure:"root"     toml:"root,omitempty"     yaml:",omitempty"`
+	Exclude  string `json:"exclude,omitempty"  mapstructure:"exclude"  toml:"exclude,omitempty"  yaml:",omitempty"`
+	Priority int    `json:"priority,omitempty" mapstructure:"priority" toml:"priority,omitempty" yaml:",omitempty"`
+
+	FailText    string `json:"fail_text,omitempty"   mapstructure:"fail_text"   toml:"fail_text,omitempty"   yaml:"fail_text,omitempty"`
+	Interactive bool   `json:"interactive,omitempty" mapstructure:"interactive" toml:"interactive,omitempty" yaml:",omitempty"`
+	UseStdin    bool   `json:"use_stdin,omitempty"   mapstructure:"use_stdin"   toml:"use_stdin,omitempty"   yaml:",omitempty"`
+	StageFixed  bool   `json:"stage_fixed,omitempty" mapstructure:"stage_fixed" toml:"stage_fixed,omitempty" yaml:"stage_fixed,omitempty"`
 }
 
 func (c Command) Validate() error {

diff --git a/internal/config/hook.go b/internal/config/hook.go
@@ -18,20 +18,20 @@ type Hook struct {
 	// Should be unmarshalled with `mapstructure:"commands"`
 	// But replacing '{cmd}' is still an issue
 	// Unmarshalling it manually, so omit auto unmarshalling
-	Commands map[string]*Command `mapstructure:"-" yaml:",omitempty" json:"commands,omitempty" toml:"commands,omitempty"`
+	Commands map[string]*Command `json:"commands,omitempty" mapstructure:"-" toml:"commands,omitempty" yaml:",omitempty"`
 
 	// Should be unmarshalled with `mapstructure:"scripts"`
 	// But parsing keys with dots in it is still an issue: https://github.com/spf13/viper/issues/324
 	// Unmarshalling it manually, so omit auto unmarshalling
-	Scripts map[string]*Script `mapstructure:"-" yaml:",omitempty" json:"scripts,omitempty" toml:"scripts,omitempty"`
-
-	Files       string      `mapstructure:"files"        yaml:",omitempty"             json:"files,omitempty"        toml:"files,omitempty"`
-	Parallel    bool        `mapstructure:"parallel"     yaml:",omitempty"             json:"parallel,omitempty"     toml:"parallel,omitempty"`
-	Piped       bool        `mapstructure:"piped"        yaml:",omitempty"             json:"piped,omitempty"        toml:"piped,omitempty"`
-	Follow      bool        `mapstructure:"follow"       yaml:",omitempty"             json:"follow,omitempty"       toml:"follow,omitempty"`
-	ExcludeTags []string    `mapstructure:"exclude_tags" yaml:"exclude_tags,omitempty" json:"exclude_tags,omitempty" toml:"exclude_tags,omitempty"`
-	Skip        interface{} `mapstructure:"skip"         yaml:",omitempty"             json:"skip,omitempty"         toml:"skip,omitempty,inline"`
-	Only        interface{} `mapstructure:"only"         yaml:",omitempty"             json:"only,omitempty"         toml:"only,omitempty,inline"`
+	Scripts map[string]*Script `json:"scripts,omitempty" mapstructure:"-" toml:"scripts,omitempty" yaml:",omitempty"`
+
+	Files       string      `json:"files,omitempty"        mapstructure:"files"        toml:"files,omitempty"        yaml:",omitempty"`
+	Parallel    bool        `json:"parallel,omitempty"     mapstructure:"parallel"     toml:"parallel,omitempty"     yaml:",omitempty"`
+	Piped       bool        `json:"piped,omitempty"        mapstructure:"piped"        toml:"piped,omitempty"        yaml:",omitempty"`
+	Follow      bool        `json:"follow,omitempty"       mapstructure:"follow"       toml:"follow,omitempty"       yaml:",omitempty"`
+	ExcludeTags []string    `json:"exclude_tags,omitempty" mapstructure:"exclude_tags" toml:"exclude_tags,omitempty" yaml:"exclude_tags,omitempty"`
+	Skip        interface{} `json:"skip,omitempty"         mapstructure:"skip"         toml:"skip,omitempty,inline"  yaml:",omitempty"`
+	Only        interface{} `json:"only,omitempty"         mapstructure:"only"         toml:"only,omitempty,inline"  yaml:",omitempty"`
 }
 
 func (h *Hook) Validate() error {

diff --git a/internal/config/remote.go b/internal/config/remote.go
@@ -1,11 +1,11 @@
 package config
 
 type Remote struct {
-	GitURL string `mapstructure:"git_url"           yaml:"git_url"    json:"git_url,omitempty" toml:"git_url"`
-	Ref    string `mapstructure:"ref,omitempty"     yaml:",omitempty" json:"ref,omitempty"     toml:"ref,omitempty"`
+	GitURL string `json:"git_url,omitempty" mapstructure:"git_url"       toml:"git_url"       yaml:"git_url"`
+	Ref    string `json:"ref,omitempty"     mapstructure:"ref,omitempty" toml:"ref,omitempty" yaml:",omitempty"`
 	// Deprecated
-	Config  string   `mapstructure:"config,omitempty"  yaml:",omitempty" json:"config,omitempty"  toml:"config,omitempty"`
-	Configs []string `mapstructure:"configs,omitempty" yaml:",omitempty" json:"configs,omitempty" toml:"configs,omitempty"`
+	Config  string   `json:"config,omitempty"  mapstructure:"config,omitempty"  toml:"config,omitempty"  yaml:",omitempty"`
+	Configs []string `json:"configs,omitempty" mapstructure:"configs,omitempty" toml:"configs,omitempty" yaml:",omitempty"`
 }
 
 func (r *Remote) Configured() bool {

diff --git a/internal/config/script.go b/internal/config/script.go
@@ -10,17 +10,17 @@ import (
 )
 
 type Script struct {
-	Runner string `mapstructure:"runner" yaml:"runner" json:"runner" toml:"runner"`
+	Runner string `json:"runner" mapstructure:"runner" toml:"runner" yaml:"runner"`
 
-	Skip interface{}       `mapstructure:"skip" yaml:",omitempty" json:"skip,omitempty" toml:"skip,omitempty,inline"`
-	Only interface{}       `mapstructure:"only" yaml:",omitempty" json:"only,omitempty" toml:"only,omitempty,inline"`
-	Tags []string          `mapstructure:"tags" yaml:",omitempty" json:"tags,omitempty" toml:"tags,omitempty"`
-	Env  map[string]string `mapstructure:"env"  yaml:",omitempty" json:"env,omitempty"  toml:"env,omitempty"`
+	Skip interface{}       `json:"skip,omitempty" mapstructure:"skip" toml:"skip,omitempty,inline" yaml:",omitempty"`
+	Only interface{}       `json:"only,omitempty" mapstructure:"only" toml:"only,omitempty,inline" yaml:",omitempty"`
+	Tags []string          `json:"tags,omitempty" mapstructure:"tags" toml:"tags,omitempty"        yaml:",omitempty"`
+	Env  map[string]string `json:"env,omitempty"  mapstructure:"env"  toml:"env,omitempty"         yaml:",omitempty"`
 
-	FailText    string `mapstructure:"fail_text"   yaml:"fail_text,omitempty"   json:"fail_text,omitempty"   toml:"fail_text,omitempty"`
-	Interactive bool   `mapstructure:"interactive" yaml:",omitempty"            json:"interactive,omitempty" toml:"interactive,omitempty"`
-	UseStdin    bool   `mapstructure:"use_stdin"   yaml:",omitempty"            json:"use_stdin,omitempty"   toml:"use_stdin,omitempty"`
-	StageFixed  bool   `mapstructure:"stage_fixed" yaml:"stage_fixed,omitempty" json:"stage_fixed,omitempty" toml:"stage_fixed,omitempty"`
+	FailText    string `json:"fail_text,omitempty"   mapstructure:"fail_text"   toml:"fail_text,omitempty"   yaml:"fail_text,omitempty"`
+	Interactive bool   `json:"interactive,omitempty" mapstructure:"interactive" toml:"interactive,omitempty" yaml:",omitempty"`
+	UseStdin    bool   `json:"use_stdin,omitempty"   mapstructure:"use_stdin"   toml:"use_stdin,omitempty"   yaml:",omitempty"`
+	StageFixed  bool   `json:"stage_fixed,omitempty" mapstructure:"stage_fixed" toml:"stage_fixed,omitempty" yaml:"stage_fixed,omitempty"`
 }
 
 func (s Script) DoSkip(gitState git.State) bool {

diff --git a/internal/lefthook/install.go b/internal/lefthook/install.go
@@ -4,7 +4,7 @@ import (
 	"bufio"
 	"crypto/md5"
 	"encoding/hex"
-	"fmt"
+	"errors"
 	"io"
 	"os"
 	"path/filepath"
@@ -31,7 +31,7 @@ const (
 var (
 	lefthookChecksumRegexp = regexp.MustCompile(`(\w+)\s+(\d+)`)
 	configGlob             = glob.MustCompile("{.,}lefthook.{yml,yaml,json,toml}")
-	errNoConfig            = fmt.Errorf("no lefthook config found")
+	errNoConfig            = errors.New("no lefthook config found")
 )
 
 // Install installs the hooks from config file to the .git/hooks.

diff --git a/internal/lefthook/run.go b/internal/lefthook/run.go
@@ -76,21 +76,16 @@ func (l *Lefthook) Run(hookName string, args RunArgs, gitArgs []string) error {
 		log.SetLevel(log.WarnLevel)
 	}
 
-	outputLogTags := os.Getenv(envOutput)
-	outputSkipTags := os.Getenv(envSkipOutput)
+	enableLogTags := os.Getenv(envOutput)
+	disableLogTags := os.Getenv(envSkipOutput)
 
-	var logSettings log.Settings
+	logSettings := log.NewSettings()
+	logSettings.Apply(enableLogTags, disableLogTags, cfg.Output, cfg.SkipOutput)
 
-	if outputSkipTags == "" && cfg.SkipOutput == nil {
-		logSettings = log.NewSettings()
-		logSettings.ApplySettings(outputLogTags, cfg.Output)
-	} else {
-		// Deprecate skip_output in the future. Leaving as is to reduce noise in output.
-		// log.Warn("skip_output is deprecated, please use output option")
-
-		logSettings = log.NewSkipSettings() //nolint:staticcheck //SA1019: for temporary backward compatibility
-		logSettings.ApplySettings(outputSkipTags, cfg.SkipOutput)
-	}
+	// Deprecate skip_output in the future. Leaving as is to reduce noise in output.
+	// if outputSkipTags != "" || cfg.SkipOutput != nil {
+	// 	 log.Warn("skip_output is deprecated, please use output option")
+	// }
 
 	if logSettings.LogMeta() {
 		log.Box(

diff --git a/internal/lefthook/run/exec/execute_unix.go b/internal/lefthook/run/exec/execute_unix.go
@@ -44,7 +44,7 @@ func (e CommandExecutor) Execute(ctx context.Context, opts Options, out io.Write
 	}
 
 	root, _ := filepath.Abs(opts.Root)
-	envs := make([]string, len(opts.Env))
+	envs := make([]string, 0, len(opts.Env))
 	for name, value := range opts.Env {
 		envs = append(
 			envs,

diff --git a/internal/lefthook/run/runner.go b/internal/lefthook/run/runner.go
@@ -83,7 +83,7 @@ func (r *Runner) RunAll(ctx context.Context, sourceDirs []string) {
 		defer log.StopSpinner()
 	}
 
-	scriptDirs := make([]string, len(sourceDirs))
+	scriptDirs := make([]string, 0, len(sourceDirs))
 	for _, sourceDir := range sourceDirs {
 		scriptDirs = append(scriptDirs, filepath.Join(
 			sourceDir, r.HookName,