Re: [PATCH v2 1/6] version: refactor redact_non_printables()

From: Christian Couder <christian.couder@gmail.com>
Date: Jan 21, 2025, 08:12 UTC
Message-ID: <CAP8UFD3ccT=bAy=fsHaha=yNEDOuFpEsJ5tR7zQ1VJWtgNDh9Q@mail.gmail.com>
In-Reply-To: <CAPSxiM-NPobarwmeRA+Z1L1DCLMEJy=1REobt3tyCKKFZOO_gw@mail.gmail.com>

On Mon, Jan 20, 2025 at 6:10 PM Usman Akinyemi <usmanakinyemi202@gmail.com> wrote:

>
> On Fri, Jan 17, 2025 at 11:56 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Usman Akinyemi <usmanakinyemi202@gmail.com> writes:

Show 10 quoted lines

> > > +static void redact_non_printables(struct strbuf *buf)
> > > +{
> > > +     strbuf_trim(buf);
> > > +     for (size_t i = 0; i < buf->len; i++) {
> > > +             if (buf->buf[i] <= 32 || buf->buf[i] >= 127)
> >
> > <sane-ctype.h> defines isprint() we can use here.
> I think it would be better to add this in another commit so that one commit
> does one thing. I will add it after this patch series got settled,
> what do you think ?

Alternatively it could be done in its own preparatory patch at the beginning of this patch series.

<sane-ctype.h> has:

#define isprint(x) ((x) >= 0x20 && (x) <= 0x7e)

So if we wanted to use isprint() we would have to use something like:

    for (size_t i = 0; i < buf->len; i++) {
            if (!isprint(buf->buf[i]) || buf->buf[i] == ' ')
                    buf->buf[i] = '.';
    }

It would have been nicer if we didn't need a special case for SP. So I would say it's likely a matter of taste if the result is nicer than the original.

> >
> > > +                     buf->buf[i] = '.';
> > > +     }
> > > +}