Re: [PATCH v2 5/5] refs/debug: trim trailing LF from reflog message

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 29, 2021, 20:59 UTC
Message-ID: <211129.86v90aekug.gmgdl@evledraar.gmail.com>
In-Reply-To: <CAFQ2z_N20ESyzkPLdGbS9q8HEHGB7_gmaX8FUBR=jGqXLGcL1Q@mail.gmail.com>

On Mon, Nov 29 2021, Han-Wen Nienhuys wrote:

Show 24 quoted lines

> On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> The API promises to have only LF, not CRLF, at the end, so
>> strbuf_trim_trailing_newline() is a bit overkill (and if payload
>> happened to end with CR, we would lose it).
>
> it would be best if there was a way to escape characters (ie. "\n" =>
> "\\n"). Do we have a function for that?
>
>> > +     trace_printf_key(&trace_refs,
>> > +                      "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
>> > +                      dbg->refname, ret, o, n, committer,
>> > +                      (long int)timestamp, trimmed.buf);
>> > +     strbuf_release(&trimmed);
>> >       return ret;
>> >  }
>>
>> Can we use counted bytes in trace_printf()?  If we can, it would be
>> simpler to just scan "msg" for LF and then show only the span
>> between the beginning of the string and the found LF using "%.*s",
>> perhaps like this?
>
> I beg to differ - despite this being fewer lines of code, I think
> pointer arithmetic is best avoided if possible.

We usually do this with pointer arithmetic, but the %.*s format doesn't require that, just code like:

    const char *str = "foobar";
    size_t len = strlen(str);
    len -= 1; /* give me less! */
    printf("%.*s", (int)len, str);

So you can also feed it (len - 1) or whatever if you know it to end with a character you don't want.

It's (more simply done as) pointer arithmetic if you're finding that end marker with strstr() or whatever, but you can also bend over backwards and get a "len" instead through other means, and in either case I think it beats reallocating the whole thing (more for readability than any optimization reasons).