Support reading multiple patches from an mbox file (#89)

Using a combined mbox file may be more convenient than providing
multiple files or running the command multiple times, depending on the
source of the patches.

Note that this doesn't do any unescaping of escaped "From " lines in the
message content. This isn't a problem for the actual patch content, but
could lead to unexpected commit messages.

Author: bluekeyes

README.md

@@ -49,9 +49,11 @@ Usage: patch2pr [options] [patch...]
 
   Create a GitHub pull request from a patch file
 
-  This command parses a patch, applies it, and creates a pull request with the
-  result. It does not clone the repository to apply the patch. If no patch file
-  is given, the command reads the patch from standard input.
+  This command parses one or more patches, applies them, and creates a pull
+  request with the result. It does not clone the repository. If no patch files
+  are given, the command reads the patches from standard input. Each file can
+  contain a single patch or multiple patches in the mbox format produced by 'git
+  format-patch --stdout' or GitHub's patch view.
 
   By default, patch2pr uses the patch header for author and committer
   information, falling back to the authenticated GitHub user if the headers are

cmd/patch2pr/main.go

@@ -148,10 +148,9 @@ func main() {
 }
 
 type Patch struct {
-	path     string
-	files    []*gitdiff.File
-	preamble string
-	header   *gitdiff.PatchHeader
+	path   string
+	files  []*gitdiff.File
+	header *gitdiff.PatchHeader
 }
 
 type Result struct {
@@ -165,7 +164,7 @@ type PullRequestResult struct {
 	URL    string `json:"url"`
 }
 
-func parse(patchFile string) (*Patch, error) {
+func parse(patchFile string) ([]Patch, error) {
 	var r io.ReadCloser
 	if patchFile == "-" {
 		r = os.Stdin
@@ -176,22 +175,28 @@ func parse(patchFile string) (*Patch, error) {
 		}
 		r = f
 	}
+	defer closeQuitely(r)
 
-	files, preamble, err := gitdiff.Parse(r)
-	if err != nil {
-		return nil, fmt.Errorf("parsing patch failed: %w", err)
-	}
-	_ = r.Close()
+	mbr := mboxMessageReader{r: r}
 
-	var header *gitdiff.PatchHeader
-	if len(preamble) > 0 {
-		header, err = gitdiff.ParsePatchHeader(preamble)
+	var patches []Patch
+	for mbr.Next() {
+		files, preamble, err := gitdiff.Parse(&mbr)
 		if err != nil {
-			fmt.Fprintf(os.Stderr, "warning: invalid patch header: %v", err)
+			return nil, fmt.Errorf("parsing patch failed: %w", err)
+		}
+
+		var header *gitdiff.PatchHeader
+		if len(preamble) > 0 {
+			header, err = gitdiff.ParsePatchHeader(preamble)
+			if err != nil {
+				fmt.Fprintf(os.Stderr, "warning: invalid patch header: %v", err)
+			}
 		}
-	}
 
-	return &Patch{patchFile, files, preamble, header}, nil
+		patches = append(patches, Patch{patchFile, files, header})
+	}
+	return patches, nil
 }
 
 func execute(ctx context.Context, client *github.Client, patchFiles []string, opts *Options) (*Result, error) {
@@ -224,23 +229,24 @@ func execute(ctx context.Context, client *github.Client, patchFiles []string, op
 		return nil, fmt.Errorf("get commit for %s failed: %w", patchBase, err)
 	}
 
-	var patches []Patch
+	var allPatches []Patch
 	for _, patchFile := range patchFiles {
-		patch, err := parse(patchFile)
+		patches, err := parse(patchFile)
 		if err != nil {
 			return nil, err
 		}
-		patches = append(patches, *patch)
+		allPatches = append(allPatches, patches...)
 	}
 
 	sourceRepo, err := prepareSourceRepo(ctx, client, opts)
 	if err != nil {
 		return nil, err
 	}
 
-	newCommit := commit
-	for _, patch := range patches {
-		applier := patch2pr.NewApplier(client, sourceRepo, newCommit)
+	applier := patch2pr.NewApplier(client, sourceRepo, commit)
+
+	var newCommit *github.Commit
+	for _, patch := range allPatches {
 		for _, file := range patch.files {
 			if _, err := applier.Apply(ctx, file); err != nil {
 				name := file.NewName
@@ -479,15 +485,21 @@ func isCode(err error, code int) bool {
 	return errors.As(err, &rerr) && rerr.Response.StatusCode == code
 }
 
+func closeQuitely(c io.Closer) {
+	_ = c.Close()
+}
+
 func helpText() string {
 	help := `
 Usage: patch2pr [options] [patch...]
 
   Create a GitHub pull request from a patch file
 
-  This command parses a patch, applies it, and creates a pull request with the
-  result. It does not clone the repository to apply the patch. If no patch file
-  is given, the command reads the patch from standard input.
+  This command parses one or more patches, applies them, and creates a pull
+  request with the result. It does not clone the repository. If no patch files
+  are given, the command reads the patches from standard input. Each file can
+  contain a single patch or multiple patches in the mbox format produced by 'git
+  format-patch --stdout' or GitHub's patch view.
 
   By default, patch2pr uses the patch header for author and committer
   information, falling back to the authenticated GitHub user if the headers are

cmd/patch2pr/mbox.go

@@ -0,0 +1,124 @@
+package main
+
+import (
+	"bytes"
+	"io"
+)
+
+var (
+	mboxHeader = []byte("From ")
+)
+
+type fileType int
+
+const (
+	fileUnknown fileType = iota
+	filePlain
+	fileMBox
+)
+
+type mboxMessageReader struct {
+	r    io.Reader
+	next bytes.Buffer
+
+	headers int
+	ftype   fileType
+
+	isLineStart bool
+	isEOF       bool
+}
+
+func (r *mboxMessageReader) Next() bool {
+	if r.isEOF && r.bufferEmpty() {
+		return false
+	}
+
+	r.headers = 0
+	r.isLineStart = true
+	return true
+}
+
+func (r *mboxMessageReader) Read(p []byte) (n int, err error) {
+	// TODO(bkeyes): This is broken if it is only called with len(p) < len(mboxHeader).
+	// This shouldn't be a problem in practice because go-gitdiff always wraps
+	// the input reader in a bufio.Reader, which uses a large enough buffer.
+
+	if len(p) == 0 {
+		return 0, nil
+	}
+	if r.headers > 1 || (r.isEOF && r.bufferEmpty()) {
+		return 0, io.EOF
+	}
+
+	if !r.bufferEmpty() {
+		n, _ = r.next.Read(p)
+		if r.bufferEmpty() {
+			r.next.Reset()
+		}
+	}
+	if n < len(p) {
+		var nn int
+		nn, err = r.r.Read(p[n:])
+		if err == io.EOF {
+			r.isEOF = true
+			err = nil
+		}
+		n += nn
+	}
+
+	if r.ftype == fileUnknown || r.ftype == fileMBox {
+		n = r.scanForHeader(p, n)
+	}
+
+	return n, err
+}
+
+func (r *mboxMessageReader) scanForHeader(p []byte, n int) int {
+	for i := 0; i < n; i++ {
+		if isSpace(p[i]) {
+			r.isLineStart = p[i] == '\n'
+			continue
+		}
+
+		if r.isLineStart {
+			if matchLen := matchMBoxHeader(p[i:n]); matchLen > 0 {
+				if matchLen == len(mboxHeader) {
+					r.ftype = fileMBox
+					r.headers++
+				}
+				if r.headers > 1 || matchLen < len(mboxHeader) {
+					r.next.Write(p[i:n])
+					return i
+				}
+			}
+		}
+
+		if r.ftype == fileUnknown {
+			r.ftype = filePlain
+			break
+		}
+	}
+	return n
+}
+
+func (r *mboxMessageReader) bufferEmpty() bool {
+	return r.next.Len() == 0
+}
+
+func matchMBoxHeader(b []byte) (n int) {
+	for n < len(b) && n < len(mboxHeader) {
+		if b[n] != mboxHeader[n] {
+			return 0
+		}
+		n++
+	}
+	return n
+}
+
+func isSpace(b byte) bool {
+	switch b {
+	case ' ', '\t', '\r', '\n':
+		return true
+	}
+	return false
+}

cmd/patch2pr/mbox_test.go

@@ -0,0 +1,102 @@
+package main
+
+import (
+	"io"
+	"os"
+	"strconv"
+	"strings"
+	"testing"
+)
+
+func TestMBoxMessageReader(t *testing.T) {
+	plainMessage, err := os.ReadFile("testdata/test.txt")
+	if err != nil {
+		t.Fatalf("error reading file: %v", err)
+	}
+
+	tests := map[string]struct {
+		File            string
+		Count           int
+		ExpectedContent map[int]string
+	}{
+		"mbox": {
+			File:  "testdata/test.mbox",
+			Count: 5,
+			ExpectedContent: map[int]string{
+				0: `From 5255ca3071e33871ad7c23de1a3962f19b215f74 Mon Sep 17 00:00:00 2001
+This is the first message
+
+It has several
+lines
+
+`,
+				1: `From 5a900b1a1f8b3e4244127bff85a5fd2d82ae2ced Mon Sep 17 00:00:00 2001
+From: Test <[email protected]>
+This is the second message
+
+It has an mbox From header in the middle of a line and also has an email From: header
+
+`,
+				2: `From e32a4a21b36ccee78e91aa13933388a976bbd9da Mon Sep 17 00:00:00 2001
+
+`,
+				4: `From fdc67b11c12a7cdc7c9714b3b8ef95746198ee40 Mon Sep 17 00:00:00 2001
+
+This is the last message in the file.
+`,
+			},
+		},
+		"short": {
+			File:  "testdata/short.mbox",
+			Count: 3,
+		},
+		"plain": {
+			File:            "testdata/test.txt",
+			Count:           1,
+			ExpectedContent: map[int]string{0: string(plainMessage)},
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			f, err := os.Open(test.File)
+			if err != nil {
+				t.Fatalf("error opening file: %v", err)
+			}
+			defer f.Close()
+
+			mbr := mboxMessageReader{r: f}
+
+			var msgs []string
+			for i := 0; mbr.Next(); i++ {
+				b, err := io.ReadAll(&mbr)
+				if err != nil {
+					t.Fatalf("unexpected error reading message %d: %v", i+1, err)
+				}
+				msgs = append(msgs, string(b))
+			}
+
+			assertMsgCount(t, msgs, test.Count)
+			for key, msg := range test.ExpectedContent {
+				assertMsgContent(t, msgs, key, msg)
+			}
+		})
+	}
+}
+
+func assertMsgCount(t *testing.T, msgs []string, count int) {
+	if len(msgs) != count {
+		msgStrs := make([]string, len(msgs))
+		for i, m := range msgs {
+			msgStrs[i] = "  " + strconv.Quote(m)
+		}
+
+		t.Fatalf("incorrect number of messages: expected %d, got %d\nmessages: [\n%s,\n]", count, len(msgs), strings.Join(msgStrs, ",\n"))
+	}
+}
+
+func assertMsgContent(t *testing.T, msgs []string, i int, expected string) {
+	if msgs[i] != expected {
+		t.Errorf("incorrect content for message %d:\nexpected: %q\n  actual: %q", i+1, expected, msgs[i])
+	}
+}

cmd/patch2pr/testdata/short.mbox

@@ -0,0 +1,5 @@
+From 1b3774094595c5a89650fa56c9cf9af940656874 Mon Sep 17 00:00:00 2001
+
+From 0baa2dbec1da4405bb149c0e252f0dbfe37f36d7 Mon Sep 17 00:00:00 2001
+
+From 66497dd445fb80da4dbda32c1eef8008ddb8643e Mon Sep 17 00:00:00 2001