[PATCH 0/1] .gitattributes: ensure t/oid-info/* has eol=lf

[PATCH 0/1] .gitattributes: ensure t/oid-info/* has eol=lf

[Derrick Stolee via GitGitGadget]

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

[PATCH 1/1] .gitattributes: ensure t/oid-info/* has eol=lf

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 .gitattributes | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitattributes b/.gitattributes
index acf853e029..3738cea7eb 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -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

Re: [PATCH 0/1] .gitattributes: ensure t/oid-info/* has eol=lf

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> The verbose output included these values. Note how '\r' appears in the
> variable values.
> ...
> ++ read tag rest

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

> Derrick Stolee (1):
>   .gitattributes: ensure t/oid-info/* has eol=lf
>
>  .gitattributes | 1 +
>  1 file changed, 1 insertion(+)

> base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7

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.

Re: [PATCH 1/1] .gitattributes: ensure t/oid-info/* has eol=lf

[SZEDER Gábor]

On Tue, Dec 11, 2018 at 12:35:46PM -0800, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> 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.

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

> Exclude the files in this folder from this conversion.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  .gitattributes | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/.gitattributes b/.gitattributes
> index acf853e029..3738cea7eb 100644
> --- a/.gitattributes
> +++ b/.gitattributes
> @@ -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

[PATCH v2 0/2] Add more eol=lf to .gitattributes

[Derrick Stolee via GitGitGadget]

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

[PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 .gitattributes | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitattributes b/.gitattributes
index acf853e029..3738cea7eb 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -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

[PATCH v2 2/2] t4256: mark support files as LF-only

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/.gitattributes | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/.gitattributes b/t/.gitattributes
index e7acedabe1..df05434d32 100644
--- a/t/.gitattributes
+++ b/t/.gitattributes
@@ -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

Re: [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---

It seems that this step is identical to v1, to which SZEDER Gábor
had trouble with (cf. <20181212133945.GV30222@szeder.dev>).  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.

Re: [PATCH v2 2/2] t4256: mark support files as LF-only

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---

Thanks, will queue.

>  t/.gitattributes | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/.gitattributes b/t/.gitattributes
> index e7acedabe1..df05434d32 100644
> --- a/t/.gitattributes
> +++ b/t/.gitattributes
> @@ -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

Re: [PATCH 1/1] .gitattributes: ensure t/oid-info/* has eol=lf

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]

Hi Gábor,

On Wed, 12 Dec 2018, SZEDER Gábor wrote:

> On Tue, Dec 11, 2018 at 12:35:46PM -0800, Derrick Stolee via GitGitGadget wrote:
> > From: Derrick Stolee <dstolee@microsoft.com>
> > 
> > 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.
> 
> "What problem?" is what people will ask when they read this commit
> message in the future.

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

> Please include the relevant details in the commit message instead of
> the cover letter.
> 
> > Exclude the files in this folder from this conversion.
> > 
> > Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> > ---
> >  .gitattributes | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/.gitattributes b/.gitattributes
> > index acf853e029..3738cea7eb 100644
> > --- a/.gitattributes
> > +++ b/.gitattributes
> > @@ -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
> 

Re: [PATCH 1/1] .gitattributes: ensure t/oid-info/* has eol=lf

[SZEDER Gábor]

On Thu, Dec 13, 2018 at 02:01:15PM +0100, Johannes Schindelin wrote:
> Hi Gábor,
> 
> On Wed, 12 Dec 2018, SZEDER Gábor wrote:
> 
> > On Tue, Dec 11, 2018 at 12:35:46PM -0800, Derrick Stolee via GitGitGadget wrote:
> > > From: Derrick Stolee <dstolee@microsoft.com>
> > > 
> > > 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.
> > 
> > "What problem?" is what people will ask when they read this commit
> > message in the future.
> 
> 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

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:

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

Re: [PATCH 1/1] .gitattributes: ensure t/oid-info/* has eol=lf

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1365 bytes --]

Hi Gábor,

On Thu, 13 Dec 2018, SZEDER Gábor wrote:

> On Thu, Dec 13, 2018 at 02:01:15PM +0100, Johannes Schindelin wrote:
> > 
> > On Wed, 12 Dec 2018, SZEDER Gábor wrote:
> > 
> > > On Tue, Dec 11, 2018 at 12:35:46PM -0800, Derrick Stolee via GitGitGadget wrote:
> > > > From: Derrick Stolee <dstolee@microsoft.com>
> > > > 
> > > > 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.
> > > 
> > > "What problem?" is what people will ask when they read this commit
> > > message in the future.
> > 
> > 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
> 
> 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:
> 
> > > Please include the relevant details in the commit message instead of
> > > the cover letter.

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

Ciao,
Dscho

Re: [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf

[Derrick Stolee]

On 12/12/2018 9:38 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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.
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
> It seems that this step is identical to v1, to which SZEDER Gábor
> had trouble with (cf. <20181212133945.GV30222@szeder.dev>).  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.

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

Re: [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

On Wed, Dec 12, 2018 at 10:14:53AM -0800, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> 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.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  .gitattributes | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/.gitattributes b/.gitattributes
> index acf853e029..3738cea7eb 100644
> --- a/.gitattributes
> +++ b/.gitattributes
> @@ -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

Yeah, this seems like a sensible thing to do. I assumed the shell on
Windows would read data as text files, not as binary files. It's kinda
hard for me as a non-Windows user to predict what will need CRLF endings
and what will need LF endings with Git on Windows.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

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

I'm kinda neutral ;-).

Re: [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf

[Johannes Schindelin]

Hi Brian,

On Fri, 14 Dec 2018, brian m. carlson wrote:

> On Wed, Dec 12, 2018 at 10:14:53AM -0800, Derrick Stolee via GitGitGadget wrote:
> > 
> > diff --git a/.gitattributes b/.gitattributes
> > index acf853e029..3738cea7eb 100644
> > --- a/.gitattributes
> > +++ b/.gitattributes
> > @@ -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
> 
> Yeah, this seems like a sensible thing to do. I assumed the shell on
> Windows would read data as text files, not as binary files.

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.

> It's kinda hard for me as a non-Windows user to predict what will need
> CRLF endings and what will need LF endings with Git on Windows.

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