git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/1] .gitattributes: ensure t/oid-info/* has eol=lf
@ 2018-12-11 20:35 Derrick Stolee via GitGitGadget
  2018-12-11 20:35 ` [PATCH 1/1] " Derrick Stolee via GitGitGadget
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2018-12-11 20:35 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/1] .gitattributes: ensure t/oid-info/* has eol=lf
  2018-12-11 20:35 [PATCH 0/1] .gitattributes: ensure t/oid-info/* has eol=lf Derrick Stolee via GitGitGadget
@ 2018-12-11 20:35 ` Derrick Stolee via GitGitGadget
  2018-12-12 13:39   ` SZEDER Gábor
  2018-12-12  8:38 ` [PATCH 0/1] " Junio C Hamano
  2018-12-12 18:14 ` [PATCH v2 0/2] Add more eol=lf to .gitattributes Derrick Stolee via GitGitGadget
  2 siblings, 1 reply; 16+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2018-12-11 20:35 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Derrick Stolee

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/1] .gitattributes: ensure t/oid-info/* has eol=lf
  2018-12-11 20:35 [PATCH 0/1] .gitattributes: ensure t/oid-info/* has eol=lf Derrick Stolee via GitGitGadget
  2018-12-11 20:35 ` [PATCH 1/1] " Derrick Stolee via GitGitGadget
@ 2018-12-12  8:38 ` Junio C Hamano
  2018-12-12 18:14 ` [PATCH v2 0/2] Add more eol=lf to .gitattributes Derrick Stolee via GitGitGadget
  2 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2018-12-12  8:38 UTC (permalink / raw)
  To: Derrick Stolee via GitGitGadget; +Cc: git

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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/1] .gitattributes: ensure t/oid-info/* has eol=lf
  2018-12-11 20:35 ` [PATCH 1/1] " Derrick Stolee via GitGitGadget
@ 2018-12-12 13:39   ` SZEDER Gábor
  2018-12-13 13:01     ` Johannes Schindelin
  0 siblings, 1 reply; 16+ messages in thread
From: SZEDER Gábor @ 2018-12-12 13:39 UTC (permalink / raw)
  To: Derrick Stolee via GitGitGadget; +Cc: git, Junio C Hamano, Derrick Stolee

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 0/2] Add more eol=lf to .gitattributes
  2018-12-11 20:35 [PATCH 0/1] .gitattributes: ensure t/oid-info/* has eol=lf Derrick Stolee via GitGitGadget
  2018-12-11 20:35 ` [PATCH 1/1] " Derrick Stolee via GitGitGadget
  2018-12-12  8:38 ` [PATCH 0/1] " Junio C Hamano
@ 2018-12-12 18:14 ` Derrick Stolee via GitGitGadget
  2018-12-12 18:14   ` [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf Derrick Stolee via GitGitGadget
  2018-12-12 18:14   ` [PATCH v2 2/2] t4256: mark support files as LF-only Johannes Schindelin via GitGitGadget
  2 siblings, 2 replies; 16+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2018-12-12 18:14 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf
  2018-12-12 18:14 ` [PATCH v2 0/2] Add more eol=lf to .gitattributes Derrick Stolee via GitGitGadget
@ 2018-12-12 18:14   ` Derrick Stolee via GitGitGadget
  2018-12-13  2:38     ` Junio C Hamano
  2018-12-14  0:51     ` brian m. carlson
  2018-12-12 18:14   ` [PATCH v2 2/2] t4256: mark support files as LF-only Johannes Schindelin via GitGitGadget
  1 sibling, 2 replies; 16+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2018-12-12 18:14 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Derrick Stolee

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 2/2] t4256: mark support files as LF-only
  2018-12-12 18:14 ` [PATCH v2 0/2] Add more eol=lf to .gitattributes Derrick Stolee via GitGitGadget
  2018-12-12 18:14   ` [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf Derrick Stolee via GitGitGadget
@ 2018-12-12 18:14   ` Johannes Schindelin via GitGitGadget
  2018-12-13  2:40     ` Junio C Hamano
  1 sibling, 1 reply; 16+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-12-12 18:14 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf
  2018-12-12 18:14   ` [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf Derrick Stolee via GitGitGadget
@ 2018-12-13  2:38     ` Junio C Hamano
  2018-12-13 16:22       ` Derrick Stolee
  2018-12-14  0:51     ` brian m. carlson
  1 sibling, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2018-12-13  2:38 UTC (permalink / raw)
  To: Derrick Stolee via GitGitGadget; +Cc: git, Derrick Stolee, SZEDER Gábor

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/2] t4256: mark support files as LF-only
  2018-12-12 18:14   ` [PATCH v2 2/2] t4256: mark support files as LF-only Johannes Schindelin via GitGitGadget
@ 2018-12-13  2:40     ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2018-12-13  2:40 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin

"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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/1] .gitattributes: ensure t/oid-info/* has eol=lf
  2018-12-12 13:39   ` SZEDER Gábor
@ 2018-12-13 13:01     ` Johannes Schindelin
  2018-12-13 13:22       ` SZEDER Gábor
  0 siblings, 1 reply; 16+ messages in thread
From: Johannes Schindelin @ 2018-12-13 13:01 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Derrick Stolee via GitGitGadget, git, Junio C Hamano, Derrick Stolee

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/1] .gitattributes: ensure t/oid-info/* has eol=lf
  2018-12-13 13:01     ` Johannes Schindelin
@ 2018-12-13 13:22       ` SZEDER Gábor
  2018-12-13 13:44         ` Johannes Schindelin
  0 siblings, 1 reply; 16+ messages in thread
From: SZEDER Gábor @ 2018-12-13 13:22 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Derrick Stolee via GitGitGadget, git, Junio C Hamano, Derrick Stolee

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/1] .gitattributes: ensure t/oid-info/* has eol=lf
  2018-12-13 13:22       ` SZEDER Gábor
@ 2018-12-13 13:44         ` Johannes Schindelin
  0 siblings, 0 replies; 16+ messages in thread
From: Johannes Schindelin @ 2018-12-13 13:44 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Derrick Stolee via GitGitGadget, git, Junio C Hamano, Derrick Stolee

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf
  2018-12-13  2:38     ` Junio C Hamano
@ 2018-12-13 16:22       ` Derrick Stolee
  2018-12-14  2:23         ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Derrick Stolee @ 2018-12-13 16:22 UTC (permalink / raw)
  To: Junio C Hamano, Derrick Stolee via GitGitGadget
  Cc: git, Derrick Stolee, SZEDER Gábor

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf
  2018-12-12 18:14   ` [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf Derrick Stolee via GitGitGadget
  2018-12-13  2:38     ` Junio C Hamano
@ 2018-12-14  0:51     ` brian m. carlson
  2018-12-14 11:10       ` Johannes Schindelin
  1 sibling, 1 reply; 16+ messages in thread
From: brian m. carlson @ 2018-12-14  0:51 UTC (permalink / raw)
  To: Derrick Stolee via GitGitGadget; +Cc: git, Junio C Hamano, Derrick Stolee

[-- 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 --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf
  2018-12-13 16:22       ` Derrick Stolee
@ 2018-12-14  2:23         ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2018-12-14  2:23 UTC (permalink / raw)
  To: Derrick Stolee
  Cc: Derrick Stolee via GitGitGadget, git, Derrick Stolee, SZEDER Gábor

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf
  2018-12-14  0:51     ` brian m. carlson
@ 2018-12-14 11:10       ` Johannes Schindelin
  0 siblings, 0 replies; 16+ messages in thread
From: Johannes Schindelin @ 2018-12-14 11:10 UTC (permalink / raw)
  To: brian m. carlson
  Cc: Derrick Stolee via GitGitGadget, git, Junio C Hamano, Derrick Stolee

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-12-14 11:11 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-11 20:35 [PATCH 0/1] .gitattributes: ensure t/oid-info/* has eol=lf Derrick Stolee via GitGitGadget
2018-12-11 20:35 ` [PATCH 1/1] " Derrick Stolee via GitGitGadget
2018-12-12 13:39   ` SZEDER Gábor
2018-12-13 13:01     ` Johannes Schindelin
2018-12-13 13:22       ` SZEDER Gábor
2018-12-13 13:44         ` Johannes Schindelin
2018-12-12  8:38 ` [PATCH 0/1] " Junio C Hamano
2018-12-12 18:14 ` [PATCH v2 0/2] Add more eol=lf to .gitattributes Derrick Stolee via GitGitGadget
2018-12-12 18:14   ` [PATCH v2 1/2] .gitattributes: ensure t/oid-info/* has eol=lf Derrick Stolee via GitGitGadget
2018-12-13  2:38     ` Junio C Hamano
2018-12-13 16:22       ` Derrick Stolee
2018-12-14  2:23         ` Junio C Hamano
2018-12-14  0:51     ` brian m. carlson
2018-12-14 11:10       ` Johannes Schindelin
2018-12-12 18:14   ` [PATCH v2 2/2] t4256: mark support files as LF-only Johannes Schindelin via GitGitGadget
2018-12-13  2:40     ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git