[RFC] cocci: .buf in a strbuf object can never be NULL

[Junio C Hamano <gitster@pobox.com>]

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

> Subject: Re: [PATCH] rerere: update to modern representation of empty strbufs
>
> Finally get rid of the special casing that was unnecessary for the
> last 19 years.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  rerere.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/rerere.c b/rerere.c
> index 6ec55964e2..0296700f9f 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -403,12 +403,8 @@ static int handle_conflict(struct strbuf *out, struct rerere_io *io,
>  			strbuf_addbuf(out, &two);
>  			rerere_strbuf_putconflict(out, '>', marker_size);
>  			if (ctx) {
> -				git_hash_update(ctx, one.buf ?
> -						one.buf : "",
> -						one.len + 1);
> -				git_hash_update(ctx, two.buf ?
> -						two.buf : "",
> -						two.len + 1);
> +				git_hash_update(ctx, one.buf, one.len + 1);
> +				git_hash_update(ctx, two.buf, two.len + 1);
>  			}
>  			break;
>  		} else if (hunk == RR_SIDE_1)

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(+)

diff --git c/contrib/coccinelle/strbuf.cocci w/contrib/coccinelle/strbuf.cocci
index 5f06105df6..3dc5cd02a3 100644
--- c/contrib/coccinelle/strbuf.cocci
+++ w/contrib/coccinelle/strbuf.cocci
@@ -60,3 +60,18 @@ expression E1, E2;
 @@
 - strbuf_addstr(E1, real_path(E2));
 + strbuf_add_real_path(E1, E2);
+
+@@
+struct strbuf SB;
+@@
+- SB.buf ? SB.buf : ""
++ SB.buf
+
+@@
+identifier funcname != { strbuf_getwholeline, parse_list_objects_filter };
+struct strbuf SB;
+@@
+  funcname(...) {<...
+- !SB.buf
++ 0
+  ...>}