Re: [RFC] cocci: .buf in a strbuf object can never be NULL
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 20, 2026, 01:46 UTC
- Message-ID
- <xmqqcy0zgtmu.fsf@gitster.g>
- In-Reply-To
- <20260319233546.GA3632561@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 32 quoted lines
>> 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;I actually was hoping that all lines in this hunk before this point can be removed, i.e., strbuf_release(sb), buf = NULL, alloc = 0.
Show 9 quoted lines
> - 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;
> }Yes.
Show 14 quoted lines
> @@ -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);
> + }I do not get all these conditionals. This is an EOF code path; we have no data in buf to return. We resetted the caller's strbuf already. Can't we return buf (if allocated) to the system and return without doing any further damage to sb at this point?
Show 12 quoted lines
> 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