{"thread":{"id":"42539","subject":"[PATCH V2 0/3] strbuf: improve API","startedAt":"2016-06-06T15:13:37Z","lastAt":"2016-06-16T02:19:50Z","messageCount":32,"participants":["William Duclot","Matthieu Moy","Junio C Hamano","Jeff King","Johannes Schindelin","Michael Haggerty","Ramsay Jones"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"288437","messageId":"20160606151340.22424-1-william.duclot@ensimag.grenoble-inp.fr","threadId":"42539","inReplyTo":null,"subject":"[PATCH V2 0/3] strbuf: improve API","fromName":"William Duclot","fromEmail":"william.duclot@ensimag.grenoble-inp.fr","sentAt":"2016-06-06T15:13:37Z","receivedAt":"2016-06-06T15:13:37Z","isPatch":true,"sender":{"key":"william.duclot@ensimag.grenoble-inp.fr","avatar":null},"body":"This patch series implements an improvment of the strbuf API, allowing\nstrbuf to use preallocated memory. This makes strbuf fit to be used\nin performance-critical operations.\n\n* The first patch is simply a preparatory work, adding tests for\nexisting strbuf implementation.\n* The second patch is also preparatory: rename a function for the third\npatch.\n* Most of the work is made in the third patch: handle pre-allocated\nmemory, extend the API, document it and test it.\n\nAs said in the third commit message, the idea comes from Michael Haggerty.\n\nWilliam Duclot (3):\n strbuf: add tests\n pretty.c: rename strbuf_wrap() function\n strbuf: allow to use preallocated memory\n\nMakefile | 1 +\npretty.c | 7 ++++---\nstrbuf.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------\nstrbuf.h | 44 ++++++++++++++++++++++++++++++++++++++--\nt/helper/test-strbuf.c | 229 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\nt/t0082-strbuf.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++\n6 files changed, 394 insertions(+), 17 deletions(-)\n"},{"id":"288438","messageId":"20160606151340.22424-2-william.duclot@ensimag.grenoble-inp.fr","threadId":"42539","inReplyTo":"20160606151340.22424-1-william.duclot@ensimag.grenoble-inp.fr","subject":"[PATCH V2 1/3] strbuf: add tests","fromName":"William Duclot","fromEmail":"william.duclot@ensimag.grenoble-inp.fr","sentAt":"2016-06-06T15:13:38Z","receivedAt":"2016-06-06T15:13:38Z","isPatch":true,"sender":{"key":"william.duclot@ensimag.grenoble-inp.fr","avatar":null},"body":"Test the strbuf API. Being used throughout all Git the API could be\nconsidered tested, but adding specific tests makes it easier to improve\nand extend the API.\n\nSigned-off-by: William Duclot \nSigned-off-by: Simon Rabourg \nSigned-off-by: Matthieu Moy \n---\nChanges since V1:\n * Use the parser API\n * Coding style fix\n * New test for string size\n\n Makefile | 1 +\n t/helper/test-strbuf.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++\n t/t0082-strbuf.sh | 19 ++++++++++\n 3 files changed, 121 insertions(+)\n create mode 100644 t/helper/test-strbuf.c\n create mode 100755 t/t0082-strbuf.sh\n\ndiff --git a/Makefile b/Makefile\nindex 3f03366..dc84f43 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -613,6 +613,7 @@ TEST_PROGRAMS_NEED_X += test-scrap-cache-tree\n TEST_PROGRAMS_NEED_X += test-sha1\n TEST_PROGRAMS_NEED_X += test-sha1-array\n TEST_PROGRAMS_NEED_X += test-sigchain\n+TEST_PROGRAMS_NEED_X += test-strbuf\n TEST_PROGRAMS_NEED_X += test-string-list\n TEST_PROGRAMS_NEED_X += test-submodule-config\n TEST_PROGRAMS_NEED_X += test-subprocess\ndiff --git a/t/helper/test-strbuf.c b/t/helper/test-strbuf.c\nnew file mode 100644\nindex 0000000..271592e\n--- /dev/null\n+++ b/t/helper/test-strbuf.c\n@@ -0,0 +1,101 @@\n+#include \"git-compat-util.h\"\n+#include \"strbuf.h\"\n+#include \"parse-options.h\"\n+#include \"builtin.h\"\n+\n+/*\n+ * Check behavior on usual use cases\n+ */\n+static int strbuf_check_behavior(struct strbuf *sb)\n+{\n+\tchar *str_test = xstrdup(\"test\"), *res, *old_buf;\n+\tsize_t size, old_alloc;\n+\n+\tstrbuf_grow(sb, 1);\n+\told_alloc = sb->alloc;\n+\tstrbuf_grow(sb, sb->alloc - sb->len + 1000);\n+\tif (old_alloc == sb->alloc)\n+\t\tdie(\"strbuf_grow does not realloc the buffer as expected\");\n+\told_buf = sb->buf;\n+\tres = strbuf_detach(sb, &size);\n+\tif (res != old_buf)\n+\t\tdie(\"strbuf_detach does not return the expected buffer\");\n+\tfree(res);\n+\n+\tstr_test = xstrdup(\"test\");\n+\tstrbuf_attach(sb, str_test, strlen(str_test), strlen(str_test) + 1);\n+\tres = strbuf_detach(sb, &size);\n+\tif (size != strlen(str_test)) \n+\t\tdie (\"size is not as expected\");\n+\n+\tif (res != str_test)\n+\t\tdie(\"strbuf_detach does not return the expected buffer\");\n+\tfree(res);\n+\tstrbuf_release(sb);\n+\n+\treturn 0;\n+}\n+\n+static int basic_grow(struct strbuf *sb) \n+{\n+\tstrbuf_init(sb, 0);\n+\tstrbuf_grow(sb, 0);\n+\tif (sb->buf == strbuf_slopbuf)\n+\t\tdie(\"strbuf_grow failed to alloc memory\");\n+\tstrbuf_release(sb);\n+\tif (sb->buf != strbuf_slopbuf)\n+\t\tdie(\"strbuf_release does not reinitialize the strbuf\");\n+\n+\treturn 0;\n+}\n+\n+static void grow_overflow(struct strbuf *sb)\n+{\n+\tstrbuf_init(sb, 1000);\n+\tstrbuf_grow(sb, maximum_unsigned_value_of_type((size_t)1));\n+}\n+\n+int main(int argc, const char *argv[])\n+{\n+\tstruct strbuf sb;\n+\tenum {\n+\t\tMODE_UNSPECIFIED = 0,\n+\t\tMODE_BASIC_GROW ,\n+\t\tMODE_STRBUF_CHECK,\n+\t\tMODE_GROW_OVERFLOW\n+\t} cmdmode = MODE_UNSPECIFIED;\n+\tstruct option options[] = {\n+\t\tOPT_CMDMODE(0, \"basic_grow\", &cmdmode,\n+\t\t\t N_(\" basic grow test\"),\n+\t\t\t MODE_BASIC_GROW),\n+\t\tOPT_CMDMODE(0, \"strbuf_check_behavior\", &cmdmode,\n+\t\t\t N_(\"check the strbuf's behavior\"),\n+\t\t\t MODE_STRBUF_CHECK),\n+\t\tOPT_CMDMODE(0, \"grow_overflow\", &cmdmode,\n+\t\t\t N_(\"test grow expecting overflow\"),\n+\t\t\t MODE_GROW_OVERFLOW),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, NULL, options, NULL, 0);\n+\n+\tif (cmdmode == MODE_BASIC_GROW) {\n+\t\t/*\n+\t\t * Check if strbuf_grow(0) allocate a new NUL-terminated buffer\n+\t\t */\n+\t\treturn basic_grow(&sb);\n+\t} else if (cmdmode == MODE_STRBUF_CHECK) {\n+\t\tstrbuf_init(&sb, 0);\n+\t\treturn strbuf_check_behavior(&sb);\n+\t} else if (cmdmode == MODE_GROW_OVERFLOW) {\n+\t\t/*\n+\t\t * size_t overflow: should die().\n+\t\t * If this does not die(), fall through to returning success.\n+\t\t */\n+\t\tgrow_overflow(&sb);\n+\t} else {\n+\t\tusage(\"test-strbuf mode\");\n+\t}\n+\n+\treturn 0;\n+}\ndiff --git a/t/t0082-strbuf.sh b/t/t0082-strbuf.sh\nnew file mode 100755\nindex 0000000..6a579a3\n--- /dev/null\n+++ b/t/t0082-strbuf.sh\n@@ -0,0 +1,19 @@\n+#!/bin/sh\n+\n+test_description='Test the strbuf API.'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'basic test on strbuf_grow()' '\n+\ttest-strbuf --basic_grow\n+'\n+\n+test_expect_success 'check strbuf behavior in usual use cases ' '\n+\ttest-strbuf --strbuf_check_behavior\n+'\n+\n+test_expect_success 'overflow while calling strbuf_grow' '\n+\ttest_must_fail test-strbuf --grow_overflow\n+'\n+\n+test_done\n-- \n2.9.0.rc1.1.geac644e\n"},{"id":"288440","messageId":"20160606151340.22424-3-william.duclot@ensimag.grenoble-inp.fr","threadId":"42539","inReplyTo":"20160606151340.22424-1-william.duclot@ensimag.grenoble-inp.fr","subject":"[PATCH V2 2/3] pretty.c: rename strbuf_wrap() function","fromName":"William Duclot","fromEmail":"william.duclot@ensimag.grenoble-inp.fr","sentAt":"2016-06-06T15:13:39Z","receivedAt":"2016-06-06T15:13:39Z","isPatch":true,"sender":{"key":"william.duclot@ensimag.grenoble-inp.fr","avatar":null},"body":"The function strbuf_wrap() is not part of the strbuf API, yet prevent to\nextend the API to include wrapping functions. Renaming it to something\nmore specific allow to use \"strbuf_wrap\" for the strbut API.\n\nSigned-off-by: William Duclot \nSigned-off-by: Simon Rabourg \nSigned-off-by: Matthieu Moy \n---\n pretty.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 87c4497..2b9e89a 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -904,8 +904,8 @@ static void parse_commit_message(struct format_commit_context *c)\n \tc->commit_message_parsed = 1;\n }\n \n-static void strbuf_wrap(struct strbuf *sb, size_t pos,\n-\t\t\tsize_t width, size_t indent1, size_t indent2)\n+static void strbuf_wrap_message(struct strbuf *sb, size_t pos,\n+\t\t\t\tsize_t width, size_t indent1, size_t indent2)\n {\n \tstruct strbuf tmp = STRBUF_INIT;\n \n@@ -926,7 +926,8 @@ static void rewrap_message_tail(struct strbuf *sb,\n \t c->indent2 == new_indent2)\n \t\treturn;\n \tif (c->wrap_start < sb->len)\n-\t\tstrbuf_wrap(sb, c->wrap_start, c->width, c->indent1, c->indent2);\n+\t\tstrbuf_wrap_message(sb, c->wrap_start, c->width, c->indent1,\n+\t\t\t\t c->indent2);\n \tc->wrap_start = sb->len;\n \tc->width = new_width;\n \tc->indent1 = new_indent1;\n-- \n2.9.0.rc1.1.geac644e\n"},{"id":"288439","messageId":"20160606151340.22424-4-william.duclot@ensimag.grenoble-inp.fr","threadId":"42539","inReplyTo":"20160606151340.22424-1-william.duclot@ensimag.grenoble-inp.fr","subject":"[PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"William Duclot","fromEmail":"william.duclot@ensimag.grenoble-inp.fr","sentAt":"2016-06-06T15:13:40Z","receivedAt":"2016-06-06T15:13:40Z","isPatch":true,"sender":{"key":"william.duclot@ensimag.grenoble-inp.fr","avatar":null},"body":"When working with strbufs (usually for dates or paths), the\nmalloc()/free() overhead could be easily avoided: as a sensible initial\nbuffer size is already known, it could be allocated on the stack. This\ncould avoid workarounds such as\n\n void f()\n {\n static struct strbuf path;\n strbuf_add(&path, ...);\n ...\n strbuf_setlen(&path, 0);\n }\n\nwhich, by using a static variable, avoid most of the malloc()/free()\noverhead, but makes the function unsafe to use recursively or from\nmultiple threads.\n\nTHE IDEA\n--------\n\nThe idea here is to enhance strbuf to allow it to use memory that it\ndoesn't own (for example, stack-allocated memory), while (optionally)\nallowing it to switch over to using allocated memory if the string grows\npast the size of the pre-allocated buffer.\n\nAPI ENHANCEMENT\n---------------\n\nAll functions of the API can still be reliably called without\nknowledge of the initialization (normal/preallocated/fixed) with the\nexception that strbuf_grow() may die() if the string tries to overflow a\nfixed buffer.\n\nThe API contract is still respected:\n\n- The API users may peek strbuf.buf in-place until they perform an\n operation that makes its size change (at which point the .buf pointer\n may point at a new piece of memory).\n\n- The API users may strbuf_detach() to obtain a piece of memory that\n belongs to them (at which point the strbuf becomes empty), hence\n needs to be freed by the callers.\n\n- The API users letting a strbuf go out of scope without taking\n ownership of the string with strbuf_detach() _must_ release the\n resource by calling strbuf_release().\n\nFull credit to Michael Haggerty for the idea and some of the wording of\nthis commit (http://mid.gmane.org/53512DB6.1070600@alum.mit.edu). The\nimplementation and bugs are all mine.\n\nSigned-off by: William Duclot \nSigned-off by: Simon Rabourg \nSigned-off by: Matthieu Moy \n---\nChanges since V1:\n * Refactoring: use a `strbuf_wrap_internal()` static function\n * Use bit fields for flags\n * Completing documentation & commit message\n * Rename `strbuf_wrap_preallocated()` to `strbuf_wrap()`\n * Introduce two initialization macros for wrapping\n\n strbuf.c | 82 ++++++++++++++++++++++++++-----\n strbuf.h | 44 ++++++++++++++++-\n t/helper/test-strbuf.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++++-\n t/t0082-strbuf.sh | 29 +++++++++++\n 4 files changed, 270 insertions(+), 15 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 1ba600b..689469e 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -1,6 +1,9 @@\n #include \"cache.h\"\n #include \"refs.h\"\n #include \"utf8.h\"\n\n+#define MAX_ALLOC(a, b) (((a)>(b))?(a):(b))\n \n int starts_with(const char *str, const char *prefix)\n {\n@@ -20,16 +23,47 @@ char strbuf_slopbuf[1];\n \n void strbuf_init(struct strbuf *sb, size_t hint)\n {\n+\tsb->owns_memory = 0;\n+\tsb->fixed_memory = 0;\n \tsb->alloc = sb->len = 0;\n \tsb->buf = strbuf_slopbuf;\n \tif (hint)\n \t\tstrbuf_grow(sb, hint);\n }\n \n+static void strbuf_wrap_internal(struct strbuf *sb, char *buf,\n+\t\t\t\t size_t buf_len, size_t alloc_len)\n+{\n+\tif (!buf)\n+\t\tdie(\"the buffer of a strbuf cannot be NULL\");\n+\n+\tstrbuf_release(sb);\n+\tsb->len = buf_len;\n+\tsb->alloc = alloc_len;\n+\tsb->buf = buf;\n+}\n+\n+void strbuf_wrap(struct strbuf *sb, char *buf,\n+\t\t size_t buf_len, size_t alloc_len)\n+{\n+\tstrbuf_wrap_internal(sb, buf, buf_len, alloc_len);\n+\tsb->owns_memory = 0;\n+\tsb->fixed_memory = 0;\n+}\n+\n+void strbuf_wrap_fixed(struct strbuf *sb, char *buf,\n+\t\t size_t buf_len, size_t alloc_len)\n+{\n+\tstrbuf_wrap_internal(sb, buf, buf_len, alloc_len);\n+\tsb->owns_memory = 0;\n+\tsb->fixed_memory = 1;\n+}\n+\n void strbuf_release(struct strbuf *sb)\n {\n \tif (sb->alloc) {\n-\t\tfree(sb->buf);\n+\t\tif (sb->owns_memory)\n+\t\t\tfree(sb->buf);\n \t\tstrbuf_init(sb, 0);\n \t}\n }\n@@ -37,8 +71,13 @@ void strbuf_release(struct strbuf *sb)\n char *strbuf_detach(struct strbuf *sb, size_t *sz)\n {\n \tchar *res;\n+\n \tstrbuf_grow(sb, 0);\n-\tres = sb->buf;\n+\tif (sb->owns_memory)\n+\t\tres = sb->buf;\n+\telse\n+\t\tres = xmemdupz(sb->buf, sb->len);\n+\n \tif (sz)\n \t\t*sz = sb->len;\n \tstrbuf_init(sb, 0);\n@@ -47,25 +86,44 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz)\n \n void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)\n {\n-\tstrbuf_release(sb);\n-\tsb->buf = buf;\n-\tsb->len = len;\n-\tsb->alloc = alloc;\n+\tstrbuf_wrap_internal(sb, buf, len, alloc);\n+\tsb->owns_memory = 1;\n+\tsb->fixed_memory = 0;\n \tstrbuf_grow(sb, 0);\n \tsb->buf[sb->len] = '\\0';\n }\n \n void strbuf_grow(struct strbuf *sb, size_t extra)\n {\n-\tint new_buf = !sb->alloc;\n \tif (unsigned_add_overflows(extra, 1) ||\n \t unsigned_add_overflows(sb->len, extra + 1))\n \t\tdie(\"you want to use way too much memory\");\n-\tif (new_buf)\n-\t\tsb->buf = NULL;\n-\tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n-\tif (new_buf)\n-\t\tsb->buf[0] = '\\0';\n+\tif ((sb->fixed_memory) &&\n+\t sb->len + extra + 1 > sb->alloc)\n+\t\tdie(\"you try to overflow the buffer of a fixed strbuf\");\n+\n+\t/*\n+\t * ALLOC_GROW may do a realloc() if needed, so we must not use it on\n+\t * a buffer the strbuf doesn't own\n+\t */\n+\tif (sb->owns_memory) {\n+\t\tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n+\t} else {\n+\t\t/*\n+\t\t * The strbuf doesn't own the buffer: to avoid to realloc it,\n+\t\t * the strbuf needs to use a new buffer without freeing the old\n+\t\t */\n+\t\tif (sb->len + extra + 1 > sb->alloc) {\n+\t\t\tsize_t new_alloc =\n+\t\t\t\tMAX_ALLOC(sb->len + extra + 1,\n+\t\t\t\t\t alloc_nr(sb->alloc));\n+\t\t\tchar *new_buf = xmalloc(new_alloc);\n+\t\t\tmemcpy(new_buf, sb->buf, sb->len + 1);\n+\t\t\tsb->buf = new_buf;\n+\t\t\tsb->alloc = new_alloc;\n+\t\t\tsb->owns_memory = 1;\n+\t\t}\n+\t}\n }\n \n void strbuf_trim(struct strbuf *sb)\ndiff --git a/strbuf.h b/strbuf.h\nindex 7987405..57b5cd1 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -11,11 +11,16 @@\n * A strbuf is NUL terminated for convenience, but no function in the\n * strbuf API actually relies on the string being free of NULs.\n *\n+ * You can avoid the malloc/free overhead of `strbuf_init()`, `strbuf_add()` and\n+ * `strbuf_release()` by wrapping pre-allocated memory (stack-allocated for\n+ * example) using `strbuf_wrap()` or `strbuf_wrap_fixed()`.\n+ *\n * strbufs have some invariants that are very important to keep in mind:\n *\n * - The `buf` member is never NULL, so it can be used in any usual C\n * string operations safely. strbuf's _have_ to be initialized either by\n- * `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.\n+ * `strbuf_init()`, `= STRBUF_INIT`, `strbuf_wrap()` or `strbuf_wrap_fixed()`\n+ * before the invariants, though.\n *\n * Do *not* assume anything on what `buf` really is (e.g. if it is\n * allocated memory or not), use `strbuf_detach()` to unwrap a memory\n@@ -45,12 +50,16 @@\n * Doing so is safe, though if it has to be done in many places, adding the\n * missing API to the strbuf module is the way to go.\n *\n+ * NOTE: strbuf_release can always be safely called, whether the strbuf is\n+ * wrapping a preallocated buffer or not.\n+ *\n * WARNING: Do _not_ assume that the area that is yours is of size `alloc\n * - 1` even if it's true in the current implementation. Alloc is somehow a\n * \"private\" member that should not be messed with. Use `strbuf_avail()`\n * instead.\n */\n \n+\n /**\n * Data Structures\n * ---------------\n@@ -62,13 +71,23 @@\n * access to the string itself.\n */\n struct strbuf {\n+\t/*\n+\t * The `owns_memory` flag is unset by default as strbuf_slopbuf is\n+\t * shared by all buffers, thus owned by none.\n+\t * The `fixed_memory` guarantees that the buffer will not be moved by\n+\t * realloc().\n+\t */\n+\tunsigned owns_memory:1, fixed_memory:1;\n \tsize_t alloc;\n \tsize_t len;\n \tchar *buf;\n };\n \n extern char strbuf_slopbuf[];\n-#define STRBUF_INIT { 0, 0, strbuf_slopbuf }\n+#define STRBUF_INIT { 0, 0, 0, 0, strbuf_slopbuf }\n+#define STRBUF_WRAP(buf, buf_len, alloc_len) { 0, 0, alloc_len, buf_len, buf }\n+#define STRBUF_WRAP_FIXED(buf, buf_len, alloc_len) \\\n+\t{ 0, 1, alloc_len, buf_len, buf }\n \n /**\n * Life Cycle Functions\n@@ -81,6 +100,25 @@ extern char strbuf_slopbuf[];\n */\n extern void strbuf_init(struct strbuf *, size_t);\n \n+/**\n+ * Allow the caller to give a pre-allocated piece of memory for the strbuf\n+ * to use. It is possible then to strbuf_grow() the string past the size of the\n+ * pre-allocated buffer: a new buffer will then be allocated. The pre-allocated\n+ * buffer will never be freed.\n+ */\n+void strbuf_wrap(struct strbuf *sb, char *buf,\n+\t\t size_t buf_len, size_t alloc_len);\n+\n+/**\n+ * Allow the caller to give a pre-allocated piece of memory for the strbuf\n+ * to use and indicate that the strbuf must use exclusively this buffer,\n+ * never realloc() it or allocate a new one. It means that the string can\n+ * grow or shrink but cannot overflow the pre-allocated buffer. The\n+ * pre-allocated buffer will never be freed.\n+ */\n+void strbuf_wrap_fixed(struct strbuf *sb, char *buf,\n+\t\t size_t buf_len, size_t alloc_len);\n+\n /**\n * Release a string buffer and the memory it used. You should not use the\n * string buffer after using this function, unless you initialize it again.\n@@ -91,6 +129,8 @@ extern void strbuf_release(struct strbuf *);\n * Detach the string from the strbuf and returns it; you now own the\n * storage the string occupies and it is your responsibility from then on\n * to release it with `free(3)` when you are done with it.\n+ * Must allocate a copy of the buffer in case of a wrapped buffer.\n+ * Performance-critical operations have to be aware of this.\n */\n extern char *strbuf_detach(struct strbuf *, size_t *);\n \ndiff --git a/t/helper/test-strbuf.c b/t/helper/test-strbuf.c\nindex 271592e..87628d9 100644\n--- a/t/helper/test-strbuf.c\n+++ b/t/helper/test-strbuf.c\n@@ -55,6 +55,75 @@ static void grow_overflow(struct strbuf *sb)\n \tstrbuf_grow(sb, maximum_unsigned_value_of_type((size_t)1));\n }\n \n+static void preallocated_NULL(struct strbuf *sb)\n+{\n+\tchar str_test[5] = \"test\";\n+\n+\tstrbuf_wrap(sb, NULL, 0, sizeof(str_test));\n+}\n+\n+static void grow_fixed_overflow(struct strbuf *sb)\n+{\n+\tchar str_foo[7] = \"foo\";\n+\n+\tstrbuf_wrap_fixed(sb, (void *)str_foo,\n+\t\t\t strlen(str_foo), sizeof(str_foo));\n+\tstrbuf_grow(sb, 3);\n+\tstrbuf_grow(sb, 1000);\n+}\n+\n+static void grow_fixed_overflow_min(struct strbuf *sb)\n+{\n+\tchar str_foo[7] = \"foo\";\n+\n+\tstrbuf_wrap_fixed(sb, (void *)str_foo,\n+\t\t\t strlen(str_foo), sizeof(str_foo));\n+\tstrbuf_grow(sb, 4);\n+}\n+\n+static int grow_fixed_success(struct strbuf *sb)\n+{\n+\tchar str_foo[7] = \"foo\";\n+\n+\tstrbuf_wrap_fixed(sb, (void *)str_foo,\n+\t\t\t strlen(str_foo), sizeof(str_foo));\n+\tstrbuf_grow(sb, 3);\n+\n+\treturn 0;\n+}\n+\n+static int detach_fixed(struct strbuf *sb)\n+{\n+\tsize_t size = 1;\n+\tchar str_test[5] = \"test\";\n+\tchar *buf;\n+\n+\tstrbuf_wrap_fixed(sb, (void *)str_test,\n+\t\t\t strlen(str_test), sizeof(str_test));\n+\tbuf = strbuf_detach(sb, &size);\n+\tif (size != strlen(str_test))\n+\t\tdie (\"size is not as expected\");\n+\n+\tif (str_test == buf)\n+\t\tdie(\"strbuf_detach does not copy the buffer\");\n+\tfree(buf);\n+\n+\treturn 0;\n+}\n+\n+static int release_fixed(struct strbuf *sb)\n+{\n+\tchar str_test[5] = \"test\";\n+\n+\tstrbuf_wrap_fixed(sb, (void *)str_test, strlen(str_test),\n+\t\t\t sizeof(sb) + 1);\n+\tstrbuf_release(sb);\n+\tif (sb->buf != strbuf_slopbuf)\n+\t\tdie(\"strbuf_release does not reinitialize the strbuf\");\n+\n+\treturn 0;\n+}\n+\n int main(int argc, const char *argv[])\n {\n \tstruct strbuf sb;\n@@ -62,7 +131,14 @@ int main(int argc, const char *argv[])\n \t\tMODE_UNSPECIFIED = 0,\n \t\tMODE_BASIC_GROW ,\n \t\tMODE_STRBUF_CHECK,\n-\t\tMODE_GROW_OVERFLOW\n+\t\tMODE_GROW_OVERFLOW,\n+\t\tMODE_PREALLOC_CHECK,\n+\t\tMODE_PREALLOC_NULL,\n+\t\tMODE_GROW_FIXED_OVERFLOW,\n+\t\tMODE_GROW_FIXED_OVERFLOW_MIN,\n+\t\tMODE_GROW_FIXED_SUCCESS,\n+\t\tMODE_DETACH_FIXED,\n+\t\tMODE_RELEASE_FIXED\n \t} cmdmode = MODE_UNSPECIFIED;\n \tstruct option options[] = {\n \t\tOPT_CMDMODE(0, \"basic_grow\", &cmdmode,\n@@ -74,6 +150,27 @@ int main(int argc, const char *argv[])\n \t\tOPT_CMDMODE(0, \"grow_overflow\", &cmdmode,\n \t\t\t N_(\"test grow expecting overflow\"),\n \t\t\t MODE_GROW_OVERFLOW),\n+\t\tOPT_CMDMODE(0, \"preallocated_check_behavior\", &cmdmode,\n+\t\t\t N_(\"check the wrap's behavior\"),\n+\t\t\t MODE_PREALLOC_CHECK),\n+\t\tOPT_CMDMODE(0, \"preallocated_NULL\", &cmdmode,\n+\t\t\t N_(\"initializing wrap with NULL\"),\n+\t\t\t MODE_PREALLOC_NULL),\n+\t\tOPT_CMDMODE(0, \"grow_fixed_overflow\", &cmdmode,\n+\t\t\t N_(\"check grow, fixed memory expecting overflow\"),\n+\t\t\t MODE_GROW_FIXED_OVERFLOW),\n+\t\tOPT_CMDMODE(0, \"grow_fixed_overflow_min\", &cmdmode,\n+\t\t\t N_(\"check grow, fixed memory and lower_bound for an overflow\"),\n+\t\t\t MODE_GROW_FIXED_OVERFLOW_MIN),\n+\t\tOPT_CMDMODE(0, \"grow_fixed_success\", &cmdmode,\n+\t\t\t N_(\"check grow, fixed memory\"),\n+\t\t\t MODE_GROW_FIXED_SUCCESS),\n+\t\tOPT_CMDMODE(0, \"detach_fixed\", &cmdmode,\n+\t\t\t N_(\"check detach, fixed memory\"),\n+\t\t\t MODE_DETACH_FIXED),\n+\t\tOPT_CMDMODE(0, \"release_fixed\", &cmdmode,\n+\t\t\t N_(\"check release, fixed memory\"),\n+\t\t\t MODE_RELEASE_FIXED),\n \t\tOPT_END()\n \t};\n \n@@ -93,6 +190,37 @@ int main(int argc, const char *argv[])\n \t\t * If this does not die(), fall through to returning success.\n \t\t */\n \t\tgrow_overflow(&sb);\n+\t} else if (cmdmode == MODE_PREALLOC_CHECK) {\n+\t\tchar str_test[5] = \"test\";\n+\n+\t\tstrbuf_wrap(&sb, (void *)str_test,\n+\t\t\t strlen(str_test), sizeof(str_test));\n+\t\treturn strbuf_check_behavior(&sb);\n+\t} else if (cmdmode == MODE_PREALLOC_NULL) {\n+\t\t /*\n+\t\t * Violation of invariant \"strbuf can't be NULL\": should die().\n+\t\t * If this does not die(), fall through to returning success.\n+\t\t */\n+\t\tpreallocated_NULL(&sb);\n+\t} else if (cmdmode == MODE_GROW_FIXED_OVERFLOW) {\n+\t\t/*\n+\t\t * Overflowing the buffer of a fixed strbuf: should die().\n+\t\t * If this does not die(), fall through to returning success.\n+\t\t */\n+\t\tgrow_fixed_overflow(&sb);\n+\t} else if (cmdmode == MODE_GROW_FIXED_OVERFLOW_MIN) {\n+\t\t/*\n+\t\t * Minimum strbuf_grow() for overflowing a fixed strbuf: should\n+\t\t * die().\n+\t\t * If this does not die(), fall through to returning success.\n+\t\t */\n+\t\tgrow_fixed_overflow_min(&sb);\n+\t} else if (cmdmode == MODE_GROW_FIXED_SUCCESS) {\n+\t\treturn grow_fixed_success(&sb);\n+\t} else if (cmdmode == MODE_DETACH_FIXED) {\n+\t\treturn detach_fixed(&sb);\n+\t} else if (cmdmode == MODE_RELEASE_FIXED) {\n+\t\treturn release_fixed(&sb);\n \t} else {\n \t\tusage(\"test-strbuf mode\");\n \t}\ndiff --git a/t/t0082-strbuf.sh b/t/t0082-strbuf.sh\nindex 6a579a3..6bea747 100755\n--- a/t/t0082-strbuf.sh\n+++ b/t/t0082-strbuf.sh\n@@ -16,4 +16,33 @@ test_expect_success 'overflow while calling strbuf_grow' '\n \ttest_must_fail test-strbuf --grow_overflow\n '\n \n+test_expect_success 'check preallocated strbuf behavior in usual use cases' '\n+\n+\ttest-strbuf --preallocated_check_behavior\n+'\n+\n+test_expect_success 'strbuf_wrap_preallocated NULL initialization' '\n+\ttest_must_fail test-strbuf --preallocated_NULL\n+'\n+\n+test_expect_success 'strbuf_grow with wrap_fixed overflow' '\n+\ttest_must_fail test-strbuf --grow_fixed_overflow\n+'\n+\n+test_expect_success 'strbuf_grow with wrap_fixed minimum overflow' '\n+\ttest_must_fail test-strbuf --grow_fixed_overflow_min\n+'\n+\n+test_expect_success 'strbuf_grow with wrap_fixed in a successful case' '\n+\ttest-strbuf --grow_fixed_success\n+'\n+\n+test_expect_success 'stbuf_detach with wrap_fixed memory' '\n+\ttest-strbuf --detach_fixed\n+'\n+\n+test_expect_success 'stbuf_release with wrap_fixed memory' '\n+\ttest-strbuf --release_fixed\n+'\n+\n test_done\n-- \n2.9.0.rc1.1.geac644e\n"},{"id":"288444","messageId":"vpq7fe2guus.fsf@anie.imag.fr","threadId":"42539","inReplyTo":"20160606151340.22424-2-william.duclot@ensimag.grenoble-inp.fr","subject":"Re: [PATCH V2 1/3] strbuf: add tests","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-06-06T16:11:55Z","receivedAt":"2016-06-06T16:11:55Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"William Duclot writes:\n\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -613,6 +613,7 @@ TEST_PROGRAMS_NEED_X += test-scrap-cache-tree\n> TEST_PROGRAMS_NEED_X += test-sha1\n> TEST_PROGRAMS_NEED_X += test-sha1-array\n> TEST_PROGRAMS_NEED_X += test-sigchain\n> +TEST_PROGRAMS_NEED_X += test-strbuf\n> TEST_PROGRAMS_NEED_X += test-string-list\n> TEST_PROGRAMS_NEED_X += test-submodule-config\n> TEST_PROGRAMS_NEED_X += test-subprocess\n> diff --git a/t/helper/test-strbuf.c b/t/helper/test-strbuf.c\n> new file mode 100644\n> index 0000000..271592e\n> --- /dev/null\n> +++ b/t/helper/test-strbuf.c\n> @@ -0,0 +1,101 @@\n> +#include \"git-compat-util.h\"\n> +#include \"strbuf.h\"\n> +#include \"parse-options.h\"\n> +#include \"builtin.h\"\n> +\n> +/*\n> + * Check behavior on usual use cases\n> + */\n> +static int strbuf_check_behavior(struct strbuf *sb)\n> +{\n> +\tchar *str_test = xstrdup(\"test\"), *res, *old_buf;\n> +\tsize_t size, old_alloc;\n> +\n> +\tstrbuf_grow(sb, 1);\n> +\told_alloc = sb->alloc;\n> +\tstrbuf_grow(sb, sb->alloc - sb->len + 1000);\n> +\tif (old_alloc == sb->alloc)\n> +\t\tdie(\"strbuf_grow does not realloc the buffer as expected\");\n> +\told_buf = sb->buf;\n> +\tres = strbuf_detach(sb, &size);\n> +\tif (res != old_buf)\n> +\t\tdie(\"strbuf_detach does not return the expected buffer\");\n> +\tfree(res);\n> +\n> +\tstr_test = xstrdup(\"test\");\n> +\tstrbuf_attach(sb, str_test, strlen(str_test), strlen(str_test) + 1);\n> +\tres = strbuf_detach(sb, &size);\n> +\tif (size != strlen(str_test)) \n> +\t\tdie (\"size is not as expected\");\n\nSpace before '('. Also, it's nice for the guy debugging that to say\n\"incorrect size. Expected %d, got %d\" or so.\n\n> diff --git a/t/t0082-strbuf.sh b/t/t0082-strbuf.sh\n> new file mode 100755\n> index 0000000..6a579a3\n> --- /dev/null\n> +++ b/t/t0082-strbuf.sh\n> @@ -0,0 +1,19 @@\n> +#!/bin/sh\n> +\n> +test_description='Test the strbuf API.'\n\nThe '.' is usually omitted.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"288445","messageId":"vpq37oqgut4.fsf@anie.imag.fr","threadId":"42539","inReplyTo":"20160606151340.22424-3-william.duclot@ensimag.grenoble-inp.fr","subject":"Re: [PATCH V2 2/3] pretty.c: rename strbuf_wrap() function","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-06-06T16:12:55Z","receivedAt":"2016-06-06T16:12:55Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"William Duclot writes:\n\n> The function strbuf_wrap() is not part of the strbuf API, yet prevent to\n> extend the API to include wrapping functions. Renaming it to something\n> more specific allow to use \"strbuf_wrap\" for the strbut API.\n\ns/strbut/strbuf/\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"288447","messageId":"vpqwpm2fg0t.fsf@anie.imag.fr","threadId":"42539","inReplyTo":"20160606151340.22424-4-william.duclot@ensimag.grenoble-inp.fr","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-06-06T16:17:38Z","receivedAt":"2016-06-06T16:17:38Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"I'm waiting for the discussion \"is this useful\" to settle before I do a\nfinal review, but I went quickly through the code and it seems OK.\n\nJust to show I read till the end:\n\nWilliam Duclot writes:\n\n> +test_expect_success 'check preallocated strbuf behavior in usual use cases' '\n> +\n> +\ttest-strbuf --preallocated_check_behavior\n\nUseless blank line.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"288461","messageId":"xmqqvb1mxmk4.fsf@gitster.mtv.corp.google.com","threadId":"42539","inReplyTo":"20160606151340.22424-4-william.duclot@ensimag.grenoble-inp.fr","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-06T17:19:07Z","receivedAt":"2016-06-06T17:19:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"William Duclot writes:\n\n> +#define MAX_ALLOC(a, b) (((a)>(b))?(a):(b))\n\nI do not see why this macro is called MAX_ALLOC(); is there anything\n\"alloc\" specific to what this does? You may happen to use it only\nfor \"alloc\" related things, but that is not a good reason for such a\nname (if the implementation can only be used for \"alloc\" specific\npurpose for some reason, then the name is appropriate).\n\n> @@ -20,16 +23,47 @@ char strbuf_slopbuf[1];\n> \n> void strbuf_init(struct strbuf *sb, size_t hint)\n> {\n> +\tsb->owns_memory = 0;\n> +\tsb->fixed_memory = 0;\n> \tsb->alloc = sb->len = 0;\n> \tsb->buf = strbuf_slopbuf;\n> \tif (hint)\n> \t\tstrbuf_grow(sb, hint);\n> }\n\nI'll complain about _grow() again later, but if the implementation\nis updated to address that complaint, slopbuf-using strbuf could\nstart it as a special case of \"starts as fixed_memory\".\n\n> +static void strbuf_wrap_internal(struct strbuf *sb, char *buf,\n> +\t\t\t\t size_t buf_len, size_t alloc_len)\n> +{\n> +\tif (!buf)\n> +\t\tdie(\"the buffer of a strbuf cannot be NULL\");\n> +\n> +\tstrbuf_release(sb);\n> +\tsb->len = buf_len;\n> +\tsb->alloc = alloc_len;\n> +\tsb->buf = buf;\n> +}\n> +\n> +void strbuf_wrap(struct strbuf *sb, char *buf,\n> +\t\t size_t buf_len, size_t alloc_len)\n> +{\n> +\tstrbuf_wrap_internal(sb, buf, buf_len, alloc_len);\n> +\tsb->owns_memory = 0;\n> +\tsb->fixed_memory = 0;\n> +}\n>\n> +void strbuf_wrap_fixed(struct strbuf *sb, char *buf,\n> +\t\t size_t buf_len, size_t alloc_len)\n> +{\n> +\tstrbuf_wrap_internal(sb, buf, buf_len, alloc_len);\n> +\tsb->owns_memory = 0;\n> +\tsb->fixed_memory = 1;\n> +}\n\nAfter seeing 2/3, I would say that the pretty side deserves the use\nof word \"wrap\" more. What the above two does is an enhancement of\nwhat strbuf API has called \"attach\". You are introducing a few new\nand different ways to attach a piece of memory to a strbuf.\n\n> @@ -37,8 +71,13 @@ void strbuf_release(struct strbuf *sb)\n> char *strbuf_detach(struct strbuf *sb, size_t *sz)\n> {\n> \tchar *res;\n> +\n> \tstrbuf_grow(sb, 0);\n> -\tres = sb->buf;\n> +\tif (sb->owns_memory)\n> +\t\tres = sb->buf;\n> +\telse\n> +\t\tres = xmemdupz(sb->buf, sb->len);\n> +\n> \tif (sz)\n> \t\t*sz = sb->len;\n> \tstrbuf_init(sb, 0);\n\nNote that the user of the API does not have to care if the memory is\nheld by the strbuf or if the strbuf is merely borrowing it from the\ninitial allocation made from elsewhere (e.g. via strbuf_wrap_*) when\nfinalizing its use of the strbuf API to build the contents and\nutilize the resulting buffer. Theoretically, if the caller knows it\nlet the piece memory _it_ owns (i.e. !owns_memory) and the piece of\nmemory it originally gave the strbuf hasn't been moved (i.e. your\nfixed_memory, which I will complain again soon), the caller\nshouldn't have to get a copy via xmemdupz(), which would both save\nan allocation here and free() in the caller once it is done.\n\nBut that is not done, and as the API it is a good thing. It frees\nthe caller from having to worry about _where_ the memory originally\ncame from.\n\n> void strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n> -\tint new_buf = !sb->alloc;\n> \tif (unsigned_add_overflows(extra, 1) ||\n> \t unsigned_add_overflows(sb->len, extra + 1))\n> \t\tdie(\"you want to use way too much memory\");\n> -\tif (new_buf)\n> -\t\tsb->buf = NULL;\n> -\tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> -\tif (new_buf)\n> -\t\tsb->buf[0] = '\\0';\n> +\tif ((sb->fixed_memory) &&\n> +\t sb->len + extra + 1 > sb->alloc)\n> +\t\tdie(\"you try to overflow the buffer of a fixed strbuf\");\n\nHaving praised how _detach() frees the user of the API from having\nto worry about minute details of allocation, this is an eyesore. I\nreally think this \"fixed_memory\" support MUST GO. This \"die()\" here\nmakes the API useless _unless_ the caller always makes sure that any\noperation it performs does not overflow the originally allocated\nsize, which makes it no better than using xsnprintf() and friends.\n\nA better design would be to get rid of fixed_memory bit and replace\nit with \"owns_memory\" bit. Its meaning would be \"The ->buf pointer\npoints at something that is not owned by this strbuf, and must not\nbe freed when resizing or releasing ->buf.\"\n\nAnd when you detect that a strbuf with unfreeable memory needs to be\nextended (the above condition you call \"die()\" on), you use the\n\"strbuf doesn't own the buffer\" logic below.\n\nThe only difference between your version and an improved one that\ngets rid of \"fixed_memory\" from the API user's point of view is that\nthey must pre-allocate absolute maximum amount of memory expected to\nbe consumed with your version in order not to crash, while a \"start\nwith fixed but dynamically allocate from heap if needed\" approach\nlets the API user preallocate just sufficient amount of memory\nexpected to be consumed in a typical workload and go without calling\nmalloc()/free() most of the time. Oh, and when a rare data comes in\nthat exceeds your initial estimate, \"fixed_memory\" version will just\ndie.\n\nSo, as I said in the previous round, I really do not see a point in\nthis fixed_memory business. It does not make the implementation of\nthe API any easier as far as I can tell, either.\n\n> +\t/*\n> +\t * ALLOC_GROW may do a realloc() if needed, so we must not use it on\n> +\t * a buffer the strbuf doesn't own\n> +\t */\n> +\tif (sb->owns_memory) {\n> +\t\tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> +\t} else {\n> +\t\t/*\n> +\t\t * The strbuf doesn't own the buffer: to avoid to realloc it,\n> +\t\t * the strbuf needs to use a new buffer without freeing the old\n> +\t\t */\n> +\t\tif (sb->len + extra + 1 > sb->alloc) {\n> +\t\t\tsize_t new_alloc =\n> +\t\t\t\tMAX_ALLOC(sb->len + extra + 1,\n> +\t\t\t\t\t alloc_nr(sb->alloc));\n\nIt is true that the you cannot simply realloc(->buf), but otherwise\nthe growth pattern of the buffer is the same between ->owns_memory\ncase and this case. And I think the above computes it like so, but\ndo you really need a one-shot macro MAX_ALLOC that is named in a\nmisleading way?\n\nI'd find it far easier to read if you wrote the core of ALLOC_GROW()\nlogic, like this:\n\n\tsize_t new_nr = sb->len + extra + 1;\n\tif (new_nr > sb->alloc) {\n\t\tsize_t new_alloc;\n\t\tif (alloc_nr(sb->alloc) < new_nr)\n \tnew_alloc = new_nr;\n\t\telse\n\t\t\tnew_alloc = alloc_nr(sb->alloc);\n\t\t... copy and make it \"owned\" ...\n\t}\n\nA better way would be to introduce a new macro ALLOC_GROW_COUNT() to\ncache.h immediately before ALLOC_GROW() is defined and use it to\nredo ALLOC_GROW(), perhaps like this:\n\n#define ALLOC_GROW_COUNT(nr, alloc) \\\n ((alloc_nr(alloc) < (nr)) ? (nr) : alloc_nr(alloc))\n\n#define ALLOC_GROW(x, nr, alloc) \\\n\tdo { \\\n if ((nr) > alloc) { \\\n \talloc = ALLOC_GROW_COUNT(nr, alloc); \\\n\t\t\tREALLOC_ARRAY(x, alloc); \\\n\t\t} \\\n\t} while (0)\t\t\t\n\nThen you can do\n\n\tif (sb->len + extra + 1 > sb->alloc) {\n\t\tsize_t new_alloc;\n char *new_buf;\n\n \tnew_alloc = ALLOC_GROW_COUNT(sb->len + extra + 1, sb->alloc);\n\t\tnew_buf = xmalloc(new_alloc);\n\t\tmemcpy(...);\n\t\t...\n"},{"id":"288485","messageId":"20160606203901.GA7667@Messiaen","threadId":"42539","inReplyTo":"xmqqvb1mxmk4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"William Duclot","fromEmail":"william.duclot@ensimag.grenoble-inp.fr","sentAt":"2016-06-06T20:39:01Z","receivedAt":"2016-06-06T20:39:01Z","isPatch":true,"sender":{"key":"william.duclot@ensimag.grenoble-inp.fr","avatar":null},"body":"On Mon, Jun 06, 2016 at 10:19:07AM -0700, Junio C Hamano wrote:\n> William Duclot writes:\n> \n>> +#define MAX_ALLOC(a, b) (((a)>(b))?(a):(b))\n> \n> I do not see why this macro is called MAX_ALLOC(); is there anything\n> \"alloc\" specific to what this does? You may happen to use it only\n> for \"alloc\" related things, but that is not a good reason for such a\n> name (if the implementation can only be used for \"alloc\" specific\n> purpose for some reason, then the name is appropriate).\n\nAbsolutely right. This macro won't be needed anyway, as you pointed out\nin the end of this review\n \n>> @@ -20,16 +23,47 @@ char strbuf_slopbuf[1];\n>> \n>> void strbuf_init(struct strbuf *sb, size_t hint)\n>> {\n>> +\tsb->owns_memory = 0;\n>> +\tsb->fixed_memory = 0;\n>> \tsb->alloc = sb->len = 0;\n>> \tsb->buf = strbuf_slopbuf;\n>> \tif (hint)\n>> \t\tstrbuf_grow(sb, hint);\n>> }\n> \n> I'll complain about _grow() again later, but if the implementation\n> is updated to address that complaint, slopbuf-using strbuf could\n> start it as a special case of \"starts as fixed_memory\".\n> \n>> +static void strbuf_wrap_internal(struct strbuf *sb, char *buf,\n>> +\t\t\t\t size_t buf_len, size_t alloc_len)\n>> +{\n>> +\tif (!buf)\n>> +\t\tdie(\"the buffer of a strbuf cannot be NULL\");\n>> +\n>> +\tstrbuf_release(sb);\n>> +\tsb->len = buf_len;\n>> +\tsb->alloc = alloc_len;\n>> +\tsb->buf = buf;\n>> +}\n>> +\n>> +void strbuf_wrap(struct strbuf *sb, char *buf,\n>> +\t\t size_t buf_len, size_t alloc_len)\n>> +{\n>> +\tstrbuf_wrap_internal(sb, buf, buf_len, alloc_len);\n>> +\tsb->owns_memory = 0;\n>> +\tsb->fixed_memory = 0;\n>> +}\n>>\n>> +void strbuf_wrap_fixed(struct strbuf *sb, char *buf,\n>> +\t\t size_t buf_len, size_t alloc_len)\n>> +{\n>> +\tstrbuf_wrap_internal(sb, buf, buf_len, alloc_len);\n>> +\tsb->owns_memory = 0;\n>> +\tsb->fixed_memory = 1;\n>> +}\n> \n> After seeing 2/3, I would say that the pretty side deserves the use\n> of word \"wrap\" more. What the above two does is an enhancement of\n> what strbuf API has called \"attach\". You are introducing a few new\n> and different ways to attach a piece of memory to a strbuf.\n\nThe naming is still debatable indeed. If there is a consensus on using\n\"attach\", fine with me.\nThat being said, I'm still hesitant on being able to initialize a strbuf\nwith an \"attach\" function, as the user shouldn't have to use\n`strbuf_init()` or `= STRBUF_INIT` beforhand. The users can't use\n`strbuf_attach()` with a non-initialized strbuf, is that a good idea to\nallow them to use `strbuf_attach_preallocated()` on a non-initialized\nbuffer?\n\n>> @@ -37,8 +71,13 @@ void strbuf_release(struct strbuf *sb)\n>> char *strbuf_detach(struct strbuf *sb, size_t *sz)\n>> {\n>> \tchar *res;\n>> +\n>> \tstrbuf_grow(sb, 0);\n>> -\tres = sb->buf;\n>> +\tif (sb->owns_memory)\n>> +\t\tres = sb->buf;\n>> +\telse\n>> +\t\tres = xmemdupz(sb->buf, sb->len);\n>> +\n>> \tif (sz)\n>> \t\t*sz = sb->len;\n>> \tstrbuf_init(sb, 0);\n> \n> Note that the user of the API does not have to care if the memory is\n> held by the strbuf or if the strbuf is merely borrowing it from the\n> initial allocation made from elsewhere (e.g. via strbuf_wrap_*) when\n> finalizing its use of the strbuf API to build the contents and\n> utilize the resulting buffer. Theoretically, if the caller knows it\n> let the piece memory _it_ owns (i.e. !owns_memory) and the piece of\n> memory it originally gave the strbuf hasn't been moved (i.e. your\n> fixed_memory, which I will complain again soon), the caller\n> shouldn't have to get a copy via xmemdupz(), which would both save\n> an allocation here and free() in the caller once it is done.\n> \n> But that is not done, and as the API it is a good thing. It frees\n> the caller from having to worry about _where_ the memory originally\n> came from.\n> \n>> void strbuf_grow(struct strbuf *sb, size_t extra)\n>> {\n>> -\tint new_buf = !sb->alloc;\n>> \tif (unsigned_add_overflows(extra, 1) ||\n>> \t unsigned_add_overflows(sb->len, extra + 1))\n>> \t\tdie(\"you want to use way too much memory\");\n>> -\tif (new_buf)\n>> -\t\tsb->buf = NULL;\n>> -\tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n>> -\tif (new_buf)\n>> -\t\tsb->buf[0] = '\\0';\n>> +\tif ((sb->fixed_memory) &&\n>> +\t sb->len + extra + 1 > sb->alloc)\n>> +\t\tdie(\"you try to overflow the buffer of a fixed strbuf\");\n> \n> Having praised how _detach() frees the user of the API from having\n> to worry about minute details of allocation, this is an eyesore. I\n> really think this \"fixed_memory\" support MUST GO. This \"die()\" here\n> makes the API useless _unless_ the caller always makes sure that any\n> operation it performs does not overflow the originally allocated\n> size, which makes it no better than using xsnprintf() and friends.\n\nI'm not a fan of this die() either.\n\n> A better design would be to get rid of fixed_memory bit and replace\n> it with \"owns_memory\" bit. Its meaning would be \"The ->buf pointer\n> points at something that is not owned by this strbuf, and must not\n> be freed when resizing or releasing ->buf.\"\n> \n> And when you detect that a strbuf with unfreeable memory needs to be\n> extended (the above condition you call \"die()\" on), you use the\n> \"strbuf doesn't own the buffer\" logic below.\n> \n> The only difference between your version and an improved one that\n> gets rid of \"fixed_memory\" from the API user's point of view is that\n> they must pre-allocate absolute maximum amount of memory expected to\n> be consumed with your version in order not to crash, while a \"start\n> with fixed but dynamically allocate from heap if needed\" approach\n> lets the API user preallocate just sufficient amount of memory\n> expected to be consumed in a typical workload and go without calling\n> malloc()/free() most of the time. Oh, and when a rare data comes in\n> that exceeds your initial estimate, \"fixed_memory\" version will just\n> die.\n> \n> So, as I said in the previous round, I really do not see a point in\n> this fixed_memory business. It does not make the implementation of\n> the API any easier as far as I can tell, either.\n\nI'm not sure to follow you. I agree that the \"fixed strbuf\" feature is\nflawed by the presence of this `die()`. But (unless misunderstanding)\nthe \"owns_memory\" bit you talk about does exist in this patch, and allow\nthe exact behavior you describe.\nThe patch allow to use a preallocated strbuf OR a preallocated-and-fixed\nstrbuf. Does the \"fixed\" part is useful, that's very debatable.\n\n>> +\t/*\n>> +\t * ALLOC_GROW may do a realloc() if needed, so we must not use it on\n>> +\t * a buffer the strbuf doesn't own\n>> +\t */\n>> +\tif (sb->owns_memory) {\n>> +\t\tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n>> +\t} else {\n>> +\t\t/*\n>> +\t\t * The strbuf doesn't own the buffer: to avoid to realloc it,\n>> +\t\t * the strbuf needs to use a new buffer without freeing the old\n>> +\t\t */\n>> +\t\tif (sb->len + extra + 1 > sb->alloc) {\n>> +\t\t\tsize_t new_alloc =\n>> +\t\t\t\tMAX_ALLOC(sb->len + extra + 1,\n>> +\t\t\t\t\t alloc_nr(sb->alloc));\n> \n> It is true that the you cannot simply realloc(->buf), but otherwise\n> the growth pattern of the buffer is the same between ->owns_memory\n> case and this case. And I think the above computes it like so, but\n> do you really need a one-shot macro MAX_ALLOC that is named in a\n> misleading way?\n> \n> I'd find it far easier to read if you wrote the core of ALLOC_GROW()\n> logic, like this:\n> \n> \tsize_t new_nr = sb->len + extra + 1;\n> \tif (new_nr > sb->alloc) {\n> \t\tsize_t new_alloc;\n> \t\tif (alloc_nr(sb->alloc) < new_nr)\n> \tnew_alloc = new_nr;\n> \t\telse\n> \t\t\tnew_alloc = alloc_nr(sb->alloc);\n> \t\t... copy and make it \"owned\" ...\n> \t}\n> \n> A better way would be to introduce a new macro ALLOC_GROW_COUNT() to\n> cache.h immediately before ALLOC_GROW() is defined and use it to\n> redo ALLOC_GROW(), perhaps like this:\n> \n> #define ALLOC_GROW_COUNT(nr, alloc) \\\n> ((alloc_nr(alloc) < (nr)) ? (nr) : alloc_nr(alloc))\n> \n> #define ALLOC_GROW(x, nr, alloc) \\\n> \tdo { \\\n> if ((nr) > alloc) { \\\n> \talloc = ALLOC_GROW_COUNT(nr, alloc); \\\n> \t\t\tREALLOC_ARRAY(x, alloc); \\\n> \t\t} \\\n> \t} while (0)\t\t\t\n> \n> Then you can do\n> \n> \tif (sb->len + extra + 1 > sb->alloc) {\n> \t\tsize_t new_alloc;\n> char *new_buf;\n> \n> \tnew_alloc = ALLOC_GROW_COUNT(sb->len + extra + 1, sb->alloc);\n> \t\tnew_buf = xmalloc(new_alloc);\n> \t\tmemcpy(...);\n> \t\t...\n\nThe second way seems better to me too, I don't know why I were hesitant\nto edit the ALLOC_GROW macro\n"},{"id":"288490","messageId":"xmqqfusquedk.fsf@gitster.mtv.corp.google.com","threadId":"42539","inReplyTo":"20160606203901.GA7667@Messiaen","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-06T22:44:07Z","receivedAt":"2016-06-06T22:44:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"William Duclot writes:\n\n> I'm not sure to follow you. I agree that the \"fixed strbuf\" feature is\n> flawed by the presence of this `die()`. But (unless misunderstanding)\n> the \"owns_memory\" bit you talk about does exist in this patch, and allow\n> the exact behavior you describe.\n\nImagine that I know most of my input lines are shorter than 80 bytes\nand definitely shorter than 128 bytes. I may want to say:\n\n\t/* allocate initial buffer ch[128] and attach it to line */\n\tstruct strbuf line = STRBUF_INIT_ON_STACK(128);\n\n\twhile (!strbuf_getline(&line, stdin)) {\n\t\t... use contents of &line ...\n\t}\n strbuf_release(&line);\n\nknowing that I won't waste too much cycles and memory from heap most\nof the time. Further imagine that one line in the input happened to\nbe 200 bytes long. After processing that line, the next call to\nstrbuf_getline() will call strbuf_reset(&line).\n\nI think that call should reset line.buf to the original buffer on\nthe stack, instead of saying \"Ok, I'll ignore the original memory\nnot owned by us and instead keep pointing at the allocated memory\",\nas the allocation was done as a fallback measure. The reason why\nstrbuf_reset() is different from strbuf_release() is because we\nanticipate that a buffer that is reset is going to be used again\nvery soon and want to avoid freeing only to call another alloc\nimmediately. We just set its logical length to 0 without shrinking\nwhat has already been allocated in the heap, because there is no\ngood hint that tells us what the optimal size of an empty\nbuffer in the particular caller that is waiting to be used before\nthis \"improve API\" effort.\n\nBut that \"lack of hint\" no longer is true with this \"imporve API\"\nthing. We _know_ that the caller thinks the anticipated typical\nworkload should fit well within the 128 bytes buffer that it\noriginally gave us. That is what I'd expect from an \"initialize a\nreasonably sized piece of memory on stack and use it most of the\ntime in strbuf\" feature.\n\n> The patch allow to use a preallocated strbuf OR a preallocated-and-fixed\n> strbuf. Does the \"fixed\" part is useful, that's very debatable.\n\n\"Fixed and non-extendable\" is useless; there is no room for\ndebating.\n"},{"id":"288491","messageId":"20160606225847.GA22756@sigill.intra.peff.net","threadId":"42539","inReplyTo":"xmqqfusquedk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-06T22:58:48Z","receivedAt":"2016-06-06T22:58:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 06, 2016 at 03:44:07PM -0700, Junio C Hamano wrote:\n\n> William Duclot writes:\n> \n> > I'm not sure to follow you. I agree that the \"fixed strbuf\" feature is\n> > flawed by the presence of this `die()`. But (unless misunderstanding)\n> > the \"owns_memory\" bit you talk about does exist in this patch, and allow\n> > the exact behavior you describe.\n> \n> Imagine that I know most of my input lines are shorter than 80 bytes\n> and definitely shorter than 128 bytes. I may want to say:\n> \n> \t/* allocate initial buffer ch[128] and attach it to line */\n> \tstruct strbuf line = STRBUF_INIT_ON_STACK(128);\n> \n> \twhile (!strbuf_getline(&line, stdin)) {\n> \t\t... use contents of &line ...\n> \t}\n> strbuf_release(&line);\n> \n> knowing that I won't waste too much cycles and memory from heap most\n> of the time. Further imagine that one line in the input happened to\n> be 200 bytes long. After processing that line, the next call to\n> strbuf_getline() will call strbuf_reset(&line).\n> \n> I think that call should reset line.buf to the original buffer on\n> the stack, instead of saying \"Ok, I'll ignore the original memory\n> not owned by us and instead keep pointing at the allocated memory\",\n> as the allocation was done as a fallback measure.\n\nI am not sure I agree. Do we think accessing the stack buffer is somehow\ncheaper than the heap buffer (perhaps because of cache effects)? If so,\nhow much cheaper?\n\nI think you can model reusing an already-allocated heap buffer as a\nhit/miss type of scenario. A \"hit\" means we see a larger-than-128 line\nand can avoid the allocation cost by reusing the heap buffer. A \"miss\"\nmeans the line is less than 128, and we pay the cost to use the heap\ninstead of the stack, whatever that is.\n\nMy suspicion is that the cost of a miss is essentially zero, so the best\nstrategy is to optimize for as many hits as possible (once the cost of\nthe initial allocation has been paid, though I am still not even\nconvinced that is a meaningful amount, especially in a loop like this\nwhere we can so easily reuse a heap buffer).\n\n-Peff\n"},{"id":"288495","messageId":"xmqqbn3dvr22.fsf@gitster.mtv.corp.google.com","threadId":"42539","inReplyTo":"20160606225847.GA22756@sigill.intra.peff.net","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-06T23:24:53Z","receivedAt":"2016-06-06T23:24:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> I think that call should reset line.buf to the original buffer on\n>> the stack, instead of saying \"Ok, I'll ignore the original memory\n>> not owned by us and instead keep pointing at the allocated memory\",\n>> as the allocation was done as a fallback measure.\n>\n> I am not sure I agree. Do we think accessing the stack buffer is somehow\n> cheaper than the heap buffer (perhaps because of cache effects)? If so,\n> how much cheaper?\n\nThis is not about stack vs heap or even \"cheaper\" (whatever your\ndefinition of cheap is). The principle applies equally if the\noriginal buffer came from BSS.\n\nPerhaps I made it clearer by using a more exaggerated example e.g. a\ntypical expected buffer size of 128 bytes, but the third line of\n1000 line input file was an anomaly that is 200k bytes long. I do\nnot want to keep that 200k bytes after finishing to process that\nthird line while using its initial 80 bytes for the remaining 977\nlines.\n\nBy the way, William seemed to be unhappy with die(), but I actually\nthink having a die() in the API may not be a bad thing if the check\nwere about something more sensible. For example, even if a strbuf\nthat can grow dynamically, capping the maximum size and say \"Hey\nthis is a one-lne-at-a-time text interface; if we need to grow the\nbuffer to 10MB, there is something wrong and a producer of such an\ninput does not even deserve a nice error message\" could be an\nentirely sensible attitude. But that does not mean an initial\nallocation should be 10MB. If the expected typical workload fits\nwithin a lot lower bound, starting from there and allowing it to\ngrow up to that maximum would be the more natural thing to do.\n\nAnd the problem I have with the proposed \"fixed\" is that it does not\nallow us to do that.\n"},{"id":"288497","messageId":"xmqq7fe1vr0c.fsf@gitster.mtv.corp.google.com","threadId":"42539","inReplyTo":"xmqqbn3dvr22.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-06T23:25:55Z","receivedAt":"2016-06-06T23:25:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>>> I think that call should reset line.buf to the original buffer on\n>>> the stack, instead of saying \"Ok, I'll ignore the original memory\n>>> not owned by us and instead keep pointing at the allocated memory\",\n>>> as the allocation was done as a fallback measure.\n>>\n>> I am not sure I agree. Do we think accessing the stack buffer is somehow\n>> cheaper than the heap buffer (perhaps because of cache effects)? If so,\n>> how much cheaper?\n>\n> This is not about stack vs heap or even \"cheaper\" (whatever your\n> definition of cheap is). The principle applies equally if the\n> original buffer came from BSS.\n>\n> Perhaps I made it clearer by using a more exaggerated example e.g. a\n\ns/I made/I should have made/; sorry but I have a horrible wifi\naround here today...\n\n> typical expected buffer size of 128 bytes, but the third line of\n> 1000 line input file was an anomaly that is 200k bytes long. I do\n> not want to keep that 200k bytes after finishing to process that\n> third line while using its initial 80 bytes for the remaining 977\n> lines.\n>\n> By the way, William seemed to be unhappy with die(), but I actually\n> think having a die() in the API may not be a bad thing if the check\n> were about something more sensible. For example, even if a strbuf\n> that can grow dynamically, capping the maximum size and say \"Hey\n> this is a one-lne-at-a-time text interface; if we need to grow the\n> buffer to 10MB, there is something wrong and a producer of such an\n> input does not even deserve a nice error message\" could be an\n> entirely sensible attitude. But that does not mean an initial\n> allocation should be 10MB. If the expected typical workload fits\n> within a lot lower bound, starting from there and allowing it to\n> grow up to that maximum would be the more natural thing to do.\n>\n> And the problem I have with the proposed \"fixed\" is that it does not\n> allow us to do that.\n"},{"id":"288498","messageId":"20160606233030.GB23537@sigill.intra.peff.net","threadId":"42539","inReplyTo":"xmqqbn3dvr22.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-06T23:30:30Z","receivedAt":"2016-06-06T23:30:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 06, 2016 at 04:24:53PM -0700, Junio C Hamano wrote:\n\n> This is not about stack vs heap or even \"cheaper\" (whatever your\n> definition of cheap is). The principle applies equally if the\n> original buffer came from BSS.\n> \n> Perhaps I made it clearer by using a more exaggerated example e.g. a\n> typical expected buffer size of 128 bytes, but the third line of\n> 1000 line input file was an anomaly that is 200k bytes long. I do\n> not want to keep that 200k bytes after finishing to process that\n> third line while using its initial 80 bytes for the remaining 977\n> lines.\n\nAh, I see. Yes, I can see that argument, though I'd counter that since\nwe _did_ see a 200k entry, perhaps the hint given in the code is not\nactually very sensible. Having seen one big line, might we expect to see\nmore?\n\nI dunno. I still feel like this whole thing is just micro-optimization\nthat is not even really going to be measurable outside of pathological\ncases.\n\n> By the way, William seemed to be unhappy with die(), but I actually\n> think having a die() in the API may not be a bad thing if the check\n> were about something more sensible. For example, even if a strbuf\n> that can grow dynamically, capping the maximum size and say \"Hey\n> this is a one-lne-at-a-time text interface; if we need to grow the\n> buffer to 10MB, there is something wrong and a producer of such an\n> input does not even deserve a nice error message\" could be an\n> entirely sensible attitude. But that does not mean an initial\n> allocation should be 10MB. If the expected typical workload fits\n> within a lot lower bound, starting from there and allowing it to\n> grow up to that maximum would be the more natural thing to do.\n\nYes, I very much agree that \"sensible limits\" should not be the same\nthing as \"initial allocated sizes\". It is easy to conflate the two when\nusing static buffers (because it lets you skip allocation entirely,\nwhich is much simpler!), but that usually leads to \"sensible limits\"\nbeing set way too low as a compromise.\n\n-Peff\n"},{"id":"288537","messageId":"alpine.DEB.2.20.1606071042290.28610@virtualbox","threadId":"42539","inReplyTo":"20160606151340.22424-2-william.duclot@ensimag.grenoble-inp.fr","subject":"Re: [PATCH V2 1/3] strbuf: add tests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-06-07T08:44:08Z","receivedAt":"2016-06-07T08:44:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi William,\n\nOn Mon, 6 Jun 2016, William Duclot wrote:\n\n> +\tenum {\n> +\t\tMODE_UNSPECIFIED = 0,\n> +\t\tMODE_BASIC_GROW ,\n> +\t\tMODE_STRBUF_CHECK,\n> +\t\tMODE_GROW_OVERFLOW\n> +\t} cmdmode = MODE_UNSPECIFIED;\n> +\tstruct option options[] = {\n> +\t\tOPT_CMDMODE(0, \"basic_grow\", &cmdmode,\n> +\t\t\t N_(\" basic grow test\"),\n> +\t\t\t MODE_BASIC_GROW),\n> +\t\tOPT_CMDMODE(0, \"strbuf_check_behavior\", &cmdmode,\n> +\t\t\t N_(\"check the strbuf's behavior\"),\n> +\t\t\t MODE_STRBUF_CHECK),\n> +\t\tOPT_CMDMODE(0, \"grow_overflow\", &cmdmode,\n> +\t\t\t N_(\"test grow expecting overflow\"),\n> +\t\t\t MODE_GROW_OVERFLOW),\n> +\t\tOPT_END()\n\nI found it to be really helpful to keep as much consistency as possible\n(Git's source code is complicated enough, no need to make it even more\ncomplicated).\n\nIn this particular instance, I would suggest to rename either\nMODE_STRBUF_CHECK or strbuf_check_behavior to match the other one.\n\nCiao,\nJohannes\n"},{"id":"288541","messageId":"alpine.DEB.2.20.1606071103190.28610@virtualbox","threadId":"42539","inReplyTo":"20160606151340.22424-3-william.duclot@ensimag.grenoble-inp.fr","subject":"Re: [PATCH V2 2/3] pretty.c: rename strbuf_wrap() function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-06-07T09:04:13Z","receivedAt":"2016-06-07T09:04:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi William,\n\nOn Mon, 6 Jun 2016, William Duclot wrote:\n\n> diff --git a/pretty.c b/pretty.c\n> index 87c4497..2b9e89a 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -904,8 +904,8 @@ static void parse_commit_message(struct format_commit_context *c)\n> \tc->commit_message_parsed = 1;\n> }\n> \n> -static void strbuf_wrap(struct strbuf *sb, size_t pos,\n> -\t\t\tsize_t width, size_t indent1, size_t indent2)\n> +static void strbuf_wrap_message(struct strbuf *sb, size_t pos,\n> +\t\t\t\tsize_t width, size_t indent1, size_t indent2)\n\nA better name might be strbuf_wrap_at_column().\n\nCiao,\nJohannes\n"},{"id":"288542","messageId":"20160607090653.GA4665@Messiaen","threadId":"42539","inReplyTo":"xmqqbn3dvr22.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"William Duclot","fromEmail":"william.duclot@ensimag.grenoble-inp.fr","sentAt":"2016-06-07T09:06:53Z","receivedAt":"2016-06-07T09:06:53Z","isPatch":true,"sender":{"key":"william.duclot@ensimag.grenoble-inp.fr","avatar":null},"body":"On Mon, Jun 06, 2016 at 04:24:53PM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>>> I think that call should reset line.buf to the original buffer on\n>>> the stack, instead of saying \"Ok, I'll ignore the original memory\n>>> not owned by us and instead keep pointing at the allocated memory\",\n>>> as the allocation was done as a fallback measure.\n>>\n>> I am not sure I agree. Do we think accessing the stack buffer is somehow\n>> cheaper than the heap buffer (perhaps because of cache effects)? If so,\n>> how much cheaper?\n> \n> This is not about stack vs heap or even \"cheaper\" (whatever your\n> definition of cheap is). The principle applies equally if the\n> original buffer came from BSS.\n> \n> Perhaps I made it clearer by using a more exaggerated example e.g. a\n> typical expected buffer size of 128 bytes, but the third line of\n> 1000 line input file was an anomaly that is 200k bytes long. I do\n> not want to keep that 200k bytes after finishing to process that\n> third line while using its initial 80 bytes for the remaining 977\n> lines.\n\n\"its initial 128 bytes\", rather than \"its initial 80 bytes\", no?\nOr else I'm lost :)\n \n> By the way, William seemed to be unhappy with die(), but I actually\n> think having a die() in the API may not be a bad thing if the check\n> were about something more sensible. For example, even if a strbuf\n> that can grow dynamically, capping the maximum size and say \"Hey\n> this is a one-lne-at-a-timve text interface; if we need to grow the\n> buffer to 10MB, there is something wrong and a producer of such an\n> input does not even deserve a nice error message\" could be an\n> entirely sensible attitude. But that does not mean an initial\n> allocation should be 10MB. If the expected typical workload fits\n> within a lot lower bound, starting from there and allowing it to\n> grow up to that maximum would be the more natural thing to do.\n> \n> And the problem I have with the proposed \"fixed\" is that it does not\n> allow us to do that.\n\nThe \"fixed\" feature was aimed to allow the users to use strbuf with\nstrings that they doesn't own themselves (a function parameter for\nexample). From Michael example in the original mail:\n\nvoid f(char *path_buf, size_t path_buf_len)\n{\n struct strbuf path;\n strbuf_wrap_fixed(&path, path_buf,\n strlen(path_buf),\n path_buf_len);\n ...\n /*\n * no strbuf_release() required here, but if called it\n * is a NOOP\n */\n}\n\nI don't have enough knowledge of the codebase to judge if this is\nuseful, you seem to think it's not.\n\nAbout this capping, I have troubles to understand if this is something\nyou'd like to see in this patch (assuming I include your changes)? Or is\nthis theoretical?\n\n\nTo sum up:\n* Rename \"wrap\" to \"attach\"\n* Forget about this \"fixed\" feature\n* Make the strbuf reuse the preallocated buffer after a reset() (and a\ndetach() probably?)\n* Introduce a more practical macro STRBUF_INIT_ON_STACK() (maybe the\nname is too technical?)\n* A few code corrections\n"},{"id":"288627","messageId":"xmqqziqwq395.fsf@gitster.mtv.corp.google.com","threadId":"42539","inReplyTo":"20160607090653.GA4665@Messiaen","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-07T18:10:14Z","receivedAt":"2016-06-07T18:10:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"William Duclot writes:\n\n>> Perhaps I made it clearer by using a more exaggerated example e.g. a\n>> typical expected buffer size of 128 bytes, but the third line of\n>> 1000 line input file was an anomaly that is 200k bytes long. I do\n>> not want to keep that 200k bytes after finishing to process that\n>> third line while using its initial 80 bytes for the remaining 977\n>> lines.\n>\n> \"its initial 128 bytes\", rather than \"its initial 80 bytes\", no?\n\nEither way would work, but 80 is closer to what I had in mind, as\nthe set-up of the example is \"I know 99% of my input will fit within\n80, but I'll allocate 128 to avoid realloc cost when there are rare\nones that bust 80. I do not want to die only because there is an\noccasional oddball that needs 200\".\n\n> The \"fixed\" feature was aimed to allow the users to use strbuf with\n> strings that they doesn't own themselves (a function parameter for\n> example). From Michael example in the original mail:\n>\n> void f(char *path_buf, size_t path_buf_len)\n> {\n> struct strbuf path;\n> strbuf_wrap_fixed(&path, path_buf,\n> strlen(path_buf),\n> path_buf_len);\n> ...\n> /*\n> * no strbuf_release() required here, but if called it\n> * is a NOOP\n> */\n> }\n\nThink what does the \"...\" part would do using the \"path\" strbuf.\n\nIf 'f()' is meant to take the \"dying is perfectly fine if the data\nwe have to process exceeds the area we were given even by one byte\"\nattitude, then the \"capped to the same length as allocated\" is\nperfectly fine, but if 'f()' cannot afford to die() and instead has\nto signal an error condition to its caller, then this function has\nto check the length currently in use (i.e. path.len) and how much\nmore memory it can still use, before making each call to strbuf_*()\nfunctions, no?\n\nIf we were to add \"fixed\" feature, we'd want to see it to help a\nfunction like f() that cannot afford to die() and does not want to\nmalloc()/realloc(). I do not think what was in this series was it.\n"},{"id":"288730","messageId":"575845D9.2010604@alum.mit.edu","threadId":"42539","inReplyTo":"20160607090653.GA4665@Messiaen","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-06-08T16:20:41Z","receivedAt":"2016-06-08T16:20:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/07/2016 11:06 AM, William Duclot wrote:\n> [...]\n> The \"fixed\" feature was aimed to allow the users to use strbuf with\n> strings that they doesn't own themselves (a function parameter for\n> example). From Michael example in the original mail:\n> \n> void f(char *path_buf, size_t path_buf_len)\n> {\n> struct strbuf path;\n> strbuf_wrap_fixed(&path, path_buf,\n> strlen(path_buf),\n> path_buf_len);\n> ...\n\nI also thought that \"fixed\" strbufs would be useful in cases where you\n*know* what size string you need and only want a strbuf wrapper because\nit offers a lot of convenience functions. Nowadays we would do that\nusing something like\n\n> static int feed_object(const unsigned char *sha1, int fd, int negative)\n> {\n> \tchar buf[42];\n> \n> \tif (negative && !has_sha1_file(sha1))\n> \t\treturn 1;\n> \n> \tmemcpy(buf + negative, sha1_to_hex(sha1), 40);\n> \tif (negative)\n> \t\tbuf[0] = '^';\n> \tbuf[40 + negative] = '\\n';\n> \treturn write_or_whine(fd, buf, 41 + negative, \"send-pack: send refs\");\n> }\n\nInstead, one could write\n\n> static int feed_object(const unsigned char *sha1, int fd, int negative)\n> {\n> \tchar buf[GIT_SHA1_HEXSZ + 2];\n> \tstruct strbuf line = WRAPPED_FIXED_STRBUF(buf);\n> \n> \tif (negative && !has_sha1_file(sha1))\n> \t\treturn 1;\n> \n> \tif (negative)\n> \t\tstrbuf_addch(&line, '^');\n> \tstrbuf_add(&line, sha1_to_hex(sha1), GIT_SHA1_HEXSZ);\n> \tstrbuf_addch(&line, '\\n');\n> \treturn write_or_whine(fd, line.buf, line.len, \"send-pack: send refs\");\n> }\n\n* It's a little less manual bookkeeping, and thus less error-prone,\n than the current code.\n\n* If somebody decides to add another character to the line but\n forgets to increase the allocation size, the code dies in testing\n rather than (a) overflowing the buffer, like the current\n code, or (b) silently becoming less performant, as if it used a\n preallocated but non-fixed strbuf.\n\n* There's no need to strbuf_release() (which can be convenient in\n a function with multiple exit paths).\n\nI don't know whether this particular function should be rewritten; I'm\njust giving an example of the type of scenario where I think it could be\nuseful.\n\nIn a world without fixed strbufs, what would one use in this situation?\n\nMichael\n"},{"id":"288747","messageId":"xmqqziqvlfk6.fsf@gitster.mtv.corp.google.com","threadId":"42539","inReplyTo":"575845D9.2010604@alum.mit.edu","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-08T18:07:53Z","receivedAt":"2016-06-08T18:07:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> * It's a little less manual bookkeeping, and thus less error-prone,\n> than the current code.\n>\n> * If somebody decides to add another character to the line but\n> forgets to increase the allocation size, the code dies in testing\n> rather than (a) overflowing the buffer, like the current\n> code, or (b) silently becoming less performant, as if it used a\n> preallocated but non-fixed strbuf.\n\nYeah, thanks. A relization of pretty much the same came to me after\nI wrote my last message on this topic. The above two are\nattractive.\n"},{"id":"288756","messageId":"20160608191918.GB19572@sigill.intra.peff.net","threadId":"42539","inReplyTo":"575845D9.2010604@alum.mit.edu","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-08T19:19:18Z","receivedAt":"2016-06-08T19:19:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 08, 2016 at 06:20:41PM +0200, Michael Haggerty wrote:\n\n> Instead, one could write\n> \n> > static int feed_object(const unsigned char *sha1, int fd, int negative)\n> > {\n> > \tchar buf[GIT_SHA1_HEXSZ + 2];\n> > \tstruct strbuf line = WRAPPED_FIXED_STRBUF(buf);\n> > \n> > \tif (negative && !has_sha1_file(sha1))\n> > \t\treturn 1;\n> > \n> > \tif (negative)\n> > \t\tstrbuf_addch(&line, '^');\n> > \tstrbuf_add(&line, sha1_to_hex(sha1), GIT_SHA1_HEXSZ);\n> > \tstrbuf_addch(&line, '\\n');\n> > \treturn write_or_whine(fd, line.buf, line.len, \"send-pack: send refs\");\n> > }\n\nHmm. I'm not sure that just replacing that with a regular heap-allocated\nstrbuf is so bad. It additionally gets rid of the SHA1_HEXSZ math in the\nallocation.\n\nSo from your list of advantages:\n\n> * It's a little less manual bookkeeping, and thus less error-prone,\n> than the current code.\n\nWe have this, but better.\n\n> * If somebody decides to add another character to the line but\n> forgets to increase the allocation size, the code dies in testing\n> rather than (a) overflowing the buffer, like the current\n> code, or (b) silently becoming less performant, as if it used a\n> preallocated but non-fixed strbuf.\n\nInstead of overflowing, it just silently works.\n\n> * There's no need to strbuf_release() (which can be convenient in\n> a function with multiple exit paths).\n\nSame.\n\nThe downside, obviously, is the cost of malloc/free. It may even be\nnoticeable here here because this really is a tight loop of strbuf\nallocation (OTOH, we immediately make a syscall; how expensive is\nwrite() compared to malloc()?).\n\nWe can hack around that by reusing the same strbuf.\n\nUnfortunately the usual trick of:\n\n struct strbuf buf = STRBUF_INIT;\n for (...) {\n\tstrbuf_reset(&buf);\n\t...\n }\n strbuf_release(&buf);\n\ndoes not work, because we are in a sub-function. We can pass in the\nbuffer as scratch space, but that makes the function interface a little\nuglier than it needs to be.\n\nLikewise, we could make the strbuf static inside feed_object(). It's\nnot so bad here, where we know there aren't re-entrancy issues, but it's\nnot a very safe pattern in general (and it leaks the memory when we're\ndone with the function).\n\nThat made me wonder if we could repeatedly reuse a buffer attached to\nthe file descriptor. And indeed, isn't that what stdio is? The whole\nreason this buffer exists is because we are using a direct descriptor\nwrite. If we switched this function to use fprintf(), we'd avoid the\nwhole buffer question, have a fixed cap on our memory use (since we just\nflush anytime the buffer is full) _and_ we'd reduce the number of\nwrite syscalls we're making by almost a factor of 100.\n\n> I don't know whether this particular function should be rewritten; I'm\n> just giving an example of the type of scenario where I think it could be\n> useful.\n>\n> In a world without fixed strbufs, what would one use in this situation?\n\nI know I've done the exact opposite of what you wanted here and talked\nabout this specific function. But I _do_ think this is a pattern I've\nseen several times, where we format into a buffer only to write() it\nout. I think they may comprise a reasonable number of our buffer-using\nloops.\n\n-Peff\n"},{"id":"288766","messageId":"20160608194216.GA3731@sigill.intra.peff.net","threadId":"42539","inReplyTo":"20160608191918.GB19572@sigill.intra.peff.net","subject":"[PATCH] send-pack: use buffered I/O to talk to pack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-08T19:42:16Z","receivedAt":"2016-06-08T19:42:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 08, 2016 at 03:19:18PM -0400, Jeff King wrote:\n\n> That made me wonder if we could repeatedly reuse a buffer attached to\n> the file descriptor. And indeed, isn't that what stdio is? The whole\n> reason this buffer exists is because we are using a direct descriptor\n> write. If we switched this function to use fprintf(), we'd avoid the\n> whole buffer question, have a fixed cap on our memory use (since we just\n> flush anytime the buffer is full) _and_ we'd reduce the number of\n> write syscalls we're making by almost a factor of 100.\n\nSo all of this strbuf discussion aside, I think it is worth doing\nsomething like this for this particular case.\n\n-- >8 --\nSubject: send-pack: use buffered I/O to talk to pack-objects\n\nWe start a pack-objects process and then write all of the\npositive and negative sha1s to it over a pipe. We do so by\nformatting each item into a fixed-size buffer and then\nwriting each individually. This has two drawbacks:\n\n 1. There's some manual computation of the buffer size,\n which is not immediately obvious is correct (though it\n is).\n\n 2. We write() once per sha1, which means a lot more system\n calls than are necessary.\n\nWe can solve both by wrapping the pipe descriptor in a stdio\nhandle; this is the same technique used by upload-pack when\nserving fetches.\n\nNote that we can also simplify and improve the error\nhandling here. The original detected a single write error\nand broke out of the loop (presumably to avoid writing the\nerror message over and over), but never actually acted on\nseeing an error; we just fed truncated input and took\nwhatever pack-objects returned.\n\nIn practice, this probably didn't matter, as the likely\nerrors would be caused by pack-objects dying (and we'd\nprobably just die with SIGPIPE anyway). But we can easily\nmake this simpler and more robust; the stdio handle keeps an\nerror flag, which we can check at the end.\n\nSigned-off-by: Jeff King \n---\n send-pack.c | 33 ++++++++++++++++-----------------\n 1 file changed, 16 insertions(+), 17 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 37ee04e..299d303 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -36,18 +36,15 @@ int option_parse_push_signed(const struct option *opt,\n \tdie(\"bad %s argument: %s\", opt->long_name, arg);\n }\n \n-static int feed_object(const unsigned char *sha1, int fd, int negative)\n+static void feed_object(const unsigned char *sha1, FILE *fh, int negative)\n {\n-\tchar buf[42];\n-\n \tif (negative && !has_sha1_file(sha1))\n-\t\treturn 1;\n+\t\treturn;\n \n-\tmemcpy(buf + negative, sha1_to_hex(sha1), 40);\n \tif (negative)\n-\t\tbuf[0] = '^';\n-\tbuf[40 + negative] = '\\n';\n-\treturn write_or_whine(fd, buf, 41 + negative, \"send-pack: send refs\");\n+\t\tputc('^', fh);\n+\tfputs(sha1_to_hex(sha1), fh);\n+\tputc('\\n', fh);\n }\n \n /*\n@@ -73,6 +70,7 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru\n \t\tNULL,\n \t};\n \tstruct child_process po = CHILD_PROCESS_INIT;\n+\tFILE *po_in;\n \tint i;\n \n \ti = 4;\n@@ -97,21 +95,22 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru\n \t * We feed the pack-objects we just spawned with revision\n \t * parameters by writing to the pipe.\n \t */\n+\tpo_in = xfdopen(po.in, \"w\");\n \tfor (i = 0; i < extra->nr; i++)\n-\t\tif (!feed_object(extra->sha1[i], po.in, 1))\n-\t\t\tbreak;\n+\t\tfeed_object(extra->sha1[i], po_in, 1);\n \n \twhile (refs) {\n-\t\tif (!is_null_oid(&refs->old_oid) &&\n-\t\t !feed_object(refs->old_oid.hash, po.in, 1))\n-\t\t\tbreak;\n-\t\tif (!is_null_oid(&refs->new_oid) &&\n-\t\t !feed_object(refs->new_oid.hash, po.in, 0))\n-\t\t\tbreak;\n+\t\tif (!is_null_oid(&refs->old_oid))\n+\t\t\tfeed_object(refs->old_oid.hash, po_in, 1);\n+\t\tif (!is_null_oid(&refs->new_oid))\n+\t\t\tfeed_object(refs->new_oid.hash, po_in, 0);\n \t\trefs = refs->next;\n \t}\n \n-\tclose(po.in);\n+\tfflush(po_in);\n+\tif (ferror(po_in))\n+\t\tdie_errno(\"error writing to pack-objects\");\n+\tfclose(po_in);\n \n \tif (args->stateless_rpc) {\n \t\tchar *buf = xmalloc(LARGE_PACKET_MAX);\n-- \n2.9.0.rc2.138.geb72a36\n"},{"id":"288768","messageId":"xmqq37onlawb.fsf@gitster.mtv.corp.google.com","threadId":"42539","inReplyTo":"20160608191918.GB19572@sigill.intra.peff.net","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-08T19:48:36Z","receivedAt":"2016-06-08T19:48:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That made me wonder if we could repeatedly reuse a buffer attached to\n> the file descriptor. And indeed, isn't that what stdio is? The whole\n> reason this buffer exists is because we are using a direct descriptor\n> write. If we switched this function to use fprintf(), we'd avoid the\n> whole buffer question, have a fixed cap on our memory use (since we just\n> flush anytime the buffer is full) _and_ we'd reduce the number of\n> write syscalls we're making by almost a factor of 100.\n\nThe primary reason why we avoid stdio in the lower level part of I/O\nis that the error reporting and handling is horrible.\n\ne.g. c.f. http://article.gmane.org/gmane.comp.version-control.git/27019\n\nOtherwise, I'd agree with your \"Why aren't we using stdio if\ncounting and avoiding overflow is so hard?\".\n"},{"id":"288769","messageId":"20160608195248.GA4264@sigill.intra.peff.net","threadId":"42539","inReplyTo":"xmqq37onlawb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-08T19:52:48Z","receivedAt":"2016-06-08T19:52:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 08, 2016 at 12:48:36PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > That made me wonder if we could repeatedly reuse a buffer attached to\n> > the file descriptor. And indeed, isn't that what stdio is? The whole\n> > reason this buffer exists is because we are using a direct descriptor\n> > write. If we switched this function to use fprintf(), we'd avoid the\n> > whole buffer question, have a fixed cap on our memory use (since we just\n> > flush anytime the buffer is full) _and_ we'd reduce the number of\n> > write syscalls we're making by almost a factor of 100.\n> \n> The primary reason why we avoid stdio in the lower level part of I/O\n> is that the error reporting and handling is horrible.\n> \n> e.g. c.f. http://article.gmane.org/gmane.comp.version-control.git/27019\n> \n> Otherwise, I'd agree with your \"Why aren't we using stdio if\n> counting and avoiding overflow is so hard?\".\n\nI agree it can be confusing (especially on the output side your errors\nare likely deferred until the next flush). But in this particular case,\nI think it's an improvement (see the patch I just sent and its\ndiscussion of error handling).\n\nI also think we could smooth over the rough edges by wrapping the\ncomplexity (we already have fprintf_or_die, which arguably could be used\nin this case, but I went with the solution that stayed closer to what\nthe original code was doing). And if stdio is truly too horrible, I'd\nsuggest we implement our own buffered I/O. Because that's effectively\nwhat such call-sites are doing, but in a really ad-hoc and\nnon-performant way.\n\n-Peff\n"},{"id":"288779","messageId":"xmqqtwh3jn89.fsf@gitster.mtv.corp.google.com","threadId":"42539","inReplyTo":"20160608195248.GA4264@sigill.intra.peff.net","subject":"Re: [PATCH V2 3/3] strbuf: allow to use preallocated memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-08T23:05:10Z","receivedAt":"2016-06-08T23:05:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I agree it can be confusing (especially on the output side your errors\n> are likely deferred until the next flush). But in this particular case,\n> I think it's an improvement (see the patch I just sent and its\n> discussion of error handling).\n\nYes, for this one, the lifetime of the file descriptor is contained\nwithin a single function and it is straight-forward to wrap it\ncorrectly inside a FILE * via fdopen() and do the error check at the\nend, so I think the patch is an improvement.\n\nWill queue.\n"},{"id":"288803","messageId":"vpqwplypnpr.fsf@anie.imag.fr","threadId":"42539","inReplyTo":"20160608194216.GA3731@sigill.intra.peff.net","subject":"Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-06-09T12:10:24Z","receivedAt":"2016-06-16T02:19:49Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King writes:\n\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -36,18 +36,15 @@ int option_parse_push_signed(const struct option *opt,\n> \tdie(\"bad %s argument: %s\", opt->long_name, arg);\n> }\n> \n> -static int feed_object(const unsigned char *sha1, int fd, int negative)\n> +static void feed_object(const unsigned char *sha1, FILE *fh, int negative)\n> {\n> -\tchar buf[42];\n> -\n> \tif (negative && !has_sha1_file(sha1))\n> -\t\treturn 1;\n> +\t\treturn;\n[...]\n> @@ -97,21 +95,22 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru\n[...]\n> \tfor (i = 0; i < extra->nr; i++)\n> -\t\tif (!feed_object(extra->sha1[i], po.in, 1))\n> -\t\t\tbreak;\n> +\t\tfeed_object(extra->sha1[i], po_in, 1);\n\nI may have missed the obvious, but doesn't this change the behavior when\n\"negative && !has_sha1_file(sha1)\" happens? I understand that you don't\nneed write_or_whine anymore, but don't understand how you get rid of the\n\"return 1\" here.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"288809","messageId":"57597E93.9040808@ramsayjones.plus.com","threadId":"42539","inReplyTo":"vpqwplypnpr.fsf@anie.imag.fr","subject":"Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-06-09T14:34:59Z","receivedAt":"2016-06-16T02:19:49Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 09/06/16 13:10, Matthieu Moy wrote:\n> Jeff King writes:\n> \n>> --- a/send-pack.c\n>> +++ b/send-pack.c\n>> @@ -36,18 +36,15 @@ int option_parse_push_signed(const struct option *opt,\n>> \tdie(\"bad %s argument: %s\", opt->long_name, arg);\n>> }\n>> \n>> -static int feed_object(const unsigned char *sha1, int fd, int negative)\n>> +static void feed_object(const unsigned char *sha1, FILE *fh, int negative)\n>> {\n>> -\tchar buf[42];\n>> -\n>> \tif (negative && !has_sha1_file(sha1))\n>> -\t\treturn 1;\n>> +\t\treturn;\n> [...]\n>> @@ -97,21 +95,22 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru\n> [...]\n>> \tfor (i = 0; i < extra->nr; i++)\n>> -\t\tif (!feed_object(extra->sha1[i], po.in, 1))\n>> -\t\t\tbreak;\n>> +\t\tfeed_object(extra->sha1[i], po_in, 1);\n> \n> I may have missed the obvious, but doesn't this change the behavior when\n> \"negative && !has_sha1_file(sha1)\" happens? I understand that you don't\n> need write_or_whine anymore, but don't understand how you get rid of the\n> \"return 1\" here.\n\nJust FYI, this patch removes the last use of write_or_whine() - should it\nbe removed?\n\nATB,\nRamsay Jones\n"},{"id":"288822","messageId":"xmqqr3c6iad1.fsf@gitster.mtv.corp.google.com","threadId":"42539","inReplyTo":"vpqwplypnpr.fsf@anie.imag.fr","subject":"Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-09T16:40:42Z","receivedAt":"2016-06-16T02:19:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Jeff King writes:\n>\n>> --- a/send-pack.c\n>> +++ b/send-pack.c\n>> @@ -36,18 +36,15 @@ int option_parse_push_signed(const struct option *opt,\n>> \tdie(\"bad %s argument: %s\", opt->long_name, arg);\n>> }\n>> \n>> -static int feed_object(const unsigned char *sha1, int fd, int negative)\n>> +static void feed_object(const unsigned char *sha1, FILE *fh, int negative)\n>> {\n>> -\tchar buf[42];\n>> -\n>> \tif (negative && !has_sha1_file(sha1))\n>> -\t\treturn 1;\n>> +\t\treturn;\n> [...]\n>> @@ -97,21 +95,22 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru\n> [...]\n>> \tfor (i = 0; i < extra->nr; i++)\n>> -\t\tif (!feed_object(extra->sha1[i], po.in, 1))\n>> -\t\t\tbreak;\n>> +\t\tfeed_object(extra->sha1[i], po_in, 1);\n>\n> I may have missed the obvious, but doesn't this change the behavior when\n> \"negative && !has_sha1_file(sha1)\" happens? I understand that you don't\n> need write_or_whine anymore, but don't understand how you get rid of the\n> \"return 1\" here.\n\nThe original feed_object() has somewhat strange interface in that a\nnon-zero return from it is \"Everything went alright!\", and zero\nmeans \"Oops, something went wrong\". When the function actually\nwrites things out, it calls write_or_whine(), whose return value\nalso uses that (unusual) convention, which is the reason why it\nbehaves that way.\n\nThe \"return 1\" you noticed happens when the function is told not to\nsend history behind one object, but that object does not exist in\nour repository. This is a perfectly normal condition and the\nfunction just ignores it and returns without feeding it to\npack-objects. It reports to the caller \"Everything went alright!\".\n\nThe original caller checks for errors to break out the feeding of\nthe process early, with things like:\n\n\tif (!feed_object(...))\n \tbreak;\n\nIOW, the caller would have continued when hitting that \"return 1\"\ncodepath.\n\nAnd the code with the patch, the caller continues unconditionally,\nso there is no behaviour change, if I am reading the code correctly.\n\nThanks for carefully reading the change and pointing out hard-to-read\nparts, as always.\n"},{"id":"288825","messageId":"20160609171246.GA9016@sigill.intra.peff.net","threadId":"42539","inReplyTo":"57597E93.9040808@ramsayjones.plus.com","subject":"Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-09T17:12:47Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 09, 2016 at 03:34:59PM +0100, Ramsay Jones wrote:\n\n> Just FYI, this patch removes the last use of write_or_whine() - should it\n> be removed?\n\nThat sounds reasonable. Want to do a patch on top?\n\n-Peff\n"},{"id":"288826","messageId":"20160609171429.GB9016@sigill.intra.peff.net","threadId":"42539","inReplyTo":"xmqqr3c6iad1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-09T17:14:29Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 09, 2016 at 09:40:42AM -0700, Junio C Hamano wrote:\n\n> >> \tfor (i = 0; i < extra->nr; i++)\n> >> -\t\tif (!feed_object(extra->sha1[i], po.in, 1))\n> >> -\t\t\tbreak;\n> >> +\t\tfeed_object(extra->sha1[i], po_in, 1);\n> >\n> > I may have missed the obvious, but doesn't this change the behavior when\n> > \"negative && !has_sha1_file(sha1)\" happens? I understand that you don't\n> > need write_or_whine anymore, but don't understand how you get rid of the\n> > \"return 1\" here.\n> [...]\n> The original caller checks for errors to break out the feeding of\n> the process early, with things like:\n> \n> \tif (!feed_object(...))\n> \tbreak;\n> \n> IOW, the caller would have continued when hitting that \"return 1\"\n> codepath.\n> \n> And the code with the patch, the caller continues unconditionally,\n> so there is no behaviour change, if I am reading the code correctly.\n\nRight, that's my reading as well (and IMHO another good motivation for\nthe patch, if it makes this all less confusing).\n\n-Peff\n"},{"id":"288828","messageId":"vpqeg86cm5u.fsf@anie.imag.fr","threadId":"42539","inReplyTo":"xmqqr3c6iad1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-06-09T17:22:21Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>> Jeff King writes:\n>>\n>>> --- a/send-pack.c\n>>> +++ b/send-pack.c\n>>> @@ -36,18 +36,15 @@ int option_parse_push_signed(const struct option *opt,\n>>> \tdie(\"bad %s argument: %s\", opt->long_name, arg);\n>>> }\n>>> \n>>> -static int feed_object(const unsigned char *sha1, int fd, int negative)\n>>> +static void feed_object(const unsigned char *sha1, FILE *fh, int negative)\n>>> {\n>>> -\tchar buf[42];\n>>> -\n>>> \tif (negative && !has_sha1_file(sha1))\n>>> -\t\treturn 1;\n>>> +\t\treturn;\n>> [...]\n>>> @@ -97,21 +95,22 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru\n>> [...]\n>>> \tfor (i = 0; i < extra->nr; i++)\n>>> -\t\tif (!feed_object(extra->sha1[i], po.in, 1))\n>>> -\t\t\tbreak;\n>>> +\t\tfeed_object(extra->sha1[i], po_in, 1);\n>>\n>> I may have missed the obvious, but doesn't this change the behavior when\n>> \"negative && !has_sha1_file(sha1)\" happens? I understand that you don't\n>> need write_or_whine anymore, but don't understand how you get rid of the\n>> \"return 1\" here.\n>\n> The original feed_object() has somewhat strange interface in that a\n> non-zero return from it is \"Everything went alright!\", and zero\n> means \"Oops, something went wrong\".\n\nIndeed, this is the \"obvious\" I've missed. So, indeed, the new \"return\"\ndoes the same thing as the old \"return 1\".\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"288857","messageId":"5759F048.7040302@ramsayjones.plus.com","threadId":"42539","inReplyTo":"20160609171246.GA9016@sigill.intra.peff.net","subject":"Re: [PATCH] send-pack: use buffered I/O to talk to pack-objects","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-06-09T22:40:08Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 09/06/16 18:12, Jeff King wrote:\n> On Thu, Jun 09, 2016 at 03:34:59PM +0100, Ramsay Jones wrote:\n> \n>> Just FYI, this patch removes the last use of write_or_whine() - should it\n>> be removed?\n> \n> That sounds reasonable. Want to do a patch on top?\n\nOK, will do.\n\nATB,\nRamsay Jones\n"}]}