Re: [PATCH v2 6/9] list-objects-filter-options: make filter_spec a strbuf

On Mon, Jun 10, 2019 at 01:13:54PM -0700, Junio C Hamano wrote:

Kind of. The first bullet point in strbuf.h says:

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

So I extrapolated that if buf is NULL, it must be because it was just xcalloc'd and not initialized. One possible improvement to the API would be to refactor it such that there is no STRBUF_INIT, but a zero-initialized strbuf is valid. If you expect to get a non-NULL buf, even for a zero-initialized strbuf, you should call a function like strbuf_nonnull_buf(&buf), and that will return the slop buf if buf is null, or the actual buf if it is non-null.

I don't understand why the API designer was so strict about requiring the buffer to be set to non-null, since it's quite a burden for API users. If I eagerly set all filter_options's strbuf's to STRBUF_INIT, it involves changing a couple of global variables which currently do not need an initializer, and it would make the code a bit messy. The structs which have a strbuf somewhere in their nested fields would need to know that, and set up an initialization macro to avoid the null buf.

I kind of suspect the right short-term fix is to avoid strbuf's and use a string_list, which I join later to a full string when needed.

Is it brittle because a strbuf may be initialized to non-zero memory, and so the "if (buf.buf == NULL)" may evaluate to false, and then go on treating garbage like a valid buffer? I would think that's almost impossible because of the use of xcalloc.

The only reason I realized the strbuf_init was necessary was not because I read the documentation, but because I mistakenly called strbuf_reset, which calls strbuf_setlen, which doesn't handle a null buf. Many other functions seem to handle it well semi-accidentially. After I ran into the crash, I finally read the documentation I cited above.