Skip to content

/ cpython

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

[windows] bpo-27425: Be more explicit in .gitattributes #840

Merged
merged 3 commits into from Jun 10, 2017
Merged

[windows] bpo-27425: Be more explicit in .gitattributes #840

merged 3 commits into from Jun 10, 2017

Conversation

@zware
Copy link
Member

zware commented Mar 27, 2017

This is largely a direct conversion of .hgeol, assuming I understand the rules of .gitattributes correctly. I also went through and made sure there were actually examples of each entry checked in.

A big difference here is that some of the test files (in test_email, xmltestdata, etc) are no longer marked as binary but rather just -text which should mean that they are not subject to EOL conversion, but git diff provides a nice text diff including line ending changes.

It also expands on .hgeol a bit, with a few more Windows-specific files marked as CRLF.
@mention-bot

This comment has been minimized.

Sign in to view
Copy link

mention-bot commented Mar 27, 2017

@zware, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vsajip to be a potential reviewer.
@berkerpeksag
berkerpeksag approved these changes Mar 27, 2017
View changes
@serhiy-storchaka
serhiy-storchaka reviewed Mar 27, 2017
View changes
.gitattributes Outdated
Lib/test/coding20731.py -text

# Third party code
Modules/zlib/** -text

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Mar 27, 2017

Member

Why the text attribute is unset? Third party code shouldn't be edited, but it can be read as a text.

This comment has been minimized.

Sign in to view
Copy link
@zware

zware Mar 27, 2017

Author Member

To avoid EOL conversion on code that's not ours. There's one file in particular in zlib that caused issues when @doko42 updated our copy of zlib before the GitHub migration; I'm frankly not sure why it's not causing issues currently.

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Mar 27, 2017

Member

This is not the only foreign code. libffi, libmpdec, blake2b, Keccak. The only file with non-LF line ending in zlib/ is zlib.map.
.gitattributes Outdated
Modules/zlib/** -text
Modules/expat/** -text

# CRLF files

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Mar 27, 2017

Member

Shouldn't the text attribute be set if use the eol attribute?

This comment has been minimized.

Sign in to view
Copy link
@AraHaan

AraHaan Mar 27, 2017 •
edited

Contributor

tbh even if I use Windows I still perfer LF over CRLF for endlines. Although I use Notepad++, I would have liked if Windows notepad was to show files with LF line endings just like it would with files with CRLF. However it does not in Windows 7 all the way up to Windows 10 and it annoys me forcing me to use Notepad++ to begin with although with Notepad++ I also get syntax highlighting that Windows Notepad also does not have. Although even then I like LF more because it can safe a little bit more space than CRLF(\r\n) would.

Be nice if there was an Windows update for Windows 7 and newer that would patch Windows Notepad to allow proper and easily read files with LF as if it was in CRLF.

Anyways @zooba mind forwarding my Windows Notepad issues to the proper team to patch it on Windows 7 SP1 and newer for me please?

This comment has been minimized.

Sign in to view
Copy link
@zware

zware Mar 27, 2017

Author Member

@serhiy-storchaka According to the gitattributes docs:

eol
This attribute sets a specific line-ending style to be used in the working directory. It enables end-of-line conversion without any content checks, effectively setting the text attribute.

@AraHaan This is not the place to discuss well-known issues with Notepad. It is also not the place to ask personal favors of anybody.

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Mar 27, 2017

Member

All examples in the gitattributes docs use eol only together with text.
Lib/venv/scripts/nt/activate.bat Outdated
set "PATH=%VIRTUAL_ENV%\__VENV_BIN_NAME__;%PATH%"

:END
echo test

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Mar 27, 2017

Member

Is this the debugging artifact?

This comment has been minimized.

Sign in to view
Copy link
@zware

zware Mar 27, 2017

Author Member

Apparently so, good catch!
.gitattributes

# Text files that should not be subject to eol conversion
Lib/test/cjkencodings/* -text
Lib/test/decimaltestdata/*.decTest -text

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Mar 27, 2017

Member

They are text files.

This comment has been minimized.

Sign in to view
Copy link
@zware

zware Mar 27, 2017

Author Member

As best I can tell, -text in .gitattributes just means "don't change the end-of-line character used in this file regardless of settings like core.autocrlf or core.eol". Editing the file and running git diff still results in a readable diff (preventing that is controlled by -diff, which is included in the binary macro).

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Mar 27, 2017 •
edited

Member

*.decTest files are read in test_decimal.py as text files. No need to prevent changing the end-of-line character.

This comment has been minimized.

Sign in to view
Copy link
@zware

zware Mar 27, 2017

Author Member

But they are imported from elsewhere, and it would be good to keep them the same as they were imported. @skrah may have an opinion on this.

This comment has been minimized.

Sign in to view
Copy link
@skrah

skrah Mar 27, 2017

Member

Most of them are imported from http://speleotrove.com/decimal/dectest.html (dectest.zip).

I agree with Zachary.

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Mar 27, 2017

Member

Should they be converted to CRLF line ending as in decTest.zip?

This comment has been minimized.

Sign in to view
Copy link
@skrah

skrah Mar 27, 2017 •
edited

Member

Which ones? I just opened Lib/test/decimaltestdata/ddInvert.decTest and it's DOS as expected.

This comment has been minimized.

Sign in to view
Copy link
@serhiy-storchaka

serhiy-storchaka Mar 27, 2017

Member

I just opened the first file abs.decTest and it has LF line ending.

This comment has been minimized.

Sign in to view
Copy link
@skrah

skrah Mar 27, 2017

Member

Okay, then it would be reasonable to convert all files that are also in decTest.zip to CRLF.

This comment has been minimized.

Sign in to view
Copy link
@skrah

skrah Mar 27, 2017

Member

Conversion of course not necessarily as part of this issue, unless someone has the patience to do that. ;)
@vstinner vstinner mentioned this pull request Mar 27, 2017
Merged
@vstinner vstinner added the OS-windows label Mar 27, 2017
@vstinner vstinner changed the title bpo-27425: Be more explicit in .gitattributes [windows] bpo-27425: Be more explicit in .gitattributes Mar 27, 2017
@serhiy-storchaka
serhiy-storchaka approved these changes Apr 2, 2017
View changes
Copy link
Member

serhiy-storchaka left a comment

LGTM, but I would prefer to add explicit text if eof= is used (official .gitattributes examples add it). And either remove zlib and expat or add other third party code directories.
zware added 2 commits Mar 13, 2017
@zware
Be more explicit in .gitattributes
d699c78
@zware
Use 'text' tag with 'eol=', don't bother with third-party stuff
74f2885
@zware zware force-pushed the zware:update_gitattributes branch from 7841d57 to c74bc40 Jun 4, 2017
@zware
Commit line ending fixes
Loading status checks…
1e61282
@zware zware force-pushed the zware:update_gitattributes branch from c74bc40 to 1e61282 Jun 4, 2017
@zware zware merged commit 6b6e687 into python:master Jun 10, 2017
3 checks passed
3 checks passed
bedevere/issue-number Issue number 27425 found.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zware zware deleted the zware:update_gitattributes branch Jun 10, 2017
This was referenced Jun 10, 2017
Merged
Merged
Merged
zware added a commit that referenced this pull request Jun 10, 2017
@zware
[3.6] bpo-27425: Be more explicit in .gitattributes (GH-840) (GH-2083)
Loading status checks…
964c261 
Also updates checked-in line endings on some files
zware added a commit that referenced this pull request Jun 10, 2017
@zware
[3.5] bpo-27425: Be more explicit in .gitattributes (GH-840) (GH-2084)
Loading status checks…
afa6a38 
Also updates checked-in line endings on some files.
zware added a commit to zware/cpython that referenced this pull request Jun 11, 2017
@zware
Fix more line endings (pythonGH-840)
60cda64
zware added a commit to zware/cpython that referenced this pull request Jun 11, 2017
@zware
Fix more line endings (pythonGH-840)
c38c591
zware added a commit to zware/cpython that referenced this pull request Jun 11, 2017
@zware
Fix more line endings (pythonGH-840)
e657b76
zware added a commit that referenced this pull request Jun 11, 2017
@zware
Show what's changed when Travis fails regen check (GH-2095)
Loading status checks…
3f8f16d 
Also fixed a few more line endings that were missed in GH-840, which were causing failure.
zware added a commit that referenced this pull request Jun 11, 2017
@zware
[3.6] Use Travis to make sure all generated files are up to date (GH-…
Loading status checks…
f57e34b 
…2080) (GH-2092)

(cherry picked from commit 0afbabe)

Also fixes some line endings missed in GH-840 backport.
zware added a commit that referenced this pull request Jun 11, 2017
@zware
[3.5] Use Travis to make sure all generated files are up to date (GH-…
Loading status checks…
80bdbf6 
…2080) (GH-2093)

(cherry picked from commit 0afbabe)

Also fixes some line endings missed in GH-840 backport.
zware added a commit that referenced this pull request Jun 11, 2017
@zware
[2.7] bpo-27425: Be more explicit in .gitattributes (GH-840) (GH-2086)
Loading status checks…
5fe8ac6 
Also updates checked-in line endings in several files.
mlouielu added a commit to mlouielu/cpython that referenced this pull request Jun 15, 2017
@zware @mlouielu
bpo-27425: Be more explicit in .gitattributes (pythonGH-840)
5d22386 
Updates checked-in line endings on several files.
mlouielu added a commit to mlouielu/cpython that referenced this pull request Jun 15, 2017
@zware @mlouielu
Show what's changed when Travis fails regen check (pythonGH-2095)
9c892b9 
Also fixed a few more line endings that were missed in pythonGH-840, which were causing failure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AraHaan AraHaan

@skrah skrah

@berkerpeksag berkerpeksag

@serhiy-storchaka serhiy-storchaka

Assignees
No one assigned
Labels
CLA signed OS-windows
Projects
None yet
Milestone
No milestone
9 participants
@zware @mention-bot @berkerpeksag @serhiy-storchaka @AraHaan @skrah @vstinner @Mariatta @the-knights-who-say-ni
You can’t perform that action at this time.