Re: Re: cat-file timing window on Cygwin
On 27/08/17 16:47, Ramsay Jones wrote: > On 27/08/17 12:33, Adam Dinwoodie wrote: [snip] >> 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. 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
RE: cat-file timing window on Cygwin
> -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:
> > On Sun, Aug 27, 2017 at 03:06:31AM +0100, Ramsay Jones wrote:
> >> On 26/08/17 22:11, Adam Dinwoodie wrote:
> >>> On Sat, Aug 26, 2017 at 11:53:37AM -0700, Jeff King wrote:
> >>>> 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
> >>>>
> >>>> 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
> >>>> 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?)
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 x333Baltimore, Maryland 21218 -
- -
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Re: cat-file timing window on Cygwin
On 27/08/17 12:33, Adam Dinwoodie wrote:
> On Sun, Aug 27, 2017 at 03:06:31AM +0100, Ramsay Jones wrote:
>> On 26/08/17 22:11, Adam Dinwoodie wrote:
>>> On Sat, Aug 26, 2017 at 11:53:37AM -0700, Jeff King wrote:
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
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
Re: cat-file timing window on Cygwin
On Sun, Aug 27, 2017 at 03:06:31AM +0100, Ramsay Jones wrote:
> On 26/08/17 22:11, Adam Dinwoodie wrote:
> > On Sat, Aug 26, 2017 at 11:53:37AM -0700, Jeff King wrote:
> >> 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
> >>
> >> 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.
Re: cat-file timing window on Cygwin
On 26/08/17 22:11, Adam Dinwoodie wrote:
> On Sat, Aug 26, 2017 at 11:53:37AM -0700, Jeff King wrote:
>> On Sat, Aug 26, 2017 at 01:57:18AM +0100, Ramsay Jones wrote:
>>
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?!)
>>
>> 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
>>
>> 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
Re: cat-file timing window on Cygwin
On Sat, Aug 26, 2017 at 11:53:37AM -0700, Jeff King wrote:
> On Sat, Aug 26, 2017 at 01:57:18AM +0100, Ramsay Jones wrote:
>
> > > 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?!)
>
> 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
>
> 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!
Re: cat-file timing window on Cygwin
On Sat, Aug 26, 2017 at 01:57:18AM +0100, Ramsay Jones wrote:
> > 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?!)
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
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
Re: cat-file timing window on Cygwin
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-6565b7137...@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.
> > 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
Re: cat-file timing window on Cygwin
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-6565b7137...@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 ...
> 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).
ATB,
Ramsay Jones
Re: cat-file timing window on Cygwin
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?
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
{
-Peff
cat-file timing window on Cygwin
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:
diff --git a/t/t8010-cat-file-filters.sh b/t/t8010-cat-file-filters.sh
index d8242e467..3aa1385ad 100755
--- a/t/t8010-cat-file-filters.sh
+++ b/t/t8010-cat-file-filters.sh
@@ -54,7 +54,7 @@
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 |
+ { printf "%s hello.txt\n" $sha1 && sleep 1 && printf "%s hello\n"
$sha1; } |
git cat-file --textconv --batch >actual &&
printf "%s blob 6\nuryyb\r\n\n%s blob 6\nhello\n\n" \
$sha1 $sha1 >expect &&
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.)
