Re: [PATCH 0/5] Abide by our own rules regarding line endings

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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