From: Adam Dinwoodie <hidden> Date: 2017-08-25 11:25:41
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.
Compare the following output, run from the t8010 trash directory after a
failed test run, where adding a `sleep` between the two lines of input
changes the output:
$ { echo $sha1 hello.txt ; echo $sha1 hello; } | git -c diff.txt.textconv='tr A-Za-z N-ZA-Mn-za-m <' cat-file --textconv --batch
ce013625030ba8dba906f756967f9e9ca394464a blob 6
uryyb
$ { echo $sha1 hello.txt ; sleep 1; echo $sha1 hello; } | git -c diff.txt.textconv='tr A-Za-z N-ZA-Mn-za-m <' cat-file --textconv --batch
ce013625030ba8dba906f756967f9e9ca394464a blob 6
uryyb
ce013625030ba8dba906f756967f9e9ca394464a blob 6
hello
Similarly, I can get t8010 to pass with the following patch:
I don't think blindly applying the patch is the solution here, however.
The correct solution is presumably to work out what is causing cat-file
to discard some of its input and to get it to stop doing that.
(For reference, to get the output above the test was run on the current
master branch, specifically v2.14.1-326-g3dc57ebfb, while the local
installed Git version was v2.14.0, although this behaviour seems to be
consistent since the originating commit.)
From: Jeff King <hidden> Date: 2017-08-25 15:08:28
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.
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?
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.
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 can't reproduce here on Linux. But does the patch below have any
impact?
From: Ramsay Jones <hidden> Date: 2017-08-26 01:00:28
On 25/08/17 16:08, Jeff King wrote:
On Fri, Aug 25, 2017 at 12:25:29PM +0100, Adam Dinwoodie wrote:
quoted
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@ramsayjones.plus.com/
... 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.
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.
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."
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 ...
quoted hunk
I can't reproduce here on Linux. But does the patch below have any
impact?
@@ -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).
ATB,
Ramsay Jones
From: Adam Dinwoodie <hidden> Date: 2017-08-26 15:43:15
On Sat, Aug 26, 2017 at 01:57:18AM +0100, Ramsay Jones wrote:
On 25/08/17 16:08, Jeff King wrote:
quoted
On Fri, Aug 25, 2017 at 12:25:29PM +0100, Adam Dinwoodie wrote:
quoted
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@ramsayjones.plus.com/
... 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.
quoted
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.
quoted
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:
(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
quoted
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 ...
quoted
I can't reproduce here on Linux. But does the patch below have any
impact?
@@ -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.
@@ -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?!)
Interesting. I find it a little hard to believe there's so obvious a bug
as "fflush(NULL) flushes stdin", but well...that's what it seems like.
If that's truly what it is, this is the minimal reproduction I came up
with:
-- >8 --
#include <stdio.h>
int main(void)
{
char buf[256];
while (fgets(buf, sizeof(buf), stdin)) {
fprintf(stdout, "got: %s", buf);
fflush(NULL);
}
return 0;
}
-- 8< --
If this really is the bug, then doing something like "seq 10 | ./a.out"
would drop some of the input lines.
-Peff
@@ -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?!)
Interesting. I find it a little hard to believe there's so obvious a bug
as "fflush(NULL) flushes stdin", but well...that's what it seems like.
If that's truly what it is, this is the minimal reproduction I came up
with:
-- >8 --
#include <stdio.h>
int main(void)
{
char buf[256];
while (fgets(buf, sizeof(buf), stdin)) {
fprintf(stdout, "got: %s", buf);
fflush(NULL);
}
return 0;
}
-- 8< --
If this really is the bug, then doing something like "seq 10 | ./a.out"
would drop some of the input lines.
...yep. It does. Specifically, I consistently only get the firsts
line:
$ seq 10 | ./a.exe
got: 1
$
If I introduce a delay between the lines of stdin (which I tested by
just typing stdin from the keyboard), it works as expected.
Looks like this one will need to go to the Cygwin mailing list; I'll
take it there shortly. Thank you all for your help getting this far!
@@ -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?!)
Interesting. I find it a little hard to believe there's so obvious a bug
as "fflush(NULL) flushes stdin", but well...that's what it seems like.
If that's truly what it is, this is the minimal reproduction I came up
with:
-- >8 --
#include <stdio.h>
int main(void)
{
char buf[256];
while (fgets(buf, sizeof(buf), stdin)) {
fprintf(stdout, "got: %s", buf);
fflush(NULL);
}
return 0;
}
-- 8< --
If this really is the bug, then doing something like "seq 10 | ./a.out"
would drop some of the input lines.
...yep. It does. Specifically, I consistently only get the firsts
line:
$ seq 10 | ./a.exe
got: 1
$
If I introduce a delay between the lines of stdin (which I tested by
just typing stdin from the keyboard), it works as expected.
Looks like this one will need to go to the Cygwin mailing list; I'll
take it there shortly. Thank you all for your help getting this far!
This is apparently fixed in cygwin v2.8.3 [see commit 78ade082fe,
('Revert "errno: Stop using _impure_ptr->_errno completely"', 19-07-2017),
commit 9cc89b0438 ("cygwin: Use errno instead of _impure_ptr->_errno", 19-07-2017), commit a674199fc9 ("cygwin: Bump DLL version to 2.8.3",
19-07-2017) and commit d2ae2f00b8 ("cygwin: add fflush fix to release
notes", 19-07-2017)].
I haven't had a chance to try v2.8.3 yet (it's 3am and I'm about to
go get some sleep).
ATB,
Ramsay Jones
From: Adam Dinwoodie <hidden> Date: 2017-08-27 11:34:08
On Sun, Aug 27, 2017 at 03:06:31AM +0100, Ramsay Jones wrote:
On 26/08/17 22:11, Adam Dinwoodie wrote:
quoted
On Sat, Aug 26, 2017 at 11:53:37AM -0700, Jeff King wrote:
quoted
Interesting. I find it a little hard to believe there's so obvious a bug
as "fflush(NULL) flushes stdin", but well...that's what it seems like.
If that's truly what it is, this is the minimal reproduction I came up
with:
-- >8 --
#include <stdio.h>
int main(void)
{
char buf[256];
while (fgets(buf, sizeof(buf), stdin)) {
fprintf(stdout, "got: %s", buf);
fflush(NULL);
}
return 0;
}
-- 8< --
If this really is the bug, then doing something like "seq 10 | ./a.out"
would drop some of the input lines.
...yep. It does. Specifically, I consistently only get the firsts
line:
$ seq 10 | ./a.exe
got: 1
$
If I introduce a delay between the lines of stdin (which I tested by
just typing stdin from the keyboard), it works as expected.
Looks like this one will need to go to the Cygwin mailing list; I'll
take it there shortly. Thank you all for your help getting this far!
This is apparently fixed in cygwin v2.8.3 [see commit 78ade082fe,
('Revert "errno: Stop using _impure_ptr->_errno completely"', 19-07-2017),
commit 9cc89b0438 ("cygwin: Use errno instead of _impure_ptr->_errno", 19-07-2017), commit a674199fc9 ("cygwin: Bump DLL version to 2.8.3",
19-07-2017) and commit d2ae2f00b8 ("cygwin: add fflush fix to release
notes", 19-07-2017)].
I haven't had a chance to try v2.8.3 yet (it's 3am and I'm about to
go get some sleep).
Cygwin 2.8.3 hasn't been released yet, but I've just tested the latest
development snapshot with Jeff's simple test case, and it works as
expected, so I'm going to assume the Git test will start passing once
that version of the Cygwin DLL is released too.
From: Ramsay Jones <hidden> Date: 2017-08-27 15:47:53
On 27/08/17 12:33, Adam Dinwoodie wrote:
On Sun, Aug 27, 2017 at 03:06:31AM +0100, Ramsay Jones wrote:
quoted
On 26/08/17 22:11, Adam Dinwoodie wrote:
quoted
On Sat, Aug 26, 2017 at 11:53:37AM -0700, Jeff King wrote:
quoted
Interesting. I find it a little hard to believe there's so obvious a bug
as "fflush(NULL) flushes stdin", but well...that's what it seems like.
If that's truly what it is, this is the minimal reproduction I came up
with:
-- >8 --
#include <stdio.h>
int main(void)
{
char buf[256];
while (fgets(buf, sizeof(buf), stdin)) {
fprintf(stdout, "got: %s", buf);
fflush(NULL);
}
return 0;
}
-- 8< --
If this really is the bug, then doing something like "seq 10 | ./a.out"
would drop some of the input lines.
...yep. It does. Specifically, I consistently only get the firsts
line:
$ seq 10 | ./a.exe
got: 1
$
If I introduce a delay between the lines of stdin (which I tested by
just typing stdin from the keyboard), it works as expected.
Looks like this one will need to go to the Cygwin mailing list; I'll
take it there shortly. Thank you all for your help getting this far!
This is apparently fixed in cygwin v2.8.3 [see commit 78ade082fe,
('Revert "errno: Stop using _impure_ptr->_errno completely"', 19-07-2017),
commit 9cc89b0438 ("cygwin: Use errno instead of _impure_ptr->_errno", 19-07-2017), commit a674199fc9 ("cygwin: Bump DLL version to 2.8.3",
19-07-2017) and commit d2ae2f00b8 ("cygwin: add fflush fix to release
notes", 19-07-2017)].
I haven't had a chance to try v2.8.3 yet (it's 3am and I'm about to
go get some sleep).
Cygwin 2.8.3 hasn't been released yet,
Heh, yes, I found that out myself this afternoon. ;-)
but I've just tested the latest
development snapshot with Jeff's simple test case, and it works as
expected, so I'm going to assume the Git test will start passing once
that version of the Cygwin DLL is released too.
Hmm, I'm not keen on installing "snapshot"(s), so I think I will
wait for the release to test it. (However, as a matter of interest,
how would I obtain/install/test this snapshot release - is it a
'low-risk' exercise?)
ATB,
Ramsay Jones
From: Jason Pyeron <hidden> Date: 2017-08-27 18:53:44
-----Original Message-----
From: Ramsay Jones
Sent: Sunday, August 27, 2017 11:48 AM
To: Adam Dinwoodie
Cc: Jeff King; git@vger.kernel.org; Johannes Schindelin
Subject: Re: cat-file timing window on Cygwin
On 27/08/17 12:33, Adam Dinwoodie wrote:
quoted
On Sun, Aug 27, 2017 at 03:06:31AM +0100, Ramsay Jones wrote:
quoted
On 26/08/17 22:11, Adam Dinwoodie wrote:
quoted
On Sat, Aug 26, 2017 at 11:53:37AM -0700, Jeff King wrote:
quoted
Interesting. I find it a little hard to believe there's
so obvious
quoted
quoted
quoted
quoted
a bug as "fflush(NULL) flushes stdin", but well...that's
what it seems like.
quoted
quoted
quoted
quoted
If that's truly what it is, this is the minimal
reproduction I came
quoted
quoted
quoted
quoted
up
with:
-- >8 --
#include <stdio.h>
int main(void)
{
char buf[256];
while (fgets(buf, sizeof(buf), stdin)) {
fprintf(stdout, "got: %s", buf);
fflush(NULL);
}
return 0;
}
-- 8< --
If this really is the bug, then doing something like
"seq 10 | ./a.out"
Tests good on latest snapshot. Fails on
Cygwin DLL version info:
DLL version: 2.8.2
DLL epoch: 19
DLL old termios: 5
DLL malloc env: 28
Cygwin conv: 181
API major: 0
API minor: 313
Shared data: 5
DLL identifier: cygwin1
Mount registry: 3
Cygwin registry name: Cygwin
Installations name: Installations
Cygdrive default prefix:
Build date:
Shared id: cygwin1S5
quoted
quoted
quoted
quoted
would drop some of the input lines.
...yep. It does. Specifically, I consistently only get
the firsts
quoted
quoted
quoted
line:
$ seq 10 | ./a.exe
got: 1
$
If I introduce a delay between the lines of stdin (which
I tested by
quoted
quoted
quoted
just typing stdin from the keyboard), it works as expected.
Looks like this one will need to go to the Cygwin mailing
list; I'll
quoted
quoted
quoted
take it there shortly. Thank you all for your help
getting this far!
quoted
quoted
This is apparently fixed in cygwin v2.8.3 [see commit 78ade082fe,
('Revert "errno: Stop using _impure_ptr->_errno completely"',
19-07-2017), commit 9cc89b0438 ("cygwin: Use errno instead of
_impure_ptr->_errno", 19-07-2017), commit a674199fc9
("cygwin: Bump
quoted
quoted
DLL version to 2.8.3",
19-07-2017) and commit d2ae2f00b8 ("cygwin: add fflush fix
to release
quoted
quoted
notes", 19-07-2017)].
I haven't had a chance to try v2.8.3 yet (it's 3am and I'm
about to
quoted
quoted
go get some sleep).
Cygwin 2.8.3 hasn't been released yet,
Heh, yes, I found that out myself this afternoon. ;-)
quoted
but I've just tested the
latest development snapshot with Jeff's simple test case,
and it works
quoted
as expected, so I'm going to assume the Git test will start passing
once that version of the Cygwin DLL is released too.
Hmm, I'm not keen on installing "snapshot"(s), so I think I
will wait for the release to test it. (However, as a matter
of interest, how would I obtain/install/test this snapshot
release - is it a 'low-risk' exercise?)
Using https://cygwin.com/snapshots/x86_64/cygwin-20170823.tar.xz
D:\inst\cygwin\cygwin-20170823>usr\bin\bash.exe
bash-4.4$ seq 10 | ./a.exe
got: 1
got: 2
got: 3
got: 4
got: 5
got: 6
got: 7
got: 8
got: 9
got: 10
bash-4.4$ cat cygcheck.out
Cygwin Configuration Diagnostics
Current System Time: Sun Aug 27 14:35:25 2017
Windows 10 Professional Ver 10.0 Build 14393
Cygwin DLL version info:
DLL version: 2.9.0
DLL epoch: 19
DLL old termios: 5
DLL malloc env: 28
Cygwin conv: 181
API major: 0
API minor: 317
Shared data: 5
DLL identifier: cygwin1
Mount registry: 3
Cygwin registry name: Cygwin
Installations name: Installations
Cygdrive default prefix:
Build date:
Snapshot date: 20170823-15:44:28
Shared id: cygwin1S5
bash-4.4$
v/r,
Jason Pyeron
--
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
- -
- Jason Pyeron PD Inc. http://www.pdinc.us -
- Principal Consultant 10 West 24th Street #100 -
- +1 (443) 269-1555 x333 Baltimore, Maryland 21218 -
- -
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
From: Ramsay Jones <hidden> Date: 2017-09-08 20:02:55
On 27/08/17 16:47, Ramsay Jones wrote:
On 27/08/17 12:33, Adam Dinwoodie wrote:
[snip]
quoted
Cygwin 2.8.3 hasn't been released yet,
Heh, yes, I found that out myself this afternoon. ;-)
quoted
but I've just tested the latest
development snapshot with Jeff's simple test case, and it works as
expected, so I'm going to assume the Git test will start passing once
that version of the Cygwin DLL is released too.
I noticed that cygwin 2.9.0 has been released, so I updated and
ran test t8010-cat-file-filters.sh which, as expected, now passes.
Since the above failure was caused, in part, by an errant errno
value, I decided to try reverting the fix for t0301-credential-cache.sh.
(ie. commit 1f180e5eb9, "credential-cache: interpret an ECONNRESET
as an EOF", 27-07-2017). However, that re-introduces the failure! ;-)
[I haven't done a complete test-suite run yet, but I don't expect
to have any failures - famous last words!]
ATB,
Ramsay Jones