storage: make benchfmt.Reader use Scanner-style interface

Change-Id: I052c2b3cc3440f081ef11ba5d999bf8e7f230c6a
Reviewed-on: https://go-review.googlesource.com/35450Reviewed-by: Russ Cox <rsc@golang.org>

storage/app/query_test.go

@@ -6,7 +6,6 @@ package app
 
 import (
 	"fmt"
-	"io"
 	"mime/multipart"
 	"net/http"
 	"net/url"
@@ -65,10 +64,10 @@ func TestQuery(t *testing.T) {
 			}
 			br := benchfmt.NewReader(resp.Body)
 			for i, num := range test.want {
-				r, err := br.Next()
-				if err != nil {
-					t.Fatalf("#%d: Next() = %v, want nil", i, err)
+				if !br.Next() {
+					t.Fatalf("#%d: Next() = false, want true (Err() = %v)", i, br.Err())
 				}
+				r := br.Result()
 				if r.Labels["label0"] != fmt.Sprintf("%d", num) {
 					t.Errorf("#%d: label0 = %q, want %d", i, r.Labels["label0"], num)
 				}
@@ -79,9 +78,11 @@ func TestQuery(t *testing.T) {
 					t.Errorf("#%d: by = %q, want %q", i, r.Labels["uploader"], "user")
 				}
 			}
-			_, err = br.Next()
-			if err != io.EOF {
-				t.Errorf("Next() = %v, want EOF", err)
+			if br.Next() {
+				t.Fatalf("Next() = true, want false")
+			}
+			if err := br.Err(); err != nil {
+				t.Errorf("Err() = %v, want nil", err)
 			}
 		})
 	}

storage/app/upload.go

@@ -194,20 +194,17 @@ func (a *App) indexFile(ctx context.Context, upload *db.Upload, p io.Reader, met
 	br := benchfmt.NewReader(tr)
 	br.AddLabels(meta)
 	i := 0
-	for {
-		result, err := br.Next()
-		if err != nil {
-			if err != io.EOF {
-				return err
-			}
-			if i == 0 {
-				return errors.New("no valid benchmark lines found")
-			}
-			return nil
-		}
+	for br.Next() {
 		i++
-		if err := upload.InsertRecord(result); err != nil {
+		if err := upload.InsertRecord(br.Result()); err != nil {
 			return err
 		}
 	}
+	if err := br.Err(); err != nil {
+		return err
+	}
+	if i == 0 {
+		return errors.New("no valid benchmark lines found")
+	}
+	return nil
 }

storage/benchfmt/benchfmt.go

@@ -18,6 +18,15 @@ import (
 )
 
 // Reader reads benchmark results from an io.Reader.
+// Use Next to advance through the results.
+//
+//   br := benchfmt.NewReader(r)
+//   for br.Next() {
+//     res := br.Result()
+//     ...
+//   }
+//   err = br.Err() // get any error encountered during iteration
+//   ...
 type Reader struct {
 	s      *bufio.Scanner
 	labels Labels
@@ -25,10 +34,11 @@ type Reader struct {
 	// file or provided by AddLabels. They cannot be overridden.
 	permLabels Labels
 	lineNum    int
+	// cached from the last call to Next
+	result *Result
+	err    error
 }
 
-// TODO(quentin): Make Reader have a Scanner-style interface instead, to match db.Query.
-
 // NewReader creates a BenchmarkReader that reads from r.
 func NewReader(r io.Reader) *Reader {
 	return &Reader{
@@ -175,7 +185,10 @@ func (l Labels) copy() Labels {
 
 // Next returns the next benchmark result from the file. If there are
 // no further results, it returns nil, io.EOF.
-func (r *Reader) Next() (*Result, error) {
+func (r *Reader) Next() bool {
+	if r.err != nil {
+		return false
+	}
 	copied := false
 	havePerm := r.permLabels != nil
 	for r.s.Scan() {
@@ -208,13 +221,29 @@ func (r *Reader) Next() (*Result, error) {
 			}
 		}
 		if fullName, ok := parseBenchmarkLine(line); ok {
-			return newResult(r.labels, r.lineNum, fullName, line), nil
+			r.result = newResult(r.labels, r.lineNum, fullName, line)
+			return true
 		}
 	}
 	if err := r.s.Err(); err != nil {
-		return nil, err
+		r.err = err
+		return false
+	}
+	r.err = io.EOF
+	return false
+}
+
+// Result returns the most recent result generated by a call to Next.
+func (r *Reader) Result() *Result {
+	return r.result
+}
+
+// Err returns the error state of the reader.
+func (r *Reader) Err() error {
+	if r.err == io.EOF {
+		return nil
 	}
-	return nil, io.EOF
+	return r.err
 }
 
 // parseKeyValueLine attempts to parse line as a key: value pair. ok

storage/benchfmt/benchfmt_test.go

@@ -7,7 +7,6 @@ package benchfmt
 import (
 	"bytes"
 	"fmt"
-	"io"
 	"io/ioutil"
 	"os"
 	"os/exec"
@@ -18,17 +17,13 @@ import (
 
 func readAllResults(t *testing.T, r *Reader) []*Result {
 	var out []*Result
-	for {
-		result, err := r.Next()
-		switch err {
-		case io.EOF:
-			return out
-		case nil:
-			out = append(out, result)
-		default:
-			t.Fatal(err)
-		}
+	for r.Next() {
+		out = append(out, r.Result())
 	}
+	if err := r.Err(); err != nil {
+		t.Fatal(err)
+	}
+	return out
 }
 
 func TestBenchmarkReader(t *testing.T) {

storage/cmd/reindex/reindex.go

@@ -16,7 +16,6 @@ import (
 	"context"
 	"flag"
 	"fmt"
-	"io"
 	"log"
 	"os"
 	"strings"
@@ -131,17 +130,13 @@ func reindexOne(ctx context.Context, u *db.Upload, bucket *storage.BucketHandle,
 	}
 	defer r.Close()
 	br := benchfmt.NewReader(r)
-	for {
-		res, err := br.Next()
-		if err == io.EOF {
-			break
-		}
-		if err != nil {
-			return err
-		}
-		if err := u.InsertRecord(res); err != nil {
+	for br.Next() {
+		if err := u.InsertRecord(br.Result()); err != nil {
 			return err
 		}
 	}
+	if err := br.Err(); err != nil {
+		return err
+	}
 	return nil
 }

storage/db/db.go

@@ -474,7 +474,15 @@ func (q *Query) Next() bool {
 		return false
 	}
 	// TODO(quentin): Needs to change when one row contains multiple Results.
-	q.result, q.err = benchfmt.NewReader(bytes.NewReader(content)).Next()
+	br := benchfmt.NewReader(bytes.NewReader(content))
+	if !br.Next() {
+		q.err = br.Err()
+		if q.err == nil {
+			q.err = io.ErrUnexpectedEOF
+		}
+		return false
+	}
+	q.result = br.Result()
 	return q.err == nil
 }
 

storage/db/db_test.go

@@ -188,13 +188,11 @@ func TestNewUpload(t *testing.T) {
 key: value
 BenchmarkName 1 ns/op
 `))
-
-	r, err := br.Next()
-	if err != nil {
-		t.Fatalf("BenchmarkReader.Next: %v", err)
+	if !br.Next() {
+		t.Fatalf("unable to read test string: %v", br.Err())
 	}
 
-	if err := u.InsertRecord(r); err != nil {
+	if err := u.InsertRecord(br.Result()); err != nil {
 		t.Fatalf("InsertRecord: %v", err)
 	}