Merge pull request #2126 from ngie-eign/fix-execute-with_stdout-no-issues

Byron · web-flow · commit 75e6c6bab4b3 · 2026-04-19T12:24:47.000+08:00
git.cmd.Git.execute(..): fix `with_stdout=False`
diff --git a/AUTHORS b/AUTHORS
@@ -56,5 +56,6 @@ Contributors are:
 -Ethan Lin <et.repositories _at_ gmail.com>
 -Jonas Scharpf <jonas.scharpf _at_ checkmk.com>
 -Gordon Marx
+-Enji Cooper
 
 Portions derived from other open source works and are clearly marked.
diff --git a/git/cmd.py b/git/cmd.py
@@ -1364,25 +1364,29 @@ def communicate() -> Tuple[AnyStr, AnyStr]:
             if output_stream is None:
                 stdout_value, stderr_value = communicate()
                 # Strip trailing "\n".
-                if stdout_value.endswith(newline) and strip_newline_in_stdout:  # type: ignore[arg-type]
+                if stdout_value is not None and stdout_value.endswith(newline) and strip_newline_in_stdout:  # type: ignore[arg-type]
                     stdout_value = stdout_value[:-1]
-                if stderr_value.endswith(newline):  # type: ignore[arg-type]
+                if stderr_value is not None and stderr_value.endswith(newline):  # type: ignore[arg-type]
                     stderr_value = stderr_value[:-1]
 
                 status = proc.returncode
             else:
                 max_chunk_size = max_chunk_size if max_chunk_size and max_chunk_size > 0 else io.DEFAULT_BUFFER_SIZE
-                stream_copy(proc.stdout, output_stream, max_chunk_size)
-                stdout_value = proc.stdout.read()
-                stderr_value = proc.stderr.read()
+                if proc.stdout is not None:
+                    stream_copy(proc.stdout, output_stream, max_chunk_size)
+                    stdout_value = proc.stdout.read()
+                if proc.stderr is not None:
+                    stderr_value = proc.stderr.read()
                 # Strip trailing "\n".
-                if stderr_value.endswith(newline):  # type: ignore[arg-type]
+                if stderr_value is not None and stderr_value.endswith(newline):  # type: ignore[arg-type]
                     stderr_value = stderr_value[:-1]
                 status = proc.wait()
             # END stdout handling
         finally:
-            proc.stdout.close()
-            proc.stderr.close()
+            if proc.stdout is not None:
+                proc.stdout.close()
+            if proc.stderr is not None:
+                proc.stderr.close()
 
         if self.GIT_PYTHON_TRACE == "full":
             cmdstr = " ".join(redacted_command)
diff --git a/test/test_git.py b/test/test_git.py
@@ -6,6 +6,7 @@
 import contextlib
 import gc
 import inspect
+import io
 import logging
 import os
 import os.path as osp
@@ -201,6 +202,25 @@ def test_it_logs_istream_summary_for_stdin(self, case):
     def test_it_executes_git_and_returns_result(self):
         self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$")
 
+    def test_it_output_stream_with_stdout_is_false(self):
+        temp_stream = io.BytesIO()
+        self.git.execute(
+            ["git", "version"],
+            output_stream=temp_stream,
+            with_stdout=False,
+        )
+        self.assertEqual(temp_stream.tell(), 0)
+
+    def test_it_executes_git_without_stdout_redirect(self):
+        returncode, stdout, stderr = self.git.execute(
+            ["git", "version"],
+            with_extended_output=True,
+            with_stdout=False,
+        )
+        self.assertEqual(returncode, 0)
+        self.assertIsNone(stdout)
+        self.assertIsNotNone(stderr)
+
     @ddt.data(
         # chdir_to_repo, shell, command, use_shell_impostor
         (False, False, ["git", "version"], False),
diff --git a/test/test_util.py b/test/test_util.py
@@ -113,7 +113,7 @@ def test_deletes_dir_with_readonly_files(self, tmp_path):
         sys.platform == "cygwin",
         reason="Cygwin can't set the permissions that make the test meaningful.",
     )
-    def test_avoids_changing_permissions_outside_tree(self, tmp_path):
+    def test_avoids_changing_permissions_outside_tree(self, tmp_path, request):
         # Automatically works on Windows, but on Unix requires either special handling
         # or refraining from attempting to fix PermissionError by making chmod calls.
 
@@ -125,9 +125,32 @@ def test_avoids_changing_permissions_outside_tree(self, tmp_path):
 
         dir2 = tmp_path / "dir2"
         dir2.mkdir()
-        (dir2 / "symlink").symlink_to(dir1 / "file")
+        symlink = dir2 / "symlink"
+        symlink.symlink_to(dir1 / "file")
         dir2.chmod(stat.S_IRUSR | stat.S_IXUSR)
 
+        def preen_dir2():
+            """Don't leave unwritable directories behind.
+
+            pytest has difficulties cleaning up after the fact on some platforms,
+            e.g., macOS, and whines incessantly until the issue is resolved--regardless
+            of the pytest session.
+            """
+            rwx = stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR
+            if not dir2.exists():
+                return
+            if symlink.exists():
+                try:
+                    # Try lchmod first, if the platform supports it.
+                    symlink.lchmod(rwx)
+                except NotImplementedError:
+                    # The platform (probably win32) doesn't support lchmod; fall back to chmod.
+                    symlink.chmod(rwx)
+            dir2.chmod(rwx)
+            rmtree(dir2)
+
+        request.addfinalizer(preen_dir2)
+
         try:
             rmtree(dir2)
         except PermissionError:
