Fix binary patching #3449
Merged
Fix binary patching #3449
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The logic for the CRLF patch adaptation failed to take into account mixed CRLF-ending files (i.e. binary files) which should obviously be left alone. read_lines updated to return a tri-state indicating if the file is CRLF-encoded, LF-encoded or mixed. The patching logic is now altered to accumulate patch lines and the patch is only adapted if both the target file requires it and also the patch itself is consistent.
Previous implementation held the entire patch in memory and with the correction for binary patching was doing silly operations over lists of lines. This version, which is fractionally simpler, accepts scanning the file three times: 1. Works out whether the entire patch is CRLF encoded 2. Works out what to do about each file's chunks 3. Actually writes the translated patch. Processing this way removes several minor inconsistencies from the previous implementation.
dra27
force-pushed
the
fix-binary-patching
branch
from
6246c99
to
eec9862
Compare
Merged
|
Just tested on an old repo of mine that gave me errors like: and I can confirm it seems to work fine with this new version ! |
AltGr
approved these changes
Jul 20, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is still slightly in progress.
The first commit fixes
OpamSystem.translate_patchcorrupting binary patches by incorrectly applying CRLF transformations. The revised version both detects that a file has mixed line endings (i.e. some are CRLF and some are LF) and does the same thing with all the chunks of the patch. If either the file being patched or the patch chunks themselves have mixed endings, then no translations occur. The edge cases for binary files which happen to be CRLF or LF encoded are also sound (the patch will either maintain them and, if it changes the "endings" then the+lines and-lines would have different endings which would also disable the function).However, doing that has made the original function even uglier than it was and so the following commit (which is the work-in-progress part - I just haven't tidied it up) rewrites the function. Before,
translate_patchread the entire patch into memory (mostly reasonable) and folded the list of lines once. The problem is that the chunk line-ending detection means that lines now have to be cached in the state machine, which is abitlot hideous. The new version accepts that the file is read 3 times - the first to determine if GNUpatch's CR-stripping will come into effect, the second time to work out which lines may or may not require CR characters to be added or removed and then the third time to actually write the patch.It also seems prudent to start testing this, and I've written a rudimentary test harness for that function. There are a couple of commits added which control the debug output a bit further, which allows the log to be used as a reference file.