Skip to content

Backport 4236 #5439

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Backport 4236 #5439

wants to merge 1 commit into from

Conversation

ytmimi
Copy link
Contributor

@ytmimi ytmimi commented Jul 12, 2022

Read the original file for comparing/detecting newline (#4236)

Closes #4097

The code base diverged enough where a simple cherry-pick wasn't possible, but implementing the logic to read the input file when NewlineStyle::Auto was simple enough to implement.

New tests were added to hopefully prevent regressions.

Read the original file for comparing/detecting newline (4236)

Closes 4097

The code base diverged enough where a simple cherry-pick wasn't
possible, but implementing the logic to read the input file when
`NewlineStyle::Auto` was simple enough to implement.

New tests were added to hopefully prevent regressions.
@ytmimi
Copy link
Contributor Author

ytmimi commented Jul 12, 2022

I want to note that the original PR removes code that this PR does not since they take slightly different approaches to how apply_newline_style is called / updated. I haven't looked too deeply into it, but I wonder if we can remove similar code after making these changes.

I was also wondering if we had any tests in test/source that used crlf newlines. I was thinking it might be a good idea to add at least one system test for this backport to ensure that we maintain the newline style of the original input text regardless of the platform when newline_style=Auto

@ytmimi
Copy link
Contributor Author

ytmimi commented Jul 12, 2022

because of updates made to apply_newline_style, there will be some minor conflicts with #5418 depending on which backport PR gets merged first.

@ytmimi ytmimi added the p-low label Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Newline style changes to Unix-style when file is modified
1 participant