-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: Improve handling of escaped markers in apply_diff #2274
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
Conversation
unescapeMarkers() is supposed to fix escaped merge conflict markers so that they match the code. However the function has a bug which requires SEARCH and REPLACE strings in the markers to be replaced. This is not part of merge conflict markers, so many valid cases were previously missed.
|
|
Revert changes to existing tests, this change should not break tests I do not think. If this change does require a test update, then please explain what happened and why... |
|
also if you have a good argument for making changes to the tests, then I am listening, but I like to be conservative making changes to tests especially when a code change should not affect them. |
|
I want to get this into tonight's release, so I pushed a change to revert changes to existing tests and to add a new test specific to this functionality. Hopefully not stepping on toes. Thanks for the work on this! |
fcc79dc to
63b29df
Compare
|
@mrubens I think the test changes were needed because the crux of the fix is that more generic escaped markers are accepted. We have many tests that check escaped |
Perfect, that is what I was looking for: keep existing tests and add new specific tests. If everything passes, then I'm good w/ the changes. Thanks @p12tic for getting this prepared! |
Context
unescapeMarkers() is supposed to fix escaped merge conflict markers so that they match the code. However the function has a bug which requires SEARCH and REPLACE strings in the markers to be replaced. This is not part of merge conflict markers, so many valid cases were previously missed.
The change was requested in #2260 (comment).
Implementation
Adjusted replacement to be less strict.
Screenshots
Not applicable.
How to Test
Unit tests
Important
Fixes
unescapeMarkersinmulti-search-replace.tsto handle escaped markers without requiring specific strings, updating tests accordingly.unescapeMarkersinmulti-search-replace.tsto handle escaped markers without requiring 'SEARCH' or 'REPLACE'.applyDiffto match new unescape logic.multi-search-replace.test.tsto remove 'SEARCH' and 'REPLACE' from escaped markers in test cases.This description was created by
for 4af2786. It will automatically update as commits are pushed.