Repository: gitpython-developers/GitPython

Issue:
GitCommand missing stderr
With GitPython 3.1.14 `GitCommandError`, for failed clones would pass on the captured stderr output into its `stderr` attribute. With 3.1.15 this contains the empty string instead.

This was caught by this unit test: https://github.com/tomtom-international/hopic/blob/9a35520388f8109ccff6a89407bc2429ed0d0557/hopic/test/test_checkout.py#L84-L103

A reduced test that reduces this to _only_ testing GitPython (instead of the full integration with hopic) looks like the code below. That exhibits the exact same problem:

```python
import git
import pytest

def test_checkout_in_non_empty_dir(tmp_path):
    orig_repo = tmp_path / "orig"
    with git.Repo.init(orig_repo, expand_vars=False) as repo:
        repo.index.commit(message='Initial commit', **_commitargs)

    non_empty_dir = tmp_path / 'non-empty-clone'
    non_empty_dir.mkdir(parents=True)
    garbage_file = non_empty_dir / 'not-empty'
    garbage_file.write_text('Garbage!')

    # Verify that the clone fails complaining about the target directory not being empty/non-existent
    with pytest.raises(git.GitCommandError, match=r'(?is).*\bfatal:\s+destination\s+path\b.*\bexists\b.*\bnot\b.*\bempty\s+directory\b'):
        git.Repo.clone_from(orig_repo, non_empty_dir)
    assert garbage_file.exists()
```

With 3.1.14 this passes. With the `pytest.raises` expectation removed this output is displayed:
```console
___________________________________________________________________ test_checkout_in_non_empty_dir ____________________________________________________________________

tmp_path = PosixPath('/tmp/pytest-of-vanschig/pytest-334/test_checkout_in_non_empty_dir0')

    def test_checkout_in_non_empty_dir(tmp_path):
        orig_repo = tmp_path / "orig"
        with git.Repo.init(orig_repo, expand_vars=False) as repo:
            repo.index.commit(message='Initial commit', **_commitargs)

        non_empty_dir = tmp_path / 'non-empty-clone'
        non_empty_dir.mkdir(parents=True)
        garbage_file = non_empty_dir / 'not-empty'
        garbage_file.write_text('Garbage!')

        # Verify that the clone fails complaining about the target directory not being empty/non-existent
>       git.Repo.clone_from(orig_repo, non_empty_dir)

hopic/test/test_checkout.py:95:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
v3.8/lib/python3.8/site-packages/git/repo/base.py:1032: in clone_from
    return cls._clone(git, url, to_path, GitCmdObjectDB, progress, multi_options, **kwargs)
v3.8/lib/python3.8/site-packages/git/repo/base.py:973: in _clone
    finalize_process(proc, stderr=stderr)
v3.8/lib/python3.8/site-packages/git/util.py:329: in finalize_process
    proc.wait(**kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <git.cmd.Git.AutoInterrupt object at 0x7f6a5883f9a0>
stderr = b"fatal: destination path '/tmp/pytest-of-vanschig/pytest-334/test_checkout_in_non_empty_dir0/non-empty-clone' already exists and is not an empty directory.\n"

    def wait(self, stderr=b''):  # TODO: Bad choice to mimic `proc.wait()` but with different args.
        """Wait for the process and return its status code.

        :param stderr: Previously read value of stderr, in case stderr is already closed.
        :warn: may deadlock if output or error pipes are used and not handled separately.
        :raise GitCommandError: if the return status is not 0"""
        if stderr is None:
            stderr = b''
        stderr = force_bytes(data=stderr, encoding='utf-8')

        status = self.proc.wait()

        def read_all_from_possibly_closed_stream(stream):
            try:
                return stderr + force_bytes(stream.read())
            except ValueError:
                return stderr or b''

        if status != 0:
            errstr = read_all_from_possibly_closed_stream(self.proc.stderr)
            log.debug('AutoInterrupt wait stderr: %r' % (errstr,))
>           raise GitCommandError(self.args, status, errstr)
E           git.exc.GitCommandError: Cmd('git') failed due to: exit code(128)
E             cmdline: git clone -v /tmp/pytest-of-vanschig/pytest-334/test_checkout_in_non_empty_dir0/orig /tmp/pytest-of-vanschig/pytest-334/test_checkout_in_non_empty_dir0/non-empty-clone
E             stderr: 'fatal: destination path '/tmp/pytest-of-vanschig/pytest-334/test_checkout_in_non_empty_dir0/non-empty-clone' already exists and is not an empty directory.
E           '

v3.8/lib/python3.8/site-packages/git/cmd.py:408: GitCommandError
```

With 3.1.15 this output is produced instead (notice the absence of the "stderr: ..." line):
```console
___________________________________________________________________ test_checkout_in_non_empty_dir ____________________________________________________________________

tmp_path = PosixPath('/tmp/pytest-of-vanschig/pytest-336/test_checkout_in_non_empty_dir0')

    def test_checkout_in_non_empty_dir(tmp_path):
        orig_repo = tmp_path / "orig"
        with git.Repo.init(orig_repo, expand_vars=False) as repo:
            repo.index.commit(message='Initial commit', **_commitargs)

        non_empty_dir = tmp_path / 'non-empty-clone'
        non_empty_dir.mkdir(parents=True)
        garbage_file = non_empty_dir / 'not-empty'
        garbage_file.write_text('Garbage!')

        # Verify that the clone fails complaining about the target directory not being empty/non-existent
>       git.Repo.clone_from(orig_repo, non_empty_dir)

hopic/test/test_checkout.py:95:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
v3.8/lib/python3.8/site-packages/git/repo/base.py:1087: in clone_from
    return cls._clone(git, url, to_path, GitCmdObjectDB, progress, multi_options, **kwargs)
v3.8/lib/python3.8/site-packages/git/repo/base.py:1017: in _clone
    handle_process_output(proc, None, progress_checked.new_message_handler(),
v3.8/lib/python3.8/site-packages/git/cmd.py:116: in handle_process_output
    return finalizer(process)
v3.8/lib/python3.8/site-packages/git/util.py:354: in finalize_process
    proc.wait(**kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <git.cmd.Git.AutoInterrupt object at 0x7fcc8fd95820>, stderr = b''

    def wait(self, stderr=b''):  # TODO: Bad choice to mimic `proc.wait()` but with different args.
        """Wait for the process and return its status code.

        :param stderr: Previously read value of stderr, in case stderr is already closed.
        :warn: may deadlock if output or error pipes are used and not handled separately.
        :raise GitCommandError: if the return status is not 0"""
        if stderr is None:
            stderr = b''
        stderr = force_bytes(data=stderr, encoding='utf-8')

        status = self.proc.wait()

        def read_all_from_possibly_closed_stream(stream):
            try:
                return stderr + force_bytes(stream.read())
            except ValueError:
                return stderr or b''

        if status != 0:
            errstr = read_all_from_possibly_closed_stream(self.proc.stderr)
            log.debug('AutoInterrupt wait stderr: %r' % (errstr,))
>           raise GitCommandError(remove_password_if_present(self.args), status, errstr)
E           git.exc.GitCommandError: Cmd('git') failed due to: exit code(128)
E             cmdline: git clone -v --progress /tmp/pytest-of-vanschig/pytest-336/test_checkout_in_non_empty_dir0/orig /tmp/pytest-of-vanschig/pytest-336/test_checkout_in_non_empty_dir0/non-empty-clone

v3.8/lib/python3.8/site-packages/git/cmd.py:409: GitCommandError
```

When catching the exception and dumping it's `stderr` attribute it turns out to be an empty string (`''`).

This seems related to #1220. But it's presentation is different as that issue still reports having content, just wrongly encoded.

Discussion:
Byron: > Thanks for finding this! I've created a unit test that fails without and succeeds with this change (also with _only_ the the condition change as I suggested).

Are you able to push it into this PR? If not I would be happy to do that for you if you post it here.
Byron: Once approved by @muggenhor this one should be ready to merge. Looks good to me (as the tests are green :) even though without the fix they are not).

Patch:
diff --git a/git/repo/base.py b/git/repo/base.py
index a28c9d28..b1d0cdbc 100644
--- a/git/repo/base.py
+++ b/git/repo/base.py
@@ -988,8 +988,6 @@ class Repo(object):
     def _clone(cls, git: 'Git', url: PathLike, path: PathLike, odb_default_type: Type[GitCmdObjectDB],
                progress: Optional[Callable], multi_options: Optional[List[str]] = None, **kwargs: Any
                ) -> 'Repo':
-        progress_checked = to_progress_instance(progress)
-
         odbt = kwargs.pop('odbt', odb_default_type)
 
         # when pathlib.Path or other classbased path is passed
@@ -1012,9 +1010,9 @@ class Repo(object):
         if multi_options:
             multi = ' '.join(multi_options).split(' ')
         proc = git.clone(multi, Git.polish_url(url), clone_path, with_extended_output=True, as_process=True,
-                         v=True, universal_newlines=True, **add_progress(kwargs, git, progress_checked))
-        if progress_checked:
-            handle_process_output(proc, None, progress_checked.new_message_handler(),
+                         v=True, universal_newlines=True, **add_progress(kwargs, git, progress))
+        if progress:
+            handle_process_output(proc, None, to_progress_instance(progress).new_message_handler(),
                                   finalize_process, decode_streams=False)
         else:
             (stdout, stderr) = proc.communicate()


Test patch:
diff --git a/test/test_clone.py b/test/test_clone.py
new file mode 100644
index 00000000..e9f6714d
--- /dev/null
+++ b/test/test_clone.py
@@ -0,0 +1,32 @@
+# -*- coding: utf-8 -*-
+# This module is part of GitPython and is released under
+# the BSD License: http://www.opensource.org/licenses/bsd-license.php
+
+from pathlib import Path
+import re
+
+import git
+
+from .lib import (
+    TestBase,
+    with_rw_directory,
+)
+
+
+class TestClone(TestBase):
+    @with_rw_directory
+    def test_checkout_in_non_empty_dir(self, rw_dir):
+        non_empty_dir = Path(rw_dir)
+        garbage_file = non_empty_dir / 'not-empty'
+        garbage_file.write_text('Garbage!')
+
+        # Verify that cloning into the non-empty dir fails while complaining about
+        # the target directory not being empty/non-existent
+        try:
+            self.rorepo.clone(non_empty_dir)
+        except git.GitCommandError as exc:
+            self.assertTrue(exc.stderr, "GitCommandError's 'stderr' is unexpectedly empty")
+            expr = re.compile(r'(?is).*\bfatal:\s+destination\s+path\b.*\bexists\b.*\bnot\b.*\bempty\s+directory\b')
+            self.assertTrue(expr.search(exc.stderr), '"%s" does not match "%s"' % (expr.pattern, exc.stderr))
+        else:
+            self.fail("GitCommandError not raised")