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