You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by krichter722 <gi...@git.apache.org> on 2018/03/18 23:25:41 UTC
[GitHub] storm pull request #2595: set '* text=auto' in .gitattributes in order to av...
GitHub user krichter722 opened a pull request:
https://github.com/apache/storm/pull/2595
set '* text=auto' in .gitattributes in order to avoid merge work because of line feed changes
Unfortunately, `git` doesn't initialize repositories by default with this `.gitattributes` and this value, but sooner or later merge work or discussions will arise from the fact that contributions don't match with the common line endings (currently both LF and a few CRLF are used). It makes sense to convert them once to the same (LF here) and enforce all contributions to have the same EOL from then on. This happened here.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/krichter722/storm gitattributes
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2595.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2595
----
commit d6201b6184473a93f8a40342ca9e6901af0f65a4
Author: Karl-Philipp Richter <kr...@...>
Date: 2018-03-18T23:14:20Z
set '* text=auto' in .gitattributes in order to avoid merge work because of line feed changes
----
---
[GitHub] storm issue #2595: set '* text=auto' in .gitattributes in order to avoid mer...
Posted by krichter722 <gi...@git.apache.org>.
Github user krichter722 commented on the issue:
https://github.com/apache/storm/pull/2595
[`*.png binary` seems to cause tests to fail because of unavailable test resources](https://travis-ci.org/krichter722/storm/builds/356601827?utm_source=email&utm_medium=notification). Omit it and trust the git 2.10 fixes?
---
[GitHub] storm issue #2595: set '* text=auto' in .gitattributes in order to avoid mer...
Posted by krichter722 <gi...@git.apache.org>.
Github user krichter722 commented on the issue:
https://github.com/apache/storm/pull/2595
> Some other changes were made to these files. Could you fix up eol again?
Done.
> Regarding the text=auto for all paths, I'm mentioning it because googling text=auto brings up instances where git guessed wrong and broke some people's png files. I think it would be easier to just exclude file types we obviously don't want to fix eol for now, rather than someone having to debug it later.
In such cases it's always save to assume that sharing your research helps others. I found an [issue mentioning incorrect detection of files unrelated to their type which is fixed in git 2.10](https://stackoverflow.com/questions/29435156/what-will-text-auto-eol-lf-in-gitattributes-do), but I added `*.png binary` and an explanation anyway - why take the risk?
---
[GitHub] storm issue #2595: set '* text=auto' in .gitattributes in order to avoid mer...
Posted by krichter722 <gi...@git.apache.org>.
Github user krichter722 commented on the issue:
https://github.com/apache/storm/pull/2595
Forget that. The last `git rm --cached -r . && git add .` deleted (!) the binary files which the test tries to load. I didn't pay attention, my bad.
---
[GitHub] storm issue #2595: set '* text=auto' in .gitattributes in order to avoid mer...
Posted by krichter722 <gi...@git.apache.org>.
Github user krichter722 commented on the issue:
https://github.com/apache/storm/pull/2595
I don't seem to understand how to get the GitHub diff view to make the first change outdated instead of the second after an amended commit. Please keep that in mind when reading my replies. I don't think it's due to a EOL-conversion issue ;)
---
[GitHub] storm pull request #2595: set '* text=auto' in .gitattributes in order to av...
Posted by krichter722 <gi...@git.apache.org>.
Github user krichter722 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2595#discussion_r175916989
--- Diff: .gitattributes ---
@@ -1,2 +1,2 @@
# Some storm-webapp logviewer tests require input files to have LF line endings due to byte counting.
-storm-webapp/src/test/resources/*.log.test text eol=lf
--- End diff --
Agreed. I thought that this would be covered, but it falls under ntars anyway.
---
[GitHub] storm pull request #2595: set '* text=auto' in .gitattributes in order to av...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2595#discussion_r175893945
--- Diff: .gitattributes ---
@@ -1,2 +1,2 @@
# Some storm-webapp logviewer tests require input files to have LF line endings due to byte counting.
-storm-webapp/src/test/resources/*.log.test text eol=lf
\ No newline at end of file
+* text=auto
--- End diff --
I agree that it makes sense to force LF on check in, but we should maybe ensure some files are excluded, e.g. png and svgs and other image files. We have a couple in the docs.
---
[GitHub] storm issue #2595: set '* text=auto' in .gitattributes in order to avoid mer...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2595
@krichter722 Thanks, merged to master. I think we don't need to track this in a jira issue since it's not a code change.
---
[GitHub] storm issue #2595: set '* text=auto' in .gitattributes in order to avoid mer...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2595
@krichter722 Great, thanks for looking into it. This looks good, +1
---
[GitHub] storm pull request #2595: set '* text=auto' in .gitattributes in order to av...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2595
---
[GitHub] storm issue #2595: set '* text=auto' in .gitattributes in order to avoid mer...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2595
@krichter722 Some other changes were made to these files. Could you fix up eol again? The change to .gitattributes LGTM
---
[GitHub] storm pull request #2595: set '* text=auto' in .gitattributes in order to av...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2595#discussion_r176485883
--- Diff: .gitattributes ---
@@ -1,2 +1,9 @@
# Some storm-webapp logviewer tests require input files to have LF line endings due to byte counting.
-storm-webapp/src/test/resources/*.log.test text eol=lf
\ No newline at end of file
+storm-webapp/src/test/resources/*.log.test text eol=lf
+
+# There're reports of EOL conversion messing up PNG files, but that might have been a bug in git 2.10 only (see https://github.com/git/git/blob/master/Documentation/RelNotes/2.10.0.txt#L248 for details)
+*.png binary
--- End diff --
https://github.com/apache/storm/blob/master/storm-server/src/test/java/org/apache/storm/localizer/AsyncLocalizerTest.java#L493
---
[GitHub] storm issue #2595: set '* text=auto' in .gitattributes in order to avoid mer...
Posted by krichter722 <gi...@git.apache.org>.
Github user krichter722 commented on the issue:
https://github.com/apache/storm/pull/2595
[Adding `*.[extension] binary` seems to cause the tests to fail regardless of the order](https://travis-ci.org/krichter722/storm/jobs/357184158). The question is why. Maybe asking a question on stackoverflow.com makes sense. However, I don't know exactly what the tests are trying to do.
---
[GitHub] storm pull request #2595: set '* text=auto' in .gitattributes in order to av...
Posted by krichter722 <gi...@git.apache.org>.
Github user krichter722 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2595#discussion_r175918249
--- Diff: .gitattributes ---
@@ -1,2 +1,2 @@
# Some storm-webapp logviewer tests require input files to have LF line endings due to byte counting.
-storm-webapp/src/test/resources/*.log.test text eol=lf
\ No newline at end of file
+* text=auto
--- End diff --
`text` automatically recognizes text formats (according to https://git-scm.com/docs/gitattributes) and I doubt that `git` handles this badly. PNG is binary and SVG is text-based, so afaik PNG would be excluded automatically and SVG's EOL whould be converted to LF on check-in which makes sense. The conversion should be done once manually with `git rm -r --cached && git add .`.
---
[GitHub] storm pull request #2595: set '* text=auto' in .gitattributes in order to av...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2595#discussion_r175892425
--- Diff: .gitattributes ---
@@ -1,2 +1,2 @@
# Some storm-webapp logviewer tests require input files to have LF line endings due to byte counting.
-storm-webapp/src/test/resources/*.log.test text eol=lf
--- End diff --
Going by the git documentation I'm pretty sure this line should still be here. We want to force LF on checkout for these files, even if the user is on Windows and checking out with `core.autocrlf=true`. `text=auto` seems to only force LF on check in.
---
[GitHub] storm pull request #2595: set '* text=auto' in .gitattributes in order to av...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2595#discussion_r176485833
--- Diff: .gitattributes ---
@@ -1,2 +1,9 @@
# Some storm-webapp logviewer tests require input files to have LF line endings due to byte counting.
-storm-webapp/src/test/resources/*.log.test text eol=lf
\ No newline at end of file
+storm-webapp/src/test/resources/*.log.test text eol=lf
+
+# There're reports of EOL conversion messing up PNG files, but that might have been a bug in git 2.10 only (see https://github.com/git/git/blob/master/Documentation/RelNotes/2.10.0.txt#L248 for details)
+*.png binary
--- End diff --
also a .tgz file in there
---
[GitHub] storm pull request #2595: set '* text=auto' in .gitattributes in order to av...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2595#discussion_r176485726
--- Diff: .gitattributes ---
@@ -1,2 +1,9 @@
# Some storm-webapp logviewer tests require input files to have LF line endings due to byte counting.
-storm-webapp/src/test/resources/*.log.test text eol=lf
\ No newline at end of file
+storm-webapp/src/test/resources/*.log.test text eol=lf
+
+# There're reports of EOL conversion messing up PNG files, but that might have been a bug in git 2.10 only (see https://github.com/git/git/blob/master/Documentation/RelNotes/2.10.0.txt#L248 for details)
+*.png binary
--- End diff --
Could you try reversing the order of this and * text=auto? I'm pretty sure the latest command wins.
It looks like the failing tests are trying to load tarballs and zip files. Does adding *.tar.gz binary and *.zip binary fix it?
---