Added tests for all failure modes of submodule add ( except for one ), and fixed a few issues on the way

Author: Byron

Diff for: lib/git/objects/submodule.py

@@ -1,11 +1,11 @@
 import base
 from util import Traversable
 from StringIO import StringIO					# need a dict to set bloody .name field
-from git.util import Iterable
+from git.util import Iterable, join_path_native, to_native_path_linux
 from git.config import GitConfigParser, SectionConstraint
-from git.util import join_path_native
 from git.exc import InvalidGitRepositoryError, NoSuchPathError
 import stat
+import git
 
 import os
 import sys
@@ -87,6 +87,7 @@ class Submodule(base.IndexObject, Iterable, Traversable):
 	type = 'submodule'
 
 	__slots__ = ('_parent_commit', '_url', '_branch', '_name', '__weakref__')
+	_cache_attrs = ('path', '_url', '_branch')
 
 	def __init__(self, repo, binsha, mode=None, path=None, name = None, parent_commit=None, url=None, branch=None):
 		"""Initialize this instance with its attributes. We only document the ones 
@@ -178,7 +179,7 @@ def _config_parser(cls, repo, parent_commit, read_only):
 
 	def _clear_cache(self):
 		# clear the possibly changed values
-		for name in ('path', '_branch', '_url'):
+		for name in self._cache_attrs:
 			try:
 				delattr(self, name)
 			except AttributeError:
@@ -235,18 +236,19 @@ def add(cls, repo, name, path, url=None, branch=k_head_default, no_checkout=Fals
 			path = path[:-1]
 		# END handle trailing slash
 
+		# INSTANTIATE INTERMEDIATE SM
 		sm = cls(repo, cls.NULL_BIN_SHA, cls.k_def_mode, path, name)
 		if sm.exists():
 			# reretrieve submodule from tree
 			return repo.head.commit.tree[path]
 		# END handle existing
 
-		branch = Head(repo, head.to_full_path(branch))
+		branch = git.Head(repo, git.Head.to_full_path(branch))
 		has_module = sm.module_exists()
 		branch_is_default = branch.name == cls.k_head_default
 		if has_module and url is not None:
 			if url not in [r.url for r in sm.module().remotes]:
-				raise ValueError("Specified URL %s does not match any remote url of the repository at %s" % (url, sm.module_path()))
+				raise ValueError("Specified URL '%s' does not match any remote url of the repository at '%s'" % (url, sm.module_path()))
 			# END check url
 		# END verify urls match
 
@@ -611,14 +613,30 @@ def exists(self):
 		""":return: True if the submodule exists, False otherwise. Please note that
 		a submodule may exist (in the .gitmodules file) even though its module
 		doesn't exist"""
+		# keep attributes for later, and restore them if we have no valid data
+		# this way we do not actually alter the state of the object
+		loc = locals()
+		for attr in self._cache_attrs:
+			if hasattr(self, attr):
+				loc[attr] = getattr(self, attr)
+			# END if we have the attribute cache
+		#END for each attr
 		self._clear_cache()
+		
 		try:
-			self.path
-			return True
-		except Exception:
-			# we raise if the path cannot be restored from configuration
-			return False
-		# END handle exceptions
+			try:
+				self.path
+				return True
+			except Exception:
+				return False
+			# END handle exceptions
+		finally:
+			for attr in self._cache_attrs:
+				if attr in loc:
+					setattr(self, attr, loc[attr])
+				# END if we have a cache
+			# END reapply each attribute
+		# END handle object state consistency
 
 	@property
 	def branch(self):

Diff for: test/git/test_submodule.py

@@ -99,6 +99,21 @@ def _do_base_tests(self, rwrepo):
 			# currently there is only one submodule
 			assert len(list(rwrepo.iter_submodules())) == 1
 
+			# TEST ADD
+			###########
+			# preliminary tests
+			# adding existing returns exactly the existing
+			sma = Submodule.add(rwrepo, sm.name, sm.path)
+			assert sma.path == sm.path
+			
+			# no url and no module at path fails
+			self.failUnlessRaises(ValueError, Submodule.add, rwrepo, "newsubm", "pathtorepo", url=None)
+			
+			# TODO: Test no remote url in existing repository
+			
+			# CONTINUE UPDATE
+			#################
+			
 			# lets update it - its a recursive one too
 			newdir = os.path.join(sm.module_path(), 'dir')
 			os.makedirs(newdir)
@@ -112,6 +127,15 @@ def _do_base_tests(self, rwrepo):
 			assert isinstance(sm.module(), git.Repo)
 			assert sm.module().working_tree_dir == sm.module_path()
 
+			# INTERLEAVE ADD TEST
+			#####################
+			# url must match the one in the existing repository ( if submodule name suggests a new one )
+			# or we raise
+			self.failUnlessRaises(ValueError, Submodule.add, rwrepo, "newsubm", sm.path, "git://someurl/repo.git")
+			
+			
+			# CONTINUE UPDATE
+			#################
 			# we should have setup a tracking branch, which is also active
 			assert sm.module().head.ref.tracking_branch() is not None