Fix incorrect CLI exit code and duplicate error message (#26346)

Follow the CLI refactoring, and add tests.

cmd/main.go

@@ -6,6 +6,7 @@ package cmd
 import (
 	"fmt"
 	"os"
+	"strings"
 
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
@@ -117,8 +118,12 @@ func prepareWorkPathAndCustomConf(action cli.ActionFunc) func(ctx *cli.Context)
 	}
 }
 
-func NewMainApp() *cli.App {
+func NewMainApp(version, versionExtra string) *cli.App {
 	app := cli.NewApp()
+	app.Name = "Gitea"
+	app.Usage = "A painless self-hosted Git service"
+	app.Description = `By default, Gitea will start serving using the web-server with no argument, which can alternatively be run by running the subcommand "web".`
+	app.Version = version + versionExtra
 	app.EnableBashCompletion = true
 
 	// these sub-commands need to use config file
@@ -166,3 +171,18 @@ func NewMainApp() *cli.App {
 
 	return app
 }
+
+func RunMainApp(app *cli.App, args ...string) error {
+	err := app.Run(args)
+	if err == nil {
+		return nil
+	}
+	if strings.HasPrefix(err.Error(), "flag provided but not defined:") {
+		// the cli package should already have output the error message, so just exit
+		cli.OsExiter(1)
+		return err
+	}
+	_, _ = fmt.Fprintf(app.ErrWriter, "Command error: %v\n", err)
+	cli.OsExiter(1)
+	return err
+}

cmd/main_test.go

@@ -5,6 +5,7 @@ package cmd
 
 import (
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
 	"strings"
@@ -12,6 +13,7 @@ import (
 
 	"code.gitea.io/gitea/models/unittest"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/test"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/urfave/cli/v2"
@@ -27,21 +29,38 @@ func makePathOutput(workPath, customPath, customConf string) string {
 	return fmt.Sprintf("WorkPath=%s\nCustomPath=%s\nCustomConf=%s", workPath, customPath, customConf)
 }
 
-func newTestApp() *cli.App {
-	app := NewMainApp()
-	testCmd := &cli.Command{
-		Name: "test-cmd",
-		Action: func(ctx *cli.Context) error {
-			_, _ = fmt.Fprint(app.Writer, makePathOutput(setting.AppWorkPath, setting.CustomPath, setting.CustomConf))
-			return nil
-		},
-	}
+func newTestApp(testCmdAction func(ctx *cli.Context) error) *cli.App {
+	app := NewMainApp("version", "version-extra")
+	testCmd := &cli.Command{Name: "test-cmd", Action: testCmdAction}
 	prepareSubcommandWithConfig(testCmd, appGlobalFlags())
 	app.Commands = append(app.Commands, testCmd)
 	app.DefaultCommand = testCmd.Name
 	return app
 }
 
+type runResult struct {
+	Stdout   string
+	Stderr   string
+	ExitCode int
+}
+
+func runTestApp(app *cli.App, args ...string) (runResult, error) {
+	outBuf := new(strings.Builder)
+	errBuf := new(strings.Builder)
+	app.Writer = outBuf
+	app.ErrWriter = errBuf
+	exitCode := -1
+	defer test.MockVariableValue(&cli.ErrWriter, app.ErrWriter)()
+	defer test.MockVariableValue(&cli.OsExiter, func(code int) {
+		if exitCode == -1 {
+			exitCode = code // save the exit code once and then reset the writer (to simulate the exit)
+			app.Writer, app.ErrWriter, cli.ErrWriter = io.Discard, io.Discard, io.Discard
+		}
+	})()
+	err := RunMainApp(app, args...)
+	return runResult{outBuf.String(), errBuf.String(), exitCode}, err
+}
+
 func TestCliCmd(t *testing.T) {
 	defaultWorkPath := filepath.Dir(setting.AppPath)
 	defaultCustomPath := filepath.Join(defaultWorkPath, "custom")
@@ -92,7 +111,10 @@ func TestCliCmd(t *testing.T) {
 		},
 	}
 
-	app := newTestApp()
+	app := newTestApp(func(ctx *cli.Context) error {
+		_, _ = fmt.Fprint(ctx.App.Writer, makePathOutput(setting.AppWorkPath, setting.CustomPath, setting.CustomConf))
+		return nil
+	})
 	var envBackup []string
 	for _, s := range os.Environ() {
 		if strings.HasPrefix(s, "GITEA_") && strings.Contains(s, "=") {
@@ -120,12 +142,39 @@ func TestCliCmd(t *testing.T) {
 			_ = os.Setenv(k, v)
 		}
 		args := strings.Split(c.cmd, " ") // for test only, "split" is good enough
-		out := new(strings.Builder)
-		app.Writer = out
-		err := app.Run(args)
+		r, err := runTestApp(app, args...)
 		assert.NoError(t, err, c.cmd)
 		assert.NotEmpty(t, c.exp, c.cmd)
-		outStr := out.String()
-		assert.Contains(t, outStr, c.exp, c.cmd)
+		assert.Contains(t, r.Stdout, c.exp, c.cmd)
 	}
 }
+
+func TestCliCmdError(t *testing.T) {
+	app := newTestApp(func(ctx *cli.Context) error { return fmt.Errorf("normal error") })
+	r, err := runTestApp(app, "./gitea", "test-cmd")
+	assert.Error(t, err)
+	assert.Equal(t, 1, r.ExitCode)
+	assert.Equal(t, "", r.Stdout)
+	assert.Equal(t, "Command error: normal error\n", r.Stderr)
+
+	app = newTestApp(func(ctx *cli.Context) error { return cli.Exit("exit error", 2) })
+	r, err = runTestApp(app, "./gitea", "test-cmd")
+	assert.Error(t, err)
+	assert.Equal(t, 2, r.ExitCode)
+	assert.Equal(t, "", r.Stdout)
+	assert.Equal(t, "exit error\n", r.Stderr)
+
+	app = newTestApp(func(ctx *cli.Context) error { return nil })
+	r, err = runTestApp(app, "./gitea", "test-cmd", "--no-such")
+	assert.Error(t, err)
+	assert.Equal(t, 1, r.ExitCode)
+	assert.Equal(t, "Incorrect Usage: flag provided but not defined: -no-such\n\n", r.Stdout)
+	assert.Equal(t, "", r.Stderr) // the cli package's strange behavior, the error message is not in stderr ....
+
+	app = newTestApp(func(ctx *cli.Context) error { return nil })
+	r, err = runTestApp(app, "./gitea", "test-cmd")
+	assert.NoError(t, err)
+	assert.Equal(t, -1, r.ExitCode) // the cli.OsExiter is not called
+	assert.Equal(t, "", r.Stdout)
+	assert.Equal(t, "", r.Stderr)
+}

main.go

@@ -5,7 +5,6 @@
 package main
 
 import (
-	"fmt"
 	"os"
 	"runtime"
 	"strings"
@@ -21,6 +20,8 @@ import (
 	_ "code.gitea.io/gitea/modules/markup/csv"
 	_ "code.gitea.io/gitea/modules/markup/markdown"
 	_ "code.gitea.io/gitea/modules/markup/orgmode"
+
+	"github.com/urfave/cli/v2"
 )
 
 // these flags will be set by the build flags
@@ -37,17 +38,12 @@ func init() {
 }
 
 func main() {
-	app := cmd.NewMainApp()
-	app.Name = "Gitea"
-	app.Usage = "A painless self-hosted Git service"
-	app.Description = `By default, Gitea will start serving using the web-server with no argument, which can alternatively be run by running the subcommand "web".`
-	app.Version = Version + formatBuiltWith()
-
-	err := app.Run(os.Args)
-	if err != nil {
-		_, _ = fmt.Fprintf(app.Writer, "\nFailed to run with %s: %v\n", os.Args, err)
+	cli.OsExiter = func(code int) {
+		log.GetManager().Close()
+		os.Exit(code)
 	}
-
+	app := cmd.NewMainApp(Version, formatBuiltWith())
+	_ = cmd.RunMainApp(app, os.Args...) // all errors should have been handled by the RunMainApp
 	log.GetManager().Close()
 }
 

modules/test/utils.go

@@ -33,3 +33,9 @@ func RedirectURL(resp http.ResponseWriter) string {
 func IsNormalPageCompleted(s string) bool {
 	return strings.Contains(s, `<footer class="page-footer"`) && strings.Contains(s, `</html>`)
 }
+
+func MockVariableValue[T any](p *T, v T) (reset func()) {
+	old := *p
+	*p = v
+	return func() { *p = old }
+}