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 <[email protected]>

Author: wxiaoguang

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
+	}
 }

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)
 	}
 }
 

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,

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{

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{

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

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)

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)
 }

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,17 +129,16 @@ 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()
 
 	for _, w := range l.eventWriters {
 		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
 }

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

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) {