Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> While documenting
> this, fix a nit in the `receive.advertiseAtomic` wording.
>  
>  receive.advertiseAtomic::
>  	By default, git-receive-pack will advertise the atomic push
> -	capability to its clients. If you don't want to this capability
> +	capability to its clients. If you don't want this capability
> +	to be advertised, set this variable to false.
> +
> +receive.advertisePushOptions::
> +	By default, git-receive-pack will advertise the push options capability
> +	to its clients. If you don't want this capability
>  	to be advertised, set this variable to false.

I think we correcting the nit by avoiding passive voice, i.e.

	If you don't want to advertise this capability, set this
	variable to false.

would make it easier to read.

>  in packet-line format to the client, followed by a flush-pkt.  The only
>  real difference is that the capability listing is different - the only
> -possible values are 'report-status', 'delete-refs' and 'ofs-delta'.
> +possible values are 'report-status', 'delete-refs', 'ofs-delta' and
> +'push-options'.

OK.

> +push-options
> +------------
> +
> +If the server sends the 'push-options' capability it is capable to accept

Two nits:

 - A comma would make it easier to read.
 - "capable" goes with "of <gerund>", while "able" goes with "to <infinitive>".

i.e. "... capability, it is capable of accepting..."

> +push options after the update commands have been sent. If the pushing client
> +requests this capability, the server will pass the options to the pre and post
> +receive hooks that process this push request.

Missing dashes, i.e. "pre- and post-receive hooks"?

> @@ -207,6 +214,8 @@ static void show_ref(const char *path, const unsigned char *sha1)
>  			      "report-status delete-refs side-band-64k quiet");
>  		if (advertise_atomic_push)
>  			strbuf_addstr(&cap, " atomic");
> +		if (advertise_push_options)
> +			strbuf_addstr(&cap, " push-options");
>  		if (prefer_ofs_delta)
>  			strbuf_addstr(&cap, " ofs-delta");
>  		if (push_cert_nonce)

Hmph, was there a good reason to add it in the middle (contrast to
the previous addition to the "only possible values are..."
enumeration)?

> +static struct string_list *read_push_options()

static struct string_list *read_push_options(void)

> +{
> +	int i;
> +	struct string_list *ret = xmalloc(sizeof(*ret));
> +	string_list_init(ret, 1);
> +
> +	/* NEEDSWORK: expose the limitations to be configurable. */
> +	int max_options = 32;
> +
> +	/*
> +	 * NEEDSWORK: expose the limitations to be configurable;
> +	 * Once the limit can be lifted, include a way for payloads
> +	 * larger than one pkt, e.g allow a payload of up to
> +	 * LARGE_PACKET_MAX - 1 only, and reserve the last byte
> +	 * to indicate whether the next pkt continues with this
> +	 * push option.
> +	 */
> +	int max_size = 1024;

Good NEEDSWORK comments; perhaps also hint that the configuration
must not come from the repository level configuration file (i.e.
Peff's "scoped configuration" from jk/upload-pack-hook topic)?

> +	for (i = 0; i < max_options; i++) {
> +		char *line;
> +		int len;
> +
> +		line = packet_read_line(0, &len);
> +
> +		if (!line)
> +			break;
> +
> +		if (len > max_size)
> +			die("protocol error: server configuration allows push "
> +			    "options of size up to %d bytes", max_size);
> +
> +		len = strcspn(line, "\n");
> +		line[len] = '\0';
> +
> +		string_list_append(ret, line);
> +	}
> +	if (i == max_options)
> +		die("protocol error: server configuration only allows up "
> +		    "to %d push options", max_options);

When not going over ssh://, does the user sees these messages?

More importantly, if we plan to make this configurable and not make
the limit a hardwired constant of the wire protocol, it may be
better to advertise push-options capability with the limit, e.g.
"push-options=32" (or even "push-options=1024/32"), so that the
client side can count and abort early?

I wondered how well the extra flush works with the extra framing
smart-http does to wrap the wire protocol; as I do not see any
change to the http side, I'd assume that there is no issue.

> +
> +	return ret;
> +}
> +
>  static const char *parse_pack_header(struct pack_header *hdr)
>  {
>  	switch (read_pack_header(0, hdr)) {
> @@ -1773,6 +1829,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
>  		const char *unpack_status = NULL;
>  		struct string_list *push_options = NULL;
>  
> +		if (use_push_options)
> +			push_options = read_push_options();
> +
>  		prepare_shallow_info(&si, &shallow);
>  		if (!si.nr_ours && !si.nr_theirs)
>  			shallow_update = 0;