fix: address PR review feedback on run migration

Fix timeout context cancel leakage, tighten exit-status matching, remove fragile
stderr re-concatenation paths, and harden author/diff argument handling.

Author: unknwon

blob.go

@@ -19,15 +19,14 @@ type Blob struct {
 // can be very slow and memory consuming for huge content.
 func (b *Blob) Bytes(ctx context.Context) ([]byte, error) {
 	stdout := new(bytes.Buffer)
-	stderr := new(bytes.Buffer)
 
 	// Preallocate memory to save ~50% memory usage on big files.
 	if size := b.Size(ctx); size > 0 && size < int64(^uint(0)>>1) {
 		stdout.Grow(int(size))
 	}
 
-	if err := b.Pipeline(ctx, stdout, stderr); err != nil {
-		return nil, concatenateError(err, stderr.String())
+	if err := b.Pipeline(ctx, stdout, nil); err != nil {
+		return nil, err
 	}
 	return stdout.Bytes(), nil
 }

command.go

@@ -7,9 +7,11 @@ package git
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"os"
+	"os/exec"
 	"strings"
 	"time"
 
@@ -29,16 +31,17 @@ const DefaultTimeout = time.Minute
 // gitCmd builds a *run.Command for "git" with the given arguments, environment
 // variables and working directory. If the context does not already have a
 // deadline, DefaultTimeout will be applied automatically.
-func gitCmd(ctx context.Context, dir string, args []string, envs []string) *run.Command {
+func gitCmd(ctx context.Context, dir string, args []string, envs []string) (*run.Command, context.CancelFunc) {
 	if ctx == nil {
 		ctx = context.Background()
 	}
+	cancel := func() {}
 
 	// Apply default timeout if the context doesn't already have a deadline.
 	if _, ok := ctx.Deadline(); !ok {
-		var cancel context.CancelFunc
-		ctx, cancel = context.WithTimeout(ctx, DefaultTimeout)
-		_ = cancel
+		var timeoutCancel context.CancelFunc
+		ctx, timeoutCancel = context.WithTimeout(ctx, DefaultTimeout)
+		cancel = timeoutCancel
 	}
 
 	// run.Cmd joins all parts into a single string and then shell-parses it.
@@ -57,25 +60,26 @@ func gitCmd(ctx context.Context, dir string, args []string, envs []string) *run.
 	if len(envs) > 0 {
 		cmd = cmd.Environ(append(os.Environ(), envs...))
 	}
-	return cmd
+	return cmd, cancel
 }
 
 // gitRun executes a git command in the given directory and returns stdout as
 // bytes. Stderr is included in the error message on failure. If the command's
 // context does not have a deadline, DefaultTimeout will be applied
 // automatically. It returns an ErrExecTimeout if the execution was timed out.
 func gitRun(ctx context.Context, dir string, args []string, envs []string) ([]byte, error) {
-	cmd := gitCmd(ctx, dir, args, envs)
+	cmd, cancel := gitCmd(ctx, dir, args, envs)
+	defer cancel()
 
-	logBuf := new(bytes.Buffer)
+	var logBuf *bytes.Buffer
 	if logOutput != nil {
+		logBuf = new(bytes.Buffer)
 		logBuf.Grow(512)
+		defer func() {
+			logf(dir, args, logBuf.Bytes())
+		}()
 	}
 
-	defer func() {
-		logf(dir, args, logBuf.Bytes())
-	}()
-
 	// Use Stream to a buffer to preserve raw bytes (including NUL bytes from
 	// commands like "ls-tree -z"). The String/Lines methods process output
 	// line-by-line which corrupts binary-ish output.
@@ -104,25 +108,27 @@ func gitRun(ctx context.Context, dir string, args []string, envs []string) ([]by
 // to the given writer. If stderr writer is provided and the command fails,
 // stderr content extracted from the error is written to it. stdin is optional.
 func gitPipeline(ctx context.Context, dir string, args []string, envs []string, stdout, stderr io.Writer, stdin io.Reader) error {
-	cmd := gitCmd(ctx, dir, args, envs)
+	cmd, cancel := gitCmd(ctx, dir, args, envs)
+	defer cancel()
 	if stdin != nil {
 		cmd = cmd.Input(stdin)
 	}
 
-	buf := new(bytes.Buffer)
+	var buf *bytes.Buffer
 	w := stdout
 	if logOutput != nil {
+		buf = new(bytes.Buffer)
 		buf.Grow(512)
 		w = &limitDualWriter{
 			W: buf,
 			N: int64(buf.Cap()),
 			w: stdout,
 		}
-	}
 
-	defer func() {
-		logf(dir, args, buf.Bytes())
-	}()
+		defer func() {
+			logf(dir, args, buf.Bytes())
+		}()
+	}
 
 	streamErr := cmd.StdOut().Run().Stream(w)
 	if streamErr != nil {
@@ -195,12 +201,6 @@ func mapContextError(err error, ctx context.Context) error {
 		}
 		return ctxErr
 	}
-	// Also check if the error itself wraps a context error.
-	if strings.Contains(err.Error(), "signal: killed") || strings.Contains(err.Error(), context.DeadlineExceeded.Error()) {
-		if ctx.Err() == context.DeadlineExceeded {
-			return ErrExecTimeout
-		}
-	}
 	return err
 }
 
@@ -210,6 +210,10 @@ func extractStderr(err error) string {
 	if err == nil {
 		return ""
 	}
+	var exitErr *exec.ExitError
+	if errors.As(err, &exitErr) && len(exitErr.Stderr) > 0 {
+		return string(exitErr.Stderr)
+	}
 	msg := err.Error()
 	// sourcegraph/run error format: "exit status N: <stderr>"
 	if idx := strings.Index(msg, ": "); idx >= 0 && strings.HasPrefix(msg, "exit status") {

repo.go

@@ -387,13 +387,13 @@ func (r *Repository) Commit(ctx context.Context, committer *Signature, message s
 	}
 
 	args := []string{"commit"}
-	args = append(args, fmt.Sprintf("--author='%s <%s>'", opt.Author.Name, opt.Author.Email))
+	args = append(args, fmt.Sprintf("--author=%s <%s>", opt.Author.Name, opt.Author.Email))
 	args = append(args, "-m", message)
 	args = append(args, opt.Args...)
 
 	_, err := gitRun(ctx, r.path, args, envs)
 	// No stderr but exit status 1 means nothing to commit.
-	if err != nil && err.Error() == "exit status 1" {
+	if err != nil && strings.HasPrefix(err.Error(), "exit status 1") {
 		return nil
 	}
 	return err
@@ -448,11 +448,10 @@ func (r *Repository) ShowNameStatus(ctx context.Context, rev string, opts ...Sho
 	args = append(args, opt.Args...)
 	args = append(args, "--end-of-options", rev)
 
-	stderr := new(bytes.Buffer)
-	err := gitPipeline(ctx, r.path, args, opt.Envs, w, stderr, nil)
+	err := gitPipeline(ctx, r.path, args, opt.Envs, w, nil, nil)
 	_ = w.Close() // Close writer to exit parsing goroutine
 	if err != nil {
-		return nil, concatenateError(err, stderr.String())
+		return nil, err
 	}
 
 	<-done
@@ -481,7 +480,7 @@ func (r *Repository) RevParse(ctx context.Context, rev string, opts ...RevParseO
 
 	commitID, err := gitRun(ctx, r.path, args, opt.Envs)
 	if err != nil {
-		if strings.Contains(err.Error(), "exit status 128") {
+		if strings.HasPrefix(err.Error(), "exit status 128") {
 			return "", ErrRevisionNotExist
 		}
 		return "", err

repo_diff.go

@@ -5,7 +5,6 @@
 package git
 
 import (
-	"bytes"
 	"context"
 	"fmt"
 	"io"
@@ -60,11 +59,10 @@ func (r *Repository) Diff(ctx context.Context, rev string, maxFiles, maxFileLine
 	done := make(chan SteamParseDiffResult)
 	go StreamParseDiff(stdout, done, maxFiles, maxFileLines, maxLineChars)
 
-	stderr := new(bytes.Buffer)
-	err = gitPipeline(ctx, r.path, args, opt.Envs, w, stderr, nil)
+	err = gitPipeline(ctx, r.path, args, opt.Envs, w, nil, nil)
 	_ = w.Close() // Close writer to exit parsing goroutine
 	if err != nil {
-		return nil, concatenateError(err, stderr.String())
+		return nil, err
 	}
 
 	result := <-done
@@ -134,9 +132,8 @@ func (r *Repository) RawDiff(ctx context.Context, rev string, diffType RawDiffFo
 		return fmt.Errorf("invalid diffType: %s", diffType)
 	}
 
-	stderr := new(bytes.Buffer)
-	if err = gitPipeline(ctx, r.path, args, opt.Envs, w, stderr, nil); err != nil {
-		return concatenateError(err, stderr.String())
+	if err = gitPipeline(ctx, r.path, args, opt.Envs, w, nil, nil); err != nil {
+		return err
 	}
 	return nil
 }
@@ -157,7 +154,7 @@ func (r *Repository) DiffBinary(ctx context.Context, base, head string, opts ...
 
 	args := []string{"diff"}
 	args = append(args, opt.Args...)
-	args = append(args, "--full-index", "--binary", base, head)
+	args = append(args, "--full-index", "--binary", "--end-of-options", base, head)
 
 	return gitRun(ctx, r.path, args, opt.Envs)
 }

repo_pull.go

@@ -31,7 +31,7 @@ func (r *Repository) MergeBase(ctx context.Context, base, head string, opts ...M
 
 	stdout, err := gitRun(ctx, r.path, args, opt.Envs)
 	if err != nil {
-		if strings.Contains(err.Error(), "exit status 1") {
+		if strings.HasPrefix(err.Error(), "exit status 1") {
 			return "", ErrNoMergeBase
 		}
 		return "", err

utils.go

@@ -5,7 +5,6 @@
 package git
 
 import (
-	"fmt"
 	"os"
 	"strings"
 	"sync"
@@ -66,13 +65,6 @@ func isExist(path string) bool {
 	return err == nil || os.IsExist(err)
 }
 
-func concatenateError(err error, stderr string) error {
-	if len(stderr) == 0 {
-		return err
-	}
-	return fmt.Errorf("%v - %s", err, stderr)
-}
-
 // bytesToStrings splits given bytes into strings by line separator ("\n"). It
 // returns empty slice if the given bytes only contains line separators.
 func bytesToStrings(in []byte) []string {