Enable gocritic `equalFold` and fix issues by silverwind · Pull Request #34952 · go-gitea/gitea

silverwind · 2025-07-04T14:07:43Z

Continuation of #34678.

wxiaoguang · 2025-07-04T14:35:49Z

Is it possible to forbid strings.EqualFold and force to use our own util.UnicodeEqualFold / AsciiEqualFold to avoid abusing?

I can see some developers will abuse strings.EqualFold more if we have this lint rule, because the strings.EqualFold itself is not well-designed and the name is confusing.

wxiaoguang · 2025-07-04T14:37:01Z

 	// 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) {


Host should be ASCII only

Hostnames can contain unicode potentially:

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

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".

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.

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

wxiaoguang · 2025-07-04T14:37:33Z


 func (c logCompression) IsNone() bool {
-	return strings.ToLower(string(c)) == "none"
+	return strings.EqualFold(string(c), "none")


It should completely drop the ToLower or EqualFold, we never used case-insensitive config options in app.ini

Done, but I think these are still potentially breaking changes.

I will help to explain if any user complains.

wxiaoguang · 2025-07-04T14:38:38Z

 	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) })


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

Sounds dangerous to just assume that for a utility function?

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.

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

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.

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.

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

silverwind · 2025-07-04T14:40:12Z

Is it possible to forbid strings.EqualFold and force to use our own util.UnicodeEqualFold / AsciiEqualFold to avoid abusing?

That would probably require a custom lint rule and I have no experience writing them.

wxiaoguang · 2025-07-04T14:41:29Z

Hmm, I can see most strings.EqualFold calls in Gitea's codebase are abuses.

silverwind · 2025-07-04T14:45:51Z

@wxiaoguang want to push your fixes here? We could just transform the PR to a be a refactor and remove the lint rule enablement. I have no strong feelings about it and if it's more hindersome, we don't need to enable it.

wxiaoguang · 2025-07-04T14:52:53Z

@wxiaoguang want to push your fixes here? We could just transform the PR to a be a refactor and remove the lint rule enablement. I have no strong feelings about it and if it's more hindersome, we don't need to enable it.

TBH I don't have motivation to touch this part code. Existing code doesn't bother me, there are far more other things that need to fix. So the strings.EqualFold priority is very low on my side.

Signed-off-by: silverwind <me@silverwind.io>

silverwind · 2025-07-04T15:19:53Z

I will leave the rule enabled as it will only trigger for comparing against strings.ToLower() output and strings.EqualFold is the exact replacement for these cases. The user is free to use util.AsciiEqualFold which will also resolve the lint issue.

* giteaofficial/main: [skip ci] Updated translations via Crowdin Rerun job only when run is done (go-gitea#34970) Enable gocritic `equalFold` and fix issues (go-gitea#34952) Fixed minor typos in two files #HSFDPMUW (go-gitea#34944) Improve project & label color picker and image scroll (go-gitea#34971) Refactor webhook and fix feishu/lark secret (go-gitea#34961) Improve OAuth2 provider (correct Issuer, respect ENABLED) (go-gitea#34966) Merge index.js (go-gitea#34963) [skip ci] Updated translations via Crowdin Mark old reviews as stale on agit pr updates (go-gitea#34933) Refactor "delete-button" to "link-action" (go-gitea#34962) Refactor frontend unique id & comment (go-gitea#34958) Refactor some trivial problems (go-gitea#34959) Upgrade security public key (go-gitea#34956) Fix git graph page (go-gitea#34948) Update JS dependencies (go-gitea#34951) Refactor head navbar icons (go-gitea#34922) # Conflicts: # templates/base/head_navbar.tmpl

Enable gocritic equalFold and fix issues

573b40d

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 4, 2025

github-actions Bot added modifies/api labels Jul 4, 2025

silverwind mentioned this pull request Jul 4, 2025

Enable gocritic equalFold and autofix issues #34678

Closed

delvh reviewed Jul 4, 2025

View reviewed changes

Comment thread models/repo/language_stats.go Outdated

Comment thread models/repo/language_stats.go Outdated

wxiaoguang reviewed Jul 4, 2025

View reviewed changes

Comment thread modules/setting/actions.go Outdated

wxiaoguang reviewed Jul 4, 2025

View reviewed changes

Comment thread modules/util/time_str.go

wxiaoguang reviewed Jul 4, 2025

View reviewed changes

Comment thread routers/api/v1/api.go

silverwind commented Jul 4, 2025

View reviewed changes

Comment thread models/repo/language_stats.go Outdated

Update models/repo/language_stats.go

f52fd70

Signed-off-by: silverwind <me@silverwind.io>

silverwind commented Jul 4, 2025

View reviewed changes

Comment thread models/repo/language_stats.go Outdated

Update models/repo/language_stats.go

3282b02

Signed-off-by: silverwind <me@silverwind.io>

silverwind commented Jul 4, 2025

View reviewed changes

Comment thread modules/setting/actions.go Outdated

Update modules/setting/actions.go

13cb941

Signed-off-by: silverwind <me@silverwind.io>

silverwind commented Jul 4, 2025

View reviewed changes

Comment thread modules/setting/actions.go Outdated

Update modules/setting/actions.go

2e8c7d4

Signed-off-by: silverwind <me@silverwind.io>

wxiaoguang approved these changes Jul 4, 2025

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 4, 2025

delvh approved these changes Jul 4, 2025

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 4, 2025

lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 6, 2025

GiteaBot added 5 commits July 6, 2025 13:04

Merge branch 'main' into equalfold2

7d17475

Merge branch 'main' into equalfold2

8e83d6c

Merge branch 'main' into equalfold2

7e914c8

Merge branch 'main' into equalfold2

c700814

Merge branch 'main' into equalfold2

edc11bc

lunny enabled auto-merge (squash) July 6, 2025 16:27

lunny merged commit 95a935a into go-gitea:main Jul 6, 2025
26 checks passed

GiteaBot added this to the 1.25.0 milestone Jul 6, 2025

GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 6, 2025

wxiaoguang deleted the equalfold2 branch July 6, 2025 17:16

go-gitea locked as resolved and limited conversation to collaborators Oct 5, 2025

Uh oh!

Conversation

silverwind commented Jul 4, 2025

Uh oh!

Uh oh!

Uh oh!

wxiaoguang commented Jul 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

silverwind commented Jul 4, 2025

Uh oh!

wxiaoguang commented Jul 4, 2025

Uh oh!

silverwind commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wxiaoguang commented Jul 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

silverwind commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

silverwind Jul 4, 2025 •

edited

Loading

silverwind Jul 4, 2025 •

edited

Loading

silverwind Jul 4, 2025 •

edited

Loading

silverwind commented Jul 4, 2025 •

edited

Loading

silverwind commented Jul 4, 2025 •

edited

Loading