Add golangci #6418
zeripath
lunny
Labels
No Label
backport/done
backport/v1.0
backport/v1.1
backport/v1.10
backport/v1.11
backport/v1.12
backport/v1.13
backport/v1.14
backport/v1.15
backport/v1.2
backport/v1.3
backport/v1.4
backport/v1.5
backport/v1.6
backport/v1.7
backport/v1.8
backport/v1.9
bounty
changelog
dependencies
frontport/done
frontport/main
good first issue
Hacktoberfest
hacktoberfest-accepted
in progress
kind/api
kind/breaking
kind/bug
kind/build
kind/deployment
kind/deprecated
kind/docs
kind/enhancement
kind/feature
kind/lint
kind/misc
kind/moderation
kind/package
kind/proposal
kind/question
kind/refactor
kind/regression
kind/security
kind/summary
kind/testing
kind/translation
kind/ui
kind/upstream-related
kind/usability
kind/ux
lgtm/done
lgtm/need 1
lgtm/need 2
performance/bigrepo
performance/cpu
performance/memory
performance/speed
priority/critical
priority/low
priority/maybe
priority/medium
proposal/rejected
reviewed/confirmed
reviewed/duplicate
reviewed/fixed
reviewed/invalid
reviewed/not-a-bug
reviewed/wontfix
skip-changelog
stale
status/blocked
status/needs-feedback
status/wip
theme/2fa
theme/authentication
theme/avatar
theme/backup-restore
theme/docker
theme/federation
theme/issues
theme/kanban
theme/markdown
theme/migration
theme/mobile
theme/pr
theme/signing
theme/sqlite
theme/timetracker
theme/webhook
theme/wiki
No Milestone
No project
No Assignees
3 Participants
No due date set.
Dependencies
No dependencies set.
Reference: lunny/gitea#6418
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "golanci"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This pr adds golangci to Gitea and replaces some linters (which golangci contains). This will (hopefully) enhance code quality.
Golangci lint calls a lot of golang lint tools directly, which make it a lot faster.
Looks like the new ci step works, now we just have to fix all the issues.
Fixed most of the issues, however there are some issues where I don't really know what to do:
It looks like
issueIndexerUpdateQueue.Run()doesn't return an error in any of its implementations. Could we just remove the error return?I don't know how it is possible to let
copyonly copy some parts of a slice.I don't really think this is an issue... or is it?
If the errors are not checked, can the whole check be removed? Or how can we check for the error?
@kolaente Is golangci support gitea? Since we will host gitea on gitea.com. If golangci don't support gitea, it will not work.
@lunny It's just a binary file which runs locally and in drone. Since we run it on our own infrastructure, we're not tied to a particular platform.
IIRC they offer tight ingration with github (things like having a bot which comments on the pr instead of just failing the pipeline) which we won't have. But I see no problem with it just failing the pipeline.
Just merged with Gitea's master, it did containt a lot of new issues which had to be fixed...
Do we still need this
make lintstep if golangci-lint covers it? (perhaps aliasmake linttomake golangci-lintjust for backwards compatibility as well.(or instead of aliasing, just add an echo saying use
make golangci-lintinstead)We probably won't need it, it is indeed covered by golangci.
I will add a deprication message.
Ok, I just noticed we're not using the normal go linter, but revive which looks pretty similar to golangci, so I'd say lets keep it. But maybe rename it, to avoid confusion?
renaming sounds good, I think if renaming is done then a note should be added to the previous name in case we have some docs pointing to make lint.
@techknowlogick I've renamed it.
That a lot of missed error checking and more could still be logged in goroutine or defer. This will really improve the codebase and review.
LGTM, I would suggest to disable most of the linter that still failed and manage to enable them one at a time in separates PRs. This will ease the process of review and limit the number of conflict appearing.
Found one template file to be removed
@ -23,4 +23,3 @@tplStars base.TplName = "user/meta/stars")// GetUserByName get user by nameThe related file should be removed
/templates/user/meta/stars.tmpl@ -23,4 +23,3 @@tplStars base.TplName = "user/meta/stars")// GetUserByName get user by nameThat file was actually empty, thanks!
@sapk I think we need to enable these now, I doubt there will be other prs soon otherwise...
@kolaente It is just a recommendation as PR related to deps or CI tends to be broken as other PR get merged and need work to be fixed before merge. And I trust you but doing a lot of change on a lot of files could be suspicious and need a lot of work to review. Breaking it in small step will ease the process.
Some tiny nits. Although the fact that there are so many that are in this PR is it golangci saying we need it this way? If so then that should be reconsidered.
This if statement is not necessary because no matter the outcome err will be returned
This if statement is not necessary because no matter the outcome err will be returned
This if statement is not necessary because no matter the outcome err will be returned
This if statement is not necessary because no matter the outcome err will be returned
This if statement is not necessary because no matter the outcome err will be returned
@ -298,7 +299,8 @@ func NewTeam(t *Team) (err error) {Get(new(Team))if err != nil {This if statement is not necessary because no matter the outcome err will be returned
@ -309,7 +311,10 @@ func NewTeam(t *Team) (err error) {}if _, err = sess.Insert(t); err != nil {This if statement is not necessary because no matter the outcome err will be returned
This if statement is not necessary because no matter the outcome err will be returned
This if statement is not necessary because no matter the outcome err will be returned
This if statement is not necessary because no matter the outcome err will be returned
This if statement is not necessary because no matter the outcome err will be returned
@ -42,2 +46,4 @@log.Error("NewRepoRedirect sess.Rollback: %v", errRollback)}return err}This if statement is not necessary because no matter the outcome err will be returned
I think a new version was just released so this should be updated to match current version.
@techknowlogick These redundant if statements exist just because I put error handling in withough paying attention to the rest around, it's not golanci.
I'll fix these later today.
I think on thoses cases we should return the first error (at end) and log the second.
I've defaulted to the proposal of @sapk for all these issues.
Done.
@lunny fixed.
Could just wrap this in an anonymous function:
@zeripath Sure I could, but since none of the implementations actually returns an error (the returned is alwasy
nil) I'd prefer to alter the signature so the function would never return an error in the first place.No worries. Fair enough.
@zeripath I've come to the conclusion to not change the function signature and wrap the call in an anonymous function instead as this leaves more room for future implementations.
Bleve search tests are failing, not sure why.
@lunny do you have an idea about the failing Bleve tests?
This should remain as %s not %v. Golang-ci is wrong here. ColoredValues have a format.
As disscussed in discord, I'm going to change the signature to return a
fmt.Formatterinstead.@kolaente It seems not only bleve. I found many failures on CI. https://drone.gitea.io/go-gitea/gitea/8110/1/9
@lunny Looks like it's only bleve: https://drone.gitea.io/go-gitea/gitea/8277/1/10
You've changed from crypto/rand to math/rand here. What this deliberate?
No, I've changed it back.
Need to move this down to join the rest of the gitea imports
Fixed.
Haven't completed the review, but found some issues.
This should be a wrong change.
Wrong import order.
The logic seems not the same as before.
As above.
@ -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 {The
errhas been resolved on line 59@ -58,6 +58,9 @@ func hashAppToken(x *xorm.Engine) error {if len(indexes) >= 1 {_, err = sess.Exec("DROP INDEX UQE_access_token_sha1 ON access_token")if err != nil {The err will be handled on line 68
why remove this line?
Here are some more issues to deal with.
I did not point out every discarded errors as in
160e86fbeb/models/issue_comment_list.go (L97)imho I think it's a bad idea to update the code to just don't trigger anymore warnings about the errors not handled without dealing with them.
@ -183,3 +184,3 @@log.Fatal("Failed to bind %s: %v", listenAddr, err)}defer listener.Close()defer func() {I think this is unnecessary.
You should update the line 184 as it is done a few lines below (L197) to not create a new local
errvar.160e86fbeb/cmd/web.go (L184)160e86fbeb/cmd/web.go (L197-L198)@ -161,0 +160,4 @@err := issue.LoadRepo()if err != nil {log.Error("LoadRepo: %v", err)}The created slice is never used.
The err returned by sess.Insert is lost.
If the sess.Rollback returns no errors, the return statement L178 will return nil.
imho I think this block should always return the err from sess.Insert and only log the err from sess.Rollback
The first error is lost.
The first error is lost.
The first error is lost.
@ -231,7 +233,9 @@ func newCommitStatus(sess *xorm.Session, opts NewCommitStatusOptions) error {The first error is lost.
Shouldn't it be (maybe with nil check):
@ -5,6 +5,7 @@package miscimport (Wrong order of import
OK to change the regexp to raw string, but why change the http method names?
@ -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 {No it is not.
A new local
errvar is created on line 44 in the block.@ -58,6 +58,9 @@ func hashAppToken(x *xorm.Engine) error {if len(indexes) >= 1 {_, err = sess.Exec("DROP INDEX UQE_access_token_sha1 ON access_token")if err != nil {Same as above.
A new local
errvar is created on line 54 in the block.Fixed
The only thing I did here was to add error handling for the error returned in line 283 and for the one in 289. The logic remains the same.
Fixed.
Same as my answer before.
@ -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 {@lunny What @ngourdon said.
@ -58,6 +58,9 @@ func hashAppToken(x *xorm.Engine) error {if len(indexes) >= 1 {_, err = sess.Exec("DROP INDEX UQE_access_token_sha1 ON access_token")if err != nil {Same as before.
Good question, I've added it back in.
@ -161,0 +160,4 @@err := issue.LoadRepo()if err != nil {log.Error("LoadRepo: %v", err)}It is, in line 171 the map is passed to
templates.ExecuteTemplate.@ -183,3 +184,3 @@log.Fatal("Failed to bind %s: %v", listenAddr, err)}defer listener.Close()defer func() {Right, good catch.
Sounds like a good idea.
I did not change any method names, which ones do you think I changed?
Changed it so the error is scoped and only available in the if around
sess.Rollback().The bleve test is still failing, @lunny do you have an idea why that is the case?
@ -161,0 +160,4 @@err := issue.LoadRepo()if err != nil {log.Error("LoadRepo: %v", err)}The value setted on line 161 is overwritten on line 163 or 165.
Sorry wasn't clear. You didn't change the names. You changed quotes by backsticks.
By doing this you changed the behaviour.
The delete is no longer in the if block.
Before the LoginSource was deleted if there was an error. Now it is deleted if there is no errors.
Yes I did...?
@ -161,0 +160,4 @@err := issue.LoadRepo()if err != nil {log.Error("LoadRepo: %v", err)}But your suggestion would not pre-allocate a map with 10 items, only make it available.
There's no need.
Raw strings should be used for regexp.MustCompile to avoid having to escape twice. Nothing more.
By the way, the multiples escaping should be deleted. I didn't see it before. I thought there were no escaping.
Should be changed to (only one backslash):
@ -161,0 +160,4 @@err := issue.LoadRepo()if err != nil {log.Error("LoadRepo: %v", err)}This seems unnecessary because it will not be used. I think that's what raised this issue in the first place IMHO.
Ah now I get what you mean, thanks for clarifying 😅 . Fixed it.
Found some tiny issues
It seems
errwill always be nil here.Why is there different handling of error on
Close()?L159, L248, L301, L453: error is ignored
L345, L393: error is returned to caller
As @lunny said before
Before the LoginSource was deleted if there was an error. Now it is deleted if there is no errors.
Same as above
IMHO this error should only be logged and the method should return the initial error returned by the
Insert().Same for L488 and L493
IMHO this error should only be logged and the method should return the initial error.
Same for L237
Sounds like a good idea, thanks.
Thanks, fixed
Mhh good point. I've changed it so the error is always returned.
You're right, I did not see that. I've changed it so that an error from
x.Deleteis at logged.OK so it looks like I've returned any error (while loading issue details) when calling
rows.Close()which always returns an error when there are no more rows left (because all are processed). So in 10/10 cases, this method returns an error if the code before did not errored out. This led to the function erroring out even if it processed all issues correctly. This caused the bleve tests to fail.I wonder why calling
rows.Close()is needed if it always fails because there are no rows left to close...TL;DR: I hopefully fixed the failing bleve indexer issues (would be interesting though to find out why this was only caught by bleve tests).
Ok, so this test is still failing:
Does anyone have an idea why?
(Bleve tests are working now)
@kolaente Could it be that the "Home" wiki page that should already exist for that test doesn't exist, and is created in wiki_test.go 196? Not show these get called and what ensures "Home" exists for that test around line 177.
@richmahn Yeah it looks like that home page just does not exist. I tried changing the order of the tests so that the one in L196 is executed first, but that did not change anything. I also tried to change the name to "Another page" instead of "Home" to see if that would work since "Another page" should have been created successfully before that check. Didn't work either, which lets me think I broke something when creating the fixtures/new wikis...
Does anyone know where the wiki fixtures (the git repos) are created in the tests?
It neither does work when I add "Home" to the list in L153
OK, so I replaced this:
with this
in models/wiki.go:118
When I revert that, the wiki test passes. and I have absolutly no idea why. Anyon have an idea?
Also when I change my modified
deferfunction call to thisit works again. And I still have no idea why. Seems very odd.
Also this:
seems to work.
So it seems like this is some error with scoping...
Anyone has an idea about why this does not work?
@kolaente looks like you need that := instead of = for err :=
@richmahn it looks like it, the unit tests are passing now...
Codecov Report
0% <ø> (ø)47.95% <ø> (+0.27%)89.36% <ø> (ø)0% <ø> (ø)72.08% <ø> (-1.02%)61.54% <ø> (+0.49%)34.18% <ø> (-0.42%)62.12% <ø> (+1.38%)83.72% <ø> (+3.72%)72.54% <ø> (+3.17%)Continue to review full report at Codecov.
Ci passed! ?
@kolaente
As you figured out you have to use
:=in defer func to declare a new local var.If you do not, you use the err which is the return value (see func declaration).
By doing so, in the defer func you overwrite the ErrWikiAlreadyExist (in your case) by a nil error (because RemoveTemporaryPath do not return an error)
seems there is the same problem in models/wiki.go:254
I've looked again at all the cases when a
deferfunction call would log an error and made sure the error would not override any other errors.With all tests passing, this should be ready to review now (finally).
This doesn't look like we're ignoring the empty rows.Close() error....
@ -439,33 +486,33 @@ func (issues IssueList) loadTotalTrackedTimes(e Engine) (err error) {}Probably don't need a named err here any more.
Well, we're not really using it, only logging it. Do you think I should explicitly ignore it?
(Also applies to the various other places where I use this)
@ -439,33 +486,33 @@ func (issues IssueList) loadTotalTrackedTimes(e Engine) (err error) {}Right, fixed it.
I think we should keep the engine here.
I think we should keep the engine here
This should take the engine, i.e. be getRepositoryByID(e, r.RepoID)
And line 53 should similarly use the provided engine
and line 58 should be getReleaseAttachments(e, r) assuming that exists...
But it was never used inside of
loadAttributes?Ah ok. Although that is nothing I changed, but that's why I removed the
Enginesince it wasn't used at all.Fixed.
I think we should explicitly ignore it. Don't log things at error if they're not actually an error. Do you know is the error returned a reasonable type?
getReleaseAttachmentsdoes not exist though.I think it needs to exist - if it doesn't any use of loadAttributes() within a transaction could cause a deadlock
(Sorry it's not the fault of this PR but it's worth fixing)
Since this kind of is a refactoring pr, I thinks it's ok to fix smaller things like this when we stumble upon them, but I'd prefer if these won't get to be the majority of things this pr fixes. So as long as there aren't too many, I'll fix these. Though we should probably do a seperate pr to specifically fix things like this (seperatly).
(I've fixed the thing with
GetReleaseAttachments)thanks @kolaente !
conflicted
@lunny fixed.
@lunny @sapk please approve.
Is this a bug of xorm of your comment?
@ -161,0 +160,4 @@err := issue.LoadRepo()if err != nil {log.Error("LoadRepo: %v", err)}This function should return error as second return parameter I think.
@ -7,1 +7,3 @@import "strings"import ("code.gitea.io/gitea/modules/log""strings"import order
maybe this should be
return err == nil?@ -14,2 +14,4 @@"strings""time"logger "code.gitea.io/gitea/modules/log"Why we need a package alias here?
It seems this change is unnecessary.
import order
@ -100,2 +100,4 @@result, n, err := transform.String(charsetEncoding.NewDecoder(), string(buf))if err != nil {// return defaultMaybe we should add a log here.
just move line 176 here.
@ -345,3 +345,3 @@curLen := n*newLen + len(s[i:])replacement := make([]byte, curLen, curLen)replacement := make([]byte, curLen)Why change this line?
blank line
I think we need keep line 166 and add check for
errs.This line could be removed?
@ -14,2 +14,4 @@"strings""time"logger "code.gitea.io/gitea/modules/log"Because there is a variable
logwhich was declared in thegitpackage.@ -345,3 +345,3 @@curLen := n*newLen + len(s[i:])replacement := make([]byte, curLen, curLen)replacement := make([]byte, curLen)Because the
capof a new slice is optional and if omitted, it is the same as the providedlen. So since it is the same here, there's no need to specify both.Yes, I overlooked that.
You mean, adding a cherck
if len(errs) > 0? And what should happen in that case?But it is used in L124 (and various other places)
@ -100,2 +100,4 @@result, n, err := transform.String(charsetEncoding.NewDecoder(), string(buf))if err != nil {// return defaultThere are a lot other cases where the default is returned if there was an error, should I log it there too?
Right.
@ -7,1 +7,3 @@import "strings"import ("code.gitea.io/gitea/modules/log""strings"Done.
Kind of,
fmt.Fprintf(w, format)would not print anything IIRC since there is no format string.fmt.Fprintf(w, "%s", format)adds this format string. But since all this is doing is returning the error, I adjusted it to justfmt.Fprint(w, format).Done.
Fixed.
Done.
@kolaente all
code.gitea.iopackages should be splitted with external packages with blank line. See https://github.com/go-gitea/gitea/blob/master/CONTRIBUTING.md#styleguideYes, I think form validate should not be ignored. If there are some
errs, we can return firsterroferrsI think or we can join all theerrsas a newerr.@ -14,2 +14,4 @@"strings""time"logger "code.gitea.io/gitea/modules/log"In this file, there is no other package named
log.I don't think
fmt.Fprintf(w, format)will not print anything, see https://play.golang.org/p/sjVCTMbTarAOh, now I understand what you mean, I though you wanted me to import it as
_. Will fix asap.Fixed.
I've joined and returned them now.
@ -161,0 +160,4 @@err := issue.LoadRepo()if err != nil {log.Error("LoadRepo: %v", err)}The problem is it is called in
SendAsyncwhich just puts the mail in a queue to be sent. There's not much room for error handling there.Sorry, missed this. This is different from previous on logic.
@ -161,0 +160,4 @@err := issue.LoadRepo()if err != nil {log.Error("LoadRepo: %v", err)}Or we have to return nil here, otherwise it may panic when invoke
issue.Repo.HTMLURL().@ -161,0 +160,4 @@err := issue.LoadRepo()if err != nil {log.Error("LoadRepo: %v", err)}The problem with this function is the error could never really be handled appropriatly. It would only be bubbled up to then be logged.
I would prefer to leave it as it is, to not having to deal with more complexty bubbling the error up would bring.
So the error from
cli.ShowSubcommandHelp(c)should just be logged and then be ignored?I think yes
Fixed.