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

2013-02-18 Thread Jonathan Nieder
Jeff King wrote:

> But is it noisy about a missing pipe? We do not get EPIPE for reading.

Ah, that makes more sense.

[...]
>>> +   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?
>
> No. That's discussed in point (3) of the "implications" in the commit
> message.

Thanks.  Sorry I missed it the first time.
--
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


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

2013-02-18 Thread Jeff King
On Mon, Feb 18, 2013 at 02:43:50AM -0800, Jonathan Nieder wrote:

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

Not from a strbuf, but basically, yes.

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

That is not new code; it is just reindented from the original safe_read.
But is it noisy about a missing pipe? We do not get EPIPE for reading.
We should just get a short read or EOF, both of which is handled later.

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

No. That's discussed in point (3) of the "implications" in the commit
message.

-Peff
--
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


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

2013-02-18 Thread Jonathan Nieder
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