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

Fix writing multi-byte characters to tar archive #627

Merged
merged 12 commits into from
Jul 16, 2018
Merged

Conversation

TadCordle
Copy link
Contributor

Fixes #626. Looks like we were using string length instead of byte array length.

@loosebazooka
Copy link
Member

loosebazooka commented Jul 16, 2018

Can we use codePointCount? Not saying we should, just curious.

Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

addEntry() seems to be called by toTarballBlob() to add what's described as a a blob. Why don't we just pass the file contents as byte[] — or better yet as ByteSource — and let the caller deal with the encoding?

If we used ByteSource then we could get rid of the other addEntry(TarArchiveEntry) and use MoreFiles.asByteSource(filePath).

@TadCordle
Copy link
Contributor Author

TadCordle commented Jul 16, 2018

So it looks like the main problem has been fixed, but using multibyte characters in tar archive entry header names on windows might still be sketchy (e.g. adding non-ascii filenames might not work). Multibyte file contents should work on all platforms, though. I'll keep looking into it, but feel free to review this now so we can get the fix out.

@coollog
Copy link
Contributor

coollog commented Jul 16, 2018

@briandealwis Blob is essentially our own minimal interface that is essentially a ByteSource, but you make a good point - can you file a separate issue for that since it's not directly related to this fix?

@TadCordle TadCordle merged commit 4d1b5de into master Jul 16, 2018
@TadCordle TadCordle deleted the fix-2byte branch July 16, 2018 23:04
@coollog
Copy link
Contributor

coollog commented Jul 17, 2018

@TadCordle oh we should add a CHANGELOG entry for this

@chanseokoh
Copy link
Member

Cool, just in case, have you tested dockerBuild and buildTar?

@TadCordle
Copy link
Contributor Author

Yeah, tested both with the same args used in the original issue.

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.

5 participants