Merge pull request #324 from ajnavarro/fix/changes-file-for-non-files-no-error

object: fix Change.Files() method behavior (fix #317)

Author: mcuadros

plumbing/object/change.go

@@ -46,13 +46,21 @@ func (c *Change) Files() (from, to *File, err error) {
 
 	if action == merkletrie.Insert || action == merkletrie.Modify {
 		to, err = c.To.Tree.TreeEntryFile(&c.To.TreeEntry)
+		if !c.To.TreeEntry.Mode.IsFile() {
+			return nil, nil, nil
+		}
+
 		if err != nil {
 			return
 		}
 	}
 
 	if action == merkletrie.Delete || action == merkletrie.Modify {
 		from, err = c.From.Tree.TreeEntryFile(&c.From.TreeEntry)
+		if !c.From.TreeEntry.Mode.IsFile() {
+			return nil, nil, nil
+		}
+
 		if err != nil {
 			return
 		}

plumbing/object/change_adaptor_test.go

@@ -214,6 +214,7 @@ func (s *ChangeAdaptorSuite) TestChangeFilesInsert(c *C) {
 	change := Change{}
 	change.To.Name = "json/long.json"
 	change.To.Tree = tree
+	change.To.TreeEntry.Mode = filemode.Regular
 	change.To.TreeEntry.Hash = plumbing.NewHash("49c6bb89b17060d7b4deacb7b338fcc6ea2352a9")
 
 	from, to, err := change.Files()
@@ -228,6 +229,7 @@ func (s *ChangeAdaptorSuite) TestChangeFilesInsertNotFound(c *C) {
 	change := Change{}
 	change.To.Name = "json/long.json"
 	change.To.Tree = tree
+	change.To.TreeEntry.Mode = filemode.Regular
 	// there is no object for this hash
 	change.To.TreeEntry.Hash = plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")
 
@@ -241,6 +243,7 @@ func (s *ChangeAdaptorSuite) TestChangeFilesDelete(c *C) {
 	change := Change{}
 	change.From.Name = "json/long.json"
 	change.From.Tree = tree
+	change.From.TreeEntry.Mode = filemode.Regular
 	change.From.TreeEntry.Hash = plumbing.NewHash("49c6bb89b17060d7b4deacb7b338fcc6ea2352a9")
 
 	from, to, err := change.Files()
@@ -255,6 +258,7 @@ func (s *ChangeAdaptorSuite) TestChangeFilesDeleteNotFound(c *C) {
 	change := Change{}
 	change.From.Name = "json/long.json"
 	change.From.Tree = tree
+	change.From.TreeEntry.Mode = filemode.Regular
 	// there is no object for this hash
 	change.From.TreeEntry.Hash = plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")
 
@@ -268,9 +272,11 @@ func (s *ChangeAdaptorSuite) TestChangeFilesModify(c *C) {
 	change := Change{}
 	change.To.Name = "json/long.json"
 	change.To.Tree = tree
+	change.To.TreeEntry.Mode = filemode.Regular
 	change.To.TreeEntry.Hash = plumbing.NewHash("49c6bb89b17060d7b4deacb7b338fcc6ea2352a9")
 	change.From.Name = "json/long.json"
 	change.From.Tree = tree
+	change.From.TreeEntry.Mode = filemode.Regular
 	change.From.TreeEntry.Hash = plumbing.NewHash("9a48f23120e880dfbe41f7c9b7b708e9ee62a492")
 
 	from, to, err := change.Files()

plumbing/object/change_test.go

@@ -198,6 +198,54 @@ func (s *ChangeSuite) TestEmptyChangeFails(c *C) {
 	c.Assert(str, Equals, "malformed change")
 }
 
+// test reproducing bug #317
+func (s *ChangeSuite) TestNoFileFilemodes(c *C) {
+	s.Suite.SetUpSuite(c)
+	f := fixtures.ByURL("https://github.com/git-fixtures/submodule.git").One()
+
+	sto, err := filesystem.NewStorage(f.DotGit())
+	c.Assert(err, IsNil)
+
+	iter, err := sto.IterEncodedObjects(plumbing.AnyObject)
+	c.Assert(err, IsNil)
+	var commits []*Commit
+	iter.ForEach(func(o plumbing.EncodedObject) error {
+		if o.Type() == plumbing.CommitObject {
+			commit, err := GetCommit(sto, o.Hash())
+			c.Assert(err, IsNil)
+			commits = append(commits, commit)
+
+		}
+
+		return nil
+	})
+
+	c.Assert(len(commits), Not(Equals), 0)
+
+	var prev *Commit
+	for _, commit := range commits {
+		if prev == nil {
+			prev = commit
+			continue
+		}
+		tree, err := commit.Tree()
+		c.Assert(err, IsNil)
+		prevTree, err := prev.Tree()
+		c.Assert(err, IsNil)
+		changes, err := DiffTree(tree, prevTree)
+		c.Assert(err, IsNil)
+		for _, change := range changes {
+			_, _, err := change.Files()
+			if err != nil {
+				panic(err)
+				c.Assert(err, IsNil)
+			}
+		}
+
+		prev = commit
+	}
+}
+
 func (s *ChangeSuite) TestErrorsFindingChildsAreDetected(c *C) {
 	// Commit 7beaad711378a4daafccc2c04bc46d36df2a0fd1 of the go-git
 	// fixture modified "examples/latest/latest.go".