From 9b8de1579760c5fb930dcb0d8a7e92f858e5eec2 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 7 Apr 2024 19:17:06 +0800 Subject: [PATCH] Clean up log messages (#30313) `log.Xxx("%v")` is not ideal, this PR adds necessary context messages. Remove some unnecessary logs. Co-authored-by: Giteabot (cherry picked from commit 83f83019ef3471b847a300f0821499b3896ec987) Conflicts: - modules/util/util.go Conflict resolved by picking `util.Iif` from 654cfd1dfbd3f3f1d94addee50b6fe2b018a49c3 --- cmd/web.go | 2 +- models/asymkey/ssh_key_fingerprint.go | 17 ++++------------- models/repo/issue.go | 2 +- modules/util/util.go | 8 ++++++++ routers/private/actions.go | 16 ++++++++-------- routers/private/hook_verification.go | 3 +-- routers/private/mail.go | 2 +- routers/web/admin/users.go | 1 - routers/web/auth/password.go | 2 -- routers/web/user/setting/account.go | 1 - services/context/captcha.go | 4 ++-- services/notify/notify.go | 4 ++-- services/repository/files/cherry_pick.go | 2 +- services/repository/files/patch.go | 2 +- services/repository/files/update.go | 2 +- services/wiki/wiki.go | 14 +++++++------- 16 files changed, 38 insertions(+), 44 deletions(-) diff --git a/cmd/web.go b/cmd/web.go index ef82486c1..44babd51c 100644 --- a/cmd/web.go +++ b/cmd/web.go @@ -114,7 +114,7 @@ func showWebStartupMessage(msg string) { log.Info("* WorkPath: %s", setting.AppWorkPath) log.Info("* CustomPath: %s", setting.CustomPath) log.Info("* ConfigFile: %s", setting.CustomConf) - log.Info("%s", msg) + log.Info("%s", msg) // show startup message } func serveInstall(ctx *cli.Context) error { diff --git a/models/asymkey/ssh_key_fingerprint.go b/models/asymkey/ssh_key_fingerprint.go index b9cfb1b25..1ed3b5df2 100644 --- a/models/asymkey/ssh_key_fingerprint.go +++ b/models/asymkey/ssh_key_fingerprint.go @@ -76,23 +76,14 @@ func calcFingerprintNative(publicKeyContent string) (string, error) { // CalcFingerprint calculate public key's fingerprint func CalcFingerprint(publicKeyContent string) (string, error) { // Call the method based on configuration - var ( - fnName, fp string - err error - ) - if len(setting.SSH.KeygenPath) == 0 { - fnName = "calcFingerprintNative" - fp, err = calcFingerprintNative(publicKeyContent) - } else { - fnName = "calcFingerprintSSHKeygen" - fp, err = calcFingerprintSSHKeygen(publicKeyContent) - } + useNative := setting.SSH.KeygenPath == "" + calcFn := util.Iif(useNative, calcFingerprintNative, calcFingerprintSSHKeygen) + fp, err := calcFn(publicKeyContent) if err != nil { if IsErrKeyUnableVerify(err) { - log.Info("%s", publicKeyContent) return "", err } - return "", fmt.Errorf("%s: %w", fnName, err) + return "", fmt.Errorf("CalcFingerprint(%s): %w", util.Iif(useNative, "native", "ssh-keygen"), err) } return fp, nil } diff --git a/models/repo/issue.go b/models/repo/issue.go index 6f6b565a0..0dd4fd5ed 100644 --- a/models/repo/issue.go +++ b/models/repo/issue.go @@ -53,7 +53,7 @@ func (repo *Repository) IsDependenciesEnabled(ctx context.Context) bool { var u *RepoUnit var err error if u, err = repo.GetUnit(ctx, unit.TypeIssues); err != nil { - log.Trace("%s", err) + log.Trace("IsDependenciesEnabled: %v", err) return setting.Service.DefaultEnableDependencies } return u.IssuesConfig().EnableDependencies diff --git a/modules/util/util.go b/modules/util/util.go index 7d1a75537..b6ea28355 100644 --- a/modules/util/util.go +++ b/modules/util/util.go @@ -213,6 +213,14 @@ func ToPointer[T any](val T) *T { return &val } +// Iif is an "inline-if", it returns "trueVal" if "condition" is true, otherwise "falseVal" +func Iif[T any](condition bool, trueVal, falseVal T) T { + if condition { + return trueVal + } + return falseVal +} + func ReserveLineBreakForTextarea(input string) string { // Since the content is from a form which is a textarea, the line endings are \r\n. // It's a standard behavior of HTML. diff --git a/routers/private/actions.go b/routers/private/actions.go index 397f20a09..425c480b3 100644 --- a/routers/private/actions.go +++ b/routers/private/actions.go @@ -27,7 +27,7 @@ func GenerateActionsRunnerToken(ctx *context.PrivateContext) { defer rd.Close() if err := json.NewDecoder(rd).Decode(&genRequest); err != nil { - log.Error("%v", err) + log.Error("JSON Decode failed: %v", err) ctx.JSON(http.StatusInternalServerError, private.Response{ Err: err.Error(), }) @@ -36,7 +36,7 @@ func GenerateActionsRunnerToken(ctx *context.PrivateContext) { owner, repo, err := parseScope(ctx, genRequest.Scope) if err != nil { - log.Error("%v", err) + log.Error("parseScope failed: %v", err) ctx.JSON(http.StatusInternalServerError, private.Response{ Err: err.Error(), }) @@ -46,18 +46,18 @@ func GenerateActionsRunnerToken(ctx *context.PrivateContext) { if errors.Is(err, util.ErrNotExist) || (token != nil && !token.IsActive) { token, err = actions_model.NewRunnerToken(ctx, owner, repo) if err != nil { - err := fmt.Sprintf("error while creating runner token: %v", err) - log.Error("%v", err) + errMsg := fmt.Sprintf("error while creating runner token: %v", err) + log.Error("NewRunnerToken failed: %v", errMsg) ctx.JSON(http.StatusInternalServerError, private.Response{ - Err: err, + Err: errMsg, }) return } } else if err != nil { - err := fmt.Sprintf("could not get unactivated runner token: %v", err) - log.Error("%v", err) + errMsg := fmt.Sprintf("could not get unactivated runner token: %v", err) + log.Error("GetLatestRunnerToken failed: %v", errMsg) ctx.JSON(http.StatusInternalServerError, private.Response{ - Err: err, + Err: errMsg, }) return } diff --git a/routers/private/hook_verification.go b/routers/private/hook_verification.go index 42b8e5abe..764c976fa 100644 --- a/routers/private/hook_verification.go +++ b/routers/private/hook_verification.go @@ -47,7 +47,7 @@ func verifyCommits(oldCommitID, newCommitID string, repo *git.Repository, env [] _ = stdoutWriter.Close() err := readAndVerifyCommitsFromShaReader(stdoutReader, repo, env) if err != nil { - log.Error("%v", err) + log.Error("readAndVerifyCommitsFromShaReader failed: %v", err) cancel() } _ = stdoutReader.Close() @@ -66,7 +66,6 @@ func readAndVerifyCommitsFromShaReader(input io.ReadCloser, repo *git.Repository line := scanner.Text() err := readAndVerifyCommit(line, repo, env) if err != nil { - log.Error("%v", err) return err } } diff --git a/routers/private/mail.go b/routers/private/mail.go index c19ee6789..cf3abb31c 100644 --- a/routers/private/mail.go +++ b/routers/private/mail.go @@ -35,7 +35,7 @@ func SendEmail(ctx *context.PrivateContext) { defer rd.Close() if err := json.NewDecoder(rd).Decode(&mail); err != nil { - log.Error("%v", err) + log.Error("JSON Decode failed: %v", err) ctx.JSON(http.StatusInternalServerError, private.Response{ Err: err.Error(), }) diff --git a/routers/web/admin/users.go b/routers/web/admin/users.go index 3dcf0d2aa..15bd667a4 100644 --- a/routers/web/admin/users.go +++ b/routers/web/admin/users.go @@ -403,7 +403,6 @@ func EditUserPost(ctx *context.Context) { ctx.Data["Err_Password"] = true ctx.RenderWithErr(ctx.Tr("auth.password_pwned"), tplUserEdit, &form) case password.IsErrIsPwnedRequest(err): - log.Error("%s", err.Error()) ctx.Data["Err_Password"] = true ctx.RenderWithErr(ctx.Tr("auth.password_pwned_err"), tplUserEdit, &form) default: diff --git a/routers/web/auth/password.go b/routers/web/auth/password.go index d47aeb5e9..d15a8b814 100644 --- a/routers/web/auth/password.go +++ b/routers/web/auth/password.go @@ -214,7 +214,6 @@ func ResetPasswdPost(ctx *context.Context) { case errors.Is(err, password.ErrIsPwned): ctx.RenderWithErr(ctx.Tr("auth.password_pwned"), tplResetPassword, nil) case password.IsErrIsPwnedRequest(err): - log.Error("%s", err.Error()) ctx.RenderWithErr(ctx.Tr("auth.password_pwned_err"), tplResetPassword, nil) default: ctx.ServerError("UpdateAuth", err) @@ -298,7 +297,6 @@ func MustChangePasswordPost(ctx *context.Context) { ctx.Data["Err_Password"] = true ctx.RenderWithErr(ctx.Tr("auth.password_pwned"), tplMustChangePassword, &form) case password.IsErrIsPwnedRequest(err): - log.Error("%s", err.Error()) ctx.Data["Err_Password"] = true ctx.RenderWithErr(ctx.Tr("auth.password_pwned_err"), tplMustChangePassword, &form) default: diff --git a/routers/web/user/setting/account.go b/routers/web/user/setting/account.go index 9cf93320a..795ee59d5 100644 --- a/routers/web/user/setting/account.go +++ b/routers/web/user/setting/account.go @@ -74,7 +74,6 @@ func AccountPost(ctx *context.Context) { case errors.Is(err, password.ErrIsPwned): ctx.Flash.Error(ctx.Tr("auth.password_pwned")) case password.IsErrIsPwnedRequest(err): - log.Error("%s", err.Error()) ctx.Flash.Error(ctx.Tr("auth.password_pwned_err")) default: ctx.ServerError("UpdateAuth", err) diff --git a/services/context/captcha.go b/services/context/captcha.go index a1999900c..fa8d779f5 100644 --- a/services/context/captcha.go +++ b/services/context/captcha.go @@ -79,11 +79,11 @@ func VerifyCaptcha(ctx *Context, tpl base.TplName, form any) { case setting.CfTurnstile: valid, err = turnstile.Verify(ctx, ctx.Req.Form.Get(cfTurnstileResponseField)) default: - ctx.ServerError("Unknown Captcha Type", fmt.Errorf("Unknown Captcha Type: %s", setting.Service.CaptchaType)) + ctx.ServerError("Unknown Captcha Type", fmt.Errorf("unknown Captcha Type: %s", setting.Service.CaptchaType)) return } if err != nil { - log.Debug("%v", err) + log.Debug("Captcha Verify failed: %v", err) } if !valid { diff --git a/services/notify/notify.go b/services/notify/notify.go index 9cb329d30..5ed63646a 100644 --- a/services/notify/notify.go +++ b/services/notify/notify.go @@ -91,7 +91,7 @@ func AutoMergePullRequest(ctx context.Context, doer *user_model.User, pr *issues // NewPullRequest notifies new pull request to notifiers func NewPullRequest(ctx context.Context, pr *issues_model.PullRequest, mentions []*user_model.User) { if err := pr.LoadIssue(ctx); err != nil { - log.Error("%v", err) + log.Error("LoadIssue failed: %v", err) return } if err := pr.Issue.LoadPoster(ctx); err != nil { @@ -112,7 +112,7 @@ func PullRequestSynchronized(ctx context.Context, doer *user_model.User, pr *iss // PullRequestReview notifies new pull request review func PullRequestReview(ctx context.Context, pr *issues_model.PullRequest, review *issues_model.Review, comment *issues_model.Comment, mentions []*user_model.User) { if err := review.LoadReviewer(ctx); err != nil { - log.Error("%v", err) + log.Error("LoadReviewer failed: %v", err) return } for _, notifier := range notifiers { diff --git a/services/repository/files/cherry_pick.go b/services/repository/files/cherry_pick.go index 613b46d8f..451a18215 100644 --- a/services/repository/files/cherry_pick.go +++ b/services/repository/files/cherry_pick.go @@ -28,7 +28,7 @@ func CherryPick(ctx context.Context, repo *repo_model.Repository, doer *user_mod t, err := NewTemporaryUploadRepository(ctx, repo) if err != nil { - log.Error("%v", err) + log.Error("NewTemporaryUploadRepository failed: %v", err) } defer t.Close() if err := t.Clone(opts.OldBranch, false); err != nil { diff --git a/services/repository/files/patch.go b/services/repository/files/patch.go index f6d5643dc..e5f7e2af9 100644 --- a/services/repository/files/patch.go +++ b/services/repository/files/patch.go @@ -111,7 +111,7 @@ func ApplyDiffPatch(ctx context.Context, repo *repo_model.Repository, doer *user t, err := NewTemporaryUploadRepository(ctx, repo) if err != nil { - log.Error("%v", err) + log.Error("NewTemporaryUploadRepository failed: %v", err) } defer t.Close() if err := t.Clone(opts.OldBranch, true); err != nil { diff --git a/services/repository/files/update.go b/services/repository/files/update.go index a6c813f02..e677949e8 100644 --- a/services/repository/files/update.go +++ b/services/repository/files/update.go @@ -143,7 +143,7 @@ func ChangeRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use t, err := NewTemporaryUploadRepository(ctx, repo) if err != nil { - log.Error("%v", err) + log.Error("NewTemporaryUploadRepository failed: %v", err) } defer t.Close() hasOldBranch := true diff --git a/services/wiki/wiki.go b/services/wiki/wiki.go index f2505e82c..ec5d0fc9e 100644 --- a/services/wiki/wiki.go +++ b/services/wiki/wiki.go @@ -209,7 +209,7 @@ func updateWikiPage(ctx context.Context, doer *user_model.User, repo *repo_model if isOldWikiExist { err := gitRepo.RemoveFilesFromIndex(oldWikiPath) if err != nil { - log.Error("%v", err) + log.Error("RemoveFilesFromIndex failed: %v", err) return err } } @@ -219,18 +219,18 @@ func updateWikiPage(ctx context.Context, doer *user_model.User, repo *repo_model objectHash, err := gitRepo.HashObject(strings.NewReader(content)) if err != nil { - log.Error("%v", err) + log.Error("HashObject failed: %v", err) return err } if err := gitRepo.AddObjectToIndex("100644", objectHash, newWikiPath); err != nil { - log.Error("%v", err) + log.Error("AddObjectToIndex failed: %v", err) return err } tree, err := gitRepo.WriteTree() if err != nil { - log.Error("%v", err) + log.Error("WriteTree failed: %v", err) return err } @@ -255,7 +255,7 @@ func updateWikiPage(ctx context.Context, doer *user_model.User, repo *repo_model commitHash, err := gitRepo.CommitTree(doer.NewGitSig(), committer, tree, commitTreeOpts) if err != nil { - log.Error("%v", err) + log.Error("CommitTree failed: %v", err) return err } @@ -270,11 +270,11 @@ func updateWikiPage(ctx context.Context, doer *user_model.User, repo *repo_model 0, ), }); err != nil { - log.Error("%v", err) + log.Error("Push failed: %v", err) if git.IsErrPushOutOfDate(err) || git.IsErrPushRejected(err) { return err } - return fmt.Errorf("Push: %w", err) + return fmt.Errorf("failed to push: %w", err) } return nil