made with_exceptions=True default (don't look before you leak ;)) and removed the combined output of stderr and stdout.

Also renamed with_status to extended_output.
The method_missing function needs to be modified, as it does a kwargs.pop(xxx, None); which resulted in with_excpetions=None -> False all the time...
Test should follow tomorrow.

Author: apollo13

lib/git/cmd.py

@@ -78,9 +78,9 @@ def get_dir(self):
     def execute(self, command,
                 istream=None,
                 keep_cwd=False,
-                with_status=False,
+                extended_output=False,
                 with_stderr=False,
-                with_exceptions=False,
+                with_exceptions=True,
                 with_raw_output=False,
                 ):
         """
@@ -98,11 +98,8 @@ def execute(self, command,
             GitPython uses get_work_tree() as its working directory by
             default and get_git_dir() for bare repositories.
 
-        ``with_status``
-            Whether to return a (status, str) tuple.
-
-        ``with_stderr``
-            Whether to combine stderr into the output.
+        ``extended_output``
+            Whether to return a (status, stdout, stderr) tuple.
 
         ``with_exceptions``
             Whether to raise an exception when git returns a non-zero status.
@@ -111,20 +108,13 @@ def execute(self, command,
             Whether to avoid stripping off trailing whitespace.
 
         Returns
-            str(output)                     # with_status = False (Default)
-            tuple(int(status), str(output)) # with_status = True
+            str(output)                     # extended_output = False (Default)
+            tuple(int(status), str(output)) # extended_output = True
         """
 
         if GIT_PYTHON_TRACE and not GIT_PYTHON_TRACE == 'full':
             print ' '.join(command)
 
-        # Allow stderr to be merged into stdout when with_stderr is True.
-        # Otherwise, throw stderr away.
-        if with_stderr:
-            stderr = subprocess.STDOUT
-        else:
-            stderr = subprocess.PIPE
-
         # Allow the user to have the command executed in their working dir.
         if keep_cwd:
           cwd = os.getcwd()
@@ -135,28 +125,29 @@ def execute(self, command,
         proc = subprocess.Popen(command,
                                 cwd=cwd,
                                 stdin=istream,
-                                stderr=stderr,
+                                stderr=subprocess.PIPE,
                                 stdout=subprocess.PIPE
                                 )
 
         # Wait for the process to return
-        stdout_value = proc.stdout.read()
-        status = proc.wait()
-        proc.stdout.close()
-
-        if proc.stderr:
-          stderr_value = proc.stderr.read()
-          proc.stderr.close()
+        try:
+            stdout_value = proc.stdout.read()
+            stderr_value = proc.stderr.read()
+            status = proc.wait()
+        finally:
+            proc.stdout.close()
+            proc.stderr.close()
 
         # Strip off trailing whitespace by default
         if not with_raw_output:
             stdout_value = stdout_value.rstrip()
+            stderr_value = stderr_value.rstrip()
+
+        print command, status
 
-        # Grab the exit status
-        status = proc.poll()
         if with_exceptions and status != 0:
-            raise GitCommandError("%s returned exit status %d"
-                                  % (str(command), status))
+            print 19
+            raise GitCommandError(command, status, stderr_value)
 
         if GIT_PYTHON_TRACE == 'full':
             if stderr_value:
@@ -167,8 +158,8 @@ def execute(self, command,
               print "%s -> %d" % (command, status)
 
         # Allow access to the command's status code
-        if with_status:
-            return (status, stdout_value)
+        if extended_output:
+            return (status, stdout_value, stderr_value)
         else:
             return stdout_value
 
@@ -217,9 +208,9 @@ def method_missing(self, method, *args, **kwargs):
         # otherwise these'll end up in args, which is bad.
         istream = kwargs.pop("istream", None)
         keep_cwd = kwargs.pop("keep_cwd", None)
-        with_status = kwargs.pop("with_status", None)
+        extended_output = kwargs.pop("extended_output", None)
         with_stderr = kwargs.pop("with_stderr", None)
-        with_exceptions = kwargs.pop("with_exceptions", None)
+        with_exceptions = kwargs.pop("with_exceptions", True)
         with_raw_output = kwargs.pop("with_raw_output", None)
 
         # Prepare the argument list
@@ -233,7 +224,7 @@ def method_missing(self, method, *args, **kwargs):
         return self.execute(call,
                             istream = istream,
                             keep_cwd = keep_cwd,
-                            with_status = with_status,
+                            extended_output = extended_output,
                             with_stderr = with_stderr,
                             with_exceptions = with_exceptions,
                             with_raw_output = with_raw_output,

lib/git/errors.py

@@ -5,4 +5,12 @@ class NoSuchPathError(Exception):
     pass
 
 class GitCommandError(Exception):
-    pass
+    def __init__(self, command, status, stderr=None):
+        self.stderr = stderr
+        self.status = status
+        self.command = command
+
+    def __str__(self):
+        return repr("%s returned exit status %d" %
+                    (str(self.command), self.status))
+

test/git/test_git.py

@@ -14,6 +14,11 @@ def test_method_missing_calls_execute(self, git):
         assert_true(git.called)
         # assert_equal(git.call_args, ((("%s version " % self.git_bin_base),), {}))
 
+    @raises(GitCommandError)
+    def test_it_raises_errors(self):
+        self.git.this_does_not_exist()
+
+
     def test_it_transforms_kwargs_into_git_command_arguments(self):
         assert_equal(["-s"], self.git.transform_kwargs(**{'s': True}))
         assert_equal(["-s5"], self.git.transform_kwargs(**{'s': 5}))
@@ -33,25 +38,6 @@ def test_it_accepts_stdin(self):
                      self.git.hash_object(istream=fh, stdin=True))
         fh.close()
 
-    def test_it_returns_status_and_ignores_stderr(self):
-        assert_equal((1, ""), self.git.this_does_not_exist(with_status=True))
-
-    @raises(GitCommandError)
-    def test_it_raises_errors(self):
-        self.git.this_does_not_exist(with_exceptions=True)
-
-    def test_it_returns_stderr_in_output(self):
-        # Note: no trailiing newline
-        assert_match(r"^git: 'this-does-not-exist' is not a git-command",
-                      self.git.this_does_not_exist(with_stderr=True))
-
-    def test_it_does_not_strip_output_when_using_with_raw_output(self):
-        # Note: trailing newline
-        assert_match(r"^git: 'this-does-not-exist' is not a git-command" \
-                     r"(\. See 'git --help'\.)?" + os.linesep,
-                      self.git.this_does_not_exist(with_stderr=True,
-                                                   with_raw_output=True))
-
     def test_it_handles_large_input(self):
         output = self.git.execute(["cat", "/bin/bash"])
         assert_true(len(output) > 4096) # at least 4k