Re: [RFC PATCH v3 5/8] Support taking over transports

["Shawn O. Pearce" <spearce@spearce.org>]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
> @@ -109,9 +120,21 @@ static struct child_process *get_helper(struct transport *transport)
>  		die("Unable to run helper: git %s", helper->argv[0]);
>  	data->helper = helper;
>  
> +	/* Open the output as FILE* so strbuf_getline() can be used.
> +	   Do this with duped fd because fclose() will close the fd,
> +	   and stuff like disowning will require the fd to remain.
> +
> +	   Set the stream to unbuffered because some reads are critical
> +	   in sense that any overreading will cause deadlocks.
> +	*/
> +        duped = dup(helper->out);

Formatting error here, the line is indented wrong.

> +	if (duped < 0)
> +		die_errno("Can't dup helper output fd");
> +	data->out = xfdopen(duped, "r");
> +	setvbuf(data->out, NULL, _IONBF, 0);

I wonder if this is really a good idea.  Most helpers actually
use a lot of text based IO to communicate.  Disabling buffering
for those helpers to avoid overreading the advertisement from a
connect is a problem.

Maybe we could leave buffering on, but use a handshake protocol
with the helper during connect:

  (1) > "connect git-upload-pack\n"
  (2) < "\n"
  (3) > "begin\n"

During 2 we are still buffered, but the only content on the pipe
should be the single blank line, so we pull that in and the FILE*
buffer should be empty.

After writing message 2 the remote helper blocks reading for the
"begin\n" message 3.  This gives the transport-helper.c code time
to switch off the FILE* and start using raw IO off the pipe before
any data starts coming down the line.

It does mean that the helper may need to run unbuffered IO, but
if the helper is only a smart helper supporting connect then this
isn't really a problem.  It can run buffered IO until connect is
received, switch to unbuffered, and use unbuffered for the single
"begin\n" message it has to consume before it starts writing output
or reading input.

-- 
Shawn.