Merge branch 'index' into improvements

* index:
  Index: Is now diffable and appears to properly implement diffing against other items as well as the working tree
  default index writing now writes the index of the current repository in a fashion comparable to the native implementation
  Added test for ConcurrentWriteOperation
  utils: Added LockFile including test
  Index now behaves more like the default index if no explicit stream is given. It will lazily read its data on first access

6 files changed, 311 insertions, 93 deletions

diff --git a/lib/git/config.py b/lib/git/config.py
index 6f979c73..ccfbae48 100644
--- a/lib/git/config.py
+++ b/lib/git/config.py
@@ -11,10 +11,12 @@ configuration files
 import re
 import os
 import ConfigParser as cp
-from git.odict import OrderedDict
 import inspect
 import cStringIO
 
+from git.odict import OrderedDict
+from git.utils import LockFile
+
 class _MetaParserBuilder(type):
 	"""
 	Utlity class wrapping base-class methods into decorators that assure read-only properties
@@ -70,7 +72,7 @@ def _set_dirty_and_flush_changes(non_const_func):
 	
 	
 
-class GitConfigParser(cp.RawConfigParser, object):
+class GitConfigParser(cp.RawConfigParser, LockFile):
 	"""
 	Implements specifics required to read git style configuration files.
 	
@@ -100,6 +102,7 @@ class GitConfigParser(cp.RawConfigParser, object):
 	
 	# list of RawConfigParser methods able to change the instance
 	_mutating_methods_ = ("add_section", "remove_section", "remove_option", "set")
+	__slots__ = ("_sections", "_defaults", "_file_or_files", "_read_only","_is_initialized")
 	
 	def __init__(self, file_or_files, read_only=True):
 		"""
@@ -120,7 +123,6 @@ class GitConfigParser(cp.RawConfigParser, object):
 		
 		self._file_or_files = file_or_files
 		self._read_only = read_only
-		self._owns_lock = False
 		self._is_initialized = False
 		
 		
@@ -129,10 +131,11 @@ class GitConfigParser(cp.RawConfigParser, object):
 				raise ValueError("Write-ConfigParsers can operate on a single file only, multiple files have been passed")
 			# END single file check
 			
-			self._file_name = file_or_files
-			if not isinstance(self._file_name, basestring):
-				self._file_name = file_or_files.name
-			# END get filename
+			if not isinstance(file_or_files, basestring):
+				file_or_files = file_or_files.name
+			# END get filename from handle/stream
+			# initialize lock base - we want to write
+			LockFile.__init__(self, file_or_files)
 			
 			self._obtain_lock_or_raise()	
 		# END read-only check
@@ -155,67 +158,6 @@ class GitConfigParser(cp.RawConfigParser, object):
 		finally:
 			self._release_lock()
 	
-	def _lock_file_path(self):
-		"""
-		Return
-			Path to lockfile
-		"""
-		return "%s.lock" % (self._file_name)
-	
-	def _has_lock(self):
-		"""
-		Return
-			True if we have a lock and if the lockfile still exists
-			
-		Raise
-			AssertionError if our lock-file does not exist
-		"""
-		if not self._owns_lock:
-			return False
-		
-		lock_file = self._lock_file_path()
-		try:
-			fp = open(lock_file, "r")
-			pid = int(fp.read())
-			fp.close()
-		except IOError:
-			raise AssertionError("GitConfigParser has a lock but the lock file at %s could not be read" % lock_file)
-		
-		if pid != os.getpid():
-			raise AssertionError("We claim to own the lock at %s, but it was not owned by our process: %i" % (lock_file, os.getpid()))
-		
-		return True
-		
-	def _obtain_lock_or_raise(self):
-		"""
-		Create a lock file as flag for other instances, mark our instance as lock-holder
-		
-		Raise
-			IOError if a lock was already present or a lock file could not be written
-		"""
-		if self._has_lock():
-			return 
-			
-		lock_file = self._lock_file_path()
-		if os.path.exists(lock_file):
-			raise IOError("Lock for file %r did already exist, delete %r in case the lock is illegal" % (self._file_name, lock_file))
-		
-		fp = open(lock_file, "w")
-		fp.write(str(os.getpid()))
-		fp.close()
-		
-		self._owns_lock = True
-		
-	def _release_lock(self):
-		"""
-		Release our lock if we have one
-		"""
-		if not self._has_lock():
-			return 
-			
-		os.remove(self._lock_file_path())
-		self._owns_lock = False
-		
 	def optionxform(self, optionstr):
 		"""
 		Do not transform options in any way when writing
diff --git a/lib/git/diff.py b/lib/git/diff.py
index 9b884502..03e6709c 100644
--- a/lib/git/diff.py
+++ b/lib/git/diff.py
@@ -18,13 +18,18 @@ class Diffable(object):
 	"""
 	__slots__ = tuple()
 	
-	# subclasses provide additional arguments to the git-diff comamnd by supplynig 
-	# them in this tuple
-	_diff_args = tuple()
-	
-	# Temporary standin for Index type until we have a real index type
+	# standin indicating you want to diff against the index
 	class Index(object):
 		pass 
+		
+	def _process_diff_args(self, args):
+		"""
+		Returns
+			possibly altered version of the given args list.
+			Method is called right before git command execution.
+			Subclasses can use it to alter the behaviour of the superclass
+		"""
+		return args
 	
 	def diff(self, other=Index, paths=None, create_patch=False, **kwargs):
 		"""
@@ -60,13 +65,13 @@ class Diffable(object):
 			On a bare repository, 'other' needs to be provided as Index or as 
 			as Tree/Commit, or a git command error will occour
 		"""
-		args = list(self._diff_args[:])
+		args = list()
 		args.append( "--abbrev=40" )		# we need full shas
 		args.append( "--full-index" )		# get full index paths, not only filenames
 		
 		if create_patch:
 			args.append("-p")
-			args.append("-M") # check for renames
+			args.append("-M") 				# check for renames
 		else:
 			args.append("--raw")
 		
@@ -87,7 +92,7 @@ class Diffable(object):
 		# END paths handling
 		
 		kwargs['as_process'] = True
-		proc = self.repo.git.diff(*args, **kwargs)
+		proc = self.repo.git.diff(*self._process_diff_args(args), **kwargs)
 		
 		diff_method = Diff._index_from_raw_format
 		if create_patch:
@@ -96,7 +101,7 @@ class Diffable(object):
 		
 		status = proc.wait()
 		if status != 0:
-			raise GitCommandError("git-diff", status, proc.stderr )
+			raise GitCommandError(("git diff",)+tuple(args), status, proc.stderr.read())
 		
 		return index
 
@@ -207,6 +212,20 @@ class Diff(object):
 		
 		self.diff = diff
 
+
+	def __eq__(self, other):
+		for name in self.__slots__:
+			if getattr(self, name) != getattr(other, name):
+				return False
+		# END for each name
+		return True
+		
+	def __ne__(self, other):
+		return not ( self == other )
+		
+	def __hash__(self):
+		return hash(tuple(getattr(self,n) for n in self.__slots__))
+
 	@property
 	def renamed(self):
 		"""
diff --git a/lib/git/errors.py b/lib/git/errors.py
index 18c58073..cde2798a 100644
--- a/lib/git/errors.py
+++ b/lib/git/errors.py
@@ -25,8 +25,8 @@ class GitCommandError(Exception):
 		self.stderr = stderr
 		self.status = status
 		self.command = command
-
+		
 	def __str__(self):
-		return repr("'%s' returned exit status %d: %r" %
-					(' '.join(self.command), self.status, str(self.stderr)))
+		return ("'%s' returned exit status %i: %s" %
+					(' '.join(str(i) for i in self.command), self.status, self.stderr))
 
diff --git a/lib/git/index.py b/lib/git/index.py
index 9a55da15..4217c9a2 100644
--- a/lib/git/index.py
+++ b/lib/git/index.py
@@ -14,8 +14,11 @@ import objects
 import tempfile
 import os
 import stat
-from git.objects import Blob, Tree
-from git.utils import SHA1Writer
+import git.diff as diff
+
+from git.objects import Blob, Tree, Object
+from git.utils import SHA1Writer, LazyMixin, ConcurrentWriteOperation
+
 
 class _TemporaryFileSwap(object):
 	"""
@@ -139,7 +142,7 @@ class IndexEntry(tuple):
 		return IndexEntry((time, time, 0, 0, blob.mode, 0, 0, blob.size, blob.id, 0, blob.path))
 
 
-class Index(object):
+class Index(LazyMixin, diff.Diffable):
 	"""
 	Implements an Index that can be manipulated using a native implementation in 
 	order to save git command function calls wherever possible.
@@ -153,20 +156,53 @@ class Index(object):
 	The index contains an entries dict whose keys are tuples of type IndexEntry
 	to facilitate access.
 	"""
-	__slots__ = ( "repo", "version", "entries", "_extension_data" )
+	__slots__ = ( "repo", "version", "entries", "_extension_data", "_is_default_index" )
 	_VERSION = 2			# latest version we support
 	S_IFGITLINK	= 0160000
 	
 	def __init__(self, repo, stream = None):
 		"""
 		Initialize this Index instance, optionally from the given ``stream``
+		
+		If a stream is not given, the stream will be initialized from the current 
+		repository's index on demand.
 		"""
 		self.repo = repo
-		self.entries = dict()
 		self.version = self._VERSION
 		self._extension_data = ''
+		self._is_default_index = True
 		if stream is not None:
+			self._is_default_index = False
 			self._read_from_stream(stream)
+		# END read from stream immediatly
+	
+	
+	def _set_cache_(self, attr):
+		if attr == "entries":
+			# read the current index
+			fp = open(self._index_path(), "r")
+			try:
+				self._read_from_stream(fp)
+			finally:
+				fp.close()
+			# END read from default index on demand
+		else:
+			super(Index, self)._set_cache_(attr)
+	
+	def _index_path(self):
+		return os.path.join(self.repo.path, "index")
+	
+	
+	@property
+	def path(self):
+		"""
+		Returns 
+			Path to the index file we are representing or None if we are 
+			a loose index that was read from a stream.
+		"""
+		if self._is_default_index:
+			return self._index_path()
+		return None
 	
 	@classmethod
 	def _read_entry(cls, stream):
@@ -197,13 +233,10 @@ class Index(object):
 	def _read_from_stream(self, stream):
 		"""
 		Initialize this instance with index values read from the given stream
-		
-		Note
-			We explicitly do not clear the entries dict here to allow for reading 
-			multiple chunks from multiple streams into the same Index instance
 		"""
 		self.version, num_entries = self._read_header(stream)
 		count = 0
+		self.entries = dict()
 		while count < num_entries:
 			entry = self._read_entry(stream)
 			self.entries[(entry.path, entry.stage)] = entry
@@ -293,12 +326,14 @@ class Index(object):
 		real_size = ((stream.tell() - beginoffset + 8) & ~7)
 		stream.write("\0" * ((beginoffset + real_size) - stream.tell()))
 
-	def write(self, stream):
+	def write(self, stream=None):
 		"""
-		Write the current state to the given stream
+		Write the current state to the given stream or to the default repository 
+		index.
 		
 		``stream``
-			File-like object
+			File-like object or None.
+			If None, the default repository index will be overwritten.
 		
 		Returns
 			self
@@ -306,6 +341,13 @@ class Index(object):
 		Note
 			Index writing based on the dulwich implementation
 		"""
+		write_op = None
+		if stream is None:
+			write_op = ConcurrentWriteOperation(self._index_path())
+			stream = write_op._begin_writing()
+			# stream = open(self._index_path()
+		# END stream handling 
+		
 		stream = SHA1Writer(stream)
 		
 		# header
@@ -325,6 +367,9 @@ class Index(object):
 		# write the sha over the content
 		stream.write_sha()
 		
+		if write_op is not None:
+			write_op._end_writing()
+			
 	
 	@classmethod
 	def from_tree(cls, repo, *treeish, **kwargs):
@@ -491,7 +536,7 @@ class Index(object):
 		Returns
 			Tree object representing this index
 		"""
-		index_path = os.path.join(self.repo.path, "index")
+		index_path = self._index_path()
 		tmp_index_mover = _TemporaryFileSwap(index_path)
 		
 		self.to_file(self, index_path)
@@ -507,4 +552,51 @@ class Index(object):
 		# END write tree handling 
 		return Tree(self.repo, tree_sha, 0, '')
 		
+		
+	def _process_diff_args(self, args):
+		try:
+			args.pop(args.index(self))
+		except IndexError:
+			pass
+		# END remove self
+		return args
+		
+	def diff(self, other=diff.Diffable.Index, paths=None, create_patch=False, **kwargs):
+		"""
+		Diff this index against the working copy or a Tree or Commit object
+		
+		For a documentation of the parameters and return values, see 
+		Diffable.diff
+		
+		Note
+			Will only work with indices that represent the default git index as 
+			they have not been initialized with a stream.
+		"""
+		if not self._is_default_index:
+			raise AssertionError( "Cannot diff custom indices as they do not represent the default git index" )
+		
+		# index against index is always empty
+		if other is self.Index:
+			return diff.DiffIndex()
+			
+		# index against anything but None is a reverse diff with the respective
+		# item. Handle existing -R flags properly. Transform strings to the object
+		# so that we can call diff on it
+		if isinstance(other, basestring):
+			other = Object.new(self.repo, other)
+		# END object conversion
+		
+		if isinstance(other, Object):
+			# invert the existing R flag
+			cur_val = kwargs.get('R', False)
+			kwargs['R'] = not cur_val
+			return other.diff(self.Index, paths, create_patch, **kwargs)
+		# END diff against other item handlin
+		
+		# if other is not None here, something is wrong 
+		if other is not None:
+			raise ValueError( "other must be None, Diffable.Index, a Tree or Commit, was %r" % other )
+		
+		# diff against working copy - can be handled by superclass natively
+		return super(Index, self).diff(other, paths, create_patch, **kwargs)
 	
diff --git a/lib/git/objects/__init__.py b/lib/git/objects/__init__.py
index 39e650b7..192750e3 100644
--- a/lib/git/objects/__init__.py
+++ b/lib/git/objects/__init__.py
@@ -2,6 +2,7 @@
 Import all submodules main classes into the package space
 """
 import inspect
+from base import *
 from tag import *
 from blob import *
 from tree import *
diff --git a/lib/git/utils.py b/lib/git/utils.py
index cdc7c55b..8cdb4804 100644
--- a/lib/git/utils.py
+++ b/lib/git/utils.py
@@ -5,6 +5,8 @@
 # the BSD License: http://www.opensource.org/licenses/bsd-license.php
 
 import os
+import sys
+import tempfile
 
 try:
     import hashlib
@@ -58,6 +60,168 @@ class SHA1Writer(object):
         return self.f.tell()
 
 
+class LockFile(object):
+	"""
+	Provides methods to obtain, check for, and release a file based lock which 
+	should be used to handle concurrent access to the same file.
+	
+	As we are a utility class to be derived from, we only use protected methods.
+	
+	Locks will automatically be released on destruction
+	"""
+	__slots__ = ("_file_path", "_owns_lock")
+	
+	def __init__(self, file_path):
+		self._file_path = file_path
+		self._owns_lock = False
+	
+	def __del__(self):
+		self._release_lock()
+	
+	def _lock_file_path(self):
+		"""
+		Return
+			Path to lockfile
+		"""
+		return "%s.lock" % (self._file_path)
+	
+	def _has_lock(self):
+		"""
+		Return
+			True if we have a lock and if the lockfile still exists
+			
+		Raise
+			AssertionError if our lock-file does not exist
+		"""
+		if not self._owns_lock:
+			return False
+		
+		lock_file = self._lock_file_path()
+		try:
+			fp = open(lock_file, "r")
+			pid = int(fp.read())
+			fp.close()
+		except IOError:
+			raise AssertionError("GitConfigParser has a lock but the lock file at %s could not be read" % lock_file)
+		
+		if pid != os.getpid():
+			raise AssertionError("We claim to own the lock at %s, but it was not owned by our process: %i" % (lock_file, os.getpid()))
+		
+		return True
+		
+	def _obtain_lock_or_raise(self):
+		"""
+		Create a lock file as flag for other instances, mark our instance as lock-holder
+		
+		Raise
+			IOError if a lock was already present or a lock file could not be written
+		"""
+		if self._has_lock():
+			return 
+			
+		lock_file = self._lock_file_path()
+		if os.path.exists(lock_file):
+			raise IOError("Lock for file %r did already exist, delete %r in case the lock is illegal" % (self._file_path, lock_file))
+		
+		fp = open(lock_file, "w")
+		fp.write(str(os.getpid()))
+		fp.close()
+		
+		self._owns_lock = True
+		
+	def _release_lock(self):
+		"""
+		Release our lock if we have one
+		"""
+		if not self._has_lock():
+			return 
+			
+		os.remove(self._lock_file_path())
+		self._owns_lock = False
+
+
+class ConcurrentWriteOperation(LockFile):
+	"""
+	This class facilitates a safe write operation to a file on disk such that we: 
+	
+		- lock the original file
+		- write to a temporary file
+		- rename temporary file back to the original one on close
+		- unlock the original file
+		
+	This type handles error correctly in that it will assure a consistent state 
+	on destruction
+	"""
+	__slots__ = "_temp_write_fp"
+	
+	def __init__(self, file_path):
+		"""
+		Initialize an instance able to write the given file_path
+		"""
+		super(ConcurrentWriteOperation, self).__init__(file_path)
+		self._temp_write_fp = None
+	
+	def __del__(self):
+		self._end_writing(successful=False)
+		
+	def _begin_writing(self):
+		"""
+		Begin writing our file, hence we get a lock and start writing 
+		a temporary file in the same directory.
+		
+		Returns
+			File Object to write to. It is still maintained by this instance
+			and you do not need to manually close 
+		"""
+		# already writing ? 
+		if self._temp_write_fp is not None:
+			return self._temp_write_fp
+			
+		self._obtain_lock_or_raise()
+		dirname, basename = os.path.split(self._file_path)
+		self._temp_write_fp = open(tempfile.mktemp(basename, '', dirname), "w")
+		return self._temp_write_fp
+		
+	def _is_writing(self):
+		"""
+		Returns 
+			True if we are currently writing a file
+		"""
+		return self._temp_write_fp is not None
+	
+	def _end_writing(self, successful=True):
+		"""
+		Indicate you successfully finished writing the file to:
+		
+			- close the underlying stream
+			- rename the remporary file to the original one
+			- release our lock
+		"""
+		# did we start a write operation ?
+		if self._temp_write_fp is None:
+			return 
+			
+		if not self._temp_write_fp.closed:
+			self._temp_write_fp.close()
+		
+		if successful:
+			# on windows, rename does not silently overwrite the existing one
+			if sys.platform == "win32":
+				if os.path.isfile(self._file_path):
+					os.remove(self._file_path)
+				# END remove if exists
+			# END win32 special handling
+			os.rename(self._temp_write_fp.name, self._file_path)
+		else:
+			# just delete the file so far, we failed
+			os.remove(self._temp_write_fp.name)
+		# END successful handling
+	
+		# finally reset our handle
+		self._release_lock()
+		self._temp_write_fp = None
+
+
 class LazyMixin(object):
 	"""
 	Base class providing an interface to lazily retrieve attribute values upon