Re: [PATCHv2 06/10] pkt-line: share buffer/descriptor reading implementation

[Jonathan Nieder <jrnieder@gmail.com>]

Jeff King wrote:

> The packet_read function reads from a descriptor.

Ah, so this introduces a new analagous helper that reads from
a strbuf, to avoid the copy-from-async-procedure hack?

[...]
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -103,12 +103,26 @@ static int safe_read(int fd, void *buffer, unsigned size, int gently)
>  	strbuf_add(buf, buffer, n);
>  }
>  
> -static int safe_read(int fd, void *buffer, unsigned size, int gently)
> +static int get_packet_data(int fd, char **src_buf, size_t *src_size,
> +			   void *dst, unsigned size, int gently)
>  {
> -	ssize_t ret = read_in_full(fd, buffer, size);
> -	if (ret < 0)
> -		die_errno("read error");
> -	else if (ret < size) {
> +	ssize_t ret;
> +
> +	/* Read up to "size" bytes from our source, whatever it is. */
> +	if (src_buf) {
> +		ret = size < *src_size ? size : *src_size;
> +		memcpy(dst, *src_buf, ret);
> +		*src_buf += size;
> +		*src_size -= size;
> +	}
> +	else {

Style: git cuddles its "else"s.

	assert(src_buf ? fd < 0 : fd >= 0);

	if (src_buf) {
		...
	} else {
		...
	}

> +		ret = read_in_full(fd, dst, size);
> +		if (ret < 0)
> +			die_errno("read error");

This is noisy about upstream pipe gone missing, which makes sense
since this is transport-related.  Maybe that deserves a comment.

[...]
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -138,28 +138,29 @@ static struct discovery* discover_refs(const char *service)
>  	if (maybe_smart &&
>  	    (5 <= last->len && last->buf[4] == '#') &&
>  	    !strbuf_cmp(&exp, &type)) {
> +		char line[1000];
> +		int len;
> +
>  		/*
>  		 * smart HTTP response; validate that the service
>  		 * pkt-line matches our request.
>  		 */
> -		if (packet_get_line(&buffer, &last->buf, &last->len) <= 0)
> -			die("%s has invalid packet header", refs_url);
> -		if (buffer.len && buffer.buf[buffer.len - 1] == '\n')
> -			strbuf_setlen(&buffer, buffer.len - 1);
> +		len = packet_read_from_buf(line, sizeof(line), &last->buf, &last->len);
> +		if (len && line[len - 1] == '\n')
> +			len--;

Was anything guaranteeing that buffer.len < 1000 before this change?

The rest looks good from a quick glance.

Jonathan