Merge pull request #300 from alcortesm/improvement-difftree-simplify-deprecated

difftree: simplify hash comparison with deprecated files modes

3 files changed, 31 insertions, 51 deletions

diff --git a/plumbing/object/difftree.go b/plumbing/object/difftree.go
index bcc78ad..3bc29cb 100644
--- a/plumbing/object/difftree.go
+++ b/plumbing/object/difftree.go
@@ -3,7 +3,6 @@ package object
 import (
 	"bytes"
 
-	"srcd.works/go-git.v4/plumbing/filemode"
 	"srcd.works/go-git.v4/utils/merkletrie"
 	"srcd.works/go-git.v4/utils/merkletrie/noder"
 )
@@ -14,6 +13,10 @@ func DiffTree(a, b *Tree) (Changes, error) {
 	from := newTreeNoder(a)
 	to := newTreeNoder(b)
 
+	hashEqual := func(a, b noder.Hasher) bool {
+		return bytes.Equal(a.Hash(), b.Hash())
+	}
+
 	merkletrieChanges, err := merkletrie.DiffTree(from, to, hashEqual)
 	if err != nil {
 		return nil, err
@@ -21,39 +24,3 @@ func DiffTree(a, b *Tree) (Changes, error) {
 
 	return newChanges(merkletrieChanges)
 }
-
-// check if the hash of the contents is different, if not, check if
-// the permissions are different (but taking into account deprecated
-// file modes).  On a treenoder, the hash of the contents is codified
-// in the first 20 bytes of the data returned by Hash() and the last
-// 4 bytes is the mode.
-func hashEqual(a, b noder.Hasher) bool {
-	hashA, hashB := a.Hash(), b.Hash()
-	contentsA, contentsB := hashA[:20], hashB[:20]
-
-	sameContents := bytes.Equal(contentsA, contentsB)
-	if !sameContents {
-		return false
-	}
-
-	modeA, modeB := hashA[20:], hashB[20:]
-
-	return equivalentMode(modeA, modeB)
-}
-
-func equivalentMode(a, b []byte) bool {
-	if isFilish(a) && isFilish(b) {
-		return true
-	}
-	return bytes.Equal(a, b)
-}
-
-var (
-	file           = filemode.Regular.Bytes()
-	fileDeprecated = filemode.Deprecated.Bytes()
-)
-
-func isFilish(b []byte) bool {
-	return bytes.Equal(b, file) ||
-		bytes.Equal(b, fileDeprecated)
-}
diff --git a/plumbing/object/difftree_test.go b/plumbing/object/difftree_test.go
index fa996c2..11f6b23 100644
--- a/plumbing/object/difftree_test.go
+++ b/plumbing/object/difftree_test.go
@@ -358,24 +358,27 @@ func (s *DiffTreeSuite) TestDiffTree(c *C) {
 }
 
 func (s *DiffTreeSuite) TestIssue279(c *C) {
-	// HashEqual should ignore files if the only change is from a 100664
-	// mode to a 100644 or vice versa.
-	from := &treeNoder{
-		hash: plumbing.NewHash("d08e895238bac36d8220586fdc28c27e1a7a76d3"),
+	// treeNoders should have the same hash when their mode is
+	// filemode.Deprecated and filemode.Regular.
+	a := &treeNoder{
+		hash: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"),
 		mode: filemode.Regular,
 	}
-	to := &treeNoder{
-		hash: plumbing.NewHash("d08e895238bac36d8220586fdc28c27e1a7a76d3"),
-		mode: filemode.Regular,
+	b := &treeNoder{
+		hash: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"),
+		mode: filemode.Deprecated,
 	}
-	c.Assert(hashEqual(from, to), Equals, true)
-	c.Assert(hashEqual(to, from), Equals, true)
+	c.Assert(a.Hash(), DeepEquals, b.Hash())
 
-	// but should detect if the contents of the file also changed.
-	to = &treeNoder{
-		hash: plumbing.NewHash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"),
+	// yet, they should have different hashes if their contents change.
+	aa := &treeNoder{
+		hash: plumbing.NewHash("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"),
 		mode: filemode.Regular,
 	}
-	c.Assert(hashEqual(from, to), Equals, false)
-	c.Assert(hashEqual(to, from), Equals, false)
+	c.Assert(a.Hash(), Not(DeepEquals), aa.Hash())
+	bb := &treeNoder{
+		hash: plumbing.NewHash("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"),
+		mode: filemode.Deprecated,
+	}
+	c.Assert(b.Hash(), Not(DeepEquals), bb.Hash())
 }
diff --git a/plumbing/object/treenoder.go b/plumbing/object/treenoder.go
index 89fcdb1..80cd9b0 100644
--- a/plumbing/object/treenoder.go
+++ b/plumbing/object/treenoder.go
@@ -45,7 +45,17 @@ func (t *treeNoder) String() string {
 	return "treeNoder <" + t.name + ">"
 }
 
+// The hash of a treeNoder is the result of concatenating the hash of
+// its contents and its mode; that way the difftree algorithm will
+// detect changes in the contents of files and also in their mode.
+//
+// Files with Regular and Deprecated file modes are considered the same
+// for the purpose of difftree, so Regular will be used as the mode for
+// Deprecated files here.
 func (t *treeNoder) Hash() []byte {
+	if t.mode == filemode.Deprecated {
+		return append(t.hash[:], filemode.Regular.Bytes()...)
+	}
 	return append(t.hash[:], t.mode.Bytes()...)
 }