Re: [PATCH v2 4/4] Cleanse reflog messages in the refs.c layer

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 10, 2020, 21:06 UTC
Message-ID: <xmqqtuyfrsg0.fsf@gitster.c.googlers.com>
In-Reply-To: <6ca5b99c8d7af1a3f35b3a7d25db284c879a2f10.1594401593.git.gitgitgadget@gmail.com>

"Junio C Hamano via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 6 quoted lines

> Currently, the cleansing of the reflog message is done by the files
> backend, before the log is written out.  This is sufficient with the
> current code, as that is the only backend that writes reflogs.  But
> new backends can be added that write reflogs, and we'd want the
> resulting log message we would read out of "log -g" the same no
> matter what backend is used.

I _think_ there is no correctness impact, but this slightly changes the semantics. We used to take whatever random string from the caller and at the lowest layer of the files backend, i.e.

>  	strbuf_addf(&sb, "%s %s %s", oid_to_hex(old_oid), oid_to_hex(new_oid), committer);
>  	if (msg && *msg)
> -		copy_reflog_msg(&sb, msg);
> +		strbuf_addstr(&sb, msg);

cleansed and added the message to strbuf.

With the updated code, normalize_reflog_message() is defined like so

Show 8 quoted lines

> +static char *normalize_reflog_message(const char *msg)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	if (msg && *msg)
> +		copy_reflog_msg(&sb, msg);
> +	return strbuf_detach(&sb, NULL);
> +}

to always return a non-NULL but an empty string, even if the caller gave us a NULL pointer. We always pass a non-NULL string around, and the updated low-level code avoids calling strbuf_addstr() because msg is not NULL, but *msg is '\0'.

We _might_ be able to optimize by tweaking the normalizer a bit further, perhaps like so:

    static char *normalize_reflog_message(const char *msg)
    {
            struct strbuf sb = STRBUF_INIT;

            if (msg && *msg)
                    copy_reflog_msg(&sb, msg);
            if (sb.len) {
                    return strbuf_detach(&sb, NULL);
            } else {
                    strbuf_release(&sb);
                    return NULL;
            }
    }

to avoid allocating 1 byte '\0' on the heap by strbuf_detach() when sb here ends up being truly empty.

But it would be a premature optimization to worry about such a thing at this point, I think.

Thanks.