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

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Jan 6, 2025 at 5:37 AM Usman Akinyemi
<usmanakinyemi202@gmail.com> wrote:
> The git_user_agent_sanitized() function performs some sanitizing to
> avoid special characters being sent over the line and possibly messing
> up with the protocol or with the parsing on the other side.
>
> Let's extract this sanitizing into a new redact_non_printables() function,
> as we will want to reuse it in a following patch.
>
> For now the new redact_non_printables() function is still static as
> it's only needed locally.
>
> While at it, let's also make a few small improvements:
>   - use 'size_t' for 'i' instead of 'int',
>   - move the declaration of 'i' inside the 'for ( ... )',

Regarding the above two items...

>   - use strbuf_detach() to explicitly detach the string contained by
>     the 'buf' strbuf.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---
> diff --git a/version.c b/version.c
> @@ -6,6 +6,20 @@
> +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)
> +                       buf->buf[i] = '.';
> +       }
> +}
> @@ -27,12 +41,8 @@ const char *git_user_agent_sanitized(void)
>                 strbuf_addstr(&buf, git_user_agent());
> -               strbuf_trim(&buf);
> -               for (size_t i = 0; i < buf.len; i++) {

... the original code appears to have already been using `size_t` and
declaring the loop variable inside the `for` statement, despite what
the commit message says. So, is the commit message out of date? Or are
the patches out of order? Or something else?

> -                       if (buf.buf[i] <= 32 || buf.buf[i] >= 127)
> -                               buf.buf[i] = '.';
> -               }
> -               agent = buf.buf;
> +               redact_non_printables(&buf);
> +               agent = strbuf_detach(&buf, NULL);