-
Notifications
You must be signed in to change notification settings - Fork 635
units: Restore original EOLs #4268
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
base: master
Are you sure you want to change the base?
Conversation
Do not manage EOL for any test input that have non-LF EOLs, in order to keep the tests as they were originally meant to be. Since 1efb9b9 all EOLs are converted by default, which is a problem for test files requiring a specific EOL type to work. Those should not see any conversion whatsoever, basically treated as binary data. The changes here are mostly generated with: ``` $ find Units/ -name 'input*' -exec file '{}' ';' | \ grep CR | \ sed 's/: .*$//' | \ while read -r f; do \ d="${f%/*}"; \ b="${f##*/}"; \ echo "$b -text" > "$d/.gitattributes"; \ git add "$d/.gitattributes"; \ done ```
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4268 +/- ##
=======================================
Coverage 85.95% 85.95%
=======================================
Files 246 246
Lines 63470 63434 -36
=======================================
- Hits 54554 54525 -29
+ Misses 8916 8909 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
When I proposed #3425 after getting the pull request #3424. To merge #3424, I merged #3425 quickly.
This is one way. I don't object to this change. If I get a pull request similar to the original commit of #3424, I can now advise the author of the new pull request what to do. However, I like this pull request more than the "just-one-line" solution.
|
OK. The only thing is that if Basically the issue I see is that test case inputs might not be consistent across checkouts, which might lead to confusion or let weird bugs go unnoticed. OTOH if the line ending shouldn't matter for the issue, maybe it's a potentially good thing (although a bit unreliable) that it can get checked out with a wider range of EOLs to create virtual additional variations tested. But maybe if we really want this we should add the specific versions we want to test, which would not depend on luck. |
Sorry to be late. This issue is so complex that I feel like running away.
It is very problematic.
What do you think? |
Hum, but do we want to specify everything manually? If we can let Git not change anything in all test input, wouldn't that be better? |
Do not manage EOL for any test input that have non-LF EOLs, in order to keep the tests as they were originally meant to be.
Since 1efb9b9 all EOLs are converted by default, which is a problem for test files requiring a specific EOL type to work. Those should not see any conversion whatsoever, basically treated as binary data.
The changes here are mostly generated with:
@masatake I see there was an attempt at this before #3425, what was the exact issues it created?
Couldn't we to something like this? (remove the text attribute rather trying to remove the
auto
value)