Refactor legacy `unknwon/com` package, improve golangci lint #19284

wxiaoguang · 2022-03-31T17:49:20Z

In this PR, the following refactoring are done. The main purpose is to refactor the legacy unknwon/com.

Remove most imports of unknwon/com, now only util/legacy.go imports the legacy unknwon/com, preparing for next round refactoring
Use golangci's depguard to process denied packages, we do not need to maintain our gitea-vet deny-list anymore in future.
Fix some incorrect values in golangci.yml, for example, the version should be quoted string "1.18", instead of a float number 1.18
Use correctly escaped content for go-import and go-source meta tags
Refactor com.Expand to our stable (and the same fast) vars.Expand.
- The vars.Expand can detect errors and show them in logs or to end users.
- To keep good user experience, our vars.Expand can still return partially rendered content even if the template is not good (eg: key mistach).
- The reason is: in some cases (eg: merge message processing, or generating readme for repo init), we should show the partially rendered templates to users instead of frighten them by 500 errors again and again
- Keep the same behavior and result as the legacy com.Expand

Gusted

Not sure why we're changing from %v", err -> %s", err.Error()

Gusted · 2022-03-31T20:06:46Z

modules/context/csrf.go

@@ -162,7 +163,12 @@ func prepareOptions(options []CsrfOptions) CsrfOptions {

 	// Defaults.
 	if len(opt.Secret) == 0 {
-		opt.Secret = string(com.RandomCreateBytes(10))
+		randBytes, err := util.CryptoRandomBytes(8)


Just to note for other reviewers/maintainers that the reduction of this isn't a concern.

Old situation:
The com.RandomCreateBytes actually doesn't give you the full 8 bits of randomness per byte(so 80 bits in the old case), because it reduces itself to only use the alphanum: https://github.com/unknwon/com/blob/b41c64acd94be7e673c9c8301344d31cce99e06c/string.go#L130 which only gives 6 bits per byte of randomness so it would only receive 48 bits of randomness.

New situation:
With CryptoRandomBytes the full 64 bits would be random.

And before, IIRC the com.RandomCreateBytes uses a math PRNG ..... not crypto-safe.

modules/markup/html.go

modules/repository/init.go

modules/util/path.go

modules/util/util.go

routers/web/repo/issue.go

routers/web/goget.go

Co-authored-by: Gusted <williamzijl7@hotmail.com>

modules/util/util.go

modules/util/copy.go

6543 · 2022-03-31T21:46:33Z

modules/templates/vars/vars.go

+
+// Expand replaces all variables like {var} to match, if error occurs,
+// the error part doesn't change and is returned as it is.
+func Expand(template string, match map[string]string) (string, error) {


doesnt do this the exact same thing the std lib for templates is doing?

Do you mean text/template? I think there are different, I can not remember if there is a exactly the same function in std library.

wxiaoguang · 2022-04-01T02:47:49Z

TBH, I wasn't going to rewrite the AESGCMEncrypt, etc.

I feel that most of us like to do the rewrite, then I can improve this PR with more changes and tests.

Will be back soon.

wxiaoguang · 2022-04-01T04:05:27Z

Updated. Since the AESGCM related functions were updated by Apply suggestions from code review, I added necessary tests for them.

And now there is only one util/legacy.go imports github.com/unknwon/com.

I think this PR is big enough and gets enough changes. More refactoring to the ToStr/ToSnakeCase could be done in a separate PR with more tests.

modules/context/repo.go

Follows: #19284 * The `CopyDir` is only used inside test code * Rewrite `ToSnakeCase` with more test cases * The `RedisCacher` only put strings into cache, here we use internal `toStr` to replace the legacy `ToStr` * The `UniqueQueue` can use string as ID directly, no need to call `ToStr`

wxiaoguang · 2022-04-01T16:58:34Z

After these refactorings, now there are only 2 unmaintained packages:

unknwon/i18n: pretty simple (200 lines) and I think it should be improved.
unknwon/paginater: not complex (180 lines + 300 lines test), no obvious defects.

Do you think we should continue to refactor these 2 packages? If vote yes (up+1), I could do it if I have time.

* giteaofficial/main: Remove legacy unmaintained packages, refactor to support change default locale (go-gitea#19308) [skip ci] Updated translations via Crowdin Prevent intermittent NPE in queue tests (go-gitea#19301) Upgrade xorm/builder from v0.3.9 to v0.3.10 (go-gitea#19296) An attempt to sync a non-mirror repo must give 400 (Bad Request) (go-gitea#19300) Remove legacy `unknwon/com` package (go-gitea#19298) Improve package registry docs (go-gitea#19273) A pull-mirror repo should be marked as such on creation (go-gitea#19295) Refactor legacy `unknwon/com` package, improve golangci lint (go-gitea#19284) Skip frontend ROOT_URL check on installation page, remove unnecessary global var (go-gitea#19291)

lunny and others added 3 commits March 21, 2022 10:12

Use a new vars template implementation

671592b

Remove most calls to com, improve vars.Expand

6b8b5a7

Merge branch 'main' into lunny/vars_expand

d99b308

wxiaoguang mentioned this pull request Mar 31, 2022

Allow custom default merge message with .gitea/default_merge_message/<merge_style>_TEMPLATE.md #18177

Merged

fine tune Expand behavior

b9c1142

wxiaoguang force-pushed the lunny/vars_expand branch from 26f47e5 to b9c1142 Compare March 31, 2022 18:28

wxiaoguang requested a review from 6543 March 31, 2022 18:30

fix goget response

c202109

Gusted reviewed Mar 31, 2022

View reviewed changes

GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 31, 2022

Apply suggestions from code review

5ea73ab

Co-authored-by: Gusted <williamzijl7@hotmail.com>

6543 reviewed Mar 31, 2022

View reviewed changes

modules/util/util.go Outdated Show resolved Hide resolved

6543 reviewed Mar 31, 2022

View reviewed changes

modules/util/util.go Outdated Show resolved Hide resolved

6543 reviewed Mar 31, 2022

View reviewed changes

modules/util/copy.go Outdated Show resolved Hide resolved

6543 reviewed Mar 31, 2022

View reviewed changes

wxiaoguang added 7 commits April 1, 2022 10:49

fix build

e9008eb

Merge branch 'main' into lunny/vars_expand

cb153a5

Merge branch 'main' into lunny/vars_expand

655506d

fine tune

7376a8a

fix lint

6ded737

fine tune

554886d

fine tune

d57054e

wxiaoguang requested review from 6543 and Gusted April 1, 2022 04:13

This comment was marked as off-topic.

Sign in to view

fine tune test

73602a2

wxiaoguang force-pushed the lunny/vars_expand branch from d1d1708 to 73602a2 Compare April 1, 2022 04:55

lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Apr 1, 2022

lunny reviewed Apr 1, 2022

View reviewed changes

modules/context/repo.go Outdated Show resolved Hide resolved

Gusted approved these changes Apr 1, 2022

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 Apr 1, 2022

wxiaoguang added 2 commits April 1, 2022 14:15

make go-import and go-source escaped correctly

60f9948

Merge branch 'main' into lunny/vars_expand

f646c1c

wxiaoguang added this to the 1.17.0 milestone Apr 1, 2022

wxiaoguang added 2 commits April 1, 2022 14:50

fix unit test

5f115ab

Merge branch 'main' into lunny/vars_expand

f51b15c

lafriks approved these changes Apr 1, 2022

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 Apr 1, 2022

wxiaoguang merged commit 65f17bf into go-gitea:main Apr 1, 2022

wxiaoguang deleted the lunny/vars_expand branch April 1, 2022 08:47

wxiaoguang mentioned this pull request Apr 1, 2022

Remove legacy unknwon/com package #19298

Merged

lunny mentioned this pull request Apr 2, 2022

Use a new vars template implementation #19156

Closed

This was referenced Apr 2, 2022

Remove legacy unmaintained packages, refactor to support change default locale #19308

Merged

Replace com.Expand #14760

Closed

go-gitea locked and limited conversation to collaborators Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor legacy `unknwon/com` package, improve golangci lint #19284

Refactor legacy `unknwon/com` package, improve golangci lint #19284

wxiaoguang commented Mar 31, 2022 •

edited

Loading

Gusted left a comment

Gusted Mar 31, 2022

wxiaoguang Apr 1, 2022

6543 Mar 31, 2022

wxiaoguang Apr 1, 2022

wxiaoguang commented Apr 1, 2022

wxiaoguang commented Apr 1, 2022 •

edited

Loading

This comment was marked as off-topic.

wxiaoguang commented Apr 1, 2022

Refactor legacy unknwon/com package, improve golangci lint #19284

Refactor legacy unknwon/com package, improve golangci lint #19284

Conversation

wxiaoguang commented Mar 31, 2022 • edited Loading

Gusted left a comment

Choose a reason for hiding this comment

Gusted Mar 31, 2022

Choose a reason for hiding this comment

wxiaoguang Apr 1, 2022

Choose a reason for hiding this comment

6543 Mar 31, 2022

Choose a reason for hiding this comment

wxiaoguang Apr 1, 2022

Choose a reason for hiding this comment

wxiaoguang commented Apr 1, 2022

wxiaoguang commented Apr 1, 2022 • edited Loading

This comment was marked as off-topic.

wxiaoguang commented Apr 1, 2022

Refactor legacy `unknwon/com` package, improve golangci lint #19284

Refactor legacy `unknwon/com` package, improve golangci lint #19284

wxiaoguang commented Mar 31, 2022 •

edited

Loading

wxiaoguang commented Apr 1, 2022 •

edited

Loading