On Tue, Jan 5, 2021 at 1:46 AM ZheNing Hu [off-list ref] wrote:

Signed-off-by: ZheNing Hu <redacted>

Please write a commit message which (at least briefly) explains why
this change is useful.

strbuf is allowed to contain '\0' characters, so this call to
xstrdup() will not allocate the correct amount of memory if there is
an embedded '\0'

+               int len = sb->len;
+               strbuf_init(sb, sb->len);
+               sb->buf = new_buf;
+               sb->len = len;
+               sb->buf[sb->len] = '\0';
+       }
+}

This function can probably be simplified to:

    void strbuf_const_to_no_const(struct strbuf *sb)
    {
        if (sb->len && !sb->alloc) {
            const char *v = sb->buf;
            size_t n = sb->len;
            strbuf_init(sb, n);
            strbuf_add(sb, v, n);
        }
    }

 void strbuf_release(struct strbuf *sb)
 {
        if (sb->alloc) {
                free(sb->buf);
                strbuf_init(sb, 0);
-       }
+       }else if(sb->len)
+               strbuf_init(sb, 0);
 }

I think this can be simplified to:

    void strbuf_release(struct strbuf *sb)
    {
        if (sb->alloc)
            free(sb->buf);
        if (sb->len)
            strbuf_init(sb, 0);
    }

But it's probably okay to simplify it even further:

    void strbuf_release(struct strbuf *sb)
    {
        if (sb->alloc)
            free(sb->buf);
        strbuf_init(sb, 0);
    }

 char *strbuf_detach(struct strbuf *sb, size_t *sz)
 {
        char *res;
+       if (sb->len && !sb->alloc)
+               die("you should not use detach in a const_strbuf");

I can't think of a good reason to enforce this harsh restriction. This
patch updates all the other destructive functions so they work
correctly with a buffer which was initialized from a constant string,
so this function should be able to do the same. For instance, I
believe the following would work instead:

    if (sb->len && !sb->alloc)
        strbuf_const_to_no_const(sb);

        strbuf_grow(sb, 0);

In fact, since you changed strbuf_grow() to convert the buffer from
const to non-const, then you should be able to remove the above
conditional and die() altogether.

There is a fundamental problem here. If the programmer writes:

    static struct strbuf x = STRBUF_INIT_CONST("");

then both `len` and `alloc` will be zero, so the conditional you use elsewhere:

    if (sb->len && !sb->alloc)

will not be able to detect that `buf` is pointing at a constant
string. You _may_ be able to work around this problem like this:

    if (!sb->alloc && (sb->len || sb->buf != strbuf_slopbuf))

to accurately detect a strbuf initialized with a constant string (but
I haven't tested this). Or, it might be possible to simplify it
further to:

    if (!sb->alloc && sb->buf != strbuf_slopbuf)

It would be a good idea to add a new (private) function which
encapsulates the complex condition so that it doesn't have to be
repeated all over the place. Perhaps:

    static int is_const(struct strbuf *sb) {
        return !sb->alloc && sb->buf != strbuf_slopbuf;
    }

or something.

+/*
+ *  Through this function, we can turn a constant buffer into a non-constant buffer
+ */
+void strbuf_const_to_no_const(struct strbuf *sb);

"constant" strbufs are an implementation detail which we probably
wouldn't want to publish as part of the public API. Unfortunately,
this function is needed by inline strbuf_setlen(), which is why you
added it to the header. Even so, because this is an implementation
detail, we may want to warn people against calling this function.
Perhaps like this:

    /* private -- do not call */
    void strbuf_const_to_no_const(struct strbuf *sb);

In [1], Dscho suggested that if the requested `len` is zero, then it
could treat that case specially by setting `buf` to `strbuf_slopbuf`
rather than going through the wasteful work of calling
strbuf_const_to_no_const(). Doing so may require moving the suggested
is_const() to the header, as well, so:

    /* private -- do not call */
    int strbuf_is_const(struct strbuf *sb);

[1]: https://public-inbox.org/git/nycvar.QRO.7.76.6.1806210857520.11870@tvgsbejvaqbjf.bet/