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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 14, 2016, 18:38 UTC
Message-ID: <xmqqk2goqd4l.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160714173920.22001-3-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 22 quoted lines

> +static struct string_list *read_push_options(void)
> +{
> +	struct string_list *ret = xmalloc(sizeof(*ret));
> +	string_list_init(ret, 1);
> +
> +	while (1) {
> +		char *line, *lf;
> +		int len;
> +
> +		line = packet_read_line(0, &len);
> +
> +		if (!line)
> +			break;
> +
> +		/*
> +		* NEEDSWORK: expose the limitations to be configurable;
> +		* Once the limit can be lifted, include a way for payloads
> +		* larger than one pkt, e.g use last byte to indicate if
> +		* the push option continues in the next packet or implement
> +		* larger packets.
> +		*/
> +		if (len > LARGE_PACKET_MAX - 1) {

packet_read_line() calls packet_read_line_generic() that calls packet_read() with the fixed packet_buffer[] that is sized to be LARGE_PACKET_MAX.

Can this check even trigger?

Show 11 quoted lines

> +			/*
> +			 * NEEDSWORK: The error message in die(..) is not
> +			 * transmitted in call cases, so ideally all die(..)
> +			 * calls are prefixed with rp_error and then we can
> +			 * combine rp_error && die into one helper function.
> +			 */
> +			rp_error("protocol error: server configuration allows push "
> +				 "options of size up to %d bytes",
> +				 LARGE_PACKET_MAX - 1);
> +			die("protocol error: push options too large");
> +		}

> +		lf = strchr(line, '\n');
> +		if (lf)
> +			*lf = '\0';

packet_read_line() -> packet_read_line_generic() calls packet_read() with PACKET_READ_CHOMP_NEWLINE flag bit; do we need this check?

Show 6 quoted lines

> +
> +		string_list_append(ret, line);
> +	}
> +
> +	return ret;
> +}

Other than that, looks good to me.

Thanks.