Re: t0027 racy?

From: Jeff King <peff@peff.net>
Date: Aug 12, 2016, 07:24 UTC
Message-ID: <20160812072417.huwa4e2tvtcew67k@sigill.intra.peff.net>
In-Reply-To: <20160811185812.GA18434@tb-raspi>

On Thu, Aug 11, 2016 at 06:58:12PM +0000, Torsten Bögershausen wrote:

Show 12 quoted lines

> commit 3754404d3d1ea4a0cbbed4986cc4ac1b5fe6b66e
> Author: Torsten Bögershausen <tboegi@web.de>
> Date:   Thu Aug 11 18:47:29 2016 +0200
> 
>     t0027: Correct raciness in NNO test
>     
>     When a non-reversible CRLF conversion is done in "git add",
>     a warning is printed on stderr.
>     
>     The commit_chk_wrnNNO() function  in t0027 was written to test this,
>     but did the wrong thing: Instead of looking at the warning
>     from "git add", it looked at the warning from "git commit".

Maybe add:

  This is racy because "git commit" may not have to do CRLF conversion
  at all if it can use the sha1 value from the index (which depends on
  whether "add" and "commit" run in a single second).

>     Thanks to Jeff King <peff@peff.net> for analizing t0027.
>     Reporyed-By: Johannes Schindelin <Johannes.Schindelin@gmx.de>

s/analizing/analyzing/; s/Reporyed/Reported/

Show 12 quoted lines

> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> index 2860d2d..6e44382 100755
> --- a/t/t0027-auto-crlf.sh
> +++ b/t/t0027-auto-crlf.sh
> @@ -119,8 +119,7 @@ commit_chk_wrnNNO () {
>  		fname=${pfx}_$f.txt &&
>  		cp $f $fname &&
>  		printf Z >>"$fname" &&
> -		git -c core.autocrlf=$crlf add $fname 2>/dev/null &&
> -		git -c core.autocrlf=$crlf commit -m "commit_$fname" $fname >"${pfx}_$f.err" 2>&1
> +		git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
>  	done

So this is the meat of the fix.

Show 13 quoted lines

> @@ -394,11 +393,10 @@ test_expect_success 'commit files attr=crlf' '
>  
>  #                 attr                    LF        CRLF      CRLFmixLF   LF_mix_CR   CRLFNUL
>  commit_chk_wrnNNO ""      ""      false   ""        ""        ""          ""          ""
> -commit_chk_wrnNNO ""      ""      true    LF_CRLF   ""        ""          ""          ""
> +commit_chk_wrnNNO ""      ""      true    ""        ""        ""          ""          ""
>  commit_chk_wrnNNO ""      ""      input   ""        ""        ""          ""          ""
> -
> -commit_chk_wrnNNO "auto"  ""      false   "$WILC"   ""        ""          ""          ""
> -commit_chk_wrnNNO "auto"  ""      true    LF_CRLF   ""        ""          ""          ""
> +commit_chk_wrnNNO "auto"  ""      false   ""        ""        ""          ""          ""
> +commit_chk_wrnNNO "auto"  ""      true    ""        ""        ""          ""          ""
>  commit_chk_wrnNNO "auto"  ""      input   ""        ""        ""          ""          ""

I am not sure I understand why this reordering is necessary, but I guess it's to group like things together in a single commit? Might be worth a mention in the commit message.

-Peff