fix: do not load binary content #1836
Conversation
License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>
There was a problem hiding this comment.
I suspect loadContent is related to progressive load of big text files added in #1646 – @rafaelramalho19 may remeber more.
Exactly, but such thing is not needed when we detect that we're looking at a binary file. So there's no point on keeping loading it I think. So let's wait for @rafaelramalho19 |
old one used 7 year old package that had hardcoded check of first 24 bytes, which was not always the best test. istextorbinary one checks name and 24 bytes at the start, middle, and end of the buffer – https://github.com/bevry/istextorbinary#readme
There was a problem hiding this comment.
I did some poking and removal of loadcontent lgtm – most likely a scar tissue from past refactors.
While at it, improved isBinary check in 39b172e – takes filenames into account + should behave better around small binary files (old library hardcoded check that assumed 24 bytes, new one is smarter)
Fixes #1819 by stopping to load the content after we determine it is binary.
I'm not completely sure why it works. I tried playing with the
loadContentfunction to see if anything was wrong, but couldn't find the culprit.@rafaelramalho19 I'm assigning you to review to make sure that the removal of that line doesn't have any issues. I'm not sure why we need to call
loadContentafter we determine that it is a binary file.License: MIT
Signed-off-by: Henrique Dias hacdias@gmail.com