Junio C Hamano <gitster@pobox.com> writes:

I wrote a trivial Coccinele rule (attached at the end) to rewrite 

    SB.buf ? SB.buf : ""

into

    SB.buf

and this found only the above instance, which is good.

However, a related rule, "it is nonsense to expect that SB.buf could sometimes be false", finds two questionable instances.

One is in list-objects-filter-options.c::parse_list_objects_filter()

        void parse_list_objects_filter(
                struct list_objects_filter_options *filter_options,
                const char *arg)
        {
                struct strbuf errbuf = STRBUF_INIT;

                if (!filter_options->filter_spec.buf)
                        BUG("filter_options not properly initialized");

The filter_options variable points at a list_objects_filter_options structure, which has an embedded "struct strbuf". This BUG() is unnecessary if the structure is properly initialized, either by the LIST_OBJECTS_FILTER_INIT macro or a list_objects_filter_init() call. But it is easy to memset(&lofo, 0, sizeof(lofo)) or zero initialize with "= {0}", so I think it is OK to special case and allow for checking the possibility that .buf might be NULL.

The other exception comes from use of getdelim() in strbuf_getwholeline(), whose early part reads like this:

        int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
        {
                ...
                /* Translate slopbuf to NULL, as we cannot call realloc on it */
                if (!sb->alloc)
                        sb->buf = NULL;
                errno = 0;
                r = getdelim(&sb->buf, &sb->alloc, term, fp);
                if (r > 0) {
                        sb->len = r;
                        return 0;
                }

Before calling getdelim(), we deliberately break the strbuf invariant ".buf is never NULL; it can point at the slopbuf if .len is 0". If we read even a single byte, we are OK, as the invariant is restored.

Upon EOF, later in the function we have

                if (!sb->buf)
                        strbuf_init(sb, 0);
                else
                        strbuf_reset(sb);
                return EOF;

to recover the strbuf invariant.

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.

Thoughts?

 contrib/coccinelle/strbuf.cocci | 15 +++++++++++++++
 1 file changed, 15 insertions(+)