Hi Jonathan, On Tue, 2 May 2017, Jonathan Nieder wrote:
> Johannes Schindelin wrote: > > > Over the past decade, there have been a couple of attempts to remedy the > [...] > > I'm intentionally skimming this cover letter, since anything important > it says needs to also be in the commit messages. Sure, makes sense. I tried to do that, too, before sending out my patch series. > [...] > > Without these fixes, Git will fail to build and pass the test suite, as > > can be verified even on Linux using this cadence: > > > > git config core.autocrlf true > > rm .git/index && git stash > > make DEVELOPER=1 -j15 test > > This should go in a commit message (or perhaps in all of them). Hmm, okay. I wanted to keep it out of them, as commit messages are (in my mind) more about the why?, and a little less about the how? when not obvious from the diff. I added a variation to the first patch (because the tests would still fail, I skipped the `test` from the `make` invocation) and the unmodified cadence to the "Fix the remaining tests..." patch. > [...] > > Johannes Schindelin (5): > > Fix build with core.autocrlf=true > > git-new-workdir: mark script as LF-only > > completion: mark bash script as LF-only > > Fix the remaining tests that failed with core.autocrlf=true > > t4051: mark supporting files as requiring LF-only line endings > > With or without that change, > Reviewed-by: Jonathan Nieder <jrnie...@gmail.com> Thanks. I added that footer to the patches (not to the two new ones, though). > The t/README bit in patch 4/5 is sad (I want to be able to open > t/README using an old version of Notepad) but changing that test to > use another file seems out of scope for what this series does. Yes, it is sad. Maybe I should list the tests that do this (use files outside any tNNNN/ directory): t4003-diff-rename-1.sh (uses t/diff-lib/COPYING) t4005-diff-rename-2.sh (uses t/diff-lib/COPYING) t4007-rename-3.sh (uses t/diff-lib/COPYING) t4008-diff-break-rewrite.sh (uses t/diff-lib/README and t/diff-lib/COPYING) t4009-diff-rename-4.sh (uses t/diff-lib/COPYING) t4022-diff-rewrite.sh (uses COPYING) t4023-diff-rename-typechange.sh (uses COPYING) t7001-mv.sh (uses README.md (!!!) and COPYING) t7060-wtstatus.sh (uses t/README) t7101-reset-empty-subdirs.sh (uses COPYING) Note most of these tests may *use* those files, but do *not* assume that they have Unix line endings! Only a couple test compare SHA-1s to hardcoded values (which, if you ask me, is a bit fragile, given that files outside the tests' control are used). Interesting side note: t0022-crlf-rename.sh copies *itself* to the trash* directory where it is then used to perform tests. So while this test uses "an outside file", that file happens to be a .sh file which we already have to mark LF-only for different reasons (Bash likes its input CR/LF-free). Another interesting side note: the convention appears to dictate that supporting files should be either generated in the test script itself, or be committed into t/tNNNN/ directories (where NNNN matches the respective test script's number, or reuses another test script's supporting files). A notable exception is t3901 which has the supporting files t3901-8859-1.txt and t3901-utf8.txt. I would wageer that this is just a remnant of ancient times before the current convention, judging by the date of the commit that added these files: a731ec5eb82 (t3901: test "format-patch | am" pipe with i18n, 2007-01-13). The scripts t0203-gettext-setlocale-sanity.sh, t9350-fast-export.sh and t9500-gitweb-standalone-no-errors.sh merely reuse those files, and when those scripts started using those files, they did not change their location. I made this move a preparatory step in this patch series. Back to the question what to do about those tests that make improper assumptions about line endings of files outside the tests' realm: I think I can do this more cleverly, as it would appear that only four test scripts make hard assumptions about the exact byte sequence of the COPYING file, and I simply turned those `cp` (and even `cat`!) calls into `tr -d "\015"` calls. I will Cc: you on v2. Ciao, Dscho
