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

cmd/main.go

@@ -0,0 +1,26 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package cmd
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/urfave/cli"
+)
+
+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

@@ -4,13 +4,81 @@
 package cmd
 
 import (
+	"fmt"
+	"io"
+	"strings"
 	"testing"
 
 	"code.gitea.io/gitea/models/unittest"
+	"code.gitea.io/gitea/modules/test"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/urfave/cli"
 )
 
 func TestMain(m *testing.M) {
 	unittest.MainTest(m, &unittest.TestOptions{
 		GiteaRootPath: "..",
 	})
 }
+
+func newTestApp(testCmdAction func(ctx *cli.Context) error) *cli.App {
+	app := cli.NewApp()
+	app.HelpName = "gitea"
+	testCmd := cli.Command{Name: "test-cmd", Action: testCmdAction}
+	app.Commands = append(app.Commands, testCmd)
+	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 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.NewExitError("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.EqualValues(t, "Incorrect Usage: flag provided but not defined: -no-such\n\nNAME:\n   gitea test-cmd - \n\nUSAGE:\n   gitea test-cmd [arguments...]\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

@@ -146,11 +146,11 @@ func main() {
 	app.Commands = append(app.Commands, subCmdWithIni...)
 	app.Commands = append(app.Commands, subCmdStandalone...)
 
-	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)
 	}
-
+	_ = cmd.RunMainApp(app, os.Args...) // all errors should have been handled by the RunMainApp
 	log.GetManager().Close()
 }
 

modules/test/utils.go

@@ -16,3 +16,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 }
+}