Skip to content

Enable gocritic equalFold and fix issues #34952

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jul 6, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ linters:
require-explanation: true
require-specific: true
gocritic:
enabled-checks:
- equalFold
disabled-checks:
- ifElseChain
- singleCaseSwitch # Every time this occurred in the code, there was no other way.
Expand Down
9 changes: 4 additions & 5 deletions models/repo/language_stats.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,18 +157,17 @@ func UpdateLanguageStats(ctx context.Context, repo *Repository, commitID string,
for lang, size := range stats {
if size > s {
s = size
topLang = strings.ToLower(lang)
topLang = lang
}
}

for lang, size := range stats {
upd := false
llang := strings.ToLower(lang)
for _, s := range oldstats {
// Update already existing language
if strings.ToLower(s.Language) == llang {
if strings.EqualFold(s.Language, lang) {
s.CommitID = commitID
s.IsPrimary = llang == topLang
s.IsPrimary = lang == topLang
s.Size = size
if _, err := sess.ID(s.ID).Cols("`commit_id`", "`size`", "`is_primary`").Update(s); err != nil {
return err
Expand All @@ -182,7 +181,7 @@ func UpdateLanguageStats(ctx context.Context, repo *Repository, commitID string,
if err := db.Insert(ctx, &LanguageStat{
RepoID: repo.ID,
CommitID: commitID,
IsPrimary: llang == topLang,
IsPrimary: lang == topLang,
Language: lang,
Size: size,
}); err != nil {
Expand Down
3 changes: 1 addition & 2 deletions modules/markup/mdstripper/mdstripper.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,7 @@ func (r *stripRenderer) processAutoLink(w io.Writer, link []byte) {
}

// Note: we're not attempting to match the URL scheme (http/https)
host := strings.ToLower(u.Host)
if host != "" && host != strings.ToLower(r.localhost.Host) {
if u.Host != "" && !strings.EqualFold(u.Host, r.localhost.Host) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Host should be ASCII only

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hostnames can contain unicode potentially:

https://en.wikipedia.org/wiki/Internationalized_domain_name

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, backend is ASCII-only, see the Punycode

In the Domain Name System, these domains use an ASCII representation consisting of the prefix "xn--" followed by the Punycode translation of the Unicode representation of the language-specific alphabet or script glyphs. For example, the Cyrillic name of Russia's IDN ccTLD is "рф". In Punycode representation, this is "p1ai", and its DNS name is "xn--p1ai".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you certain that domains will enter in their punycode form into this function? I am not and therefore it's better to use unicode-aware functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you certain that domains will enter in their punycode form into this function?

image

// Process out of band
r.links = append(r.links, linkStr)
return
Expand Down
2 changes: 1 addition & 1 deletion modules/packages/pub/metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func ParsePackage(r io.Reader) (*Package, error) {
if err != nil {
return nil, err
}
} else if strings.ToLower(hd.Name) == "readme.md" {
} else if strings.EqualFold(hd.Name, "readme.md") {
data, err := io.ReadAll(tr)
if err != nil {
return nil, err
Expand Down
4 changes: 2 additions & 2 deletions modules/setting/actions.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ func (c logCompression) IsValid() bool {
}

func (c logCompression) IsNone() bool {
return strings.ToLower(string(c)) == "none"
return string(c) == "none"
}

func (c logCompression) IsZstd() bool {
return c == "" || strings.ToLower(string(c)) == "zstd"
return c == "" || string(c) == "zstd"
}

func loadActionsFrom(rootCfg ConfigProvider) error {
Expand Down
3 changes: 1 addition & 2 deletions modules/util/slice.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ import (
// SliceContainsString sequential searches if string exists in slice.
func SliceContainsString(slice []string, target string, insensitive ...bool) bool {
if len(insensitive) != 0 && insensitive[0] {
target = strings.ToLower(target)
return slices.ContainsFunc(slice, func(t string) bool { return strings.ToLower(t) == target })
return slices.ContainsFunc(slice, func(t string) bool { return strings.EqualFold(t, target) })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC SliceContainsString is only used for ASCII-only cases, for config options or something similar.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds dangerous to just assume that for a utility function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In real world, we have used to assume "case-insensitive" means ASCII-only.

That's why I believe Golang's strings.EqualFold is a wrong design, Unicode case-insensitive doesn't make sense for backend logic.

Copy link
Member Author

@silverwind silverwind Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that strings.ToLower is also unicode-aware, so code using that vs. strings.EqualFold should be strictly equivalent.

Copy link
Member Author

@silverwind silverwind Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I think we are blowing this out of proportion. Nowadays it should be assumed that strings contain unicode and not ASCII and it's good to use unicode-aware case convertions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I think we are blowing this out of proportion. Nowadays it should be assumed that strings contain unicode and not ASCII and it's good to use unicode-aware case convertions.

But not for "backend logic" like "path handling", "name comparing" and "protocol parsing" (we have discussed it in another PR)

We have strictly required "username" to be "ASCII-only", it doesn't make sense to use "Unicode case-insensitive functions" to compare a user's input to the ASCII-only internal username.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I will leave this case unchanged as its a generic utility function.

}

return slices.Contains(slice, target)
Expand Down
2 changes: 1 addition & 1 deletion modules/util/time_str.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func TimeEstimateParse(timeStr string) (int64, error) {
unit := timeStr[match[4]:match[5]]
found := false
for _, u := range timeStrGlobalVars().units {
if strings.ToLower(unit) == u.name {
if strings.EqualFold(unit, u.name) {
total += amount * u.num
found = true
break
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ func repoAssignment() func(ctx *context.APIContext) {
)

// Check if the user is the same as the repository owner.
if ctx.IsSigned && ctx.Doer.LowerName == strings.ToLower(userName) {
if ctx.IsSigned && strings.EqualFold(ctx.Doer.LowerName, userName) {
owner = ctx.Doer
} else {
owner, err = user_model.GetUserByName(ctx, userName)
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/repo/collaborators.go
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ func GetRepoPermissions(ctx *context.APIContext) {
// "$ref": "#/responses/forbidden"

collaboratorUsername := ctx.PathParam("collaborator")
if !ctx.Doer.IsAdmin && ctx.Doer.LowerName != strings.ToLower(collaboratorUsername) && !ctx.IsUserRepoAdmin() {
if !ctx.Doer.IsAdmin && !strings.EqualFold(ctx.Doer.LowerName, collaboratorUsername) && !ctx.IsUserRepoAdmin() {
ctx.APIError(http.StatusForbidden, "Only admins can query all permissions, repo admins can query all repo permissions, collaborators can query only their own")
return
}
Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/repo/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,7 @@ func updateBasicProperties(ctx *context.APIContext, opts api.EditRepoOption) err
newRepoName = *opts.Name
}
// Check if repository name has been changed and not just a case change
if repo.LowerName != strings.ToLower(newRepoName) {
if !strings.EqualFold(repo.LowerName, newRepoName) {
if err := repo_service.ChangeRepositoryName(ctx, ctx.Doer, repo, newRepoName); err != nil {
switch {
case repo_model.IsErrRepoAlreadyExist(err):
Expand Down
2 changes: 1 addition & 1 deletion routers/web/repo/setting/setting.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ func handleSettingsPostUpdate(ctx *context.Context) {

newRepoName := form.RepoName
// Check if repository name has been changed.
if repo.LowerName != strings.ToLower(newRepoName) {
if !strings.EqualFold(repo.LowerName, newRepoName) {
// Close the GitRepo if open
if ctx.Repo.GitRepo != nil {
ctx.Repo.GitRepo.Close()
Expand Down
2 changes: 1 addition & 1 deletion services/auth/source/ldap/source_search.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ func (source *Source) listLdapGroupMemberships(l *ldap.Conn, uid string, applyGr
}

func (source *Source) getUserAttributeListedInGroup(entry *ldap.Entry) string {
if strings.ToLower(source.UserUID) == "dn" {
if strings.EqualFold(source.UserUID, "dn") {
return entry.DN
}

Expand Down
2 changes: 1 addition & 1 deletion services/context/org.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ func OrgAssignment(opts OrgAssignmentOptions) func(ctx *Context) {
if len(teamName) > 0 {
teamExists := false
for _, team := range ctx.Org.Teams {
if team.LowerName == strings.ToLower(teamName) {
if strings.EqualFold(team.LowerName, teamName) {
teamExists = true
ctx.Org.Team = team
ctx.Org.IsTeamMember = true
Expand Down
2 changes: 1 addition & 1 deletion services/context/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ func RepoAssignment(ctx *Context) {
}

// Check if the user is the same as the repository owner
if ctx.IsSigned && ctx.Doer.LowerName == strings.ToLower(userName) {
if ctx.IsSigned && strings.EqualFold(ctx.Doer.LowerName, userName) {
ctx.Repo.Owner = ctx.Doer
} else {
ctx.Repo.Owner, err = user_model.GetUserByName(ctx, userName)
Expand Down
2 changes: 1 addition & 1 deletion services/context/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func UserAssignmentAPI() func(ctx *APIContext) {
func userAssignment(ctx *Base, doer *user_model.User, errCb func(int, any)) (contextUser *user_model.User) {
username := ctx.PathParam("username")

if doer != nil && doer.LowerName == strings.ToLower(username) {
if doer != nil && strings.EqualFold(doer.LowerName, username) {
contextUser = doer
} else {
var err error
Expand Down
2 changes: 1 addition & 1 deletion services/repository/files/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ func CleanGitTreePath(name string) string {
name = util.PathJoinRel(name)
// Git disallows any filenames to have a .git directory in them.
for part := range strings.SplitSeq(name, "/") {
if strings.ToLower(part) == ".git" {
if strings.EqualFold(part, ".git") {
return ""
}
}
Expand Down