Repository navigation
utils/curl: guess charset from a <meta> tag - #24257
Conversation
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
68d7f5f1-0805-4109-8f79-54cc9f2bdff1
📒 Files selected for processing: 2
Library/Homebrew/test/utils/curl_spec.rbLibrary/Homebrew/utils/curl.rb
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout: 19
- GitHub Check: upload_sarif
- GitHub Check: docker (x86_64 Ubuntu 26.04)
- GitHub Check: docker (x86_64 Ubuntu 24.04)
- GitHub Check: docker (arm64 Ubuntu 26.04)
- GitHub Check: docker (arm64 Ubuntu 24.04)
- GitHub Check: tests (Linux, 1/2)
- GitHub Check: tests (load-only macOS)
- GitHub Check: tests (macOS, 2/2)
- GitHub Check: tests (online, 1/2)
- GitHub Check: tests (online, 2/2)
- GitHub Check: tests (no Sorbet)
- GitHub Check: tests (generic OS, 1/2)
- GitHub Check: tests (generic OS, 2/2)
- GitHub Check: tests (macOS, 1/2)
- GitHub Check: tests (Linux, 2/2)
- GitHub Check: tests (load-only Linux)
- GitHub Check: Analyze
- GitHub Check: syntax
- GitHub Check: docs
🔇 Additional comments: 1
Library/Homebrew/test/utils/curl_spec.rb: 1
708-744: LGTM!
07254bc to
a8c223e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
c08c252e-f87a-45a8-b364-336c07927b3d
📒 Files selected for processing: 2
Library/Homebrew/test/utils/curl_spec.rbLibrary/Homebrew/utils/curl.rb
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout: 19
- GitHub Check: docker (arm64 Ubuntu 24.04)
- GitHub Check: docker (x86_64 Ubuntu 26.04)
- GitHub Check: docker (arm64 Ubuntu 26.04)
- GitHub Check: docker (x86_64 Ubuntu 24.04)
- GitHub Check: workflow_syntax
- GitHub Check: tests (load-only macOS)
- GitHub Check: tests (no Sorbet)
- GitHub Check: tests (Linux, 2/2)
- GitHub Check: tests (Linux, 1/2)
- GitHub Check: tests (generic OS, 2/2)
- GitHub Check: tests (online, 1/2)
- GitHub Check: tests (macOS, 2/2)
- GitHub Check: tests (macOS, 1/2)
- GitHub Check: tests (load-only Linux)
- GitHub Check: tests (online, 2/2)
- GitHub Check: tests (generic OS, 1/2)
- GitHub Check: docs
- GitHub Check: syntax
- GitHub Check: Analyze
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Good catch, one wee comment!
a8c223e to
f6ca5f6
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Normalize decoded bodies before comparison. · curl.rb:681-691
Library/Homebrew/utils/curl.rb:681-691
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize decoded bodies before comparison.
When HTTP and HTTPS responses use different ASCII-compatible charsets,
scrubandgsubpreserve those encodings. Equivalent non-ASCII content can therefore compare unequal. Normalize both bodies to UTF-8 beforehttp_content == https_content.Suggested fix
- http_content = details[:file]&.scrub&.gsub(no_protocol_file_contents, "/") - https_content = secure_details[:file]&.scrub&.gsub(no_protocol_file_contents, "/") + http_content = details[:file]&.scrub&.gsub(no_protocol_file_contents, "/")&.encode(Encoding::UTF_8, + invalid: :replace, undef: :replace) + https_content = secure_details[:file]&.scrub&.gsub(no_protocol_file_contents, "/")&.encode(Encoding::UTF_8, + invalid: :replace, undef: :replace)
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
f01bb3fd-19ad-4e7f-97e9-c3ec73cd52c8
📒 Files selected for processing: 2
Library/Homebrew/test/utils/curl_spec.rbLibrary/Homebrew/utils/curl.rb
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout: 17
- GitHub Check: docker (x86_64 Ubuntu 24.04)
- GitHub Check: docker (arm64 Ubuntu 26.04)
- GitHub Check: docker (x86_64 Ubuntu 26.04)
- GitHub Check: docker (arm64 Ubuntu 24.04)
- GitHub Check: docs
- GitHub Check: tests (Linux, 2/2)
- GitHub Check: tests (no Sorbet)
- GitHub Check: tests (macOS, 2/2)
- GitHub Check: tests (online, 2/2)
- GitHub Check: tests (macOS, 1/2)
- GitHub Check: tests (load-only macOS)
- GitHub Check: tests (Linux, 1/2)
- GitHub Check: tests (load-only Linux)
- GitHub Check: tests (generic OS, 2/2)
- GitHub Check: tests (online, 1/2)
- GitHub Check: tests (generic OS, 1/2)
- GitHub Check: syntax
🔇 Additional comments: 2
Library/Homebrew/utils/curl.rb: 1
676-689: LGTM!Library/Homebrew/test/utils/curl_spec.rb: 1
707-756: LGTM!
|
@hyuraku above comment looks worth a look. |
f6ca5f6 to
dab4105
Compare
curl_http_content_headers_and_checksumreads the page body to compare the HTTP and HTTPS versions of a homepage. It only took the charset from the HTTP header, treating pages without one as UTF-8. #13223 left aTODOto also read it from<meta>tags, after a page like https://opencsg.org/ that only declares it there.Now it also detects charsets from
<meta>tags within the first 1024 bytes, supporting bothcharsetandhttp-equivforms. The HTTP header still takes priority.It also ignores non-ASCII-compatible encodings like UTF-16 to prevent
Encoding::CompatibilityErrorduring regex processing.Added tests for charset detection, header priority, the 1024-byte limit, and UTF-16 handling.
brew benchmarkresults.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?AI (Claude Code) helped me pick this
TODOand wrote the code and tests. I reviewed the change and ranbrew lgtmlocally.Summary by CodeRabbit
Content-Typeheader or, when absent, HTML metadata within the first 1,024 bytes. HTML metadata inside comments is ignored.