From: Junio C Hamano Date: Thu, 19 Mar 2026 22:14:27 GMT Subject: [RFC] cocci: .buf in a strbuf object can never be NULL Message-ID: In-Reply-To: Junio C Hamano writes: > Subject: Re: [PATCH] rerere: update to modern representation of empty strbufs > > Finally get rid of the special casing that was unnecessary for the > last 19 years. > > Signed-off-by: Junio C Hamano > --- > rerere.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/rerere.c b/rerere.c > index 6ec55964e2..0296700f9f 100644 > --- a/rerere.c > +++ b/rerere.c > @@ -403,12 +403,8 @@ static int handle_conflict(struct strbuf *out, struct rerere_io *io, > strbuf_addbuf(out, &two); > rerere_strbuf_putconflict(out, '>', marker_size); > if (ctx) { > - git_hash_update(ctx, one.buf ? > - one.buf : "", > - one.len + 1); > - git_hash_update(ctx, two.buf ? > - two.buf : "", > - two.len + 1); > + git_hash_update(ctx, one.buf, one.len + 1); > + git_hash_update(ctx, two.buf, two.len + 1); > } > break; > } else if (hunk == RR_SIDE_1) I wrote a trivial Coccinele rule (attached at the end) to rewrite SB.buf ? SB.buf : "" into SB.buf and this found only the above instance, which is good. However, a related rule, "it is nonsense to expect that SB.buf could sometimes be false", finds two questionable instances. 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. The other exception comes from use of getdelim() in strbuf_getwholeline(), whose early part reads like this: int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) { ... /* Translate slopbuf to NULL, as we cannot call realloc on it */ if (!sb->alloc) sb->buf = NULL; errno = 0; r = getdelim(&sb->buf, &sb->alloc, term, fp); if (r > 0) { sb->len = r; return 0; } Before calling getdelim(), we deliberately break the strbuf invariant ".buf is never NULL; it can point at the slopbuf if .len is 0". If we read even a single byte, we are OK, as the invariant is restored. Upon EOF, later in the function we have if (!sb->buf) strbuf_init(sb, 0); else strbuf_reset(sb); return EOF; to recover the strbuf invariant. 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. Thoughts? contrib/coccinelle/strbuf.cocci | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git c/contrib/coccinelle/strbuf.cocci w/contrib/coccinelle/strbuf.cocci index 5f06105df6..3dc5cd02a3 100644 --- c/contrib/coccinelle/strbuf.cocci +++ w/contrib/coccinelle/strbuf.cocci @@ -60,3 +60,18 @@ expression E1, E2; @@ - strbuf_addstr(E1, real_path(E2)); + strbuf_add_real_path(E1, E2); + +@@ +struct strbuf SB; +@@ +- SB.buf ? SB.buf : "" ++ SB.buf + +@@ +identifier funcname != { strbuf_getwholeline, parse_list_objects_filter }; +struct strbuf SB; +@@ + funcname(...) {<... +- !SB.buf ++ 0 + ...>}