{"thread":{"id":"28876","subject":"[PATCH] blame.c: Properly initialize strbuf after calling textconv_object(), again","startedAt":"2011-11-07T15:53:10Z","lastAt":"2011-11-07T17:41:36Z","messageCount":6,"participants":["Sebastian Schuberth","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"179040","messageId":"4EB7FEE6.9000609@gmail.com","threadId":"28876","inReplyTo":null,"subject":"[PATCH] blame.c: Properly initialize strbuf after calling textconv_object(), again","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2011-11-07T15:53:10Z","receivedAt":"2011-11-07T15:53:10Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"2564aa4 started to initialize buf.alloc, but that should actually be one\nmore byte than the string length due to the trailing \\0.\n---\n builtin/blame.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 86c0537..45f0dcc 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2114,7 +2114,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \t\tcase S_IFREG:\n \t\t\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&\n \t\t\t textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) {\n-\t\t\t\tbuf.alloc = buf_len;\n+\t\t\t\tbuf.alloc = buf_len + 1;\n \t\t\t\tbuf.len = buf_len;\n \t\t\t}\n \t\t\telse if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)\n-- \n1.7.8.rc0.46.g5ae0f.dirty\n"},{"id":"179047","messageId":"7vy5vr282s.fsf@alter.siamese.dyndns.org","threadId":"28876","inReplyTo":"4EB7FEE6.9000609@gmail.com","subject":"Re: [PATCH] blame.c: Properly initialize strbuf after calling textconv_object(), again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-07T16:47:23Z","receivedAt":"2011-11-07T16:47:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> 2564aa4 started to initialize buf.alloc, but that should actually be one\n> more byte than the string length due to the trailing \\0.\n\nEven when the conversion result is a zero-length string?\n\n> ---\n\nSign-off?\n\n> builtin/blame.c | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 86c0537..45f0dcc 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2114,7 +2114,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n> \t\tcase S_IFREG:\n> \t\t\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&\n> \t\t\t textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) {\n> -\t\t\t\tbuf.alloc = buf_len;\n> +\t\t\t\tbuf.alloc = buf_len + 1;\n> \t\t\t\tbuf.len = buf_len;\n> \t\t\t}\n> \t\t\telse if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)\n"},{"id":"179048","messageId":"20111107164946.GD27055@sigill.intra.peff.net","threadId":"28876","inReplyTo":"4EB7FEE6.9000609@gmail.com","subject":"Re: [PATCH] blame.c: Properly initialize strbuf after calling textconv_object(), again","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-07T16:49:46Z","receivedAt":"2011-11-07T16:49:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 07, 2011 at 04:53:10PM +0100, Sebastian Schuberth wrote:\n\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2114,7 +2114,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n> \t\tcase S_IFREG:\n> \t\t\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&\n> \t\t\t textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) {\n> -\t\t\t\tbuf.alloc = buf_len;\n> +\t\t\t\tbuf.alloc = buf_len + 1;\n> \t\t\t\tbuf.len = buf_len;\n> \t\t\t}\n> \t\t\telse if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)\n\nYour patch is correct in the sense that this fixes a bug, but it just\nseems wrong to be touching the alloc parameter outside of the strbuf\ncode. It looks like the intent is to do the equivalent of:\n\n strbuf_add(&buf, some_heap_buf, len);\n free(some_heap_buf);\n\nbut avoid the extra allocation and copy. Should there be a\n\"strbuf_attach\" function to encapsulate this pattern?\n\n-Peff\n"},{"id":"179054","messageId":"4EB815EB.30702@gmail.com","threadId":"28876","inReplyTo":"7vy5vr282s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] blame.c: Properly initialize strbuf after calling textconv_object(), again","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2011-11-07T17:31:23Z","receivedAt":"2011-11-07T17:31:23Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 07.11.2011 17:47, Junio C Hamano wrote:\n\n>> 2564aa4 started to initialize buf.alloc, but that should actually be one\n>> more byte than the string length due to the trailing \\0.\n>\n> Even when the conversion result is a zero-length string?\n\nIn this case, yes. The string buffer is initialized (and detached) in \nrun_textconv, which calls strbuf_read, which grows the (yet empty) \nstring to 8192 bytes. So alloc is always > 0, and the detach will trim \nalloc to len + 1.\n\nHowever, Peff made another valid point and I'll send v2 soon.\n\n-- \nSebastian Schuberth\n"},{"id":"179055","messageId":"4EB8166E.9000703@gmail.com","threadId":"28876","inReplyTo":"20111107164946.GD27055@sigill.intra.peff.net","subject":"[PATCHv2] blame.c: Properly initialize strbuf after calling textconv_object(), again","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2011-11-07T17:33:34Z","receivedAt":"2011-11-07T17:33:34Z","isPatch":false,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"2564aa4 started to initialize buf.alloc, but that should actually be one\nmore byte than the string length due to the trailing \\0. Also, do not\nmodify buf.alloc out of the strbuf code. Use the existing strbuf_attach\ninstead.\n\nSigned-off-by: Sebastian Schuberth \n---\n builtin/blame.c | 7 +++----\n 1 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 86c0537..80febbe 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2096,6 +2096,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \tif (!contents_from || strcmp(\"-\", contents_from)) {\n \t\tstruct stat st;\n \t\tconst char *read_from;\n+\t\tchar *buf_ptr;\n \t\tunsigned long buf_len;\n \n \t\tif (contents_from) {\n@@ -2113,10 +2114,8 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \t\tswitch (st.st_mode & S_IFMT) {\n \t\tcase S_IFREG:\n \t\t\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&\n-\t\t\t textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) {\n-\t\t\t\tbuf.alloc = buf_len;\n-\t\t\t\tbuf.len = buf_len;\n-\t\t\t}\n+\t\t\t textconv_object(read_from, mode, null_sha1, &buf_ptr, &buf_len))\n+\t\t\t\tstrbuf_attach(&buf, buf_ptr, buf_len, buf_len + 1);\n \t\t\telse if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)\n \t\t\t\tdie_errno(\"cannot open or read '%s'\", read_from);\n \t\t\tbreak;\n-- \n1.7.8.rc0.47.gf2c75.dirty\n"},{"id":"179056","messageId":"20111107174136.GA4047@sigill.intra.peff.net","threadId":"28876","inReplyTo":"4EB8166E.9000703@gmail.com","subject":"Re: [PATCHv2] blame.c: Properly initialize strbuf after calling textconv_object(), again","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-07T17:41:36Z","receivedAt":"2011-11-07T17:41:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 07, 2011 at 06:33:34PM +0100, Sebastian Schuberth wrote:\n\n> 2564aa4 started to initialize buf.alloc, but that should actually be one\n> more byte than the string length due to the trailing \\0. Also, do not\n> modify buf.alloc out of the strbuf code. Use the existing strbuf_attach\n> instead.\n\nHeh. When I wrote the previous email I just made up the name\n\"strbuf_attach\" as the opposite of \"strbuf_detach\". I didn't know it\nactually already existed.\n\n> -\t\t\t textconv_object(read_from, mode, null_sha1, &buf.buf, &buf_len)) {\n> -\t\t\t\tbuf.alloc = buf_len;\n> -\t\t\t\tbuf.len = buf_len;\n> -\t\t\t}\n> +\t\t\t textconv_object(read_from, mode, null_sha1, &buf_ptr, &buf_len))\n> +\t\t\t\tstrbuf_attach(&buf, buf_ptr, buf_len, buf_len + 1);\n\nLooks OK to me.\n\n-Peff\n"}]}