From 757eb2f2676e963df05507721ddda30e4d0ce3ea Mon Sep 17 00:00:00 2001 From: Oliver Eikemeier Date: Fri, 6 Mar 2026 00:48:06 +0100 Subject: [PATCH] chore: handle error types consistently (#9873) Some error types are used inconsistently or wrong: - `forgejo.org/modules/git.ErrNotExist` is meant to be a value error: <[modules/git/error.go#L23](https://codeberg.org/forgejo/forgejo/src/tag/v13.0.2/modules/git/error.go#L23)> - `forgejo.org/models/repo.ErrRepoNotExist` is meant to be a value error: <[models/repo/repo.go#L750](https://codeberg.org/forgejo/forgejo/src/tag/v13.0.2/models/repo/repo.go#L750)> - `errors.Is(logErr, &net.OpError{})` is always `false`: <[services/context/context_response.go#L188](https://codeberg.org/forgejo/forgejo/src/tag/v13.0.2/services/context/context_response.go#L188)> - `forgejo.org/models/issues.ErrIssueContentHistoryNotExist` is used inconsistently: <[models/issues/content_history.go#L211](https://codeberg.org/forgejo/forgejo/src/tag/v13.0.2/models/issues/content_history.go#L211)> Decided to use a value, since the structure is small and to be in line with the above errors. These issued where found with the [errortype](https://codeberg.org/fillmore-labs/errortype) linter and add this to Makefile as part of the linter suite. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9873 Reviewed-by: Gusted Co-authored-by: Oliver Eikemeier Co-committed-by: Oliver Eikemeier --- Makefile | 3 +++ models/issues/content_history.go | 2 +- modules/git/batch_reader.go | 2 +- modules/log/color_console_other.go | 2 +- services/context/context_response.go | 3 ++- services/pull/temp_repo.go | 4 ++-- 6 files changed, 10 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index ce271333c4..ca0aa27206 100644 --- a/Makefile +++ b/Makefile @@ -46,6 +46,7 @@ XGO_PACKAGE ?= src.techknowlogick.com/xgo@latest GO_LICENSES_PACKAGE ?= github.com/google/go-licenses@v1.6.0 # renovate: datasource=go GOVULNCHECK_PACKAGE ?= golang.org/x/vuln/cmd/govulncheck@v1 # renovate: datasource=go DEADCODE_PACKAGE ?= golang.org/x/tools/cmd/deadcode@v0.42.0 # renovate: datasource=go +ERRORTYPE_PACKAGE ?= fillmore-labs.com/errortype@v0.0.10 # renovate: datasource=go GOMOCK_PACKAGE ?= go.uber.org/mock/mockgen@v0.6.0 # renovate: datasource=go RENOVATE_NPM_PACKAGE ?= renovate@43.31.1 # renovate: datasource=docker packageName=data.forgejo.org/renovate/renovate @@ -486,6 +487,7 @@ RUN_DEADCODE = $(GO) run $(DEADCODE_PACKAGE) -generated=false -f='{{println .Pat .PHONY: lint-go lint-go: $(GO) run $(GOLANGCI_LINT_PACKAGE) run $(GOLANGCI_LINT_ARGS) + $(GO) run $(ERRORTYPE_PACKAGE) ./... $(RUN_DEADCODE) > .cur-deadcode-out @$(DIFF) .deadcode-out .cur-deadcode-out \ || (code=$$?; echo "Please run 'make lint-go-fix' and commit the result"; exit $${code}) @@ -955,6 +957,7 @@ deps-tools: $(GO) install $(GO_LICENSES_PACKAGE) $(GO) install $(GOVULNCHECK_PACKAGE) $(GO) install $(GOMOCK_PACKAGE) + $(GO) install $(ERRORTYPE_PACKAGE) node_modules: package-lock.json npm install --no-save diff --git a/models/issues/content_history.go b/models/issues/content_history.go index 476c6e0f90..0849686607 100644 --- a/models/issues/content_history.go +++ b/models/issues/content_history.go @@ -222,7 +222,7 @@ func GetIssueContentHistoryAndPrev(dbCtx context.Context, issueID, id int64) (hi return nil, nil, err } else if !has { log.Error("issue content history does not exist. id=%v. err=%v", id, err) - return nil, nil, &ErrIssueContentHistoryNotExist{id} + return nil, nil, ErrIssueContentHistoryNotExist{id} } prevHistory = &ContentHistory{} diff --git a/modules/git/batch_reader.go b/modules/git/batch_reader.go index 1297c7247f..a8cd626d81 100644 --- a/modules/git/batch_reader.go +++ b/modules/git/batch_reader.go @@ -273,7 +273,7 @@ func ParseTreeLine(objectFormat ObjectFormat, rd *bufio.Reader, modeBuf, fnameBu idx := bytes.IndexByte(readBytes, ' ') if idx < 0 { log.Debug("missing space in readBytes ParseTreeLine: %s", readBytes) - return mode, fname, sha, n, &ErrNotExist{} + return mode, fname, sha, n, ErrNotExist{} } n += idx + 1 diff --git a/modules/log/color_console_other.go b/modules/log/color_console_other.go index 6573d093a5..7505deef61 100644 --- a/modules/log/color_console_other.go +++ b/modules/log/color_console_other.go @@ -39,7 +39,7 @@ func fileStatDevIno(file *os.File) (uint64, uint64, bool) { // Do a type conversion to uint64, because Dev isn't always uint64 // on every operating system + architecture combination. - return uint64(stat.Dev), stat.Ino, true //nolint:unconvert + return uint64(stat.Dev), stat.Ino, true //nolint:unconvert,nolintlint } func fileIsDevIno(file *os.File, dev, ino uint64) bool { diff --git a/services/context/context_response.go b/services/context/context_response.go index f1470e1ad0..e111bafe4a 100644 --- a/services/context/context_response.go +++ b/services/context/context_response.go @@ -185,7 +185,8 @@ func (ctx *Context) ServerError(logMsg string, logErr error) { func (ctx *Context) serverErrorInternal(logMsg string, logErr error) { if logErr != nil { log.ErrorWithSkip(2, "%s: %v", logMsg, logErr) - if _, ok := logErr.(*net.OpError); ok || errors.Is(logErr, &net.OpError{}) { + var opError *net.OpError + if errors.As(logErr, &opError) { // This is an error within the underlying connection // and further rendering will not work so just return return diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go index 76ae0df018..801f4d5ede 100644 --- a/services/pull/temp_repo.go +++ b/services/pull/temp_repo.go @@ -53,7 +53,7 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) return nil, nil, fmt.Errorf("%v LoadHeadRepo: %w", pr, err) } else if pr.HeadRepo == nil { log.Error("%-v HeadRepo %d does not exist", pr, pr.HeadRepoID) - return nil, nil, &repo_model.ErrRepoNotExist{ + return nil, nil, repo_model.ErrRepoNotExist{ ID: pr.HeadRepoID, } } else if err := pr.LoadBaseRepo(ctx); err != nil { @@ -61,7 +61,7 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) return nil, nil, fmt.Errorf("%v LoadBaseRepo: %w", pr, err) } else if pr.BaseRepo == nil { log.Error("%-v BaseRepo %d does not exist", pr, pr.BaseRepoID) - return nil, nil, &repo_model.ErrRepoNotExist{ + return nil, nil, repo_model.ErrRepoNotExist{ ID: pr.BaseRepoID, } } else if err := pr.HeadRepo.LoadOwner(ctx); err != nil {