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

Follow the CLI refactoring, and add tests.

diff --git a/cmd/main.go b/cmd/main.go
index 13f9bba0139f81c54ee198012536a411a3c3f2d0..feda41e68b24a2ed9f6f3155d34053c01835011d 100644 (file)
--- 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 e9204d09cce3ad0827251b5ff0557125d0f7a371..0e32dce564872f45feab056e33a17caea4945983 100644 (file)
--- 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)
+}

diff --git a/main.go b/main.go
index 652a71195e58d17e19a876ebaee962f8d2200390..775c729c569ea5c1c8c2426bc8dc8ddddec72e9b 100644 (file)
--- a/main.go
+++ b/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()
 }
 

diff --git a/modules/test/utils.go b/modules/test/utils.go
index 2917741c45a69d49251014eea850e71d40b80dc5..4a0c2f1b3b298e051986c59be2c5d7d952bd8c96 100644 (file)
--- a/modules/test/utils.go
+++ b/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 }
+}