Skip to content

fix(perforce): Support Unicode Perforce server connections#115775

Merged
mujacica merged 2 commits into
masterfrom
fix/perforce-unicode
May 20, 2026
Merged

fix(perforce): Support Unicode Perforce server connections#115775
mujacica merged 2 commits into
masterfrom
fix/perforce-unicode

Conversation

@mujacica
Copy link
Copy Markdown
Contributor

  • Support Unicode Perforce server connections

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 19, 2026
@mujacica mujacica marked this pull request as ready for review May 19, 2026 09:55
@mujacica mujacica requested review from a team as code owners May 19, 2026 09:55
Comment on lines +105 to +108
A Unicode-enabled server rejects clients that don't declare a charset on
connect. The value here is passed verbatim to `p4.charset` before
`p4.connect()`. "none" preserves legacy behavior (non-Unicode server) by
skipping `p4.charset` entirely.
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.

Would it be safe to just use unicode by default when connecting?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sadly there is no handshake between the server and client. Both server and client need to have the same encoding configured so that the connection can succeed - before any data gets exchanged. So if server is unicode, client needs to set unicode. If server is not unicode, client can't set unicode - otherwise it won't work again.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also there is an 'auto' mode - but it just takes the current machine encoding and not the server encoding, since the encoding is needed for connection itself....

Copy link
Copy Markdown
Contributor

@vaind vaind left a comment

Choose a reason for hiding this comment

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

LGTM but for the potential concern about pre-existing integrations.

"initialized in Unicode mode (`p4d -xi`). Unicode servers "
"reject clients that do not declare a charset on connect."
),
"required": True,
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.

required=true

do we need a backfill for already defined integrations?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't be necessary - this code handles installation configuration form. The rest of the code (https://github.com/getsentry/sentry/pull/115775/changes#diff-bd63a841162b9ef7990a5ba091a324f5155871943afd92f750e1d9720f2ecb55R196) and (https://github.com/getsentry/sentry/pull/115775/changes#diff-bd63a841162b9ef7990a5ba091a324f5155871943afd92f750e1d9720f2ecb55R141) - gracefully handles empty states. For the new integrations and/or configuration changes we require this field to be set. Until then the dropdown menu is going to be shown empty.

@mujacica mujacica merged commit c41af98 into master May 20, 2026
64 checks passed
@mujacica mujacica deleted the fix/perforce-unicode branch May 20, 2026 09:50
JonasBa pushed a commit that referenced this pull request May 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants