{"thread":{"id":"52832","subject":"Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions","startedAt":"2020-02-18T09:31:06Z","lastAt":"2020-02-19T10:44:59Z","messageCount":8,"participants":["Abhishek Kumar","Robear Selwans","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"391977","messageId":"CAHk66fskrfcJ0YFDhfimVBTJZB4um7r=GdQuM8heJdZtF8D7UQ@mail.gmail.com","threadId":"52832","inReplyTo":null,"subject":"Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions","fromName":"Abhishek Kumar","fromEmail":"abhishekkumar8222@gmail.com","sentAt":"2020-02-18T09:30:00Z","receivedAt":"2020-02-18T09:31:06Z","isPatch":true,"sender":{"key":"abhishekkumar8222@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31231064?v=4"},"body":"Greetings Robear,\n\nI don't have the expertise to talk about the larger picture here, but\nI have the following suggestions if you do go with it.\n\nI don't agree with this patchset being two different patches - The\nchanges in the second patch eliminate possibly buggy behavior of using\nthe new function.\n\nI would also prefer the term \"immutable\" over \"const\" since const\nalready has implications in C programming.\n\n> STRBUF_INIT_CONST: a new way to initialize strbuf\n\nUse imperative mood and be more specific in the commit title -\n`strbuf: Teach strbuf to initialize immutable strings`\n\n> In a previous commit, a new function `STRBUF_INIT_CONST(const_str)`,\n> which would allow for the quick initialization of constant `strbuf`s,\n> was introduced.\n\nRedundant if you merge both patches.\n\nIn this commit, I check through the strbuf_* functions to edit the ones\nthat would try to edit the passed `strbuf` so that they are guaranteed to\nbehave in a predictable way when met with a constant.\n\n> Added Functions:\n> `strbuf_make_var`\n>\n> Updated Functions:\n> `strbuf_grow`\n> `strbuf_setlen`\n> `strbuf_trim`\n> `strbuf_trim_trailing_dir_sep`\n> `strbuf_trim_trailing_newline`\n> `strbuf_tolower`\n> `strbuf_splice`\n>\n> Functions where comments were added to clarify the expected behavior:\n> `strbuf_getcwd`\n\nI feel this is self-explanatory when you go through the diff.\n\nSigned-off-by: Robear Selwans \n---\n strbuf.c | 25 +++++++++++++++++++++++++\n strbuf.h | 14 ++++++++++++++\n 2 files changed, 39 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex f19da55b07..48af039b6e 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -89,6 +89,8 @@ void strbuf_attach(struct strbuf *sb, void *buf,\nsize_t len, size_t alloc)\n\n void strbuf_grow(struct strbuf *sb, size_t extra)\n {\n+ if (sb->len > sb->alloc)\n+ strbuf_make_var(sb);\n int new_buf = !sb->alloc;\n if (unsigned_add_overflows(extra, 1) ||\n unsigned_add_overflows(sb->len, extra + 1))\n@@ -100,8 +102,18 @@ void strbuf_grow(struct strbuf *sb, size_t extra)\n sb->buf[0] = '\\0';\n }\n\n+void strbuf_make_var(struct strbuf *sb)\n+{\n+ char* str_cpy;\n+ ALLOC_ARRAY(str_cpy, sb->len + 1);\n+ memcpy(str_cpy, sb->buf, sb->len);\n+ strbuf_attach(sb, str_cpy, sb->len, sb->len + 1);\n+}\n+\n void strbuf_trim(struct strbuf *sb)\n {\n+ if (sb->len > sb->alloc)\n+ strbuf_make_var(sb);\n strbuf_rtrim(sb);\n strbuf_ltrim(sb);\n }\n@@ -115,6 +127,8 @@ void strbuf_rtrim(struct strbuf *sb)\n\n void strbuf_trim_trailing_dir_sep(struct strbuf *sb)\n {\n+ if (sb->len > sb->alloc)\n+ strbuf_make_var(sb);\n while (sb->len > 0 && is_dir_sep((unsigned char)sb->buf[sb->len - 1]))\n sb->len--;\n sb->buf[sb->len] = '\\0';\n@@ -122,6 +136,9 @@ void strbuf_trim_trailing_dir_sep(struct strbuf *sb)\n\n void strbuf_trim_trailing_newline(struct strbuf *sb)\n {\n+ if (sb->buf[sb->len - 1] == '\\n')\n> + if (sb->len > sb->alloc)\n> + strbuf_make_var(sb);\n\nEnclose this explicitly in braces.\n\n if (sb->len > 0 && sb->buf[sb->len - 1] == '\\n') {\n if (--sb->len > 0 && sb->buf[sb->len - 1] == '\\r')\n --sb->len;\n@@ -158,6 +175,8 @@ int strbuf_reencode(struct strbuf *sb, const char\n*from, const char *to)\n\n void strbuf_tolower(struct strbuf *sb)\n {\n+ if (sb->len > sb->alloc)\n+ strbuf_make_var(sb);\n char *p = sb->buf, *end = sb->buf + sb->len;\n for (; p < end; p++)\n *p = tolower(*p);\n@@ -234,6 +253,8 @@ void strbuf_splice(struct strbuf *sb, size_t pos,\nsize_t len,\n die(\"`pos' is too far after the end of the buffer\");\n if (pos + len > sb->len)\n die(\"`pos + len' is too far after the end of the buffer\");\n+ if (sb->len > sb->alloc)\n+ strbuf_make_var(sb);\n\n if (dlen >= len)\n strbuf_grow(sb, dlen - len);\n@@ -572,6 +593,10 @@ int strbuf_readlink(struct strbuf *sb, const char\n*path, size_t hint)\n\n int strbuf_getcwd(struct strbuf *sb)\n {\n+ /*\n+ * If sb was a constant, then an error would lead to\n+ * strbuf_release() being called on the sb.\n+ */\n size_t oldalloc = sb->alloc;\n size_t guessed_len = 128;\n\ndiff --git a/strbuf.h b/strbuf.h\nindex 1a1753424c..a33bc4d90e 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -128,6 +128,10 @@ static inline void strbuf_swap(struct strbuf *a,\nstruct strbuf *b)\n SWAP(*a, *b);\n }\n\n> +/**\n> + * Constant string buffer is turned into a variable one.\n> + */\n> +void strbuf_make_var(struct strbuf *sb);\n\nUse imperative mood - Convert an immutable string buffer to a variable\nstring buffer.\n\n /**\n * Functions related to the size of the buffer\n@@ -160,6 +164,16 @@ void strbuf_grow(struct strbuf *sb, size_t amount);\n */\n static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n {\n> + if (sb->len > sb->alloc)\n> + {\n> + if (!len)\n> + {\n> + sb->buf = strbuf_slopbuf;\n> + return;\n> + }\n> + else\n> + strbuf_make_var(sb);\n> + }\n\nUse braces on the same line as the condition.\n\n if (len > (sb->alloc ? sb->alloc - 1 : 0))\n die(\"BUG: strbuf_setlen() beyond buffer\");\n sb->len = len;\n--\n2.17.1\n"},{"id":"391987","messageId":"CALH1-Xp_3vSM71DbDNpDK5EHHzworcn_N3JzArwLvvCVTK2mTw@mail.gmail.com","threadId":"52832","inReplyTo":"CAHk66fskrfcJ0YFDhfimVBTJZB4um7r=GdQuM8heJdZtF8D7UQ@mail.gmail.com","subject":"Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions","fromName":"Robear Selwans","fromEmail":"rwagih.rw@gmail.com","sentAt":"2020-02-18T14:42:34Z","receivedAt":"2020-02-18T14:42:48Z","isPatch":true,"sender":{"key":"rwagih.rw@gmail.com","avatar":"https://gravatar.com/avatar/196a8e523e919aa337627b4427365961719a4b821499438fa0698ccfdc8dcf1e?d=mp&s=160"},"body":"All what you said makes a lot of sense. Thanks for the help.\n"},{"id":"392016","messageId":"xmqq36b7k4i6.fsf@gitster-ct.c.googlers.com","threadId":"52832","inReplyTo":"CAHk66fskrfcJ0YFDhfimVBTJZB4um7r=GdQuM8heJdZtF8D7UQ@mail.gmail.com","subject":"Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-18T20:46:41Z","receivedAt":"2020-02-18T20:46:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhishek Kumar writes:\n\n> I would also prefer the term \"immutable\" over \"const\" since const\n> already has implications in C programming.\n\nAs long as the implication the established word conveys is what the\npatch wants to do, it is *better* not to invent another phrase and\ninstead use the well-known term, no?\n\n>> STRBUF_INIT_CONST: a new way to initialize strbuf\n>\n> Use imperative mood and be more specific in the commit title -\n> `strbuf: Teach strbuf to initialize immutable strings`\n\ns/T/t/;\n\n> I feel this is self-explanatory when you go through the diff.\n\nTrue.\n\n> void strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> + if (sb->len > sb->alloc)\n> + strbuf_make_var(sb);\n> int new_buf = !sb->alloc;\n\nThis introduces decl-after-stmt error.\n\nAlso, isn't \"if (sb->alloc < sb->len)\" too loose a check for the new\nfeature? AFAICS in 1/2, a strbuf that is still borrowing a const\nstring always has sb->alloc==0. Other instances of strbuf that\nhappens to satisify the above condition, e.g. (sb->len == 5 &&\nsb->alloc == 1), is an error. If we are to check the condition\nabout sb->len, shouldn't we diagnose such a case as an error, no?\n\n> +void strbuf_make_var(struct strbuf *sb)\n> +{\n> + char* str_cpy;\n\nIsn't make_var() an implementation detail that should not leak\nto the strbuf API users? IOW, does it have to be extern?\n\nIn our codebase (eh, rather, in C as opposed to C++), the asterisk\nsticks to the identifier, not to the type.\n\n> void strbuf_trim_trailing_newline(struct strbuf *sb)\n> {\n> + if (sb->buf[sb->len - 1] == '\\n')\n>> + if (sb->len > sb->alloc)\n>> + strbuf_make_var(sb);\n>\n> Enclose this explicitly in braces.\n\nYup. Also the repetition we see is a sign that something is wrong.\nPerhaps adding a small inline helper \n\n\tstatic inline void strbuf_premutate(sb) \n\t{\n\t\tif (!sb->alloc) {\n\t\t\t... body of strbuf_make_var() comes here ...\n\t\t}\n\t}\n\nand getting rid of strbuf_make_var() would help?\n\nAs Peff, I am a bit hesitant about leaving a strbuf that hasn't been\nmade mutable around, though.\n"},{"id":"392041","messageId":"CALH1-Xr3HVZzDn2-9EvmdiBWmxWQ-zfExM2LNJyR1wR+dgxRSQ@mail.gmail.com","threadId":"52832","inReplyTo":"xmqq36b7k4i6.fsf@gitster-ct.c.googlers.com","subject":"Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions","fromName":"Robear Selwans","fromEmail":"rwagih.rw@gmail.com","sentAt":"2020-02-19T01:43:26Z","receivedAt":"2020-02-19T01:43:41Z","isPatch":true,"sender":{"key":"rwagih.rw@gmail.com","avatar":"https://gravatar.com/avatar/196a8e523e919aa337627b4427365961719a4b821499438fa0698ccfdc8dcf1e?d=mp&s=160"},"body":"On Tue, Feb 18, 2020 at 10:46 PM Junio C Hamano wrote:\n> Abhishek Kumar writes:\n> >> STRBUF_INIT_CONST: a new way to initialize strbuf\n> > Use imperative mood and be more specific in the commit title -\n> > `strbuf: Teach strbuf to initialize immutable strings`\n> s/T/t/;\nI don't get what you mean by that.\n\n> Also, isn't \"if (sb->alloc < sb->len)\" too loose a check for the new\n> feature? AFAICS in 1/2, a strbuf that is still borrowing a const\n> string always has sb->alloc==0. Other instances of strbuf that\n> happens to satisify the above condition, e.g. (sb->len == 5 &&\n> sb->alloc == 1), is an error. If we are to check the condition\n> about sb->len, shouldn't we diagnose such a case as an error, no?\nAFAIK after reading the documentation for `strbuf`, there is no other\ncase where `sb->len > sb->alloc` as `alloc` needs to always be more\nthan `len`. I'd like to be corrected if mistaken, though.\n\n> As Peff, I am a bit hesitant about leaving a strbuf that hasn't been\n> made mutable around, though.\nYeah, I started to get that when I read Peff's reply. I think I'll go with\nthe approach that Peff suggested, where the constant string is\ncopied to a stack array and so is made mutable to a degree. Since\nthis is a different way, I guess I'll make that from scratch instead of\nediting the existing one.\n"},{"id":"392043","messageId":"20200219020550.GA81751@coredump.intra.peff.net","threadId":"52832","inReplyTo":"CALH1-Xr3HVZzDn2-9EvmdiBWmxWQ-zfExM2LNJyR1wR+dgxRSQ@mail.gmail.com","subject":"Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-19T02:05:50Z","receivedAt":"2020-02-19T02:05:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 19, 2020 at 03:43:26AM +0200, Robear Selwans wrote:\n\n> On Tue, Feb 18, 2020 at 10:46 PM Junio C Hamano wrote:\n> > Abhishek Kumar writes:\n> > >> STRBUF_INIT_CONST: a new way to initialize strbuf\n> > > Use imperative mood and be more specific in the commit title -\n> > > `strbuf: Teach strbuf to initialize immutable strings`\n> > s/T/t/;\n> I don't get what you mean by that.\n\nWe don't usually capitalize the sentence fragment in a subject line\n(hence \"replace T with t\", but said in sed jargon).\n\n> > Also, isn't \"if (sb->alloc < sb->len)\" too loose a check for the new\n> > feature? AFAICS in 1/2, a strbuf that is still borrowing a const\n> > string always has sb->alloc==0. Other instances of strbuf that\n> > happens to satisify the above condition, e.g. (sb->len == 5 &&\n> > sb->alloc == 1), is an error. If we are to check the condition\n> > about sb->len, shouldn't we diagnose such a case as an error, no?\n> AFAIK after reading the documentation for `strbuf`, there is no other\n> case where `sb->len > sb->alloc` as `alloc` needs to always be more\n> than `len`. I'd like to be corrected if mistaken, though.\n\nYeah, I don't see how it could be for an allocated buffer, since that\nwould imply writing past the allocated size.\n\n> > As Peff, I am a bit hesitant about leaving a strbuf that hasn't been\n> > made mutable around, though.\n> Yeah, I started to get that when I read Peff's reply. I think I'll go with\n> the approach that Peff suggested, where the constant string is\n> copied to a stack array and so is made mutable to a degree. Since\n> this is a different way, I guess I'll make that from scratch instead of\n> editing the existing one.\n\nYeah, I think it's sufficiently different to start from scratch. Do note\nthat I think you may need to stuff an extra bit into the struct somehow.\nFor a const string, you can set alloc to \"0\" to denote the situation.\nBut if we're going to be able to attach another buffer that can be\nwritten to (but may not yet be full), you'd need to be able to set alloc\nto the true size of that buffer. And then we need some way to note that\nit's not a real heap buffer.\n\nThere are probably clever tricks you could play with the low bits of the\n\"alloc\" field to avoid making the struct any bigger. But given the way\nwe use strbufs, I suspect it may not bring a measurable performance\nimprovement. And I do have dreams of eventually adding more bits. E.g.,\nthere are a few cases where it would be convenient to extend the \"strbuf\nthat points to a stack buffer but grows\" into \"strbuf that points to a\nstack buffer but truncates\" (e.g., in our error handling routines which\nwant to avoid calling malloc).\n\nThat's definitely a few steps out from where we are now, of course, but\nwe may get there eventually.\n\n-Peff\n"},{"id":"392044","messageId":"xmqqv9o3gthb.fsf@gitster-ct.c.googlers.com","threadId":"52832","inReplyTo":"CALH1-Xr3HVZzDn2-9EvmdiBWmxWQ-zfExM2LNJyR1wR+dgxRSQ@mail.gmail.com","subject":"Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-19T03:13:04Z","receivedAt":"2020-02-19T03:13:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robear Selwans writes:\n\n>> Also, isn't \"if (sb->alloc < sb->len)\" too loose a check for the new\n>> feature? AFAICS in 1/2, a strbuf that is still borrowing a const\n>> string always has sb->alloc==0. Other instances of strbuf that\n>> happens to satisify the above condition, e.g. (sb->len == 5 &&\n>> sb->alloc == 1), is an error. If we are to check the condition\n>> about sb->len, shouldn't we diagnose such a case as an error, no?\n> AFAIK after reading the documentation for `strbuf`, there is no other\n> case where `sb->len > sb->alloc` as `alloc` needs to always be more\n> than `len`. I'd like to be corrected if mistaken, though.\n\nYes, but the case that matters to _your_ use is sb->alloc == 0. You\ndo not want to let a broken strbuf (presumably broken by changes\nother than your own) to pass, when you can detect it. And for that,\npaying attention to sb->len _might_ make sense, but then the check\nwon't be \n\n\tif (sb->alloc < sb->len)\n\t\tmake it mutable;\n\nyou'd rather be writing something like\n\n\tif (!sb->alloc)\n\t\tmake it mutable;\n\telse if (sb->alloc < sb->len)\n\t\tBUG(\"somebody fed a corrupt strbuf to me\");\n\nIf the primary purpose of make_mutable() is *not* about catching\nrandom strbuf corruption, then the whole \"else if\" part is not\nneeded, and the check should become a equation only about sb->alloc,\nnot a comparison between alloc and len (which would trigger for *both*\nyour const-initialized strbuf *and* a corrupt one you did not anticipate\nto see).\n"},{"id":"392048","messageId":"CALH1-XoXdpAxYj7r1ouc-v4KrVuYDocBFFC++7eFiRwdHFxmUA@mail.gmail.com","threadId":"52832","inReplyTo":"xmqqv9o3gthb.fsf@gitster-ct.c.googlers.com","subject":"Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions","fromName":"Robear Selwans","fromEmail":"rwagih.rw@gmail.com","sentAt":"2020-02-19T04:34:19Z","receivedAt":"2020-02-19T04:34:35Z","isPatch":true,"sender":{"key":"rwagih.rw@gmail.com","avatar":"https://gravatar.com/avatar/196a8e523e919aa337627b4427365961719a4b821499438fa0698ccfdc8dcf1e?d=mp&s=160"},"body":"On Wed, Feb 19, 2020 at 5:13 AM Junio C Hamano wrote:\n>\n> Yes, but the case that matters to _your_ use is sb->alloc == 0. You\n> do not want to let a broken strbuf (presumably broken by changes\n> other than your own) to pass, when you can detect it. And for that,\n> paying attention to sb->len _might_ make sense, but then the check\n> won't be\n>\n> if (sb->alloc < sb->len)\n> make it mutable;\n>\n> you'd rather be writing something like\n>\n> if (!sb->alloc)\n> make it mutable;\n> else if (sb->alloc < sb->len)\n> BUG(\"somebody fed a corrupt strbuf to me\");\n\nOoh so what you meant, is that corrupt `strbuf`s need to be\nanticipated even if they\ndon't make much sense. Smart.\n"},{"id":"392055","messageId":"xmqqmu9ehn4t.fsf@gitster-ct.c.googlers.com","threadId":"52832","inReplyTo":"CALH1-XoXdpAxYj7r1ouc-v4KrVuYDocBFFC++7eFiRwdHFxmUA@mail.gmail.com","subject":"Re: [GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-19T10:44:50Z","receivedAt":"2020-02-19T10:44:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robear Selwans writes:\n\n> On Wed, Feb 19, 2020 at 5:13 AM Junio C Hamano wrote:\n>>\n>> Yes, but the case that matters to _your_ use is sb->alloc == 0. You\n>> do not want to let a broken strbuf (presumably broken by changes\n>> other than your own) to pass, when you can detect it. And for that,\n>> paying attention to sb->len _might_ make sense, but then the check\n>> won't be\n>>\n>> if (sb->alloc < sb->len)\n>> make it mutable;\n>>\n>> you'd rather be writing something like\n>>\n>> if (!sb->alloc)\n>> make it mutable;\n>> else if (sb->alloc < sb->len)\n>> BUG(\"somebody fed a corrupt strbuf to me\");\n>\n> Ooh so what you meant, is that corrupt `strbuf`s need to be\n> anticipated even if they\n> don't make much sense. Smart.\n\nI don't know if that is smart, but the point is that sb->alloc is\nthe only thing you need to care about if you want to see if the\nstrbuf is borrowing from a const string, and it does not make much\nsense not to catch a corruption, _if_ you are to check the value of\nsb->len as well.\n\n"}]}