Diff: fixed bug that caused a string to end up as a blob mode

9 files changed, 111 insertions, 103 deletions

diff --git a/lib/git/diff.py b/lib/git/diff.py
index b8585a4c..79a0c3c1 100644
--- a/lib/git/diff.py
+++ b/lib/git/diff.py
@@ -196,23 +196,24 @@ class Diff(object):
 	def __init__(self, repo, a_path, b_path, a_blob_id, b_blob_id, a_mode,
 				 b_mode, new_file, deleted_file, rename_from,
 				 rename_to, diff):
+	
+		self.a_mode = a_mode
+		self.b_mode = b_mode
+		
+		if self.a_mode:
+			self.a_mode = mode_str_to_int(self.a_mode)
+		if self.b_mode:
+			self.b_mode = mode_str_to_int(self.b_mode)
+		
 		if a_blob_id is None:
 			self.a_blob = None
 		else:
-			self.a_blob = Blob(repo, hex_to_bin(a_blob_id), mode=a_mode, path=a_path)
+			self.a_blob = Blob(repo, hex_to_bin(a_blob_id), mode=self.a_mode, path=a_path)
 		if b_blob_id is None:
 			self.b_blob = None
 		else:
-			self.b_blob = Blob(repo, hex_to_bin(b_blob_id), mode=b_mode, path=b_path)
-
-		self.a_mode = a_mode
-		self.b_mode = b_mode
+			self.b_blob = Blob(repo, hex_to_bin(b_blob_id), mode=self.b_mode, path=b_path)
 		
-		if self.a_mode:
-			self.a_mode = mode_str_to_int( self.a_mode )
-		if self.b_mode:
-			self.b_mode = mode_str_to_int( self.b_mode )
-			
 		self.new_file = new_file
 		self.deleted_file = deleted_file
 		
diff --git a/lib/git/ext/gitdb b/lib/git/ext/gitdb
-Subproject 39b00429999eefc62b70230fb8e0261948a2f31
+Subproject c265c97f9130d2225b923b427736796c0a0d957
diff --git a/lib/git/index/base.py b/lib/git/index/base.py
index b51d6251..03da52b7 100644
--- a/lib/git/index/base.py
+++ b/lib/git/index/base.py
@@ -482,7 +482,7 @@ class IndexFile(LazyMixin, diff.Diffable, Serializable):
 			# delete all possible stages
 			for stage in (1, 2, 3):
 				try:
-					del( self.entries[(blob.path, stage)] )
+					del( self.entries[(blob.path, stage)])
 				except KeyError:
 					pass
 				# END ignore key errors
diff --git a/lib/git/objects/base.py b/lib/git/objects/base.py
index 118bc3ca..97b7898c 100644
--- a/lib/git/objects/base.py
+++ b/lib/git/objects/base.py
@@ -36,6 +36,7 @@ class Object(LazyMixin):
 		super(Object,self).__init__()
 		self.repo = repo
 		self.binsha = binsha
+		assert len(binsha) == 20, "Require 20 byte binary sha, got %r, len = %i" % (binsha, len(binsha))
 
 	@classmethod
 	def new(cls, repo, id):
diff --git a/lib/git/objects/commit.py b/lib/git/objects/commit.py
index f365c994..f88bb0e8 100644
--- a/lib/git/objects/commit.py
+++ b/lib/git/objects/commit.py
@@ -105,6 +105,8 @@ class Commit(base.Object, Iterable, Diffable, Traversable, Serializable):
 			as what time.altzone returns. The sign is inverted compared to git's 
 			UTC timezone."""
 		super(Commit,self).__init__(repo, binsha)
+		if tree is not None:
+			assert isinstance(tree, Tree), "Tree needs to be a Tree instance, was %s" % type(tree)
 		self._set_self_from_args_(locals())
 		
 	@classmethod
diff --git a/lib/git/objects/fun.py b/lib/git/objects/fun.py
index 2d0fd634..e56ef8fe 100644
--- a/lib/git/objects/fun.py
+++ b/lib/git/objects/fun.py
@@ -65,7 +65,6 @@ def tree_entries_from_data(data):
 		i += 1
 		sha = data[i:i+20]
 		i = i + 20
-		
 		out.append((sha, mode, name))
 	# END for each byte in data stream
 	return out
diff --git a/lib/git/remote.py b/lib/git/remote.py
index 94bd285b..9c46a027 100644
--- a/lib/git/remote.py
+++ b/lib/git/remote.py
@@ -384,7 +384,7 @@ class FetchInfo(object):
 				split_token = '...'
 				if control_character == ' ':
 					split_token = split_token[:-1]
-				old_commit = Commit(repo, operation.split(split_token)[0])
+				old_commit = Commit.new(repo, operation.split(split_token)[0])
 			# END handle refspec
 		# END reference flag handling
 		
diff --git a/test/git/performance/test_commit.py b/test/git/performance/test_commit.py
index a951f242..62d409fc 100644
--- a/test/git/performance/test_commit.py
+++ b/test/git/performance/test_commit.py
@@ -82,7 +82,7 @@ class TestPerformance(TestBigRepoRW):
 		nc = 5000
 		st = time()
 		for i in xrange(nc):
-			cm = Commit(	rwrepo, Commit.NULL_HEX_SHA, hc.tree, 
+			cm = Commit(	rwrepo, Commit.NULL_BIN_SHA, hc.tree, 
 							hc.author, hc.authored_date, hc.author_tz_offset, 
 							hc.committer, hc.committed_date, hc.committer_tz_offset, 
 							str(i), parents=hc.parents, encoding=hc.encoding)
diff --git a/test/git/test_diff.py b/test/git/test_diff.py
index a113b992..ade21c1b 100644
--- a/test/git/test_diff.py
+++ b/test/git/test_diff.py
@@ -8,96 +8,101 @@ from test.testlib import *
 from git import *
 
 class TestDiff(TestBase):
-            
-    def _assert_diff_format(self, diffs):
-        # verify that the format of the diff is sane
-        for diff in diffs:
-            if diff.a_blob:
-                assert not diff.a_blob.path.endswith('\n')
-            if diff.b_blob:
-                assert not diff.b_blob.path.endswith('\n')
-        # END for each diff
-        return diffs
-    
-    def test_list_from_string_new_mode(self):
-        output = StringProcessAdapter(fixture('diff_new_mode'))
-        diffs = Diff._index_from_patch_format(self.rorepo, output.stdout)
-        self._assert_diff_format(diffs)
-        
-        assert_equal(1, len(diffs))
-        assert_equal(10, len(diffs[0].diff.splitlines()))
+			
+	def _assert_diff_format(self, diffs):
+		# verify that the format of the diff is sane
+		for diff in diffs:
+			if diff.a_mode:
+				assert isinstance(diff.a_mode, int)
+			if diff.b_mode:
+				assert isinstance(diff.b_mode, int)
+				
+			if diff.a_blob:
+				assert not diff.a_blob.path.endswith('\n')
+			if diff.b_blob:
+				assert not diff.b_blob.path.endswith('\n')
+		# END for each diff
+		return diffs
+	
+	def test_list_from_string_new_mode(self):
+		output = StringProcessAdapter(fixture('diff_new_mode'))
+		diffs = Diff._index_from_patch_format(self.rorepo, output.stdout)
+		self._assert_diff_format(diffs)
+		
+		assert_equal(1, len(diffs))
+		assert_equal(10, len(diffs[0].diff.splitlines()))
 
-    def test_diff_with_rename(self):
-        output = StringProcessAdapter(fixture('diff_rename'))
-        diffs = Diff._index_from_patch_format(self.rorepo, output.stdout)
-        self._assert_diff_format(diffs)
-        
-        assert_equal(1, len(diffs))
+	def test_diff_with_rename(self):
+		output = StringProcessAdapter(fixture('diff_rename'))
+		diffs = Diff._index_from_patch_format(self.rorepo, output.stdout)
+		self._assert_diff_format(diffs)
+		
+		assert_equal(1, len(diffs))
 
-        diff = diffs[0]
-        assert_true(diff.renamed)
-        assert_equal(diff.rename_from, 'AUTHORS')
-        assert_equal(diff.rename_to, 'CONTRIBUTORS')
+		diff = diffs[0]
+		assert_true(diff.renamed)
+		assert_equal(diff.rename_from, 'AUTHORS')
+		assert_equal(diff.rename_to, 'CONTRIBUTORS')
 
-    def test_diff_patch_format(self):
-        # test all of the 'old' format diffs for completness - it should at least
-        # be able to deal with it
-        fixtures = ("diff_2", "diff_2f", "diff_f", "diff_i", "diff_mode_only", 
-                    "diff_new_mode", "diff_numstat", "diff_p", "diff_rename", 
-                    "diff_tree_numstat_root" )
-        
-        for fixture_name in fixtures:
-            diff_proc = StringProcessAdapter(fixture(fixture_name))
-            diffs = Diff._index_from_patch_format(self.rorepo, diff_proc.stdout)
-        # END for each fixture
+	def test_diff_patch_format(self):
+		# test all of the 'old' format diffs for completness - it should at least
+		# be able to deal with it
+		fixtures = ("diff_2", "diff_2f", "diff_f", "diff_i", "diff_mode_only", 
+					"diff_new_mode", "diff_numstat", "diff_p", "diff_rename", 
+					"diff_tree_numstat_root" )
+		
+		for fixture_name in fixtures:
+			diff_proc = StringProcessAdapter(fixture(fixture_name))
+			diffs = Diff._index_from_patch_format(self.rorepo, diff_proc.stdout)
+		# END for each fixture
 
-    def test_diff_interface(self):
-        # test a few variations of the main diff routine
-        assertion_map = dict()
-        for i, commit in enumerate(self.rorepo.iter_commits('0.1.6', max_count=2)):
-            diff_item = commit
-            if i%2 == 0:
-                diff_item = commit.tree
-            # END use tree every second item
-            
-            for other in (None, commit.Index, commit.parents[0]):
-                for paths in (None, "CHANGES", ("CHANGES", "lib")):
-                    for create_patch in range(2):
-                        diff_index = diff_item.diff(other, paths, create_patch)
-                        assert isinstance(diff_index, DiffIndex)
-                        
-                        if diff_index:
-                            self._assert_diff_format(diff_index)
-                            for ct in DiffIndex.change_type:
-                                key = 'ct_%s'%ct
-                                assertion_map.setdefault(key, 0)
-                                assertion_map[key] = assertion_map[key]+len(list(diff_index.iter_change_type(ct)))  
-                            # END for each changetype
-                            
-                            # check entries
-                            diff_set = set()
-                            diff_set.add(diff_index[0])
-                            diff_set.add(diff_index[0])
-                            assert len(diff_set) == 1
-                            assert diff_index[0] == diff_index[0]
-                            assert not (diff_index[0] != diff_index[0])
-                        # END diff index checking 
-                    # END for each patch option
-                # END for each path option
-            # END for each other side
-        # END for each commit
-        
-        # assert we could always find at least one instance of the members we 
-        # can iterate in the diff index - if not this indicates its not working correctly
-        # or our test does not span the whole range of possibilities
-        for key,value in assertion_map.items():
-            assert value, "Did not find diff for %s" % key
-        # END for each iteration type 
-        
-        # test path not existing in the index - should be ignored
-        c = self.rorepo.head.commit
-        cp = c.parents[0]
-        diff_index = c.diff(cp, ["does/not/exist"])
-        assert len(diff_index) == 0
-        
-    
+	def test_diff_interface(self):
+		# test a few variations of the main diff routine
+		assertion_map = dict()
+		for i, commit in enumerate(self.rorepo.iter_commits('0.1.6', max_count=2)):
+			diff_item = commit
+			if i%2 == 0:
+				diff_item = commit.tree
+			# END use tree every second item
+			
+			for other in (None, commit.Index, commit.parents[0]):
+				for paths in (None, "CHANGES", ("CHANGES", "lib")):
+					for create_patch in range(2):
+						diff_index = diff_item.diff(other, paths, create_patch)
+						assert isinstance(diff_index, DiffIndex)
+						
+						if diff_index:
+							self._assert_diff_format(diff_index)
+							for ct in DiffIndex.change_type:
+								key = 'ct_%s'%ct
+								assertion_map.setdefault(key, 0)
+								assertion_map[key] = assertion_map[key]+len(list(diff_index.iter_change_type(ct)))	
+							# END for each changetype
+							
+							# check entries
+							diff_set = set()
+							diff_set.add(diff_index[0])
+							diff_set.add(diff_index[0])
+							assert len(diff_set) == 1
+							assert diff_index[0] == diff_index[0]
+							assert not (diff_index[0] != diff_index[0])
+						# END diff index checking 
+					# END for each patch option
+				# END for each path option
+			# END for each other side
+		# END for each commit
+		
+		# assert we could always find at least one instance of the members we 
+		# can iterate in the diff index - if not this indicates its not working correctly
+		# or our test does not span the whole range of possibilities
+		for key,value in assertion_map.items():
+			assert value, "Did not find diff for %s" % key
+		# END for each iteration type 
+		
+		# test path not existing in the index - should be ignored
+		c = self.rorepo.head.commit
+		cp = c.parents[0]
+		diff_index = c.diff(cp, ["does/not/exist"])
+		assert len(diff_index) == 0
+		
+