From: Jeff King <peff@peff•net>
To: Junio C Hamano <gitster@pobox•com>
Cc: git@vger•kernel.org
Subject: Re: [RFC] cocci: .buf in a strbuf object can never be NULL
Date: Thu, 19 Mar 2026 19:35:46 -0400 [thread overview]
Message-ID: <20260319233546.GA3632561@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqcy0zii0s.fsf@gitster.g>
On Thu, Mar 19, 2026 at 03:14:27PM -0700, Junio C Hamano wrote:
> One is in list-objects-filter-options.c::parse_list_objects_filter()
>
> void parse_list_objects_filter(
> struct list_objects_filter_options *filter_options,
> const char *arg)
> {
> struct strbuf errbuf = STRBUF_INIT;
>
> if (!filter_options->filter_spec.buf)
> BUG("filter_options not properly initialized");
>
> The filter_options variable points at a list_objects_filter_options
> structure, which has an embedded "struct strbuf". This BUG() is
> unnecessary if the structure is properly initialized, either by the
> LIST_OBJECTS_FILTER_INIT macro or a list_objects_filter_init() call.
> But it is easy to memset(&lofo, 0, sizeof(lofo)) or zero initialize
> with "= {0}", so I think it is OK to special case and allow for
> checking the possibility that .buf might be NULL.
Yeah, this is about catching _other_ code which accidentally violates
the invariant. I don't think there is any choice between special-casing
it or just removing the BUG() check. It is probably OK to do the latter
at this point. As part of the transition to LIST_OBJECTS_FILTER_INIT it
was a bigger risk, but that is less likely now. So I am OK either way.
> Because strbuf_getwholeline() discards what is originally in sb and
> replaces it with what getdelim() returns, I have a suspicion that
> working with bare char * and size_t to interact with getdelim() and
> then using strbuf_attach() on the success case would be simpler to
> read and maintain. Once such a rewrite of this function is done
> (#leftoverbits), the special case we see in the Coccinelle rule can
> be lifted.
Hmm. I think that is something like this:
diff --git a/strbuf.c b/strbuf.c
index 3939863cf3..0333aea261 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -631,6 +631,8 @@ int strbuf_getcwd(struct strbuf *sb)
#ifdef HAVE_GETDELIM
int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
{
+ char *buf;
+ size_t alloc;
ssize_t r;
if (feof(fp))
@@ -639,12 +641,14 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
strbuf_reset(sb);
/* Translate slopbuf to NULL, as we cannot call realloc on it */
- if (!sb->alloc)
- sb->buf = NULL;
+ alloc = sb->alloc;
+ buf = alloc ? sb->buf : NULL;
errno = 0;
- r = getdelim(&sb->buf, &sb->alloc, term, fp);
+ r = getdelim(&buf, &alloc, term, fp);
if (r > 0) {
+ sb->buf = buf;
+ sb->alloc = alloc;
sb->len = r;
return 0;
}
@@ -669,10 +673,13 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
* we can just re-init, but otherwise we should make sure that our
* length is empty, and that the result is NUL-terminated.
*/
- if (!sb->buf)
+ if (!buf)
strbuf_init(sb, 0);
- else
- strbuf_reset(sb);
+ else {
+ sb->buf = buf;
+ sb->alloc = alloc;
+ strbuf_reset(&sb);
+ }
return EOF;
}
#else
So I don't know that it makes anything simpler. We have to copy the
values back into the strbuf either way, and we still have to handle
restoring the strbuf invariants. Even the strbuf_init() case is still
needed, because we don't know whether getdelim() just didn't allocate
(in which case we could leave the strbuf alone) or if it actually ate
the allocation we passed in (which was just a copy of sb->buf).
-Peff
next prev parent reply other threads:[~2026-03-19 23:35 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-19 7:15 [PATCH] rerere: update to modern representation of empty strbufs Junio C Hamano
2026-03-19 7:57 ` Patrick Steinhardt
2026-03-19 22:14 ` [RFC] cocci: .buf in a strbuf object can never be NULL Junio C Hamano
2026-03-19 23:35 ` Jeff King [this message]
2026-03-20 1:46 ` Junio C Hamano
2026-03-20 4:18 ` Jeff King
2026-03-20 5:45 ` Junio C Hamano
2026-03-20 5:57 ` Jeff King
2026-03-20 6:06 ` Junio C Hamano
2026-03-20 6:18 ` Jeff King
2026-03-21 13:14 ` René Scharfe
2026-03-21 16:41 ` Jeff King
2026-03-21 20:47 ` René Scharfe
2026-03-21 21:18 ` Jeff King
2026-03-21 23:41 ` René Scharfe
2026-03-22 1:44 ` Jeff King
2026-03-22 1:22 ` Junio C Hamano
2026-03-22 1:40 ` Jeff King
2026-03-21 16:24 ` Junio C Hamano
2026-03-21 16:39 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260319233546.GA3632561@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox