Enable more revive linter rules (#30608)

Noteable additions:

- `redefines-builtin-id` forbid variable names that shadow go builtins
- `empty-lines` remove unnecessary empty lines that `gofumpt` does not
remove for some reason
- `superfluous-else` eliminate more superfluous `else` branches

Rules are also sorted alphabetically and I cleaned up various parts of
`.golangci.yml`.

Author: silverwind

.golangci.yml

@@ -1,13 +1,14 @@
 linters:
+  enable-all: false
+  disable-all: true
+  fast: false
   enable:
     - bidichk
-    # - deadcode # deprecated - https://github.com/golangci/golangci-lint/issues/1841
     - depguard
     - dupl
     - errcheck
     - forbidigo
     - gocritic
-    # - gocyclo # The cyclomatic complexety of a lot of functions is too high, we should refactor those another time.
     - gofmt
     - gofumpt
     - gosimple
@@ -17,20 +18,18 @@ linters:
     - nolintlint
     - revive
     - staticcheck
-    # - structcheck # deprecated - https://github.com/golangci/golangci-lint/issues/1841
     - stylecheck
     - typecheck
     - unconvert
     - unused
-    # - varcheck # deprecated - https://github.com/golangci/golangci-lint/issues/1841
     - wastedassign
-  enable-all: false
-  disable-all: true
-  fast: false
 
 run:
   timeout: 10m
 
+output:
+  sort-results: true
+
 linters-settings:
   stylecheck:
     checks: ["all", "-ST1005", "-ST1003"]
@@ -47,27 +46,37 @@ linters-settings:
     errorCode: 1
     warningCode: 1
     rules:
+      - name: atomic
+      - name: bare-return
       - name: blank-imports
+      - name: constant-logical-expr
       - name: context-as-argument
       - name: context-keys-type
       - name: dot-imports
+      - name: duplicated-imports
+      - name: empty-lines
+      - name: error-naming
       - name: error-return
       - name: error-strings
-      - name: error-naming
+      - name: errorf
       - name: exported
+      - name: identical-branches
       - name: if-return
       - name: increment-decrement
-      - name: var-naming
-      - name: var-declaration
+      - name: indent-error-flow
+      - name: modifies-value-receiver
       - name: package-comments
       - name: range
       - name: receiver-naming
+      - name: redefines-builtin-id
+      - name: string-of-int
+      - name: superfluous-else
       - name: time-naming
+      - name: unconditional-recursion
       - name: unexported-return
-      - name: indent-error-flow
-      - name: errorf
-      - name: duplicated-imports
-      - name: modifies-value-receiver
+      - name: unreachable-code
+      - name: var-declaration
+      - name: var-naming
   gofumpt:
     extra-rules: true
   depguard:
@@ -93,8 +102,8 @@ issues:
   max-issues-per-linter: 0
   max-same-issues: 0
   exclude-dirs: [node_modules, public, web_src]
+  exclude-case-sensitive: true
   exclude-rules:
-    # Exclude some linters from running on tests files.
     - path: _test\.go
       linters:
         - gocyclo
@@ -112,19 +121,19 @@ issues:
     - path: cmd
       linters:
         - forbidigo
-    - linters:
+    - text: "webhook"
+      linters:
         - dupl
-      text: "webhook"
-    - linters:
+    - text: "`ID' should not be capitalized"
+      linters:
         - gocritic
-      text: "`ID' should not be capitalized"
-    - linters:
+    - text: "swagger"
+      linters:
         - unused
         - deadcode
-      text: "swagger"
-    - linters:
+    - text: "argument x is overwritten before first use"
+      linters:
         - staticcheck
-      text: "argument x is overwritten before first use"
     - text: "commentFormatting: put a space between `//` and comment text"
       linters:
         - gocritic

cmd/hook.go

@@ -465,7 +465,7 @@ func hookPrintResult(output, isCreate bool, branch, url string) {
 		fmt.Fprintf(os.Stderr, "  %s\n", url)
 	}
 	fmt.Fprintln(os.Stderr, "")
-	os.Stderr.Sync()
+	_ = os.Stderr.Sync()
 }
 
 func pushOptions() map[string]string {

models/asymkey/gpg_key_commit_verification.go

@@ -110,7 +110,6 @@ func ParseCommitWithSignature(ctx context.Context, c *git.Commit) *CommitVerific
 					Reason:         "gpg.error.no_committer_account",
 				}
 			}
-
 		}
 	}
 

models/db/engine.go

@@ -227,7 +227,6 @@ func NamesToBean(names ...string) ([]any, error) {
 	// Need to map provided names to beans...
 	beanMap := make(map[string]any)
 	for _, bean := range tables {
-
 		beanMap[strings.ToLower(reflect.Indirect(reflect.ValueOf(bean)).Type().Name())] = bean
 		beanMap[strings.ToLower(x.TableName(bean))] = bean
 		beanMap[strings.ToLower(x.TableName(bean, true))] = bean

models/issues/review.go

@@ -345,11 +345,9 @@ func CreateReview(ctx context.Context, opts CreateReviewOptions) (*Review, error
 				return nil, err
 			}
 		}
-
 	} else if opts.ReviewerTeam != nil {
 		review.Type = ReviewTypeRequest
 		review.ReviewerTeamID = opts.ReviewerTeam.ID
-
 	} else {
 		return nil, fmt.Errorf("provide either reviewer or reviewer team")
 	}

models/migrations/base/db.go

@@ -177,7 +177,6 @@ func RecreateTable(sess *xorm.Session, bean any) error {
 			log.Error("Unable to recreate uniques on table %s. Error: %v", tableName, err)
 			return err
 		}
-
 	case setting.Database.Type.IsMySQL():
 		// MySQL will drop all the constraints on the old table
 		if _, err := sess.Exec(fmt.Sprintf("DROP TABLE `%s`", tableName)); err != nil {
@@ -228,7 +227,6 @@ func RecreateTable(sess *xorm.Session, bean any) error {
 				return err
 			}
 			sequenceMap[sequence] = sequenceData
-
 		}
 
 		// CASCADE causes postgres to drop all the constraints on the old table
@@ -293,9 +291,7 @@ func RecreateTable(sess *xorm.Session, bean any) error {
 					return err
 				}
 			}
-
 		}
-
 	case setting.Database.Type.IsMSSQL():
 		// MSSQL will drop all the constraints on the old table
 		if _, err := sess.Exec(fmt.Sprintf("DROP TABLE `%s`", tableName)); err != nil {
@@ -308,7 +304,6 @@ func RecreateTable(sess *xorm.Session, bean any) error {
 			log.Error("Unable to rename %s to %s. Error: %v", tempTableName, tableName, err)
 			return err
 		}
-
 	default:
 		log.Fatal("Unrecognized DB")
 	}

models/migrations/v1_11/v111.go

@@ -262,7 +262,6 @@ func AddBranchProtectionCanPushAndEnableWhitelist(x *xorm.Engine) error {
 		for _, u := range units {
 			var found bool
 			for _, team := range teams {
-
 				var teamU []*TeamUnit
 				var unitEnabled bool
 				err = sess.Where("team_id = ?", team.ID).Find(&teamU)
@@ -331,7 +330,6 @@ func AddBranchProtectionCanPushAndEnableWhitelist(x *xorm.Engine) error {
 		}
 
 		if !protectedBranch.EnableApprovalsWhitelist {
-
 			perm, err := getUserRepoPermission(sess, baseRepo, reviewer)
 			if err != nil {
 				return false, err

models/migrations/v1_20/v250.go

@@ -104,7 +104,7 @@ func ChangeContainerMetadataMultiArch(x *xorm.Engine) error {
 
 		// Convert to new metadata format
 
-		new := &MetadataNew{
+		newMetadata := &MetadataNew{
 			Type:             old.Type,
 			IsTagged:         old.IsTagged,
 			Platform:         old.Platform,
@@ -119,7 +119,7 @@ func ChangeContainerMetadataMultiArch(x *xorm.Engine) error {
 			Manifests:        manifests,
 		}
 
-		metadataJSON, err := json.Marshal(new)
+		metadataJSON, err := json.Marshal(newMetadata)
 		if err != nil {
 			return err
 		}

models/migrations/v1_6/v71.go

@@ -61,7 +61,6 @@ func AddScratchHash(x *xorm.Engine) error {
 			if _, err := sess.ID(tfa.ID).Cols("scratch_salt, scratch_hash").Update(tfa); err != nil {
 				return fmt.Errorf("couldn't add in scratch_hash and scratch_salt: %w", err)
 			}
-
 		}
 	}
 

models/migrations/v1_9/v85.go

@@ -81,7 +81,6 @@ func HashAppToken(x *xorm.Engine) error {
 			if _, err := sess.ID(token.ID).Cols("token_hash, token_salt, token_last_eight, sha1").Update(token); err != nil {
 				return fmt.Errorf("couldn't add in sha1, token_hash, token_salt and token_last_eight: %w", err)
 			}
-
 		}
 	}