chore: fix some trivial problems and TODOs (#33473)

1. Fix incorrect `MentionCount` (actually it seems to be deadcode,
affects nothing)
2. Remove fallback sha1 support for time limit token
3. Use route middleware `reqRepoActionsWriter` for `ArtifactsDeleteView`
4. Use clearer message "Failed to authenticate user" instead of "Verify"
when auth fails
5. `tests/integration/benchmarks_test.go` is not quite right, actually
it is never used, so delete it.
6. Remove or update TODO comments

models/issues/issue_stats.go

@@ -107,7 +107,7 @@ func GetIssueStats(ctx context.Context, opts *IssuesOptions) (*IssueStats, error
 		accum.YourRepositoriesCount += stats.YourRepositoriesCount
 		accum.AssignCount += stats.AssignCount
 		accum.CreateCount += stats.CreateCount
-		accum.OpenCount += stats.MentionCount
+		accum.MentionCount += stats.MentionCount
 		accum.ReviewRequestedCount += stats.ReviewRequestedCount
 		accum.ReviewedCount += stats.ReviewedCount
 		i = chunk

models/system/notice_test.go

@@ -45,8 +45,6 @@ func TestCreateRepositoryNotice(t *testing.T) {
 	unittest.AssertExistsAndLoadBean(t, noticeBean)
 }
 
-// TODO TestRemoveAllWithNotice
-
 func TestCountNotices(t *testing.T) {
 	assert.NoError(t, unittest.PrepareTestDatabase())
 	assert.Equal(t, int64(3), system.CountNotices(db.DefaultContext))

modules/base/tool.go

@@ -18,7 +18,6 @@ import (
 	"time"
 
 	"code.gitea.io/gitea/modules/git"
-	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/util"
 
@@ -64,10 +63,7 @@ func VerifyTimeLimitCode(now time.Time, data string, minutes int, code string) b
 	// check code
 	retCode := CreateTimeLimitCode(data, aliveTime, startTimeStr, nil)
 	if subtle.ConstantTimeCompare([]byte(retCode), []byte(code)) != 1 {
-		retCode = CreateTimeLimitCode(data, aliveTime, startTimeStr, sha1.New()) // TODO: this is only for the support of legacy codes, remove this in/after 1.23
+		return false
-		if subtle.ConstantTimeCompare([]byte(retCode), []byte(code)) != 1 {
-			return false
-		}
 	}
 
 	// check time is expired or not: startTime <= now && now < startTime + minutes
@@ -144,13 +140,12 @@ func Int64sToStrings(ints []int64) []string {
 	return strs
 }
 
-// EntryIcon returns the octicon class for displaying files/directories
+// EntryIcon returns the octicon name for displaying files/directories
 func EntryIcon(entry *git.TreeEntry) string {
 	switch {
 	case entry.IsLink():
 		te, err := entry.FollowLink()
 		if err != nil {
-			log.Debug(err.Error())
 			return "file-symlink-file"
 		}
 		if te.IsDir() {

modules/base/tool_test.go

@@ -86,13 +86,10 @@ JWT_SECRET = %s
 		verifyDataCode := func(c string) bool {
 			return VerifyTimeLimitCode(now, "data", 2, c)
 		}
-		code1 := CreateTimeLimitCode("data", 2, now, sha1.New())
+		code := CreateTimeLimitCode("data", 2, now, nil)
-		code2 := CreateTimeLimitCode("data", 2, now, nil)
+		assert.True(t, verifyDataCode(code))
-		assert.True(t, verifyDataCode(code1))
-		assert.True(t, verifyDataCode(code2))
 		initGeneralSecret("000_QLUd4fYVyxetjxC4eZkrBgWM2SndOOWDNtgUUko")
-		assert.False(t, verifyDataCode(code1))
+		assert.False(t, verifyDataCode(code))
-		assert.False(t, verifyDataCode(code2))
 	})
 }
 
@@ -137,5 +134,3 @@ func TestInt64sToStrings(t *testing.T) {
 		Int64sToStrings([]int64{1, 4, 16, 64, 256}),
 	)
 }
-
-// TODO: Test EntryIcon

modules/markup/html.go

@@ -47,7 +47,7 @@ var globalVars = sync.OnceValue(func() *globalVarsType {
 	// NOTE: All below regex matching do not perform any extra validation.
 	// Thus a link is produced even if the linked entity does not exist.
 	// While fast, this is also incorrect and lead to false positives.
-	// TODO: fix invalid linking issue
+	// TODO: fix invalid linking issue (update: stale TODO, what issues? maybe no TODO anymore)
 
 	// valid chars in encoded path and parameter: [-+~_%.a-zA-Z0-9/]
 

modules/web/middleware/binding.go

@@ -78,7 +78,7 @@ func GetInclude(field reflect.StructField) string {
 	return getRuleBody(field, "Include(")
 }
 
-// Validate validate TODO:
+// Validate validate
 func Validate(errs binding.Errors, data map[string]any, f Form, l translation.Locale) binding.Errors {
 	if errs.Len() == 0 {
 		return errs

routers/web/repo/actions/view.go

@@ -628,11 +628,6 @@ func getRunJobs(ctx *context_module.Context, runIndex, jobIndex int64) (*actions
 }
 
 func ArtifactsDeleteView(ctx *context_module.Context) {
-	if !ctx.Repo.CanWrite(unit.TypeActions) {
-		ctx.Error(http.StatusForbidden, "no permission")
-		return
-	}
-
 	runIndex := getRunIndex(ctx)
 	artifactName := ctx.PathParam("artifact_name")
 

routers/web/web.go

@@ -118,7 +118,7 @@ func webAuth(authMethod auth_service.Method) func(*context.Context) {
 		ar, err := common.AuthShared(ctx.Base, ctx.Session, authMethod)
 		if err != nil {
 			log.Error("Failed to verify user: %v", err)
-			ctx.Error(http.StatusUnauthorized, "Verify")
+			ctx.Error(http.StatusUnauthorized, "Failed to authenticate user")
 			return
 		}
 		ctx.Doer = ar.Doer
@@ -1430,7 +1430,7 @@ func registerRoutes(m *web.Router) {
 			m.Post("/cancel", reqRepoActionsWriter, actions.Cancel)
 			m.Post("/approve", reqRepoActionsWriter, actions.Approve)
 			m.Get("/artifacts/{artifact_name}", actions.ArtifactsDownloadView)
-			m.Delete("/artifacts/{artifact_name}", actions.ArtifactsDeleteView)
+			m.Delete("/artifacts/{artifact_name}", reqRepoActionsWriter, actions.ArtifactsDeleteView)
 			m.Post("/rerun", reqRepoActionsWriter, actions.Rerun)
 		})
 		m.Group("/workflows/{workflow_name}", func() {

services/mailer/mail_test.go

@@ -85,7 +85,7 @@ func TestComposeIssueCommentMessage(t *testing.T) {
 
 	recipients := []*user_model.User{{Name: "Test", Email: "test@gitea.com"}, {Name: "Test2", Email: "test2@gitea.com"}}
 	msgs, err := composeIssueCommentMessages(&mailCommentContext{
-		Context: context.TODO(), // TODO: use a correct context
+		Context: context.TODO(),
 		Issue:   issue, Doer: doer, ActionType: activities_model.ActionCommentIssue,
 		Content: fmt.Sprintf("test @%s %s#%d body", doer.Name, issue.Repo.FullName(), issue.Index),
 		Comment: comment,
@@ -131,7 +131,7 @@ func TestComposeIssueMessage(t *testing.T) {
 
 	recipients := []*user_model.User{{Name: "Test", Email: "test@gitea.com"}, {Name: "Test2", Email: "test2@gitea.com"}}
 	msgs, err := composeIssueCommentMessages(&mailCommentContext{
-		Context: context.TODO(), // TODO: use a correct context
+		Context: context.TODO(),
 		Issue:   issue, Doer: doer, ActionType: activities_model.ActionCreateIssue,
 		Content: "test body",
 	}, "en-US", recipients, false, "issue create")
@@ -178,14 +178,14 @@ func TestTemplateSelection(t *testing.T) {
 	}
 
 	msg := testComposeIssueCommentMessage(t, &mailCommentContext{
-		Context: context.TODO(), // TODO: use a correct context
+		Context: context.TODO(),
 		Issue:   issue, Doer: doer, ActionType: activities_model.ActionCreateIssue,
 		Content: "test body",
 	}, recipients, false, "TestTemplateSelection")
 	expect(t, msg, "issue/new/subject", "issue/new/body")
 
 	msg = testComposeIssueCommentMessage(t, &mailCommentContext{
-		Context: context.TODO(), // TODO: use a correct context
+		Context: context.TODO(),
 		Issue:   issue, Doer: doer, ActionType: activities_model.ActionCommentIssue,
 		Content: "test body", Comment: comment,
 	}, recipients, false, "TestTemplateSelection")
@@ -194,14 +194,14 @@ func TestTemplateSelection(t *testing.T) {
 	pull := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2, Repo: repo, Poster: doer})
 	comment = unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 4, Issue: pull})
 	msg = testComposeIssueCommentMessage(t, &mailCommentContext{
-		Context: context.TODO(), // TODO: use a correct context
+		Context: context.TODO(),
 		Issue:   pull, Doer: doer, ActionType: activities_model.ActionCommentPull,
 		Content: "test body", Comment: comment,
 	}, recipients, false, "TestTemplateSelection")
 	expect(t, msg, "pull/comment/subject", "pull/comment/body")
 
 	msg = testComposeIssueCommentMessage(t, &mailCommentContext{
-		Context: context.TODO(), // TODO: use a correct context
+		Context: context.TODO(),
 		Issue:   issue, Doer: doer, ActionType: activities_model.ActionCloseIssue,
 		Content: "test body", Comment: comment,
 	}, recipients, false, "TestTemplateSelection")
@@ -220,7 +220,7 @@ func TestTemplateServices(t *testing.T) {
 
 		recipients := []*user_model.User{{Name: "Test", Email: "test@gitea.com"}}
 		msg := testComposeIssueCommentMessage(t, &mailCommentContext{
-			Context: context.TODO(), // TODO: use a correct context
+			Context: context.TODO(),
 			Issue:   issue, Doer: doer, ActionType: actionType,
 			Content: "test body", Comment: comment,
 		}, recipients, fromMention, "TestTemplateServices")
@@ -263,7 +263,7 @@ func testComposeIssueCommentMessage(t *testing.T, ctx *mailCommentContext, recip
 func TestGenerateAdditionalHeaders(t *testing.T) {
 	doer, _, issue, _ := prepareMailerTest(t)
 
-	ctx := &mailCommentContext{Context: context.TODO() /* TODO: use a correct context */, Issue: issue, Doer: doer}
+	ctx := &mailCommentContext{Context: context.TODO(), Issue: issue, Doer: doer}
 	recipient := &user_model.User{Name: "test", Email: "test@gitea.com"}
 
 	headers := generateAdditionalHeaders(ctx, "dummy-reason", recipient)

tests/integration/benchmarks_test.go

@@ -1,69 +0,0 @@
-// Copyright 2017 The Gitea Authors. All rights reserved.
-// SPDX-License-Identifier: MIT
-
-package integration
-
-import (
-	"math/rand/v2"
-	"net/http"
-	"net/url"
-	"testing"
-
-	repo_model "code.gitea.io/gitea/models/repo"
-	"code.gitea.io/gitea/models/unittest"
-	api "code.gitea.io/gitea/modules/structs"
-)
-
-// StringWithCharset random string (from https://www.calhoun.io/creating-random-strings-in-go/)
-func StringWithCharset(length int, charset string) string {
-	b := make([]byte, length)
-	for i := range b {
-		b[i] = charset[rand.IntN(len(charset))]
-	}
-	return string(b)
-}
-
-func BenchmarkRepoBranchCommit(b *testing.B) {
-	onGiteaRun(b, func(b *testing.B, u *url.URL) {
-		samples := []int64{1, 2, 3}
-		b.ResetTimer()
-
-		for _, repoID := range samples {
-			b.StopTimer()
-			repo := unittest.AssertExistsAndLoadBean(b, &repo_model.Repository{ID: repoID})
-			b.StartTimer()
-			b.Run(repo.Name, func(b *testing.B) {
-				session := loginUser(b, "user2")
-				b.ResetTimer()
-				b.Run("CreateBranch", func(b *testing.B) {
-					b.StopTimer()
-					branchName := StringWithCharset(5+rand.IntN(10), "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789")
-					b.StartTimer()
-					for i := 0; i < b.N; i++ {
-						b.Run("new_"+branchName, func(b *testing.B) {
-							b.Skip("benchmark broken") // TODO fix
-							testAPICreateBranch(b, session, repo.OwnerName, repo.Name, repo.DefaultBranch, "new_"+branchName, http.StatusCreated)
-						})
-					}
-				})
-				b.Run("GetBranches", func(b *testing.B) {
-					req := NewRequestf(b, "GET", "/api/v1/repos/%s/branches", repo.FullName())
-					session.MakeRequest(b, req, http.StatusOK)
-				})
-				b.Run("AccessCommits", func(b *testing.B) {
-					var branches []*api.Branch
-					req := NewRequestf(b, "GET", "/api/v1/repos/%s/branches", repo.FullName())
-					resp := session.MakeRequest(b, req, http.StatusOK)
-					DecodeJSON(b, resp, &branches)
-					b.ResetTimer() // We measure from here
-					if len(branches) != 0 {
-						for i := 0; i < b.N; i++ {
-							req := NewRequestf(b, "GET", "/api/v1/repos/%s/commits?sha=%s", repo.FullName(), branches[i%len(branches)].Name)
-							session.MakeRequest(b, req, http.StatusOK)
-						}
-					}
-				})
-			})
-		}
-	})
-}