Re: [PATCHv2 06/10] pkt-line: share buffer/descriptor reading implementation
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:56:11
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? [...]
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
--- 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