Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not store binary data inside php files #13911

Conversation

mikhainin
Copy link
Contributor

Follow up on #10404, the initial description:

For the sake of performance, the hex2bin call was removed from the generated PHP code in #8006
However, after this PR all autogenerated files contain binary data, which makes it hard or even impossible to see using common tools (git and github included (i.e. this file), as well as some code editors) since most software considers such files as binary ones and not as code.

Alexander suggested using a hex representation of string literals and I updated the original patch a bit, not hex-encoding printable characters.

Benchmarks from #10404#issuecomment-1635939062

The percent of printable chars: 0.83869 (80112 vs 15408)
hex parsing: 3.22371, length=382090
hex2Bin parsing: 1.18489, length=191059
hexAscii parsing: 0.89524, length=142306 (suggested option)
binary parsing: 0.26437, length=95542

@mikhainin mikhainin requested a review from a team as a code owner September 8, 2023 08:38
@mikhainin mikhainin requested review from anandolee and removed request for a team September 8, 2023 08:38
@haberman haberman added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Sep 21, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Sep 21, 2023
@haberman
Copy link
Member

Some tests are failing, and they look unrelated to your change. Could you merge with main and re-push?

@mikhainin mikhainin force-pushed the do_not_store_binary_data_inside_php_files branch from 7676ac3 to 2538cae Compare September 22, 2023 20:02
@mikhainin
Copy link
Contributor Author

Apologies for this. I rebased from main and force-pushed the branch

@haberman haberman added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Sep 25, 2023
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Sep 25, 2023
@mikhainin
Copy link
Contributor Author

Found one more issue with binary data.

When the repository has settings in .gitattributes:

*.php text eol=lf

git will break protobuf descriptors and will give developers hard debugging times. The same effect will be achieved if git config core.autocrlf is set to true.

Not sure if we need to make it a separate issue or no: it definitely worsens developer experience but probably not directly relates to this PR.

@mikhainin
Copy link
Contributor Author

@haberman, would it be possible to take a look by any chance?

@haberman haberman added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 2, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants