Git development
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: phillip.wood@dunelm.org.uk
Cc: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, gitster@pobox.com, newren@gmail.com,
	Derrick Stolee <stolee@gmail.com>
Subject: Re: [PATCH 0/6] [RFC] Create a 'safe' strbuf API
Date: Wed, 23 Sep 2026 15:46:28 -0400	[thread overview]
Message-ID: <20260923194628.GA44327@coredump.intra.peff.net> (raw)
In-Reply-To: <9adb1d94-c72b-43f2-aa02-004e3e476eb1@gmail.com>

On Sat, Sep 19, 2026 at 04:23:41PM +0100, Phillip Wood wrote:

> > The goal of this short RFC, such as it is, is to get some feedback on
> > whether this is a worthwhile direction to pursue or if I should abandon this
> > idea of having this definition of "safe" for some APIs. This decision may
> > also determine if we should abandon ds/trace2-tolerate-failed-timestamps or
> > leave the existing behavior as-is.
> 
> I think having APIs that return errors rather than dying on allocation
> failures or overflow is reasonable. The xdiff and reftable code already have
> something similar. I'm not sure "safe" is a good description for those APIs
> though as it does not describe how they differ from the existing APIs.
> Instead of talking about safety I'd rather the documentation talked about
> returning errors on failure and the function naming somehow reflected that.

Yeah, I don't love the term "safe" here for two reasons:

  1. It implies the normal strbuf functions aren't safe. The flaw here
     is the "malloc failure is fatal" scheme, but that's just fine and
     shared with most of the rest of our code. I think we usually
     reserve safe/unsafe for things that you should be extra careful of
     using (like refs_resolve_ref_unsafe, or the unsafe hash algos).

  2. There are many types of safety, and this is just implementing one
     of them. ;) In particular, for the use case in the trace code we're
     looking at, I'd expect that we would want to avoid calling malloc()
     at all, because it relies on locks. So this new code would not be
     safe to call from a signal handler, for example.

So what I had imagined when seeing the initial subject lines was not
strbufs who report malloc errors, but rather strbuf-like functions that
operate on a fixed-size buffer (with either a static max-size like 4k,
or perhaps a per-variable max-size recorded in the struct).

You can still run into errors, of course; we might run out of room in
the buffer. But we'd see those cases deterministically for a given
input, rather than occasionally when races or other external factors
cause malloc to unexpectedly fail.

> For the strbuf API having to check for failure on every function call does
> not sound attractive, I think having a sticky error bit like the stdio
> functions so that one can build a string and check there have been no
> failures once just before using it would be a nicer approach.

Agreed. I think that is a good approach for the static_strbuf idea
above, too.

-Peff

  reply	other threads:[~2026-09-23 19:46 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-18 13:02 [PATCH 0/6] [RFC] Create a 'safe' strbuf API Derrick Stolee via GitGitGadget
2026-09-18 13:02 ` [PATCH 1/6] strbuf: add header for 'safe' API Derrick Stolee via GitGitGadget
2026-09-21 21:18   ` Junio C Hamano
2026-09-23 19:25   ` Mark C. Chu-Carroll
2026-09-23 20:14     ` Junio C Hamano
2026-09-18 13:02 ` [PATCH 2/6] wrapper: initialize GIT_ALLOC_LIMIT proactively Derrick Stolee via GitGitGadget
2026-09-18 13:02 ` [PATCH 3/6] wrapper: create safe_memory_limit_check() Derrick Stolee via GitGitGadget
2026-09-21 21:24   ` Junio C Hamano
2026-09-18 13:02 ` [PATCH 4/6] strbuf-safe: add sstrbuf_grow() Derrick Stolee via GitGitGadget
2026-09-21 21:29   ` Junio C Hamano
2026-09-18 13:02 ` [PATCH 5/6] json-writer: include strbuf-safe.h Derrick Stolee via GitGitGadget
2026-09-18 13:02 ` [PATCH 6/6] strbuf-safe: add init and release methods Derrick Stolee via GitGitGadget
2026-09-21 21:44   ` Junio C Hamano
2026-09-21 22:34     ` Junio C Hamano
2026-09-19 15:23 ` [PATCH 0/6] [RFC] Create a 'safe' strbuf API Phillip Wood
2026-09-23 19:46   ` Jeff King [this message]
2026-10-06 14:33     ` Derrick Stolee

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=20260923194628.GA44327@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=newren@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
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