Skip to content

Commit f5ca4a5

Browse files
codexByron
authored andcommitted
fix(cmd): preserve bytes in streamed output and diagnostics
Strict text decoding could make fetch/push output readers fail after Git successfully transferred refs, leaving empty result lists. Clone diagnostics, progress messages, failed-process stderr and timeout messages could likewise raise Unicode errors when names contained arbitrary bytes. Use the shared lossless helpers for output pumps, progress parsing and error re-encoding. Default subprocess text streams to surrogateescape while respecting an explicit errors policy. Keep binary streams binary and retain existing newline, timeout and process-finalization behavior. Nine regressions demonstrate the former failures and pass after the fix. They cover synchronous/streamed output, binary pumps, saved stderr, timeout and clone diagnostics, and actual SHA-1/SHA-256 fetch/push results. Reftable fixtures carry byte refs without invalid loose filenames or byte-valued refspec arguments. The connected Gix command/remote run passes 295 tests and four subtests; its remaining remote test passes with the project runner's isolated Git config instead of inherited fetch.prune=true. Ruff and mypy pass.
1 parent 5cbdf76 commit f5ca4a5

5 files changed

Lines changed: 113 additions & 8 deletions

File tree

‎doc/source/changes.rst‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,9 @@ Git executable and safety
4747
* Known command outcomes retain established GitPython/configparser exceptions
4848
where distinguishable. Other failures raise ``GitCommandError`` with Git's
4949
exit status and diagnostic output; exact error text may differ.
50+
Text and streamed command output preserve undecodable bytes with
51+
``surrogateescape``, including fetch/push results, clone errors and timeout
52+
diagnostics containing byte names.
5053

5154
API changes
5255
-----------

‎git/cmd.py‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@
4343
overload,
4444
)
4545

46-
from git.compat import defenc, force_bytes, safe_decode
46+
from git.compat import defenc, safe_decode, safe_encode
4747
from git import _backend
4848
from git.exc import (
4949
CommandError,
@@ -199,7 +199,7 @@ def pump_stream(
199199
if handler:
200200
if is_decode:
201201
assert isinstance(line, bytes)
202-
line_str = line.decode(defenc)
202+
line_str = safe_decode(line)
203203
handler(line_str)
204204
else:
205205
handler(line)
@@ -451,15 +451,15 @@ def wait(self, stderr: Union[None, str, bytes] = b"") -> int:
451451
status = self.status
452452
p_stderr = None
453453

454-
stderr_b = force_bytes(data=stderr, encoding="utf-8") or b""
454+
stderr_b = safe_encode(stderr) or b""
455455
if self._timeout_error is not None:
456456
stderr_b += (b"\n" if stderr_b else b"") + self._timeout_error.encode("utf-8")
457457
status = status or 1
458458

459459
def read_all_from_possibly_closed_stream(stream: Union[IO[bytes], None]) -> bytes:
460460
if stream:
461461
try:
462-
return stderr_b + force_bytes(stream.read())
462+
return stderr_b + safe_encode(stream.read())
463463
except (OSError, ValueError):
464464
return stderr_b or b""
465465
else:
@@ -1457,8 +1457,9 @@ def execute(
14571457
If ``True``, default ``True``, we open stdout on the created process.
14581458
14591459
:param universal_newlines:
1460-
If ``True``, pipes will be opened as text, and lines are split at all known
1461-
line endings.
1460+
If ``True``, pipes are opened as text with CR, LF and CRLF newline
1461+
handling. Undecodable bytes are preserved with ``surrogateescape``
1462+
unless an explicit ``errors`` subprocess argument selects another policy.
14621463
14631464
:param shell:
14641465
Whether to invoke commands through a shell
@@ -1569,6 +1570,8 @@ def execute(
15691570
shell,
15701571
universal_newlines,
15711572
)
1573+
if universal_newlines:
1574+
subprocess_kwargs.setdefault("errors", "surrogateescape")
15721575
try:
15731576
proc = safer_popen(
15741577
command,
@@ -1609,7 +1612,7 @@ def kill_process(pid: int) -> None:
16091612

16101613
def make_timeout_error() -> Union[str, bytes]:
16111614
err = f'Timeout: the command "{" ".join(redacted_command)}" did not complete in {timeout:g} secs.'
1612-
return err if universal_newlines else err.encode(defenc)
1615+
return err if universal_newlines else safe_encode(err)
16131616

16141617
def communicate() -> Tuple[Union[str, bytes, None], Union[str, bytes, None]]:
16151618
assert watchdog is not None

‎git/util.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@
8585
from git.remote import Remote
8686
from git.repo.base import Repo
8787

88+
from git.compat import safe_decode
8889
from git.types import (
8990
HSH_TD,
9091
Files_TD,
@@ -728,7 +729,7 @@ def _parse_progress_line(self, line: AnyStr) -> Optional[object]:
728729
# Compressing objects: 100% (2/2)
729730
# Compressing objects: 100% (2/2), done.
730731
if isinstance(line, bytes): # mypy argues about ternary assignment.
731-
line_str = line.decode("utf-8")
732+
line_str = safe_decode(line)
732733
else:
733734
line_str = line
734735
self._cur_line = line_str

‎test/test_byte_output.py‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
"""Git output carries paths and reference names with arbitrary bytes."""
2+
3+
import os
4+
import sys
5+
6+
import pytest
7+
8+
from git import Git, GitCommandError, Repo, RemoteProgress
9+
from git.cmd import handle_process_output
10+
from git.compat import safe_decode, safe_encode
11+
from test.lib.fixtures import writable_fixture
12+
13+
14+
@pytest.mark.parametrize("as_process", [False, True])
15+
def test_text_process_output_preserves_bytes(as_process):
16+
proc = Git().execute(
17+
[sys.executable, "-c", "import sys; sys.stdout.buffer.write(b'name-\\xff\\n')"],
18+
universal_newlines=True,
19+
as_process=as_process,
20+
)
21+
if as_process:
22+
lines = []
23+
handle_process_output(proc, lines.append, None, decode_streams=False)
24+
proc.wait()
25+
assert safe_encode("".join(lines)) == b"name-\xff\n"
26+
else:
27+
assert safe_encode(proc) == b"name-\xff"
28+
29+
30+
def test_binary_process_output_handler_preserves_bytes():
31+
proc = Git().execute(
32+
[sys.executable, "-c", "import sys; sys.stdout.buffer.write(b'name-\\xff\\n')"], as_process=True
33+
)
34+
lines = []
35+
handle_process_output(proc, lines.append, None)
36+
proc.wait()
37+
assert safe_encode("".join(lines)) == b"name-\xff\n"
38+
39+
40+
def test_process_error_preserves_previously_read_bytes():
41+
proc = Git().execute([sys.executable, "-c", "raise SystemExit(1)"], as_process=True)
42+
with pytest.raises(GitCommandError) as error:
43+
proc.wait(stderr=safe_decode(b"fatal: name-\xff"))
44+
assert "name-\udcff" in str(error.value)
45+
46+
47+
def test_progress_preserves_byte_diagnostics():
48+
progress = RemoteProgress()
49+
progress._parse_progress_line(b"fatal: name-\xff")
50+
assert safe_encode(progress.error_lines[0]) == b"fatal: name-\xff"
51+
52+
53+
@pytest.mark.skipif(os.name == "nt", reason="POSIX timeout and byte-argument handling")
54+
def test_timeout_preserves_byte_arguments():
55+
with pytest.raises(GitCommandError) as error:
56+
Git().execute(
57+
[sys.executable, "-c", "import time; time.sleep(5)", safe_decode(b"name-\xff")],
58+
kill_after_timeout=0.1,
59+
)
60+
assert "name-\udcff" in str(error.value)
61+
62+
63+
@pytest.mark.skipif(os.name == "nt", reason="Windows arguments cannot express arbitrary path bytes")
64+
def test_clone_preserves_byte_path_in_failure():
65+
with writable_fixture("remote-byte-output.sh", "sha1") as temporary:
66+
root = temporary.path()
67+
with pytest.raises(GitCommandError) as error:
68+
Repo.clone_from(root / safe_decode(b"missing-\xff"), root / "clone")
69+
assert "missing-\udcff" in str(error.value)
70+
71+
72+
@pytest.mark.parametrize("object_format", ["sha1", "sha256"])
73+
def test_fetch_and_push_preserve_byte_reference_results(object_format):
74+
with writable_fixture("remote-byte-output.sh", object_format) as temporary:
75+
root = temporary.path()
76+
with Repo(root / "repo") as repo:
77+
origin = repo.create_remote("origin", str(root / "source"))
78+
fetched = origin.fetch()
79+
assert {safe_encode(info.ref.path) for info in fetched} == {
80+
b"refs/remotes/origin/main",
81+
b"refs/remotes/origin/branch-\xff",
82+
}
83+
with Repo(root / "source") as repo:
84+
target = repo.create_remote("target", str(root / "target"))
85+
pushed = target.push("refs/heads/*:refs/heads/*")
86+
assert not pushed.error
87+
assert {safe_encode(info.remote_ref_string) for info in pushed} == {
88+
b"refs/heads/main",
89+
b"refs/heads/branch-\xff",
90+
}
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
#!/usr/bin/env bash
2+
set -eu
3+
git init --initial-branch=main --object-format="$1" --ref-format=reftable source
4+
git -C source commit --allow-empty -m initial
5+
oid=$(git -C source rev-parse HEAD)
6+
printf 'create refs/heads/branch-\377\0%s\0' "$oid" | git -C source update-ref --stdin -z
7+
git init --initial-branch=main --object-format="$1" --ref-format=reftable repo
8+
git init --bare --object-format="$1" --ref-format=reftable target

0 commit comments

Comments
 (0)