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

From: Shawn O. Pearce <spearce@spearce.org>
Date: Dec 4, 2009, 18:37 UTC
Message-ID: <20091204183713.GE1483@spearce.org>
In-Reply-To: <1259942168-24869-7-git-send-email-ilari.liusvaara@elisanet.fi>

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> wrote:

Show 7 quoted lines

>  
> +'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?

Show 8 quoted lines

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

Show 9 quoted lines

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

Show 21 quoted lines

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