On Sat, Aug 26, 2017 at 01:57:18AM +0100, Ramsay Jones wrote:
> On 25/08/17 16:08, Jeff King wrote:
> > On Fri, Aug 25, 2017 at 12:25:29PM +0100, Adam Dinwoodie wrote:
> >
> >> As of v2.10.0-rc1-4-g321459439 ("cat-file: support --textconv/--filters
> >> in batch mode"), t8010-cat-file-filters.sh has been failing on Cygwin.
> >> Digging into this, the test looks to expose a timing window: it appears
> >> that if `git cat-file --textconv --batch` receives input on stdin too
> >> quickly, it fails to parse some of that input.
>
> This has not been failing since the above commit (when this
> feature was first introduced), but (for me) when testing the
> v2.13.0-rc0 build. Please refer to my original email:
>
> https://public-inbox.org/git/bf7db655-d90f-e711-afc8-6565b71373d2@xxxxxxxxxxxxxxxxxxxx/
>
> ... where I was reasonably sure this was caused by a change in
> the cygwin dll while upgrading from 2.7.0-1 -> 2.8.0-1.
The failure started at some point when I wasn't paying much attention to
the state of Cygwin or Git due to personal health stuff. Now I'm
catching up with the backlog from that period, I bisected the Git builds
to get to the commit referenced above, but it's entirely plausible the
behaviour change is in the Cygwin DLL and is merely exposed by the test
that commit added.
> > Hmm. That commit doesn't seem to actually touch the stdin loop. That
> > happens via strbuf_getline(), which should be pretty robust to timing.
> > But here are a few random thoughts:
> >
> > 1. Do you build with HAVE_GETDELIM? Does toggling it have any effect?
>
> If Adam builds using the configure script, then yes, else no. ;-)
> I never use configure, so I don't have HAVE_GETDELIM set.
I do use the configure script and do run with HAVE_GETDELIM set. As you
might expect given that Ramsay doesn't and they see the same behaviour,
toggling it makes no difference to whether t8010.8 passes or fails.
> > 2. Does the problem happen only with --textconv?
> >
> > If so, I wonder if spawning the child process is somehow draining
> > stdin. I don't see how the child would accidentally read from our
> > stdin; we replace its stdin with a pipe so it shouldn't get a
> > chance to see our descriptor at all.
>
> As I said in my previous email, "the loop in batch_objects()
> (builtin/cat-file.c lines #489-505) only reads one line from
> stdin, then gets EOF on the stream."
Testing now, removing --textconv does seem to make a difference. With
the following diff, t8010 passes:
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
index d8242e467..7c85bef9b 100755
--- a/t/t8010-cat-file-filters.sh
+++ b/t/t8010-cat-file-filters.sh
@@ -54,9 +54,9 @@
test_expect_success 'cat-file --textconv --batch works' '
sha1=$(git rev-parse -q --verify HEAD:world.txt) &&
test_config diff.txt.textconv "tr A-Za-z N-ZA-Mn-za-m <" &&
- printf "%s hello.txt\n%s hello\n" $sha1 $sha1 |
- git cat-file --textconv --batch >actual &&
- printf "%s blob 6\nuryyb\r\n\n%s blob 6\nhello\n\n" \
+ printf "%s\n%s\n" $sha1 $sha1 |
+ git cat-file --batch >actual &&
+ printf "%s blob 6\nhello\n\n%s blob 6\nhello\n\n" \
$sha1 $sha1 >expect &&
test_cmp expect actual
'
(I am mildly baffled by the necessary change in line endings in the
`expect` file, but that's the output that was being produced, and I
actually think the single CRLF line ending in the file that otherwise
uses LF in the original test is the oddity here.)
A similar test using my live version of Git also works as expected:
$ { echo $sha1 ; echo $sha1 ; } | git cat-file --batch
ce013625030ba8dba906f756967f9e9ca394464a blob 6
hello
ce013625030ba8dba906f756967f9e9ca394464a blob 6
hello
> > If we accidentally called fflush(stdin) that would discard buffered
> > input and give the results you're seeing. We don't do that
> > directly, of course, but we do call fflush(NULL) in run-command
> > before spawning the child. That _should_ just flush output handles,
> > but it's possible that there's a cygwin bug, I guess.
>
> I suspect so, see previous email ...
>
> > I can't reproduce here on Linux. But does the patch below have any
> > impact?
> >
> > diff --git a/run-command.c b/run-command.c
> > index 98621faca8..064ebd1995 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -641,7 +641,6 @@ int start_command(struct child_process *cmd)
> > }
> >
> > trace_argv_printf(cmd->argv, "trace: run_command:");
> > - fflush(NULL);
> >
> > #ifndef GIT_WINDOWS_NATIVE
> > {
>
> I suspect not, but I can give it a try ...
>
> ... oh, wow, that works! Ahem. (Hmm, so it's flushing stdin?!)
>
> Also, t5526-fetch-submodules.sh still works as well (see commit
> 13af8cbd6a "start_command: flush buffers in the WIN32 code path
> as well", 04-02-2011).
Likewise, this gets t8010 passing for me. I've run through the entire
test suite and there are no other new breakages, too.
