Skip to content

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

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

Merged
merged 3 commits into from
Jun 10, 2017

Conversation

zware
Copy link
Member

@zware 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
Copy link

@zware, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vsajip to be a potential reviewer.

.gitattributes Outdated
Lib/test/coding20731.py -text

# Third party code
Modules/zlib/** -text
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@AraHaan AraHaan Mar 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All examples in the gitattributes docs use eol only together with text.

set "PATH=%VIRTUAL_ENV%\__VENV_BIN_NAME__;%PATH%"

:END
echo test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the debugging artifact?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently so, good catch!


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are text files.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@serhiy-storchaka serhiy-storchaka Mar 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I agree with Zachary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@skrah skrah Mar 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conversion of course not necessarily as part of this issue, unless someone has the patience to do that. ;)

@vstinner vstinner changed the title bpo-27425: Be more explicit in .gitattributes [windows] bpo-27425: Be more explicit in .gitattributes Mar 27, 2017
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 zware force-pushed the update_gitattributes branch from 7841d57 to c74bc40 Compare June 4, 2017 19:42
@zware zware force-pushed the update_gitattributes branch from c74bc40 to 1e61282 Compare June 4, 2017 19:50
@zware zware merged commit 6b6e687 into python:master Jun 10, 2017
@zware zware deleted the update_gitattributes branch June 10, 2017 19:58
zware added a commit that referenced this pull request Jun 10, 2017
Also updates checked-in line endings on some files
zware added a commit that referenced this pull request Jun 10, 2017
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 added a commit to zware/cpython that referenced this pull request Jun 11, 2017
zware added a commit to zware/cpython that referenced this pull request Jun 11, 2017
zware added a commit that referenced this pull request Jun 11, 2017
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
…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
…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
Also updates checked-in line endings in several files.
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.

9 participants