git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* cat-file timing window on Cygwin
@ 2017-08-25 11:25 Adam Dinwoodie
  2017-08-25 15:08 ` Jeff King
  0 siblings, 1 reply; 11+ messages in thread
From: Adam Dinwoodie @ 2017-08-25 11:25 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin

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.)

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: cat-file timing window on Cygwin
  2017-08-25 11:25 cat-file timing window on Cygwin Adam Dinwoodie
@ 2017-08-25 15:08 ` Jeff King
  2017-08-26  0:57   ` Ramsay Jones
  0 siblings, 1 reply; 11+ messages in thread
From: Jeff King @ 2017-08-25 15:08 UTC (permalink / raw)
  To: Adam Dinwoodie; +Cc: git, Johannes Schindelin

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: cat-file timing window on Cygwin
  2017-08-25 15:08 ` Jeff King
@ 2017-08-26  0:57   ` Ramsay Jones
  2017-08-26 15:43     ` Adam Dinwoodie
  2017-08-26 18:53     ` Jeff King
  0 siblings, 2 replies; 11+ messages in thread
From: Ramsay Jones @ 2017-08-26  0:57 UTC (permalink / raw)
  To: Jeff King, Adam Dinwoodie; +Cc: git, Johannes Schindelin



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@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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: cat-file timing window on Cygwin
  2017-08-26  0:57   ` Ramsay Jones
@ 2017-08-26 15:43     ` Adam Dinwoodie
  2017-08-26 18:53     ` Jeff King
  1 sibling, 0 replies; 11+ messages in thread
From: Adam Dinwoodie @ 2017-08-26 15:43 UTC (permalink / raw)
  To: Ramsay Jones, Jeff King; +Cc: git, Johannes Schindelin

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@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 suite and there are no other new breakages, too.

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: cat-file timing window on Cygwin
  2017-08-26  0:57   ` Ramsay Jones
  2017-08-26 15:43     ` Adam Dinwoodie
@ 2017-08-26 18:53     ` Jeff King
  2017-08-26 21:11       ` Adam Dinwoodie
  1 sibling, 1 reply; 11+ messages in thread
From: Jeff King @ 2017-08-26 18:53 UTC (permalink / raw)
  To: Ramsay Jones; +Cc: Adam Dinwoodie, git, Johannes Schindelin

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 <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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: cat-file timing window on Cygwin
  2017-08-26 18:53     ` Jeff King
@ 2017-08-26 21:11       ` Adam Dinwoodie
  2017-08-27  2:06         ` Ramsay Jones
  0 siblings, 1 reply; 11+ messages in thread
From: Adam Dinwoodie @ 2017-08-26 21:11 UTC (permalink / raw)
  To: Jeff King; +Cc: Ramsay Jones, git, Johannes Schindelin

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 <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!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: cat-file timing window on Cygwin
  2017-08-26 21:11       ` Adam Dinwoodie
@ 2017-08-27  2:06         ` Ramsay Jones
  2017-08-27 11:33           ` Adam Dinwoodie
  0 siblings, 1 reply; 11+ messages in thread
From: Ramsay Jones @ 2017-08-27  2:06 UTC (permalink / raw)
  To: Adam Dinwoodie, Jeff King; +Cc: git, Johannes Schindelin



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 <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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: cat-file timing window on Cygwin
  2017-08-27  2:06         ` Ramsay Jones
@ 2017-08-27 11:33           ` Adam Dinwoodie
  2017-08-27 15:47             ` Ramsay Jones
  0 siblings, 1 reply; 11+ messages in thread
From: Adam Dinwoodie @ 2017-08-27 11:33 UTC (permalink / raw)
  To: Ramsay Jones; +Cc: Jeff King, git, Johannes Schindelin

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 <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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: cat-file timing window on Cygwin
  2017-08-27 11:33           ` Adam Dinwoodie
@ 2017-08-27 15:47             ` Ramsay Jones
  2017-08-27 18:53               ` Jason Pyeron
  2017-09-08 20:02               ` Ramsay Jones
  0 siblings, 2 replies; 11+ messages in thread
From: Ramsay Jones @ 2017-08-27 15:47 UTC (permalink / raw)
  To: Adam Dinwoodie; +Cc: Jeff King, git, Johannes Schindelin



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 <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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: cat-file timing window on Cygwin
  2017-08-27 15:47             ` Ramsay Jones
@ 2017-08-27 18:53               ` Jason Pyeron
  2017-09-08 20:02               ` Ramsay Jones
  1 sibling, 0 replies; 11+ messages in thread
From: Jason Pyeron @ 2017-08-27 18:53 UTC (permalink / raw)
  To: 'Ramsay Jones', 'Adam Dinwoodie'
  Cc: 'Jeff King', git, 'Johannes Schindelin'

> -----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 <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


> >>>> 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 x333            Baltimore, Maryland 21218   -
-                                                               -
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Re: cat-file timing window on Cygwin
  2017-08-27 15:47             ` Ramsay Jones
  2017-08-27 18:53               ` Jason Pyeron
@ 2017-09-08 20:02               ` Ramsay Jones
  1 sibling, 0 replies; 11+ messages in thread
From: Ramsay Jones @ 2017-09-08 20:02 UTC (permalink / raw)
  To: Adam Dinwoodie; +Cc: Jeff King, git, Johannes Schindelin



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


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-09-08 20:02 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-25 11:25 cat-file timing window on Cygwin Adam Dinwoodie
2017-08-25 15:08 ` Jeff King
2017-08-26  0:57   ` Ramsay Jones
2017-08-26 15:43     ` Adam Dinwoodie
2017-08-26 18:53     ` Jeff King
2017-08-26 21:11       ` Adam Dinwoodie
2017-08-27  2:06         ` Ramsay Jones
2017-08-27 11:33           ` Adam Dinwoodie
2017-08-27 15:47             ` Ramsay Jones
2017-08-27 18:53               ` Jason Pyeron
2017-09-08 20:02               ` Ramsay Jones

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).