Re: t0027 racy?
- From
Torsten Bögershausen <tboegi@web.de>
- Date
- Aug 9, 2016, 11:33 UTC
- Message-ID
- <20160809113337.GA1143@tb-raspi>
- In-Reply-To
- <20160809065110.GB17777@peff.net>
On Tue, Aug 09, 2016 at 02:51:10AM -0400, Jeff King wrote:
Show 54 quoted lines
> On Mon, Aug 08, 2016 at 08:32:24PM +0000, Torsten Bögershausen wrote:
>
> > > The verbose output is not very exciting, though:
> > >
> > > expecting success:
> > > check_warning "$lfwarn" ${pfx}_LF.err
> > >
> > > --- NNO_attr_auto_aeol_crlf_false_LF.err.expect 2016-08-08 15:26:37.061701392 +0000
> > > +++ NNO_attr_auto_aeol_crlf_false_LF.err.actual 2016-08-08 15:26:37.061701392 +0000
> > > @@ -1 +0,0 @@
> > > -warning: LF will be replaced by CRLF
> > > not ok 114 - commit NNO files crlf=false attr=auto LF
> > [...]
> > The warning is missing, but should be there:
> >
> > The file has LF, and after commit and a new checkout these LF will
> > be convertet into CRLF.
> >
> > So why isn't the warning there (but here on my oldish machines)
>
> To be clear, the warning _is_ there when I just run t0027 by itself, and
> the test passes. It's only under heavy load that it isn't. So it's a
> race condition either in the test script or in git itself.
>
> Usually race conditions like these are due to one of:
>
> - git dying from SIGPIPE before it has a chance to output the command.
> But I don't see any pipes being used in the test script.
>
> - index raciness causing us to avoid reading file content. For
> example, if you do:
>
> echo foo >bar
> git add bar
>
> Then _usually_ "bar" and the index will have the same mtime. And
> therefore subsequent commands that need to refresh the index will
> re-read the content of "bar", because they cannot tell from the stat
> information if we have the latest version of "bar" in the index or
> not (it could have been written after the index update, but in the
> same second).
>
> But on a slow or heavily loaded system (or if you simply get unlucky
> in crossing the boundary to a new second), they'll have different
> mtimes. And therefore git knows it can skip reading the content from
> the filesystem.
>
> So if your test relies on git actually re-converting the file
> content, it would sometimes randomly fail.
>
> The second one seems plausible, given the history of issues with
> changing CRLF settings for an existing checkout. I'm not sure if it
> would be feasible to reset the index completely before each tested
> command, but that would probably solve it.The content of the file has been changed (we appended the letter 'Z' to it, so even if mtime is the same, st.st_size should differ. And it seems as if the commit is triggered, see below.
>
(I got the stress script working; no I can reproduce it nicely)
Is it possible to vote for a 3rd option ? I added some more warnings, to have always some output in stderr.
This is the good case, this test case passed: cat NNO_attr_auto_aeol_crlf_true_LF.err
warning: check_safe_crlf in NNO_attr_auto_aeol_crlf_true_LF.txt 2 warning: LF was here in NNO_attr_auto_aeol_crlf_true_LF.txt.. warning: LF will be replaced by CRLF in NNO_attr_auto_aeol_crlf_true_LF.txt. The file will have its original line endings in your working directory. warning: check_safe_crlf in NNO_attr__aeol_crlf_true_CRLF.txt 0 warning: check_safe_crlf in NNO_attr__aeol_crlf_true_CRLF_mix_LF.txt 0 warning: check_safe_crlf in NNO_attr__aeol_crlf_true_LF.txt 0 warning: check_safe_crlf in NNO_attr__aeol_lf_true_CRLF.txt 0 warning: check_safe_crlf in NNO_attr__aeol_lf_true_CRLF_mix_LF.txt 0 warning: check_safe_crlf in NNO_attr__aeol_lf_true_LF.txt 0 warning: check_safe_crlf in NNO_attr_auto_aeol_lf_true_LF.txt 0 [master 2decee0] commit_NNO_attr_auto_aeol_crlf_true_LF.txt Author: A U Thor <author@example.com> 1 file changed, 2 insertions(+), 2 deletions(-) --------------------------- And this one failed, the same code base, but a different file: cat commit_NNO_attr_auto_aeol_crlf_input_LF.
[master ce2045a] commit_NNO_attr_auto_aeol_crlf_input_LF.txt Author: A U Thor <author@example.com> 1 file changed, 2 insertions(+), 2 deletions(-)
--------------------------- Both had been generated with a patched convert.c: git diff convert.c index 67d69b5..ae64a58 100644
--- a/convert.c +++ b/convert.c @@ -191,6 +191,7 @@ static enum eol output_eol(enum crlf_action crlf_action) static void check_safe_crlf(const char *path, enum crlf_action crlf_action, struct text_stat *stats, enum safe_crlf checksafe) { + warning("check_safe_crlf in %s %d", path, (int)checksafe); if (!checksafe) return; @@ -210,6 +211,7 @@ static void check_safe_crlf(const char *path, enum crlf_action crlf_action, * CRLFs would be added by checkout: * check if we have "naked" LFs */ + warning("LF was here in %s..", path); if (stats->lonelf) { if (checksafe == SAFE_CRLF_WARN) warning("LF will be replaced by CRLF in %s.\nThe --------------------- In the failing case, there is no warning at all printed, even if I would have expected the list of files to be printed. The commit has been done, I think. See t0027, around line 124: 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 Both stdout and stderr are piped into the .err file. The content of stdout seems to be there always. The content of stderr seems to be there most of the time. When the test case fails, the content of stderr seems to be completely missing. Reverting f4c3edc0b156362a92bf9de4f0ec794e90a757f does not fix the problem Reverting f4c3edc0b15 and 3b331e9267146961466 does not fix the problem.