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

Follow the CLI refactoring, and add tests.

2 files changed, 85 insertions, 16 deletions

diff --git a/cmd/main.go b/cmd/main.go
index 13f9bba013..feda41e68b 100644
--- a/cmd/main.go
+++ b/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
+}
diff --git a/cmd/main_test.go b/cmd/main_test.go
index e9204d09cc..0e32dce564 100644
--- a/cmd/main_test.go
+++ b/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)
+}