Re: [PATCH 2/1] t3920: support CR-eating grep

[Philippe Blain <levraiphilippeblain@gmail.com>]

Hi René,

Le 2022-12-02 à 11:51, René Scharfe a écrit :
> grep(1) converts CRLF line endings to CR on current MinGW:
> 
>    $ uname -sr
>    MINGW64_NT-10.0-22621 3.3.6-341.x86_64

I don't really know the conventions in this regards, but for me 
"MinGW" refers to the port of GCC to Windows. Here it would be more 
correct to refer to "the Git for Windows flavor of MSYS2" or something
like this, in my (maybe uninformed) opinion.

> 
>    $ printf 'a\r\n' | hexdump.exe -C
>    00000000  61 0d 0a                                          |a..|
>    00000003
> 
>    $ printf 'a\r\n' | grep . | hexdump.exe -C
>    00000000  61 0a                                             |a.|
>    00000002
> 
> Create the intended test file by grepping the original file with LF
> line endings and adding CRs explicitly.
> 
> The missing CRs went unnoticed because test_cmp on MinGW ignores line
> endings since 4d715ac05c (Windows: a test_cmp that is agnostic to random
> LF <> CRLF conversions, 2013-10-26). 

Reading that commit's messages, if I understand correctly it's more MSYS2's Bash
that is the culprit, rather than its grep, no?

> Fix this test anyway to avoid
> depending on that special test_cmp behavior, especially since this is
> the only test that needs it.

Do you mean the only test in that file, or in the whole Git test suite (which would
mean after this series we could completely remove the Windows-specific 'test_cmp' ) ?

> 
> Piping the output of grep(1) through append_cr has the side-effect of
> ignoring its return value.  That means we no longer need the explicit
> "|| true" to support commit messages without a body.
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  t/t3920-crlf-messages.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh
> index a58522c163..67fd2345af 100755
> --- a/t/t3920-crlf-messages.sh
> +++ b/t/t3920-crlf-messages.sh
> @@ -12,7 +12,7 @@ create_crlf_ref () {
>  	cat >.crlf-orig-$branch.txt &&
>  	cat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&
>  	grep 'Subject' .crlf-orig-$branch.txt | tr '\n' ' ' | sed 's/[ ]*$//' | tr -d '\n' >.crlf-subject-$branch.txt &&
> -	{ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true; } &&
> +	grep 'Body' .crlf-orig-$branch.txt | append_cr >.crlf-body-$branch.txt &&
>  	LIB_CRLF_BRANCHES="${LIB_CRLF_BRANCHES} ${branch}" &&
>  	test_tick &&
>  	hash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&
> --
> 2.38.1.windows.1
> 

apart from the minor things in the commit message, the 3 patches look good to me :) 
Here is my Acked-by: for all 3 :

Acked-by: Philippe Blain <levraiphilippeblain@gmail.com>

Thanks for detailed messages as always, it definitely went over my radar at the time
that both 'grep' and  'test_cmp' acted differently on Windows.

Cheers,
Philippe.
p.s. I wonder what happened for format-patch to generate these 2/1, 3/1 and 4/1 patch prefixes :P