Add fallback-encoding per-repo option for non-utf8 text files - #388
Add fallback-encoding per-repo option for non-utf8 text files#388tgulacsi wants to merge 2 commits into
Conversation
salemhilal
left a comment
salemhilal
left a comment
There was a problem hiding this comment.
Hey, thank you for contributing! Encoding stuff can be pretty scary — can you add some tests to your changes? We're putting some eyes on this PR now otherwise, but we'd want some test coverage before we merge.
| manifestFilename = "metadata.gob" | ||
| excludedFileJsonFilename = "excluded_files.json" | ||
| filePeekSize = 2048 | ||
| filePeekSize = 1 << 20 |
There was a problem hiding this comment.
Hello! Why this big jump in filePeekSize?
There was a problem hiding this comment.
That pesky comment that contains á that breaks UTF-8 detection may be at the end of the file.
This is not a full solution, just a bet that source files should be less than a megabyte long.
A full solution could be retrying the whole indexing on decoding error.
But that felt more involved - though I could try it if you confirm that'd be better.
|
Hey! We're meeting now and generally like this PR. We have two asks before we merge it:
Thank you again! |
The big peek buf was needed to ensure that a non-utf8 rune at the end of the file does get the attention and be tried with the fallback encoding. A more robust approach is to just try the reading as-is, validating the encoding, and try the fallback encoding if this fails. Also add a test case.
|
Sorry for my slow reply, but now I've added a test case, and implemented the more robust retrying logic. |
|
May I assist reviewing this PR? |
3 participants
This allows to specify an alternate charset (fallback-encoding) per repository.