Merge pull request #994 from epiclabs-io/fix-thin-pack

plumbing/format/packfile: Fix broken "thin" packfile support. Fixes #991

2 files changed, 95 insertions, 47 deletions

diff --git a/plumbing/format/packfile/parser.go b/plumbing/format/packfile/parser.go
index 28582b5..5a62d63 100644
--- a/plumbing/format/packfile/parser.go
+++ b/plumbing/format/packfile/parser.go
@@ -38,15 +38,14 @@ type Observer interface {
 // Parser decodes a packfile and calls any observer associated to it. Is used
 // to generate indexes.
 type Parser struct {
-	storage          storer.EncodedObjectStorer
-	scanner          *Scanner
-	count            uint32
-	oi               []*objectInfo
-	oiByHash         map[plumbing.Hash]*objectInfo
-	oiByOffset       map[int64]*objectInfo
-	hashOffset       map[plumbing.Hash]int64
-	pendingRefDeltas map[plumbing.Hash][]*objectInfo
-	checksum         plumbing.Hash
+	storage    storer.EncodedObjectStorer
+	scanner    *Scanner
+	count      uint32
+	oi         []*objectInfo
+	oiByHash   map[plumbing.Hash]*objectInfo
+	oiByOffset map[int64]*objectInfo
+	hashOffset map[plumbing.Hash]int64
+	checksum   plumbing.Hash
 
 	cache *cache.BufferLRU
 	// delta content by offset, only used if source is not seekable
@@ -78,13 +77,12 @@ func NewParserWithStorage(
 	}
 
 	return &Parser{
-		storage:          storage,
-		scanner:          scanner,
-		ob:               ob,
-		count:            0,
-		cache:            cache.NewBufferLRUDefault(),
-		pendingRefDeltas: make(map[plumbing.Hash][]*objectInfo),
-		deltas:           deltas,
+		storage: storage,
+		scanner: scanner,
+		ob:      ob,
+		count:   0,
+		cache:   cache.NewBufferLRUDefault(),
+		deltas:  deltas,
 	}, nil
 }
 
@@ -150,10 +148,6 @@ func (p *Parser) Parse() (plumbing.Hash, error) {
 		return plumbing.ZeroHash, err
 	}
 
-	if len(p.pendingRefDeltas) > 0 {
-		return plumbing.ZeroHash, ErrReferenceDeltaNotFound
-	}
-
 	if err := p.onFooter(p.checksum); err != nil {
 		return plumbing.ZeroHash, err
 	}
@@ -205,18 +199,21 @@ func (p *Parser) indexObjects() error {
 			parent.Children = append(parent.Children, ota)
 		case plumbing.REFDeltaObject:
 			delta = true
-
 			parent, ok := p.oiByHash[oh.Reference]
-			if ok {
-				ota = newDeltaObject(oh.Offset, oh.Length, t, parent)
-				parent.Children = append(parent.Children, ota)
-			} else {
-				ota = newBaseObject(oh.Offset, oh.Length, t)
-				p.pendingRefDeltas[oh.Reference] = append(
-					p.pendingRefDeltas[oh.Reference],
-					ota,
-				)
+			if !ok {
+				// can't find referenced object in this pack file
+				// this must be a "thin" pack.
+				parent = &objectInfo{ //Placeholder parent
+					SHA1:        oh.Reference,
+					ExternalRef: true, // mark as an external reference that must be resolved
+					Type:        plumbing.AnyObject,
+					DiskType:    plumbing.AnyObject,
+				}
+				p.oiByHash[oh.Reference] = parent
 			}
+			ota = newDeltaObject(oh.Offset, oh.Length, t, parent)
+			parent.Children = append(parent.Children, ota)
+
 		default:
 			ota = newBaseObject(oh.Offset, oh.Length, t)
 		}
@@ -297,16 +294,20 @@ func (p *Parser) resolveDeltas() error {
 	return nil
 }
 
-func (p *Parser) get(o *objectInfo) ([]byte, error) {
-	b, ok := p.cache.Get(o.Offset)
+func (p *Parser) get(o *objectInfo) (b []byte, err error) {
+	var ok bool
+	if !o.ExternalRef { // skip cache check for placeholder parents
+		b, ok = p.cache.Get(o.Offset)
+	}
+
 	// If it's not on the cache and is not a delta we can try to find it in the
-	// storage, if there's one.
+	// storage, if there's one. External refs must enter here.
 	if !ok && p.storage != nil && !o.Type.IsDelta() {
-		var err error
 		e, err := p.storage.EncodedObject(plumbing.AnyObject, o.SHA1)
 		if err != nil {
 			return nil, err
 		}
+		o.Type = e.Type()
 
 		r, err := e.Reader()
 		if err != nil {
@@ -323,6 +324,11 @@ func (p *Parser) get(o *objectInfo) ([]byte, error) {
 		return b, nil
 	}
 
+	if o.ExternalRef {
+		// we were not able to resolve a ref in a thin pack
+		return nil, ErrReferenceDeltaNotFound
+	}
+
 	var data []byte
 	if o.DiskType.IsDelta() {
 		base, err := p.get(o.Parent)
@@ -335,7 +341,6 @@ func (p *Parser) get(o *objectInfo) ([]byte, error) {
 			return nil, err
 		}
 	} else {
-		var err error
 		data, err = p.readData(o)
 		if err != nil {
 			return nil, err
@@ -367,14 +372,6 @@ func (p *Parser) resolveObject(
 		return nil, err
 	}
 
-	if pending, ok := p.pendingRefDeltas[o.SHA1]; ok {
-		for _, po := range pending {
-			po.Parent = o
-			o.Children = append(o.Children, po)
-		}
-		delete(p.pendingRefDeltas, o.SHA1)
-	}
-
 	if p.storage != nil {
 		obj := new(plumbing.MemoryObject)
 		obj.SetSize(o.Size())
@@ -447,10 +444,11 @@ func getSHA1(t plumbing.ObjectType, data []byte) (plumbing.Hash, error) {
 }
 
 type objectInfo struct {
-	Offset   int64
-	Length   int64
-	Type     plumbing.ObjectType
-	DiskType plumbing.ObjectType
+	Offset      int64
+	Length      int64
+	Type        plumbing.ObjectType
+	DiskType    plumbing.ObjectType
+	ExternalRef bool // indicates this is an external reference in a thin pack file
 
 	Crc32 uint32
 
diff --git a/plumbing/format/packfile/parser_test.go b/plumbing/format/packfile/parser_test.go
index 012a140..6e7c84b 100644
--- a/plumbing/format/packfile/parser_test.go
+++ b/plumbing/format/packfile/parser_test.go
@@ -1,10 +1,13 @@
 package packfile_test
 
 import (
+	"io"
 	"testing"
 
+	git "gopkg.in/src-d/go-git.v4"
 	"gopkg.in/src-d/go-git.v4/plumbing"
 	"gopkg.in/src-d/go-git.v4/plumbing/format/packfile"
+	"gopkg.in/src-d/go-git.v4/plumbing/storer"
 
 	. "gopkg.in/check.v1"
 	"gopkg.in/src-d/go-git-fixtures.v3"
@@ -74,6 +77,53 @@ func (s *ParserSuite) TestParserHashes(c *C) {
 	c.Assert(obs.objects, DeepEquals, objs)
 }
 
+func (s *ParserSuite) TestThinPack(c *C) {
+
+	// Initialize an empty repository
+	fs, err := git.PlainInit(c.MkDir(), true)
+	c.Assert(err, IsNil)
+
+	// Try to parse a thin pack without having the required objects in the repo to
+	// see if the correct errors are returned
+	thinpack := fixtures.ByTag("thinpack").One()
+	scanner := packfile.NewScanner(thinpack.Packfile())
+	parser, err := packfile.NewParserWithStorage(scanner, fs.Storer) // ParserWithStorage writes to the storer all parsed objects!
+	c.Assert(err, IsNil)
+
+	_, err = parser.Parse()
+	c.Assert(err, Equals, plumbing.ErrObjectNotFound)
+
+	// start over with a clean repo
+	fs, err = git.PlainInit(c.MkDir(), true)
+	c.Assert(err, IsNil)
+
+	// Now unpack a base packfile into our empty repo:
+	f := fixtures.ByURL("https://github.com/spinnaker/spinnaker.git").One()
+	w, err := fs.Storer.(storer.PackfileWriter).PackfileWriter()
+	c.Assert(err, IsNil)
+	_, err = io.Copy(w, f.Packfile())
+	c.Assert(err, IsNil)
+	w.Close()
+
+	// Check that the test object that will come with our thin pack is *not* in the repo
+	_, err = fs.Storer.EncodedObject(plumbing.CommitObject, thinpack.Head)
+	c.Assert(err, Equals, plumbing.ErrObjectNotFound)
+
+	// Now unpack the thin pack:
+	scanner = packfile.NewScanner(thinpack.Packfile())
+	parser, err = packfile.NewParserWithStorage(scanner, fs.Storer) // ParserWithStorage writes to the storer all parsed objects!
+	c.Assert(err, IsNil)
+
+	h, err := parser.Parse()
+	c.Assert(err, IsNil)
+	c.Assert(h, Equals, plumbing.NewHash("1288734cbe0b95892e663221d94b95de1f5d7be8"))
+
+	// Check that our test object is now accessible
+	_, err = fs.Storer.EncodedObject(plumbing.CommitObject, thinpack.Head)
+	c.Assert(err, IsNil)
+
+}
+
 type observerObject struct {
 	hash   string
 	otype  plumbing.ObjectType