Improved GitConfigurationParser to better deal with streams and the corresponding locks. Submodule class now operates on parent_commits, the configuration is either streamed from the repository or written directly into a blob ( or file ) dependending on whether we have a working tree checkout or not which matches our parent_commit

Author: Byron

Diff for: lib/git/config.py

@@ -271,9 +271,9 @@ def read(self):
 			if not hasattr(file_object, "seek"):
 				try:
 					fp = open(file_object)
+					close_fp = True
 				except IOError,e:
 					continue
-				close_fp = True
 			# END fp handling
 
 			try:
@@ -308,17 +308,21 @@ def write(self):
 		:raise IOError: if this is a read-only writer instance or if we could not obtain 
 			a file lock"""
 		self._assure_writable("write")
-		self._lock._obtain_lock()
-		
 
 		fp = self._file_or_files
 		close_fp = False
 
+		# we have a physical file on disk, so get a lock
+		if isinstance(fp, (basestring, file)):
+			self._lock._obtain_lock()
+		# END get lock for physical files
+		
 		if not hasattr(fp, "seek"):
 			fp = open(self._file_or_files, "w")
 			close_fp = True
 		else:
 			fp.seek(0)
+		# END handle stream or file
 
 		# WRITE DATA
 		try:
@@ -390,7 +394,7 @@ def get_value(self, section, option, default = None):
 		return valuestr
 
 	@needs_values
-	@set_dirty_and_flush_changes	
+	@set_dirty_and_flush_changes
 	def set_value(self, section, option, value):
 		"""Sets the given option in section to the given value.
 		It will create the section if required, and will not throw as opposed to the default 

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

@@ -6,6 +6,13 @@
 
 __all__ = ("Submodule", )
 
+class SubmoduleConfigParser(GitConfigParser):
+	"""Catches calls to _write, and updates the .gitmodules blob in the index
+	with the new data, if we have written into a stream. Otherwise it will 
+	add the local file to the index to make it correspond with the working tree."""
+	_mutating_methods_ = tuple()
+	
+
 class Submodule(base.IndexObject):
 	"""Implements access to a git submodule. They are special in that their sha
 	represents a commit in the submodule's repository which is to be checked out
@@ -20,14 +27,14 @@ class Submodule(base.IndexObject):
 	# this is a bogus type for base class compatability
 	type = 'submodule'
 
-	__slots__ = ('_root_tree', '_url', '_ref')
+	__slots__ = ('_parent_commit', '_url', '_ref')
 
 	def _set_cache_(self, attr):
 		if attr == 'size':
 			raise ValueError("Submodules do not have a size as they do not refer to anything in this repository")
-		elif attr == '_root_tree':
+		elif attr == '_parent_commit':
 			# set a default value, which is the root tree of the current head
-			self._root_tree = self.repo.tree()
+			self._parent_commit = self.repo.commit()
 		elif attr in ('path', '_url', '_ref'):
 			reader = self.config_reader()
 			# default submodule values
@@ -39,13 +46,26 @@ def _set_cache_(self, attr):
 			super(Submodule, self)._set_cache_(attr)
 		# END handle attribute name
 
-	def _fp_config(self):
+	def _sio_modules(self):
 		""":return: Configuration file as StringIO - we only access it through the respective blob's data"""
-		return StringIO(self._root_tree[self.kModulesFile].datastream.read())
+		sio = StringIO(self._parent_commit.tree[self.kModulesFile].datastream.read())
+		sio.name = self.kModulesFile
+		return sio
 
 	def _config_parser(self, read_only):
 		""":return: Config Parser constrained to our submodule in read or write mode"""
-		parser = GitConfigParser(self._fp_config(), read_only = read_only)
+		parent_matches_head = self.repo.head.commit == self._parent_commit
+		if not self.repo.bare and parent_matches_head:
+			fp_module = self.kModulesFile
+		else:
+			fp_module = self._sio_modules()
+		# END handle non-bare working tree
+		
+		if not read_only and not parent_matches_head:
+			raise ValueError("Cannot write blobs of 'historical' submodule configurations")
+		# END handle writes of historical submodules
+		
+		parser = GitConfigParser(fp_module, read_only = read_only)
 		return SectionConstraint(parser, 'submodule "%s"' % self.path)
 
 	#{ Edit Interface
@@ -61,21 +81,24 @@ def add(cls, repo, path, url, skip_init=False):
 		:param skip_init: if True, the new repository will not be cloned to its location.
 		:return: The newly created submodule instance"""
 
-	def set_root_tree(self, root_tree):
-		"""Set this instance to use the given tree which is supposed to contain the 
-		.gitmodules blob.
-		:param root_tree: Tree'ish reference pointing at the root_tree
-		:raise ValueError: if the root_tree didn't contain the .gitmodules blob."""
-		tree = self.repo.tree(root_tree)
-		if self.kModulesFile not in tree:
-			raise ValueError("Tree %s did not contain the %s file" % (root_tree, self.kModulesFile))
+	def set_parent_commit(self, commit):
+		"""Set this instance to use the given commit whose tree is supposed to 
+		contain the .gitmodules blob.
+		:param commit: Commit'ish reference pointing at the root_tree
+		:raise ValueError: if the commit's tree didn't contain the .gitmodules blob."""
+		pcommit = self.repo.commit(commit)
+		if self.kModulesFile not in pcommit.tree:
+			raise ValueError("Tree of commit %s did not contain the %s file" % (commit, self.kModulesFile))
 		# END handle exceptions
-		self._root_tree = tree
+		self._parent_commit = pcommit
 
-		# clear the possibly changing values
-		del(self.path)
-		del(self._ref)
-		del(self._url)
+		# clear the possibly changed values
+		for name in ('path', '_ref', '_url'):
+			try:
+				delattr(self, name)
+			except AttributeError:
+				pass
+		# END for each name to delete
 
 	def config_writer(self):
 		""":return: a config writer instance allowing you to read and write the data
@@ -108,11 +131,10 @@ def url(self):
 		""":return: The url to the repository which our module-repository refers to"""
 		return self._url
 
-	def root_tree(self):
-		""":return: Tree instance referring to the tree which contains the .gitmodules file
-		we are to use
-		:note: will always point to the current head's root tree if it was not set explicitly"""
-		return self._root_tree
+	def parent_commit(self):
+		""":return: Commit instance with the tree containing the .gitmodules file
+		:note: will always point to the current head's commit if it was not set explicitly"""
+		return self._parent_commit
 
 	def config_reader(self):
 		""":return: ConfigReader instance which allows you to qurey the configuration values

Diff for: test/git/test_config.py

@@ -14,9 +14,7 @@ class TestBase(TestCase):
 
     def _to_memcache(self, file_path):
         fp = open(file_path, "r")
-        sio = StringIO.StringIO()
-        sio.write(fp.read())
-        sio.seek(0)
+        sio = StringIO.StringIO(fp.read())
         sio.name = file_path
         return sio
 

Diff for: test/git/test_submodule.py

@@ -19,7 +19,9 @@ def _do_base_tests(self, rwrepo):
 		# size is invalid
 		self.failUnlessRaises(ValueError, getattr, sm, 'size')
 
-		# fails if tree has no gitmodule file
+		# set_parent_commit fails if tree has no gitmodule file
+		
+		
 
 		if rwrepo.bare:
 			# module fails
@@ -28,6 +30,8 @@ def _do_base_tests(self, rwrepo):
 			# get the module repository
 			pass
 		# END bare handling
+		
+		# Writing of historical submodule configurations must not work
 
 	@with_rw_repo(kCOTag)
 	def test_base_rw(self, rwrepo):