Refactor to use core.ObjectReader and core.ObjectWriter

* New function signatures provide the necessary interface to stream data from disk when using filesystem-based storage in the future
* New function signatures provide proper error handling
* ObjectReader and ObjectWriter interfaces added to avoid future refactoring, currently are type aliases for io.ReadCloser and io.WriteCloser respectively
* Object.Reader now returns (ObjectReader, error)
* Object.Writer now returns (ObjectWriter, error)
* File.Contents now returns (string, error)
* File.Lines now returns ([]string, error)
* Blob.Reader now returns (core.ObjectReader, error)
* Added internal close helper function for deferred calls to Close that need to check the return value

Author: JoshuaSjoding

blame.go

@@ -82,7 +82,10 @@ func (c *Commit) Blame(path string) (*Blame, error) {
 	if err != nil {
 		return nil, err
 	}
-	finalLines := file.Lines()
+	finalLines, err := file.Lines()
+	if err != nil {
+		return nil, err
+	}
 
 	lines, err := newLines(finalLines, b.sliceGraph(len(b.graph)-1))
 	if err != nil {
@@ -153,7 +156,10 @@ func (b *blame) fillGraphAndData() error {
 		if err != nil {
 			return nil
 		}
-		b.data[i] = file.Contents()
+		b.data[i], err = file.Contents()
+		if err != nil {
+			return err
+		}
 		nLines := countLines(b.data[i])
 		// create a node for each line
 		b.graph[i] = make([]*Commit, nLines)
@@ -221,7 +227,10 @@ func (b *blame) GoString() string {
 	if err != nil {
 		panic("PrettyPrint: internal error in repo.Data")
 	}
-	contents := file.Contents()
+	contents, err := file.Contents()
+	if err != nil {
+		panic("PrettyPrint: internal error in repo.Data")
+	}
 
 	lines := strings.Split(contents, "\n")
 	// max line number length

blame_test.go

@@ -56,7 +56,8 @@ func (s *BlameCommon) mockBlame(t blameTest, c *C) (blame *Blame) {
 
 	file, err := commit.File(t.path)
 	c.Assert(err, IsNil)
-	lines := file.Lines()
+	lines, err := file.Lines()
+	c.Assert(err, IsNil)
 	c.Assert(len(t.blames), Equals, len(lines), Commentf(
 		"repo=%s, path=%s, rev=%s: the number of lines in the file and the number of expected blames differ (len(blames)=%d, len(lines)=%d)\nblames=%#q\nlines=%#q", t.repo, t.path, t.rev, len(t.blames), len(lines), t.blames, lines))
 

commit.go

@@ -50,13 +50,20 @@ func (c *Commit) File(path string) (file *File, err error) {
 }
 
 // Decode transform an core.Object into a Blob struct
-func (c *Commit) Decode(o core.Object) error {
+func (c *Commit) Decode(o core.Object) (err error) {
 	if o.Type() != core.CommitObject {
 		return ErrUnsupportedObject
 	}
 
 	c.Hash = o.Hash()
-	r := bufio.NewReader(o.Reader())
+
+	reader, err := o.Reader()
+	if err != nil {
+		return err
+	}
+	defer close(reader, &err)
+
+	r := bufio.NewReader(reader)
 
 	var message bool
 	for {

common.go

@@ -1,6 +1,9 @@
 package git
 
-import "strings"
+import (
+	"io"
+	"strings"
+)
 
 // countLines returns the number of lines in a string à la git, this is
 // The newline character is assumed to be '\n'.  The empty string
@@ -18,3 +21,23 @@ func countLines(s string) int {
 
 	return nEOL + 1
 }
+
+// close is used with defer to close the given io.Closer and check its
+// returned error value. If Close returns an error and the given *error
+// is not nil, *error is set to the error returned by Close.
+//
+// close is typically used with named return values like so:
+//
+//   func do(obj *Object) (err error) {
+//     w, err := obj.Writer()
+//     if err != nil {
+//       return nil
+//     }
+//     defer close(w, &err)
+//     // work with w
+//   }
+func close(c io.Closer, err *error) {
+	if cerr := c.Close(); cerr != nil && *err == nil {
+		*err = cerr
+	}
+}

core/object.go

@@ -10,15 +10,30 @@ var (
 	ObjectNotFoundErr = errors.New("object not found")
 )
 
+// TODO: Consider adding a Hash function to the ObjectReader and ObjectWriter
+//       interfaces that returns the hash calculated for the reader or writer.
+
+// ObjectReader is a generic representation of an object reader.
+//
+// ObjectReader implements io.ReadCloser. Close should be called when finished
+// with it.
+type ObjectReader io.ReadCloser
+
+// ObjectWriter is a generic representation of an object writer.
+//
+// ObjectWriter implements io.WriterCloser. Close should be called when finished
+// with it.
+type ObjectWriter io.WriteCloser
+
 // Object is a generic representation of any git object
 type Object interface {
 	Hash() Hash
 	Type() ObjectType
 	SetType(ObjectType)
 	Size() int64
 	SetSize(int64)
-	Reader() io.Reader
-	Writer() io.Writer
+	Reader() (ObjectReader, error)
+	Writer() (ObjectWriter, error)
 }
 
 // ObjectStorage generic storage of objects

file.go

@@ -18,24 +18,35 @@ func newFile(name string, b *Blob) *File {
 }
 
 // Contents returns the contents of a file as a string.
-func (f *File) Contents() string {
+func (f *File) Contents() (content string, err error) {
+	reader, err := f.Reader()
+	if err != nil {
+		return "", err
+	}
+	defer close(reader, &err)
+
 	buf := new(bytes.Buffer)
-	buf.ReadFrom(f.Reader())
+	buf.ReadFrom(reader)
 
-	return buf.String()
+	return buf.String(), nil
 }
 
 // Lines returns a slice of lines from the contents of a file, stripping
 // all end of line characters. If the last line is empty (does not end
 // in an end of line), it is also stripped.
-func (f *File) Lines() []string {
-	splits := strings.Split(f.Contents(), "\n")
+func (f *File) Lines() ([]string, error) {
+	content, err := f.Contents()
+	if err != nil {
+		return nil, err
+	}
+
+	splits := strings.Split(content, "\n")
 	// remove the last line if it is empty
 	if splits[len(splits)-1] == "" {
-		return splits[:len(splits)-1]
+		return splits[:len(splits)-1], nil
 	}
 
-	return splits
+	return splits, nil
 }
 
 type FileIter struct {

file_test.go

@@ -125,7 +125,9 @@ func (s *SuiteFile) TestContents(c *C) {
 
 		file, err := commit.File(t.path)
 		c.Assert(err, IsNil)
-		c.Assert(file.Contents(), Equals, t.contents, Commentf(
+		content, err := file.Contents()
+		c.Assert(err, IsNil)
+		c.Assert(content, Equals, t.contents, Commentf(
 			"subtest %d: commit=%s, path=%s", i, t.commit, t.path))
 	}
 }
@@ -172,7 +174,9 @@ func (s *SuiteFile) TestLines(c *C) {
 
 		file, err := commit.File(t.path)
 		c.Assert(err, IsNil)
-		c.Assert(file.Lines(), DeepEquals, t.lines, Commentf(
+		lines, err := file.Lines()
+		c.Assert(err, IsNil)
+		c.Assert(lines, DeepEquals, t.lines, Commentf(
 			"subtest %d: commit=%s, path=%s", i, t.commit, t.path))
 	}
 }
@@ -201,7 +205,7 @@ func (s *SuiteFile) TestIgnoreEmptyDirEntries(c *C) {
 		iter := commit.Tree().Files()
 		defer iter.Close()
 		for file, err := iter.Next(); err == nil; file, err = iter.Next() {
-			_ = file.Contents()
+			_, _ = file.Contents()
 			// this would probably panic if we are not ignoring empty dirs
 		}
 	}

formats/packfile/common.go

@@ -41,3 +41,23 @@ func (t *trackingReader) ReadByte() (c byte, err error) {
 	t.position++
 	return p[0], nil
 }
+
+// close is used with defer to close the given io.Closer and check its
+// returned error value. If Close returns an error and the given *error
+// is not nil, *error is set to the error returned by Close.
+//
+// close is typically used with named return values like so:
+//
+//   func do(obj *Object) (err error) {
+//     w, err := obj.Writer()
+//     if err != nil {
+//       return nil
+//     }
+//     defer close(w, &err)
+//     // work with w
+//   }
+func close(c io.Closer, err *error) {
+	if cerr := c.Close(); cerr != nil && *err == nil {
+		*err = cerr
+	}
+}

formats/packfile/reader.go

@@ -187,7 +187,7 @@ func (r *Reader) newObject() (core.Object, error) {
 	return raw, err
 }
 
-func (r *Reader) readREFDelta(raw core.Object) error {
+func (r *Reader) readREFDelta(raw core.Object) (err error) {
 	var ref core.Hash
 	if _, err := io.ReadFull(r.r, ref[:]); err != nil {
 		return err
@@ -206,28 +206,45 @@ func (r *Reader) readREFDelta(raw core.Object) error {
 		return err
 	}
 
-	d, _ := ioutil.ReadAll(referenced.Reader())
+	reader, err := referenced.Reader()
+	if err != nil {
+		return err
+	}
+	defer close(reader, &err)
+
+	d, err := ioutil.ReadAll(reader)
+	if err != nil {
+		return err
+	}
+
 	patched := patchDelta(d, buf.Bytes())
 	if patched == nil {
 		return PatchingErr.n("hash %q", ref)
 	}
 
 	raw.SetType(referenced.Type())
 	raw.SetSize(int64(len(patched)))
-	raw.Writer().Write(patched)
+
+	writer, err := raw.Writer()
+	if err != nil {
+		return err
+	}
+	defer close(writer, &err)
+
+	writer.Write(patched)
 
 	return nil
 }
 
-func (r *Reader) readOFSDelta(raw core.Object, steps int64) error {
+func (r *Reader) readOFSDelta(raw core.Object, steps int64) (err error) {
 	start := r.r.position
 	offset, err := decodeOffset(r.r, steps)
 	if err != nil {
 		return err
 	}
 
 	buf := bytes.NewBuffer(nil)
-	if err := r.inflate(buf); err != nil {
+	if err = r.inflate(buf); err != nil {
 		return err
 	}
 
@@ -236,22 +253,49 @@ func (r *Reader) readOFSDelta(raw core.Object, steps int64) error {
 		return PackEntryNotFoundErr.n("offset %d", start+offset)
 	}
 
-	referenced, _ := r.s.Get(ref) // FIXME: Handle error returned from Get()
-	d, _ := ioutil.ReadAll(referenced.Reader())
+	referenced, err := r.s.Get(ref)
+	if err != nil {
+		return err
+	}
+
+	reader, err := referenced.Reader()
+	if err != nil {
+		return err
+	}
+	defer close(reader, &err)
+
+	d, err := ioutil.ReadAll(reader)
+	if err != nil {
+		return err
+	}
+
 	patched := patchDelta(d, buf.Bytes())
 	if patched == nil {
 		return PatchingErr.n("hash %q", ref)
 	}
 
 	raw.SetType(referenced.Type())
 	raw.SetSize(int64(len(patched)))
-	raw.Writer().Write(patched)
+
+	writer, err := raw.Writer()
+	if err != nil {
+		return err
+	}
+	defer close(writer, &err)
+
+	writer.Write(patched)
 
 	return nil
 }
 
-func (r *Reader) readObject(raw core.Object) error {
-	return r.inflate(raw.Writer())
+func (r *Reader) readObject(raw core.Object) (err error) {
+	writer, err := raw.Writer()
+	if err != nil {
+		return err
+	}
+	defer close(writer, &err)
+
+	return r.inflate(writer)
 }
 
 func (r *Reader) inflate(w io.Writer) error {

objects.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
-	"io"
 	"strconv"
 	"time"
 
@@ -35,7 +34,7 @@ func (b *Blob) Decode(o core.Object) error {
 }
 
 // Reader returns a reader allow the access to the content of the blob
-func (b *Blob) Reader() io.Reader {
+func (b *Blob) Reader() (core.ObjectReader, error) {
 	return b.obj.Reader()
 }