reviewdiff: Stream branch diffs

internal/forge/shamhub/review.go

@@ -1,6 +1,7 @@
 package shamhub
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -746,7 +747,7 @@ func (sh *ShamHub) reviewCommentOutdated(
 		return false, fmt.Errorf("diff reviewed revision: %w", err)
 	}
 
-	patch, err := reviewdiff.Parse(out)
+	patch, err := reviewdiff.Parse(bytes.NewReader(out))
 	if err != nil {
 		return false, fmt.Errorf("parse reviewed revision diff: %w", err)
 	}

internal/git/diff_wt.go

@@ -2,7 +2,9 @@ package git
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"io"
 	"os"
 )
 
@@ -20,17 +22,45 @@ func (w *Worktree) DiffBranch(ctx context.Context, base, head string) error {
 	return nil
 }
 
-// DiffBranchBytes returns the unified diff output
-// between base and head using triple-dot syntax.
-func (w *Worktree) DiffBranchBytes(
+// OpenBranchDiff starts a unified diff between base and head using triple-dot
+// syntax.
+//
+// The caller must close the returned reader to wait for Git and receive its
+// exit status.
+func (w *Worktree) OpenBranchDiff(
 	ctx context.Context,
 	base, head string,
-) ([]byte, error) {
-	out, err := w.gitCmd(
-		ctx, "diff", base+"..."+head,
-	).Output()
+) (io.ReadCloser, error) {
+	cmd := w.gitCmd(ctx, "diff", base+"..."+head)
+	stdout, err := cmd.StdoutPipe()
 	if err != nil {
-		return nil, fmt.Errorf("diff: %w", err)
+		return nil, fmt.Errorf("pipe stdout: %w", err)
 	}
-	return out, nil
+	if err := cmd.Start(); err != nil {
+		return nil, errors.Join(
+			fmt.Errorf("start diff: %w", err),
+			stdout.Close(),
+		)
+	}
+
+	return &branchDiffReader{
+		ReadCloser: stdout,
+		cmd:        cmd,
+	}, nil
+}
+
+// branchDiffReader waits for the Git process after closing its stdout pipe.
+type branchDiffReader struct {
+	io.ReadCloser
+	cmd *gitCmd
+}
+
+// Close releases the pipe and reports the Git process exit status.
+func (r *branchDiffReader) Close() error {
+	closeErr := r.ReadCloser.Close()
+	waitErr := r.cmd.Wait()
+	if waitErr != nil {
+		waitErr = fmt.Errorf("diff: %w", waitErr)
+	}
+	return errors.Join(closeErr, waitErr)
 }

internal/git/diff_wt_test.go

@@ -0,0 +1,74 @@
+package git_test
+
+import (
+	"errors"
+	"io"
+	"os/exec"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.abhg.dev/gs/internal/git"
+	"go.uber.org/mock/gomock"
+)
+
+func TestWorktree_OpenBranchDiff(t *testing.T) {
+	t.Parallel()
+
+	t.Run("Success", func(t *testing.T) {
+		mockExecer := git.NewMockExecer(gomock.NewController(t))
+		_, wt := git.NewFakeRepository(t, "", mockExecer)
+
+		mockExecer.EXPECT().
+			Start(gomock.Any()).
+			DoAndReturn(func(cmd *exec.Cmd) error {
+				assert.Equal(t, []string{
+					"git", "diff", "main...feature",
+				}, cmd.Args)
+				_, err := io.WriteString(cmd.Stdout, "diff output\n")
+				return errors.Join(err, cmd.Stdout.(io.Closer).Close())
+			})
+		mockExecer.EXPECT().
+			Wait(gomock.Any()).
+			Return(nil)
+
+		diff, err := wt.OpenBranchDiff(t.Context(), "main", "feature")
+		require.NoError(t, err)
+		got, err := io.ReadAll(diff)
+		require.NoError(t, err)
+		require.NoError(t, diff.Close())
+		assert.Equal(t, "diff output\n", string(got))
+	})
+
+	t.Run("StartFailure", func(t *testing.T) {
+		mockExecer := git.NewMockExecer(gomock.NewController(t))
+		_, wt := git.NewFakeRepository(t, "", mockExecer)
+
+		mockExecer.EXPECT().
+			Start(gomock.Any()).
+			Return(errors.New("git did not start"))
+
+		_, err := wt.OpenBranchDiff(t.Context(), "main", "feature")
+		assert.ErrorContains(t, err, "start diff: git did not start")
+	})
+
+	t.Run("CommandFailure", func(t *testing.T) {
+		mockExecer := git.NewMockExecer(gomock.NewController(t))
+		_, wt := git.NewFakeRepository(t, "", mockExecer)
+
+		mockExecer.EXPECT().
+			Start(gomock.Any()).
+			DoAndReturn(func(cmd *exec.Cmd) error {
+				return cmd.Stdout.(io.Closer).Close()
+			})
+		mockExecer.EXPECT().
+			Wait(gomock.Any()).
+			Return(errors.New("git command failed"))
+
+		diff, err := wt.OpenBranchDiff(t.Context(), "main", "feature")
+		require.NoError(t, err)
+		_, err = io.ReadAll(diff)
+		require.NoError(t, err)
+		assert.ErrorContains(t, diff.Close(), "diff: git command failed")
+	})
+}

internal/reviewdiff/patch.go

@@ -2,8 +2,8 @@
 package reviewdiff
 
 import (
-	"bytes"
 	"fmt"
+	"io"
 
 	"github.com/bluekeyes/go-gitdiff/gitdiff"
 )
@@ -19,8 +19,8 @@ type Patch struct {
 }
 
 // Parse parses a Git patch for review-comment queries.
-func Parse(src []byte) (*Patch, error) {
-	files, _, err := gitdiff.Parse(bytes.NewReader(src))
+func Parse(src io.Reader) (*Patch, error) {
+	files, _, err := gitdiff.Parse(src)
 	if err != nil {
 		return nil, fmt.Errorf("parse Git patch: %w", err)
 	}

internal/reviewdiff/patch_test.go

@@ -1,6 +1,7 @@
 package reviewdiff_test
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -9,7 +10,7 @@ import (
 )
 
 func TestPatchContains(t *testing.T) {
-	patch, err := reviewdiff.Parse([]byte(`diff --git a/main.go b/main.go
+	patch, err := reviewdiff.Parse(strings.NewReader(`diff --git a/main.go b/main.go
 --- a/main.go
 +++ b/main.go
 @@ -1,4 +1,5 @@
@@ -61,7 +62,7 @@ new mode 100755
 }
 
 func TestPatchDeletes(t *testing.T) {
-	patch, err := reviewdiff.Parse([]byte(`diff --git a/main.go b/main.go
+	patch, err := reviewdiff.Parse(strings.NewReader(`diff --git a/main.go b/main.go
 --- a/main.go
 +++ b/main.go
 @@ -2,5 +2,4 @@ package main
@@ -95,7 +96,7 @@ rename to new.go
 }
 
 func TestParseError(t *testing.T) {
-	_, err := reviewdiff.Parse([]byte(`detached fragment
+	_, err := reviewdiff.Parse(strings.NewReader(`detached fragment
 @@ -1 +1 @@
 `))
 	require.Error(t, err)

review_comment.go

@@ -85,11 +85,12 @@ func (cmd *reviewCommentCmd) Run(
 	if err != nil {
 		return err
 	}
-	diff, err := wt.DiffBranchBytes(ctx, b.Base, branch)
+	diff, err := wt.OpenBranchDiff(ctx, b.Base, branch)
 	if err != nil {
-		return fmt.Errorf("get diff: %w", err)
+		return fmt.Errorf("open diff: %w", err)
 	}
 	patch, err := reviewdiff.Parse(diff)
+	err = errors.Join(err, diff.Close())
 	if err != nil {
 		return fmt.Errorf("parse diff: %w", err)
 	}

review_publish.go

@@ -92,12 +92,12 @@ func (cmd *reviewPublishCmd) Run(
 
 	// Draft roots use the selected branch's postimage coordinates. Parse the
 	// review diff once so every root can be checked before anything is sent.
-	diff, err := wt.DiffBranchBytes(ctx, b.Base, branch)
+	diff, err := wt.OpenBranchDiff(ctx, b.Base, branch)
 	if err != nil {
-		return fmt.Errorf("get diff: %w", err)
+		return fmt.Errorf("open diff: %w", err)
 	}
-
 	patch, err := reviewdiff.Parse(diff)
+	err = errors.Join(err, diff.Close())
 	if err != nil {
 		return fmt.Errorf("parse diff: %w", err)
 	}