{"thread":{"id":"65300","subject":"[PATCH] rerere: update to modern representation of empty strbufs","startedAt":"2026-03-19T07:16:02Z","lastAt":"2026-03-22T01:44:31Z","messageCount":20,"participants":["Junio C Hamano","Patrick Steinhardt","Jeff King","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"539369","messageId":"xmqq341wnvbk.fsf@gitster.g","threadId":"65300","inReplyTo":null,"subject":"[PATCH] rerere: update to modern representation of empty strbufs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-19T07:15:59Z","receivedAt":"2026-03-19T07:16:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Back when b4833a2c (rerere: Fix use of an empty strbuf.buf,\n2007-09-26) was written, a freshly initialized empty strbuf\nhad NULL in its .buf member, with .len set to 0. The code this\npatch touches in rerere.c was written to _fix_ the original code\nthat assumed that the .buf member is always pointing at a NUL-terminated\nstring, even for an empty string, which did not hold back then.\n\nThat changed in b315c5c0 (strbuf change: be sure ->buf is never ever\nNULL., 2007-09-27), and it has again become safe to assume that .buf\nis never NULL, and .buf[0] has '\\0' for an empty string (i.e., a\nstrbuf with its .len member set to 0).\n\nA funny thing is, this piece of code has been moved around from\nbuiltin-rerere.c to rerere.c and also adjusted for updates to the\nhash function API over the years, but nobody bothered to question\nif this special casing for an empty strbuf was still necessary:\n\n b4833a2c62 (rerere: Fix use of an empty strbuf.buf, 2007-09-26)\n 5b2fd95606 (rerere: Separate libgit and builtin functions, 2008-07-09)\n 9126f0091f (fix openssl headers conflicting with custom SHA1 implementations, 2008-10-01)\n c0f16f8e14 (rerere: factor out handle_conflict function, 2018-08-05)\n 0d7c419a94 (rerere: convert to use the_hash_algo, 2018-10-15)\n 0578f1e66a (global: adapt callers to use generic hash context helpers, 2025-01-31)\n\nFinally get rid of the special casing that was unnecessary for the\nlast 19 years.\n\nSigned-off-by: Junio C Hamano \n---\n rerere.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex 6ec55964e2..0296700f9f 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -403,12 +403,8 @@ static int handle_conflict(struct strbuf *out, struct rerere_io *io,\n \t\t\tstrbuf_addbuf(out, &two);\n \t\t\trerere_strbuf_putconflict(out, '>', marker_size);\n \t\t\tif (ctx) {\n-\t\t\t\tgit_hash_update(ctx, one.buf ?\n-\t\t\t\t\t\tone.buf : \"\",\n-\t\t\t\t\t\tone.len + 1);\n-\t\t\t\tgit_hash_update(ctx, two.buf ?\n-\t\t\t\t\t\ttwo.buf : \"\",\n-\t\t\t\t\t\ttwo.len + 1);\n+\t\t\t\tgit_hash_update(ctx, one.buf, one.len + 1);\n+\t\t\t\tgit_hash_update(ctx, two.buf, two.len + 1);\n \t\t\t}\n \t\t\tbreak;\n \t\t} else if (hunk == RR_SIDE_1)\n-- \n2.53.0-781-gf5b2cca52b\n\n"},{"id":"539371","messageId":"abusg-X1vtd48UpZ@pks.im","threadId":"65300","inReplyTo":"xmqq341wnvbk.fsf@gitster.g","subject":"Re: [PATCH] rerere: update to modern representation of empty strbufs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-19T07:57:55Z","receivedAt":"2026-03-19T07:58:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Mar 19, 2026 at 12:15:59AM -0700, Junio C Hamano wrote:\n> Back when b4833a2c (rerere: Fix use of an empty strbuf.buf,\n> 2007-09-26) was written, a freshly initialized empty strbuf\n> had NULL in its .buf member, with .len set to 0. The code this\n> patch touches in rerere.c was written to _fix_ the original code\n> that assumed that the .buf member is always pointing at a NUL-terminated\n> string, even for an empty string, which did not hold back then.\n> \n> That changed in b315c5c0 (strbuf change: be sure ->buf is never ever\n> NULL., 2007-09-27), and it has again become safe to assume that .buf\n> is never NULL, and .buf[0] has '\\0' for an empty string (i.e., a\n> strbuf with its .len member set to 0).\n\nRight. We always require strbufs to be initialized, and that will always\ncause us to set the `.buf` member.\n\n> diff --git a/rerere.c b/rerere.c\n> index 6ec55964e2..0296700f9f 100644\n> --- a/rerere.c\n> +++ b/rerere.c\n> @@ -403,12 +403,8 @@ static int handle_conflict(struct strbuf *out, struct rerere_io *io,\n> \t\t\tstrbuf_addbuf(out, &two);\n> \t\t\trerere_strbuf_putconflict(out, '>', marker_size);\n> \t\t\tif (ctx) {\n> -\t\t\t\tgit_hash_update(ctx, one.buf ?\n> -\t\t\t\t\t\tone.buf : \"\",\n> -\t\t\t\t\t\tone.len + 1);\n> -\t\t\t\tgit_hash_update(ctx, two.buf ?\n> -\t\t\t\t\t\ttwo.buf : \"\",\n> -\t\t\t\t\t\ttwo.len + 1);\n> +\t\t\t\tgit_hash_update(ctx, one.buf, one.len + 1);\n> +\t\t\t\tgit_hash_update(ctx, two.buf, two.len + 1);\n> \t\t\t}\n\nYup, this should be indeed equivalent given that we declare the `.buf`\nthing as `char strbuf_slopbuf[1]`. So we'll get a single NUL byte here,\nwhich is the same as the empty string.\n\nSo this looks good to me, thanks!\n\nPatrick\n"},{"id":"539426","messageId":"xmqqcy0zii0s.fsf@gitster.g","threadId":"65300","inReplyTo":"xmqq341wnvbk.fsf@gitster.g","subject":"[RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-19T22:14:27Z","receivedAt":"2026-03-19T22:14:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Subject: Re: [PATCH] rerere: update to modern representation of empty strbufs\n>\n> Finally get rid of the special casing that was unnecessary for the\n> last 19 years.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> rerere.c | 8 ++------\n> 1 file changed, 2 insertions(+), 6 deletions(-)\n>\n> diff --git a/rerere.c b/rerere.c\n> index 6ec55964e2..0296700f9f 100644\n> --- a/rerere.c\n> +++ b/rerere.c\n> @@ -403,12 +403,8 @@ static int handle_conflict(struct strbuf *out, struct rerere_io *io,\n> \t\t\tstrbuf_addbuf(out, &two);\n> \t\t\trerere_strbuf_putconflict(out, '>', marker_size);\n> \t\t\tif (ctx) {\n> -\t\t\t\tgit_hash_update(ctx, one.buf ?\n> -\t\t\t\t\t\tone.buf : \"\",\n> -\t\t\t\t\t\tone.len + 1);\n> -\t\t\t\tgit_hash_update(ctx, two.buf ?\n> -\t\t\t\t\t\ttwo.buf : \"\",\n> -\t\t\t\t\t\ttwo.len + 1);\n> +\t\t\t\tgit_hash_update(ctx, one.buf, one.len + 1);\n> +\t\t\t\tgit_hash_update(ctx, two.buf, two.len + 1);\n> \t\t\t}\n> \t\t\tbreak;\n> \t\t} else if (hunk == RR_SIDE_1)\n\nI wrote a trivial Coccinele rule (attached at the end) to rewrite \n\n SB.buf ? SB.buf : \"\"\n\ninto\n\n SB.buf\n\nand this found only the above instance, which is good.\n\nHowever, a related rule, \"it is nonsense to expect that SB.buf could\nsometimes be false\", finds two questionable instances.\n\nOne is in list-objects-filter-options.c::parse_list_objects_filter()\n\n void parse_list_objects_filter(\n struct list_objects_filter_options *filter_options,\n const char *arg)\n {\n struct strbuf errbuf = STRBUF_INIT;\n\n if (!filter_options->filter_spec.buf)\n BUG(\"filter_options not properly initialized\");\n\nThe filter_options variable points at a list_objects_filter_options\nstructure, which has an embedded \"struct strbuf\". This BUG() is\nunnecessary if the structure is properly initialized, either by the\nLIST_OBJECTS_FILTER_INIT macro or a list_objects_filter_init() call.\nBut it is easy to memset(&lofo, 0, sizeof(lofo)) or zero initialize\nwith \"= {0}\", so I think it is OK to special case and allow for\nchecking the possibility that .buf might be NULL.\n\nThe other exception comes from use of getdelim() in\nstrbuf_getwholeline(), whose early part reads like this:\n\n int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n {\n ...\n /* Translate slopbuf to NULL, as we cannot call realloc on it */\n if (!sb->alloc)\n sb->buf = NULL;\n errno = 0;\n r = getdelim(&sb->buf, &sb->alloc, term, fp);\n if (r > 0) {\n sb->len = r;\n return 0;\n }\n\n\nBefore calling getdelim(), we deliberately break the strbuf\ninvariant \".buf is never NULL; it can point at the slopbuf if .len\nis 0\". If we read even a single byte, we are OK, as the invariant\nis restored.\n\nUpon EOF, later in the function we have\n\n if (!sb->buf)\n strbuf_init(sb, 0);\n else\n strbuf_reset(sb);\n return EOF;\n\nto recover the strbuf invariant.\n\nBecause strbuf_getwholeline() discards what is originally in sb and\nreplaces it with what getdelim() returns, I have a suspicion that\nworking with bare char * and size_t to interact with getdelim() and\nthen using strbuf_attach() on the success case would be simpler to\nread and maintain. Once such a rewrite of this function is done\n(#leftoverbits), the special case we see in the Coccinelle rule can\nbe lifted.\n\nThoughts?\n\n\n contrib/coccinelle/strbuf.cocci | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git c/contrib/coccinelle/strbuf.cocci w/contrib/coccinelle/strbuf.cocci\nindex 5f06105df6..3dc5cd02a3 100644\n--- c/contrib/coccinelle/strbuf.cocci\n+++ w/contrib/coccinelle/strbuf.cocci\n@@ -60,3 +60,18 @@ expression E1, E2;\n @@\n - strbuf_addstr(E1, real_path(E2));\n + strbuf_add_real_path(E1, E2);\n+\n+@@\n+struct strbuf SB;\n+@@\n+- SB.buf ? SB.buf : \"\"\n++ SB.buf\n+\n+@@\n+identifier funcname != { strbuf_getwholeline, parse_list_objects_filter };\n+struct strbuf SB;\n+@@\n+ funcname(...) {<...\n+- !SB.buf\n++ 0\n+ ...>}\n\n\n"},{"id":"539447","messageId":"20260319233546.GA3632561@coredump.intra.peff.net","threadId":"65300","inReplyTo":"xmqqcy0zii0s.fsf@gitster.g","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-19T23:35:46Z","receivedAt":"2026-03-19T23:35:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2026 at 03:14:27PM -0700, Junio C Hamano wrote:\n\n> One is in list-objects-filter-options.c::parse_list_objects_filter()\n> \n> void parse_list_objects_filter(\n> struct list_objects_filter_options *filter_options,\n> const char *arg)\n> {\n> struct strbuf errbuf = STRBUF_INIT;\n> \n> if (!filter_options->filter_spec.buf)\n> BUG(\"filter_options not properly initialized\");\n> \n> The filter_options variable points at a list_objects_filter_options\n> structure, which has an embedded \"struct strbuf\". This BUG() is\n> unnecessary if the structure is properly initialized, either by the\n> LIST_OBJECTS_FILTER_INIT macro or a list_objects_filter_init() call.\n> But it is easy to memset(&lofo, 0, sizeof(lofo)) or zero initialize\n> with \"= {0}\", so I think it is OK to special case and allow for\n> checking the possibility that .buf might be NULL.\n\nYeah, this is about catching _other_ code which accidentally violates\nthe invariant. I don't think there is any choice between special-casing\nit or just removing the BUG() check. It is probably OK to do the latter\nat this point. As part of the transition to LIST_OBJECTS_FILTER_INIT it\nwas a bigger risk, but that is less likely now. So I am OK either way.\n\n> Because strbuf_getwholeline() discards what is originally in sb and\n> replaces it with what getdelim() returns, I have a suspicion that\n> working with bare char * and size_t to interact with getdelim() and\n> then using strbuf_attach() on the success case would be simpler to\n> read and maintain. Once such a rewrite of this function is done\n> (#leftoverbits), the special case we see in the Coccinelle rule can\n> be lifted.\n\nHmm. I think that is something like this:\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 3939863cf3..0333aea261 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -631,6 +631,8 @@ int strbuf_getcwd(struct strbuf *sb)\n #ifdef HAVE_GETDELIM\n int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n {\n+\tchar *buf;\n+\tsize_t alloc;\n \tssize_t r;\n \n \tif (feof(fp))\n@@ -639,12 +641,14 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n \tstrbuf_reset(sb);\n \n \t/* Translate slopbuf to NULL, as we cannot call realloc on it */\n-\tif (!sb->alloc)\n-\t\tsb->buf = NULL;\n+\talloc = sb->alloc;\n+\tbuf = alloc ? sb->buf : NULL;\n \terrno = 0;\n-\tr = getdelim(&sb->buf, &sb->alloc, term, fp);\n+\tr = getdelim(&buf, &alloc, term, fp);\n \n \tif (r > 0) {\n+\t\tsb->buf = buf;\n+\t\tsb->alloc = alloc;\n \t\tsb->len = r;\n \t\treturn 0;\n \t}\n@@ -669,10 +673,13 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n \t * we can just re-init, but otherwise we should make sure that our\n \t * length is empty, and that the result is NUL-terminated.\n \t */\n-\tif (!sb->buf)\n+\tif (!buf)\n \t\tstrbuf_init(sb, 0);\n-\telse\n-\t\tstrbuf_reset(sb);\n+\telse {\n+\t\tsb->buf = buf;\n+\t\tsb->alloc = alloc;\n+\t\tstrbuf_reset(&sb);\n+\t}\n \treturn EOF;\n }\n #else\n\nSo I don't know that it makes anything simpler. We have to copy the\nvalues back into the strbuf either way, and we still have to handle\nrestoring the strbuf invariants. Even the strbuf_init() case is still\nneeded, because we don't know whether getdelim() just didn't allocate\n(in which case we could leave the strbuf alone) or if it actually ate\nthe allocation we passed in (which was just a copy of sb->buf).\n\n-Peff\n"},{"id":"539464","messageId":"xmqqcy0zgtmu.fsf@gitster.g","threadId":"65300","inReplyTo":"20260319233546.GA3632561@coredump.intra.peff.net","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-20T01:46:33Z","receivedAt":"2026-03-20T01:46:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Because strbuf_getwholeline() discards what is originally in sb and\n>> replaces it with what getdelim() returns, I have a suspicion that\n>> working with bare char * and size_t to interact with getdelim() and\n>> then using strbuf_attach() on the success case would be simpler to\n>> read and maintain. Once such a rewrite of this function is done\n>> (#leftoverbits), the special case we see in the Coccinelle rule can\n>> be lifted.\n>\n> Hmm. I think that is something like this:\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index 3939863cf3..0333aea261 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -631,6 +631,8 @@ int strbuf_getcwd(struct strbuf *sb)\n> #ifdef HAVE_GETDELIM\n> int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n> {\n> +\tchar *buf;\n> +\tsize_t alloc;\n> \tssize_t r;\n> \n> \tif (feof(fp))\n> @@ -639,12 +641,14 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n> \tstrbuf_reset(sb);\n> \n> \t/* Translate slopbuf to NULL, as we cannot call realloc on it */\n> -\tif (!sb->alloc)\n> -\t\tsb->buf = NULL;\n> +\talloc = sb->alloc;\n> +\tbuf = alloc ? sb->buf : NULL;\n> \terrno = 0;\n\nI actually was hoping that all lines in this hunk before this point\ncan be removed, i.e., strbuf_release(sb), buf = NULL, alloc = 0.\n\n> -\tr = getdelim(&sb->buf, &sb->alloc, term, fp);\n> +\tr = getdelim(&buf, &alloc, term, fp);\n> \n> \tif (r > 0) {\n> +\t\tsb->buf = buf;\n> +\t\tsb->alloc = alloc;\n> \t\tsb->len = r;\n> \t\treturn 0;\n> \t}\n\nYes.\n\n> @@ -669,10 +673,13 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n> \t * we can just re-init, but otherwise we should make sure that our\n> \t * length is empty, and that the result is NUL-terminated.\n> \t */\n> -\tif (!sb->buf)\n> +\tif (!buf)\n> \t\tstrbuf_init(sb, 0);\n> -\telse\n> -\t\tstrbuf_reset(sb);\n> +\telse {\n> +\t\tsb->buf = buf;\n> +\t\tsb->alloc = alloc;\n> +\t\tstrbuf_reset(&sb);\n> +\t}\n\nI do not get all these conditionals. This is an EOF code path; we\nhave no data in buf to return. We resetted the caller's strbuf\nalready. Can't we return buf (if allocated) to the system and\nreturn without doing any further damage to sb at this point?\n\n> \treturn EOF;\n> }\n> #else\n>\n> So I don't know that it makes anything simpler. We have to copy the\n> values back into the strbuf either way, and we still have to handle\n> restoring the strbuf invariants. Even the strbuf_init() case is still\n> needed, because we don't know whether getdelim() just didn't allocate\n> (in which case we could leave the strbuf alone) or if it actually ate\n> the allocation we passed in (which was just a copy of sb->buf).\n>\n> -Peff\n"},{"id":"539468","messageId":"20260320041803.GA18125@coredump.intra.peff.net","threadId":"65300","inReplyTo":"xmqqcy0zgtmu.fsf@gitster.g","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-20T04:18:03Z","receivedAt":"2026-03-20T04:18:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2026 at 06:46:33PM -0700, Junio C Hamano wrote:\n\n> > diff --git a/strbuf.c b/strbuf.c\n> > index 3939863cf3..0333aea261 100644\n> > --- a/strbuf.c\n> > +++ b/strbuf.c\n> > @@ -631,6 +631,8 @@ int strbuf_getcwd(struct strbuf *sb)\n> > #ifdef HAVE_GETDELIM\n> > int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n> > {\n> > +\tchar *buf;\n> > +\tsize_t alloc;\n> > \tssize_t r;\n> > \n> > \tif (feof(fp))\n> > @@ -639,12 +641,14 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n> > \tstrbuf_reset(sb);\n> > \n> > \t/* Translate slopbuf to NULL, as we cannot call realloc on it */\n> > -\tif (!sb->alloc)\n> > -\t\tsb->buf = NULL;\n> > +\talloc = sb->alloc;\n> > +\tbuf = alloc ? sb->buf : NULL;\n> > \terrno = 0;\n> \n> I actually was hoping that all lines in this hunk before this point\n> can be removed, i.e., strbuf_release(sb), buf = NULL, alloc = 0.\n\nI'm not quite sure what you mean. The function right now looks like:\n\n ssize_t r;\n \n if (feof(fp))\n return EOF;\n \n strbuf_reset(sb);\n \n /* Translate slopbuf to NULL, as we cannot call realloc on it */\n if (!sb->alloc)\n sb->buf = NULL;\n errno = 0;\n\nI think the strbuf_reset() could go away even without any other changes.\nWe always adjust sb->len in the end to match what happened with\ngetdelim(), so there is no point in doing it up front.\n\nWe could strbuf_release() and set buf to NULL, but that would defeat the\npurpose of the function, wouldn't it? We want to reuse sb->buf in each\ncall, not allocate it fresh each time. I.e., in a loop like:\n\n while (strbuf_getline(&sb) != EOF) {\n ...look at sb.buf...\n }\n\nwe want to use the same buffer over and over.\n\n> > @@ -669,10 +673,13 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n> > \t * we can just re-init, but otherwise we should make sure that our\n> > \t * length is empty, and that the result is NUL-terminated.\n> > \t */\n> > -\tif (!sb->buf)\n> > +\tif (!buf)\n> > \t\tstrbuf_init(sb, 0);\n> > -\telse\n> > -\t\tstrbuf_reset(sb);\n> > +\telse {\n> > +\t\tsb->buf = buf;\n> > +\t\tsb->alloc = alloc;\n> > +\t\tstrbuf_reset(&sb);\n> > +\t}\n> \n> I do not get all these conditionals. This is an EOF code path; we\n> have no data in buf to return. We resetted the caller's strbuf\n> already. Can't we return buf (if allocated) to the system and\n> return without doing any further damage to sb at this point?\n\nThe conditional is trying to keep any allocated buffer returned from\ngetdelim() attached to the strbuf. Since this is EOF (or error), I agree\nit would probably be OK to just free it. Even in a loop like the one\nabove, the loop will generally end at EOF, and we don't care about\nreusing the buffer further.\n\nBut it's not quite enough to just do:\n\n free(buf);\n\nBecause \"buf\" is a copy of sb->buf, and we handed \"buf\" off to\ngetdelim(), we need to make sure sb->buf is not still pointing there. I\nthink it would be enough to do:\n\n free(buf);\n strbuf_init(sb, 0);\n\nIf we did a strbuf_release() at the top of the function then that is not\na concern (you know that sb->buf is pointing at the slopbuf). But I\ndon't think that is a good idea for the reason I gave above.\n\n-Peff\n"},{"id":"539471","messageId":"xmqq341vgilb.fsf@gitster.g","threadId":"65300","inReplyTo":"20260320041803.GA18125@coredump.intra.peff.net","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-20T05:45:04Z","receivedAt":"2026-03-20T05:45:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm not quite sure what you mean. The function right now looks like:\n> ...\n> But it's not quite enough to just do:\n>\n> free(buf);\n>\n> Because \"buf\" is a copy of sb->buf,...\n\nI may have phrased the idea very poorly. In short, the core of the\nidea is that we do not have to use the original content of the\nstrbuf at all. I.e., \"buf\" does not have to be anything related to\nsb->buf.\n\nIn the following patch, I am _removing_ strbuf_reset() near the\nbeginning, but replacing it with strbuf_release() may illustrate the\nidea more clearly. I didn't do so primarily because the first thing\nstrbuf_attach() does is to call strbuf_release(), so the call would\nbe redundant in the normal code flow.\n\nWhen getdelim() did not return anything positive, after asserting\nthe return value is -1 (i.e., EOF), we only need to return EOF while\nemptying the caller-supplied strbuf. As the \"char *buf\" we have and\npassed to getdelim() never had anything to do with caller-supplied\nstrbuf *sb, there is no need to worry about slopbuf or anything\nassociated with it.\n\n strbuf.c | 28 +++++++++++++---------------\n 1 file changed, 13 insertions(+), 15 deletions(-)\n\ndiff --git c/strbuf.c w/strbuf.c\nindex 3939863cf3..ef61bd5b14 100644\n--- c/strbuf.c\n+++ w/strbuf.c\n@@ -631,21 +631,20 @@ int strbuf_getcwd(struct strbuf *sb)\n #ifdef HAVE_GETDELIM\n int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n {\n+\tsize_t alloc;\n+\tchar *buf;\n \tssize_t r;\n \n \tif (feof(fp))\n \t\treturn EOF;\n \n-\tstrbuf_reset(sb);\n-\n-\t/* Translate slopbuf to NULL, as we cannot call realloc on it */\n-\tif (!sb->alloc)\n-\t\tsb->buf = NULL;\n+\tbuf = NULL;\n+\talloc = 0;\n \terrno = 0;\n-\tr = getdelim(&sb->buf, &sb->alloc, term, fp);\n+\tr = getdelim(&buf, &alloc, term, fp);\n \n \tif (r > 0) {\n-\t\tsb->len = r;\n+\t\tstrbuf_attach(sb, buf, (size_t) r, alloc);\n \t\treturn 0;\n \t}\n \tassert(r == -1);\n@@ -664,15 +663,14 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n \tif (errno == ENOMEM)\n \t\tdie(\"Out of memory, getdelim failed\");\n \n-\t/*\n-\t * Restore strbuf invariants; if getdelim left us with a NULL pointer,\n-\t * we can just re-init, but otherwise we should make sure that our\n-\t * length is empty, and that the result is NUL-terminated.\n+\t/* \n+\t * We got an EOF. If getdelim() allocated any memory, we\n+\t * would return that to the system.\n \t */\n-\tif (!sb->buf)\n-\t\tstrbuf_init(sb, 0);\n-\telse\n-\t\tstrbuf_reset(sb);\n+\tfree(buf);\n+\n+\t/* And empty the strbuf */\n+\tstrbuf_release(sb);\n \treturn EOF;\n }\n #else\n"},{"id":"539472","messageId":"20260320055709.GA35291@coredump.intra.peff.net","threadId":"65300","inReplyTo":"xmqq341vgilb.fsf@gitster.g","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-20T05:57:09Z","receivedAt":"2026-03-20T05:57:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2026 at 10:45:04PM -0700, Junio C Hamano wrote:\n\n> I may have phrased the idea very poorly. In short, the core of the\n> idea is that we do not have to use the original content of the\n> strbuf at all. I.e., \"buf\" does not have to be anything related to\n> sb->buf.\n\nThis seems like a non-starter to me, though, as it means getdelim() will\nalways allocate a fresh buffer, even though we had a buffer it could\nhave used. I.e., here:\n\n> +\tbuf = NULL;\n> +\talloc = 0;\n> \terrno = 0;\n> -\tr = getdelim(&sb->buf, &sb->alloc, term, fp);\n> +\tr = getdelim(&buf, &alloc, term, fp);\n\nwe will always get a new allocation. And so looping over\nstrbuf_getline() will incur one allocation per call, rather than using\nthe same buffer over and over.\n\nI haven't measured to see what the exact cost is, but I know that\nlooping over a strbuf (with a reset in the loop, or the implied reset\nfrom a getline call) is a common optimization trick that does have a\nmeasurable improvement for some cases.\n\n-Peff\n"},{"id":"539474","messageId":"xmqqtsubf31e.fsf@gitster.g","threadId":"65300","inReplyTo":"20260320055709.GA35291@coredump.intra.peff.net","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-20T06:06:21Z","receivedAt":"2026-03-20T06:06:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I haven't measured to see what the exact cost is, but I know that\n> looping over a strbuf (with a reset in the loop, or the implied reset\n> from a getline call) is a common optimization trick that does have a\n> measurable improvement for some cases.\n\nYeah, that part I missed. Thanks.\n"},{"id":"539477","messageId":"20260320061852.GA35538@coredump.intra.peff.net","threadId":"65300","inReplyTo":"20260320055709.GA35291@coredump.intra.peff.net","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-20T06:18:52Z","receivedAt":"2026-03-20T06:18:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 20, 2026 at 01:57:09AM -0400, Jeff King wrote:\n\n> I haven't measured to see what the exact cost is, but I know that\n> looping over a strbuf (with a reset in the loop, or the implied reset\n> from a getline call) is a common optimization trick that does have a\n> measurable improvement for some cases.\n\nTry something like this:\n\n # input file is all objects in linux.git\n cd linux.git\n git repack -ad\n git show-index <.git/objects/pack/pack-*.idx | cut -d' ' -f2 >input\n\n # now do something that primarily reads a bunch of lines\n git cat-file --buffer --batch-check='%(objectname)\" \n> ssize_t r;\n> \n> if (feof(fp))\n> return EOF;\n> \n> strbuf_reset(sb);\n> \n> /* Translate slopbuf to NULL, as we cannot call realloc on it */\n> if (!sb->alloc)\n> sb->buf = NULL;\n> errno = 0;\n> \n> I think the strbuf_reset() could go away even without any other changes.\n> We always adjust sb->len in the end to match what happened with\n> getdelim(), so there is no point in doing it up front.\n\nYes. Same with the EOF check; getdelim(3) is (must be) prepared to handle\nthat for us. An early return at the end of the file avoids the translate\neffort once per file, but adds the cost of checking for each line.\n\nRené\n\n"},{"id":"539601","messageId":"xmqqqzpdb172.fsf@gitster.g","threadId":"65300","inReplyTo":"xmqqcy0zii0s.fsf@gitster.g","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-21T16:24:17Z","receivedAt":"2026-03-21T16:24:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> diff --git c/contrib/coccinelle/strbuf.cocci w/contrib/coccinelle/strbuf.cocci\n> index 5f06105df6..3dc5cd02a3 100644\n> --- c/contrib/coccinelle/strbuf.cocci\n> +++ w/contrib/coccinelle/strbuf.cocci\n> ...\n> +@@\n> +identifier funcname != { strbuf_getwholeline, parse_list_objects_filter };\n> +struct strbuf SB;\n> +@@\n> + funcname(...) {<...\n> +- !SB.buf\n> ++ 0\n> + ...>}\n\nHere is my second try. strbuf_getwholeline() does not have to break\nstrbuf invariants even tentatively. We just grab the guts of sb,\nlet getdelim() possibly reallocate, and then return it in the normal\ncase.\n\nIn the EOF code path, the only special thing we need is when we\nstarted with slopbuf[] and getdelim() allocated some bytes yet\nreturned EOF. We are expected to free it before returning.\n\nBy the way, the big comment about xrealloc() in the middle, most of\nwhich is outside the post-context of the first hunk, should be\nupdated, as our xrealloc() do not aggressively try to recover these\ndays, if I understand correctly. I left it outside the scope of\nthis patch, whose sole focus is to reduce the number of places in\nthe codebase that check if sb->buf is NULL.\n\n\n strbuf.c | 31 +++++++++++++++++--------------\n 1 file changed, 17 insertions(+), 14 deletions(-)\n\ndiff --git c/strbuf.c w/strbuf.c\nindex 3939863cf3..89933c3814 100644\n--- c/strbuf.c\n+++ w/strbuf.c\n@@ -632,24 +632,26 @@ int strbuf_getcwd(struct strbuf *sb)\n int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n {\n \tssize_t r;\n+\tchar *buf = sb->buf;\n+\tsize_t alloc = sb->alloc;\n \n \tif (feof(fp))\n \t\treturn EOF;\n \n-\tstrbuf_reset(sb);\n-\n \t/* Translate slopbuf to NULL, as we cannot call realloc on it */\n-\tif (!sb->alloc)\n-\t\tsb->buf = NULL;\n+\tif (!alloc)\n+\t\tbuf = NULL;\n \terrno = 0;\n-\tr = getdelim(&sb->buf, &sb->alloc, term, fp);\n+\tr = getdelim(&buf, &alloc, term, fp);\n \n \tif (r > 0) {\n+\t\tsb->buf = buf;\n+\t\tsb->alloc = alloc;\n \t\tsb->len = r;\n \t\treturn 0;\n \t}\n-\tassert(r == -1);\n \n+\tassert(r == -1);\n \t/*\n \t * Normally we would have called xrealloc, which will try to free\n \t * memory and recover. But we have no way to tell getdelim() to do so.\n@@ -664,15 +666,16 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n \tif (errno == ENOMEM)\n \t\tdie(\"Out of memory, getdelim failed\");\n \n-\t/*\n-\t * Restore strbuf invariants; if getdelim left us with a NULL pointer,\n-\t * we can just re-init, but otherwise we should make sure that our\n-\t * length is empty, and that the result is NUL-terminated.\n+\t/* \n+\t * If getdelim() allocated when we had no allocation, free it.\n+\t */\n+\tif (!alloc)\n+\t\tfree(buf);\n+\n+\t/* \n+\t * We haven't touched sb at all; as with the initial \"were we\n+\t * already at EOF?\" case, return EOF without touching sb.\n \t */\n-\tif (!sb->buf)\n-\t\tstrbuf_init(sb, 0);\n-\telse\n-\t\tstrbuf_reset(sb);\n \treturn EOF;\n }\n #else\n"},{"id":"539605","messageId":"20260321163941.GA717067@coredump.intra.peff.net","threadId":"65300","inReplyTo":"xmqqqzpdb172.fsf@gitster.g","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-21T16:39:41Z","receivedAt":"2026-03-21T16:39:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 21, 2026 at 09:24:17AM -0700, Junio C Hamano wrote:\n\n> Here is my second try. strbuf_getwholeline() does not have to break\n> strbuf invariants even tentatively. We just grab the guts of sb,\n> let getdelim() possibly reallocate, and then return it in the normal\n> case.\n> \n> In the EOF code path, the only special thing we need is when we\n> started with slopbuf[] and getdelim() allocated some bytes yet\n> returned EOF. We are expected to free it before returning.\n\nThis is similar to what I initially wrote (but revised before sending),\nbut I don't think it works because...\n\n> +\t/* \n> +\t * We haven't touched sb at all; as with the initial \"were we\n> +\t * already at EOF?\" case, return EOF without touching sb.\n> \t */\n\n...this part isn't necessarily true. We handed sb->buf (copied via the\nlocal \"buf\") to getdelim(). It might have reallocated it behind our\nbacks and returned the new pointer, and now sb->buf is dangling.\n\nAnd in that sense, assigning sb->buf to a local buf becomes _more_\nconfusing, because now we have two copies of a pointer that is being\nmutated.\n\n> By the way, the big comment about xrealloc() in the middle, most of\n> which is outside the post-context of the first hunk, should be\n> updated, as our xrealloc() do not aggressively try to recover these\n> days, if I understand correctly. I left it outside the scope of\n> this patch, whose sole focus is to reduce the number of places in\n> the codebase that check if sb->buf is NULL.\n\nYes, I think you're right.\n\n-Peff\n"},{"id":"539606","messageId":"20260321164131.GA717199@coredump.intra.peff.net","threadId":"65300","inReplyTo":"bed43331-ad9d-437c-a56a-94a50877f719@web.de","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-21T16:41:31Z","receivedAt":"2026-03-21T16:41:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 21, 2026 at 02:14:51PM +0100, René Scharfe wrote:\n\n> > if (feof(fp))\n> > return EOF;\n> > \n> > strbuf_reset(sb);\n> [...]\n> > I think the strbuf_reset() could go away even without any other changes.\n> > We always adjust sb->len in the end to match what happened with\n> > getdelim(), so there is no point in doing it up front.\n> \n> Yes. Same with the EOF check; getdelim(3) is (must be) prepared to handle\n> that for us. An early return at the end of the file avoids the translate\n> effort once per file, but adds the cost of checking for each line.\n\nI think you're probably right. I added it in the original as an attempt\nto simplify away a tricky case before manipulating the strbuf, but we\nhave to eventually deal with those tricky cases anyway, since we may see\nthe EOF fresh from getdelim().\n\n-Peff\n"},{"id":"539613","messageId":"3e387439-c066-4e45-b28b-43f77c8824d6@web.de","threadId":"65300","inReplyTo":"20260319233546.GA3632561@coredump.intra.peff.net","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-03-21T20:47:18Z","receivedAt":"2026-03-21T20:47:26Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 3/20/26 12:35 AM, Jeff King wrote:\n> \n> @@ -669,10 +673,13 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n> \t * we can just re-init, but otherwise we should make sure that our\n> \t * length is empty, and that the result is NUL-terminated.\n> \t */\n> -\tif (!sb->buf)\n> +\tif (!buf)\n> \t\tstrbuf_init(sb, 0);\n> -\telse\n> -\t\tstrbuf_reset(sb);\n> +\telse {\n> +\t\tsb->buf = buf;\n> +\t\tsb->alloc = alloc;\n> +\t\tstrbuf_reset(&sb);\n> +\t}\n> \treturn EOF;\n> }\n> #else\n> \n> So I don't know that it makes anything simpler. We have to copy the\n> values back into the strbuf either way, and we still have to handle\n> restoring the strbuf invariants. Even the strbuf_init() case is still\n> needed, because we don't know whether getdelim() just didn't allocate\n> (in which case we could leave the strbuf alone) or if it actually ate\n> the allocation we passed in (which was just a copy of sb->buf).\nAnd yet this function can turn an empty strbuf into an allocated one\nwithout rolling it back on error, leaving code similar to this silly\nexample here leaking:\n\n\tint copy_one_line(FILE *in, FILE *out, int term)\n\t{\n\t\tstruct strbuf sb = STRBUF_INIT;\n\t\tif (strbuf_getwholeline(&sb, in, term))\n\t\t\treturn -1;\n\t\tfwrite(sb.buf, 1, sb.len, out);\n\t\tstrbuf_release(&sb);\n\t\treturn 0;\n\t}\n\nSome strbuf functions restore the original state in such a case by\ncalling strbuf_release(), strbuf_getwholeline() doesn't. If we are OK\nwith that then it could be simplified by growing the buffer upfront:\n\n\tint strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n\t{\n\t\tssize_t r;\n\n\t\tstrbuf_grow(sb, 0);\n\t\terrno = 0;\n\t\tr = getdelim(&sb->buf, &sb->alloc, term, fp);\n\n\t\tif (r > 0) {\n\t\t\tsb->len = r;\n\t\t\treturn 0;\n\t\t}\n\n\t\tassert(r == -1);\n\t\tif (errno == ENOMEM)\n\t\t\tdie(\"Out of memory, getdelim failed\");\n\t\tstrbuf_reset(sb);\n\t\treturn EOF;\n\t}\n\nRené\n\n"},{"id":"539615","messageId":"20260321211828.GB736981@coredump.intra.peff.net","threadId":"65300","inReplyTo":"3e387439-c066-4e45-b28b-43f77c8824d6@web.de","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-21T21:18:28Z","receivedAt":"2026-03-21T21:18:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 21, 2026 at 09:47:18PM +0100, René Scharfe wrote:\n\n> And yet this function can turn an empty strbuf into an allocated one\n> without rolling it back on error, leaving code similar to this silly\n> example here leaking:\n> \n> \tint copy_one_line(FILE *in, FILE *out, int term)\n> \t{\n> \t\tstruct strbuf sb = STRBUF_INIT;\n> \t\tif (strbuf_getwholeline(&sb, in, term))\n> \t\t\treturn -1;\n> \t\tfwrite(sb.buf, 1, sb.len, out);\n> \t\tstrbuf_release(&sb);\n> \t\treturn 0;\n> \t}\n\nYes, I almost pointed that out, but I think it's mostly a non-issue\nin practice because you'd generally call it multiple times (usually in a\nloop, but sometimes just multiple individual calls). And then you have\nto release if any call ever succeeded, which means either doing so after\nthe loop ends or in a cleanup block.\n\nGrepping for 'if (strbuf_get.*line', the closest I found was\nget_mail_commit_oid(), which reads a single line. It doesn't have an\nearly return, though, since it has to clean up the FILE pointer anyway.\n\nSo I dunno. I don't think it's been a problem in practice, but I'm not\nopposed to future-proofing if it's easy to do.\n\n> Some strbuf functions restore the original state in such a case by\n> calling strbuf_release(), strbuf_getwholeline() doesn't. If we are OK\n> with that then it could be simplified by growing the buffer upfront:\n> \n> \tint strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)\n> \t{\n> \t\tssize_t r;\n> \n> \t\tstrbuf_grow(sb, 0);\n> \t\terrno = 0;\n> \t\tr = getdelim(&sb->buf, &sb->alloc, term, fp);\n\nThis causes two allocations, but presumably only the first call of many,\nso not a big deal in practice.\n\nI feel like there's a lot of discussion in this thread but we're not\nachieving anything practical. If we do anything, I think it would be:\n\n - drop the feof and reset at the top of the function, which are\n redundant\n\n - make a noop read on an unallocated strbuf retain the unallocated\n state (your example above)\n\nCould the function be rewritten differently, or maybe even made a little\nsimpler? Perhaps, but who cares? The function has been largely untouched\nfor a decade and the behavior is fine. And there are a bunch of pitfalls\nthat a rewrite risks falling into.\n\n-Peff\n"},{"id":"539621","messageId":"ca9fa6c7-f693-4b85-a17f-8deeb05b45f7@web.de","threadId":"65300","inReplyTo":"20260321211828.GB736981@coredump.intra.peff.net","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-03-21T23:41:04Z","receivedAt":"2026-03-21T23:41:14Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 3/21/26 10:18 PM, Jeff King wrote:\n> On Sat, Mar 21, 2026 at 09:47:18PM +0100, René Scharfe wrote:\n> \n>> And yet this function can turn an empty strbuf into an allocated one\n>> without rolling it back on error, leaving code similar to this silly\n>> example here leaking:\n>>\n>> \tint copy_one_line(FILE *in, FILE *out, int term)\n>> \t{\n>> \t\tstruct strbuf sb = STRBUF_INIT;\n>> \t\tif (strbuf_getwholeline(&sb, in, term))\n>> \t\t\treturn -1;\n>> \t\tfwrite(sb.buf, 1, sb.len, out);\n>> \t\tstrbuf_release(&sb);\n>> \t\treturn 0;\n>> \t}\n> \n> Yes, I almost pointed that out, but I think it's mostly a non-issue\n> in practice because you'd generally call it multiple times (usually in a\n> loop, but sometimes just multiple individual calls). And then you have\n> to release if any call ever succeeded, which means either doing so after\n> the loop ends or in a cleanup block.\n> \n> Grepping for 'if (strbuf_get.*line', the closest I found was\n> get_mail_commit_oid(), which reads a single line. It doesn't have an\n> early return, though, since it has to clean up the FILE pointer anyway.\n\nCaller strbuf_appendwholeline() handles a single line and invokes\nstrbuf_release() on error, so it swings in the opposite direction.\n\n> So I dunno. I don't think it's been a problem in practice, but I'm not\n> opposed to future-proofing if it's easy to do.\n\nI also don't think it's a problem.\n\n> I feel like there's a lot of discussion in this thread but we're not\n> achieving anything practical. \n\nFunny how attention works.\n\n> If we do anything, I think it would be:\n> \n> - drop the feof and reset at the top of the function, which are\n> redundant\n\nEasy win. We can also drop the feof(3) call from the non-getdelim(3)\nversion, but need to keep the reset there.\n\n> - make a noop read on an unallocated strbuf retain the unallocated\n> state (your example above)\n\nThat makes the function conform to the convention of rolling back on\nerror. This transactional behavior is a bit easier to understand. The\nnon-getdelim(3) version doesn't do that, though. It returns whatever\nit got and leaves error checking and rollback to its callers.\n\ngetdelim(3) doesn't allow that -- it has no way to indicate the length\nof partial reads. If we are OK with throwing away partial lines then\nwe better do that consistently in both versions? Sounds a bit messed\nup to bin perfectly good data just because some other platform has a\nfancy function that goes quiet when it stumbles. The alternative of\nhaving inconsistent behavior seems worse, though.\n\nRené\n\n"},{"id":"539624","messageId":"xmqq5x6oac9t.fsf@gitster.g","threadId":"65300","inReplyTo":"20260321211828.GB736981@coredump.intra.peff.net","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-22T01:22:38Z","receivedAt":"2026-03-22T01:22:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Could the function be rewritten differently, or maybe even made a little\n> simpler? Perhaps, but who cares? The function has been largely untouched\n> for a decade and the behavior is fine. And there are a bunch of pitfalls\n> that a rewrite risks falling into.\n\nWell, my only interest in this codepath was to get rid of \"if\n(!sb->buf)\" so that I can lift the special case in the Coccinelle\nrule. Nothing else.\n"},{"id":"539626","messageId":"20260322014023.GA816875@coredump.intra.peff.net","threadId":"65300","inReplyTo":"xmqq5x6oac9t.fsf@gitster.g","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-22T01:40:23Z","receivedAt":"2026-03-22T01:40:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 21, 2026 at 06:22:38PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Could the function be rewritten differently, or maybe even made a little\n> > simpler? Perhaps, but who cares? The function has been largely untouched\n> > for a decade and the behavior is fine. And there are a bunch of pitfalls\n> > that a rewrite risks falling into.\n> \n> Well, my only interest in this codepath was to get rid of \"if\n> (!sb->buf)\" so that I can lift the special case in the Coccinelle\n> rule. Nothing else.\n\nYeah. IMHO it is better just to keep the special case.\n\n-Peff\n"},{"id":"539627","messageId":"20260322014430.GB816875@coredump.intra.peff.net","threadId":"65300","inReplyTo":"ca9fa6c7-f693-4b85-a17f-8deeb05b45f7@web.de","subject":"Re: [RFC] cocci: .buf in a strbuf object can never be NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-22T01:44:30Z","receivedAt":"2026-03-22T01:44:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 22, 2026 at 12:41:04AM +0100, René Scharfe wrote:\n\n> > - make a noop read on an unallocated strbuf retain the unallocated\n> > state (your example above)\n> \n> That makes the function conform to the convention of rolling back on\n> error. This transactional behavior is a bit easier to understand. The\n> non-getdelim(3) version doesn't do that, though. It returns whatever\n> it got and leaves error checking and rollback to its callers.\n\nYeah, I didn't look at the fallback version. They definitely should\nmatch if we are going to change the behavior on an unallocated strbuf.\n\n> getdelim(3) doesn't allow that -- it has no way to indicate the length\n> of partial reads. If we are OK with throwing away partial lines then\n> we better do that consistently in both versions? Sounds a bit messed\n> up to bin perfectly good data just because some other platform has a\n> fancy function that goes quiet when it stumbles. The alternative of\n> having inconsistent behavior seems worse, though.\n\nI'd expect a partial read via getdelim() to return the number of bytes\nread, and set an internal flag such that ferror(f) returns true (and\nreturn -1 next time). But that is based more on wishful thinking than\nlooking at the implementation (and the details may even vary between\nimplementations).\n\nTo some degree, one you see an error on a FILE handle, all bets are off,\nand keeping or throwing away a partial line or not is not really\nimportant. You can't realistically go back and retry.\n\n-Peff\n"}]}