Re: [RFC PATCH v2 4/8] Support remote helpers implementing smart transports

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

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:
>  
> +'connect' <service>::
> +	Connects to given service. Stdin and stdout of helper are
> +	connected to specified service (git prefix is included in service
> +	name so e.g. fetching uses 'git-upload-pack' as service) on
> +	remote side. Valid replies to this command are empty line
> +	(connection established), 'FALLBACK' (no smart transport support,

Why not 'fallback' to remain consistent with this protocol and many
others in git where we stick to lowercase ASCII?

> +	fall back to dumb transports) and just exiting with error message
> +	printed (can't connect, don't bother trying to fall back). After
> +	line feed terminating the positive (empty) response, the output
> +	of service starts. After the connection ends, the remote
> +	helper exits. Note that to prevent deadlocking, all read data
> +	should be immediately flushed to outgoing connection (excepting
> +	remote initial advertisments, which should be flushed on first
> +	flush packet (0000 as length) encountered.

Why is the initial advertisement special?  If the helper always
flushes both sides, it shouldn't ever deadlock the protocol.  Also,
note that a helper should be able to implement a tiny delay like
Nagle's algorithm does in TCP.  It just can't sit on a byte forever.

> @@ -72,7 +73,10 @@ static struct child_process *get_helper(struct transport *transport)
>  	helper->argv = xcalloc(4, sizeof(*helper->argv));
>  	strbuf_addf(&buf, "remote-%s", data->name);
>  	helper->argv[0] = strbuf_detach(&buf, NULL);
> -	helper->argv[1] = transport->remote->name;
> +	if(transport->remote)
> +		helper->argv[1] = transport->remote->name;
> +	else
> +		helper->argv[1] = "";

This hunk appears to be unrelated.  And actually, if transport has
no remote, shouldn't the arg here be NULL so the helper gets only
1 argument and not 2 arguments?

> +static int _process_connect(struct transport *transport,
> +				      const char *name, const char *exec)
> +{
> +	struct helper_data *data = transport->data;
> +	struct strbuf cmdbuf = STRBUF_INIT;
> +	struct child_process *helper;
> +	int r;
> +
> +	helper = get_helper(transport);
> +
> +	/* Handle --upload-pack and friends. This is fire and forget...
> +	   just warn if it fails. */
> +	if(exec && strcmp(name, exec)) {
> +		r = set_helper_option(transport, "servpath", exec);
> +		if(r > 0)
> +			fprintf(stderr, "Warning: Setting remote service path "
> +				"not supported by protocol.\n");
> +		else if(r < 0)
> +			fprintf(stderr, "Warning: Invalid remote service "
> +				"path.\n");
> +	}

I think exec winds up defaulting to name if --upload-pack was not
used on the command line, and remote.$name.uploadpack was not set.
See transport.c where you initialize the git options struct, these
fields were defaulted in.

My point is, we shouldn't send option servpath to the helper if
name is equal to servpath, because the helper might not support
servpath and the option command will issue a warning above for no
reason at all.

-- 
Shawn.