Merge pull request #907 from erizocosmico/feature/fix-tests

plumbing: packfile, fix package tests
Signed-off-by: Miguel Molina <miguel@erizocosmi.co>

7 files changed, 116 insertions, 45 deletions

diff --git a/plumbing/format/idxfile/idxfile.go b/plumbing/format/idxfile/idxfile.go
index d4a9365..71c7630 100644
--- a/plumbing/format/idxfile/idxfile.go
+++ b/plumbing/format/idxfile/idxfile.go
@@ -67,6 +67,10 @@ func (idx *MemoryIndex) findHashIndex(h plumbing.Hash) int {
 		return -1
 	}
 
+	if len(idx.Names) <= k {
+		return -1
+	}
+
 	data := idx.Names[k]
 	high := uint64(len(idx.Offset32[k])) >> 2
 	if high == 0 {
@@ -103,6 +107,10 @@ func (idx *MemoryIndex) Contains(h plumbing.Hash) (bool, error) {
 
 // FindOffset implements the Index interface.
 func (idx *MemoryIndex) FindOffset(h plumbing.Hash) (int64, error) {
+	if len(idx.FanoutMapping) <= int(h[0]) {
+		return 0, plumbing.ErrObjectNotFound
+	}
+
 	k := idx.FanoutMapping[h[0]]
 	i := idx.findHashIndex(h)
 	if i < 0 {
diff --git a/plumbing/format/idxfile/writer.go b/plumbing/format/idxfile/writer.go
index efcdcc6..a22cf16 100644
--- a/plumbing/format/idxfile/writer.go
+++ b/plumbing/format/idxfile/writer.go
@@ -25,6 +25,7 @@ type Writer struct {
 	offset64 uint32
 	finished bool
 	index    *MemoryIndex
+	added    map[plumbing.Hash]struct{}
 }
 
 // Index returns a previously created MemoryIndex or creates a new one if
@@ -45,7 +46,15 @@ func (w *Writer) Add(h plumbing.Hash, pos uint64, crc uint32) {
 	w.m.Lock()
 	defer w.m.Unlock()
 
-	w.objects = append(w.objects, Entry{h, crc, pos})
+	if w.added == nil {
+		w.added = make(map[plumbing.Hash]struct{})
+	}
+
+	if _, ok := w.added[h]; !ok {
+		w.added[h] = struct{}{}
+		w.objects = append(w.objects, Entry{h, crc, pos})
+	}
+
 }
 
 func (w *Writer) Finished() bool {
diff --git a/plumbing/format/packfile/decoder.go b/plumbing/format/packfile/decoder.go
index d6bc0ef..6bb0677 100644
--- a/plumbing/format/packfile/decoder.go
+++ b/plumbing/format/packfile/decoder.go
@@ -122,6 +122,7 @@ func NewDecoderForType(s *Scanner, o storer.EncodedObjectStorer,
 		deltaBaseCache: cacheObject,
 
 		idx:          idxfile.NewMemoryIndex(),
+		writer:       new(idxfile.Writer),
 		offsetToType: make(map[int64]plumbing.ObjectType),
 		offsetToHash: make(map[int64]plumbing.Hash),
 		decoderType:  t,
@@ -152,7 +153,12 @@ func (d *Decoder) Decode() (checksum plumbing.Hash, err error) {
 
 	if !d.hasBuiltIndex {
 		d.writer.OnFooter(checksum)
-		d.idx = d.Index()
+
+		idx, err := d.writer.Index()
+		if err != nil {
+			return plumbing.ZeroHash, err
+		}
+		d.SetIndex(idx)
 	}
 
 	return checksum, err
@@ -186,12 +192,8 @@ func (d *Decoder) doDecode() error {
 	}
 
 	if !d.hasBuiltIndex {
-		// TODO: MemoryIndex is not writable, change to something else
-		d.idx = idxfile.NewMemoryIndex()
-		d.writer = new(idxfile.Writer)
 		d.writer.OnHeader(count)
 	}
-	defer func() { d.hasBuiltIndex = true }()
 
 	if d.hasBuiltIndex && !d.s.IsSeekable {
 		if err := d.fillOffsetsToHashes(); err != nil {
@@ -202,12 +204,18 @@ func (d *Decoder) doDecode() error {
 	_, isTxStorer := d.o.(storer.Transactioner)
 	switch {
 	case d.o == nil:
-		return d.decodeObjects(int(count))
+		err = d.decodeObjects(int(count))
 	case isTxStorer:
-		return d.decodeObjectsWithObjectStorerTx(int(count))
+		err = d.decodeObjectsWithObjectStorerTx(int(count))
 	default:
-		return d.decodeObjectsWithObjectStorer(int(count))
+		err = d.decodeObjectsWithObjectStorer(int(count))
+	}
+
+	if err != nil {
+		return err
 	}
+
+	return nil
 }
 
 func (d *Decoder) decodeObjects(count int) error {
@@ -509,8 +517,10 @@ func (d *Decoder) recallByHash(h plumbing.Hash) (plumbing.EncodedObject, error)
 func (d *Decoder) recallByHashNonSeekable(h plumbing.Hash) (obj plumbing.EncodedObject, err error) {
 	if d.tx != nil {
 		obj, err = d.tx.EncodedObject(plumbing.AnyObject, h)
-	} else {
+	} else if d.o != nil {
 		obj, err = d.o.EncodedObject(plumbing.AnyObject, h)
+	} else {
+		return nil, plumbing.ErrObjectNotFound
 	}
 
 	if err != plumbing.ErrObjectNotFound {
diff --git a/plumbing/format/packfile/decoder_test.go b/plumbing/format/packfile/decoder_test.go
index 4fe9b5e..d4f7145 100644
--- a/plumbing/format/packfile/decoder_test.go
+++ b/plumbing/format/packfile/decoder_test.go
@@ -5,6 +5,7 @@ import (
 
 	"gopkg.in/src-d/go-git.v4/plumbing"
 	"gopkg.in/src-d/go-git.v4/plumbing/cache"
+	"gopkg.in/src-d/go-git.v4/plumbing/format/idxfile"
 	"gopkg.in/src-d/go-git.v4/plumbing/format/packfile"
 	"gopkg.in/src-d/go-git.v4/plumbing/storer"
 	"gopkg.in/src-d/go-git.v4/storage/filesystem"
@@ -46,7 +47,6 @@ func (s *ReaderSuite) TestDecode(c *C) {
 	})
 }
 
-/*
 func (s *ReaderSuite) TestDecodeByTypeRefDelta(c *C) {
 	f := fixtures.Basic().ByTag("ref-delta").One()
 
@@ -101,9 +101,7 @@ func (s *ReaderSuite) TestDecodeByTypeRefDeltaError(c *C) {
 	})
 
 }
-*/
 
-/*
 func (s *ReaderSuite) TestDecodeByType(c *C) {
 	ts := []plumbing.ObjectType{
 		plumbing.CommitObject,
@@ -142,7 +140,6 @@ func (s *ReaderSuite) TestDecodeByType(c *C) {
 		}
 	})
 }
-*/
 
 func (s *ReaderSuite) TestDecodeByTypeConstructor(c *C) {
 	f := fixtures.Basic().ByTag("packfile").One()
@@ -184,7 +181,7 @@ func (s *ReaderSuite) TestDecodeMultipleTimes(c *C) {
 func (s *ReaderSuite) TestDecodeInMemory(c *C) {
 	fixtures.Basic().ByTag("packfile").Test(c, func(f *fixtures.Fixture) {
 		scanner := packfile.NewScanner(f.Packfile())
-		d, err := packfile.NewDecoder(scanner, nil)
+		d, err := packfile.NewDecoder(scanner, memory.NewStorage())
 		c.Assert(err, IsNil)
 
 		ch, err := d.Decode()
@@ -284,7 +281,6 @@ var expectedHashes = []string{
 	"7e59600739c96546163833214c36459e324bad0a",
 }
 
-/*
 func (s *ReaderSuite) TestDecodeCRCs(c *C) {
 	f := fixtures.Basic().ByTag("ofs-delta").One()
 
@@ -297,8 +293,16 @@ func (s *ReaderSuite) TestDecodeCRCs(c *C) {
 	c.Assert(err, IsNil)
 
 	var sum uint64
-	idx := d.Index().ToIdxFile()
-	for _, e := range idx.Entries {
+	iter, err := d.Index().Entries()
+	c.Assert(err, IsNil)
+
+	for {
+		e, err := iter.Next()
+		if err == io.EOF {
+			break
+		}
+
+		c.Assert(err, IsNil)
 		sum += uint64(e.CRC32)
 	}
 
@@ -349,12 +353,30 @@ func (s *ReaderSuite) TestIndex(c *C) {
 	d, err := packfile.NewDecoder(scanner, nil)
 	c.Assert(err, IsNil)
 
-	c.Assert(d.Index().ToIdxFile().Entries, HasLen, 0)
+	c.Assert(indexEntries(c, d), Equals, 0)
 
 	_, err = d.Decode()
 	c.Assert(err, IsNil)
 
-	c.Assert(len(d.Index().ToIdxFile().Entries), Equals, 31)
+	c.Assert(indexEntries(c, d), Equals, 31)
+}
+
+func indexEntries(c *C, d *packfile.Decoder) int {
+	var count int
+	entries, err := d.Index().Entries()
+	c.Assert(err, IsNil)
+
+	for {
+		_, err := entries.Next()
+		if err == io.EOF {
+			break
+		}
+
+		c.Assert(err, IsNil)
+		count++
+	}
+
+	return count
 }
 
 func (s *ReaderSuite) TestSetIndex(c *C) {
@@ -363,18 +385,25 @@ func (s *ReaderSuite) TestSetIndex(c *C) {
 	d, err := packfile.NewDecoder(scanner, nil)
 	c.Assert(err, IsNil)
 
-	idx := packfile.NewIndex(1)
+	w := new(idxfile.Writer)
 	h := plumbing.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5")
-	idx.Add(h, uint64(42), 0)
+	w.Add(h, uint64(42), 0)
+	w.OnFooter(plumbing.ZeroHash)
+
+	var idx idxfile.Index
+	idx, err = w.Index()
+	c.Assert(err, IsNil)
 	d.SetIndex(idx)
 
-	idxf := d.Index().ToIdxFile()
-	c.Assert(idxf.Entries, HasLen, 1)
-	c.Assert(idxf.Entries[0].Offset, Equals, uint64(42))
-}*/
+	idx = d.Index()
+	c.Assert(indexEntries(c, d), Equals, 1)
 
-func assertObjects(c *C, s storer.EncodedObjectStorer, expects []string) {
+	offset, err := idx.FindOffset(h)
+	c.Assert(err, IsNil)
+	c.Assert(offset, Equals, int64(42))
+}
 
+func assertObjects(c *C, s storer.EncodedObjectStorer, expects []string) {
 	i, err := s.IterEncodedObjects(plumbing.AnyObject)
 	c.Assert(err, IsNil)
 
@@ -390,13 +419,12 @@ func assertObjects(c *C, s storer.EncodedObjectStorer, expects []string) {
 	}
 }
 
-/*
-func getIndexFromIdxFile(r io.Reader) *packfile.Index {
-	idxf := idxfile.NewIdxfile()
+func getIndexFromIdxFile(r io.Reader) idxfile.Index {
+	idxf := idxfile.NewMemoryIndex()
 	d := idxfile.NewDecoder(r)
 	if err := d.Decode(idxf); err != nil {
 		panic(err)
 	}
 
-	return packfile.NewIndexFromIdxFile(idxf)
-}*/
+	return idxf
+}
diff --git a/plumbing/format/packfile/packfile.go b/plumbing/format/packfile/packfile.go
index 00014f6..2e831f2 100644
--- a/plumbing/format/packfile/packfile.go
+++ b/plumbing/format/packfile/packfile.go
@@ -3,6 +3,7 @@ package packfile
 import (
 	"bytes"
 	"io"
+	"os"
 
 	billy "gopkg.in/src-d/go-billy.v4"
 	"gopkg.in/src-d/go-git.v4/plumbing"
@@ -55,6 +56,10 @@ func (p *Packfile) GetByOffset(o int64) (plumbing.EncodedObject, error) {
 	}
 
 	if _, err := p.s.SeekFromStart(o); err != nil {
+		if err == io.EOF || isInvalid(err) {
+			return nil, plumbing.ErrObjectNotFound
+		}
+
 		return nil, err
 	}
 
@@ -187,6 +192,9 @@ func (p *Packfile) getObjectType(h *ObjectHeader) (typ plumbing.ObjectType, err
 func (p *Packfile) nextObject() (plumbing.EncodedObject, error) {
 	h, err := p.nextObjectHeader()
 	if err != nil {
+		if err == io.EOF || isInvalid(err) {
+			return nil, plumbing.ErrObjectNotFound
+		}
 		return nil, err
 	}
 
@@ -403,3 +411,21 @@ func (i *objectIter) ForEach(f func(plumbing.EncodedObject) error) error {
 func (i *objectIter) Close() {
 	i.iter.Close()
 }
+
+// isInvalid checks whether an error is an os.PathError with an os.ErrInvalid
+// error inside. It also checks for the windows error, which is different from
+// os.ErrInvalid.
+func isInvalid(err error) bool {
+	pe, ok := err.(*os.PathError)
+	if !ok {
+		return false
+	}
+
+	errstr := pe.Err.Error()
+	return errstr == errInvalidUnix || errstr == errInvalidWindows
+}
+
+// errInvalidWindows is the Windows equivalent to os.ErrInvalid
+const errInvalidWindows = "The parameter is incorrect."
+
+var errInvalidUnix = os.ErrInvalid.Error()
diff --git a/plumbing/format/packfile/packfile_test.go b/plumbing/format/packfile/packfile_test.go
index 0d7a806..e234794 100644
--- a/plumbing/format/packfile/packfile_test.go
+++ b/plumbing/format/packfile/packfile_test.go
@@ -45,7 +45,7 @@ func (s *PackfileSuite) TestGetByOffset(c *C) {
 	}
 
 	_, err := s.p.GetByOffset(math.MaxInt64)
-	c.Assert(err, Equals, io.EOF)
+	c.Assert(err, Equals, plumbing.ErrObjectNotFound)
 }
 
 func (s *PackfileSuite) TestID(c *C) {
@@ -109,7 +109,7 @@ var expectedEntries = map[plumbing.Hash]int64{
 }
 
 func (s *PackfileSuite) TestContent(c *C) {
-	storer := memory.NewObjectStorage()
+	storer := memory.NewStorage()
 	decoder, err := NewDecoder(NewScanner(s.f.Packfile()), storer)
 	c.Assert(err, IsNil)
 
@@ -153,7 +153,7 @@ func (s *PackfileSuite) TestContent(c *C) {
 func (s *PackfileSuite) SetUpTest(c *C) {
 	s.f = fixtures.Basic().One()
 
-	f, err := osfs.New("/").Open(s.f.Packfile().Name())
+	f, err := osfs.New("").Open(s.f.Packfile().Name())
 	c.Assert(err, IsNil)
 
 	s.idx = idxfile.NewMemoryIndex()
diff --git a/storage/memory/storage.go b/storage/memory/storage.go
index a950a62..2e32509 100644
--- a/storage/memory/storage.go
+++ b/storage/memory/storage.go
@@ -91,16 +91,6 @@ type ObjectStorage struct {
 	Tags    map[plumbing.Hash]plumbing.EncodedObject
 }
 
-func NewObjectStorage() *ObjectStorage {
-	return &ObjectStorage{
-		Objects: make(map[plumbing.Hash]plumbing.EncodedObject),
-		Commits: make(map[plumbing.Hash]plumbing.EncodedObject),
-		Trees:   make(map[plumbing.Hash]plumbing.EncodedObject),
-		Blobs:   make(map[plumbing.Hash]plumbing.EncodedObject),
-		Tags:    make(map[plumbing.Hash]plumbing.EncodedObject),
-	}
-}
-
 func (o *ObjectStorage) NewEncodedObject() plumbing.EncodedObject {
 	return &plumbing.MemoryObject{}
 }