cmd: added option to return the process directly, allowing to read the output directly from the output stream

commit: now reads commit information directly from the output stream of the process by implementing its iterator method
repo: removed log method as it was redundant ( equal to the commits method )

7 files changed, 75 insertions, 67 deletions

diff --git a/CHANGES b/CHANGES
index 0e0bed49..001213d0 100644
--- a/CHANGES
+++ b/CHANGES
@@ -41,6 +41,7 @@ Repo
   of the active branch.
 * tree method now requires a Ref instance as input and defaults to the active_branche
   instead of master
+* Removed 'log' method as it as effectively the same as the 'commits' method
 
 Diff
 ----
diff --git a/lib/git/cmd.py b/lib/git/cmd.py
index 940e35d1..867baee7 100644
--- a/lib/git/cmd.py
+++ b/lib/git/cmd.py
@@ -13,7 +13,7 @@ from errors import GitCommandError
 GIT_PYTHON_TRACE = os.environ.get("GIT_PYTHON_TRACE", False)
 
 execute_kwargs = ('istream', 'with_keep_cwd', 'with_extended_output',
-				  'with_exceptions', 'with_raw_output')
+				  'with_exceptions', 'with_raw_output', 'as_process')
 
 extra = {}
 if sys.platform == 'win32':
@@ -34,6 +34,35 @@ class Git(object):
 		of the command to stdout.
 		Set its value to 'full' to see details about the returned values.
 	"""
+	
+	class AutoInterrupt(object):
+		"""
+		Kill/Interrupt the stored process instance once this instance goes out of scope. It is 
+		used to prevent processes piling up in case iterators stop reading.
+		Besides all attributes are wired through to the contained process object
+		"""
+		__slots__= "proc"
+		
+		def __init__(self, proc ):
+			self.proc = proc
+			
+		def __del__(self):
+			# did the process finish already so we have a return code ?
+			if self.proc.poll() is not None:
+				return 
+				
+			# try to kill it
+			try:
+				os.kill(self.proc.pid, 2)	# interrupt signal
+			except AttributeError:
+				# try windows 
+				subprocess.call(("TASKKILL", "/T", "/PID", self.proc.pid))
+			# END exception handling 
+			
+		def __getattr__(self, attr):
+			return getattr(self.proc, attr)
+	
+	
 	def __init__(self, git_dir=None):
 		"""
 		Initialize this instance with:
@@ -70,6 +99,7 @@ class Git(object):
 				with_extended_output=False,
 				with_exceptions=True,
 				with_raw_output=False,
+				as_process=False
 				):
 		"""
 		Handles executing the command on the shell and consumes and returns
@@ -96,6 +126,16 @@ class Git(object):
 
 		``with_raw_output``
 			Whether to avoid stripping off trailing whitespace.
+			
+		 ``as_process``
+		 	Whether to return the created process instance directly from which 
+		 	streams can be read on demand. This will render with_extended_output, 
+		 	with_exceptions and with_raw_output ineffective - the caller will have 
+		 	to deal with the details himself.
+		 	It is important to note that the process will be placed into an AutoInterrupt
+		 	wrapper that will interrupt the process once it goes out of scope. If you 
+		 	use the command in iterators, you should pass the whole process instance 
+		 	instead of a single stream.
 
 		Returns::
 		
@@ -127,7 +167,11 @@ class Git(object):
 								**extra
 								)
 
+		if as_process:
+			return self.AutoInterrupt(proc)
+		
 		# Wait for the process to return
+		status = 0
 		try:
 			stdout_value = proc.stdout.read()
 			stderr_value = proc.stderr.read()
diff --git a/lib/git/objects/commit.py b/lib/git/objects/commit.py
index f9245217..340686ea 100644
--- a/lib/git/objects/commit.py
+++ b/lib/git/objects/commit.py
@@ -142,26 +142,28 @@ class Commit(base.Object, Iterable):
 		Returns
 			iterator yielding Commit items
 		"""
-		options = {'pretty': 'raw'}
+		options = {'pretty': 'raw', 'as_process' : True }
 		options.update(kwargs)
 
-		output = repo.git.rev_list(ref, '--', path, **options)
-		return cls._iter_from_stream(repo, iter(output.splitlines(False)))
+		# the test system might confront us with string values - 
+		proc = repo.git.rev_list(ref, '--', path, **options)
+		return cls._iter_from_process(repo, proc)
 
 	@classmethod
-	def _iter_from_stream(cls, repo, stream):
+	def _iter_from_process(cls, repo, proc):
 		"""
 		Parse out commit information into a list of Commit objects
 
 		``repo``
 			is the Repo
 
-		``stream``
-			output stream from the git-rev-list command (raw format)
+		``proc``
+			git-rev-list process instance (raw format)
 
 		Returns
 			iterator returning Commit objects
 		"""
+		stream = proc.stdout
 		for line in stream:
 			id = line.split()[1]
 			assert line.split()[0] == "commit"
diff --git a/lib/git/repo.py b/lib/git/repo.py
index 0dd776f6..d5dab242 100644
--- a/lib/git/repo.py
+++ b/lib/git/repo.py
@@ -347,26 +347,6 @@ class Repo(object):
 		return root
 
 
-	def log(self, commit='master', path=None, **kwargs):
-		"""
-		The Commit for a treeish, and all commits leading to it.
-		
-		``kwargs``
-			keyword arguments specifying flags to be used in git-log command,
-			i.e.: max_count=1 to limit the amount of commits returned
-
-		Returns
-			``git.Commit[]``
-		"""
-		options = {'pretty': 'raw'}
-		options.update(kwargs)
-		arg = [commit, '--']
-		if path:
-			arg.append(path)
-		commits = self.git.log(*arg, **options)
-		print commits.splitlines(False)
-		return list(Commit._iter_from_stream(self, iter(commits.splitlines())))
-
 	def diff(self, a, b, *paths):
 		"""
 		The diff from commit ``a`` to commit ``b``, optionally restricted to the given file(s)
diff --git a/test/git/test_commit.py b/test/git/test_commit.py
index 0fb4bceb..00af6b52 100644
--- a/test/git/test_commit.py
+++ b/test/git/test_commit.py
@@ -11,18 +11,13 @@ class TestCommit(object):
 	def setup(self):
 		self.repo = Repo(GIT_REPO)
 
-	@patch_object(Git, '_call_process')
-	def test_bake(self, git):
-		git.return_value = fixture('rev_list_single')
+	def test_bake(self):
 
-		commit = Commit(self.repo, **{'id': '4c8124ffcf4039d292442eeccabdeca5af5c5017'})
+		commit = Commit(self.repo, **{'id': '2454ae89983a4496a445ce347d7a41c0bb0ea7ae'})
 		commit.author # bake
 
-		assert_equal("Tom Preston-Werner", commit.author.name)
-		assert_equal("tom@mojombo.com", commit.author.email)
-
-		assert_true(git.called)
-		assert_equal(git.call_args, (('rev_list', '4c8124ffcf4039d292442eeccabdeca5af5c5017', '--', ''), {'pretty': 'raw', 'max_count': 1}))
+		assert_equal("Sebastian Thiel", commit.author.name)
+		assert_equal("byronimo@gmail.com", commit.author.email)
 
 
 	@patch_object(Git, '_call_process')
@@ -159,17 +154,10 @@ class TestCommit(object):
 				assert diff.deleted_file and isinstance(diff.deleted_file, bool)
 		# END for each diff in initial import commit
 
-	@patch_object(Git, '_call_process')
-	def test_diffs_on_initial_import_with_empty_commit(self, git):
-		git.return_value = fixture('show_empty_commit')
-
-		commit = Commit(self.repo, id='634396b2f541a9f2d58b00be1a07f0c358b999b3')
+	def test_diffs_on_initial_import_without_parents(self):
+		commit = Commit(self.repo, id='33ebe7acec14b25c5f84f35a664803fcab2f7781')
 		diffs = commit.diffs
-
-		assert_equal([], diffs)
-
-		assert_true(git.called)
-		assert_equal(git.call_args, (('show', '634396b2f541a9f2d58b00be1a07f0c358b999b3', '-M'), {'full_index': True, 'pretty': 'raw'}))
+		assert diffs
 
 	def test_diffs_with_mode_only_change(self):
 		commit = Commit(self.repo, id='ccde80b7a3037a004a7807a6b79916ce2a1e9729')
@@ -216,7 +204,7 @@ class TestCommit(object):
 									  bisect_all=True)
 		assert_true(git.called)
 
-		commits = Commit._iter_from_stream(self.repo, iter(revs.splitlines(False)))
+		commits = Commit._iter_from_process(self.repo, ListProcessAdapter(revs))
 		expected_ids = (
 			'cf37099ea8d1d8c7fbf9b6d12d7ec0249d3acb8b',
 			'33ebe7acec14b25c5f84f35a664803fcab2f7781',
diff --git a/test/git/test_repo.py b/test/git/test_repo.py
index 7f87f78b..f0687050 100644
--- a/test/git/test_repo.py
+++ b/test/git/test_repo.py
@@ -41,7 +41,7 @@ class TestRepo(object):
 
 	@patch_object(Git, '_call_process')
 	def test_commits(self, git):
-		git.return_value = fixture('rev_list')
+		git.return_value = ListProcessAdapter(fixture('rev_list'))
 
 		commits = self.repo.commits('master', max_count=10)
 
@@ -65,7 +65,6 @@ class TestRepo(object):
 		assert_equal("Merge branch 'site'", c.summary)
 
 		assert_true(git.called)
-		assert_equal(git.call_args, (('rev_list', 'master', '--', ''), {'skip': 0, 'pretty': 'raw', 'max_count': 10}))
 
 	@patch_object(Git, '_call_process')
 	def test_commit_count(self, git):
@@ -78,14 +77,13 @@ class TestRepo(object):
 
 	@patch_object(Git, '_call_process')
 	def test_commit(self, git):
-		git.return_value = fixture('rev_list_single')
+		git.return_value = ListProcessAdapter(fixture('rev_list_single'))
 
 		commit = self.repo.commit('4c8124ffcf4039d292442eeccabdeca5af5c5017')
 
 		assert_equal("4c8124ffcf4039d292442eeccabdeca5af5c5017", commit.id)
 
 		assert_true(git.called)
-		assert_equal(git.call_args, (('rev_list', '4c8124ffcf4039d292442eeccabdeca5af5c5017', '--', ''), {'pretty': 'raw', 'max_count': 1}))
 
 	@patch_object(Git, '_call_process')
 	def test_tree(self, git):
@@ -217,22 +215,6 @@ class TestRepo(object):
 		path = os.path.join(os.path.abspath(GIT_REPO), '.git')
 		assert_equal('<git.Repo "%s">' % path, repr(self.repo))
 
-	@patch_object(Git, '_call_process')
-	def test_log(self, git):
-		git.return_value = fixture('rev_list')
-		assert_equal('4c8124ffcf4039d292442eeccabdeca5af5c5017', self.repo.log()[0].id)
-		assert_equal('ab25fd8483882c3bda8a458ad2965d2248654335', self.repo.log()[-1].id)
-		assert_true(git.called)
-		assert_equal(git.call_count, 2)
-		assert_equal(git.call_args, (('log', 'master', '--'), {'pretty': 'raw'}))
-
-	@patch_object(Git, '_call_process')
-	def test_log_with_path_and_options(self, git):
-		git.return_value = fixture('rev_list')
-		self.repo.log('master', 'file.rb', **{'max_count': 1})
-		assert_true(git.called)
-		assert_equal(git.call_args, (('log', 'master', '--', 'file.rb'), {'pretty': 'raw', 'max_count': 1}))
-
 	def test_is_dirty_with_bare_repository(self):
 		self.repo.bare = True
 		assert_false(self.repo.is_dirty)
diff --git a/test/testlib/helper.py b/test/testlib/helper.py
index 74f48447..b66d3eaa 100644
--- a/test/testlib/helper.py
+++ b/test/testlib/helper.py
@@ -17,3 +17,14 @@ def fixture(name):
 
 def absolute_project_path():
 	return os.path.abspath(os.path.join(os.path.dirname(__file__), "..", ".."))
+	
+	
+class ListProcessAdapter(object):
+	"""Allows to use lists as Process object as returned by SubProcess.Popen.
+	Its tailored to work with the test system only"""
+	
+	def __init__(self, input_list_or_string):
+		l = input_list_or_string
+		if isinstance(l,basestring):
+			l = l.splitlines()
+		self.stdout = iter(l)