{"thread":{"id":"52831","subject":"[GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf","startedAt":"2020-02-18T04:18:23Z","lastAt":"2020-02-27T15:55:46Z","messageCount":11,"participants":["Robear Selwans","Jeff King","Johannes Sixt","René Scharfe","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"391961","messageId":"20200218041805.10939-2-robear.selwans@outlook.com","threadId":"52831","inReplyTo":"20200218041805.10939-1-robear.selwans@outlook.com","subject":"[GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf","fromName":"Robear Selwans","fromEmail":"rwagih.rw@gmail.com","sentAt":"2020-02-18T04:18:04Z","receivedAt":"2020-02-18T04:18:23Z","isPatch":true,"sender":{"key":"rwagih.rw@gmail.com","avatar":"https://gravatar.com/avatar/196a8e523e919aa337627b4427365961719a4b821499438fa0698ccfdc8dcf1e?d=mp&s=160"},"body":"A new function `STRBUF_INIT_CONST(const_str)` was added to allow for a\nquick initialization of strbuf.\n\nDetails:\nUsing `STRBUF_INIT_CONST(str)` creates a new struct of type `strbuf` and\ninitializes its `buf`, `len` and `alloc` as `str`, `strlen(str)` and\n`0`, respectively.\n\nUse Case:\nThis is meant to be used to initialize strbufs with constant values and\nthus, only allocating memory when needed.\n\nUsage Example:\n```\nstrbuf env_var = STRBUF_INIT_CONST(\"dev\");\n```\n\nThis was added according to the issue opened at [https://github.com/gitgitgadget/git/issues/398]\n\nSigned-off-by: Robear Selwans <robear.selwans@outlook.com>\n---\n strbuf.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex bfa66569a4..1a1753424c 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -71,6 +71,7 @@ struct strbuf {\n \n extern char strbuf_slopbuf[];\n #define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }\n+#define STRBUF_INIT_CONST(const_str)  { .alloc = 0, .len = strlen(const_str), .buf = const_str }\n \n /*\n  * Predeclare this here, since cache.h includes this file before it defines the\n-- \n2.17.1\n\n"},{"id":"391962","messageId":"20200218041805.10939-1-robear.selwans@outlook.com","threadId":"52831","inReplyTo":null,"subject":"[GSoC][RFC][PATCH 0/2] STRBUF_INIT_CONST Cover","fromName":"Robear Selwans","fromEmail":"rwagih.rw@gmail.com","sentAt":"2020-02-18T04:18:03Z","receivedAt":"2020-02-18T04:18:23Z","isPatch":true,"sender":{"key":"rwagih.rw@gmail.com","avatar":"https://gravatar.com/avatar/196a8e523e919aa337627b4427365961719a4b821499438fa0698ccfdc8dcf1e?d=mp&s=160"},"body":"This was added according to the issue opened at [https://github.com/gitgitgadget/git/issues/398]\n\n[Copied directly from the issue description]\n> This new way to initialize an strbuf would set buf to the specified\n> constant string, the appropriate len = strlen(string), and alloc to 0.\n \n> That way, we can initialize strbufs with constant values, only\n> allocating memory when/if needed.\n\nThings Done:\n\t- Added a new way to initialize constant `strbuf`\n\t- Edited functions that try to edit the `strbuf` to check if it was\n\tinitialized as a constant\n\nRobear Selwans (2):\n  STRBUF_INIT_CONST: a new way to initialize strbuf\n  STRBUF_INIT_CONST: Adapting strbuf_* functions\n\n strbuf.c | 25 +++++++++++++++++++++++++\n strbuf.h | 15 +++++++++++++++\n 2 files changed, 40 insertions(+)\n\n-- \n2.17.1\n\n"},{"id":"391963","messageId":"20200218041805.10939-3-robear.selwans@outlook.com","threadId":"52831","inReplyTo":"20200218041805.10939-1-robear.selwans@outlook.com","subject":"[GSoC][RFC][PATCH 2/2] STRBUF_INIT_CONST: Adapting strbuf_* functions","fromName":"Robear Selwans","fromEmail":"rwagih.rw@gmail.com","sentAt":"2020-02-18T04:18:05Z","receivedAt":"2020-02-18T04:18:24Z","isPatch":true,"sender":{"key":"rwagih.rw@gmail.com","avatar":"https://gravatar.com/avatar/196a8e523e919aa337627b4427365961719a4b821499438fa0698ccfdc8dcf1e?d=mp&s=160"},"body":"In a previous commit, a new function `STRBUF_INIT_CONST(const_str)`,\nwhich would allow for the quick initialization of constant `strbuf`s,\nwas introduced.\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\nAdded Functions:\n    `strbuf_make_var`\n\nUpdated 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\nFunctions where comments were added to clarify the expected behavior:\n    `strbuf_getcwd`\n\nSigned-off-by: Robear Selwans <robear.selwans@outlook.com>\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, size_t len, size_t alloc)\n \n void strbuf_grow(struct strbuf *sb, size_t extra)\n {\n+\tif (sb->len > sb->alloc)\n+\t\tstrbuf_make_var(sb);\n \tint new_buf = !sb->alloc;\n \tif (unsigned_add_overflows(extra, 1) ||\n \t    unsigned_add_overflows(sb->len, extra + 1))\n@@ -100,8 +102,18 @@ void strbuf_grow(struct strbuf *sb, size_t extra)\n \t\tsb->buf[0] = '\\0';\n }\n \n+void strbuf_make_var(struct strbuf *sb)\n+{\n+\tchar* str_cpy;\n+\tALLOC_ARRAY(str_cpy, sb->len + 1);\n+\tmemcpy(str_cpy, sb->buf, sb->len);\n+\tstrbuf_attach(sb, str_cpy, sb->len, sb->len + 1);\n+}\n+\n void strbuf_trim(struct strbuf *sb)\n {\n+\tif (sb->len > sb->alloc)\n+\t\tstrbuf_make_var(sb);\n \tstrbuf_rtrim(sb);\n \tstrbuf_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+\tif (sb->len > sb->alloc)\n+\t\tstrbuf_make_var(sb);\n \twhile (sb->len > 0 && is_dir_sep((unsigned char)sb->buf[sb->len - 1]))\n \t\tsb->len--;\n \tsb->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+\tif (sb->buf[sb->len - 1] == '\\n')\n+\t\tif (sb->len > sb->alloc)\n+\t\t\tstrbuf_make_var(sb);\n \tif (sb->len > 0 && sb->buf[sb->len - 1] == '\\n') {\n \t\tif (--sb->len > 0 && sb->buf[sb->len - 1] == '\\r')\n \t\t\t--sb->len;\n@@ -158,6 +175,8 @@ int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)\n \n void strbuf_tolower(struct strbuf *sb)\n {\n+\tif (sb->len > sb->alloc)\n+\t\tstrbuf_make_var(sb);\n \tchar *p = sb->buf, *end = sb->buf + sb->len;\n \tfor (; p < end; p++)\n \t\t*p = tolower(*p);\n@@ -234,6 +253,8 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,\n \t\tdie(\"`pos' is too far after the end of the buffer\");\n \tif (pos + len > sb->len)\n \t\tdie(\"`pos + len' is too far after the end of the buffer\");\n+\tif (sb->len > sb->alloc)\n+\t\tstrbuf_make_var(sb);\n \n \tif (dlen >= len)\n \t\tstrbuf_grow(sb, dlen - len);\n@@ -572,6 +593,10 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\n \n int strbuf_getcwd(struct strbuf *sb)\n {\n+\t/*\n+\t * If sb was a constant, then an error would lead to \n+\t * strbuf_release() being called on the sb.\n+\t */\n \tsize_t oldalloc = sb->alloc;\n \tsize_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, struct strbuf *b)\n \tSWAP(*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 \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+\tif (sb->len > sb->alloc)\n+\t{\n+\t\tif (!len)\n+\t\t{\n+\t\t\tsb->buf = strbuf_slopbuf;\n+\t\t\treturn;\n+\t\t}\n+\t\telse\n+\t\t\tstrbuf_make_var(sb);\n+\t}\n \tif (len > (sb->alloc ? sb->alloc - 1 : 0))\n \t\tdie(\"BUG: strbuf_setlen() beyond buffer\");\n \tsb->len = len;\n-- \n2.17.1\n\n"},{"id":"391969","messageId":"20200218062124.GF1641086@coredump.intra.peff.net","threadId":"52831","inReplyTo":"20200218041805.10939-2-robear.selwans@outlook.com","subject":"Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-18T06:21:24Z","receivedAt":"2020-02-18T06:21:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 18, 2020 at 04:18:04AM +0000, Robear Selwans wrote:\n\n> A new function `STRBUF_INIT_CONST(const_str)` was added to allow for a\n> quick initialization of strbuf.\n> \n> Details:\n> Using `STRBUF_INIT_CONST(str)` creates a new struct of type `strbuf` and\n> initializes its `buf`, `len` and `alloc` as `str`, `strlen(str)` and\n> `0`, respectively.\n> \n> Use Case:\n> This is meant to be used to initialize strbufs with constant values and\n> thus, only allocating memory when needed.\n> \n> Usage Example:\n> ```\n> strbuf env_var = STRBUF_INIT_CONST(\"dev\");\n> ```\n\nThis seems a bit dangerous to me, as we're initializing a non-const\npointer with a string literal. In fact, I'm a little surprised that the\ncompiler doesn't complain, but I think it's mostly due to historical\nC-isms (the type of string literals is array-of-char). Using gcc's\n-Wwrite-strings does complain, but there are several other cases already\nin Git (looking at a few, I think there are some opportunities for\ncleanup).\n\nYour second patch catches cases where the strbuf functions want to write\nto the buffer. But we've always been pretty open about the fact that\nstrbuf.buf is a writeable C-style string. So something like this:\n\n  struct strbuf x = STRBUF_INIT_CONST(\"foo\");\n  size_t i;\n\n  for (i = 0; i < x.len; i++)\n\tx.buf[i] = toupper(x.buf[i]);\n\nwould generate no compile-time warnings, but would invoke undefined\nbehavior (on my system it segfaults when run, but it could have even\nmore confusing outcomes). Even though this is called out specifically in\nthe strbuf docs:\n\n   However, it is totally safe to modify anything in the string pointed\n   by the `buf` member, between the indices `0` and `len-1` (inclusive).\n\nOf course it would be easy to fix that by adding a strbuf_make_var()\ncall. But my concern is cases where the const-ness and the use of the\nstrbuf are far apart. The point of a strbuf is that you can just use it\nwithout worrying, but now it's carrying this extra hidden state.\n\nIf we want to pursue this direction, I think we'd do better to give each\nstrbuf a matching array. Something like:\n\n  #define STRBUF_INIT_FROM(buf) { .alloc = 0, .buf = buf, .len = ARRAY_SIZE(buf)-1 }\n  ...\n  char foo_buf[] = \"this is the constant value\";\n  struct strbuf foo = STRBUF_INIT_FROM(foo_buf);\n\nThat gives you a true writeable buffer with the const data in it. _And_\nit opens up the option of strbufs using stack buffers with an empty\ninitial value for efficiency (i.e., avoiding the heap at all for short\ncommon cases, but being able to grow when needed). One trouble is that\nyou can't do it all in a single variable. You'd need something like:\n\n  #define DECLARE_STACK_STRBUF(name, contents) \\\n\tchar name##_buf[] = (contents);\n\tstruct strbuf name = STRBUF_INIT_FROM(name##_buf)\n  ...\n  DECLARE_STACK_STRBUF(foo, \"this is the constant value\");\n\nBut that gets weirdly un-C-like (your macro expands to multiple\nstatements, which is usually a macro pitfall; but we can't use the usual\n\"do { } while(0)\" trick here, because the variables would go out of\nscope at the end of the fake block.\n\nSo I think there are interesting directions here, but there's a lot of\nstuff to figure out.\n\nI notice you put GSoC in your subject line. If you're looking at this as\na microproject, IMHO this is _way_ more complicated and subtle than a\nmicroproject should be. The goal there is to give something so easy that\nyou get to focus on getting your patches in and interacting with the\ncommunity. The scope I'd expect is more along the lines of compiling\nwith -Wwrite-strings and cleaning up some of the locations that\ncomplain.\n\n-Peff\n"},{"id":"391986","messageId":"CALH1-XoWmojFZir1JTU490GNHE1CETs4jbjswH316cAWqjF+Bg@mail.gmail.com","threadId":"52831","inReplyTo":"20200218062124.GF1641086@coredump.intra.peff.net","subject":"Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf","fromName":"Robear Selwans","fromEmail":"rwagih.rw@gmail.com","sentAt":"2020-02-18T14:19:38Z","receivedAt":"2020-02-18T14:19:53Z","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 8:21 AM Jeff King <peff@peff.net> wrote:\n> Your second patch catches cases where the strbuf functions want to write\n> to the buffer. But we've always been pretty open about the fact that\n> strbuf.buf is a writeable C-style string. So something like this:\n> ...\n> would generate no compile-time warnings, but would invoke undefined\n> behavior (on my system it segfaults when run, but it could have even\n> more confusing outcomes).\nOh right, I didn't think about that. Ignorant of me to expect everyone to just\ncall the functions and not edit the buf directly.\n\n> If we want to pursue this direction, I think we'd do better to give each\n> strbuf a matching array. Something like:\n> ...\n> So I think there are interesting directions here, but there's a lot of\n> stuff to figure out.\nI think that got me a bit fired up now.\n\n> I notice you put GSoC in your subject line. If you're looking at this as\n> a microproject, IMHO this is _way_ more complicated and subtle than a\n> microproject should be. The goal there is to give something so easy that\n> you get to focus on getting your patches in and interacting with the\n> community. The scope I'd expect is more along the lines of compiling\n> with -Wwrite-strings and cleaning up some of the locations that\n> complain.\nI'm actually planning to keep on contributing to git, so I kind of\ndidn't want to\ndo something trivial. Despite the fact that I'm planning to apply to\ngit for GSoC,\nI'm mostly putting the [GSoC] so that reviewers would go easy on me :D. That\nsaid, I might actually do the -Wwrite-strings clean-up after this one\nis finished.\n\nThanks for the help, I guess I'll start editing it ASAP, then.\n\n- mo7sener\n"},{"id":"392014","messageId":"20200218203309.GA22630@coredump.intra.peff.net","threadId":"52831","inReplyTo":"CALH1-XoWmojFZir1JTU490GNHE1CETs4jbjswH316cAWqjF+Bg@mail.gmail.com","subject":"Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-18T20:33:09Z","receivedAt":"2020-02-18T20:33:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 18, 2020 at 04:19:38PM +0200, Robear Selwans wrote:\n\n> > I notice you put GSoC in your subject line. If you're looking at this as\n> > a microproject, IMHO this is _way_ more complicated and subtle than a\n> > microproject should be. The goal there is to give something so easy that\n> > you get to focus on getting your patches in and interacting with the\n> > community. The scope I'd expect is more along the lines of compiling\n> > with -Wwrite-strings and cleaning up some of the locations that\n> > complain.\n> I'm actually planning to keep on contributing to git, so I kind of\n> didn't want to\n> do something trivial. Despite the fact that I'm planning to apply to\n> git for GSoC,\n> I'm mostly putting the [GSoC] so that reviewers would go easy on me :D. That\n> said, I might actually do the -Wwrite-strings clean-up after this one\n> is finished.\n\nOK. If you want to go further, I certainly won't stop you. :)\n\n-Peff\n"},{"id":"392050","messageId":"467c035f-c7cd-01e1-e64c-2c915610de01@kdbg.org","threadId":"52831","inReplyTo":"20200218041805.10939-2-robear.selwans@outlook.com","subject":"Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2020-02-19T08:13:49Z","receivedAt":"2020-02-19T08:13:56Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 18.02.20 um 05:18 schrieb Robear Selwans:\n> A new function `STRBUF_INIT_CONST(const_str)` was added to allow for a\n> quick initialization of strbuf.\n> \n> Details:\n> Using `STRBUF_INIT_CONST(str)` creates a new struct of type `strbuf` and\n> initializes its `buf`, `len` and `alloc` as `str`, `strlen(str)` and\n> `0`, respectively.\n> \n> Use Case:\n> This is meant to be used to initialize strbufs with constant values and\n> thus, only allocating memory when needed.\n> \n> Usage Example:\n> ```\n> strbuf env_var = STRBUF_INIT_CONST(\"dev\");\n> ```\n> \n> This was added according to the issue opened at [https://github.com/gitgitgadget/git/issues/398]\n\nI am not a friend of this change at all. Why do so many functions and\nstrbuf instances have to pay a price (check for immutable string) for a\nfeature that they are not using?\n\nAs the macro is just intended for convenience, I suggest to implement it\nusing strbuf_addstr() under the hood. That is much less code churn, and\nthe price is paid only by the strbufs that actually use the feature.\n\n-- Hannes\n"},{"id":"392183","messageId":"113d4221-d7bd-cf0c-ebfc-1fc08442c303@web.de","threadId":"52831","inReplyTo":"467c035f-c7cd-01e1-e64c-2c915610de01@kdbg.org","subject":"Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-20T18:49:18Z","receivedAt":"2020-02-20T18:49:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.02.20 um 09:13 schrieb Johannes Sixt:\n> Am 18.02.20 um 05:18 schrieb Robear Selwans:\n>> A new function `STRBUF_INIT_CONST(const_str)` was added to allow for a\n>> quick initialization of strbuf.\n>>\n>> Details:\n>> Using `STRBUF_INIT_CONST(str)` creates a new struct of type `strbuf` and\n>> initializes its `buf`, `len` and `alloc` as `str`, `strlen(str)` and\n>> `0`, respectively.\n>>\n>> Use Case:\n>> This is meant to be used to initialize strbufs with constant values and\n>> thus, only allocating memory when needed.\n>>\n>> Usage Example:\n>> ```\n>> strbuf env_var = STRBUF_INIT_CONST(\"dev\");\n>> ```\n>>\n>> This was added according to the issue opened at [https://github.com/gitgitgadget/git/issues/398]\n>\n> I am not a friend of this change at all. Why do so many functions and\n> strbuf instances have to pay a price (check for immutable string) for a\n> feature that they are not using?\n>\n> As the macro is just intended for convenience, I suggest to implement it\n> using strbuf_addstr() under the hood. That is much less code churn, and\n> the price is paid only by the strbufs that actually use the feature.\n\nI was also wondering what the benefits of this change might be.  Saving\none line and thus increasing convenience slightly doesn't justify all\nthis added complexity.  Saving an allocation in the following sequence\nmight be worthwhile:\n\n\tstruct strbuf sb = STRBUF_INIT;\n\tstrbuf_addstr(&sb, \"foo\");\n\t/* Use sb without modifying it. */\n\tstrbuf_release(&sb); /* or leak it */\n\nI found two examples of this pattern in the code, one in range-diff.c in\nthe function show_range_diff(), and below is a patch for getting rid of\nthe second one.  Are there other reasons why we'd want that feature?\nCould you perhaps include a patch that makes use of it in this series,\nto highlight its benefits?\n\n-- >8 --\nSubject: [PATCH] commit-graph: use progress title directly\n\nmerge_commit_graphs() copies the (translated) progress message into a\nstrbuf and passes the copy to start_delayed_progress() at each loop\niteration.  The latter function takes a string pointer, so let's avoid\nthe detour and hand the string to it directly.  That's shorter, simpler\nand slightly more efficient.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n commit-graph.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 656dd647d5..f013a84e29 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1657,19 +1657,15 @@ static void merge_commit_graphs(struct write_commit_graph_context *ctx)\n {\n \tstruct commit_graph *g = ctx->r->objects->commit_graph;\n \tuint32_t current_graph_number = ctx->num_commit_graphs_before;\n-\tstruct strbuf progress_title = STRBUF_INIT;\n\n \twhile (g && current_graph_number >= ctx->num_commit_graphs_after) {\n \t\tcurrent_graph_number--;\n\n-\t\tif (ctx->report_progress) {\n-\t\t\tstrbuf_addstr(&progress_title, _(\"Merging commit-graph\"));\n-\t\t\tctx->progress = start_delayed_progress(progress_title.buf, 0);\n-\t\t}\n+\t\tif (ctx->report_progress)\n+\t\t\tctx->progress = start_delayed_progress(_(\"Merging commit-graph\"), 0);\n\n \t\tmerge_commit_graph(ctx, g);\n \t\tstop_progress(&ctx->progress);\n-\t\tstrbuf_release(&progress_title);\n\n \t\tg = g->base_graph;\n \t}\n--\n2.25.1\n"},{"id":"392251","messageId":"CALH1-XoDm26QMQDgA0zKmSMPnbDxJCOF6k2xyyeePL15APV68w@mail.gmail.com","threadId":"52831","inReplyTo":"113d4221-d7bd-cf0c-ebfc-1fc08442c303@web.de","subject":"Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf","fromName":"Robear Selwans","fromEmail":"rwagih.rw@gmail.com","sentAt":"2020-02-21T05:21:46Z","receivedAt":"2020-02-21T05:22:01Z","isPatch":true,"sender":{"key":"rwagih.rw@gmail.com","avatar":"https://gravatar.com/avatar/196a8e523e919aa337627b4427365961719a4b821499438fa0698ccfdc8dcf1e?d=mp&s=160"},"body":"On Thu, Feb 20, 2020 at 8:49 PM René Scharfe <l.s.r@web.de> wrote:\n>\n> Could you perhaps include a patch that makes use of it in this series,\n> to highlight its benefits?\n\nWell to begin with, I'm actually doing this in response to this issue\n[https://github.com/gitgitgadget/git/issues/398].\nThe issue was created because of the following mail thread, though.\n[https://public-inbox.org/git/20180601200146.114919-1-sbeller@google.com/]\nTo be honest, I'm not entirely sure about how making these changes\nwould help, as my experience is still quite limited. But from what\nI've read, I think the main\nuse-case would be using const `strbuf`s to avoid memory leaks when\ndealing with config strings.\n"},{"id":"392602","messageId":"a94a32f6-a650-4ec7-927e-032e22a23ffe@gmail.com","threadId":"52831","inReplyTo":"113d4221-d7bd-cf0c-ebfc-1fc08442c303@web.de","subject":"Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-02-27T06:50:00Z","receivedAt":"2020-02-27T06:50:04Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/20/2020 1:49 PM, René Scharfe wrote:\n> Am 19.02.20 um 09:13 schrieb Johannes Sixt:\n>> As the macro is just intended for convenience, I suggest to implement it\n>> using strbuf_addstr() under the hood. That is much less code churn, and\n>> the price is paid only by the strbufs that actually use the feature.\n> \n> I was also wondering what the benefits of this change might be.  Saving\n> one line and thus increasing convenience slightly doesn't justify all\n> this added complexity.  Saving an allocation in the following sequence\n> might be worthwhile:\n> \n> \tstruct strbuf sb = STRBUF_INIT;\n> \tstrbuf_addstr(&sb, \"foo\");\n> \t/* Use sb without modifying it. */\n> \tstrbuf_release(&sb); /* or leak it */\n> \n> I found two examples of this pattern in the code, one in range-diff.c in\n> the function show_range_diff(), and below is a patch for getting rid of\n> the second one.  Are there other reasons why we'd want that feature?\n> Could you perhaps include a patch that makes use of it in this series,\n> to highlight its benefits?\n> \n> -- >8 --\n> Subject: [PATCH] commit-graph: use progress title directly\n> \n> merge_commit_graphs() copies the (translated) progress message into a\n> strbuf and passes the copy to start_delayed_progress() at each loop\n> iteration.  The latter function takes a string pointer, so let's avoid\n> the detour and hand the string to it directly.  That's shorter, simpler\n> and slightly more efficient.\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  commit-graph.c | 8 ++------\n>  1 file changed, 2 insertions(+), 6 deletions(-)\n> \n> diff --git a/commit-graph.c b/commit-graph.c\n> index 656dd647d5..f013a84e29 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1657,19 +1657,15 @@ static void merge_commit_graphs(struct write_commit_graph_context *ctx)\n>  {\n>  \tstruct commit_graph *g = ctx->r->objects->commit_graph;\n>  \tuint32_t current_graph_number = ctx->num_commit_graphs_before;\n> -\tstruct strbuf progress_title = STRBUF_INIT;\n> \n>  \twhile (g && current_graph_number >= ctx->num_commit_graphs_after) {\n>  \t\tcurrent_graph_number--;\n> \n> -\t\tif (ctx->report_progress) {\n> -\t\t\tstrbuf_addstr(&progress_title, _(\"Merging commit-graph\"));\n> -\t\t\tctx->progress = start_delayed_progress(progress_title.buf, 0);\n> -\t\t}\n> +\t\tif (ctx->report_progress)\n> +\t\t\tctx->progress = start_delayed_progress(_(\"Merging commit-graph\"), 0);\n> \n>  \t\tmerge_commit_graph(ctx, g);\n>  \t\tstop_progress(&ctx->progress);\n> -\t\tstrbuf_release(&progress_title);\n> \n>  \t\tg = g->base_graph;\n>  \t}\n> --\n\nNot only is this a good change, it no longer leaks memory.\nThanks!\n\n-Stolee\n"},{"id":"392620","messageId":"ef831d8f-7667-3315-ab80-2cb13bb963a1@web.de","threadId":"52831","inReplyTo":"a94a32f6-a650-4ec7-927e-032e22a23ffe@gmail.com","subject":"Re: [GSoC][RFC][PATCH 1/2] STRBUF_INIT_CONST: a new way to initialize strbuf","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-27T15:55:08Z","receivedAt":"2020-02-27T15:55:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"\nAm 27.02.20 um 07:50 schrieb Derrick Stolee:\n> On 2/20/2020 1:49 PM, René Scharfe wrote:\n>> Subject: [PATCH] commit-graph: use progress title directly\n>>\n>> merge_commit_graphs() copies the (translated) progress message into a\n>> strbuf and passes the copy to start_delayed_progress() at each loop\n>> iteration.  The latter function takes a string pointer, so let's avoid\n>> the detour and hand the string to it directly.  That's shorter, simpler\n>> and slightly more efficient.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>>  commit-graph.c | 8 ++------\n>>  1 file changed, 2 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/commit-graph.c b/commit-graph.c\n>> index 656dd647d5..f013a84e29 100644\n>> --- a/commit-graph.c\n>> +++ b/commit-graph.c\n>> @@ -1657,19 +1657,15 @@ static void merge_commit_graphs(struct write_commit_graph_context *ctx)\n>>  {\n>>  \tstruct commit_graph *g = ctx->r->objects->commit_graph;\n>>  \tuint32_t current_graph_number = ctx->num_commit_graphs_before;\n>> -\tstruct strbuf progress_title = STRBUF_INIT;\n>>\n>>  \twhile (g && current_graph_number >= ctx->num_commit_graphs_after) {\n>>  \t\tcurrent_graph_number--;\n>>\n>> -\t\tif (ctx->report_progress) {\n>> -\t\t\tstrbuf_addstr(&progress_title, _(\"Merging commit-graph\"));\n>> -\t\t\tctx->progress = start_delayed_progress(progress_title.buf, 0);\n>> -\t\t}\n>> +\t\tif (ctx->report_progress)\n>> +\t\t\tctx->progress = start_delayed_progress(_(\"Merging commit-graph\"), 0);\n>>\n>>  \t\tmerge_commit_graph(ctx, g);\n>>  \t\tstop_progress(&ctx->progress);\n>> -\t\tstrbuf_release(&progress_title);\n>>\n>>  \t\tg = g->base_graph;\n>>  \t}\n>> --\n>\n> Not only is this a good change, it no longer leaks memory.\n> Thanks!\n\nstrbuf_release() frees the allocated memory, so I don't think the code\nwas leaking before.  (It would have with strbuf_reset()).\n\nRené\n"}]}