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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to