Add golangci (#6418)

.drone.yml

@@ -74,12 +74,10 @@ pipeline:
     commands:
       - make clean
       - make generate
-      - make vet
-      - make lint
-      - make fmt-check
+      - make golangci-lint
+      - make revive
       - make swagger-check
       - make swagger-validate
-      - make misspell-check
       - make test-vendor
       - make build
     when:

.golangci.yml

@@ -0,0 +1,97 @@
+linters:
+  enable:
+    - gosimple
+    - deadcode
+    - typecheck
+    - govet
+    - errcheck
+    - staticcheck
+    - unused
+    - structcheck
+    - varcheck
+    - golint
+    - dupl
+    #- gocyclo # The cyclomatic complexety of a lot of functions is too high, we should refactor those another time.
+    - gofmt
+    - misspell
+    - gocritic
+  enable-all: false
+  disable-all: true
+  fast: false
+
+linters-settings:
+  gocritic:
+    disabled-checks:
+      - ifElseChain
+      - singleCaseSwitch # Every time this occured in the code, there  was no other way.
+
+issues:
+  exclude-rules:
+    # Exclude some linters from running on tests files.
+    - path: _test\.go
+      linters:
+        - gocyclo
+        - errcheck
+        - dupl
+        - gosec
+        - unparam
+        - staticcheck
+    - path: models/migrations/v
+      linters:
+        - gocyclo
+        - errcheck
+        - dupl
+        - gosec
+    - linters:
+        - dupl
+      text: "webhook"
+    - linters:
+        - gocritic
+      text: "`ID' should not be capitalized"
+    - path: modules/templates/helper.go
+      linters:
+        - gocritic
+    - linters:
+        - unused
+        - deadcode
+      text: "swagger"
+    - path: contrib/pr/checkout.go
+      linters:
+        - errcheck
+    - path: models/issue.go
+      linters:
+        - errcheck
+    - path: models/migrations/
+      linters:
+        - errcheck
+    - path: modules/log/
+      linters:
+        - errcheck
+    - path: routers/routes/routes.go
+      linters:
+        - dupl
+    - path: routers/repo/view.go
+      linters:
+        - dupl
+    - path: models/migrations/
+      linters:
+        - unused
+    - linters:
+        - staticcheck
+      text: "argument x is overwritten before first use"
+    - path: modules/httplib/httplib.go
+      linters:
+        - staticcheck
+    # Enabling this would require refactoring the methods and how they are called.
+    - path: models/issue_comment_list.go
+      linters:
+        - dupl
+    # "Destroy" is misspelled in github.com/go-macaron/session/session.go:213 so it's not our responsability to fix it
+    - path: modules/session/virtual.go
+      linters:
+        - misspell
+      text: '`Destory` is a misspelling of `Destroy`'
+    - path: modules/session/memory.go
+      linters:
+        - misspell
+      text: '`Destory` is a misspelling of `Destroy`'

Makefile

@@ -135,6 +135,10 @@ errcheck:
 
 .PHONY: lint
 lint:
+	@echo 'make lint is depricated. Use "make revive" if you want to use the old lint tool, or "make golangci-lint" to run a complete code check.'
+
+.PHONY: revive
+revive:
 	@hash revive > /dev/null 2>&1; if [ $$? -ne 0 ]; then \
 		$(GO) get -u github.com/mgechev/revive; \
 	fi
@@ -461,3 +465,10 @@ generate-images:
 .PHONY: pr
 pr:
 	$(GO) run contrib/pr/checkout.go $(PR)
+
+.PHONY: golangci-lint
+golangci-lint:
+	@hash golangci-lint > /dev/null 2>&1; if [ $$? -ne 0 ]; then \
+		curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $(GOPATH)/bin v1.16.0; \
+	fi
+	golangci-lint run

cmd/admin.go

@@ -481,7 +481,7 @@ func runUpdateOauth(c *cli.Context) error {
 	}
 
 	// update custom URL mapping
-	var customURLMapping *oauth2.CustomURLMapping
+	var customURLMapping = &oauth2.CustomURLMapping{}
 
 	if oAuth2Config.CustomURLMapping != nil {
 		customURLMapping.TokenURL = oAuth2Config.CustomURLMapping.TokenURL

cmd/cert.go

@@ -170,17 +170,28 @@ func runCert(c *cli.Context) error {
 	if err != nil {
 		log.Fatalf("Failed to open cert.pem for writing: %v", err)
 	}
-	pem.Encode(certOut, &pem.Block{Type: "CERTIFICATE", Bytes: derBytes})
-	certOut.Close()
+	err = pem.Encode(certOut, &pem.Block{Type: "CERTIFICATE", Bytes: derBytes})
+	if err != nil {
+		log.Fatalf("Failed to encode certificate: %v", err)
+	}
+	err = certOut.Close()
+	if err != nil {
+		log.Fatalf("Failed to write cert: %v", err)
+	}
 	log.Println("Written cert.pem")
 
 	keyOut, err := os.OpenFile("key.pem", os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600)
 	if err != nil {
 		log.Fatalf("Failed to open key.pem for writing: %v", err)
 	}
-	pem.Encode(keyOut, pemBlockForKey(priv))
-	keyOut.Close()
+	err = pem.Encode(keyOut, pemBlockForKey(priv))
+	if err != nil {
+		log.Fatalf("Failed to encode key: %v", err)
+	}
+	err = keyOut.Close()
+	if err != nil {
+		log.Fatalf("Failed to write key: %v", err)
+	}
 	log.Println("Written key.pem")
-
 	return nil
 }

cmd/serv.go

@@ -30,7 +30,6 @@ import (
 )
 
 const (
-	accessDenied        = "Repository does not exist or you do not have access"
 	lfsAuthenticateVerb = "git-lfs-authenticate"
 )
 
@@ -67,7 +66,7 @@ func checkLFSVersion() {
 }
 
 func setup(logPath string) {
-	log.DelLogger("console")
+	_ = log.DelLogger("console")
 	setting.NewContext()
 	checkLFSVersion()
 }
@@ -112,7 +111,9 @@ func runServ(c *cli.Context) error {
 	}
 
 	if len(c.Args()) < 1 {
-		cli.ShowSubcommandHelp(c)
+		if err := cli.ShowSubcommandHelp(c); err != nil {
+			fmt.Printf("error showing subcommand help: %v\n", err)
+		}
 		return nil
 	}
 

cmd/web.go

@@ -178,11 +178,16 @@ func runWeb(ctx *cli.Context) error {
 		}
 		err = runHTTPS(listenAddr, setting.CertFile, setting.KeyFile, context2.ClearHandler(m))
 	case setting.FCGI:
-		listener, err := net.Listen("tcp", listenAddr)
+		var listener net.Listener
+		listener, err = net.Listen("tcp", listenAddr)
 		if err != nil {
 			log.Fatal("Failed to bind %s: %v", listenAddr, err)
 		}
-		defer listener.Close()
+		defer func() {
+			if err := listener.Close(); err != nil {
+				log.Fatal("Failed to stop server: %v", err)
+			}
+		}()
 		err = fcgi.Serve(listener, context2.ClearHandler(m))
 	case setting.UnixSocket:
 		if err := os.Remove(listenAddr); err != nil && !os.IsNotExist(err) {

contrib/pr/checkout.go

@@ -91,8 +91,7 @@ func runPR() {
 	routers.NewServices()
 	//x, err = xorm.NewEngine("sqlite3", "file::memory:?cache=shared")
 
-	var helper testfixtures.Helper
-	helper = &testfixtures.SQLite{}
+	var helper testfixtures.Helper = &testfixtures.SQLite{}
 	models.NewEngine(func(_ *xorm.Engine) error {
 		return nil
 	})

integrations/branches_test.go

@@ -62,7 +62,7 @@ func branchAction(t *testing.T, button string) (*HTMLDoc, string) {
 	req = NewRequestWithValues(t, "POST", link, map[string]string{
 		"_csrf": getCsrf(t, htmlDoc.doc),
 	})
-	resp = session.MakeRequest(t, req, http.StatusOK)
+	session.MakeRequest(t, req, http.StatusOK)
 
 	url, err := url.Parse(link)
 	assert.NoError(t, err)

integrations/editor_test.go

@@ -34,7 +34,7 @@ func TestCreateFile(t *testing.T) {
 			"content":       "Content",
 			"commit_choice": "direct",
 		})
-		resp = session.MakeRequest(t, req, http.StatusFound)
+		session.MakeRequest(t, req, http.StatusFound)
 	})
 }
 
@@ -48,7 +48,7 @@ func TestCreateFileOnProtectedBranch(t *testing.T) {
 			"_csrf":     csrf,
 			"protected": "on",
 		})
-		resp := session.MakeRequest(t, req, http.StatusFound)
+		session.MakeRequest(t, req, http.StatusFound)
 		// Check if master branch has been locked successfully
 		flashCookie := session.GetCookie("macaron_flash")
 		assert.NotNil(t, flashCookie)
@@ -56,7 +56,7 @@ func TestCreateFileOnProtectedBranch(t *testing.T) {
 
 		// Request editor page
 		req = NewRequest(t, "GET", "/user2/repo1/_new/master/")
-		resp = session.MakeRequest(t, req, http.StatusOK)
+		resp := session.MakeRequest(t, req, http.StatusOK)
 
 		doc := NewHTMLParser(t, resp.Body)
 		lastCommit := doc.GetInputValueByName("last_commit")

integrations/integration_test.go

@@ -42,7 +42,7 @@ type NilResponseRecorder struct {
 }
 
 func (n *NilResponseRecorder) Write(b []byte) (int, error) {
-	n.Length = n.Length + len(b)
+	n.Length += len(b)
 	return len(b), nil
 }
 
@@ -141,8 +141,7 @@ func initIntegrationTest() {
 		if err != nil {
 			log.Fatalf("sql.Open: %v", err)
 		}
-		rows, err := db.Query(fmt.Sprintf("SELECT 1 FROM pg_database WHERE datname = '%s'",
-			models.DbCfg.Name))
+		rows, err := db.Query(fmt.Sprintf("SELECT 1 FROM pg_database WHERE datname = '%s'", models.DbCfg.Name))
 		if err != nil {
 			log.Fatalf("db.Query: %v", err)
 		}
@@ -210,7 +209,7 @@ func (s *TestSession) MakeRequest(t testing.TB, req *http.Request, expectedStatu
 	resp := MakeRequest(t, req, expectedStatus)
 
 	ch := http.Header{}
-	ch.Add("Cookie", strings.Join(resp.HeaderMap["Set-Cookie"], ";"))
+	ch.Add("Cookie", strings.Join(resp.Header()["Set-Cookie"], ";"))
 	cr := http.Request{Header: ch}
 	s.jar.SetCookies(baseURL, cr.Cookies())
 
@@ -226,7 +225,7 @@ func (s *TestSession) MakeRequestNilResponseRecorder(t testing.TB, req *http.Req
 	resp := MakeRequestNilResponseRecorder(t, req, expectedStatus)
 
 	ch := http.Header{}
-	ch.Add("Cookie", strings.Join(resp.HeaderMap["Set-Cookie"], ";"))
+	ch.Add("Cookie", strings.Join(resp.Header()["Set-Cookie"], ";"))
 	cr := http.Request{Header: ch}
 	s.jar.SetCookies(baseURL, cr.Cookies())
 
@@ -266,7 +265,7 @@ func loginUserWithPassword(t testing.TB, userName, password string) *TestSession
 	resp = MakeRequest(t, req, http.StatusFound)
 
 	ch := http.Header{}
-	ch.Add("Cookie", strings.Join(resp.HeaderMap["Set-Cookie"], ";"))
+	ch.Add("Cookie", strings.Join(resp.Header()["Set-Cookie"], ";"))
 	cr := http.Request{Header: ch}
 
 	session := emptyTestSession(t)

integrations/lfs_getobject_test.go

@@ -45,7 +45,7 @@ func storeObjectInRepo(t *testing.T, repositoryID int64, content *[]byte) string
 		lfsMetaObject = &models.LFSMetaObject{Oid: oid, Size: int64(len(*content)), RepositoryID: repositoryID}
 	}
 
-	lfsID = lfsID + 1
+	lfsID++
 	lfsMetaObject, err = models.NewLFSMetaObject(lfsMetaObject)
 	assert.NoError(t, err)
 	contentStore := &lfs.ContentStore{BasePath: setting.LFS.ContentPath}

integrations/migration-test/migration_test.go

@@ -57,21 +57,6 @@ func initMigrationTest(t *testing.T) {
 	setting.NewLogServices(true)
 }
 
-func getDialect() string {
-	dialect := "sqlite"
-	switch {
-	case setting.UseSQLite3:
-		dialect = "sqlite"
-	case setting.UseMySQL:
-		dialect = "mysql"
-	case setting.UsePostgreSQL:
-		dialect = "pgsql"
-	case setting.UseMSSQL:
-		dialect = "mssql"
-	}
-	return dialect
-}
-
 func availableVersions() ([]string, error) {
 	migrationsDir, err := os.Open("integrations/migration-test")
 	if err != nil {

integrations/testlogger.go

@@ -73,7 +73,7 @@ func PrintCurrentTest(t testing.TB, skip ...int) {
 	_, filename, line, _ := runtime.Caller(actualSkip)
 
 	if log.CanColorStdout {
-		fmt.Fprintf(os.Stdout, "=== %s (%s:%d)\n", log.NewColoredValue(t.Name()), strings.TrimPrefix(filename, prefix), line)
+		fmt.Fprintf(os.Stdout, "=== %s (%s:%d)\n", fmt.Formatter(log.NewColoredValue(t.Name())), strings.TrimPrefix(filename, prefix), line)
 	} else {
 		fmt.Fprintf(os.Stdout, "=== %s (%s:%d)\n", t.Name(), strings.TrimPrefix(filename, prefix), line)
 	}

main.go

@@ -42,7 +42,7 @@ var (
 
 func init() {
 	setting.AppVer = Version
-	setting.AppBuiltWith = formatBuiltWith(Tags)
+	setting.AppBuiltWith = formatBuiltWith()
 
 	// Grab the original help templates
 	originalAppHelpTemplate = cli.AppHelpTemplate
@@ -56,7 +56,7 @@ func main() {
 	app.Usage = "A painless self-hosted Git service"
 	app.Description = `By default, gitea will start serving using the webserver with no
 arguments - which can alternatively be run by running the subcommand web.`
-	app.Version = Version + formatBuiltWith(Tags)
+	app.Version = Version + formatBuiltWith()
 	app.Commands = []cli.Command{
 		cmd.CmdWeb,
 		cmd.CmdServ,
@@ -179,7 +179,7 @@ DEFAULT CONFIGURATION:
 `, originalTemplate, setting.CustomPath, overrided, setting.CustomConf, setting.AppPath, setting.AppWorkPath)
 }
 
-func formatBuiltWith(makeTags string) string {
+func formatBuiltWith() string {
 	var version = runtime.Version()
 	if len(MakeVersion) > 0 {
 		version = MakeVersion + ", " + runtime.Version()

models/access_test.go

@@ -10,13 +10,6 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-var accessModes = []AccessMode{
-	AccessModeRead,
-	AccessModeWrite,
-	AccessModeAdmin,
-	AccessModeOwner,
-}
-
 func TestAccessLevel(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 

models/branches.go

@@ -126,14 +126,14 @@ func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(pr *PullRequest)
 }
 
 // GetProtectedBranchByRepoID getting protected branch by repo ID
-func GetProtectedBranchByRepoID(RepoID int64) ([]*ProtectedBranch, error) {
+func GetProtectedBranchByRepoID(repoID int64) ([]*ProtectedBranch, error) {
 	protectedBranches := make([]*ProtectedBranch, 0)
-	return protectedBranches, x.Where("repo_id = ?", RepoID).Desc("updated_unix").Find(&protectedBranches)
+	return protectedBranches, x.Where("repo_id = ?", repoID).Desc("updated_unix").Find(&protectedBranches)
 }
 
 // GetProtectedBranchBy getting protected branch by ID/Name
-func GetProtectedBranchBy(repoID int64, BranchName string) (*ProtectedBranch, error) {
-	rel := &ProtectedBranch{RepoID: repoID, BranchName: BranchName}
+func GetProtectedBranchBy(repoID int64, branchName string) (*ProtectedBranch, error) {
+	rel := &ProtectedBranch{RepoID: repoID, BranchName: branchName}
 	has, err := x.Get(rel)
 	if err != nil {
 		return nil, err

models/git_blame.go

@@ -40,7 +40,7 @@ func (r *BlameReader) NextPart() (*BlamePart, error) {
 	scanner := r.scanner
 
 	if r.lastSha != nil {
-		blamePart = &BlamePart{*r.lastSha, make([]string, 0, 0)}
+		blamePart = &BlamePart{*r.lastSha, make([]string, 0)}
 	}
 
 	for scanner.Scan() {
@@ -56,7 +56,7 @@ func (r *BlameReader) NextPart() (*BlamePart, error) {
 			sha1 := lines[1]
 
 			if blamePart == nil {
-				blamePart = &BlamePart{sha1, make([]string, 0, 0)}
+				blamePart = &BlamePart{sha1, make([]string, 0)}
 			}
 
 			if blamePart.Sha != sha1 {

models/git_diff.go

@@ -384,13 +384,9 @@ func CutDiffAroundLine(originalDiff io.Reader, line int64, old bool, numbersOfLi
 	// headers + hunk header
 	newHunk := make([]string, headerLines)
 	// transfer existing headers
-	for idx, lof := range hunk[:headerLines] {
-		newHunk[idx] = lof
-	}
+	copy(newHunk, hunk[:headerLines])
 	// transfer last n lines
-	for _, lof := range hunk[len(hunk)-numbersOfLine-1:] {
-		newHunk = append(newHunk, lof)
-	}
+	newHunk = append(newHunk, hunk[len(hunk)-numbersOfLine-1:]...)
 	// calculate newBegin, ... by counting lines
 	for i := len(hunk) - 1; i >= len(hunk)-numbersOfLine; i-- {
 		switch hunk[i][0] {
@@ -582,7 +578,10 @@ func ParsePatch(maxLines, maxLineCharacters, maxFiles int, reader io.Reader) (*D
 			diff.Files = append(diff.Files, curFile)
 			if len(diff.Files) >= maxFiles {
 				diff.IsIncomplete = true
-				io.Copy(ioutil.Discard, reader)
+				_, err := io.Copy(ioutil.Discard, reader)
+				if err != nil {
+					return nil, fmt.Errorf("Copy: %v", err)
+				}
 				break
 			}
 			curFileLinesCount = 0

models/git_diff_test.go

@@ -17,12 +17,6 @@ func assertEqual(t *testing.T, s1 string, s2 template.HTML) {
 	}
 }
 
-func assertLineEqual(t *testing.T, d1 *DiffLine, d2 *DiffLine) {
-	if d1 != d2 {
-		t.Errorf("%v should be equal %v", d1, d2)
-	}
-}
-
 func TestDiffToHTML(t *testing.T) {
 	assertEqual(t, "+foo <span class=\"added-code\">bar</span> biz", diffToHTML([]dmp.Diff{
 		{Type: dmp.DiffEqual, Text: "foo "},

models/issue.go

@@ -1330,7 +1330,7 @@ func sortIssuesSession(sess *xorm.Session, sortType string) {
 	}
 }
 
-func (opts *IssuesOptions) setupSession(sess *xorm.Session) error {
+func (opts *IssuesOptions) setupSession(sess *xorm.Session) {
 	if opts.Page >= 0 && opts.PageSize > 0 {
 		var start int
 		if opts.Page == 0 {
@@ -1389,7 +1389,6 @@ func (opts *IssuesOptions) setupSession(sess *xorm.Session) error {
 				fmt.Sprintf("issue.id = il%[1]d.issue_id AND il%[1]d.label_id = %[2]d", i, labelID))
 		}
 	}
-	return nil
 }
 
 // CountIssuesByRepo map from repoID to number of issues matching the options
@@ -1397,9 +1396,7 @@ func CountIssuesByRepo(opts *IssuesOptions) (map[int64]int64, error) {
 	sess := x.NewSession()
 	defer sess.Close()
 
-	if err := opts.setupSession(sess); err != nil {
-		return nil, err
-	}
+	opts.setupSession(sess)
 
 	countsSlice := make([]*struct {
 		RepoID int64
@@ -1424,9 +1421,7 @@ func Issues(opts *IssuesOptions) ([]*Issue, error) {
 	sess := x.NewSession()
 	defer sess.Close()
 
-	if err := opts.setupSession(sess); err != nil {
-		return nil, err
-	}
+	opts.setupSession(sess)
 	sortIssuesSession(sess, opts.SortType)
 
 	issues := make([]*Issue, 0, setting.UI.IssuePagingNum)

models/issue_comment.go

@@ -171,17 +171,6 @@ func (c *Comment) loadPoster(e Engine) (err error) {
 	return err
 }
 
-func (c *Comment) loadAttachments(e Engine) (err error) {
-	if len(c.Attachments) > 0 {
-		return
-	}
-	c.Attachments, err = getAttachmentsByCommentID(e, c.ID)
-	if err != nil {
-		log.Error("getAttachmentsByCommentID[%d]: %v", c.ID, err)
-	}
-	return err
-}
-
 // AfterDelete is invoked from XORM after the object is deleted.
 func (c *Comment) AfterDelete() {
 	if c.ID <= 0 {
@@ -463,7 +452,7 @@ func (c *Comment) LoadReview() error {
 	return c.loadReview(x)
 }
 
-func (c *Comment) checkInvalidation(e Engine, doer *User, repo *git.Repository, branch string) error {
+func (c *Comment) checkInvalidation(doer *User, repo *git.Repository, branch string) error {
 	// FIXME differentiate between previous and proposed line
 	commit, err := repo.LineBlame(branch, repo.Path, c.TreePath, uint(c.UnsignedLine()))
 	if err != nil {
@@ -479,7 +468,7 @@ func (c *Comment) checkInvalidation(e Engine, doer *User, repo *git.Repository,
 // CheckInvalidation checks if the line of code comment got changed by another commit.
 // If the line got changed the comment is going to be invalidated.
 func (c *Comment) CheckInvalidation(repo *git.Repository, doer *User, branch string) error {
-	return c.checkInvalidation(x, doer, repo, branch)
+	return c.checkInvalidation(doer, repo, branch)
 }
 
 // DiffSide returns "previous" if Comment.Line is a LOC of the previous changes and "proposed" if it is a LOC of the proposed changes.
@@ -915,7 +904,7 @@ func CreateCodeComment(doer *User, repo *Repository, issue *Issue, content, tree
 		commit, err := gitRepo.LineBlame(pr.GetGitRefName(), gitRepo.Path, treePath, uint(line))
 		if err == nil {
 			commitID = commit.ID.String()
-		} else if err != nil && !strings.Contains(err.Error(), "exit status 128 - fatal: no such path") {
+		} else if !strings.Contains(err.Error(), "exit status 128 - fatal: no such path") {
 			return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %v", pr.GetGitRefName(), gitRepo.Path, treePath, line, err)
 		}
 	}

models/issue_comment_list.go

@@ -36,7 +36,7 @@ func (comments CommentList) loadPosters(e Engine) error {
 		if err != nil {
 			return err
 		}
-		left = left - limit
+		left -= limit
 		posterIDs = posterIDs[limit:]
 	}
 
@@ -94,13 +94,13 @@ func (comments CommentList) loadLabels(e Engine) error {
 			var label Label
 			err = rows.Scan(&label)
 			if err != nil {
-				rows.Close()
+				_ = rows.Close()
 				return err
 			}
 			commentLabels[label.ID] = &label
 		}
-		rows.Close()
-		left = left - limit
+		_ = rows.Close()
+		left -= limit
 		labelIDs = labelIDs[limit:]
 	}
 
@@ -143,7 +143,7 @@ func (comments CommentList) loadMilestones(e Engine) error {
 		if err != nil {
 			return err
 		}
-		left = left - limit
+		left -= limit
 		milestoneIDs = milestoneIDs[limit:]
 	}
 
@@ -186,7 +186,7 @@ func (comments CommentList) loadOldMilestones(e Engine) error {
 		if err != nil {
 			return err
 		}
-		left = left - limit
+		left -= limit
 		milestoneIDs = milestoneIDs[limit:]
 	}
 
@@ -236,9 +236,9 @@ func (comments CommentList) loadAssignees(e Engine) error {
 
 			assignees[user.ID] = &user
 		}
-		rows.Close()
+		_ = rows.Close()
 
-		left = left - limit
+		left -= limit
 		assigneeIDs = assigneeIDs[limit:]
 	}
 
@@ -310,9 +310,9 @@ func (comments CommentList) loadIssues(e Engine) error {
 
 			issues[issue.ID] = &issue
 		}
-		rows.Close()
+		_ = rows.Close()
 
-		left = left - limit
+		left -= limit
 		issueIDs = issueIDs[limit:]
 	}
 
@@ -361,15 +361,15 @@ func (comments CommentList) loadDependentIssues(e Engine) error {
 			var issue Issue
 			err = rows.Scan(&issue)
 			if err != nil {
-				rows.Close()
+				_ = rows.Close()
 				return err
 			}
 
 			issues[issue.ID] = &issue
 		}
-		rows.Close()
+		_ = rows.Close()
 
-		left = left - limit
+		left -= limit
 		issueIDs = issueIDs[limit:]
 	}
 
@@ -406,14 +406,14 @@ func (comments CommentList) loadAttachments(e Engine) (err error) {
 			var attachment Attachment
 			err = rows.Scan(&attachment)
 			if err != nil {
-				rows.Close()
+				_ = rows.Close()
 				return err
 			}
 			attachments[attachment.CommentID] = append(attachments[attachment.CommentID], &attachment)
 		}
 
-		rows.Close()
-		left = left - limit
+		_ = rows.Close()
+		left -= limit
 		commentsIDs = commentsIDs[limit:]
 	}
 
@@ -457,15 +457,15 @@ func (comments CommentList) loadReviews(e Engine) error {
 			var review Review
 			err = rows.Scan(&review)
 			if err != nil {
-				rows.Close()
+				_ = rows.Close()
 				return err
 			}
 
 			reviews[review.ID] = &review
 		}
-		rows.Close()
+		_ = rows.Close()
 
-		left = left - limit
+		left -= limit
 		reviewIDs = reviewIDs[limit:]
 	}
 

models/issue_label.go

@@ -401,14 +401,6 @@ func NewIssueLabels(issue *Issue, labels []*Label, doer *User) (err error) {
 	return sess.Commit()
 }
 
-func getIssueLabels(e Engine, issueID int64) ([]*IssueLabel, error) {
-	issueLabels := make([]*IssueLabel, 0, 10)
-	return issueLabels, e.
-		Where("issue_id=?", issueID).
-		Asc("label_id").
-		Find(&issueLabels)
-}
-
 func deleteIssueLabel(e *xorm.Session, issue *Issue, label *Label, doer *User) (err error) {
 	if count, err := e.Delete(&IssueLabel{
 		IssueID: issue.ID,

models/issue_list.go

@@ -7,6 +7,8 @@ package models
 import (
 	"fmt"
 
+	"code.gitea.io/gitea/modules/log"
+
 	"github.com/go-xorm/builder"
 )
 
@@ -47,7 +49,7 @@ func (issues IssueList) loadRepositories(e Engine) ([]*Repository, error) {
 		if err != nil {
 			return nil, fmt.Errorf("find repository: %v", err)
 		}
-		left = left - limit
+		left -= limit
 		repoIDs = repoIDs[limit:]
 	}
 
@@ -91,7 +93,7 @@ func (issues IssueList) loadPosters(e Engine) error {
 		if err != nil {
 			return err
 		}
-		left = left - limit
+		left -= limit
 		posterIDs = posterIDs[limit:]
 	}
 
@@ -146,13 +148,21 @@ func (issues IssueList) loadLabels(e Engine) error {
 			var labelIssue LabelIssue
 			err = rows.Scan(&labelIssue)
 			if err != nil {
-				rows.Close()
+				// When there are no rows left and we try to close it, xorm will complain with an error.
+				// Since that is not relevant for us, we can safely ignore it.
+				if err := rows.Close(); err != nil {
+					log.Error("IssueList.loadLabels: Close: %v", err)
+				}
 				return err
 			}
 			issueLabels[labelIssue.IssueLabel.IssueID] = append(issueLabels[labelIssue.IssueLabel.IssueID], labelIssue.Label)
 		}
-		rows.Close()
-		left = left - limit
+		// When there are no rows left and we try to close it, xorm will complain with an error.
+		// Since that is not relevant for us, we can safely ignore it.
+		if err := rows.Close(); err != nil {
+			log.Error("IssueList.loadLabels: Close: %v", err)
+		}
+		left -= limit
 		issueIDs = issueIDs[limit:]
 	}
 
@@ -191,7 +201,7 @@ func (issues IssueList) loadMilestones(e Engine) error {
 		if err != nil {
 			return err
 		}
-		left = left - limit
+		left -= limit
 		milestoneIDs = milestoneIDs[limit:]
 	}
 
@@ -231,15 +241,22 @@ func (issues IssueList) loadAssignees(e Engine) error {
 			var assigneeIssue AssigneeIssue
 			err = rows.Scan(&assigneeIssue)
 			if err != nil {
-				rows.Close()
+				// When there are no rows left and we try to close it, xorm will complain with an error.
+				// Since that is not relevant for us, we can safely ignore it.
+				if err := rows.Close(); err != nil {
+					log.Error("IssueList.loadAssignees: Close: %v", err)
+				}
 				return err
 			}
 
 			assignees[assigneeIssue.IssueAssignee.IssueID] = append(assignees[assigneeIssue.IssueAssignee.IssueID], assigneeIssue.Assignee)
 		}
-		rows.Close()
-
-		left = left - limit
+		// When there are no rows left and we try to close it, xorm will complain with an error.
+		// Since that is not relevant for us, we can safely ignore it.
+		if err := rows.Close(); err != nil {
+			log.Error("IssueList.loadAssignees: Close: %v", err)
+		}
+		left -= limit
 		issueIDs = issueIDs[limit:]
 	}
 
@@ -283,14 +300,21 @@ func (issues IssueList) loadPullRequests(e Engine) error {
 			var pr PullRequest
 			err = rows.Scan(&pr)
 			if err != nil {
-				rows.Close()
+				// When there are no rows left and we try to close it, xorm will complain with an error.
+				// Since that is not relevant for us, we can safely ignore it.
+				if err := rows.Close(); err != nil {
+					log.Error("IssueList.loadPullRequests: Close: %v", err)
+				}
 				return err
 			}
 			pullRequestMaps[pr.IssueID] = &pr
 		}
-
-		rows.Close()
-		left = left - limit
+		// When there are no rows left and we try to close it, xorm will complain with an error.
+		// Since that is not relevant for us, we can safely ignore it.
+		if err := rows.Close(); err != nil {
+			log.Error("IssueList.loadPullRequests: Close: %v", err)
+		}
+		left -= limit
 		issuesIDs = issuesIDs[limit:]
 	}
 
@@ -325,14 +349,21 @@ func (issues IssueList) loadAttachments(e Engine) (err error) {
 			var attachment Attachment
 			err = rows.Scan(&attachment)
 			if err != nil {
-				rows.Close()
+				// When there are no rows left and we try to close it, xorm will complain with an error.
+				// Since that is not relevant for us, we can safely ignore it.
+				if err := rows.Close(); err != nil {
+					log.Error("IssueList.loadAttachments: Close: %v", err)
+				}
 				return err
 			}
 			attachments[attachment.IssueID] = append(attachments[attachment.IssueID], &attachment)
 		}
-
-		rows.Close()
-		left = left - limit
+		// When there are no rows left and we try to close it, xorm will complain with an error.
+		// Since that is not relevant for us, we can safely ignore it.
+		if err := rows.Close(); err != nil {
+			log.Error("IssueList.loadAttachments: Close: %v", err)
+		}
+		left -= limit
 		issuesIDs = issuesIDs[limit:]
 	}
 
@@ -368,13 +399,21 @@ func (issues IssueList) loadComments(e Engine, cond builder.Cond) (err error) {
 			var comment Comment
 			err = rows.Scan(&comment)
 			if err != nil {
-				rows.Close()
+				// When there are no rows left and we try to close it, xorm will complain with an error.
+				// Since that is not relevant for us, we can safely ignore it.
+				if err := rows.Close(); err != nil {
+					log.Error("IssueList.loadComments: Close: %v", err)
+				}
 				return err
 			}
 			comments[comment.IssueID] = append(comments[comment.IssueID], &comment)
 		}
-		rows.Close()
-		left = left - limit
+		// When there are no rows left and we try to close it, xorm will complain with an error.
+		// Since that is not relevant for us, we can safely ignore it.
+		if err := rows.Close(); err != nil {
+			log.Error("IssueList.loadComments: Close: %v", err)
+		}
+		left -= limit
 		issuesIDs = issuesIDs[limit:]
 	}
 
@@ -422,13 +461,21 @@ func (issues IssueList) loadTotalTrackedTimes(e Engine) (err error) {
 			var totalTime totalTimesByIssue
 			err = rows.Scan(&totalTime)
 			if err != nil {
-				rows.Close()
+				// When there are no rows left and we try to close it, xorm will complain with an error.
+				// Since that is not relevant for us, we can safely ignore it.
+				if err := rows.Close(); err != nil {
+					log.Error("IssueList.loadTotalTrackedTimes: Close: %v", err)
+				}
 				return err
 			}
 			trackedTimes[totalTime.IssueID] = totalTime.Time
 		}
-		rows.Close()
-		left = left - limit
+		// When there are no rows left and we try to close it, xorm will complain with an error.
+		// Since that is not relevant for us, we can safely ignore it.
+		if err := rows.Close(); err != nil {
+			log.Error("IssueList.loadTotalTrackedTimes: Close: %v", err)
+		}
+		left -= limit
 		ids = ids[limit:]
 	}
 
@@ -439,33 +486,33 @@ func (issues IssueList) loadTotalTrackedTimes(e Engine) (err error) {
 }
 
 // loadAttributes loads all attributes, expect for attachments and comments
-func (issues IssueList) loadAttributes(e Engine) (err error) {
-	if _, err = issues.loadRepositories(e); err != nil {
-		return
+func (issues IssueList) loadAttributes(e Engine) error {
+	if _, err := issues.loadRepositories(e); err != nil {
+		return fmt.Errorf("issue.loadAttributes: loadRepositories: %v", err)
 	}
 
-	if err = issues.loadPosters(e); err != nil {
-		return
+	if err := issues.loadPosters(e); err != nil {
+		return fmt.Errorf("issue.loadAttributes: loadPosters: %v", err)
 	}
 
-	if err = issues.loadLabels(e); err != nil {
-		return
+	if err := issues.loadLabels(e); err != nil {
+		return fmt.Errorf("issue.loadAttributes: loadLabels: %v", err)
 	}
 
-	if err = issues.loadMilestones(e); err != nil {
-		return
+	if err := issues.loadMilestones(e); err != nil {
+		return fmt.Errorf("issue.loadAttributes: loadMilestones: %v", err)
 	}
 
-	if err = issues.loadAssignees(e); err != nil {
-		return
+	if err := issues.loadAssignees(e); err != nil {
+		return fmt.Errorf("issue.loadAttributes: loadAssignees: %v", err)
 	}
 
-	if err = issues.loadPullRequests(e); err != nil {
-		return
+	if err := issues.loadPullRequests(e); err != nil {
+		return fmt.Errorf("issue.loadAttributes: loadPullRequests: %v", err)
 	}
 
-	if err = issues.loadTotalTrackedTimes(e); err != nil {
-		return
+	if err := issues.loadTotalTrackedTimes(e); err != nil {
+		return fmt.Errorf("issue.loadAttributes: loadTotalTrackedTimes: %v", err)
 	}
 
 	return nil

models/log.go

@@ -15,7 +15,6 @@ import (
 // XORMLogBridge a logger bridge from Logger to xorm
 type XORMLogBridge struct {
 	showSQL bool
-	level   core.LogLevel
 	logger  *log.Logger
 }
 
@@ -34,42 +33,42 @@ func (l *XORMLogBridge) Log(skip int, level log.Level, format string, v ...inter
 
 // Debug show debug log
 func (l *XORMLogBridge) Debug(v ...interface{}) {
-	l.Log(2, log.DEBUG, fmt.Sprint(v...))
+	_ = l.Log(2, log.DEBUG, fmt.Sprint(v...))
 }
 
 // Debugf show debug log
 func (l *XORMLogBridge) Debugf(format string, v ...interface{}) {
-	l.Log(2, log.DEBUG, format, v...)
+	_ = l.Log(2, log.DEBUG, format, v...)
 }
 
 // Error show error log
 func (l *XORMLogBridge) Error(v ...interface{}) {
-	l.Log(2, log.ERROR, fmt.Sprint(v...))
+	_ = l.Log(2, log.ERROR, fmt.Sprint(v...))
 }
 
 // Errorf show error log
 func (l *XORMLogBridge) Errorf(format string, v ...interface{}) {
-	l.Log(2, log.ERROR, format, v...)
+	_ = l.Log(2, log.ERROR, format, v...)
 }
 
 // Info show information level log
 func (l *XORMLogBridge) Info(v ...interface{}) {
-	l.Log(2, log.INFO, fmt.Sprint(v...))
+	_ = l.Log(2, log.INFO, fmt.Sprint(v...))
 }
 
 // Infof show information level log
 func (l *XORMLogBridge) Infof(format string, v ...interface{}) {
-	l.Log(2, log.INFO, format, v...)
+	_ = l.Log(2, log.INFO, format, v...)
 }
 
 // Warn show warning log
 func (l *XORMLogBridge) Warn(v ...interface{}) {
-	l.Log(2, log.WARN, fmt.Sprint(v...))
+	_ = l.Log(2, log.WARN, fmt.Sprint(v...))
 }
 
 // Warnf show warnning log
 func (l *XORMLogBridge) Warnf(format string, v ...interface{}) {
-	l.Log(2, log.WARN, format, v...)
+	_ = l.Log(2, log.WARN, format, v...)
 }
 
 // Level get logger level

models/login_source.go

@@ -164,8 +164,7 @@ func Cell2Int64(val xorm.Cell) int64 {
 
 // BeforeSet is invoked from XORM before setting the value of a field of this object.
 func (source *LoginSource) BeforeSet(colName string, val xorm.Cell) {
-	switch colName {
-	case "type":
+	if colName == "type" {
 		switch LoginType(Cell2Int64(val)) {
 		case LoginLDAP, LoginDLDAP:
 			source.Cfg = new(LDAPConfig)
@@ -282,10 +281,12 @@ func CreateLoginSource(source *LoginSource) error {
 		oAuth2Config := source.OAuth2()
 		err = oauth2.RegisterProvider(source.Name, oAuth2Config.Provider, oAuth2Config.ClientID, oAuth2Config.ClientSecret, oAuth2Config.OpenIDConnectAutoDiscoveryURL, oAuth2Config.CustomURLMapping)
 		err = wrapOpenIDConnectInitializeError(err, source.Name, oAuth2Config)
-
 		if err != nil {
 			// remove the LoginSource in case of errors while registering OAuth2 providers
-			x.Delete(source)
+			if _, err := x.Delete(source); err != nil {
+				log.Error("CreateLoginSource: Error while wrapOpenIDConnectInitializeError: %v", err)
+			}
+			return err
 		}
 	}
 	return err
@@ -325,10 +326,12 @@ func UpdateSource(source *LoginSource) error {
 		oAuth2Config := source.OAuth2()
 		err = oauth2.RegisterProvider(source.Name, oAuth2Config.Provider, oAuth2Config.ClientID, oAuth2Config.ClientSecret, oAuth2Config.OpenIDConnectAutoDiscoveryURL, oAuth2Config.CustomURLMapping)
 		err = wrapOpenIDConnectInitializeError(err, source.Name, oAuth2Config)
-
 		if err != nil {
 			// restore original values since we cannot update the provider it self
-			x.ID(source.ID).AllCols().Update(originalLoginSource)
+			if _, err := x.ID(source.ID).AllCols().Update(originalLoginSource); err != nil {
+				log.Error("UpdateSource: Error while wrapOpenIDConnectInitializeError: %v", err)
+			}
+			return err
 		}
 	}
 	return err
@@ -385,7 +388,7 @@ func composeFullName(firstname, surname, username string) string {
 }
 
 var (
-	alphaDashDotPattern = regexp.MustCompile("[^\\w-\\.]")
+	alphaDashDotPattern = regexp.MustCompile(`[^\w-\.]`)
 )
 
 // LoginViaLDAP queries if login/password is valid against the LDAP directory pool,
@@ -401,7 +404,7 @@ func LoginViaLDAP(user *User, login, password string, source *LoginSource, autoR
 
 	if !autoRegister {
 		if isAttributeSSHPublicKeySet && synchronizeLdapSSHPublicKeys(user, source, sr.SSHPublicKey) {
-			RewriteAllPublicKeys()
+			return user, RewriteAllPublicKeys()
 		}
 
 		return user, nil
@@ -435,7 +438,7 @@ func LoginViaLDAP(user *User, login, password string, source *LoginSource, autoR
 	err := CreateUser(user)
 
 	if err == nil && isAttributeSSHPublicKeySet && addLdapSSHPublicKeys(user, source, sr.SSHPublicKey) {
-		RewriteAllPublicKeys()
+		err = RewriteAllPublicKeys()
 	}
 
 	return user, err

models/mail.go

@@ -157,10 +157,13 @@ func composeTplData(subject, body, link string) map[string]interface{} {
 
 func composeIssueCommentMessage(issue *Issue, doer *User, content string, comment *Comment, tplName base.TplName, tos []string, info string) *mailer.Message {
 	subject := issue.mailSubject()
-	issue.LoadRepo()
+	err := issue.LoadRepo()
+	if err != nil {
+		log.Error("LoadRepo: %v", err)
+	}
 	body := string(markup.RenderByType(markdown.MarkupName, []byte(content), issue.Repo.HTMLURL(), issue.Repo.ComposeMetas()))
 
-	data := make(map[string]interface{}, 10)
+	var data = make(map[string]interface{}, 10)
 	if comment != nil {
 		data = composeTplData(subject, body, issue.HTMLURL()+"#"+comment.HashTag())
 	} else {

models/migrations/migrations.go

@@ -399,7 +399,7 @@ func trimCommitActionAppURLPrefix(x *xorm.Engine) error {
 			return fmt.Errorf("marshal action content[%d]: %v", actID, err)
 		}
 
-		if _, err = sess.Id(actID).Update(&Action{
+		if _, err = sess.ID(actID).Update(&Action{
 			Content: string(p),
 		}); err != nil {
 			return fmt.Errorf("update action[%d]: %v", actID, err)
@@ -503,7 +503,7 @@ func attachmentRefactor(x *xorm.Engine) error {
 
 	// Update database first because this is where error happens the most often.
 	for _, attach := range attachments {
-		if _, err = sess.Id(attach.ID).Update(attach); err != nil {
+		if _, err = sess.ID(attach.ID).Update(attach); err != nil {
 			return err
 		}
 
@@ -581,7 +581,7 @@ func renamePullRequestFields(x *xorm.Engine) (err error) {
 		if pull.Index == 0 {
 			continue
 		}
-		if _, err = sess.Id(pull.ID).Update(pull); err != nil {
+		if _, err = sess.ID(pull.ID).Update(pull); err != nil {
 			return err
 		}
 	}
@@ -661,7 +661,7 @@ func generateOrgRandsAndSalt(x *xorm.Engine) (err error) {
 		if org.Salt, err = generate.GetRandomString(10); err != nil {
 			return err
 		}
-		if _, err = sess.Id(org.ID).Update(org); err != nil {
+		if _, err = sess.ID(org.ID).Update(org); err != nil {
 			return err
 		}
 	}

models/migrations/v27.go

@@ -58,13 +58,13 @@ func convertIntervalToDuration(x *xorm.Engine) (err error) {
 		return fmt.Errorf("Query repositories: %v", err)
 	}
 	for _, mirror := range mirrors {
-		mirror.Interval = mirror.Interval * time.Hour
+		mirror.Interval *= time.Hour
 		if mirror.Interval < setting.Mirror.MinInterval {
 			log.Info("Mirror interval less than Mirror.MinInterval, setting default interval: repo id %v", mirror.RepoID)
 			mirror.Interval = setting.Mirror.DefaultInterval
 		}
 		log.Debug("Mirror interval set to %v for repo id %v", mirror.Interval, mirror.RepoID)
-		_, err := sess.Id(mirror.ID).Cols("interval").Update(mirror)
+		_, err := sess.ID(mirror.ID).Cols("interval").Update(mirror)
 		if err != nil {
 			return fmt.Errorf("update mirror interval failed: %v", err)
 		}

models/migrations/v78.go

@@ -48,6 +48,9 @@ func renameRepoIsBareToIsEmpty(x *xorm.Engine) error {
 
 		if len(indexes) >= 1 {
 			_, err = sess.Exec("DROP INDEX IDX_repository_is_bare ON repository")
+			if err != nil {
+				return fmt.Errorf("Drop index failed: %v", err)
+			}
 		}
 	} else {
 		_, err = sess.Exec("DROP INDEX IDX_repository_is_bare ON repository")