From fa431b377d7055c6c1362787eebda5b8b67e8d58 Mon Sep 17 00:00:00 2001
From: wxiaoguang <wxiaoguang@gmail.com>
Date: Sat, 5 Aug 2023 23:37:04 +0800
Subject: [PATCH] Fix incorrect CLI exit code and duplicate error message
 (#26346) (#26347)

Backport #26346

Follow the CLI refactoring, and add tests.
---
 cmd/main.go           | 26 +++++++++++++++++
 cmd/main_test.go      | 68 +++++++++++++++++++++++++++++++++++++++++++
 main.go               |  8 ++---
 modules/test/utils.go |  6 ++++
 4 files changed, 104 insertions(+), 4 deletions(-)
 create mode 100644 cmd/main.go

diff --git a/cmd/main.go b/cmd/main.go
new file mode 100644
index 0000000000..0dfdb08bbb
--- /dev/null
+++ b/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
+}
diff --git a/cmd/main_test.go b/cmd/main_test.go
index 6e20be6945..a8628f9f21 100644
--- a/cmd/main_test.go
+++ b/cmd/main_test.go
@@ -4,9 +4,16 @@
 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) {
@@ -14,3 +21,64 @@ func TestMain(m *testing.M) {
 		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)
+}
diff --git a/main.go b/main.go
index 9b561376c3..8b48e8142e 100644
--- a/main.go
+++ b/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()
 }
 
diff --git a/modules/test/utils.go b/modules/test/utils.go
index 282895eaa9..7809882f5e 100644
--- a/modules/test/utils.go
+++ b/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 }
+}