fix(file): don't flag UTF-8 files as binary on truncated read sample - #81493
Closed
branchingjade wants to merge 1 commit into
Closed
fix(file): don't flag UTF-8 files as binary on truncated read sample#81493branchingjade wants to merge 1 commit into
branchingjade wants to merge 1 commit into
Conversation
head -c 1000 can split a UTF-8 multi-byte char at the 1000-byte boundary; the truncated tail decodes to a single U+FFFD even in legitimate UTF-8 text (common with CJK files). The old check treated any U+FFFD as binary, making large CJK text files read-only. Allow exactly one U+FFFD in the sample: genuine decode failures (e.g. GBK bytes read as UTF-8) produce many replacement chars and are still caught by the >1 threshold.
alt-glitch
added
type/bug
Collaborator
Contributor
|
Closing — superseded by #81961, which fixed this bug class (1000-byte sample cutting a multibyte char → binary false positive) at the byte layer across read_file, patch, and search_files, with regression tests for truncated-CJK/BOM/UTF-16/NUL cases. ~24 PRs raced on this one; earliest diagnosis credit to @webtecnica (#76924), merged implementation from @ayushnangia (#80440). Thanks for jumping on it. |
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
3 participants
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.
Summary
read_fileuses a 1000-bytehead -csample to decide whether a file is binary. When the 1000th byte splits a UTF-8 multi-byte character, the truncated tail decodes (witherrors="replace") to exactly one U+FFFD — even in perfectly legitimate UTF-8 text. This is very common with CJK files (3 bytes per char).The old check (
"\ufffd" in sample) treated any U+FFFD as binary, so large CJK text files (>= ~333 chars) were permanently flagged binary/read-only.Change
Allow exactly one U+FFFD in the sample:
sample.count("\ufffd") > 1flags binary. Genuine decode failures (GBK bytes read as UTF-8, latin-1 decoded with replace) produce many replacement chars and are still caught.Tests
Added to
tests/tools/test_file_operations.py::TestReadNonUtf8IsBinary:test_truncated_cjk_tail_single_ufffd_not_flagged— 333 CJK chars (999 bytes) + truncated char tail → 1 U+FFFD, must NOT be binarytest_many_ufffd_still_flagged_binary— mojibake with dozens of U+FFFD still flaggedVerified: 6/6 binary-related tests pass; full-file run shows no new failures vs. base (8 pre-existing Windows-only failures: POSIX file-mode + symlink tests).