{"thread":{"id":"50389","subject":"[PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf","startedAt":"2019-02-03T16:51:59Z","lastAt":"2019-02-06T18:12:02Z","messageCount":13,"participants":["Johannes Sixt","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"368439","messageId":"896ae9dd-7ac3-182e-6692-c09bc4864de0@kdbg.org","threadId":"50389","inReplyTo":null,"subject":"[PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-02-03T16:51:54Z","receivedAt":"2019-02-03T16:51:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"strbuf_vinsertf inserts a formatted string in the middle of an existing\nstrbuf value. It makes room in the strbuf by moving existing string to\nthe back, then formats the string to insert directly into the hole.\n\nIt uses vsnprintf to format the string. The buffer size provided in the\ninvocation is the number of characters available in the allocated space\nbehind the final string. This does not make any sense at all.\n\nFix it to pass the length of the inserted string plus one for the NUL.\n(The functions saves and restores the character that the NUL occupies.)\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n I found this, because in my environment I have to compile with\n SNPRINTF_RETURNS_BOGUS. Our implementation of vsnprintf in\n compat/snprintf.c writes into the end of the buffer unconditionally,\n at a spot that is unrelated to the formatted string, and this leads to\n \"BUG: a NUL byte in commit log message not allowed\" in some \"stash\"\n tests.\n\n strbuf.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex bfbbdadbf3..87ecf7f975 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -270,7 +270,7 @@ void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt, va_list ap)\n \tmemmove(sb->buf + pos + len, sb->buf + pos, sb->len - pos);\n \t/* vsnprintf() will append a NUL, overwriting one of our characters */\n \tsave = sb->buf[pos + len];\n-\tlen2 = vsnprintf(sb->buf + pos, sb->alloc - sb->len, fmt, ap);\n+\tlen2 = vsnprintf(sb->buf + pos, len + 1, fmt, ap);\n \tsb->buf[pos + len] = save;\n \tif (len2 != len)\n \t\tBUG(\"your vsnprintf is broken (returns inconsistent lengths)\");\n-- \n2.20.1.86.gb0de946387\n"},{"id":"368445","messageId":"5d521649-0b21-04e3-3182-e8714fcbfeac@kdbg.org","threadId":"50389","inReplyTo":"896ae9dd-7ac3-182e-6692-c09bc4864de0@kdbg.org","subject":"Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-02-04T07:25:38Z","receivedAt":"2019-02-04T07:25:45Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 03.02.19 um 17:51 schrieb Johannes Sixt:\n> strbuf_vinsertf inserts a formatted string in the middle of an existing\n> strbuf value.\n\nQuite frankly, this is a really unusual operation, and I'd prefer to get\nrid of it. There is only one call, and it looks like it only wants to be\nlazy and save one strbuf variable.\n\nThis helper adds way more code than a non-lazy caller would need. There\nwouldn't even be a mental burden. Like this (except that strbuf_addstr\ndoesn't do what I thought it would do...).\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 74e6ff62b5..95d202aea3 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1101,7 +1101,7 @@ static int stash_working_tree(struct stash_info *info, struct pathspec ps)\n \treturn ret;\n }\n \n-static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,\n+static int do_create_stash(struct pathspec ps, const char *stash_msg,\n \t\t\t   int include_untracked, int patch_mode,\n \t\t\t   struct stash_info *info, struct strbuf *patch,\n \t\t\t   int quiet)\n@@ -1117,6 +1117,7 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,\n \tstruct strbuf msg = STRBUF_INIT;\n \tstruct strbuf commit_tree_label = STRBUF_INIT;\n \tstruct strbuf untracked_files = STRBUF_INIT;\n+\tstruct strbuf stash_msg_buf = STRBUF_INIT;\n \n \tprepare_fallback_ident(\"git stash\", \"git@stash\");\n \n@@ -1188,10 +1189,12 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,\n \t\t}\n \t}\n \n-\tif (!stash_msg_buf->len)\n-\t\tstrbuf_addf(stash_msg_buf, \"WIP on %s\", msg.buf);\n-\telse\n-\t\tstrbuf_insertf(stash_msg_buf, 0, \"On %s: \", branch_name);\n+\tif (!*stash_msg) {\n+\t\tstrbuf_addf(&stash_msg_buf, \"WIP on %s\", msg.buf);\n+\t} else {\n+\t\tstrbuf_addf(&stash_msg_buf, \"On %s: \", branch_name);\n+\t\tstrbuf_addstr(&stash_msg_buf, stash_msg);\n+\t}\n \n \t/*\n \t * `parents` will be empty after calling `commit_tree()`, so there is\n@@ -1206,7 +1209,7 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,\n \t\t\t   &parents);\n \tcommit_list_insert(head_commit, &parents);\n \n-\tif (commit_tree(stash_msg_buf->buf, stash_msg_buf->len, &info->w_tree,\n+\tif (commit_tree(stash_msg_buf.buf, stash_msg_buf.len, &info->w_tree,\n \t\t\tparents, &info->w_commit, NULL, NULL)) {\n \t\tif (!quiet)\n \t\t\tfprintf_ln(stderr, _(\"Cannot record \"\n@@ -1216,6 +1219,7 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,\n \t}\n \n done:\n+\tstrbuf_release(&stash_msg_buf);\n \tstrbuf_release(&commit_tree_label);\n \tstrbuf_release(&msg);\n \tstrbuf_release(&untracked_files);\n@@ -1236,7 +1240,7 @@ static int create_stash(int argc, const char **argv, const char *prefix)\n \tif (!check_changes_tracked_files(ps))\n \t\treturn 0;\n \n-\tif (!(ret = do_create_stash(ps, &stash_msg_buf, 0, 0, &info, NULL, 0)))\n+\tif (!(ret = do_create_stash(ps, stash_msg_buf.buf, 0, 0, &info, NULL, 0)))\n \t\tprintf_ln(\"%s\", oid_to_hex(&info.w_commit));\n \n \tstrbuf_release(&stash_msg_buf);\n@@ -1300,7 +1304,7 @@ static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,\n \n \tif (stash_msg)\n \t\tstrbuf_addstr(&stash_msg_buf, stash_msg);\n-\tif (do_create_stash(ps, &stash_msg_buf, include_untracked, patch_mode,\n+\tif (do_create_stash(ps, stash_msg_buf.buf, include_untracked, patch_mode,\n \t\t\t    &info, &patch, quiet)) {\n \t\tret = -1;\n \t\tgoto done;\n"},{"id":"368450","messageId":"nycvar.QRO.7.76.6.1902041135320.41@tvgsbejvaqbjf.bet","threadId":"50389","inReplyTo":"5d521649-0b21-04e3-3182-e8714fcbfeac@kdbg.org","subject":"Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-04T10:38:03Z","receivedAt":"2019-02-04T10:38:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Mon, 4 Feb 2019, Johannes Sixt wrote:\n\n> Am 03.02.19 um 17:51 schrieb Johannes Sixt:\n> > strbuf_vinsertf inserts a formatted string in the middle of an existing\n> > strbuf value.\n> \n> Quite frankly, this is a really unusual operation, and I'd prefer to get\n> rid of it. There is only one call, and it looks like it only wants to be\n> lazy and save one strbuf variable.\n\nThe only reason why there are not more callers is that I did not convert\nany of the appropriate places. We have quite a few places where we\nallocate a new strbuf for the sole purpose of formatting something that is\nthen inserted into an already existing strbuf (possibly extending the\nbuffer, which might require a move of the buffer just because that\ntemporary strbuf is in the way).\n\nIt does not sound like good practice to me to allocate things left and\nright, only to reallocate something that was just allocated anyway and to\ncopy things into that and then release things left and right.\n\nCiao,\nDscho\n\n> This helper adds way more code than a non-lazy caller would need. There\n> wouldn't even be a mental burden. Like this (except that strbuf_addstr\n> doesn't do what I thought it would do...).\n> \n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 74e6ff62b5..95d202aea3 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -1101,7 +1101,7 @@ static int stash_working_tree(struct stash_info *info, struct pathspec ps)\n>  \treturn ret;\n>  }\n>  \n> -static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,\n> +static int do_create_stash(struct pathspec ps, const char *stash_msg,\n>  \t\t\t   int include_untracked, int patch_mode,\n>  \t\t\t   struct stash_info *info, struct strbuf *patch,\n>  \t\t\t   int quiet)\n> @@ -1117,6 +1117,7 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,\n>  \tstruct strbuf msg = STRBUF_INIT;\n>  \tstruct strbuf commit_tree_label = STRBUF_INIT;\n>  \tstruct strbuf untracked_files = STRBUF_INIT;\n> +\tstruct strbuf stash_msg_buf = STRBUF_INIT;\n>  \n>  \tprepare_fallback_ident(\"git stash\", \"git@stash\");\n>  \n> @@ -1188,10 +1189,12 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,\n>  \t\t}\n>  \t}\n>  \n> -\tif (!stash_msg_buf->len)\n> -\t\tstrbuf_addf(stash_msg_buf, \"WIP on %s\", msg.buf);\n> -\telse\n> -\t\tstrbuf_insertf(stash_msg_buf, 0, \"On %s: \", branch_name);\n> +\tif (!*stash_msg) {\n> +\t\tstrbuf_addf(&stash_msg_buf, \"WIP on %s\", msg.buf);\n> +\t} else {\n> +\t\tstrbuf_addf(&stash_msg_buf, \"On %s: \", branch_name);\n> +\t\tstrbuf_addstr(&stash_msg_buf, stash_msg);\n> +\t}\n>  \n>  \t/*\n>  \t * `parents` will be empty after calling `commit_tree()`, so there is\n> @@ -1206,7 +1209,7 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,\n>  \t\t\t   &parents);\n>  \tcommit_list_insert(head_commit, &parents);\n>  \n> -\tif (commit_tree(stash_msg_buf->buf, stash_msg_buf->len, &info->w_tree,\n> +\tif (commit_tree(stash_msg_buf.buf, stash_msg_buf.len, &info->w_tree,\n>  \t\t\tparents, &info->w_commit, NULL, NULL)) {\n>  \t\tif (!quiet)\n>  \t\t\tfprintf_ln(stderr, _(\"Cannot record \"\n> @@ -1216,6 +1219,7 @@ static int do_create_stash(struct pathspec ps, struct strbuf *stash_msg_buf,\n>  \t}\n>  \n>  done:\n> +\tstrbuf_release(&stash_msg_buf);\n>  \tstrbuf_release(&commit_tree_label);\n>  \tstrbuf_release(&msg);\n>  \tstrbuf_release(&untracked_files);\n> @@ -1236,7 +1240,7 @@ static int create_stash(int argc, const char **argv, const char *prefix)\n>  \tif (!check_changes_tracked_files(ps))\n>  \t\treturn 0;\n>  \n> -\tif (!(ret = do_create_stash(ps, &stash_msg_buf, 0, 0, &info, NULL, 0)))\n> +\tif (!(ret = do_create_stash(ps, stash_msg_buf.buf, 0, 0, &info, NULL, 0)))\n>  \t\tprintf_ln(\"%s\", oid_to_hex(&info.w_commit));\n>  \n>  \tstrbuf_release(&stash_msg_buf);\n> @@ -1300,7 +1304,7 @@ static int do_push_stash(struct pathspec ps, const char *stash_msg, int quiet,\n>  \n>  \tif (stash_msg)\n>  \t\tstrbuf_addstr(&stash_msg_buf, stash_msg);\n> -\tif (do_create_stash(ps, &stash_msg_buf, include_untracked, patch_mode,\n> +\tif (do_create_stash(ps, stash_msg_buf.buf, include_untracked, patch_mode,\n>  \t\t\t    &info, &patch, quiet)) {\n>  \t\tret = -1;\n>  \t\tgoto done;\n> \n"},{"id":"368451","messageId":"nycvar.QRO.7.76.6.1902041138550.41@tvgsbejvaqbjf.bet","threadId":"50389","inReplyTo":"896ae9dd-7ac3-182e-6692-c09bc4864de0@kdbg.org","subject":"Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-04T10:54:20Z","receivedAt":"2019-02-04T10:54:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Sun, 3 Feb 2019, Johannes Sixt wrote:\n\n> strbuf_vinsertf inserts a formatted string in the middle of an existing\n> strbuf value. It makes room in the strbuf by moving existing string to\n> the back, then formats the string to insert directly into the hole.\n> \n> It uses vsnprintf to format the string. The buffer size provided in the\n> invocation is the number of characters available in the allocated space\n> behind the final string. This does not make any sense at all.\n> \n> Fix it to pass the length of the inserted string plus one for the NUL.\n> (The functions saves and restores the character that the NUL occupies.)\n> \n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n>  I found this, because in my environment I have to compile with\n>  SNPRINTF_RETURNS_BOGUS. Our implementation of vsnprintf in\n>  compat/snprintf.c writes into the end of the buffer unconditionally,\n>  at a spot that is unrelated to the formatted string, and this leads to\n>  \"BUG: a NUL byte in commit log message not allowed\" in some \"stash\"\n>  tests.\n> \n>  strbuf.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/strbuf.c b/strbuf.c\n> index bfbbdadbf3..87ecf7f975 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -270,7 +270,7 @@ void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt, va_list ap)\n>  \tmemmove(sb->buf + pos + len, sb->buf + pos, sb->len - pos);\n>  \t/* vsnprintf() will append a NUL, overwriting one of our characters */\n>  \tsave = sb->buf[pos + len];\n> -\tlen2 = vsnprintf(sb->buf + pos, sb->alloc - sb->len, fmt, ap);\n> +\tlen2 = vsnprintf(sb->buf + pos, len + 1, fmt, ap);\n\nIt is really unfortunate that we use a non-dynamic code review system\nwhere it is pretty impossible to increase the amount of context lines\neasily.\n\nAnd in this instance, a single line before the shown context would\nsuffice:\n\n\tstrbuf_grow(sb, len);\n\nWhich is in line with moving from `pos` to `pos + len`, and it is also in\nline with saving the byte at `pos + len`. And since we must consider that\nbyte as part of the buffer, and since `vsnprintf()` wants that size of the\nbuffer (including trailing NUL), `len + 1` is correct.\n\nAnd since `strbuf_grow(sb, len)` would not in general grow the buffer by\nexactly `len` bytes, you indeed fixed a bug.\n\nFor historical context, when I first implemented `strbuf_vinsertf()`, I\nfirst grew the buffer, then let `vsnprintf()` write to the end, and then\nwould rotate the bytes into their correct location. This required the\nimplementation of an in-place rotation scheme, which was a lot of fun, and\ntotally unnecessary. That `sb->alloc - sb->len` parameter you fixed was a\nremainder of that fun side project.\n\nThanks for fixing it,\nDscho\n\nP.S.: Side note: I just realized that we could also write\n\n\tsave = sb->buf[pos];\n  \tmemmove(sb->buf + pos + len + 1, sb->buf + pos + 1, sb->len - pos - 1);\n\ninstead, i.e. not move the first byte just to have it overwritten by\nvsnprintf() immediately, saving on moving one byte. But quite frankly, in\nthis case I do agree that readability is more important than trying to\nsqueeze out the last bit of performance.\n\n>  \tsb->buf[pos + len] = save;\n>  \tif (len2 != len)\n>  \t\tBUG(\"your vsnprintf is broken (returns inconsistent lengths)\");\n> -- \n> 2.20.1.86.gb0de946387\n> \n"},{"id":"368488","messageId":"ea7b1875-1a23-e4e8-e30e-5b769d282323@kdbg.org","threadId":"50389","inReplyTo":"nycvar.QRO.7.76.6.1902041135320.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-02-04T21:13:13Z","receivedAt":"2019-02-04T21:13:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 04.02.19 um 11:38 schrieb Johannes Schindelin:\n> On Mon, 4 Feb 2019, Johannes Sixt wrote:\n> \n>> Am 03.02.19 um 17:51 schrieb Johannes Sixt:\n>>> strbuf_vinsertf inserts a formatted string in the middle of an existing\n>>> strbuf value.\n>>\n>> Quite frankly, this is a really unusual operation, and I'd prefer to get\n>> rid of it. There is only one call, and it looks like it only wants to be\n>> lazy and save one strbuf variable.\n> \n> The only reason why there are not more callers is that I did not convert\n> any of the appropriate places. We have quite a few places where we\n> allocate a new strbuf for the sole purpose of formatting something that is\n> then inserted into an already existing strbuf (possibly extending the\n> buffer, which might require a move of the buffer just because that\n> temporary strbuf is in the way).\n> \n> It does not sound like good practice to me to allocate things left and\n> right, only to reallocate something that was just allocated anyway and to\n> copy things into that and then release things left and right.\n\nI prefer separation of concerns at the expense of a bit of resource\nconsumption that is not measurable. But that is the only argument that I\nhave.\n\n-- Hannes\n"},{"id":"368495","messageId":"xmqqd0o7tdxt.fsf@gitster-ct.c.googlers.com","threadId":"50389","inReplyTo":"896ae9dd-7ac3-182e-6692-c09bc4864de0@kdbg.org","subject":"Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-04T21:57:02Z","receivedAt":"2019-02-04T21:57:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> strbuf_vinsertf inserts a formatted string in the middle of an existing\n> strbuf value. It makes room in the strbuf by moving existing string to\n> the back, then formats the string to insert directly into the hole.\n>\n> It uses vsnprintf to format the string. The buffer size provided in the\n> invocation is the number of characters available in the allocated space\n> behind the final string. This does not make any sense at all.\n>\n> Fix it to pass the length of the inserted string plus one for the NUL.\n> (The functions saves and restores the character that the NUL occupies.)\n>\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n>  I found this, because in my environment I have to compile with\n>  SNPRINTF_RETURNS_BOGUS. Our implementation of vsnprintf in\n>  compat/snprintf.c writes into the end of the buffer unconditionally,\n>  at a spot that is unrelated to the formatted string, and this leads to\n>  \"BUG: a NUL byte in commit log message not allowed\" in some \"stash\"\n>  tests.\n\nAn embarrassing indication that not every line is read during\ndevelopment or review with fine toothed comb.  I guess it won't\ntrigger without the returns-bogus thing, and the \"testing\" done on\nplatforms did not help.\n\nThanks for finding it.  This needs to be squashed into bfc3fe33\n(\"strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`\",\n2018-12-20)?\n\nAs you mention in the direct response of this message, it might make\nsense to get rid of the helper with YAGNI, but that's something we\nneed to see what its potential users should be doing (note: I did\nnot say \"what its potential users are doing\").\n\nIt could turn out that there may be many places that could use this\nhelper, but it may merely be an indication that these many places\nare structured poorly to compute the \"shell\" string too early before\nthe string to be inserted becomes computable, in which case the real\nfix may not be to use this helper but to change the order of\nbuilding up the string.  Perhaps we want a string that is\nconcatenation of part #1, part #2 and part #3, but we somehow first\ncompute concatenation of part #1 and part #3 in a buffer, which\nrequires us to insert part #2 in between.  If we restructure the\ncode to keep parts #1 and #3 separate, or delay computing part #3,\nuntil part #2 becomes available, that would fix the \"potential\nusers\" in a better way without having to make calls into this\nhelper, for example.\n\n\n>  strbuf.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index bfbbdadbf3..87ecf7f975 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -270,7 +270,7 @@ void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt, va_list ap)\n>  \tmemmove(sb->buf + pos + len, sb->buf + pos, sb->len - pos);\n>  \t/* vsnprintf() will append a NUL, overwriting one of our characters */\n>  \tsave = sb->buf[pos + len];\n> -\tlen2 = vsnprintf(sb->buf + pos, sb->alloc - sb->len, fmt, ap);\n> +\tlen2 = vsnprintf(sb->buf + pos, len + 1, fmt, ap);\n>  \tsb->buf[pos + len] = save;\n>  \tif (len2 != len)\n>  \t\tBUG(\"your vsnprintf is broken (returns inconsistent lengths)\");\n"},{"id":"368517","messageId":"nycvar.QRO.7.76.6.1902051136410.41@tvgsbejvaqbjf.bet","threadId":"50389","inReplyTo":"ea7b1875-1a23-e4e8-e30e-5b769d282323@kdbg.org","subject":"Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-05T10:38:43Z","receivedAt":"2019-02-05T10:38:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Mon, 4 Feb 2019, Johannes Sixt wrote:\n\n> Am 04.02.19 um 11:38 schrieb Johannes Schindelin:\n> > On Mon, 4 Feb 2019, Johannes Sixt wrote:\n> > \n> >> Am 03.02.19 um 17:51 schrieb Johannes Sixt:\n> >>> strbuf_vinsertf inserts a formatted string in the middle of an existing\n> >>> strbuf value.\n> >>\n> >> Quite frankly, this is a really unusual operation, and I'd prefer to get\n> >> rid of it. There is only one call, and it looks like it only wants to be\n> >> lazy and save one strbuf variable.\n> > \n> > The only reason why there are not more callers is that I did not convert\n> > any of the appropriate places. We have quite a few places where we\n> > allocate a new strbuf for the sole purpose of formatting something that is\n> > then inserted into an already existing strbuf (possibly extending the\n> > buffer, which might require a move of the buffer just because that\n> > temporary strbuf is in the way).\n> > \n> > It does not sound like good practice to me to allocate things left and\n> > right, only to reallocate something that was just allocated anyway and to\n> > copy things into that and then release things left and right.\n> \n> I prefer separation of concerns at the expense of a bit of resource\n> consumption that is not measurable. But that is the only argument that I\n> have.\n\nAs far as I am concerned, the `strbuf_vinsertf()` function does not\nconflate any concerns... It has one, very easily explained concern: to\ninterpolate text into the middle of a `strbuf`, much like `strbuf_vaddf()`\ninterpolates text at the end of one...\n\nYou found a bug in the implementation of that concern, is all, and fixed\nit, for which I am grateful.\n\nCiao,\nDscho\n"},{"id":"368518","messageId":"nycvar.QRO.7.76.6.1902051140060.41@tvgsbejvaqbjf.bet","threadId":"50389","inReplyTo":"xmqqd0o7tdxt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-05T11:06:06Z","receivedAt":"2019-02-05T11:06:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 4 Feb 2019, Junio C Hamano wrote:\n\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n> > strbuf_vinsertf inserts a formatted string in the middle of an existing\n> > strbuf value. It makes room in the strbuf by moving existing string to\n> > the back, then formats the string to insert directly into the hole.\n> >\n> > It uses vsnprintf to format the string. The buffer size provided in the\n> > invocation is the number of characters available in the allocated space\n> > behind the final string. This does not make any sense at all.\n> >\n> > Fix it to pass the length of the inserted string plus one for the NUL.\n> > (The functions saves and restores the character that the NUL occupies.)\n> >\n> > Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> > ---\n> >  I found this, because in my environment I have to compile with\n> >  SNPRINTF_RETURNS_BOGUS. Our implementation of vsnprintf in\n> >  compat/snprintf.c writes into the end of the buffer unconditionally,\n> >  at a spot that is unrelated to the formatted string, and this leads to\n> >  \"BUG: a NUL byte in commit log message not allowed\" in some \"stash\"\n> >  tests.\n> \n> An embarrassing indication that not every line is read during\n> development or review with fine toothed comb.\n\nOr that they are read with a fine toothed comb, but that the focus lies\nmore on style and maintainability than correctness. We talked about this\nin the past.\n\n> I guess it won't trigger without the returns-bogus thing, and the\n> \"testing\" done on platforms did not help.\n\nIt won't trigger with implementations of vsnprintf() that write just the\nnecessary bytes. You will note that our compat/ version writes at the end\nof the provided buffer, which is legal, but actually not necessary.\n\n> Thanks for finding it.  This needs to be squashed into bfc3fe33\n> (\"strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`\",\n> 2018-12-20)?\n\nSince you want to open that can of worms again, you will also want to\nsquash ed5d77f7d382 (stash: fix segmentation fault when files were added with\nintent, 2019-01-18) into 1f5a011d90ec (stash: convert create to builtin,\n2018-12-20). It will have trivial merge conflicts, and the addition of the\nregression test will be surprising without modifying the commit message to\nexplain that there was a regression that was fixed very late in the\ndevelopment, and that that regression test intends to guarantee that it\nwon't need to be fixed again.\n\n> As you mention in the direct response of this message, it might make\n> sense to get rid of the helper with YAGNI, but that's something we\n> need to see what its potential users should be doing (note: I did\n> not say \"what its potential users are doing\").\n\nAs I already mentioned, there are a couple of potential users that I even\nincluded in my original proposal to Paul, but we decided that this is a\nseparate concern and should therefore be addressed in a separate patch\nseries.\n\n> It could turn out that there may be many places that could use this\n> helper, but it may merely be an indication that these many places\n> are structured poorly to compute the \"shell\" string too early before\n> the string to be inserted becomes computable, in which case the real\n> fix may not be to use this helper but to change the order of\n> building up the string.\n\nWe do not usually do that, as our reviewers would almost certainly point\nout such a convoluted operation.\n\nAnd you seem to make a case against having `strbuf_insert()` to begin\nwith. So I am wondering whether you want to take a step back before going\nfurther down that road.\n\nThe real examples are much more mundane, and very different from what you\nsuspected, e.g. inserting the tag header before the tag message in\n`create_tag()` in `builtin/tag.c`. Basically, it is building up a strbuf\nfor the sake of calling `strbuf_insert()` to insert that string elsewhere.\n\nIn any case, the mere existence, and use, of `strbuf_insert()` is a pretty\nclear counter case to the idea that `strbuf_vinsertf()` would encourage\ninvalid code flows.\n\nCiao,\nDscho\n\n> Perhaps we want a string that is concatenation of part #1, part #2 and\n> part #3, but we somehow first compute concatenation of part #1 and part\n> #3 in a buffer, which requires us to insert part #2 in between.  If we\n> restructure the code to keep parts #1 and #3 separate, or delay\n> computing part #3, until part #2 becomes available, that would fix the\n> \"potential users\" in a better way without having to make calls into this\n> helper, for example.\n> \n> \n> >  strbuf.c | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/strbuf.c b/strbuf.c\n> > index bfbbdadbf3..87ecf7f975 100644\n> > --- a/strbuf.c\n> > +++ b/strbuf.c\n> > @@ -270,7 +270,7 @@ void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt, va_list ap)\n> >  \tmemmove(sb->buf + pos + len, sb->buf + pos, sb->len - pos);\n> >  \t/* vsnprintf() will append a NUL, overwriting one of our characters */\n> >  \tsave = sb->buf[pos + len];\n> > -\tlen2 = vsnprintf(sb->buf + pos, sb->alloc - sb->len, fmt, ap);\n> > +\tlen2 = vsnprintf(sb->buf + pos, len + 1, fmt, ap);\n> >  \tsb->buf[pos + len] = save;\n> >  \tif (len2 != len)\n> >  \t\tBUG(\"your vsnprintf is broken (returns inconsistent lengths)\");\n> \n"},{"id":"368532","messageId":"xmqqzhraru5x.fsf@gitster-ct.c.googlers.com","threadId":"50389","inReplyTo":"nycvar.QRO.7.76.6.1902051140060.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-05T18:01:46Z","receivedAt":"2019-02-05T18:01:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Or that they are read with a fine toothed comb, but that the focus lies\n> more on style and maintainability than correctness. We talked about this\n> in the past.\n\nPerhaps we can do better the next time, then.  I find unreadable\ncode is impossible to reason about its correctness, so you'd need\nto pay attention to style and maintenance issues to quickly get past\nthat step.\n\n>> Thanks for finding it.  This needs to be squashed into bfc3fe33\n>> (\"strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`\",\n>> 2018-12-20)?\n>\n> Since you want to open that can of worms again, you will also want to\n> squash ed5d77f7d382 (stash: fix segmentation fault when files were added with\n> intent, 2019-01-18) into 1f5a011d90ec (stash: convert create to builtin,\n> 2018-12-20). It will have trivial merge conflicts, and the addition of the\n> regression test will be surprising without modifying the commit message to\n> explain that there was a regression that was fixed very late in the\n> development, and that that regression test intends to guarantee that it\n> won't need to be fixed again.\n\nAre you saying that I should not merge the series as is but expect\nan update that does these squashing?  I was planning to make this a\nmerge-down day, but let me exclude this topic from the \"for next\"\nbatch just in case for today.\n\nThanks.\n"},{"id":"368543","messageId":"ef5f66fb-7467-93be-00fd-7900ce961f72@kdbg.org","threadId":"50389","inReplyTo":"nycvar.QRO.7.76.6.1902051140060.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-02-05T20:30:31Z","receivedAt":"2019-02-05T20:30:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 05.02.19 um 12:06 schrieb Johannes Schindelin:\n> The real examples are much more mundane, and very different from what you\n> suspected, e.g. inserting the tag header before the tag message in\n> `create_tag()` in `builtin/tag.c`. Basically, it is building up a strbuf\n> for the sake of calling `strbuf_insert()` to insert that string elsewhere.\n\nI had a look, and this example does not convince me at all. If\nseparation of concerns were applied well around the launch_editor APIs,\nyou would not need strbuf_insert() in the first place. But, alas, these\nfunctions focus more on DRY, and that is often the opposite of\nseparation of concerns.\n\n> In any case, the mere existence, and use, of `strbuf_insert()` is a pretty\n> clear counter case to the idea that `strbuf_vinsertf()` would encourage\n> invalid code flows.\n\nNobody wants to get rid of strbuf_insert(). I have worked with at least\n3 string APIs. All have insert operations, and some have string\nformatting capabilities, but none of them conflate the two operations\ninto one. Maybe, there is a plan behind it? strbuf is the first (my 4th)\nAPI that does, and it was non-trivial to get it right...\n\n-- Hannes\n"},{"id":"368582","messageId":"nycvar.QRO.7.76.6.1902061234250.41@tvgsbejvaqbjf.bet","threadId":"50389","inReplyTo":"xmqqzhraru5x.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-06T11:41:30Z","receivedAt":"2019-02-06T11:41:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 5 Feb 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> >> Thanks for finding it.  This needs to be squashed into bfc3fe33\n> >> (\"strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`\",\n> >> 2018-12-20)?\n> >\n> > Since you want to open that can of worms again, you will also want to\n> > squash ed5d77f7d382 (stash: fix segmentation fault when files were\n> > added with intent, 2019-01-18) into 1f5a011d90ec (stash: convert\n> > create to builtin, 2018-12-20). It will have trivial merge conflicts,\n> > and the addition of the regression test will be surprising without\n> > modifying the commit message to explain that there was a regression\n> > that was fixed very late in the development, and that that regression\n> > test intends to guarantee that it won't need to be fixed again.\n> \n> Are you saying that I should not merge the series as is but expect\n> an update that does these squashing?\n\nNo, I thought I had made my wish clear in the past: to advance\nps/stash-in-c to `next` a long time ago, mainly because it became obvious\nthat Paul is too occupied with University things to really pay attention\nto the discussions on the Git mailing list. Consequently, we cannot really\ndo the regular thing where the branch is cooked in `pu` because the main\ncontributor is not really able to spend the time cooking. So to me, it\nsounded like the safest way forward (without losing the entire\nps/stash-in-c branch) would be to merge it into `next`, with known fixes\non top, and cook it from there, with more cooks involved (which I heard\nsome people believe is not a wise thing to do, but then, we do that in Git\nall the time).\n\nSo what I tried to say is that I am a bit opposed to start squashing into\nPaul's patches, and rather do things on top as the --intent-to-add fixup\nthat I already provided. You seemed to suggest completely otherwise, which\ngot me puzzled.\n\n> I was planning to make this a merge-down day, but let me exclude this\n> topic from the \"for next\" batch just in case for today.\n\nWell, you're the boss of git.git. I find it sad though, personally, that\nwe could not move this forward, especially since we introduced the safety\nhatch for the very purpose of moving this forward, all the way into the\nupcoming release.\n\nI would have wished that ps/stash-in-c went into `next` already, what with\nthe few corner case bug fixes we had recently.\n\nCiao,\nDscho\n"},{"id":"368584","messageId":"nycvar.QRO.7.76.6.1902061242380.41@tvgsbejvaqbjf.bet","threadId":"50389","inReplyTo":"ef5f66fb-7467-93be-00fd-7900ce961f72@kdbg.org","subject":"Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-06T11:56:42Z","receivedAt":"2019-02-06T11:56:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Tue, 5 Feb 2019, Johannes Sixt wrote:\n\n> Am 05.02.19 um 12:06 schrieb Johannes Schindelin:\n> > The real examples are much more mundane, and very different from what you\n> > suspected, e.g. inserting the tag header before the tag message in\n> > `create_tag()` in `builtin/tag.c`. Basically, it is building up a strbuf\n> > for the sake of calling `strbuf_insert()` to insert that string elsewhere.\n> \n> I had a look, and this example does not convince me at all. If\n> separation of concerns were applied well around the launch_editor APIs,\n> you would not need strbuf_insert() in the first place. But, alas, these\n> functions focus more on DRY, and that is often the opposite of\n> separation of concerns.\n\nSo you actually are convinced that it is needed in this instance. Good.\n\n> > In any case, the mere existence, and use, of `strbuf_insert()` is a\n> > pretty clear counter case to the idea that `strbuf_vinsertf()` would\n> > encourage invalid code flows.\n> \n> Nobody wants to get rid of strbuf_insert(). I have worked with at least\n> 3 string APIs. All have insert operations, and some have string\n> formatting capabilities, but none of them conflate the two operations\n> into one. Maybe, there is a plan behind it? strbuf is the first (my 4th)\n> API that does, and it was non-trivial to get it right...\n\nSorry, by that token `strbuf_vaddf()` would already be a violation of the\nseparation of concerns: it also makes space first, and then it also\nformats a string.\n\nRender me unconvinced.\n\nI am still glad you found and fixed the bug,\nDscho\n"},{"id":"368618","messageId":"f02e73e6-7e0e-6cd9-3d3a-b2b74d03f6be@kdbg.org","threadId":"50389","inReplyTo":"nycvar.QRO.7.76.6.1902061242380.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH ps/stash-in-c] strbuf_vinsertf: provide the correct buffer size to vsnprintf","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-02-06T18:11:57Z","receivedAt":"2019-02-06T18:12:02Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 06.02.19 um 12:56 schrieb Johannes Schindelin:\n> On Tue, 5 Feb 2019, Johannes Sixt wrote:\n>> Am 05.02.19 um 12:06 schrieb Johannes Schindelin:\n>>> The real examples are much more mundane, and very different from what you\n>>> suspected, e.g. inserting the tag header before the tag message in\n>>> `create_tag()` in `builtin/tag.c`. Basically, it is building up a strbuf\n>>> for the sake of calling `strbuf_insert()` to insert that string elsewhere.\n>>\n>> I had a look, and this example does not convince me at all. If\n>> separation of concerns were applied well around the launch_editor APIs,\n>> you would not need strbuf_insert() in the first place. But, alas, these\n>> functions focus more on DRY, and that is often the opposite of\n>> separation of concerns.\n> \n> So you actually are convinced that it is needed in this instance. Good.\n\nI thought I said quite the opposite.\n\nBut never mind. Since I'm not going to help refactor any C code anyway\nas long as the result is still C code, who am I to complain about the\nexistence of strbuf_vinsertf()?\n\n-- Hannes\n"}]}