Re: [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking

From: Jeff King <peff@peff.net>
Date: Aug 5, 2022, 15:36 UTC
Message-ID: <Yu05GjncDaGRTgce@coredump.intra.peff.net>
In-Reply-To: <41477326-5493-4d3c-246d-8a28969fa73e@web.de>

On Wed, Aug 03, 2022 at 11:56:13PM +0200, René Scharfe wrote:

> Without that line the added test hangs for me on the Git for Windows
> SDK on Windows 11.

Hmph. Interesting that it passes in CI, but not on your local setup. I wonder why pipes would behave differently. Or perhaps there is even some configuration different that means we are still running the perl add--interactive there, though I kind of doubt it (and couldn't find anything pointing there).

Still, if it fails in at least one spot, it's something we need to deal with. On the plus side, once we figure out how to fix it, the hand-grenade of "enable_nonblock() does nothing on Windows" will not be present anymore. ;)

Show 20 quoted lines

> With the patch below it fails and reports basically nothing:
> [...]
>    not ok 57 - handle very large filtered diff
>    #
>    #               git reset --hard &&
>    #               # The specific number here is not important, but it must
>    #               # be large enough that the output of "git diff --color"
>    #               # fills up the pipe buffer. 10,000 results in ~200k of
>    #               # colored output.
>    #               test_seq 10000 >test &&
>    #               test_config interactive.diffFilter cat &&
>    #               printf y >y &&
>    #               force_color git add -p >output 2>&1 <y &&
>    #               git diff-files --exit-code -- test
>    #
>    1..57
> 
> The file "output" contains "error: failed to run 'cat'".  This is
> add-patch.c::parse_diff() reporting that pipe_command() failed.  So
> that's not it, yet.  (I don't actually know what I'm doing here.)

That implies that your call to enable_nonblock() succeeded, or pipe_command() itself would have complained, too. Maybe instrument it like this:

diff --git a/run-command.c b/run-command.c
index 8ea609d4ae..27e79c928a 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1473,11 +1473,17 @@ int pipe_command(struct child_process *cmd,
 	}
 
 	if (pump_io(io, nr) < 0) {
+		error_errno("pumping io failed");
 		finish_command(cmd); /* throw away exit code */
 		return -1;
 	}
 
-	return finish_command(cmd);
+	{
+		int ret = finish_command(cmd);
+		if (ret)
+			error("child returned failure %d", ret);
+		return ret;
+	}
 }
 
 enum child_state {

Normally we stay pretty quiet there and let the caller report any
problems, but it lacks enough context to make a more specific error
report.

>  int enable_nonblock(int fd)
>  {
> +	DWORD mode;
> +	HANDLE handle = winansi_get_osfhandle(fd);
> +	if (!handle)
> +		return -1;
> +	if (!GetNamedPipeHandleState(handle, &mode, NULL, NULL, NULL, NULL, 0))
> +		return -1;
> +	if (mode & PIPE_NOWAIT)
> +		return 0;
> +	mode |= PIPE_NOWAIT;
> +	if (!SetNamedPipeHandleState(handle, &mode, NULL, NULL))
> +		return -1;
>  	return 0;
>  }

This looks plausibly correct to me. ;) We might want to change the name
of the compat layer to enable_pipe_nonblock(), since one assumes from
the function names this only works for pipes.

Thanks for poking at this.

-Peff