Skip to content

fix(tools): decode overlap command output as UTF-8 - #1989

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
yeahboi233:fix/pr-overlap-utf8
Oct 9, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
yeahboi233:fix/pr-overlap-utf8

Conversation

@yeahboi233

Copy link
Copy Markdown
Contributor

What

On Windows with a legacy cp1252 locale, the overlap checker decodes the GitHub CLI's UTF-8 output using the host code page. Unicode in a pull-request body can then raise UnicodeDecodeError before any branches are compared.

Explicitly decode stdout and stderr as UTF-8 in the GitHub CLI, Git and merge-tree subprocess calls. This preserves Unicode metadata, diff text, conflict paths and error messages while retaining the existing exit-status handling. Fixes #1949.

Based on main f145139676a48306681d3a4d5fabf83938759daf. No open PR implements this decoding fix. #1741 changes export detection in the same file; its complete patch applies cleanly alongside this change, and both test suites pass together. The encoding tests have their own file and CTest registration. There is no dependency on #1741.

Tested

macOS 26.6.2, Apple Silicon arm64, AppleClang 21, Python 3.12.14, Release relinker-only build:

  • Before the fix: six of the eight new regression checks fail with cp1252 decoding errors; the two ASCII controls pass.
  • After the fix: all eight checks pass. They run real child processes that emit UTF-8 while simulating a cp1252 default decoder, and verify decoded JSON/text, Unicode conflict paths, subprocess failures and successful merges. No live GitHub calls are needed by the tests.
  • Full registered relinker-only CTest suite: 35/35 passed in 2.69 s, including pr_overlap_encoding.
  • Isolated copy with the complete fix(tools): count only definitions as exports in the overlap report #1741 patch: eight encoding checks and three export checks pass together.
  • python tools/check_conventions.py --base origin/main and git diff origin/main --check pass.

Native Windows execution has not been verified locally. The tests simulate its legacy code page; no game files or console are required.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source (none added)
  • Generic behaviour, not specific to one title
  • Depends on: none
  • AI-assisted: yes; implementation, regression tests and this description were written with Codex assistance

@github-actions github-actions Bot added the build label Oct 9, 2026
@yeahboi233
yeahboi233 marked this pull request as ready for review October 9, 2026 03:46

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the concurrent #1990, which covers the same tools/pr_overlap.py UTF-8 decoding fix and also sets encoding="utf-8" when writing per-PR report files. That output-file change belongs to this issue too: once Unicode PR metadata is decoded, writing it with the Windows locale can still fail for characters outside cp1252. Your PR has the stronger registered regression coverage; please coordinate with #1990’s author and consider carrying that one-line report fix here so one PR resolves the complete Windows UTF-8 path.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the consolidation direction: #1990 is now closed as a duplicate. I checked this head’s Actions runs; Build and Conventions are both action_required, so no upstream CI has run yet. This account has read-only permission on the upstream repository and cannot approve those workflows; a maintainer will need to approve them before the review is complete. Please also keep the UTF-8 report-file write as the follow-up promised in #1990 after this PR merges.

@MarcZX10

MarcZX10 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

I reviewed the complete diff and current checks: the UTF-8 decoding change is covered by the registered regression tests, and all reported Linux, Windows, macOS relinker, and conventions checks pass. #1990 is now closed by its author as the duplicate, with the report-file encoding change deferred until this lands.

One contribution-rule follow-up: this PR's base is still f145139676a48306681d3a4d5fabf83938759daf, while current upstream main is 6cc0503dacc9d9697d23001d4103780ad2e9acba. Please rebase onto current main and rerun the checks on the rebased head before merge.

@MarcZX10

MarcZX10 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Contribution-rule follow-up: upstream main has advanced to d9484c719b2a5ac6201e27f8657e0fb0f01e648c since my previous note. This PR is still based on f145139676a48306681d3a4d5fabf83938759daf, so please rebase onto the current main and rerun checks on the rebased head. Under the now-merged #2113 CI policy, Conventions and eligible relinker jobs run automatically; a full Linux/Windows Build must be started by an upstream collaborator from main with this PR number.

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown

Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.

Pull request Conflicting files (lines on main) Same exports Same new files
#2127 CMakeLists.txt L39

@oneandonlydean
oneandonlydean merged commit 3bd74c1 into boykopovar:main Oct 9, 2026
7 checks passed
@MarcZX10 MarcZX10 mentioned this pull request Oct 9, 2026
7 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tools: pr_overlap crashes on Unicode PR metadata on Windows

3 participants