From 65d3e1161b0544f50d94dbbecb6bcf135535e112 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 28 Jun 2023 14:02:06 +0800 Subject: [PATCH] Fix sub-command log level (#25537) More fix for #24981 * #24981 Close #22361 * #22361 There were many patches for Gitea's sub-commands to satisfy the facts: * Some sub-commands shouldn't output any log, otherwise the git protocol would be broken * Sometimes the users want to see "verbose" or "quiet" outputs That's a longstanding problem, and very fragile. This PR is only a quick patch for the problem. In the future, the sub-command system should be refactored to a clear solution. ---- Other changes: * Use `ReplaceAllWriters` to replace `RemoveAllWriters().AddWriters(writer)`, then it's an atomic operation. * Remove unnecessary `syncLevelInternal` calls, because `AddWriters/addWritersInternal` already calls it. Co-authored-by: Giteabot --- cmd/cmd.go | 18 +++++++++++++++++- cmd/doctor.go | 2 +- cmd/hook.go | 2 ++ cmd/keys.go | 2 ++ cmd/serv.go | 1 + cmd/web.go | 6 +----- main.go | 7 +++++-- modules/log/logger_global.go | 2 +- modules/log/logger_impl.go | 13 +++++++------ modules/log/manager_test.go | 2 +- modules/setting/log.go | 2 +- 11 files changed, 39 insertions(+), 18 deletions(-) diff --git a/cmd/cmd.go b/cmd/cmd.go index 8076aceca..4ed636a9b 100644 --- a/cmd/cmd.go +++ b/cmd/cmd.go @@ -106,5 +106,21 @@ func setupConsoleLogger(level log.Level, colorize bool, out io.Writer) { WriterOption: log.WriterConsoleOption{Stderr: out == os.Stderr}, } writer := log.NewEventWriterConsole("console-default", writeMode) - log.GetManager().GetLogger(log.DEFAULT).RemoveAllWriters().AddWriters(writer) + log.GetManager().GetLogger(log.DEFAULT).ReplaceAllWriters(writer) +} + +// PrepareConsoleLoggerLevel by default, use INFO level for console logger, but some sub-commands (for git/ssh protocol) shouldn't output any log to stdout. +// Any log appears in git stdout pipe will break the git protocol, eg: client can't push and hangs forever. +func PrepareConsoleLoggerLevel(defaultLevel log.Level) func(*cli.Context) error { + return func(c *cli.Context) error { + level := defaultLevel + if c.Bool("quiet") || c.GlobalBoolT("quiet") { + level = log.FATAL + } + if c.Bool("debug") || c.GlobalBool("debug") || c.Bool("verbose") || c.GlobalBool("verbose") { + level = log.TRACE + } + log.SetConsoleLogger(log.DEFAULT, "console-default", level) + return nil + } } diff --git a/cmd/doctor.go b/cmd/doctor.go index b79436fc0..cd5f125e2 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -151,7 +151,7 @@ func setupDoctorDefaultLogger(ctx *cli.Context, colorize bool) { log.FallbackErrorf("unable to create file log writer: %v", err) return } - log.GetManager().GetLogger(log.DEFAULT).RemoveAllWriters().AddWriters(writer) + log.GetManager().GetLogger(log.DEFAULT).ReplaceAllWriters(writer) } } diff --git a/cmd/hook.go b/cmd/hook.go index 645326783..ed6efc19e 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -15,6 +15,7 @@ import ( "time" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/private" repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" @@ -32,6 +33,7 @@ var ( Name: "hook", Usage: "Delegate commands to corresponding Git hooks", Description: "This should only be called by Git", + Before: PrepareConsoleLoggerLevel(log.FATAL), Subcommands: []cli.Command{ subcmdHookPreReceive, subcmdHookUpdate, diff --git a/cmd/keys.go b/cmd/keys.go index deb94fca5..8aeee3752 100644 --- a/cmd/keys.go +++ b/cmd/keys.go @@ -8,6 +8,7 @@ import ( "fmt" "strings" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/private" "github.com/urfave/cli" @@ -17,6 +18,7 @@ import ( var CmdKeys = cli.Command{ Name: "keys", Usage: "This command queries the Gitea database to get the authorized command for a given ssh key fingerprint", + Before: PrepareConsoleLoggerLevel(log.FATAL), Action: runKeys, Flags: []cli.Flag{ cli.StringFlag{ diff --git a/cmd/serv.go b/cmd/serv.go index 01102d380..79052e58a 100644 --- a/cmd/serv.go +++ b/cmd/serv.go @@ -44,6 +44,7 @@ var CmdServ = cli.Command{ Name: "serv", Usage: "This command should only be called by SSH shell", Description: "Serv provides access auth for repositories", + Before: PrepareConsoleLoggerLevel(log.FATAL), Action: runServ, Flags: []cli.Flag{ cli.BoolFlag{ diff --git a/cmd/web.go b/cmd/web.go index 7a257a62a..05f3b2ddb 100644 --- a/cmd/web.go +++ b/cmd/web.go @@ -35,6 +35,7 @@ var CmdWeb = cli.Command{ Usage: "Start Gitea web server", Description: `Gitea web server is the only thing you need to run, and it takes care of all the other things for you`, + Before: PrepareConsoleLoggerLevel(log.INFO), Action: runWeb, Flags: []cli.Flag{ cli.StringFlag{ @@ -206,11 +207,6 @@ func servePprof() { } func runWeb(ctx *cli.Context) error { - if ctx.Bool("verbose") { - setupConsoleLogger(log.TRACE, log.CanColorStdout, os.Stdout) - } else if ctx.Bool("quiet") { - setupConsoleLogger(log.FATAL, log.CanColorStdout, os.Stdout) - } defer func() { if panicked := recover(); panicked != nil { log.Fatal("PANIC: %v\n%s", panicked, log.Stack(2)) diff --git a/main.go b/main.go index 7b447e753..f604311f5 100644 --- a/main.go +++ b/main.go @@ -108,7 +108,9 @@ func main() { cmd.CmdActions, } - // default configuration flags + // shared configuration flags, they are for global and for each sub-command at the same time + // eg: such command is valid: "./gitea --config /tmp/app.ini web --config /tmp/app.ini", while it's discouraged indeed + // keep in mind that the short flags like "-C", "-c" and "-w" are globally polluted, they can't be used for sub-commands anymore. globalFlags := []cli.Flag{ cli.HelpFlag, cli.StringFlag{ @@ -128,9 +130,10 @@ func main() { // Set the default to be equivalent to cmdWeb and add the default flags app.Flags = append(app.Flags, globalFlags...) - app.Flags = append(app.Flags, cmd.CmdWeb.Flags...) + app.Flags = append(app.Flags, cmd.CmdWeb.Flags...) // TODO: the web flags polluted the global flags, they are not really global flags app.Action = prepareWorkPathAndCustomConf(cmd.CmdWeb.Action) app.HideHelp = true // use our own help action to show helps (with more information like default config) + app.Before = cmd.PrepareConsoleLoggerLevel(log.INFO) app.Commands = append(app.Commands, cmdHelp) for i := range app.Commands { prepareSubcommands(&app.Commands[i], globalFlags) diff --git a/modules/log/logger_global.go b/modules/log/logger_global.go index 5ccef34b5..994acfedb 100644 --- a/modules/log/logger_global.go +++ b/modules/log/logger_global.go @@ -79,5 +79,5 @@ func SetConsoleLogger(loggerName, writerName string, level Level) { Colorize: CanColorStdout, WriterOption: WriterConsoleOption{}, }) - GetManager().GetLogger(loggerName).RemoveAllWriters().AddWriters(writer) + GetManager().GetLogger(loggerName).ReplaceAllWriters(writer) } diff --git a/modules/log/logger_impl.go b/modules/log/logger_impl.go index 903d8cefc..d38c6516e 100644 --- a/modules/log/logger_impl.go +++ b/modules/log/logger_impl.go @@ -96,7 +96,10 @@ func (l *LoggerImpl) removeWriterInternal(w EventWriter) { func (l *LoggerImpl) AddWriters(writer ...EventWriter) { l.eventWriterMu.Lock() defer l.eventWriterMu.Unlock() + l.addWritersInternal(writer...) +} +func (l *LoggerImpl) addWritersInternal(writer ...EventWriter) { for _, w := range writer { if old, ok := l.eventWriters[w.GetWriterName()]; ok { l.removeWriterInternal(old) @@ -126,8 +129,8 @@ func (l *LoggerImpl) RemoveWriter(modeName string) error { return nil } -// RemoveAllWriters removes all writers from the logger, non-shared writers are closed and flushed -func (l *LoggerImpl) RemoveAllWriters() *LoggerImpl { +// ReplaceAllWriters replaces all writers from the logger, non-shared writers are closed and flushed +func (l *LoggerImpl) ReplaceAllWriters(writer ...EventWriter) { l.eventWriterMu.Lock() defer l.eventWriterMu.Unlock() @@ -135,8 +138,7 @@ func (l *LoggerImpl) RemoveAllWriters() *LoggerImpl { l.removeWriterInternal(w) } l.eventWriters = map[string]EventWriter{} - l.syncLevelInternal() - return l + l.addWritersInternal(writer...) } // DumpWriters dumps the writers as a JSON map, it's used for debugging and display purposes. @@ -161,7 +163,7 @@ func (l *LoggerImpl) DumpWriters() map[string]any { // Close closes the logger, non-shared writers are closed and flushed func (l *LoggerImpl) Close() { - l.RemoveAllWriters() + l.ReplaceAllWriters() l.ctxCancel() } @@ -233,7 +235,6 @@ func NewLoggerWithWriters(ctx context.Context, name string, writer ...EventWrite l.ctx, l.ctxCancel = newProcessTypedContext(ctx, "Logger: "+name) l.LevelLogger = BaseLoggerToGeneralLogger(l) l.eventWriters = map[string]EventWriter{} - l.syncLevelInternal() l.AddWriters(writer...) return l } diff --git a/modules/log/manager_test.go b/modules/log/manager_test.go index aa01f7998..b8fbf8461 100644 --- a/modules/log/manager_test.go +++ b/modules/log/manager_test.go @@ -23,7 +23,7 @@ func TestSharedWorker(t *testing.T) { loggerTest := m.GetLogger("test") loggerTest.AddWriters(w) loggerTest.Info("msg-1") - loggerTest.RemoveAllWriters() // the shared writer is not closed here + loggerTest.ReplaceAllWriters() // the shared writer is not closed here loggerTest.Info("never seen") // the shared writer can still be used later diff --git a/modules/setting/log.go b/modules/setting/log.go index af64ea8d8..66206f8f4 100644 --- a/modules/setting/log.go +++ b/modules/setting/log.go @@ -244,7 +244,7 @@ func initLoggerByName(manager *log.LoggerManager, rootCfg ConfigProvider, logger eventWriters = append(eventWriters, eventWriter) } - manager.GetLogger(loggerName).RemoveAllWriters().AddWriters(eventWriters...) + manager.GetLogger(loggerName).ReplaceAllWriters(eventWriters...) } func InitSQLLoggersForCli(level log.Level) {