{"thread":{"id":"10156","subject":"[PATCH] Fix in-place editing in crlf_to_git and ident_to_git.","startedAt":"2007-10-05T08:11:59Z","lastAt":"2007-10-05T19:33:42Z","messageCount":18,"participants":["Pierre Habouzit","Johannes Schindelin","Johannes Sixt","Bernt Hansen","Linus Torvalds","Sam Ravnborg","Dmitry Potapov"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"54925","messageId":"20071005085522.32EFF1E16E@madism.org","threadId":"10156","inReplyTo":"20071005082026.GE19879@artemis.corp","subject":"[AGGREGATED PATCH] Fix in-place editing functions in convert.c","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-10-05T08:11:59Z","receivedAt":"2007-10-05T08:11:59Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"* crlf_to_git and ident_to_git:\n\n Don't grow the buffer if there is enough space in the first place.\n As a side effect, when the editing is done \"in place\", we don't grow, so\n the buffer pointer doesn't changes, and `src' isn't invalidated anymore.\n\n Thanks to Bernt Hansen for the bug report.\n\n* apply_filter:\n\n Fix memory leak due to fake in-place editing that didn't collected the\n old buffer when the filter succeeds. Also a cosmetic fix.\n\nSigned-off-by: Pierre Habouzit \n---\n\nThis patch is on top of master, and supersedes both patch I sent before.\nFollowing dscho's remark, I only grow the buffer if they aren't big enough\nin the first place, which ensures that buffers are not touched if edited in\nplace.\n\n convert.c | 17 ++++++++++-------\n 1 files changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 0d5e909..aa95834 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -110,7 +110,9 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t\treturn 0;\n \t}\n \n-\tstrbuf_grow(buf, len);\n+\t/* only grow if not in place */\n+\tif (strbuf_avail(buf) + buf->len < len)\n+\t\tstrbuf_grow(buf, len - buf->len);\n \tdst = buf->buf;\n \tif (action == CRLF_GUESS) {\n \t\t/*\n@@ -281,20 +283,19 @@ static int apply_filter(const char *path, const char *src, size_t len,\n \t\tret = 0;\n \t}\n \tif (close(pipe_feed[0])) {\n-\t\tret = error(\"read from external filter %s failed\", cmd);\n+\t\terror(\"read from external filter %s failed\", cmd);\n \t\tret = 0;\n \t}\n \tstatus = finish_command(&child_process);\n \tif (status) {\n-\t\tret = error(\"external filter %s failed %d\", cmd, -status);\n+\t\terror(\"external filter %s failed %d\", cmd, -status);\n \t\tret = 0;\n \t}\n \n \tif (ret) {\n-\t\t*dst = nbuf;\n-\t} else {\n-\t\tstrbuf_release(&nbuf);\n+\t\tstrbuf_swap(dst, &nbuf);\n \t}\n+\tstrbuf_release(&nbuf);\n \treturn ret;\n }\n \n@@ -422,7 +423,9 @@ static int ident_to_git(const char *path, const char *src, size_t len,\n \tif (!ident || !count_ident(src, len))\n \t\treturn 0;\n \n-\tstrbuf_grow(buf, len);\n+\t/* only grow if not in place */\n+\tif (strbuf_avail(buf) + buf->len < len)\n+\t\tstrbuf_grow(buf, len - buf->len);\n \tdst = buf->buf;\n \tfor (;;) {\n \t\tdollar = memchr(src, '$', len);\n-- \n1.5.3.4.207.gb504-dirty\n"},{"id":"54915","messageId":"20071005082026.GE19879@artemis.corp","threadId":"10156","inReplyTo":"87wsu2sad0.fsf@gollum.intra.norang.ca","subject":"[PATCH] Fix in-place editing in crlf_to_git and ident_to_git.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-10-05T08:20:26Z","receivedAt":"2007-10-05T08:20:26Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"When crlf_to_git or ident_to_git are called \"in place\", the buffer already\nis big enough and should not be resized (as it could make the buffer address\nchange, hence invalidate the `src' pointers !).\n\nAlso fix the growth length at the same time: we want to replace the buffer\ncontent (not append) in those functions as they are filters.\n\nThanks to Bernt Hansen for the bug report.\n\nSigned-off-by: Pierre Habouzit \n---\n\n This patch is done on top of master, as strbuf's have been merged.\nThis is a major issue.\n\n convert.c | 12 ++++++++----\n 1 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 0d5e909..4664197 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -110,7 +110,9 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t\treturn 0;\n \t}\n \n-\tstrbuf_grow(buf, len);\n+\t/* only grow if not in place */\n+\tif (src != buf->buf)\n+\t\tstrbuf_grow(buf, len - buf->len);\n \tdst = buf->buf;\n \tif (action == CRLF_GUESS) {\n \t\t/*\n@@ -281,12 +283,12 @@ static int apply_filter(const char *path, const char *src, size_t len,\n \t\tret = 0;\n \t}\n \tif (close(pipe_feed[0])) {\n-\t\tret = error(\"read from external filter %s failed\", cmd);\n+\t\terror(\"read from external filter %s failed\", cmd);\n \t\tret = 0;\n \t}\n \tstatus = finish_command(&child_process);\n \tif (status) {\n-\t\tret = error(\"external filter %s failed %d\", cmd, -status);\n+\t\terror(\"external filter %s failed %d\", cmd, -status);\n \t\tret = 0;\n \t}\n \n@@ -422,7 +424,9 @@ static int ident_to_git(const char *path, const char *src, size_t len,\n \tif (!ident || !count_ident(src, len))\n \t\treturn 0;\n \n-\tstrbuf_grow(buf, len);\n+\t/* only grow if not in place */\n+\tif (src != buf->buf)\n+\t\tstrbuf_grow(buf, len - buf->len);\n \tdst = buf->buf;\n \tfor (;;) {\n \t\tdollar = memchr(src, '$', len);\n-- \n1.5.3.4.207.gc79d4-dirty\n"},{"id":"54917","messageId":"20071005082711.GF19879@artemis.corp","threadId":"10156","inReplyTo":"20071005082026.GE19879@artemis.corp","subject":"[PATCH] Fix memory leak in apply_filter.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-10-05T08:27:11Z","receivedAt":"2007-10-05T08:27:11Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"Signed-off-by: Pierre Habouzit \n---\n\nWhile we're at it... Here is a stupid memory leak in apply_filter.\nOn top of the previous commit.\n\n convert.c | 5 ++---\n 1 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 4664197..d5c197f 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -293,10 +293,9 @@ static int apply_filter(const char *path, const char *src, size_t len,\n \t}\n \n \tif (ret) {\n-\t\t*dst = nbuf;\n-\t} else {\n-\t\tstrbuf_release(&nbuf);\n+\t\tstrbuf_swap(dst, &nbuf);\n \t}\n+\tstrbuf_release(&nbuf);\n \treturn ret;\n }\n \n-- \n1.5.3.4.208.gdcc67-dirty\n"},{"id":"54919","messageId":"20071005082925.GG19879@artemis.corp","threadId":"10156","inReplyTo":"20071005082026.GE19879@artemis.corp","subject":"[PATCH] Fix memory leak in apply_filter.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-10-05T08:29:25Z","receivedAt":"2007-10-05T08:29:25Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"Signed-off-by: Pierre Habouzit \n---\n\nWhile we're at it... Here is a stupid memory leak in apply_filter.\nOn top of the previous commit.\n\nThe same, repost, unsigned doh.\n\n convert.c | 5 ++---\n 1 files changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 4664197..d5c197f 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -293,10 +293,9 @@ static int apply_filter(const char *path, const char *src, size_t len,\n \t}\n \n \tif (ret) {\n-\t\t*dst = nbuf;\n-\t} else {\n-\t\tstrbuf_release(&nbuf);\n+\t\tstrbuf_swap(dst, &nbuf);\n \t}\n+\tstrbuf_release(&nbuf);\n \treturn ret;\n }\n \n-- \n1.5.3.4.208.gdcc67-dirty\n"},{"id":"54920","messageId":"Pine.LNX.4.64.0710050930030.4174@racer.site","threadId":"10156","inReplyTo":"20071005082026.GE19879@artemis.corp","subject":"Re: [PATCH] Fix in-place editing in crlf_to_git and ident_to_git.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-10-05T08:30:45Z","receivedAt":"2007-10-05T08:30:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 5 Oct 2007, Pierre Habouzit wrote:\n\n> When crlf_to_git or ident_to_git are called \"in place\", the buffer \n> already is big enough and should not be resized (as it could make the \n> buffer address change, hence invalidate the `src' pointers !).\n\nI wonder why we resize at all if the buffer is big enough to begin with.\n\nCiao,\nDscho\n"},{"id":"54922","messageId":"20071005084054.GH19879@artemis.corp","threadId":"10156","inReplyTo":"Pine.LNX.4.64.0710050930030.4174@racer.site","subject":"Re: [PATCH] Fix in-place editing in crlf_to_git and ident_to_git.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-10-05T08:40:54Z","receivedAt":"2007-10-05T08:40:54Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Fri, Oct 05, 2007 at 08:30:45AM +0000, Johannes Schindelin wrote:\n> Hi,\n> \n> On Fri, 5 Oct 2007, Pierre Habouzit wrote:\n> \n> > When crlf_to_git or ident_to_git are called \"in place\", the buffer \n> > already is big enough and should not be resized (as it could make the \n> > buffer address change, hence invalidate the `src' pointers !).\n> \n> I wonder why we resize at all if the buffer is big enough to begin with.\n\n strbuf_grow takes care of that itself but indeed you make me see that\nmy patch is wrong. if buf->len > len then len - buf->len is err a bit\nbig.\n\n I'll roll better ones.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"54928","messageId":"470602B1.8090706@viscovery.net","threadId":"10156","inReplyTo":"20071005085522.32EFF1E16E@madism.org","subject":"Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-10-05T09:24:01Z","receivedAt":"2007-10-05T09:24:01Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Pierre Habouzit schrieb:\n> @@ -281,20 +283,19 @@ static int apply_filter(const char *path, const char *src, size_t len,\n> \t\tret = 0;\n> \t}\n> \tif (close(pipe_feed[0])) {\n> -\t\tret = error(\"read from external filter %s failed\", cmd);\n> +\t\terror(\"read from external filter %s failed\", cmd);\n> \t\tret = 0;\n> \t}\n> \tstatus = finish_command(&child_process);\n> \tif (status) {\n> -\t\tret = error(\"external filter %s failed %d\", cmd, -status);\n> +\t\terror(\"external filter %s failed %d\", cmd, -status);\n> \t\tret = 0;\n> \t}\n\nIf you want to, you can leave away these cosmetical corrections since I'm \nworking on a patch that replaces this entire passus. (Will be needed for the \nMinGW port).\n\n-- Hannes\n"},{"id":"54949","messageId":"87r6k9r9qb.fsf@gollum.intra.norang.ca","threadId":"10156","inReplyTo":"20071005085522.32EFF1E16E@madism.org","subject":"Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c","fromName":"Bernt Hansen","fromEmail":"bernt@norang.ca","sentAt":"2007-10-05T13:07:08Z","receivedAt":"2007-10-05T13:07:08Z","isPatch":true,"sender":{"key":"bernt@norang.ca","avatar":null},"body":"This fixes it for me. Thanks!!\n\nBernt\n"},{"id":"54962","messageId":"alpine.LFD.0.999.0710050819540.23684@woody.linux-foundation.org","threadId":"10156","inReplyTo":"20071005085522.32EFF1E16E@madism.org","subject":"Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-10-05T15:26:44Z","receivedAt":"2007-10-05T15:26:44Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 5 Oct 2007, Pierre Habouzit wrote:\n> \n> -\tstrbuf_grow(buf, len);\n> +\t/* only grow if not in place */\n> +\tif (strbuf_avail(buf) + buf->len < len)\n> +\t\tstrbuf_grow(buf, len - buf->len);\n\nUmm. This is really ugly.\n\nThe whole point of strbuf's was that you shouldn't be doing your own \nallocation decisions etc. So why do it?\n\nWouldn't it be much better to have a strbuf_make_room() interface that \njust guarantees that there is enough room fo \"len\"? \n\nOtherwise, code like the above would seem to make the whole point of a \nsafer string interface rather pointless. The above code only makes sense \nif you know how the strbuf's are internally done, so it should not exists \nexcept as internal strbuf code. No?\n\n\t\tLinus\n"},{"id":"54965","messageId":"20071005155023.GA20305@artemis.corp","threadId":"10156","inReplyTo":"alpine.LFD.0.999.0710050819540.23684@woody.linux-foundation.org","subject":"Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-10-05T15:50:23Z","receivedAt":"2007-10-05T15:50:23Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Fri, Oct 05, 2007 at 03:26:44PM +0000, Linus Torvalds wrote:\n> \n> \n> On Fri, 5 Oct 2007, Pierre Habouzit wrote:\n> > \n> > -\tstrbuf_grow(buf, len);\n> > +\t/* only grow if not in place */\n> > +\tif (strbuf_avail(buf) + buf->len < len)\n> > +\t\tstrbuf_grow(buf, len - buf->len);\n> \n> Umm. This is really ugly.\n\n I agree.\n\n> The whole point of strbuf's was that you shouldn't be doing your own \n> allocation decisions etc. So why do it?\n\n the point here is that it's in a \"filter\" that is called like this:\n\nsome_filter(buf->buf, buf->len, buf);\n src len dst\n\n You can call the filter with src/len being data from anywere,\nincluding the current content of the destination buffer.\n\n Then there is two cases, either the filter is known to be done in\nplace, either we can't know or we know it wont.\n\n In the latter case, we have a bit of code like that:\n\n char *to_free = NULL;\n\n if (buf->buf == src)\n\t to_free = strbuf_detach(&buf);\n\n .. hack ..\n free(to_free);\n\n\n In the former case, then there is a small glitch, being that if we are\ndoing in place editing, we should not touch buffer at all (or it would\ninvalidate \"src\"). If we are not in the in-place editing code though,\nthen we have to make the resulting buffer be big enough...\n\n> Wouldn't it be much better to have a strbuf_make_room() interface that \n> just guarantees that there is enough room fo \"len\"? \n\n Right, that would do the same btw ;)\n\n> Otherwise, code like the above would seem to make the whole point of a \n> safer string interface rather pointless. The above code only makes sense \n> if you know how the strbuf's are internally done, so it should not exists \n> except as internal strbuf code. No?\n\n Well, the above code is used in filters to spare reallocations. So if\nwe want to \"blackbox\" such a think, strbuf_make_room isn't the proper\nAPI. We should rather use\n void *strbuf_begin_filter(struct strbuf *sb, const char *src, size_t reslen);\n strbuf_end_filter(void *);\n\n `strbuf_begin_filter` would decide upon the hint `reslen` argument if\nwe know if we can work in place or not (has a meaning iff src points\ninto the strbuf buffer). If not, it could stash the strbuf buffer in the\nreturned void * to be freed at the end of the filter. It seems like a\nbetter alternative than a strbuf_make_room.\n\n Of course, strbuf_begin_filter() would really be simple and basically\nbe:\n\n char *tmp;\n if (src points into sb->buf && reslen > sb->alloc - 1) {\n // in place editing is OK\n return NULL;\n }\n tmp = strbuf_release(&sb);\n strbuf_grow(&sb, len);\n return tmp;\n\n\n and strbuf_end_filter would just be \"free\" :)\n\n\n We could even make \"reslen\" be a ssize_t so that -1 would mean \"I've\nabsolutely no idea how much space I'll need (or just in place editing is\nnot supported). This way, both hacks I described in this mail could be\nhidden in the strbuf module, and be properly documented _and_ safe _and_\nefficient.\n\nWhat do you think ?\n\n[Though if we do that, I still think it's more important to fix the\nbug in master, and have a new patch implementing this approach]\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"54972","messageId":"20071005162139.GC31413@uranus.ravnborg.org","threadId":"10156","inReplyTo":"alpine.LFD.0.999.0710050819540.23684@woody.linux-foundation.org","subject":"Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c","fromName":"Sam Ravnborg","fromEmail":"sam@ravnborg.org","sentAt":"2007-10-05T16:21:39Z","receivedAt":"2007-10-05T16:21:39Z","isPatch":true,"sender":{"key":"sam@ravnborg.org","avatar":"https://gravatar.com/avatar/168a912606ed0742d840bb365e3cc21db390c36531a58341dc7a069cc1f15f62?d=mp&s=160"},"body":"On Fri, Oct 05, 2007 at 08:26:44AM -0700, Linus Torvalds wrote:\n> \n> \n> On Fri, 5 Oct 2007, Pierre Habouzit wrote:\n> > \n> > -\tstrbuf_grow(buf, len);\n> > +\t/* only grow if not in place */\n> > +\tif (strbuf_avail(buf) + buf->len < len)\n> > +\t\tstrbuf_grow(buf, len - buf->len);\n> \n> Umm. This is really ugly.\n> \n> The whole point of strbuf's was that you shouldn't be doing your own \n> allocation decisions etc. So why do it?\n> \n> Wouldn't it be much better to have a strbuf_make_room() interface that \n> just guarantees that there is enough room fo \"len\"? \n> \n> Otherwise, code like the above would seem to make the whole point of a \n> safer string interface rather pointless. The above code only makes sense \n> if you know how the strbuf's are internally done, so it should not exists \n> except as internal strbuf code. No?\n\nTook a short look at strbuf.h after seeing the above code.\nAnd I was suprised to see that all strbuf users were exposed to\nthe strbuf structure.\nFollowing patch would at least make sure noone fiddle with strbuf internals.\nCut'n'paste - only for the example of it.\nIt simply moves strbuf declaration to the .c file where it rightfully belongs.\n\ngit did not build with this change....\n\n\tSam\n\n\ndiff --git a/strbuf.c b/strbuf.c\nindex e33d06b..0d2d578 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -1,6 +1,14 @@\n #include \"cache.h\"\n #include \"strbuf.h\"\n \n+struct strbuf {\n+ int alloc;\n+ int len;\n+ int eof;\n+ char *buf;\n+};\n+\n+\n void strbuf_init(struct strbuf *sb) {\n sb->buf = NULL;\n sb->eof = sb->alloc = sb->len = 0;\ndiff --git a/strbuf.h b/strbuf.h\nindex 74cc012..c057be3 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -1,11 +1,6 @@\n #ifndef STRBUF_H\n #define STRBUF_H\n-struct strbuf {\n- int alloc;\n- int len;\n- int eof;\n- char *buf;\n-};\n+struct strbuf;\n \n extern void strbuf_init(struct strbuf *);\n extern void read_line(struct strbuf *, FILE *, int);\n"},{"id":"54977","messageId":"20071005163517.GD20305@artemis.corp","threadId":"10156","inReplyTo":"20071005162139.GC31413@uranus.ravnborg.org","subject":"Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-10-05T16:35:17Z","receivedAt":"2007-10-05T16:35:17Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Fri, Oct 05, 2007 at 04:21:39PM +0000, Sam Ravnborg wrote:\n> On Fri, Oct 05, 2007 at 08:26:44AM -0700, Linus Torvalds wrote:\n> > \n> > \n> > On Fri, 5 Oct 2007, Pierre Habouzit wrote:\n> > > \n> > > -\tstrbuf_grow(buf, len);\n> > > +\t/* only grow if not in place */\n> > > +\tif (strbuf_avail(buf) + buf->len < len)\n> > > +\t\tstrbuf_grow(buf, len - buf->len);\n> > \n> > Umm. This is really ugly.\n> > \n> > The whole point of strbuf's was that you shouldn't be doing your own \n> > allocation decisions etc. So why do it?\n> > \n> > Wouldn't it be much better to have a strbuf_make_room() interface that \n> > just guarantees that there is enough room fo \"len\"? \n> > \n> > Otherwise, code like the above would seem to make the whole point of a \n> > safer string interface rather pointless. The above code only makes sense \n> > if you know how the strbuf's are internally done, so it should not exists \n> > except as internal strbuf code. No?\n> \n> Took a short look at strbuf.h after seeing the above code.\n> And I was suprised to see that all strbuf users were exposed to\n> the strbuf structure.\n> Following patch would at least make sure noone fiddle with strbuf internals.\n> Cut'n'paste - only for the example of it.\n> It simply moves strbuf declaration to the .c file where it rightfully belongs.\n\n you're looking at an antiquated version, please look at the one in\ncurrent master on current next. In this one, what you can do or not do\nwith the struct is explained\n\n> git did not build with this change....\n\n Of course it doesn't! people want to have direct access to ->buf and\n->len, and it's definitely OK.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"54980","messageId":"alpine.LFD.0.999.0710050933330.23684@woody.linux-foundation.org","threadId":"10156","inReplyTo":"20071005162139.GC31413@uranus.ravnborg.org","subject":"Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-10-05T16:43:19Z","receivedAt":"2007-10-05T16:43:19Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 5 Oct 2007, Sam Ravnborg wrote:\n> \n> Took a short look at strbuf.h after seeing the above code.\n> And I was suprised to see that all strbuf users were exposed to\n> the strbuf structure.\n\nWell, they *have* to. We want people to declare their strbufs as automatic \nor static structures, and using a opaque struct pointer is *not* an option \n(like \"FILE\" is doing in stdio.h).\n\n> Following patch would at least make sure noone fiddle with strbuf internals.\n\nNo, following patch is fundamentally broken - it's not even a good \nstarting point. It's bad, bad, bad.\n\nIt's also broken in another way: we want it to be really easy to use \nstrbuf's as normal C strings.\n\nYes, many (totally idiotic and broken) interfaces think it's so important \nto \"protect\" their internal data structures that you have a \n\"string_to_c()\" helper function for that. That may be \"good abstraction\", \nbut it's totally idiotic, because it results in horrible source code!\n\nTell me which is more readable:\n\n\tprintf(\"Hello %s\\n\", sb->buf);\n\nor\n\n\tprintf(\"Hello %s\\n\", strbuf_to_c(sb));\n\nand I claim that anybody who claims that the latter is \"more readable\" is \nfull of shit, and has an agenda to push, so it's \"more agenda-friendly\" \nrather than readable!\n\nSo having \"sb->buf\" and \"sb->len\" be visible to users is a *good* thing. \nOtherwise you end up having to create millions of idiotic small helper \nfunctions, rather than just use the standard ones.\n\n\t\t\tLinus\n"},{"id":"54985","messageId":"20071005172425.GD31413@uranus.ravnborg.org","threadId":"10156","inReplyTo":"alpine.LFD.0.999.0710050933330.23684@woody.linux-foundation.org","subject":"Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c","fromName":"Sam Ravnborg","fromEmail":"sam@ravnborg.org","sentAt":"2007-10-05T17:24:25Z","receivedAt":"2007-10-05T17:24:25Z","isPatch":true,"sender":{"key":"sam@ravnborg.org","avatar":"https://gravatar.com/avatar/168a912606ed0742d840bb365e3cc21db390c36531a58341dc7a069cc1f15f62?d=mp&s=160"},"body":"Hi Linus.\n\n> No, following patch is fundamentally broken - it's not even a good \n> starting point. It's bad, bad, bad.\n> \n> It's also broken in another way: we want it to be really easy to use \n> strbuf's as normal C strings.\n> \n> Yes, many (totally idiotic and broken) interfaces think it's so important \n> to \"protect\" their internal data structures that you have a \n> \"string_to_c()\" helper function for that. That may be \"good abstraction\", \n> but it's totally idiotic, because it results in horrible source code!\n> \n> Tell me which is more readable:\n> \n> \tprintf(\"Hello %s\\n\", sb->buf);\n> \n> or\n> \n> \tprintf(\"Hello %s\\n\", strbuf_to_c(sb));\n\nPoint taken although no sane person would name it strbuf_to_c(...).\n\n\tSam\n"},{"id":"54986","messageId":"20071005172505.GE31413@uranus.ravnborg.org","threadId":"10156","inReplyTo":"20071005163517.GD20305@artemis.corp","subject":"Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c","fromName":"Sam Ravnborg","fromEmail":"sam@ravnborg.org","sentAt":"2007-10-05T17:25:05Z","receivedAt":"2007-10-05T17:25:05Z","isPatch":true,"sender":{"key":"sam@ravnborg.org","avatar":"https://gravatar.com/avatar/168a912606ed0742d840bb365e3cc21db390c36531a58341dc7a069cc1f15f62?d=mp&s=160"},"body":"On Fri, Oct 05, 2007 at 06:35:17PM +0200, Pierre Habouzit wrote:\n> On Fri, Oct 05, 2007 at 04:21:39PM +0000, Sam Ravnborg wrote:\n> > On Fri, Oct 05, 2007 at 08:26:44AM -0700, Linus Torvalds wrote:\n> > > \n> > > \n> > > On Fri, 5 Oct 2007, Pierre Habouzit wrote:\n> > > > \n> > > > -\tstrbuf_grow(buf, len);\n> > > > +\t/* only grow if not in place */\n> > > > +\tif (strbuf_avail(buf) + buf->len < len)\n> > > > +\t\tstrbuf_grow(buf, len - buf->len);\n> > > \n> > > Umm. This is really ugly.\n> > > \n> > > The whole point of strbuf's was that you shouldn't be doing your own \n> > > allocation decisions etc. So why do it?\n> > > \n> > > Wouldn't it be much better to have a strbuf_make_room() interface that \n> > > just guarantees that there is enough room fo \"len\"? \n> > > \n> > > Otherwise, code like the above would seem to make the whole point of a \n> > > safer string interface rather pointless. The above code only makes sense \n> > > if you know how the strbuf's are internally done, so it should not exists \n> > > except as internal strbuf code. No?\n> > \n> > Took a short look at strbuf.h after seeing the above code.\n> > And I was suprised to see that all strbuf users were exposed to\n> > the strbuf structure.\n> > Following patch would at least make sure noone fiddle with strbuf internals.\n> > Cut'n'paste - only for the example of it.\n> > It simply moves strbuf declaration to the .c file where it rightfully belongs.\n> \n> you're looking at an antiquated version, please look at the one in\n> current master on current next. In this one, what you can do or not do\n> with the struct is explained\n> \n> > git did not build with this change....\n> \n> Of course it doesn't! people want to have direct access to ->buf and\n> ->len, and it's definitely OK.\n\nUnderstood now - thanks for the clarification.\n\n\tSam\n"},{"id":"54989","messageId":"alpine.LFD.0.999.0710051036282.23684@woody.linux-foundation.org","threadId":"10156","inReplyTo":"20071005172425.GD31413@uranus.ravnborg.org","subject":"Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-10-05T18:05:01Z","receivedAt":"2007-10-05T18:05:01Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 5 Oct 2007, Sam Ravnborg wrote:\n> \n> Point taken although no sane person would name it strbuf_to_c(...).\n\nI agree with the \"no sane person\", but the problem is that the insane \npeople seem to be in no short supply.\n\nGo look at string libraries, and they all do something like that. Or \nworse.\n\n - ustr: yes, it uses *exactly* what I described: \"ustr_cstr()\" and \n \"ustr_len()\" instead of having the data/length available easily \n (although it claims to do it for size reasons - perhaps valid in some \n cases!)\n\n - libast: SPIF_CHARPTR_C(x). No, really.\n\n - Vstr: doesn't have a linear data representation. Needs explicit \n flattening - although it appears to be something you're not ever \n supposed to do - it has 200+ functions to do various magic things. \n\n - Qt (QString): QString::\"data()\", \"ascii()\" or \"utf8()\" or something.\n\n At least this has the excuse of really being able to handle different \n locales (it didn't do that originally, though!), but they end up having \n a million helper functions exactly because you cannot use the normal \n string routines on anything!\n\n - safesrtr, bstring: you just cast the pointer to \"char *\". Now *that* is \n classy and safe.\n\nSo there's a few sane out there, but I actually think they are in the \nminority (Glib, others)\n\n\t\t\tLinus\n"},{"id":"54992","messageId":"37fcd2780710051227n96d29a9v543831ca8292c862@mail.gmail.com","threadId":"10156","inReplyTo":"alpine.LFD.0.999.0710051036282.23684@woody.linux-foundation.org","subject":"Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2007-10-05T19:27:55Z","receivedAt":"2007-10-05T19:27:55Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On 10/5/07, Linus Torvalds wrote:\n> - Qt (QString): QString::\"data()\", \"ascii()\" or \"utf8()\" or something.\n>\n> At least this has the excuse of really being able to handle different\n> locales (it didn't do that originally, though!), but they end up having\n> a million helper functions exactly because you cannot use the normal\n> string routines on anything!\n\nI am afraid you cannot allow the direct access to the internal buffer of\na string if this buffer can be implicitly shared between different\ninstances as it is the case with QString. Because when you want to make\nsome modification or want to get a non-const pointer to this buffer, its\ncontent has to be copied if the buffer is shared between a few copies.\n\nOn the other hand, I don't see what is the problem with using C string\nroutines with it. ::data() returns a pointer and :capacity () returns\nallocated size of the buffer. ::resize() changes the size of the string.\nIf you need a greater allocated size, you can use ::reserve().\nOr did I miss something?\n\nDmitry\n"},{"id":"54993","messageId":"alpine.LFD.0.999.0710051232300.23684@woody.linux-foundation.org","threadId":"10156","inReplyTo":"37fcd2780710051227n96d29a9v543831ca8292c862@mail.gmail.com","subject":"Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-10-05T19:33:42Z","receivedAt":"2007-10-05T19:33:42Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 5 Oct 2007, Dmitry Potapov wrote:\n> \n> On the other hand, I don't see what is the problem with using C string\n> routines with it. ::data() returns a pointer and :capacity () returns\n> allocated size of the buffer. ::resize() changes the size of the string.\n> If you need a greater allocated size, you can use ::reserve().\n> Or did I miss something?\n\nNo, you didn't miss anything. Except for the fact that the original point \nwas that it's just a damn lot simpler and more straightforward to code \njust using \"sb->buf\" for the data. IOW, go back two or three emails in the \nthread.\n\n\t\tLinus\n"}]}