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

18 files changed, 237 insertions, 54 deletions

diff --git a/blame.go b/blame.go
index 7dc2cc0..3eb4c35 100644
--- a/blame.go
+++ b/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
diff --git a/blame_test.go b/blame_test.go
index f64dfa9..880b0ad 100644
--- a/blame_test.go
+++ b/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))
 
diff --git a/commit.go b/commit.go
index 492a928..4b7d918 100644
--- a/commit.go
+++ b/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 {
diff --git a/common.go b/common.go
index 6174339..d40e1c4 100644
--- a/common.go
+++ b/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
+	}
+}
diff --git a/core/object.go b/core/object.go
index 74ea555..d56a44a 100644
--- a/core/object.go
+++ b/core/object.go
@@ -10,6 +10,21 @@ 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
@@ -17,8 +32,8 @@ type Object interface {
 	SetType(ObjectType)
 	Size() int64
 	SetSize(int64)
-	Reader() io.Reader
-	Writer() io.Writer
+	Reader() (ObjectReader, error)
+	Writer() (ObjectWriter, error)
 }
 
 // ObjectStorage generic storage of objects
diff --git a/file.go b/file.go
index 72a7145..0ec524e 100644
--- a/file.go
+++ b/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 {
diff --git a/file_test.go b/file_test.go
index e3b9b4b..d9024b0 100644
--- a/file_test.go
+++ b/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
 		}
 	}
diff --git a/formats/packfile/common.go b/formats/packfile/common.go
index 57bc0b9..d207563 100644
--- a/formats/packfile/common.go
+++ b/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
+	}
+}
diff --git a/formats/packfile/reader.go b/formats/packfile/reader.go
index 6f30f8a..e14b84e 100644
--- a/formats/packfile/reader.go
+++ b/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,7 +206,17 @@ 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)
@@ -214,12 +224,19 @@ func (r *Reader) readREFDelta(raw core.Object) error {
 
 	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 {
@@ -227,7 +244,7 @@ func (r *Reader) readOFSDelta(raw core.Object, steps int64) error {
 	}
 
 	buf := bytes.NewBuffer(nil)
-	if err := r.inflate(buf); err != nil {
+	if err = r.inflate(buf); err != nil {
 		return err
 	}
 
@@ -236,8 +253,22 @@ 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)
@@ -245,13 +276,26 @@ func (r *Reader) readOFSDelta(raw core.Object, steps int64) error {
 
 	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 {
diff --git a/objects.go b/objects.go
index c43348f..961502a 100644
--- a/objects.go
+++ b/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()
 }
 
diff --git a/objects_test.go b/objects_test.go
index 10b9661..6826957 100644
--- a/objects_test.go
+++ b/objects_test.go
@@ -67,7 +67,10 @@ func (s *ObjectsSuite) TestParseTree(c *C) {
 	for f, err := iter.Next(); err == nil; f, err = iter.Next() {
 		count++
 		if f.Name == "go/example.go" {
-			content, _ := ioutil.ReadAll(f.Reader())
+			reader, err := f.Reader()
+			c.Assert(err, IsNil)
+			defer func() { c.Assert(reader.Close(), IsNil) }()
+			content, _ := ioutil.ReadAll(reader)
 			c.Assert(content, HasLen, 2780)
 		}
 	}
@@ -79,7 +82,12 @@ func (s *ObjectsSuite) TestBlobHash(c *C) {
 	o := &memory.Object{}
 	o.SetType(core.BlobObject)
 	o.SetSize(3)
-	o.Writer().Write([]byte{'F', 'O', 'O'})
+
+	writer, err := o.Writer()
+	c.Assert(err, IsNil)
+	defer func() { c.Assert(writer.Close(), IsNil) }()
+
+	writer.Write([]byte{'F', 'O', 'O'})
 
 	blob := &Blob{}
 	c.Assert(blob.Decode(o), IsNil)
@@ -87,7 +95,11 @@ func (s *ObjectsSuite) TestBlobHash(c *C) {
 	c.Assert(blob.Size, Equals, int64(3))
 	c.Assert(blob.Hash.String(), Equals, "d96c7efbfec2814ae0301ad054dc8d9fc416c9b5")
 
-	data, err := ioutil.ReadAll(blob.Reader())
+	reader, err := blob.Reader()
+	c.Assert(err, IsNil)
+	defer func() { c.Assert(reader.Close(), IsNil) }()
+
+	data, err := ioutil.ReadAll(reader)
 	c.Assert(err, IsNil)
 	c.Assert(string(data), Equals, "FOO")
 }
diff --git a/references.go b/references.go
index 593b975..29f9a71 100644
--- a/references.go
+++ b/references.go
@@ -178,7 +178,10 @@ func patch(c *Commit, path string) ([]diffmatchpatch.Diff, error) {
 	if err != nil {
 		return nil, err
 	}
-	content := file.Contents()
+	content, err := file.Contents()
+	if err != nil {
+		return nil, err
+	}
 
 	// get contents of the file in the first parent of the commit
 	var contentParent string
@@ -191,7 +194,10 @@ func patch(c *Commit, path string) ([]diffmatchpatch.Diff, error) {
 	if err != nil {
 		contentParent = ""
 	} else {
-		contentParent = file.Contents()
+		contentParent, err = file.Contents()
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	// compare the contents of parent and child
diff --git a/repository.go b/repository.go
index cce7097..0e741a7 100644
--- a/repository.go
+++ b/repository.go
@@ -79,9 +79,7 @@ func (r *Repository) Pull(remoteName, branch string) (err error) {
 	if err != nil {
 		return err
 	}
-	defer func() {
-		err = reader.Close()
-	}()
+	defer close(reader, &err)
 
 	pr := packfile.NewReader(reader)
 	if _, err = pr.Read(r.Storage); err != nil {
diff --git a/storage/memory/object.go b/storage/memory/object.go
index eda6ab7..125e887 100644
--- a/storage/memory/object.go
+++ b/storage/memory/object.go
@@ -2,7 +2,7 @@ package memory
 
 import (
 	"bytes"
-	"io"
+	"io/ioutil"
 
 	"gopkg.in/src-d/go-git.v3/core"
 )
@@ -39,17 +39,21 @@ func (o *Object) Size() int64 { return o.size }
 // SetSize set the object size, the given size should be written afterwards
 func (o *Object) SetSize(s int64) { o.size = s }
 
-// Reader returns a io.Reader used to read the object content
-func (o *Object) Reader() io.Reader {
-	return bytes.NewBuffer(o.content)
+// Reader returns a core.ObjectReader used to read the object's content.
+func (o *Object) Reader() (core.ObjectReader, error) {
+	return ioutil.NopCloser(bytes.NewBuffer(o.content)), nil
 }
 
-// Writer returns a io.Writed used to write the object content
-func (o *Object) Writer() io.Writer {
-	return o
+// Writer returns a core.ObjectWriter used to write the object's content.
+func (o *Object) Writer() (core.ObjectWriter, error) {
+	return o, nil
 }
 
 func (o *Object) Write(p []byte) (n int, err error) {
 	o.content = append(o.content, p...)
 	return len(p), nil
 }
+
+// Close releases any resources consumed by the object when it is acting as a
+// core.ObjectWriter.
+func (o *Object) Close() error { return nil }
diff --git a/storage/memory/object_test.go b/storage/memory/object_test.go
index f9dd25d..f2873fa 100644
--- a/storage/memory/object_test.go
+++ b/storage/memory/object_test.go
@@ -51,7 +51,11 @@ func (s *ObjectSuite) TestSize(c *C) {
 func (s *ObjectSuite) TestReader(c *C) {
 	o := &Object{content: []byte("foo")}
 
-	b, err := ioutil.ReadAll(o.Reader())
+	reader, err := o.Reader()
+	c.Assert(err, IsNil)
+	defer func() { c.Assert(reader.Close(), IsNil) }()
+
+	b, err := ioutil.ReadAll(reader)
 	c.Assert(err, IsNil)
 	c.Assert(b, DeepEquals, []byte("foo"))
 }
@@ -59,7 +63,11 @@ func (s *ObjectSuite) TestReader(c *C) {
 func (s *ObjectSuite) TestWriter(c *C) {
 	o := &Object{}
 
-	n, err := o.Writer().Write([]byte("foo"))
+	writer, err := o.Writer()
+	c.Assert(err, IsNil)
+	defer func() { c.Assert(writer.Close(), IsNil) }()
+
+	n, err := writer.Write([]byte("foo"))
 	c.Assert(err, IsNil)
 	c.Assert(n, Equals, 3)
 
diff --git a/storage/memory/storage_test.go b/storage/memory/storage_test.go
index ba7e224..f2fd3da 100644
--- a/storage/memory/storage_test.go
+++ b/storage/memory/storage_test.go
@@ -25,7 +25,12 @@ func (s *ObjectStorageSuite) TestSet(c *C) {
 
 	o.SetType(core.CommitObject)
 	o.SetSize(3)
-	o.Writer().Write([]byte("foo"))
+
+	writer, err := o.Writer()
+	c.Assert(err, IsNil)
+	defer func() { c.Assert(writer.Close(), IsNil) }()
+
+	writer.Write([]byte("foo"))
 
 	h, err := os.Set(o)
 	c.Assert(h.String(), Equals, "bc9968d75e48de59f0870ffb71f5e160bbbdcf52")
@@ -39,7 +44,12 @@ func (s *ObjectStorageSuite) TestGet(c *C) {
 
 	o.SetType(core.CommitObject)
 	o.SetSize(3)
-	o.Writer().Write([]byte("foo"))
+
+	writer, err := o.Writer()
+	c.Assert(err, IsNil)
+	defer func() { c.Assert(writer.Close(), IsNil) }()
+
+	writer.Write([]byte("foo"))
 
 	h, err := os.Set(o)
 	c.Assert(err, IsNil)
diff --git a/tag.go b/tag.go
index 4d2253a..4b3eb4d 100644
--- a/tag.go
+++ b/tag.go
@@ -29,14 +29,20 @@ type Tag struct {
 }
 
 // Decode transforms a core.Object into a Tag struct.
-func (t *Tag) Decode(o core.Object) error {
+func (t *Tag) Decode(o core.Object) (err error) {
 	if o.Type() != core.TagObject {
 		return ErrUnsupportedObject
 	}
 
 	t.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)
 	for {
 		line, err := r.ReadSlice('\n')
 		if err != nil && err != io.EOF {
diff --git a/tree.go b/tree.go
index 3a33f09..0ab06b3 100644
--- a/tree.go
+++ b/tree.go
@@ -128,7 +128,7 @@ func (t *Tree) Files() *FileIter {
 }
 
 // Decode transform an core.Object into a Tree struct
-func (t *Tree) Decode(o core.Object) error {
+func (t *Tree) Decode(o core.Object) (err error) {
 	if o.Type() != core.TreeObject {
 		return ErrUnsupportedObject
 	}
@@ -141,7 +141,13 @@ func (t *Tree) Decode(o core.Object) error {
 	t.Entries = nil
 	t.m = nil
 
-	r := bufio.NewReader(o.Reader())
+	reader, err := o.Reader()
+	if err != nil {
+		return err
+	}
+	defer close(reader, &err)
+
+	r := bufio.NewReader(reader)
 	for {
 		mode, err := r.ReadString(' ')
 		if err != nil {