.gitattributes: ensure t/oid-info/* has eol=lf

I noticed that our CI builds (see [1] for an example) were returning success much faster than they did before Git v2.20.0. Turns out that there was a test script failure involving the new test hash logic.

error: bug in the test script: bad hash algorithm make[1]: *** [Makefile:56: t0000-basic.sh] Error 1 make[1]: *** Waiting for unfinished jobs....

The reason this passed was because we were running this command in our build:

make GIT_TEST_OPTS=--quiet -j 10 -C t ||
make GIT_TEST_OPTS=\"-i -v -x\" -k -C t failed

The first make failed, but the test script did not record any failed tests and hence the second command succeeded.

The test bug relates to this function added by 2c02b110d "t: add test functions to translate hash-related values":

+# Load key-value pairs from stdin suitable for use with test_oid. Blank lines +# and lines starting with "#" are ignored. Keys must be shell identifier +# characters.
+#
+# Examples:
+# rawsz sha1:20
+# rawsz sha256:32
+test_oid_cache () {
+ local tag rest k v &&
+

+       { test -n "$test_hash_algo" || test_detect_hash; } &&
+       while read tag rest
+       do

+ case $tag in
+ \#*)
+ continue;;
+ ?*)
+ # non-empty
+ ;;
+ *)
+ # blank line
+ continue;;
+ esac &&
+
+ k="${rest%:*}" &&
+ v="${rest#*:}" &&
+
+ if ! expr "$k" : '[a-z0-9][a-z0-9]*$' >/dev/null
+ then
+ error 'bug in the test script: bad hash algorithm'
+ fi &&
+ eval "test_oid_${k}_$tag=\"\$v\""
+ done
+}

The verbose output included these values. Note how '\r' appears in the variable values.

++ test_oid_init
++ test -n ''
++ test_detect_hash
++ test_hash_algo=sha1
++ test_oid_cache
++ local tag rest k v
++ test -n sha1
++ read tag rest
++ case $tag in
++ k=sha1
++ v=$'20\r'
++ expr sha1 : '[a-z0-9][a-z0-9]*$'
++ eval 'test_oid_sha1_rawsz="$v"'
+++ test_oid_sha1_rawsz=$'20\r'
++ read tag rest
++ case $tag in
++ k=sha256
++ v=$'32\r'
++ expr sha256 : '[a-z0-9][a-z0-9]*$'
++ eval 'test_oid_sha256_rawsz="$v"'
+++ test_oid_sha256_rawsz=$'32\r'
++ read tag rest
++ case $tag in
++ k=
++ v=
++ expr '' : '[a-z0-9][a-z0-9]*$'

[1] https://gvfs.visualstudio.com/ci/_build/results?buildId=4815

Derrick Stolee (1):
.gitattributes: ensure t/oid-info/* has eol=lf

 .gitattributes | 1 +
 1 file changed, 1 insertion(+)

base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-98%2Fderrickstolee%2Ftest-oid-fix-windows-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-98/derrickstolee/test-oid-fix-windows-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/98

gitgitgadget

From: Derrick Stolee <[email protected]>

The new test_oid machinery in the test library requires reading some information from t/oid-info/hash-info and t/oid-info/oid. The shell logic that reads from these files is sensitive to CRLF line endings, causing a problem when the test suite is run on a Windows machine that converts LF to CRLF.

Exclude the files in this folder from this conversion.

---
 .gitattributes | 1 +
 1 file changed, 1 insertion(+)

index acf853e029..3738cea7eb 100644
@@ -13,3 +13,4 @@
 /Documentation/gitk.txt conflict-marker-size=32
 /Documentation/user-manual.txt conflict-marker-size=32
 /t/t????-*.sh conflict-marker-size=32
+/t/oid-info/* eol=lf

gitgitgadget

Interesting. "read" does not realize that it is reading from a text file and do whatever appropriate for the platform (i.e. treat CRLF as the end-of-line)?

Sounds good. The base is 2.20; the problematic topic was designed to be mergeable to 2.19.x track, but I do not forsee us merging the bc/hash-independent-tests topic to the maintenance track, so for this particular fix, it should be OK to base the fix there.

"What problem?" is what people will ask when they read this commit message in the future.

Please include the relevant details in the commit message instead of the cover letter.

I noticed that our CI builds (see [1] for an example) were returning success much faster than they did before Git v2.20.0. Turns out that there was a test script failure involving the new test hash logic.

error: bug in the test script: bad hash algorithm make[1]: *** [Makefile:56: t0000-basic.sh] Error 1 make[1]: *** Waiting for unfinished jobs....

This failure was due to an LF -> CRLF conversion in some data files in t/oid-info/. Don't munge these files, and the tests can continue.

UPDATED IN V2: Unfortunately, I didn't check the full test suite after getting beyond this point, and found another LF -> CRLF conversion problem in t4256-am-format-flowed.sh due to example patches in t/t4256/1/. Add these in a second commit. Thanks, dscho, for helping with the correct placement.

Thanks, -Stolee

[1] https://gvfs.visualstudio.com/ci/_build/results?buildId=4815

Derrick Stolee (1):
.gitattributes: ensure t/oid-info/* has eol=lf

Johannes Schindelin (1):
t4256: mark support files as LF-only

 .gitattributes   | 1 +
 t/.gitattributes | 1 +
 2 files changed, 2 insertions(+)

base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-98%2Fderrickstolee%2Ftest-oid-fix-windows-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-98/derrickstolee/test-oid-fix-windows-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/98

Range-diff vs v1:

 1:  4a22502a31 = 1:  4a22502a31 .gitattributes: ensure t/oid-info/* has eol=lf
 -:  ---------- > 2:  4275b8a581 t4256: mark support files as LF-only

gitgitgadget

From: Derrick Stolee <[email protected]>

The new test_oid machinery in the test library requires reading some information from t/oid-info/hash-info and t/oid-info/oid. The shell logic that reads from these files is sensitive to CRLF line endings, causing a problem when the test suite is run on a Windows machine that converts LF to CRLF.

Exclude the files in this folder from this conversion.

---
 .gitattributes | 1 +
 1 file changed, 1 insertion(+)

index acf853e029..3738cea7eb 100644
@@ -13,3 +13,4 @@
 /Documentation/gitk.txt conflict-marker-size=32
 /Documentation/user-manual.txt conflict-marker-size=32
 /t/t????-*.sh conflict-marker-size=32
+/t/oid-info/* eol=lf

gitgitgadget

From: Johannes Schindelin <[email protected]>

The test t4256-am-format-flowed.sh requires carefully applying a patch after ignoring padding whitespace. This breaks if the file is munged to include CRLF line endings instead of LF.

---
 t/.gitattributes | 1 +
 1 file changed, 1 insertion(+)

index e7acedabe1..df05434d32 100644
@@ -16,6 +16,7 @@ t[0-9][0-9][0-9][0-9]/* -whitespace
 /t4135/* eol=lf
 /t4211/* eol=lf
 /t4252/* eol=lf
+/t4256/1/* eol=lf
 /t5100/* eol=lf
 /t5515/* eol=lf
 /t556x_common eol=lf

gitgitgadget

It seems that this step is identical to v1, to which SZEDER Gábor had trouble with (cf. <[email protected]>). I am guessing that the review and v2 e-mails have crossed.

FWIW, I personally do not think "is sensitive to CRLF" is too bad, as my attempt to clarify it does not make it much better, e.g.

The logic to read from these files in shell uses built-in "read" command, which leaves CR at the end of these text files when they are checked out with CRLF line endings, at least when run with bash shipped with Git for Windows. This results in an unexpected value in the variable these lines are read into, leading the tests to fail.

So, I'll keep what I queued when I received v1 for now.

"Johannes Schindelin via GitGitGadget" <[email protected]>

Thanks, will queue.

Hi Gábor,

The test script (not test case) fails with the rather terrifying message

error: bug in the test script: bad hash algorithm

See e.g. line 958 of the Build & Test log in the Windows phase of
https://dev.azure.com/git-for-windows/git/_build/results?buildId=26757

Ciao,
Dscho

Yeah, I saw that in the cover letter. And that was my point, that I saw this there, not in the proposed commit log message:

Hi Gábor,

Oy, oy, oy. I missed that. Where is the coffee.

Ciao,
Dscho

Sorry for missing the edit to the message. You are correct that v2 just added one commit.

I like your rewording, if you feel like editing it.

Thanks,

-Stolee

I'm kinda neutral ;-).

Hi Brian,

This is a tricky thing right there. The Bash we use is borrowed from the MSYS2 project, which tries to stay as close to Unix/Linux as possible, i.e. it does *not* treat Carriage Return as part of the line ending. Changing that default would break tons of things, I would expect.

Right. It is my hope that I get the Azure Pipelines support going soon, so that Pull Requests on GitHub are tested on Windows, too. Hopefully that would help.

Typically, I monitor the Windows builds of pu closely. But in this case, it did not catch because the current Azure Pipeline that runs these tests (triggered via Travis) specifically forces core.autocrlf to false, as that definition pre-dates the work I've done to make Git's source code CR/LF safe.

I do have a build definition to test with core.autocrlf = true, but that only runs on Git for Windows' master, and once git.git has its own Azure Pipeline, I plan on adding another phase to test that directly.

Ciao,
Dscho