From 0d999e1db6cd8736ab697de8ce848fa3a5274b9f Mon Sep 17 00:00:00 2001 From: Joshua Sjoding Date: Wed, 24 Feb 2016 22:40:30 -0800 Subject: 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 --- blame.go | 15 ++++++++-- blame_test.go | 3 +- commit.go | 11 ++++++-- common.go | 25 ++++++++++++++++- core/object.go | 19 +++++++++++-- file.go | 25 ++++++++++++----- file_test.go | 10 +++++-- formats/packfile/common.go | 20 +++++++++++++ formats/packfile/reader.go | 64 +++++++++++++++++++++++++++++++++++------- objects.go | 3 +- objects_test.go | 18 ++++++++++-- references.go | 10 +++++-- repository.go | 4 +-- storage/memory/object.go | 18 +++++++----- storage/memory/object_test.go | 12 ++++++-- storage/memory/storage_test.go | 14 +++++++-- tag.go | 10 +++++-- tree.go | 10 +++++-- 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 { -- cgit