fix: address second round of PR review feedback

- Fix exit status matching: replace loose HasPrefix with isExitStatus
  helper that won't match "exit status 1" against "exit status 128"
- Fix opt.Args ordering in ShowRefVerify and DeleteTag: move before
  --end-of-options so flag args work correctly
- Fix gitRun log capture: capture partial stdout on error instead of
  logging empty output for failed commands
- Quote args with special characters in log output for readability
- Fix repo_pull_test: "bad_revision" causes exit 128, not exit 1

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: unknwon

command.go

@@ -12,6 +12,7 @@ import (
 	"io"
 	"os"
 	"os/exec"
+	"strconv"
 	"strings"
 	"time"
 
@@ -85,10 +86,9 @@ func gitRun(ctx context.Context, dir string, args []string, envs []string) ([]by
 	// line-by-line which corrupts binary-ish output.
 	stdout := new(bytes.Buffer)
 	err := cmd.StdOut().Run().Stream(stdout)
-	if err != nil {
-		return nil, mapContextError(err, ctx)
-	}
 
+	// Capture (partial) stdout for logging even on error, so failed commands
+	// produce a useful log entry rather than an empty one.
 	if logOutput != nil {
 		data := stdout.Bytes()
 		limit := len(data)
@@ -101,6 +101,9 @@ func gitRun(ctx context.Context, dir string, args []string, envs []string) ([]by
 		}
 	}
 
+	if err != nil {
+		return nil, mapContextError(err, ctx)
+	}
 	return stdout.Bytes(), nil
 }
 
@@ -152,7 +155,15 @@ func committerEnvs(committer *Signature) []string {
 func logf(dir string, args []string, output []byte) {
 	cmdStr := "git"
 	if len(args) > 0 {
-		cmdStr = "git " + strings.Join(args, " ")
+		quoted := make([]string, len(args))
+		for i, a := range args {
+			if strings.ContainsAny(a, " \t\n\"'\\<>") {
+				quoted[i] = strconv.Quote(a)
+			} else {
+				quoted[i] = a
+			}
+		}
+		cmdStr = "git " + strings.Join(quoted, " ")
 	}
 	if len(dir) == 0 {
 		log("%s\n%s", cmdStr, output)
@@ -204,6 +215,18 @@ func mapContextError(err error, ctx context.Context) error {
 	return err
 }
 
+// isExitStatus reports whether err represents a specific process exit status
+// code. It handles both the bare "exit status N" format and the
+// "exit status N: <stderr>" format produced by sourcegraph/run.
+func isExitStatus(err error, code int) bool {
+	if err == nil {
+		return false
+	}
+	prefix := fmt.Sprintf("exit status %d", code)
+	msg := err.Error()
+	return msg == prefix || strings.HasPrefix(msg, prefix+":")
+}
+
 // extractStderr attempts to extract the stderr portion from a sourcegraph/run
 // error. The error format is typically "exit status N: <stderr content>".
 func extractStderr(err error) string {

repo.go

@@ -393,7 +393,7 @@ func (r *Repository) Commit(ctx context.Context, committer *Signature, message s
 
 	_, err := gitRun(ctx, r.path, args, envs)
 	// No stderr but exit status 1 means nothing to commit.
-	if err != nil && strings.HasPrefix(err.Error(), "exit status 1") {
+	if isExitStatus(err, 1) {
 		return nil
 	}
 	return err
@@ -480,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.HasPrefix(err.Error(), "exit status 128") {
+		if isExitStatus(err, 128) {
 			return "", ErrRevisionNotExist
 		}
 		return "", err

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.HasPrefix(err.Error(), "exit status 1") {
+		if isExitStatus(err, 1) {
 			return "", ErrNoMergeBase
 		}
 		return "", err

repo_pull_test.go

@@ -14,9 +14,11 @@ import (
 func TestRepository_MergeBase(t *testing.T) {
 	ctx := context.Background()
 
-	t.Run("no merge base", func(t *testing.T) {
+	t.Run("bad revision", func(t *testing.T) {
+		// "bad_revision" doesn't exist, so git fails with exit status 128 (fatal),
+		// not exit status 1 (no merge base).
 		mb, err := testrepo.MergeBase(ctx, "0eedd79eba4394bbef888c804e899731644367fe", "bad_revision")
-		assert.Equal(t, ErrNoMergeBase, err)
+		assert.Error(t, err)
 		assert.Empty(t, mb)
 	})
 

repo_reference.go

@@ -51,8 +51,9 @@ func (r *Repository) ShowRefVerify(ctx context.Context, ref string, opts ...Show
 		opt = opts[0]
 	}
 
-	args := []string{"show-ref", "--verify", "--end-of-options", ref}
+	args := []string{"show-ref", "--verify"}
 	args = append(args, opt.Args...)
+	args = append(args, "--end-of-options", ref)
 
 	stdout, err := gitRun(ctx, r.path, args, opt.Envs)
 	if err != nil {

repo_remote.go

@@ -136,7 +136,6 @@ func (r *Repository) RemoteRemove(ctx context.Context, name string, opts ...Remo
 
 	_, err := gitRun(ctx, r.path, args, opt.Envs)
 	if err != nil {
-		// the error status may differ from git clients
 		if strings.Contains(err.Error(), "error: No such remote") ||
 			strings.Contains(err.Error(), "fatal: No such remote") {
 			return ErrRemoteNotExist

repo_tag.go

@@ -234,8 +234,9 @@ func (r *Repository) DeleteTag(ctx context.Context, name string, opts ...DeleteT
 		opt = opts[0]
 	}
 
-	args := []string{"tag", "--delete", "--end-of-options", name}
+	args := []string{"tag", "--delete"}
 	args = append(args, opt.Args...)
+	args = append(args, "--end-of-options", name)
 
 	_, err := gitRun(ctx, r.path, args, opt.Envs)
 	return err