fix(tools): decode git output as UTF-8 in working_diff on Windows
_run() used text=True without an encoding, so Windows decoded git's UTF-8 output with the locale code page (cp932) and raised UnicodeDecodeError on non-ASCII filenames or diff content, breaking the "Never raises on git failure" contract in its docstring. Match the utf-8 + errors="replace" policy checkpoint_manager's _run_git already uses. Legacy cp932-encoded blob content degrades to replacement characters instead of crashing; a test pins that trade-off so it stays a documented choice. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -10,6 +10,7 @@ import subprocess
|
||||
|
||||
import pytest
|
||||
|
||||
import tools.working_diff as working_diff
|
||||
from tools.working_diff import collect_working_diff
|
||||
|
||||
pytestmark = pytest.mark.skipif(
|
||||
@@ -57,3 +58,66 @@ def test_unknown_mode_rejected(repo):
|
||||
result = collect_working_diff(str(repo), mode="bogus")
|
||||
assert result["success"] is False
|
||||
assert "bogus" in result["error"]
|
||||
|
||||
|
||||
def test_run_decodes_git_output_as_utf8(monkeypatch, repo):
|
||||
"""``_run`` must force UTF-8 decoding of git's output.
|
||||
|
||||
Without ``encoding="utf-8"`` (and a lossy ``errors=``), ``subprocess.run``
|
||||
falls back to the platform locale encoding. On Windows that's typically
|
||||
cp932: UTF-8 multibyte output (e.g. a Japanese filename or diff content)
|
||||
then either raises ``UnicodeDecodeError`` or silently decodes as mojibake,
|
||||
depending on the byte sequence. The raise violates the "Never raises on
|
||||
git failure" contract in ``_run``'s docstring.
|
||||
"""
|
||||
captured = {}
|
||||
real_run = subprocess.run
|
||||
|
||||
def fake_run(*args, **kwargs):
|
||||
captured.update(kwargs)
|
||||
return real_run(*args, **kwargs)
|
||||
|
||||
monkeypatch.setattr(working_diff.subprocess, "run", fake_run)
|
||||
|
||||
working_diff._run(["status"], str(repo))
|
||||
|
||||
assert captured.get("encoding") == "utf-8"
|
||||
assert captured.get("errors") == "replace"
|
||||
|
||||
|
||||
def test_non_ascii_untracked_file_does_not_raise(repo):
|
||||
"""A non-ASCII filename + content must decode cleanly, not raise.
|
||||
|
||||
Regression test for the UnicodeDecodeError observed on real Windows
|
||||
machines when git output contains UTF-8 multibyte sequences (Japanese
|
||||
filename/content here) and the platform locale is not UTF-8.
|
||||
"""
|
||||
(repo / "日本語ファイル.py").write_text("x = 'こんにちは'\n", encoding="utf-8")
|
||||
|
||||
result = collect_working_diff(str(repo))
|
||||
|
||||
assert result["success"] is True
|
||||
assert any("日本語ファイル.py" in f for f in result["untracked"])
|
||||
assert "こんにちは" in result["diff"]
|
||||
|
||||
|
||||
def test_cp932_content_is_lossy_but_never_raises(repo):
|
||||
"""Non-UTF-8 blob content degrades to replacement characters, not a crash.
|
||||
|
||||
Git emits blob bytes uninterpreted, so a repo whose files are cp932-encoded
|
||||
decodes lossily under the forced UTF-8 policy. That is the same trade-off
|
||||
checkpoint_manager's ``_run_git`` already makes (utf-8 + errors="replace"
|
||||
on every git call): a readable-but-lossy diff for legacy encodings, never
|
||||
an exception. This test pins the trade-off so it stays a documented choice.
|
||||
"""
|
||||
legacy = repo / "legacy.txt"
|
||||
legacy.write_bytes("before=東京\n".encode("cp932"))
|
||||
_git(repo, "add", ".")
|
||||
_git(repo, "commit", "-q", "-m", "legacy")
|
||||
legacy.write_bytes("after=日本語\n".encode("cp932"))
|
||||
|
||||
result = collect_working_diff(str(repo))
|
||||
|
||||
assert result["success"] is True
|
||||
assert "legacy.txt" in result["diff"]
|
||||
assert "\ufffd" in result["diff"] # lossy by design, matching _run_git
|
||||
|
||||
@@ -35,6 +35,7 @@ def _run(args: List[str], cwd: str, timeout: int = _GIT_TIMEOUT):
|
||||
proc = subprocess.run(
|
||||
["git", "-c", "core.quotePath=false", *args],
|
||||
cwd=cwd, capture_output=True, text=True, timeout=timeout,
|
||||
encoding="utf-8", errors="replace",
|
||||
)
|
||||
return proc.returncode, proc.stdout
|
||||
|
||||
|
||||
Reference in New Issue
Block a user