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

Conversation

silverwind
Copy link
Member

Continuation of #34678.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 4, 2025
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/internal labels Jul 4, 2025
@wxiaoguang
Copy link
Contributor

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.

@@ -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

@@ -62,11 +62,11 @@ func (c logCompression) IsValid() bool {
}

func (c logCompression) IsNone() bool {
return strings.ToLower(string(c)) == "none"
return strings.EqualFold(string(c), "none")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I will help to explain if any user complains.

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

@silverwind
Copy link
Member Author

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
Copy link
Contributor

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

@silverwind
Copy link
Member Author

silverwind commented Jul 4, 2025

@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
Copy link
Contributor

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

@GiteaBot 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
@GiteaBot 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
@silverwind
Copy link
Member Author

silverwind commented Jul 4, 2025

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.

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 6, 2025
@lunny lunny enabled auto-merge (squash) July 6, 2025 16:27
@lunny lunny merged commit 95a935a into go-gitea:main Jul 6, 2025
26 checks passed
@GiteaBot GiteaBot added this to the 1.25.0 milestone Jul 6, 2025
@GiteaBot 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 wxiaoguang deleted the equalfold2 branch July 6, 2025 17:16
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 7, 2025
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/internal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants