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

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


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:

 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