Refactor git.Command.Run*, introduce RunWithContextString and RunWithContextBytes (#19266)

This follows 
* https://github.com/go-gitea/gitea/issues/18553

Introduce `RunWithContextString` and `RunWithContextBytes` to help the refactoring. Add related unit tests. They keep the same behavior to save stderr into err.Error() as `RunInXxx` before.

Remove `RunInDirTimeoutPipeline` `RunInDirTimeoutFullPipeline` `RunInDirTimeout` `RunInDirTimeoutEnv`  `RunInDirPipeline`  `RunInDirFullPipeline`  `RunTimeout`, `RunInDirTimeoutEnvPipeline`, `RunInDirTimeoutEnvFullPipeline`, `RunInDirTimeoutEnvFullPipelineFunc`.

Then remaining `RunInDir` `RunInDirBytes` `RunInDirWithEnv` can be easily refactored in next PR with a simple search & replace:
* before: `stdout, err := RunInDir(path)`
* next: `stdout, _, err := RunWithContextString(&git.RunContext{Dir:path})`

Other changes:
1. When `timeout <= 0`, use default. Because `timeout==0` is meaningless and could cause bugs. And now many functions becomes more simple, eg: `GitGcRepos` 9 lines to 1 line. `Fsck` 6 lines to 1 line.
2. Only set defaultCommandExecutionTimeout when the option `setting.Git.Timeout.Default > 0`

modules/git/command.go

@@ -14,6 +14,7 @@ import (
 	"os/exec"
 	"strings"
 	"time"
+	"unsafe"
 
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/process"
@@ -93,32 +94,6 @@ func (c *Command) AddArguments(args ...string) *Command {
 	return c
 }
 
-// RunInDirTimeoutEnvPipeline executes the command in given directory with given timeout,
-// it pipes stdout and stderr to given io.Writer.
-func (c *Command) RunInDirTimeoutEnvPipeline(env []string, timeout time.Duration, dir string, stdout, stderr io.Writer) error {
-	return c.RunInDirTimeoutEnvFullPipeline(env, timeout, dir, stdout, stderr, nil)
-}
-
-// RunInDirTimeoutEnvFullPipeline executes the command in given directory with given timeout,
-// it pipes stdout and stderr to given io.Writer and passes in an io.Reader as stdin.
-func (c *Command) RunInDirTimeoutEnvFullPipeline(env []string, timeout time.Duration, dir string, stdout, stderr io.Writer, stdin io.Reader) error {
-	return c.RunInDirTimeoutEnvFullPipelineFunc(env, timeout, dir, stdout, stderr, stdin, nil)
-}
-
-// RunInDirTimeoutEnvFullPipelineFunc executes the command in given directory with given timeout,
-// it pipes stdout and stderr to given io.Writer and passes in an io.Reader as stdin. Between cmd.Start and cmd.Wait the passed in function is run.
-func (c *Command) RunInDirTimeoutEnvFullPipelineFunc(env []string, timeout time.Duration, dir string, stdout, stderr io.Writer, stdin io.Reader, fn func(context.Context, context.CancelFunc) error) error {
-	return c.RunWithContext(&RunContext{
-		Env:          env,
-		Timeout:      timeout,
-		Dir:          dir,
-		Stdout:       stdout,
-		Stderr:       stderr,
-		Stdin:        stdin,
-		PipelineFunc: fn,
-	})
-}
-
 // RunContext represents parameters to run the command
 type RunContext struct {
 	Env            []string
@@ -131,7 +106,7 @@ type RunContext struct {
 
 // RunWithContext run the command with context
 func (c *Command) RunWithContext(rc *RunContext) error {
-	if rc.Timeout == -1 {
+	if rc.Timeout <= 0 {
 		rc.Timeout = defaultCommandExecutionTimeout
 	}
 
@@ -203,58 +178,73 @@ func (c *Command) RunWithContext(rc *RunContext) error {
 	return ctx.Err()
 }
 
-// RunInDirTimeoutPipeline executes the command in given directory with given timeout,
-// it pipes stdout and stderr to given io.Writer.
-func (c *Command) RunInDirTimeoutPipeline(timeout time.Duration, dir string, stdout, stderr io.Writer) error {
-	return c.RunInDirTimeoutEnvPipeline(nil, timeout, dir, stdout, stderr)
+type RunError interface {
+	error
+	Stderr() string
 }
 
-// RunInDirTimeoutFullPipeline executes the command in given directory with given timeout,
-// it pipes stdout and stderr to given io.Writer, and stdin from the given io.Reader
-func (c *Command) RunInDirTimeoutFullPipeline(timeout time.Duration, dir string, stdout, stderr io.Writer, stdin io.Reader) error {
-	return c.RunInDirTimeoutEnvFullPipeline(nil, timeout, dir, stdout, stderr, stdin)
+type runError struct {
+	err    error
+	stderr string
+	errMsg string
 }
 
-// RunInDirTimeout executes the command in given directory with given timeout,
-// and returns stdout in []byte and error (combined with stderr).
-func (c *Command) RunInDirTimeout(timeout time.Duration, dir string) ([]byte, error) {
-	return c.RunInDirTimeoutEnv(nil, timeout, dir)
-}
-
-// RunInDirTimeoutEnv executes the command in given directory with given timeout,
-// and returns stdout in []byte and error (combined with stderr).
-func (c *Command) RunInDirTimeoutEnv(env []string, timeout time.Duration, dir string) ([]byte, error) {
-	stdout := new(bytes.Buffer)
-	stderr := new(bytes.Buffer)
-	if err := c.RunInDirTimeoutEnvPipeline(env, timeout, dir, stdout, stderr); err != nil {
-		return nil, ConcatenateError(err, stderr.String())
+func (r *runError) Error() string {
+	// the stderr must be in the returned error text, some code only checks `strings.Contains(err.Error(), "git error")`
+	if r.errMsg == "" {
+		r.errMsg = ConcatenateError(r.err, r.stderr).Error()
 	}
-	if stdout.Len() > 0 && log.IsTrace() {
-		tracelen := stdout.Len()
-		if tracelen > 1024 {
-			tracelen = 1024
-		}
-		log.Trace("Stdout:\n %s", stdout.Bytes()[:tracelen])
-	}
-	return stdout.Bytes(), nil
+	return r.errMsg
 }
 
-// RunInDirPipeline executes the command in given directory,
-// it pipes stdout and stderr to given io.Writer.
-func (c *Command) RunInDirPipeline(dir string, stdout, stderr io.Writer) error {
-	return c.RunInDirFullPipeline(dir, stdout, stderr, nil)
+func (r *runError) Unwrap() error {
+	return r.err
 }
 
-// RunInDirFullPipeline executes the command in given directory,
-// it pipes stdout and stderr to given io.Writer.
-func (c *Command) RunInDirFullPipeline(dir string, stdout, stderr io.Writer, stdin io.Reader) error {
-	return c.RunInDirTimeoutFullPipeline(-1, dir, stdout, stderr, stdin)
+func (r *runError) Stderr() string {
+	return r.stderr
+}
+
+func bytesToString(b []byte) string {
+	return *(*string)(unsafe.Pointer(&b)) // that's what Golang's strings.Builder.String() does (go/src/strings/builder.go)
+}
+
+// RunWithContextString run the command with context and returns stdout/stderr as string. and store stderr to returned error (err combined with stderr).
+func (c *Command) RunWithContextString(rc *RunContext) (stdout, stderr string, runErr RunError) {
+	stdoutBytes, stderrBytes, err := c.RunWithContextBytes(rc)
+	stdout = bytesToString(stdoutBytes)
+	stderr = bytesToString(stderrBytes)
+	if err != nil {
+		return stdout, stderr, &runError{err: err, stderr: stderr}
+	}
+	// even if there is no err, there could still be some stderr output, so we just return stdout/stderr as they are
+	return stdout, stderr, nil
+}
+
+// RunWithContextBytes run the command with context and returns stdout/stderr as bytes. and store stderr to returned error (err combined with stderr).
+func (c *Command) RunWithContextBytes(rc *RunContext) (stdout, stderr []byte, runErr RunError) {
+	if rc.Stdout != nil || rc.Stderr != nil {
+		// we must panic here, otherwise there would be bugs if developers set Stdin/Stderr by mistake, and it would be very difficult to debug
+		panic("stdout and stderr field must be nil when using RunWithContextBytes")
+	}
+	stdoutBuf := &bytes.Buffer{}
+	stderrBuf := &bytes.Buffer{}
+	rc.Stdout = stdoutBuf
+	rc.Stderr = stderrBuf
+	err := c.RunWithContext(rc)
+	stderr = stderrBuf.Bytes()
+	if err != nil {
+		return nil, stderr, &runError{err: err, stderr: string(stderr)}
+	}
+	// even if there is no err, there could still be some stderr output
+	return stdoutBuf.Bytes(), stderr, nil
 }
 
 // RunInDirBytes executes the command in given directory
 // and returns stdout in []byte and error (combined with stderr).
 func (c *Command) RunInDirBytes(dir string) ([]byte, error) {
-	return c.RunInDirTimeout(-1, dir)
+	stdout, _, err := c.RunWithContextBytes(&RunContext{Dir: dir})
+	return stdout, err
 }
 
 // RunInDir executes the command in given directory
@@ -266,27 +256,15 @@ func (c *Command) RunInDir(dir string) (string, error) {
 // RunInDirWithEnv executes the command in given directory
 // and returns stdout in string and error (combined with stderr).
 func (c *Command) RunInDirWithEnv(dir string, env []string) (string, error) {
-	stdout, err := c.RunInDirTimeoutEnv(env, -1, dir)
-	if err != nil {
-		return "", err
-	}
-	return string(stdout), nil
-}
-
-// RunTimeout executes the command in default working directory with given timeout,
-// and returns stdout in string and error (combined with stderr).
-func (c *Command) RunTimeout(timeout time.Duration) (string, error) {
-	stdout, err := c.RunInDirTimeout(timeout, "")
-	if err != nil {
-		return "", err
-	}
-	return string(stdout), nil
+	stdout, _, err := c.RunWithContextString(&RunContext{Env: env, Dir: dir})
+	return stdout, err
 }
 
 // Run executes the command in default working directory
 // and returns stdout in string and error (combined with stderr).
 func (c *Command) Run() (string, error) {
-	return c.RunTimeout(-1)
+	stdout, _, err := c.RunWithContextString(&RunContext{})
+	return stdout, err
 }
 
 // AllowLFSFiltersArgs return globalCommandArgs with lfs filter, it should only be used for tests

modules/git/command_race_test.go

@@ -0,0 +1,40 @@
+// Copyright 2017 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+//go:build race
+// +build race
+
+package git
+
+import (
+	"context"
+	"testing"
+	"time"
+)
+
+func TestRunWithContextNoTimeout(t *testing.T) {
+	maxLoops := 10
+
+	// 'git --version' does not block so it must be finished before the timeout triggered.
+	cmd := NewCommand(context.Background(), "--version")
+	for i := 0; i < maxLoops; i++ {
+		if err := cmd.RunWithContext(&RunContext{}); err != nil {
+			t.Fatal(err)
+		}
+	}
+}
+
+func TestRunWithContextTimeout(t *testing.T) {
+	maxLoops := 10
+
+	// 'git hash-object --stdin' blocks on stdin so we can have the timeout triggered.
+	cmd := NewCommand(context.Background(), "hash-object", "--stdin")
+	for i := 0; i < maxLoops; i++ {
+		if err := cmd.RunWithContext(&RunContext{Timeout: 1 * time.Millisecond}); err != nil {
+			if err != context.DeadlineExceeded {
+				t.Fatalf("Testing %d/%d: %v", i, maxLoops, err)
+			}
+		}
+	}
+}

modules/git/command_test.go

@@ -1,40 +1,29 @@
-// Copyright 2017 The Gitea Authors. All rights reserved.
+// Copyright 2022 The Gitea Authors. All rights reserved.
 // Use of this source code is governed by a MIT-style
 // license that can be found in the LICENSE file.
 
-//go:build race
-// +build race
-
 package git
 
 import (
 	"context"
 	"testing"
-	"time"
+
+	"github.com/stretchr/testify/assert"
 )
 
-func TestRunInDirTimeoutPipelineNoTimeout(t *testing.T) {
-	maxLoops := 1000
-
-	// 'git --version' does not block so it must be finished before the timeout triggered.
+func TestRunWithContextStd(t *testing.T) {
 	cmd := NewCommand(context.Background(), "--version")
-	for i := 0; i < maxLoops; i++ {
-		if err := cmd.RunInDirTimeoutPipeline(-1, "", nil, nil); err != nil {
-			t.Fatal(err)
-		}
-	}
-}
-
-func TestRunInDirTimeoutPipelineAlwaysTimeout(t *testing.T) {
-	maxLoops := 1000
-
-	// 'git hash-object --stdin' blocks on stdin so we can have the timeout triggered.
-	cmd := NewCommand(context.Background(), "hash-object", "--stdin")
-	for i := 0; i < maxLoops; i++ {
-		if err := cmd.RunInDirTimeoutPipeline(1*time.Microsecond, "", nil, nil); err != nil {
-			if err != context.DeadlineExceeded {
-				t.Fatalf("Testing %d/%d: %v", i, maxLoops, err)
-			}
-		}
+	stdout, stderr, err := cmd.RunWithContextString(&RunContext{})
+	assert.NoError(t, err)
+	assert.Empty(t, stderr)
+	assert.Contains(t, stdout, "git version")
+
+	cmd = NewCommand(context.Background(), "--no-such-arg")
+	stdout, stderr, err = cmd.RunWithContextString(&RunContext{})
+	if assert.Error(t, err) {
+		assert.Equal(t, stderr, err.Stderr())
+		assert.Contains(t, err.Stderr(), "unknown option:")
+		assert.Contains(t, err.Error(), "exit status 129 - unknown option:")
+		assert.Empty(t, stdout)
 	}
 }

modules/git/git.go

@@ -124,7 +124,9 @@ func VersionInfo() string {
 func Init(ctx context.Context) error {
 	DefaultContext = ctx
 
+	if setting.Git.Timeout.Default > 0 {
 		defaultCommandExecutionTimeout = time.Duration(setting.Git.Timeout.Default) * time.Second
+	}
 
 	if err := SetExecutablePath(setting.Git.Path); err != nil {
 		return err
@@ -295,10 +297,5 @@ func checkAndRemoveConfig(key, value string) error {
 
 // Fsck verifies the connectivity and validity of the objects in the database
 func Fsck(ctx context.Context, repoPath string, timeout time.Duration, args ...string) error {
-	// Make sure timeout makes sense.
-	if timeout <= 0 {
-		timeout = -1
-	}
-	_, err := NewCommand(ctx, "fsck").AddArguments(args...).RunInDirTimeout(timeout, repoPath)
-	return err
+	return NewCommand(ctx, "fsck").AddArguments(args...).RunWithContext(&RunContext{Timeout: timeout, Dir: repoPath})
 }

modules/process/manager.go

@@ -86,6 +86,10 @@ func (pm *Manager) AddContext(parent context.Context, description string) (ctx c
 // Most processes will not need to use the cancel function but there will be cases whereby you want to cancel the process but not immediately remove it from the
 // process table.
 func (pm *Manager) AddContextTimeout(parent context.Context, timeout time.Duration, description string) (ctx context.Context, cancel context.CancelFunc, finshed FinishedFunc) {
+	if timeout <= 0 {
+		// it's meaningless to use timeout <= 0, and it must be a bug! so we must panic here to tell developers to make the timeout correct
+		panic("the timeout must be greater than zero, otherwise the context will be cancelled immediately")
+	}
 	ctx, cancel = context.WithTimeout(parent, timeout)
 
 	ctx, pid, finshed := pm.Add(ctx, description, cancel)
@@ -239,7 +243,7 @@ func (pm *Manager) ExecDirEnv(ctx context.Context, timeout time.Duration, dir, d
 // Returns its complete stdout and stderr
 // outputs and an error, if any (including timeout)
 func (pm *Manager) ExecDirEnvStdIn(ctx context.Context, timeout time.Duration, dir, desc string, env []string, stdIn io.Reader, cmdName string, args ...string) (string, string, error) {
-	if timeout == -1 {
+	if timeout <= 0 {
 		timeout = 60 * time.Second
 	}
 

routers/web/repo/http.go

@@ -542,7 +542,7 @@ func GetInfoRefs(ctx *context.Context) {
 		}
 		h.environ = append(os.Environ(), h.environ...)
 
-		refs, err := git.NewCommand(ctx, service, "--stateless-rpc", "--advertise-refs", ".").RunInDirTimeoutEnv(h.environ, -1, h.dir)
+		refs, _, err := git.NewCommand(ctx, service, "--stateless-rpc", "--advertise-refs", ".").RunWithContextBytes(&git.RunContext{Env: h.environ, Dir: h.dir})
 		if err != nil {
 			log.Error(fmt.Sprintf("%v - %s", err, string(refs)))
 		}

services/repository/check.go

@@ -77,15 +77,7 @@ func GitGcRepos(ctx context.Context, timeout time.Duration, args ...string) erro
 				SetDescription(fmt.Sprintf("Repository Garbage Collection: %s", repo.FullName()))
 			var stdout string
 			var err error
-			if timeout > 0 {
-				var stdoutBytes []byte
-				stdoutBytes, err = command.RunInDirTimeout(
-					timeout,
-					repo.RepoPath())
-				stdout = string(stdoutBytes)
-			} else {
-				stdout, err = command.RunInDir(repo.RepoPath())
-			}
+			stdout, _, err = command.RunWithContextString(&git.RunContext{Timeout: timeout, Dir: repo.RepoPath()})
 
 			if err != nil {
 				log.Error("Repository garbage collection failed for %v. Stdout: %s\nError: %v", repo, stdout, err)

services/repository/fork.go

@@ -108,10 +108,10 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork
 		needsRollback = true
 
 		repoPath := repo_model.RepoPath(owner.Name, repo.Name)
-		if stdout, err := git.NewCommand(txCtx,
+		if stdout, _, err := git.NewCommand(txCtx,
 			"clone", "--bare", oldRepoPath, repoPath).
 			SetDescription(fmt.Sprintf("ForkRepository(git clone): %s to %s", opts.BaseRepo.FullName(), repo.FullName())).
-			RunInDirTimeout(10*time.Minute, ""); err != nil {
+			RunWithContextBytes(&git.RunContext{Timeout: 10 * time.Minute}); err != nil {
 			log.Error("Fork Repository (git clone) Failed for %v (from %v):\nStdout: %s\nError: %v", repo, opts.BaseRepo, stdout, err)
 			return fmt.Errorf("git clone: %v", err)
 		}