fix(windows): decode git output as UTF-8, not the process locale - #293
Open
ppcvote wants to merge 1 commit into
Open
fix(windows): decode git output as UTF-8, not the process locale#293ppcvote wants to merge 1 commit into
ppcvote wants to merge 1 commit into
Conversation
Git reports paths as UTF-8 on every platform. `subprocess.run(..., text=True)` decodes with the process locale, which on Windows is the ANSI codepage, so any non-ASCII path is decoded with the wrong codec. Fixes openai#192, and covers a second call site the report does not reach. The failure has two shapes depending on the codepage, which is why the filed report and this one look different: - Single-byte (cp1251, the reporter's): every byte decodes, so nothing raises and a silently wrong string comes back. `Path.relative_to` then fails and the scan is refused with "Scan target must stay inside its Git working tree." - Multi-byte (cp932/936/950): the sequence is rejected outright. The UnicodeDecodeError is raised on subprocess's reader thread, so `run` returns with returncode 0 and `stdout` set to None, and `git_output` calls `.strip()` on it. That surfaces as `AttributeError: 'NoneType' object has no attribute 'strip'` rather than a diagnosable message. Reproduced on Windows 10, Python 3.11.6, ANSI codepage cp950, git 2.52.0: before CJK repo directory AttributeError ... 'strip' before Cyrillic repo directory AttributeError ... 'strip' before CJK file in ASCII repo AttributeError ... 'split' after all three resolve correctly, ASCII control unchanged The third case is outside openai#192's scope and is the reason for the second file. `generate_rank_input.run_git_changed_paths` reads names out of `git diff --name-status -z`, so the repository path does not need to contain non-ASCII at all: one tracked file with a non-ASCII name is enough to end the scan on any non-UTF-8 Windows host. Naming the encoding is what PYTHONUTF8=1 achieves, which the reporter confirmed as a workaround, without depending on the environment. `git_bytes` is untouched and still returns undecoded bytes; the text branch is now explicit so the two modes cannot be conflated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #192.
Git reports paths as UTF-8 on every platform.
subprocess.run(..., text=True)decodes with the process locale, which on Windows is the ANSI codepage, so a non-ASCII path is decoded with the wrong codec.I hit this on a Traditional Chinese host (cp950) and the symptom did not match the filed report, which turned out to be informative rather than a separate bug.
Two shapes, one cause
Path.relative_tofails → "Scan target must stay inside its Git working tree."UnicodeDecodeErroron subprocess's reader thread, sorunreturns withreturncode 0andstdoutNone, andgit_outputcalls.strip()on it →AttributeError: 'NoneType' object has no attribute 'strip'The same bytes, decoded five ways:
A second call site, outside the issue's scope
Sweeping the plugin for the same pattern found three
text=Truecalls with noencoding=, and no call anywhere that passes one. Two route throughgit_command; the third is separate:generate_rank_input.run_git_changed_pathsreads names out ofgit diff --name-status -z. The repository path does not need to contain non-ASCII at all — one tracked file with a non-ASCII name is enough:That widens the population from "checkout is in a non-ASCII folder" to "any repository containing a non-ASCII file name, scanned from a non-UTF-8 Windows host".
File I/O in the plugin is already binary (
"rb"/"wb") throughout, so this is the whole exposure I could find.Verification
Windows 10, Python 3.11.6, ANSI codepage cp950, git 2.52.0.windows.1, exercising the patched modules directly rather than a copy of their logic:
The Cyrillic row is the reporter's own case, confirmed fixed on this host. The ASCII control and
git_bytesrows are there to show the text-mode branch did not disturb the binary path.Notes on the change
encoding="utf-8"is exactly whatPYTHONUTF8=1achieves, which Windows: valid Git worktree scans fail for non-ASCII target paths #192 confirms as a workaround, without depending on the environment being set.errors=handler. On Windows the filesystem is UTF-16 and git converts to UTF-8, so strict decoding is correct here; addingsurrogateescapewould only move a failure into JSON serialisation later.text=text, becauseencoding=implies text mode and the two modes should not be conflated. That also let theFileNotFoundErrorfallback drop itsstr | bytesunion.Happy to split the second file into its own PR if you would rather keep this scoped strictly to #192.