{"thread":{"id":"39469","subject":"[PATCH] strbuf_read: skip unnecessary strbuf_grow at eof","startedAt":"2015-05-31T18:16:45Z","lastAt":"2015-06-01T16:23:47Z","messageCount":3,"participants":["Jim Hill","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"262525","messageId":"1433096205-14516-1-git-send-email-gjthill@gmail.com","threadId":"39469","inReplyTo":null,"subject":"[PATCH] strbuf_read: skip unnecessary strbuf_grow at eof","fromName":"Jim Hill","fromEmail":"gjthill@gmail.com","sentAt":"2015-05-31T18:16:45Z","receivedAt":"2015-05-31T18:16:45Z","isPatch":true,"sender":{"key":"gjthill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/80352?v=4"},"body":"Make strbuf_read not try to do read_in_full's job too.  If xread returns\nless than was requested it can be either eof or an interrupted read.  If\nread_in_full returns less than was requested, it's eof. Use read_in_full\nto detect eof and not iterate when eof has been seen.\n\nSigned-off-by: Jim Hill <gjthill@gmail.com>\n---\n strbuf.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 88cafd4..c70733e 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -364,19 +364,19 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \n \tstrbuf_grow(sb, hint ? hint : 8192);\n \tfor (;;) {\n-\t\tssize_t cnt;\n+\t\tssize_t want = sb->alloc - sb->len - 1;\n+\t\tssize_t got = read_in_full(fd, sb->buf + sb->len, want);\n \n-\t\tcnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n-\t\tif (cnt < 0) {\n+\t\tif (got < 0) {\n \t\t\tif (oldalloc == 0)\n \t\t\t\tstrbuf_release(sb);\n \t\t\telse\n \t\t\t\tstrbuf_setlen(sb, oldlen);\n \t\t\treturn -1;\n \t\t}\n-\t\tif (!cnt)\n+\t\tsb->len += got;\n+\t\tif (got < want)\n \t\t\tbreak;\n-\t\tsb->len += cnt;\n \t\tstrbuf_grow(sb, 8192);\n \t}\n \n-- \n2.4.1.4.gfc728c2\n"},{"id":"262580","messageId":"20150601105901.GE31792@peff.net","threadId":"39469","inReplyTo":"1433096205-14516-1-git-send-email-gjthill@gmail.com","subject":"Re: [PATCH] strbuf_read: skip unnecessary strbuf_grow at eof","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-01T10:59:01Z","receivedAt":"2015-06-01T10:59:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 31, 2015 at 11:16:45AM -0700, Jim Hill wrote:\n\n> Make strbuf_read not try to do read_in_full's job too.  If xread returns\n> less than was requested it can be either eof or an interrupted read.  If\n> read_in_full returns less than was requested, it's eof. Use read_in_full\n> to detect eof and not iterate when eof has been seen.\n\nI think this makes sense. I somehow had to read this over several times\nto understand that the main point is not the cleanup, but rather the\nspace savings from not doing an extra strbuf_grow. Perhaps it is because\nthe main idea is mentioned only in the subject. Or perhaps I was just\nbeing dense.\n\n-Peff\n"},{"id":"262615","messageId":"xmqq382b8kj0.fsf@gitster.dls.corp.google.com","threadId":"39469","inReplyTo":"20150601105901.GE31792@peff.net","subject":"Re: [PATCH] strbuf_read: skip unnecessary strbuf_grow at eof","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-01T16:23:47Z","receivedAt":"2015-06-01T16:23:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sun, May 31, 2015 at 11:16:45AM -0700, Jim Hill wrote:\n>\n>> Make strbuf_read not try to do read_in_full's job too.  If xread returns\n>> less than was requested it can be either eof or an interrupted read.  If\n>> read_in_full returns less than was requested, it's eof. Use read_in_full\n>> to detect eof and not iterate when eof has been seen.\n>\n> I think this makes sense. I somehow had to read this over several times\n> to understand that the main point is not the cleanup, but rather the\n> space savings from not doing an extra strbuf_grow. Perhaps it is because\n> the main idea is mentioned only in the subject. Or perhaps I was just\n> being dense.\n\nEven after seeing (not \"reading\", as it was before my first cup of\ncaffeine ;-) this message 30 minutes ago and then reading the patch\nwith a fresh eye, I had the same impression, until I realized there\nis \"too\" at the end of the first sentence.\n\nPerhaps\n\n\tThe loop in strbuf_read() uses xread() repeatedly while\n\textending the strbuf until the call returns zero.  If the\n\tbuffer is sufficiently large to begin with, this results in\n\txread() returning the remainder of the file to the end\n\t(returning non-zero), the loop extending the strbuf, and\n\tthen making another call to xread() to have it return zero.\n\n\tBy using read_in_full(), we can tell when the read reached\n\tthe end of file: when it returns less than was requested,\n\tit's eof.  This way we can avoid an extra iteration that\n\tallocates an extra 8kB that is never used.\n\nIn any case, the change is very sensible.  Thanks, both.\n"}]}