Re: [PATCH 1/6] strbuf: add header for 'safe' API

General comment: I really like the idea of this. While I haven't
encountered this specific issue with git, I've dealt with similar issues
in other systems, and even if the cascading error case is rare, it's
incredibly frustrating to deal with the loss of error details because
they used unsafe operations to generate their messages!

I'm not really qualified to comment much on the code yet, but there's a 
couple of small writing style things that I'll nitpick for
clarity/readibilty. Feel free to ignore these if you disagree.

On Fri Sep 18, 2026 at 9:02 AM EDT, Derrick Stolee via GitGitGadget wrote:

From: Derrick Stolee <redacted>

The strbuf library is an important API used all over the Git codebase.
Contributors use it in nearly any string-manipulating action. However, the
implementation uses other helping functions that die() on failure instead of
returning an error code. Thus, the strbuf API isn't _safe_.

In particular, we cannot include 'banned-die.h' in 'strbuf.c'.

I think we prefer to avoid "we" in these comments; and 
the "in particular" here feels a little abrupt - maybe "In order to
ensure that strbuf functions can't call die, strbuf.c should not include ..."

To start the creation of a safe strbuf API, move the struct definition into
a new 'strbuf-safe.h' header file. All consumers of 'strbuf.h' will consume
that header transitively.

In the future, we will hope to have consumers that need a 'safe' API will
include 'strbuf-safe.h' instead of 'strbuf.h'.

Again, avoiding we; and I don't think the quotes belong there. 

Maybe "In the future, consumers that need a safe API will include ..."

We will see in future changes the inclusion of new implementations that
return an error code instead of halting.

The structure of this sentence is confusing. I had to read it a couple
of times to figure out how to parse it. Better something like: 
"Future changes will include new implementations that return an error
code instead of halting."

As with the prose comments above, safe shouldn't be in quotes.

+
+struct string_list;
+
+/**
+ * strbufs are meant to be used with all the usual C string and memory
+ * APIs. Given that the length of the buffer is known, it's often better to
+ * use the mem* functions than a str* one (e.g., memchr vs. strchr).
+ * Though, one has to be careful about the fact that str* functions often
+ * stop on NULs and that strbufs may have embedded NULs.
+ *
+ * A strbuf is NUL terminated for convenience, but no function in the
+ * strbuf API actually relies on the string being free of NULs.
+ *
+ * strbufs have some invariants that are very important to keep in mind:

I think this should be stronger - they shouldn't just be kept in mind, they 
should be strictly enforced: "strbufs have some invariants that _must_
be maintained".

+ *
+ *  - The `buf` member is never NULL, so it can be used in any usual C
+ *    string operations safely. strbufs _have_ to be initialized either by
+ *    `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.

I think "must" is better that "have to" here; I know some non-native
english speakers get confused by that.

+ *
+ *    Do *not* assume anything on what `buf` really is (e.g. if it is
+ *    allocated memory or not), use `strbuf_detach()` to unwrap a memory
+ *    buffer from its strbuf shell in a safe way. That is the sole supported
+ *    way. This will give you a malloced buffer that you can later `free()`.
+ *
+ *    However, it is totally safe to modify anything in the string pointed by
+ *    the `buf` member, between the indices `0` and `len-1` (inclusive).
+ *
+ *  - The `buf` member is a byte array that has at least `len + 1` bytes
+ *    allocated. The extra byte is used to store a `'\0'`, allowing the
+ *    `buf` member to be a valid C-string. All strbuf functions ensure this
+ *    invariant is preserved.

I think there should be a "must" before ensure".

+ *
+ *    NOTE: It is OK to "play" with the buffer directly if you work it this
+ *    way:

I don't think "play" is good, and in any case it shouldn't be in quotes. 
Maybe "It is OK to manipulate the buffer directly..."

+ *
+ *        strbuf_grow(sb, SOME_SIZE); <1>
+ *        strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);
+ *
+ *    <1> Here, the memory array starting at `sb->buf`, and of length
+ *    `strbuf_avail(sb)` is all yours, and you can be sure that
+ *    `strbuf_avail(sb)` is at least `SOME_SIZE`.
+ *
+ *    NOTE: `SOME_OTHER_SIZE` must be smaller or equal to `strbuf_avail(sb)`.
+ *
+ *    Doing so is safe, though if it has to be done in many places, adding the
+ *    missing API to the strbuf module is the way to go.
+ *
+ *    WARNING: Do _not_ assume that the area that is yours is of size `alloc
+ *    - 1` even if it's true in the current implementation. Alloc is somehow a
+ *    "private" member that should not be messed with. Use `strbuf_avail()`
+ *    instead.

Again, the quote around private. (I had an undergrad advisor who was a
stickler about the right way to use quotes, and he pounded into me so that
now I die a little bit every time I see them used as emphasis or as a marker 
of "not really".)

+*/
+
+/**
+ * Data Structures
+ * ---------------
+ */
+
+/**
+ * This is the string buffer structure. The `len` member can be used to
+ * determine the current length of the string, and `buf` member provides
+ * access to the string itself.
+ */
+struct strbuf {
+	size_t alloc;
+	size_t len;
+	char *buf;
+};
+
+extern char strbuf_slopbuf[];
+#define STRBUF_INIT  { .buf = strbuf_slopbuf }
+
+#endif /* STRBUF_SAFE_H */ 

Repeat comments above for the repetitions in the other file.


-- 
Mark Craig Chu-Carroll (@MarkChuCarroll at gitlab)
*** Software Tools/Math Geek - Software Engineer at Gitlab
*** Work Email: mcarroll@gitlab.com / markchucarroll@fastmail.com
*** Personal Blog: http://goodmath.org/blog / Personal email: markcc@gmail.com