Skip to content

Commit b7f7b6f

Browse files
unknwonclaude
andcommitted
fix: address third round of PR review feedback
- Remove nil check on ctx in gitCmd (callers always provide non-nil ctx) - Remove stdin parameter from gitPipeline (no production caller uses it) - Rewrite isExitStatus to use run.ExitCoder interface instead of string parsing, as suggested by reviewer - Remove unused errors and os/exec imports - Update tests to use gitCmd directly for stdin-based scenarios Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 762afdf commit b7f7b6f

File tree

5 files changed

+22
-31
lines changed

5 files changed

+22
-31
lines changed

blob.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,5 +34,5 @@ func (b *Blob) Bytes(ctx context.Context) ([]byte, error) {
3434
// Pipeline reads the content of the blob and pipes stdout and stderr to
3535
// supplied io.Writer.
3636
func (b *Blob) Pipeline(ctx context.Context, stdout, stderr io.Writer) error {
37-
return gitPipeline(ctx, b.parent.repo.path, []string{"show", b.id.String()}, nil, stdout, stderr, nil)
37+
return gitPipeline(ctx, b.parent.repo.path, []string{"show", b.id.String()}, nil, stdout, stderr)
3838
}

command.go

Lines changed: 5 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,9 @@ package git
77
import (
88
"bytes"
99
"context"
10-
"errors"
1110
"fmt"
1211
"io"
1312
"os"
14-
"os/exec"
1513
"strconv"
1614
"strings"
1715
"time"
@@ -33,9 +31,6 @@ const DefaultTimeout = time.Minute
3331
// variables and working directory. If the context does not already have a
3432
// deadline, DefaultTimeout will be applied automatically.
3533
func gitCmd(ctx context.Context, dir string, args []string, envs []string) (*run.Command, context.CancelFunc) {
36-
if ctx == nil {
37-
ctx = context.Background()
38-
}
3934
cancel := func() {}
4035

4136
// Apply default timeout if the context doesn't already have a deadline.
@@ -109,13 +104,10 @@ func gitRun(ctx context.Context, dir string, args []string, envs []string) ([]by
109104

110105
// gitPipeline executes a git command in the given directory, streaming stdout
111106
// to the given writer. If stderr writer is provided and the command fails,
112-
// stderr content extracted from the error is written to it. stdin is optional.
113-
func gitPipeline(ctx context.Context, dir string, args []string, envs []string, stdout, stderr io.Writer, stdin io.Reader) error {
107+
// stderr content extracted from the error is written to it.
108+
func gitPipeline(ctx context.Context, dir string, args []string, envs []string, stdout, stderr io.Writer) error {
114109
cmd, cancel := gitCmd(ctx, dir, args, envs)
115110
defer cancel()
116-
if stdin != nil {
117-
cmd = cmd.Input(stdin)
118-
}
119111

120112
var buf *bytes.Buffer
121113
w := stdout
@@ -216,15 +208,10 @@ func mapContextError(err error, ctx context.Context) error {
216208
}
217209

218210
// isExitStatus reports whether err represents a specific process exit status
219-
// code. It handles both the bare "exit status N" format and the
220-
// "exit status N: <stderr>" format produced by sourcegraph/run.
211+
// code, using the run.ExitCoder interface provided by sourcegraph/run.
221212
func isExitStatus(err error, code int) bool {
222-
if err == nil {
223-
return false
224-
}
225-
prefix := fmt.Sprintf("exit status %d", code)
226-
msg := err.Error()
227-
return msg == prefix || strings.HasPrefix(msg, prefix+":")
213+
exitCoder, ok := err.(run.ExitCoder)
214+
return ok && exitCoder.ExitCode() == code
228215
}
229216

230217
// extractStderr attempts to extract the stderr portion from a sourcegraph/run
@@ -233,10 +220,6 @@ func extractStderr(err error) string {
233220
if err == nil {
234221
return ""
235222
}
236-
var exitErr *exec.ExitError
237-
if errors.As(err, &exitErr) && len(exitErr.Stderr) > 0 {
238-
return string(exitErr.Stderr)
239-
}
240223
msg := err.Error()
241224
// sourcegraph/run error format: "exit status N: <stderr>"
242225
if idx := strings.Index(msg, ": "); idx >= 0 && strings.HasPrefix(msg, "exit status") {

command_test.go

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,13 @@ func TestGitRun_ContextTimeout(t *testing.T) {
2626
ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)
2727
defer cancel()
2828

29-
// Use a blocking reader so the command starts successfully and blocks
30-
// reading stdin until the context deadline fires.
31-
err := gitPipeline(ctx, "", []string{"hash-object", "--stdin"}, nil, io.Discard, io.Discard, blockingReader{cancel: ctx.Done()})
29+
// Use gitCmd directly with a blocking stdin so the command starts
30+
// successfully and blocks reading until the context deadline fires.
31+
cmd, timeoutCancel := gitCmd(ctx, "", []string{"hash-object", "--stdin"}, nil)
32+
defer timeoutCancel()
33+
34+
err := cmd.Input(blockingReader{cancel: ctx.Done()}).StdOut().Run().Stream(io.Discard)
35+
err = mapContextError(err, ctx)
3236
assert.Equal(t, ErrExecTimeout, err)
3337
})
3438
}
@@ -46,7 +50,7 @@ func (r blockingReader) Read(p []byte) (int, error) {
4650
return 0, io.EOF
4751
}
4852

49-
func TestGitPipeline_ContextCancellation(t *testing.T) {
53+
func TestGitCmd_ContextCancellation(t *testing.T) {
5054
ctx, cancel := context.WithCancel(context.Background())
5155

5256
// Cancel in the background after a short delay so the command is already
@@ -58,7 +62,11 @@ func TestGitPipeline_ContextCancellation(t *testing.T) {
5862
close(done)
5963
}()
6064

61-
err := gitPipeline(ctx, "", []string{"hash-object", "--stdin"}, nil, io.Discard, io.Discard, blockingReader{cancel: done})
65+
cmd, timeoutCancel := gitCmd(ctx, "", []string{"hash-object", "--stdin"}, nil)
66+
defer timeoutCancel()
67+
68+
err := cmd.Input(blockingReader{cancel: done}).StdOut().Run().Stream(io.Discard)
69+
err = mapContextError(err, ctx)
6270
assert.ErrorIs(t, err, context.Canceled)
6371
// Must NOT be ErrExecTimeout — cancellation is distinct from deadline.
6472
assert.NotEqual(t, ErrExecTimeout, err)

repo.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -448,7 +448,7 @@ func (r *Repository) ShowNameStatus(ctx context.Context, rev string, opts ...Sho
448448
args = append(args, opt.Args...)
449449
args = append(args, "--end-of-options", rev)
450450

451-
err := gitPipeline(ctx, r.path, args, opt.Envs, w, nil, nil)
451+
err := gitPipeline(ctx, r.path, args, opt.Envs, w, nil)
452452
_ = w.Close() // Close writer to exit parsing goroutine
453453
if err != nil {
454454
return nil, err

repo_diff.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ func (r *Repository) Diff(ctx context.Context, rev string, maxFiles, maxFileLine
5959
done := make(chan SteamParseDiffResult)
6060
go StreamParseDiff(stdout, done, maxFiles, maxFileLines, maxLineChars)
6161

62-
err = gitPipeline(ctx, r.path, args, opt.Envs, w, nil, nil)
62+
err = gitPipeline(ctx, r.path, args, opt.Envs, w, nil)
6363
_ = w.Close() // Close writer to exit parsing goroutine
6464
if err != nil {
6565
return nil, err
@@ -132,7 +132,7 @@ func (r *Repository) RawDiff(ctx context.Context, rev string, diffType RawDiffFo
132132
return fmt.Errorf("invalid diffType: %s", diffType)
133133
}
134134

135-
if err = gitPipeline(ctx, r.path, args, opt.Envs, w, nil, nil); err != nil {
135+
if err = gitPipeline(ctx, r.path, args, opt.Envs, w, nil); err != nil {
136136
return err
137137
}
138138
return nil

0 commit comments

Comments
 (0)