{"thread":{"id":"36897","subject":"[PATCH v3 0/2] add strbuf_set operations","startedAt":"2014-06-12T07:29:32Z","lastAt":"2014-06-14T04:49:06Z","messageCount":16,"participants":["Jeremiah Mahler","Thomas Braun","Eric Sunshine","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"243937","messageId":"cover.1402557437.git.jmmahler@gmail.com","threadId":"36897","inReplyTo":null,"subject":"[PATCH v3 0/2] add strbuf_set operations","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-12T07:29:32Z","receivedAt":"2014-06-12T07:29:32Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Addition of strbuf_set operations, version 3.\n\nIncludes suggestions from Eric Sunshine [1]:\n\n  - Revise log message to better argue why this patch is worthwhile.\n\n  - Avoid documentation redundancy: \"Setting buffer\", \"Replace buffer\".\n\n  - Remove unnecessary changes which didn't have a significant benefit\n\tto avoid unnecessary \"code churn\".  builtin/remote.c was the one\n\tfile which showed a significant benefit.  Others with negligible\n\tbenefits have been left as is.\n\nThe possible performance improvements using a strbuf_grow_to() operation\nas suggested by Michael Haggerty [2] has been left for a later patch.\n\n[1]: http://marc.info/?l=git&m=140247618416057&w=2\n\n[2]: http://marc.info/?l=git&m=140248834420244&w=2\n\nJeremiah Mahler (2):\n  add strbuf_set operations\n  builtin/remote: improve readability via strbuf_set()\n\n Documentation/technical/api-strbuf.txt | 18 ++++++++++\n builtin/remote.c                       | 63 +++++++++++++---------------------\n strbuf.c                               | 21 ++++++++++++\n strbuf.h                               | 13 +++++++\n 4 files changed, 75 insertions(+), 40 deletions(-)\n\n-- \n2.0.0\n"},{"id":"243938","messageId":"f4d043b7c1e00f9c967faff39244274fe40fd371.1402557437.git.jmmahler@gmail.com","threadId":"36897","inReplyTo":"cover.1402557437.git.jmmahler@gmail.com","subject":"[PATCH v3 1/2] add strbuf_set operations","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-12T07:29:33Z","receivedAt":"2014-06-12T07:29:33Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"A common use case with strubfs is to set the buffer to a new value.\nThis must be done in two steps: a reset followed by an add.\n\n  strbuf_reset(buf);\n  strbuf_add(buf, new_buf, len);\n\nIn cases where the buffer is being built up in steps, these operations\nmake sense and correctly convey what is being performed.\n\n  strbuf_reset(buf);\n  strbuf_add(buf, data1, len1);\n  strbuf_add(buf, data2, len2);\n  strbuf_add(buf, data3, len3);\n\nHowever, in other cases, it can be confusing and is not very concise.\n\n  strbuf_reset(buf);\n  strbuf_add(buf, default, len1);\n\n  if (cond1) {\n    strbuf_reset(buf);\n    strbuf_add(buf, data2, len2);\n  }\n\n  if (cond2) {\n    strbuf_reset(buf);\n    strbuf_add(buf, data3, len3);\n  }\n\nAdd strbuf_set operations so that it can be re-written in a clear and\nconcise way.\n\n  strbuf_set(buf, default len1);\n\n  if (cond1) {\n    strbuf_set(buf, data2, len2);\n  }\n\n  if (cond2) {\n    strbuf_set(buf, data3, len3);\n  }\n\nSigned-off-by: Jeremiah Mahler <jmmahler@gmail.com>\n---\n Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++\n strbuf.c                               | 21 +++++++++++++++++++++\n strbuf.h                               | 13 +++++++++++++\n 3 files changed, 52 insertions(+)\n\ndiff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt\nindex f9c06a7..ae9c9cc 100644\n--- a/Documentation/technical/api-strbuf.txt\n+++ b/Documentation/technical/api-strbuf.txt\n@@ -149,6 +149,24 @@ Functions\n \tthan zero if the first buffer is found, respectively, to be less than,\n \tto match, or be greater than the second buffer.\n \n+* Setting the buffer\n+\n+`strbuf_set`::\n+\n+\tReplace content with data of a given length.\n+\n+`strbuf_setstr`::\n+\n+\tReplace content with data from a NUL-terminated string.\n+\n+`strbuf_setf`::\n+\n+\tReplace content with a formatted string.\n+\n+`strbuf_setbuf`::\n+\n+\tReplace content with data from another buffer.\n+\n * Adding data to the buffer\n \n NOTE: All of the functions in this section will grow the buffer as necessary.\ndiff --git a/strbuf.c b/strbuf.c\nindex ac62982..9d64b00 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -189,6 +189,27 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,\n \tstrbuf_setlen(sb, sb->len + dlen - len);\n }\n \n+void strbuf_set(struct strbuf *sb, const void *data, size_t len)\n+{\n+\tstrbuf_reset(sb);\n+\tstrbuf_add(sb, data, len);\n+}\n+\n+void strbuf_setf(struct strbuf *sb, const char *fmt, ...)\n+{\n+\tva_list ap;\n+\tstrbuf_reset(sb);\n+\tva_start(ap, fmt);\n+\tstrbuf_vaddf(sb, fmt, ap);\n+\tva_end(ap);\n+}\n+\n+void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2)\n+{\n+\tstrbuf_reset(sb);\n+\tstrbuf_add(sb, sb2->buf, sb2->len);\n+}\n+\n void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)\n {\n \tstrbuf_splice(sb, pos, 0, data, len);\ndiff --git a/strbuf.h b/strbuf.h\nindex e9ad03e..5041c35 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -101,6 +101,19 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,\n  */\n extern void strbuf_list_free(struct strbuf **);\n \n+/*----- set buffer to data -----*/\n+extern void strbuf_set(struct strbuf *sb, const void *data, size_t len);\n+\n+static inline void strbuf_setstr(struct strbuf *sb, const char *s)\n+{\n+\tstrbuf_set(sb, s, strlen(s));\n+}\n+\n+__attribute__((format (printf,2,3)))\n+extern void strbuf_setf(struct strbuf *sb, const char *fmt, ...);\n+\n+extern void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2);\n+\n /*----- add data in your buffer -----*/\n static inline void strbuf_addch(struct strbuf *sb, int c)\n {\n-- \n2.0.0\n"},{"id":"243939","messageId":"82aec34d46552c590e952af1e17614b9606ea3ef.1402557437.git.jmmahler@gmail.com","threadId":"36897","inReplyTo":"cover.1402557437.git.jmmahler@gmail.com","subject":"[PATCH v3 2/2] builtin/remote: improve readability via strbuf_set()","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-12T07:29:34Z","receivedAt":"2014-06-12T07:29:34Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"builtin/remote.c has many cases where a strbuf was being set to a new\nvalue.  Improve its readability by using strbuf_set() operations instead\nof reset + add.\n\nSigned-off-by: Jeremiah Mahler <jmmahler@gmail.com>\n---\n builtin/remote.c | 63 +++++++++++++++++++++-----------------------------------\n 1 file changed, 23 insertions(+), 40 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex c9b67ff..94f03e2 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -184,17 +184,16 @@ static int add(int argc, const char **argv)\n \t\t\tremote->fetch_refspec_nr))\n \t\tdie(_(\"remote %s already exists.\"), name);\n \n-\tstrbuf_addf(&buf2, \"refs/heads/test:refs/remotes/%s/test\", name);\n+\tstrbuf_setf(&buf2, \"refs/heads/test:refs/remotes/%s/test\", name);\n \tif (!valid_fetch_refspec(buf2.buf))\n \t\tdie(_(\"'%s' is not a valid remote name\"), name);\n \n-\tstrbuf_addf(&buf, \"remote.%s.url\", name);\n+\tstrbuf_setf(&buf, \"remote.%s.url\", name);\n \tif (git_config_set(buf.buf, url))\n \t\treturn 1;\n \n \tif (!mirror || mirror & MIRROR_FETCH) {\n-\t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addf(&buf, \"remote.%s.fetch\", name);\n+\t\tstrbuf_setf(&buf, \"remote.%s.fetch\", name);\n \t\tif (track.nr == 0)\n \t\t\tstring_list_append(&track, \"*\");\n \t\tfor (i = 0; i < track.nr; i++) {\n@@ -205,15 +204,13 @@ static int add(int argc, const char **argv)\n \t}\n \n \tif (mirror & MIRROR_PUSH) {\n-\t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addf(&buf, \"remote.%s.mirror\", name);\n+\t\tstrbuf_setf(&buf, \"remote.%s.mirror\", name);\n \t\tif (git_config_set(buf.buf, \"true\"))\n \t\t\treturn 1;\n \t}\n \n \tif (fetch_tags != TAGS_DEFAULT) {\n-\t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addf(&buf, \"remote.%s.tagopt\", name);\n+\t\tstrbuf_setf(&buf, \"remote.%s.tagopt\", name);\n \t\tif (git_config_set(buf.buf,\n \t\t\tfetch_tags == TAGS_SET ? \"--tags\" : \"--no-tags\"))\n \t\t\treturn 1;\n@@ -223,11 +220,9 @@ static int add(int argc, const char **argv)\n \t\treturn 1;\n \n \tif (master) {\n-\t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addf(&buf, \"refs/remotes/%s/HEAD\", name);\n+\t\tstrbuf_setf(&buf, \"refs/remotes/%s/HEAD\", name);\n \n-\t\tstrbuf_reset(&buf2);\n-\t\tstrbuf_addf(&buf2, \"refs/remotes/%s/%s\", name, master);\n+\t\tstrbuf_setf(&buf2, \"refs/remotes/%s/%s\", name, master);\n \n \t\tif (create_symref(buf.buf, buf2.buf, \"remote add\"))\n \t\t\treturn error(_(\"Could not setup master '%s'\"), master);\n@@ -584,19 +579,17 @@ static int migrate_file(struct remote *remote)\n \tint i;\n \tconst char *path = NULL;\n \n-\tstrbuf_addf(&buf, \"remote.%s.url\", remote->name);\n+\tstrbuf_setf(&buf, \"remote.%s.url\", remote->name);\n \tfor (i = 0; i < remote->url_nr; i++)\n \t\tif (git_config_set_multivar(buf.buf, remote->url[i], \"^$\", 0))\n \t\t\treturn error(_(\"Could not append '%s' to '%s'\"),\n \t\t\t\t\tremote->url[i], buf.buf);\n-\tstrbuf_reset(&buf);\n-\tstrbuf_addf(&buf, \"remote.%s.push\", remote->name);\n+\tstrbuf_setf(&buf, \"remote.%s.push\", remote->name);\n \tfor (i = 0; i < remote->push_refspec_nr; i++)\n \t\tif (git_config_set_multivar(buf.buf, remote->push_refspec[i], \"^$\", 0))\n \t\t\treturn error(_(\"Could not append '%s' to '%s'\"),\n \t\t\t\t\tremote->push_refspec[i], buf.buf);\n-\tstrbuf_reset(&buf);\n-\tstrbuf_addf(&buf, \"remote.%s.fetch\", remote->name);\n+\tstrbuf_setf(&buf, \"remote.%s.fetch\", remote->name);\n \tfor (i = 0; i < remote->fetch_refspec_nr; i++)\n \t\tif (git_config_set_multivar(buf.buf, remote->fetch_refspec[i], \"^$\", 0))\n \t\t\treturn error(_(\"Could not append '%s' to '%s'\"),\n@@ -640,27 +633,24 @@ static int mv(int argc, const char **argv)\n \tif (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))\n \t\tdie(_(\"remote %s already exists.\"), rename.new);\n \n-\tstrbuf_addf(&buf, \"refs/heads/test:refs/remotes/%s/test\", rename.new);\n+\tstrbuf_setf(&buf, \"refs/heads/test:refs/remotes/%s/test\", rename.new);\n \tif (!valid_fetch_refspec(buf.buf))\n \t\tdie(_(\"'%s' is not a valid remote name\"), rename.new);\n \n-\tstrbuf_reset(&buf);\n-\tstrbuf_addf(&buf, \"remote.%s\", rename.old);\n-\tstrbuf_addf(&buf2, \"remote.%s\", rename.new);\n+\tstrbuf_setf(&buf, \"remote.%s\", rename.old);\n+\tstrbuf_setf(&buf2, \"remote.%s\", rename.new);\n \tif (git_config_rename_section(buf.buf, buf2.buf) < 1)\n \t\treturn error(_(\"Could not rename config section '%s' to '%s'\"),\n \t\t\t\tbuf.buf, buf2.buf);\n \n-\tstrbuf_reset(&buf);\n-\tstrbuf_addf(&buf, \"remote.%s.fetch\", rename.new);\n+\tstrbuf_setf(&buf, \"remote.%s.fetch\", rename.new);\n \tif (git_config_set_multivar(buf.buf, NULL, NULL, 1))\n \t\treturn error(_(\"Could not remove config section '%s'\"), buf.buf);\n-\tstrbuf_addf(&old_remote_context, \":refs/remotes/%s/\", rename.old);\n+\tstrbuf_setf(&old_remote_context, \":refs/remotes/%s/\", rename.old);\n \tfor (i = 0; i < oldremote->fetch_refspec_nr; i++) {\n \t\tchar *ptr;\n \n-\t\tstrbuf_reset(&buf2);\n-\t\tstrbuf_addstr(&buf2, oldremote->fetch_refspec[i]);\n+\t\tstrbuf_setstr(&buf2, oldremote->fetch_refspec[i]);\n \t\tptr = strstr(buf2.buf, old_remote_context.buf);\n \t\tif (ptr) {\n \t\t\trefspec_updated = 1;\n@@ -683,8 +673,7 @@ static int mv(int argc, const char **argv)\n \t\tstruct string_list_item *item = branch_list.items + i;\n \t\tstruct branch_info *info = item->util;\n \t\tif (info->remote_name && !strcmp(info->remote_name, rename.old)) {\n-\t\t\tstrbuf_reset(&buf);\n-\t\t\tstrbuf_addf(&buf, \"branch.%s.remote\", item->string);\n+\t\t\tstrbuf_setf(&buf, \"branch.%s.remote\", item->string);\n \t\t\tif (git_config_set(buf.buf, rename.new)) {\n \t\t\t\treturn error(_(\"Could not set '%s'\"), buf.buf);\n \t\t\t}\n@@ -715,12 +704,10 @@ static int mv(int argc, const char **argv)\n \n \t\tif (item->util)\n \t\t\tcontinue;\n-\t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addstr(&buf, item->string);\n+\t\tstrbuf_setstr(&buf, item->string);\n \t\tstrbuf_splice(&buf, strlen(\"refs/remotes/\"), strlen(rename.old),\n \t\t\t\trename.new, strlen(rename.new));\n-\t\tstrbuf_reset(&buf2);\n-\t\tstrbuf_addf(&buf2, \"remote: renamed %s to %s\",\n+\t\tstrbuf_setf(&buf2, \"remote: renamed %s to %s\",\n \t\t\t\titem->string, buf.buf);\n \t\tif (rename_ref(item->string, buf.buf, buf2.buf))\n \t\t\tdie(_(\"renaming '%s' failed\"), item->string);\n@@ -730,16 +717,13 @@ static int mv(int argc, const char **argv)\n \n \t\tif (!item->util)\n \t\t\tcontinue;\n-\t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addstr(&buf, item->string);\n+\t\tstrbuf_setstr(&buf, item->string);\n \t\tstrbuf_splice(&buf, strlen(\"refs/remotes/\"), strlen(rename.old),\n \t\t\t\trename.new, strlen(rename.new));\n-\t\tstrbuf_reset(&buf2);\n-\t\tstrbuf_addstr(&buf2, item->util);\n+\t\tstrbuf_setstr(&buf2, item->util);\n \t\tstrbuf_splice(&buf2, strlen(\"refs/remotes/\"), strlen(rename.old),\n \t\t\t\trename.new, strlen(rename.new));\n-\t\tstrbuf_reset(&buf3);\n-\t\tstrbuf_addf(&buf3, \"remote: renamed %s to %s\",\n+\t\tstrbuf_setf(&buf3, \"remote: renamed %s to %s\",\n \t\t\t\titem->string, buf.buf);\n \t\tif (create_symref(buf.buf, buf2.buf, buf3.buf))\n \t\t\tdie(_(\"creating '%s' failed\"), buf.buf);\n@@ -804,8 +788,7 @@ static int rm(int argc, const char **argv)\n \t\tif (info->remote_name && !strcmp(info->remote_name, remote->name)) {\n \t\t\tconst char *keys[] = { \"remote\", \"merge\", NULL }, **k;\n \t\t\tfor (k = keys; *k; k++) {\n-\t\t\t\tstrbuf_reset(&buf);\n-\t\t\t\tstrbuf_addf(&buf, \"branch.%s.%s\",\n+\t\t\t\tstrbuf_setf(&buf, \"branch.%s.%s\",\n \t\t\t\t\t\titem->string, *k);\n \t\t\t\tif (git_config_set(buf.buf, NULL)) {\n \t\t\t\t\tstrbuf_release(&buf);\n-- \n2.0.0\n"},{"id":"243940","messageId":"539960B8.1080709@virtuell-zuhause.de","threadId":"36897","inReplyTo":"f4d043b7c1e00f9c967faff39244274fe40fd371.1402557437.git.jmmahler@gmail.com","subject":"Re: [PATCH v3 1/2] add strbuf_set operations","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2014-06-12T08:11:36Z","receivedAt":"2014-06-12T08:11:36Z","isPatch":true,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"Am 12.06.2014 09:29, schrieb Jeremiah Mahler:\n> A common use case with strubfs is to set the buffer to a new value.\n> This must be done in two steps: a reset followed by an add.\n> \n>   strbuf_reset(buf);\n>   strbuf_add(buf, new_buf, len);\n> \n> In cases where the buffer is being built up in steps, these operations\n> make sense and correctly convey what is being performed.\n> \n>   strbuf_reset(buf);\n>   strbuf_add(buf, data1, len1);\n>   strbuf_add(buf, data2, len2);\n>   strbuf_add(buf, data3, len3);\n> \n> However, in other cases, it can be confusing and is not very concise.\n> \n>   strbuf_reset(buf);\n>   strbuf_add(buf, default, len1);\n> \n>   if (cond1) {\n>     strbuf_reset(buf);\n>     strbuf_add(buf, data2, len2);\n>   }\n> \n>   if (cond2) {\n>     strbuf_reset(buf);\n>     strbuf_add(buf, data3, len3);\n>   }\n> \n> Add strbuf_set operations so that it can be re-written in a clear and\n> concise way.\n> \n>   strbuf_set(buf, default len1);\nvery minor nit: missing comma between default and len1.\n"},{"id":"243941","messageId":"CAPig+cR2x_nzRPzcOx+_8-+JZB_TegRaPrZUWXTOXBH27MnkMw@mail.gmail.com","threadId":"36897","inReplyTo":"82aec34d46552c590e952af1e17614b9606ea3ef.1402557437.git.jmmahler@gmail.com","subject":"Re: [PATCH v3 2/2] builtin/remote: improve readability via strbuf_set()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-12T08:19:44Z","receivedAt":"2014-06-12T08:19:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jun 12, 2014 at 3:29 AM, Jeremiah Mahler <jmmahler@gmail.com> wrote:\n> builtin/remote.c has many cases where a strbuf was being set to a new\n> value.  Improve its readability by using strbuf_set() operations instead\n> of reset + add.\n>\n> Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>\n> ---\n>  builtin/remote.c | 63 +++++++++++++++++++++-----------------------------------\n>  1 file changed, 23 insertions(+), 40 deletions(-)\n>\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index c9b67ff..94f03e2 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -184,17 +184,16 @@ static int add(int argc, const char **argv)\n>                         remote->fetch_refspec_nr))\n>                 die(_(\"remote %s already exists.\"), name);\n>\n> -       strbuf_addf(&buf2, \"refs/heads/test:refs/remotes/%s/test\", name);\n> +       strbuf_setf(&buf2, \"refs/heads/test:refs/remotes/%s/test\", name);\n>         if (!valid_fetch_refspec(buf2.buf))\n>                 die(_(\"'%s' is not a valid remote name\"), name);\n>\n> -       strbuf_addf(&buf, \"remote.%s.url\", name);\n> +       strbuf_setf(&buf, \"remote.%s.url\", name);\n>         if (git_config_set(buf.buf, url))\n>                 return 1;\n>\n>         if (!mirror || mirror & MIRROR_FETCH) {\n> -               strbuf_reset(&buf);\n> -               strbuf_addf(&buf, \"remote.%s.fetch\", name);\n> +               strbuf_setf(&buf, \"remote.%s.fetch\", name);\n\nThis hunk nicely illustrates an important maintenance benefit of\nstrbuf_set() not mentioned as justification in patch 1/2. The original\nprogrammer knew that 'buf' and 'buf2' had not yet been used, even\nthough their declarations are quite distant from this bit of code, so\nhe omitted the call to strbuf_reset(). Although the omission is valid,\nit also is potentially dangerous and a maintenance burden. The person\nwho next inserts code which uses 'buf' or 'buf2' between here and the\ndeclarations must remember or know to add strbuf_reset()s here, but\nthat's easily overlooked. strbuf_set() eliminates that burden.\n\nThis is the strongest argument I've thus far seen in favor of\nstrbuf_set() (though I'm not, in any way, suggesting you reroll the\nseries merely to mention this).\n\n>                 if (track.nr == 0)\n>                         string_list_append(&track, \"*\");\n>                 for (i = 0; i < track.nr; i++) {\n> @@ -205,15 +204,13 @@ static int add(int argc, const char **argv)\n>         }\n>\n>         if (mirror & MIRROR_PUSH) {\n> -               strbuf_reset(&buf);\n> -               strbuf_addf(&buf, \"remote.%s.mirror\", name);\n> +               strbuf_setf(&buf, \"remote.%s.mirror\", name);\n>                 if (git_config_set(buf.buf, \"true\"))\n>                         return 1;\n>         }\n>\n>         if (fetch_tags != TAGS_DEFAULT) {\n> -               strbuf_reset(&buf);\n> -               strbuf_addf(&buf, \"remote.%s.tagopt\", name);\n> +               strbuf_setf(&buf, \"remote.%s.tagopt\", name);\n>                 if (git_config_set(buf.buf,\n>                         fetch_tags == TAGS_SET ? \"--tags\" : \"--no-tags\"))\n>                         return 1;\n> @@ -223,11 +220,9 @@ static int add(int argc, const char **argv)\n>                 return 1;\n>\n>         if (master) {\n> -               strbuf_reset(&buf);\n> -               strbuf_addf(&buf, \"refs/remotes/%s/HEAD\", name);\n> +               strbuf_setf(&buf, \"refs/remotes/%s/HEAD\", name);\n>\n> -               strbuf_reset(&buf2);\n> -               strbuf_addf(&buf2, \"refs/remotes/%s/%s\", name, master);\n> +               strbuf_setf(&buf2, \"refs/remotes/%s/%s\", name, master);\n>\n>                 if (create_symref(buf.buf, buf2.buf, \"remote add\"))\n>                         return error(_(\"Could not setup master '%s'\"), master);\n> @@ -584,19 +579,17 @@ static int migrate_file(struct remote *remote)\n>         int i;\n>         const char *path = NULL;\n>\n> -       strbuf_addf(&buf, \"remote.%s.url\", remote->name);\n> +       strbuf_setf(&buf, \"remote.%s.url\", remote->name);\n>         for (i = 0; i < remote->url_nr; i++)\n>                 if (git_config_set_multivar(buf.buf, remote->url[i], \"^$\", 0))\n>                         return error(_(\"Could not append '%s' to '%s'\"),\n>                                         remote->url[i], buf.buf);\n> -       strbuf_reset(&buf);\n> -       strbuf_addf(&buf, \"remote.%s.push\", remote->name);\n> +       strbuf_setf(&buf, \"remote.%s.push\", remote->name);\n>         for (i = 0; i < remote->push_refspec_nr; i++)\n>                 if (git_config_set_multivar(buf.buf, remote->push_refspec[i], \"^$\", 0))\n>                         return error(_(\"Could not append '%s' to '%s'\"),\n>                                         remote->push_refspec[i], buf.buf);\n> -       strbuf_reset(&buf);\n> -       strbuf_addf(&buf, \"remote.%s.fetch\", remote->name);\n> +       strbuf_setf(&buf, \"remote.%s.fetch\", remote->name);\n>         for (i = 0; i < remote->fetch_refspec_nr; i++)\n>                 if (git_config_set_multivar(buf.buf, remote->fetch_refspec[i], \"^$\", 0))\n>                         return error(_(\"Could not append '%s' to '%s'\"),\n> @@ -640,27 +633,24 @@ static int mv(int argc, const char **argv)\n>         if (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))\n>                 die(_(\"remote %s already exists.\"), rename.new);\n>\n> -       strbuf_addf(&buf, \"refs/heads/test:refs/remotes/%s/test\", rename.new);\n> +       strbuf_setf(&buf, \"refs/heads/test:refs/remotes/%s/test\", rename.new);\n>         if (!valid_fetch_refspec(buf.buf))\n>                 die(_(\"'%s' is not a valid remote name\"), rename.new);\n>\n> -       strbuf_reset(&buf);\n> -       strbuf_addf(&buf, \"remote.%s\", rename.old);\n> -       strbuf_addf(&buf2, \"remote.%s\", rename.new);\n> +       strbuf_setf(&buf, \"remote.%s\", rename.old);\n> +       strbuf_setf(&buf2, \"remote.%s\", rename.new);\n>         if (git_config_rename_section(buf.buf, buf2.buf) < 1)\n>                 return error(_(\"Could not rename config section '%s' to '%s'\"),\n>                                 buf.buf, buf2.buf);\n>\n> -       strbuf_reset(&buf);\n> -       strbuf_addf(&buf, \"remote.%s.fetch\", rename.new);\n> +       strbuf_setf(&buf, \"remote.%s.fetch\", rename.new);\n>         if (git_config_set_multivar(buf.buf, NULL, NULL, 1))\n>                 return error(_(\"Could not remove config section '%s'\"), buf.buf);\n> -       strbuf_addf(&old_remote_context, \":refs/remotes/%s/\", rename.old);\n> +       strbuf_setf(&old_remote_context, \":refs/remotes/%s/\", rename.old);\n>         for (i = 0; i < oldremote->fetch_refspec_nr; i++) {\n>                 char *ptr;\n>\n> -               strbuf_reset(&buf2);\n> -               strbuf_addstr(&buf2, oldremote->fetch_refspec[i]);\n> +               strbuf_setstr(&buf2, oldremote->fetch_refspec[i]);\n>                 ptr = strstr(buf2.buf, old_remote_context.buf);\n>                 if (ptr) {\n>                         refspec_updated = 1;\n> @@ -683,8 +673,7 @@ static int mv(int argc, const char **argv)\n>                 struct string_list_item *item = branch_list.items + i;\n>                 struct branch_info *info = item->util;\n>                 if (info->remote_name && !strcmp(info->remote_name, rename.old)) {\n> -                       strbuf_reset(&buf);\n> -                       strbuf_addf(&buf, \"branch.%s.remote\", item->string);\n> +                       strbuf_setf(&buf, \"branch.%s.remote\", item->string);\n>                         if (git_config_set(buf.buf, rename.new)) {\n>                                 return error(_(\"Could not set '%s'\"), buf.buf);\n>                         }\n> @@ -715,12 +704,10 @@ static int mv(int argc, const char **argv)\n>\n>                 if (item->util)\n>                         continue;\n> -               strbuf_reset(&buf);\n> -               strbuf_addstr(&buf, item->string);\n> +               strbuf_setstr(&buf, item->string);\n>                 strbuf_splice(&buf, strlen(\"refs/remotes/\"), strlen(rename.old),\n>                                 rename.new, strlen(rename.new));\n> -               strbuf_reset(&buf2);\n> -               strbuf_addf(&buf2, \"remote: renamed %s to %s\",\n> +               strbuf_setf(&buf2, \"remote: renamed %s to %s\",\n>                                 item->string, buf.buf);\n>                 if (rename_ref(item->string, buf.buf, buf2.buf))\n>                         die(_(\"renaming '%s' failed\"), item->string);\n> @@ -730,16 +717,13 @@ static int mv(int argc, const char **argv)\n>\n>                 if (!item->util)\n>                         continue;\n> -               strbuf_reset(&buf);\n> -               strbuf_addstr(&buf, item->string);\n> +               strbuf_setstr(&buf, item->string);\n>                 strbuf_splice(&buf, strlen(\"refs/remotes/\"), strlen(rename.old),\n>                                 rename.new, strlen(rename.new));\n> -               strbuf_reset(&buf2);\n> -               strbuf_addstr(&buf2, item->util);\n> +               strbuf_setstr(&buf2, item->util);\n>                 strbuf_splice(&buf2, strlen(\"refs/remotes/\"), strlen(rename.old),\n>                                 rename.new, strlen(rename.new));\n> -               strbuf_reset(&buf3);\n> -               strbuf_addf(&buf3, \"remote: renamed %s to %s\",\n> +               strbuf_setf(&buf3, \"remote: renamed %s to %s\",\n>                                 item->string, buf.buf);\n>                 if (create_symref(buf.buf, buf2.buf, buf3.buf))\n>                         die(_(\"creating '%s' failed\"), buf.buf);\n> @@ -804,8 +788,7 @@ static int rm(int argc, const char **argv)\n>                 if (info->remote_name && !strcmp(info->remote_name, remote->name)) {\n>                         const char *keys[] = { \"remote\", \"merge\", NULL }, **k;\n>                         for (k = keys; *k; k++) {\n> -                               strbuf_reset(&buf);\n> -                               strbuf_addf(&buf, \"branch.%s.%s\",\n> +                               strbuf_setf(&buf, \"branch.%s.%s\",\n>                                                 item->string, *k);\n>                                 if (git_config_set(buf.buf, NULL)) {\n>                                         strbuf_release(&buf);\n> --\n> 2.0.0\n>\n"},{"id":"243942","messageId":"20140612082218.GA5419@hudson.localdomain","threadId":"36897","inReplyTo":"539960B8.1080709@virtuell-zuhause.de","subject":"Re: [PATCH v3 1/2] add strbuf_set operations","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-12T08:22:18Z","receivedAt":"2014-06-12T08:22:18Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Thomas,\n\nOn Thu, Jun 12, 2014 at 10:11:36AM +0200, Thomas Braun wrote:\n> Am 12.06.2014 09:29, schrieb Jeremiah Mahler:\n> > A common use case with strubfs is to set the buffer to a new value.\n> > This must be done in two steps: a reset followed by an add.\n> > \n> >   strbuf_reset(buf);\n> >   strbuf_add(buf, new_buf, len);\n> > \n> > In cases where the buffer is being built up in steps, these operations\n> > make sense and correctly convey what is being performed.\n> > \n> >   strbuf_reset(buf);\n> >   strbuf_add(buf, data1, len1);\n> >   strbuf_add(buf, data2, len2);\n> >   strbuf_add(buf, data3, len3);\n> > \n> > However, in other cases, it can be confusing and is not very concise.\n> > \n> >   strbuf_reset(buf);\n> >   strbuf_add(buf, default, len1);\n> > \n> >   if (cond1) {\n> >     strbuf_reset(buf);\n> >     strbuf_add(buf, data2, len2);\n> >   }\n> > \n> >   if (cond2) {\n> >     strbuf_reset(buf);\n> >     strbuf_add(buf, data3, len3);\n> >   }\n> > \n> > Add strbuf_set operations so that it can be re-written in a clear and\n> > concise way.\n> > \n> >   strbuf_set(buf, default len1);\n> very minor nit: missing comma between default and len1.\n\nI can't believe I missed that.  Good catch ;-)\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"244016","messageId":"xmqqr42u55dq.fsf@gitster.dls.corp.google.com","threadId":"36897","inReplyTo":"f4d043b7c1e00f9c967faff39244274fe40fd371.1402557437.git.jmmahler@gmail.com","subject":"Re: [PATCH v3 1/2] add strbuf_set operations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-12T18:50:41Z","receivedAt":"2014-06-12T18:50:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeremiah Mahler <jmmahler@gmail.com> writes:\n\n> A common use case with strubfs is to set the buffer to a new value.\n> This must be done in two steps: a reset followed by an add.\n>\n>   strbuf_reset(buf);\n>   strbuf_add(buf, new_buf, len);\n>\n> In cases where the buffer is being built up in steps, these operations\n> make sense and correctly convey what is being performed.\n>\n>   strbuf_reset(buf);\n>   strbuf_add(buf, data1, len1);\n>   strbuf_add(buf, data2, len2);\n>   strbuf_add(buf, data3, len3);\n>\n> However, in other cases, it can be confusing and is not very concise.\n>\n>   strbuf_reset(buf);\n>   strbuf_add(buf, default, len1);\n>\n>   if (cond1) {\n>     strbuf_reset(buf);\n>     strbuf_add(buf, data2, len2);\n>   }\n>\n>   if (cond2) {\n>     strbuf_reset(buf);\n>     strbuf_add(buf, data3, len3);\n>   }\n>\n> Add strbuf_set operations so that it can be re-written in a clear and\n> concise way.\n>\n>   strbuf_set(buf, default len1);\n>\n>   if (cond1) {\n>     strbuf_set(buf, data2, len2);\n>   }\n>\n>   if (cond2) {\n>     strbuf_set(buf, data3, len3);\n>   }\n\nOr even more concisely without making unnecessary internal calls to\nstrbuf_reset():\n\n\tstrbuf_reset(buf);\n        if (cond2)\n        \tstrbuf_add(buf, data3, len3);\n\telse if (cond1)\n        \tstrbuf_add(buf, data2, len2);\n\telse\n        \tstrbuf_add(buf, default, len2);\n\n;-)\n\nI am on the fence.\n\nI have this suspicion that the addition of strbuf_set() would *only*\nhelp when the original written with reset-and-then-add sequence was\nsuboptimal to begin with, and it helps *only* how the code reads,\nwithout correcting the fact that it is still doing unnecessary\n\"first set to a value to be discarded and then reset to set the\nright value\", sweeping the issue under the rug.\n\nRepeated reset-and-then-add on the same strbuf used to be something\nthat may indicate that the code is doing unnecessary work.  Now,\nrepeated uses of strbuf_set on the same strbuf replaced that pattern\nto be watched for to spot wasteful code paths.\n\nI dunno...\n\n> Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>\n> ---\n>  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++\n>  strbuf.c                               | 21 +++++++++++++++++++++\n>  strbuf.h                               | 13 +++++++++++++\n>  3 files changed, 52 insertions(+)\n>\n> diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt\n> index f9c06a7..ae9c9cc 100644\n> --- a/Documentation/technical/api-strbuf.txt\n> +++ b/Documentation/technical/api-strbuf.txt\n> @@ -149,6 +149,24 @@ Functions\n>  \tthan zero if the first buffer is found, respectively, to be less than,\n>  \tto match, or be greater than the second buffer.\n>  \n> +* Setting the buffer\n> +\n> +`strbuf_set`::\n> +\n> +\tReplace content with data of a given length.\n> +\n> +`strbuf_setstr`::\n> +\n> +\tReplace content with data from a NUL-terminated string.\n> +\n> +`strbuf_setf`::\n> +\n> +\tReplace content with a formatted string.\n> +\n> +`strbuf_setbuf`::\n> +\n> +\tReplace content with data from another buffer.\n> +\n>  * Adding data to the buffer\n>  \n>  NOTE: All of the functions in this section will grow the buffer as necessary.\n> diff --git a/strbuf.c b/strbuf.c\n> index ac62982..9d64b00 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -189,6 +189,27 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,\n>  \tstrbuf_setlen(sb, sb->len + dlen - len);\n>  }\n>  \n> +void strbuf_set(struct strbuf *sb, const void *data, size_t len)\n> +{\n> +\tstrbuf_reset(sb);\n> +\tstrbuf_add(sb, data, len);\n> +}\n> +\n> +void strbuf_setf(struct strbuf *sb, const char *fmt, ...)\n> +{\n> +\tva_list ap;\n> +\tstrbuf_reset(sb);\n> +\tva_start(ap, fmt);\n> +\tstrbuf_vaddf(sb, fmt, ap);\n> +\tva_end(ap);\n> +}\n> +\n> +void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2)\n> +{\n> +\tstrbuf_reset(sb);\n> +\tstrbuf_add(sb, sb2->buf, sb2->len);\n> +}\n> +\n>  void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)\n>  {\n>  \tstrbuf_splice(sb, pos, 0, data, len);\n> diff --git a/strbuf.h b/strbuf.h\n> index e9ad03e..5041c35 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -101,6 +101,19 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,\n>   */\n>  extern void strbuf_list_free(struct strbuf **);\n>  \n> +/*----- set buffer to data -----*/\n> +extern void strbuf_set(struct strbuf *sb, const void *data, size_t len);\n> +\n> +static inline void strbuf_setstr(struct strbuf *sb, const char *s)\n> +{\n> +\tstrbuf_set(sb, s, strlen(s));\n> +}\n> +\n> +__attribute__((format (printf,2,3)))\n> +extern void strbuf_setf(struct strbuf *sb, const char *fmt, ...);\n> +\n> +extern void strbuf_setbuf(struct strbuf *sb, const struct strbuf *sb2);\n> +\n>  /*----- add data in your buffer -----*/\n>  static inline void strbuf_addch(struct strbuf *sb, int c)\n>  {\n"},{"id":"244017","messageId":"xmqqmwdi55co.fsf@gitster.dls.corp.google.com","threadId":"36897","inReplyTo":"20140612082218.GA5419@hudson.localdomain","subject":"Re: [PATCH v3 1/2] add strbuf_set operations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-12T18:51:19Z","receivedAt":"2014-06-12T18:51:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeremiah Mahler <jmmahler@gmail.com> writes:\n\n> Thomas,\n>\n> On Thu, Jun 12, 2014 at 10:11:36AM +0200, Thomas Braun wrote:\n>> Am 12.06.2014 09:29, schrieb Jeremiah Mahler:\n>> > A common use case with strubfs is to set the buffer to a new value.\n\nstrubfs???\n\n>> > This must be done in two steps: a reset followed by an add.\n>> > \n>> >   strbuf_reset(buf);\n>> >   strbuf_add(buf, new_buf, len);\n>> > \n>> > In cases where the buffer is being built up in steps, these operations\n>> > make sense and correctly convey what is being performed.\n>> > \n>> >   strbuf_reset(buf);\n>> >   strbuf_add(buf, data1, len1);\n>> >   strbuf_add(buf, data2, len2);\n>> >   strbuf_add(buf, data3, len3);\n>> > \n>> > However, in other cases, it can be confusing and is not very concise.\n>> > \n>> >   strbuf_reset(buf);\n>> >   strbuf_add(buf, default, len1);\n>> > \n>> >   if (cond1) {\n>> >     strbuf_reset(buf);\n>> >     strbuf_add(buf, data2, len2);\n>> >   }\n>> > \n>> >   if (cond2) {\n>> >     strbuf_reset(buf);\n>> >     strbuf_add(buf, data3, len3);\n>> >   }\n>> > \n>> > Add strbuf_set operations so that it can be re-written in a clear and\n>> > concise way.\n>> > \n>> >   strbuf_set(buf, default len1);\n>> very minor nit: missing comma between default and len1.\n>\n> I can't believe I missed that.  Good catch ;-)\n"},{"id":"244022","messageId":"20140612193144.GA17077@hudson.localdomain","threadId":"36897","inReplyTo":"xmqqr42u55dq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 1/2] add strbuf_set operations","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-12T19:31:44Z","receivedAt":"2014-06-12T19:31:44Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Junio,\n\nComments below...\n\nOn Thu, Jun 12, 2014 at 11:50:41AM -0700, Junio C Hamano wrote:\n> Jeremiah Mahler <jmmahler@gmail.com> writes:\n> \n> > A common use case with strubfs is to set the buffer to a new value.\n> > This must be done in two steps: a reset followed by an add.\n> >\n> >   strbuf_reset(buf);\n> >   strbuf_add(buf, new_buf, len);\n> >\n> > In cases where the buffer is being built up in steps, these operations\n> > make sense and correctly convey what is being performed.\n> >\n> >   strbuf_reset(buf);\n> >   strbuf_add(buf, data1, len1);\n> >   strbuf_add(buf, data2, len2);\n> >   strbuf_add(buf, data3, len3);\n> >\n> > However, in other cases, it can be confusing and is not very concise.\n> >\n> >   strbuf_reset(buf);\n> >   strbuf_add(buf, default, len1);\n> >\n> >   if (cond1) {\n> >     strbuf_reset(buf);\n> >     strbuf_add(buf, data2, len2);\n> >   }\n> >\n> >   if (cond2) {\n> >     strbuf_reset(buf);\n> >     strbuf_add(buf, data3, len3);\n> >   }\n> >\n> > Add strbuf_set operations so that it can be re-written in a clear and\n> > concise way.\n> >\n> >   strbuf_set(buf, default len1);\n> >\n> >   if (cond1) {\n> >     strbuf_set(buf, data2, len2);\n> >   }\n> >\n> >   if (cond2) {\n> >     strbuf_set(buf, data3, len3);\n> >   }\n> \n> Or even more concisely without making unnecessary internal calls to\n> strbuf_reset():\n> \n> \tstrbuf_reset(buf);\n>         if (cond2)\n>         \tstrbuf_add(buf, data3, len3);\n> \telse if (cond1)\n>         \tstrbuf_add(buf, data2, len2);\n> \telse\n>         \tstrbuf_add(buf, default, len2);\n> \n> ;-)\n\n_add would work in your example.  strbuf_set would also work and it\nwould perform the same number of operations (one set and one add).\n\nHowever your example is different from mine in which cond1 and cond2 are\nnot necessarily mutually exclusive.\n\n> \n> I am on the fence.\n> \n> I have this suspicion that the addition of strbuf_set() would *only*\n> help when the original written with reset-and-then-add sequence was\n> suboptimal to begin with, and it helps *only* how the code reads,\n> without correcting the fact that it is still doing unnecessary\n> \"first set to a value to be discarded and then reset to set the\n> right value\", sweeping the issue under the rug.\n> \nIt is certainly possible that builtin/remote.c (PATCH 2/2) saw the most\nbenefit from this operation because it is so badly designed.  But this\nseems unlikely given the review process around here ;-)\n\nThe one case where it would be doing extra work is when a strbuf_set\nreplaces a strbuf_add that didn't previously have a strbuf_reset.\nstrbuf_set is not appropriate for all cases, as I mentioned in the\npatch, but in some cases I think it makes it more readable.  And in this\ncase it would be doing a reset on an empty strbuf.  Is avoiding this\nexpense worth the reduction in readability?\n\nAlso, as Eric Sunshine pointed out, being able to easily re-order\noperations can make the code easier to maintain.\n\n> Repeated reset-and-then-add on the same strbuf used to be something\n> that may indicate that the code is doing unnecessary work.  Now,\n> repeated uses of strbuf_set on the same strbuf replaced that pattern\n> to be watched for to spot wasteful code paths.\n> \nIf a reset followed by and add was a rare occurrence I would tend to\nagree more.\n\n> I dunno...\n> \n> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>\n> > ---\n> >  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++\n> >  strbuf.c                               | 21 +++++++++++++++++++++\n> >  strbuf.h                               | 13 +++++++++++++\n> >  3 files changed, 52 insertions(+)\n> >\n> > diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt\n> > index f9c06a7..ae9c9cc 100644\n> > --- a/Documentation/technical/api-strbuf.txt\n> > +++ b/Documentation/technical/api-strbuf.txt\n> > @@ -149,6 +149,24 @@ Functions\n> >  \tthan zero if the first buffer is found, respectively, to be less than,\n> >  \tto match, or be greater than the second buffer.\n> >  /*----- add data in your buffer -----*/\n...\n> >  static inline void strbuf_addch(struct strbuf *sb, int c)\n> >  {\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"244023","messageId":"20140612193642.GB17077@hudson.localdomain","threadId":"36897","inReplyTo":"xmqqmwdi55co.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 1/2] add strbuf_set operations","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-12T19:36:42Z","receivedAt":"2014-06-12T19:36:42Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Junio,\n\nOn Thu, Jun 12, 2014 at 11:51:19AM -0700, Junio C Hamano wrote:\n> Jeremiah Mahler <jmmahler@gmail.com> writes:\n> \n> > Thomas,\n> >\n> > On Thu, Jun 12, 2014 at 10:11:36AM +0200, Thomas Braun wrote:\n> >> Am 12.06.2014 09:29, schrieb Jeremiah Mahler:\n> >> > A common use case with strubfs is to set the buffer to a new value.\n> \n> strubfs???\n> \nI was trying to make it plural.  Perhaps strbuf's?\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"244034","messageId":"CAPig+cT_StMkTgEd-RVpm=4e_A23zj+V5k83PhMtaN=tr4GBzA@mail.gmail.com","threadId":"36897","inReplyTo":"20140612193642.GB17077@hudson.localdomain","subject":"Re: [PATCH v3 1/2] add strbuf_set operations","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-12T21:18:29Z","receivedAt":"2014-06-12T21:18:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jun 12, 2014 at 3:36 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:\n> On Thu, Jun 12, 2014 at 11:51:19AM -0700, Junio C Hamano wrote:\n>> Jeremiah Mahler <jmmahler@gmail.com> writes:\n>>\n>> > Thomas,\n>> >\n>> > On Thu, Jun 12, 2014 at 10:11:36AM +0200, Thomas Braun wrote:\n>> >> Am 12.06.2014 09:29, schrieb Jeremiah Mahler:\n>> >> > A common use case with strubfs is to set the buffer to a new value.\n>>\n>> strubfs???\n>>\n> I was trying to make it plural.  Perhaps strbuf's?\n\nJunio was pointing out your misspelling, not your intention to pluralize.\n"},{"id":"244035","messageId":"CAPig+cTVLJQOsW7H4Ht2NNYkeiMb=EWT7BG3sNu0wNsTQ=oZNA@mail.gmail.com","threadId":"36897","inReplyTo":"20140612193144.GA17077@hudson.localdomain","subject":"Re: [PATCH v3 1/2] add strbuf_set operations","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-12T21:48:52Z","receivedAt":"2014-06-12T21:48:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jun 12, 2014 at 3:31 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:\n> On Thu, Jun 12, 2014 at 11:50:41AM -0700, Junio C Hamano wrote:\n>> I am on the fence.\n>>\n>> I have this suspicion that the addition of strbuf_set() would *only*\n>> help when the original written with reset-and-then-add sequence was\n>> suboptimal to begin with, and it helps *only* how the code reads,\n>> without correcting the fact that it is still doing unnecessary\n>> \"first set to a value to be discarded and then reset to set the\n>> right value\", sweeping the issue under the rug.\n>>\n> It is certainly possible that builtin/remote.c (PATCH 2/2) saw the most\n> benefit from this operation because it is so badly designed.  But this\n> seems unlikely given the review process around here ;-)\n>\n> The one case where it would be doing extra work is when a strbuf_set\n> replaces a strbuf_add that didn't previously have a strbuf_reset.\n> strbuf_set is not appropriate for all cases, as I mentioned in the\n> patch, but in some cases I think it makes it more readable.  And in this\n> case it would be doing a reset on an empty strbuf.  Is avoiding this\n> expense worth the reduction in readability?\n>\n> Also, as Eric Sunshine pointed out, being able to easily re-order\n> operations can make the code easier to maintain.\n>\n>> Repeated reset-and-then-add on the same strbuf used to be something\n>> that may indicate that the code is doing unnecessary work.  Now,\n>> repeated uses of strbuf_set on the same strbuf replaced that pattern\n>> to be watched for to spot wasteful code paths.\n>>\n> If a reset followed by and add was a rare occurrence I would tend to\n> agree more.\n\nWhen composing my review of the builtin/remote.c changes, I wrote\nsomething like this:\n\n    Although strbuf_set() does make the code a bit easier to read\n    when strbufs are repeatedly re-used, re-using a variable for\n    different purposes is generally considered poor programming\n    practice. It's likely that heavy re-use of strbufs has been\n    tolerated to avoid multiple heap allocations, but that may be a\n    case of premature (allocation) optimization, rather than good\n    programming. A different (\"better\") way to make the code more\n    readable and maintainable may be to ban re-use of strbufs for\n    different purposes.\n\nBut I deleted it before sending because it's a somewhat tangential\nissue not introduced by your changes. However, I do see strbuf_set()\nas a Band-Aid for the problem described above, rather than as a useful\nfeature on its own. If the practice of re-using strbufs (as a\npremature optimization) ever becomes taboo, then strbuf_set() loses\nits value.\n\n>> I dunno...\n>>\n>> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>\n>> > ---\n>> >  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++\n>> >  strbuf.c                               | 21 +++++++++++++++++++++\n>> >  strbuf.h                               | 13 +++++++++++++\n>> >  3 files changed, 52 insertions(+)\n>> >\n>> > diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt\n>> > index f9c06a7..ae9c9cc 100644\n>> > --- a/Documentation/technical/api-strbuf.txt\n>> > +++ b/Documentation/technical/api-strbuf.txt\n>> > @@ -149,6 +149,24 @@ Functions\n>> >     than zero if the first buffer is found, respectively, to be less than,\n>> >     to match, or be greater than the second buffer.\n>> >  /*----- add data in your buffer -----*/\n> ...\n>> >  static inline void strbuf_addch(struct strbuf *sb, int c)\n>> >  {\n>\n> --\n> Jeremiah Mahler\n> jmmahler@gmail.com\n> http://github.com/jmahler\n"},{"id":"244042","messageId":"20140612231417.GA17803@hudson.localdomain","threadId":"36897","inReplyTo":"CAPig+cT_StMkTgEd-RVpm=4e_A23zj+V5k83PhMtaN=tr4GBzA@mail.gmail.com","subject":"Re: [PATCH v3 1/2] add strbuf_set operations","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-12T23:14:17Z","receivedAt":"2014-06-12T23:14:17Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Thu, Jun 12, 2014 at 05:18:29PM -0400, Eric Sunshine wrote:\n> On Thu, Jun 12, 2014 at 3:36 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:\n> > On Thu, Jun 12, 2014 at 11:51:19AM -0700, Junio C Hamano wrote:\n> >> Jeremiah Mahler <jmmahler@gmail.com> writes:\n> >>\n> >> > Thomas,\n> >> >\n> >> > On Thu, Jun 12, 2014 at 10:11:36AM +0200, Thomas Braun wrote:\n> >> >> Am 12.06.2014 09:29, schrieb Jeremiah Mahler:\n> >> >> > A common use case with strubfs is to set the buffer to a new value.\n> >>\n> >> strubfs???\n> >>\n> > I was trying to make it plural.  Perhaps strbuf's?\n> \n> Junio was pointing out your misspelling, not your intention to pluralize.\n\nOK, got it.\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"244044","messageId":"20140612234637.GB17803@hudson.localdomain","threadId":"36897","inReplyTo":"CAPig+cTVLJQOsW7H4Ht2NNYkeiMb=EWT7BG3sNu0wNsTQ=oZNA@mail.gmail.com","subject":"Re: [PATCH v3 1/2] add strbuf_set operations","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-12T23:46:37Z","receivedAt":"2014-06-12T23:46:37Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Eric,\n\nOn Thu, Jun 12, 2014 at 05:48:52PM -0400, Eric Sunshine wrote:\n> On Thu, Jun 12, 2014 at 3:31 PM, Jeremiah Mahler <jmmahler@gmail.com> wrote:\n> > On Thu, Jun 12, 2014 at 11:50:41AM -0700, Junio C Hamano wrote:\n> >> I am on the fence.\n> >>\n> >> I have this suspicion that the addition of strbuf_set() would *only*\n> >> help when the original written with reset-and-then-add sequence was\n> >> suboptimal to begin with, and it helps *only* how the code reads,\n> >> without correcting the fact that it is still doing unnecessary\n> >> \"first set to a value to be discarded and then reset to set the\n> >> right value\", sweeping the issue under the rug.\n> >>\n> > It is certainly possible that builtin/remote.c (PATCH 2/2) saw the most\n> > benefit from this operation because it is so badly designed.  But this\n> > seems unlikely given the review process around here ;-)\n> >\n> > The one case where it would be doing extra work is when a strbuf_set\n> > replaces a strbuf_add that didn't previously have a strbuf_reset.\n> > strbuf_set is not appropriate for all cases, as I mentioned in the\n> > patch, but in some cases I think it makes it more readable.  And in this\n> > case it would be doing a reset on an empty strbuf.  Is avoiding this\n> > expense worth the reduction in readability?\n> >\n> > Also, as Eric Sunshine pointed out, being able to easily re-order\n> > operations can make the code easier to maintain.\n> >\n> >> Repeated reset-and-then-add on the same strbuf used to be something\n> >> that may indicate that the code is doing unnecessary work.  Now,\n> >> repeated uses of strbuf_set on the same strbuf replaced that pattern\n> >> to be watched for to spot wasteful code paths.\n> >>\n> > If a reset followed by and add was a rare occurrence I would tend to\n> > agree more.\n> \n> When composing my review of the builtin/remote.c changes, I wrote\n> something like this:\n> \n>     Although strbuf_set() does make the code a bit easier to read\n>     when strbufs are repeatedly re-used, re-using a variable for\n>     different purposes is generally considered poor programming\n>     practice. It's likely that heavy re-use of strbufs has been\n>     tolerated to avoid multiple heap allocations, but that may be a\n>     case of premature (allocation) optimization, rather than good\n>     programming. A different (\"better\") way to make the code more\n>     readable and maintainable may be to ban re-use of strbufs for\n>     different purposes.\n> \n> But I deleted it before sending because it's a somewhat tangential\n> issue not introduced by your changes. However, I do see strbuf_set()\n> as a Band-Aid for the problem described above, rather than as a useful\n> feature on its own. If the practice of re-using strbufs (as a\n> premature optimization) ever becomes taboo, then strbuf_set() loses\n> its value.\n> \n\nI am getting the feeling that I have mis-understood the purpose of\nstrbufs.  It is not just a library to use in place of char*.\n\nIf strbufs should only be added to and never reset a good test would be\nto re-write builtin/remote.c without the use of strbuf_reset.\n\nbuiltin/remote.c does re-use the buffers.  But it seems if a buffer is\nused N times then to avoid a reset you would need N buffers.\n\nBut on the other hand I agree with your comment that re-using a variable\nfor different purposes is poor practice.\n\nNow I am not even sure if I want my own patch :-)\n\n> >> I dunno...\n> >>\n> >> > Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>\n> >> > ---\n> >> >  Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++\n> >> >  strbuf.c                               | 21 +++++++++++++++++++++\n> >> >  strbuf.h                               | 13 +++++++++++++\n> >> >  3 files changed, 52 insertions(+)\n> >> >\n> >> > diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt\n> >> > index f9c06a7..ae9c9cc 100644\n> >> > --- a/Documentation/technical/api-strbuf.txt\n> >> > +++ b/Documentation/technical/api-strbuf.txt\n> >> > @@ -149,6 +149,24 @@ Functions\n> >> >     than zero if the first buffer is found, respectively, to be less than,\n> >> >     to match, or be greater than the second buffer.\n> >> >  /*----- add data in your buffer -----*/\n> > ...\n> >> >  static inline void strbuf_addch(struct strbuf *sb, int c)\n> >> >  {\n> >\n> > --\n> > Jeremiah Mahler\n> > jmmahler@gmail.com\n> > http://github.com/jmahler\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"244060","messageId":"20140613071550.GC7908@sigill.intra.peff.net","threadId":"36897","inReplyTo":"20140612234637.GB17803@hudson.localdomain","subject":"Re: [PATCH v3 1/2] add strbuf_set operations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T07:15:50Z","receivedAt":"2014-06-13T07:15:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 12, 2014 at 04:46:37PM -0700, Jeremiah Mahler wrote:\n\n> >     Although strbuf_set() does make the code a bit easier to read\n> >     when strbufs are repeatedly re-used, re-using a variable for\n> >     different purposes is generally considered poor programming\n> >     practice. It's likely that heavy re-use of strbufs has been\n> >     tolerated to avoid multiple heap allocations, but that may be a\n> >     case of premature (allocation) optimization, rather than good\n> >     programming. A different (\"better\") way to make the code more\n> >     readable and maintainable may be to ban re-use of strbufs for\n> >     different purposes.\n> > \n> > But I deleted it before sending because it's a somewhat tangential\n> > issue not introduced by your changes. However, I do see strbuf_set()\n> > as a Band-Aid for the problem described above, rather than as a useful\n> > feature on its own. If the practice of re-using strbufs (as a\n> > premature optimization) ever becomes taboo, then strbuf_set() loses\n> > its value.\n> > \n> \n> I am getting the feeling that I have mis-understood the purpose of\n> strbufs.  It is not just a library to use in place of char*.\n> \n> If strbufs should only be added to and never reset a good test would be\n> to re-write builtin/remote.c without the use of strbuf_reset.\n> \n> builtin/remote.c does re-use the buffers.  But it seems if a buffer is\n> used N times then to avoid a reset you would need N buffers.\n> \n> But on the other hand I agree with your comment that re-using a variable\n> for different purposes is poor practice.\n> \n> Now I am not even sure if I want my own patch :-)\n\nI think reusing buffers like remote.c does makes things uglier and more\nconfusing than necessary, and probably doesn't have any appreciable\nperformance gain. We are saving a handful of allocations, and have such\nwonderful variable names as \"buf2\" (when is it OK to reuse that one,\nversus regular \"buf\"?).\n\nA better reason I think is to reuse allocations across a loop, like:\n\n  struct strbuf buf = STRBUF_INIT;\n\n  for (i = 0; i < nr; i++) {\n\tstrbuf_reset(&buf);\n\tstrbuf_add(&buf, foo[i]);\n\t... do something with buf ...\n  }\n  strbuf_release(&buf);\n\nYou can write that as:\n\n  for (i = 0; i < nr; i++) {\n\tstruct strbuf buf = STRBUF_INIT;\n\tstrbuf_add(&buf, foo[i]);\n\t... do something ...\n\tstrbuf_release(&buf);\n  }\n\nand it is definitely still a case of premature optimization. But:\n\n  1. \"nr\" here may be very large, so the amortized benefits are greater\n\n  2. It's only one call to strbuf_reset to cover many items. Not one\n     sprinkled every few lines.\n\nYou'll note that strbuf_getline uses a \"set\" convention (making it\ndifferent from the rest of strbuf) to handle this looping case.\n\nI don't have a problem with strbuf_set, but just peeking at remote.c, I\nthink I'd rather see it cleaned up. It looks like one of the major uses\nis setting config variables. I wonder how hard it would be to make a\ngit_config_set variant that took printf-style formats, and handled the\nstrbuf itself.\n\n-Peff\n"},{"id":"244191","messageId":"20140614044906.GB1375@hudson.localdomain","threadId":"36897","inReplyTo":"20140613071550.GC7908@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/2] add strbuf_set operations","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-14T04:49:06Z","receivedAt":"2014-06-14T04:49:06Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Peff,\n\nOn Fri, Jun 13, 2014 at 03:15:50AM -0400, Jeff King wrote:\n> On Thu, Jun 12, 2014 at 04:46:37PM -0700, Jeremiah Mahler wrote:\n> \n> > >     Although strbuf_set() does make the code a bit easier to read\n> > >     when strbufs are repeatedly re-used, re-using a variable for\n> > >     different purposes is generally considered poor programming\n> > >     practice. It's likely that heavy re-use of strbufs has been\n> > >     tolerated to avoid multiple heap allocations, but that may be a\n> > >     case of premature (allocation) optimization, rather than good\n> > >     programming. A different (\"better\") way to make the code more\n> > >     readable and maintainable may be to ban re-use of strbufs for\n> > >     different purposes.\n> > > \n> > > But I deleted it before sending because it's a somewhat tangential\n> > > issue not introduced by your changes. However, I do see strbuf_set()\n> > > as a Band-Aid for the problem described above, rather than as a useful\n> > > feature on its own. If the practice of re-using strbufs (as a\n> > > premature optimization) ever becomes taboo, then strbuf_set() loses\n> > > its value.\n> > > \n> > \n> > I am getting the feeling that I have mis-understood the purpose of\n> > strbufs.  It is not just a library to use in place of char*.\n> > \n> > If strbufs should only be added to and never reset a good test would be\n> > to re-write builtin/remote.c without the use of strbuf_reset.\n> > \n> > builtin/remote.c does re-use the buffers.  But it seems if a buffer is\n> > used N times then to avoid a reset you would need N buffers.\n> > \n> > But on the other hand I agree with your comment that re-using a variable\n> > for different purposes is poor practice.\n> > \n> > Now I am not even sure if I want my own patch :-)\n> \n> I think reusing buffers like remote.c does makes things uglier and more\n> confusing than necessary, and probably doesn't have any appreciable\n> performance gain. We are saving a handful of allocations, and have such\n> wonderful variable names as \"buf2\" (when is it OK to reuse that one,\n> versus regular \"buf\"?).\n> \n> A better reason I think is to reuse allocations across a loop, like:\n> \n>   struct strbuf buf = STRBUF_INIT;\n> \n>   for (i = 0; i < nr; i++) {\n> \tstrbuf_reset(&buf);\n> \tstrbuf_add(&buf, foo[i]);\n> \t... do something with buf ...\n>   }\n>   strbuf_release(&buf);\n> \n> You can write that as:\n> \n>   for (i = 0; i < nr; i++) {\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tstrbuf_add(&buf, foo[i]);\n> \t... do something ...\n> \tstrbuf_release(&buf);\n>   }\n> \n> and it is definitely still a case of premature optimization. But:\n> \n>   1. \"nr\" here may be very large, so the amortized benefits are greater\n> \n>   2. It's only one call to strbuf_reset to cover many items. Not one\n>      sprinkled every few lines.\n> \n> You'll note that strbuf_getline uses a \"set\" convention (making it\n> different from the rest of strbuf) to handle this looping case.\n> \n> I don't have a problem with strbuf_set, but just peeking at remote.c, I\n> think I'd rather see it cleaned up. It looks like one of the major uses\n> is setting config variables. I wonder how hard it would be to make a\n> git_config_set variant that took printf-style formats, and handled the\n> strbuf itself.\n> \n> -Peff\n\nImproving remote.c sounds like a better direction than adding set\noperations.  I will start looking in to it.\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"}]}