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

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

Usman Akinyemi <usmanakinyemi202@gmail.com> writes:

> 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 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>
> ---
>  version.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/version.c b/version.c
> index 4d763ab48d..78f025c808 100644
> --- a/version.c
> +++ b/version.c
> @@ -6,6 +6,20 @@
>  const char git_version_string[] = GIT_VERSION;
>  const char git_built_from_commit_string[] = GIT_BUILT_FROM_COMMIT;
>  
> +/*
> + * Trim and replace each character with ascii code below 32 or above
> + * 127 (included) using a dot '.' character.

/*
 * Trim and replace each byte outside ASCII printable
 * (33 to 127, inclusive) with a dot '.'.
 */

perhaps?

> + * TODO: ensure consecutive non-printable characters are only replaced once

I am not sure what your plans are for this change.  Has the list
reached the consensus to squish consecutive redaction dots into one
in the user-agent string?  If not, let's not mention it.  Making an
incompatible change to the user-agent string is not the primary aim
of this topic anyway.

> +*/

Funny indentation.  The asterisk should have a SP before it, just
like on the previous 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.

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

Do we want to do anything special when the resulting buf->buf[]
becomes empty or just full of dots without anything else?  Should
the caller be told about such a condition, or is it callers'
responsibility to check if they care?  I am inclined to say that it
is the latter.

> @@ -27,12 +41,8 @@ const char *git_user_agent_sanitized(void)
>  		struct strbuf buf = STRBUF_INIT;
>  
>  		strbuf_addstr(&buf, git_user_agent());
> -		strbuf_trim(&buf);
> -		for (size_t i = 0; i < buf.len; i++) {
> -			if (buf.buf[i] <= 32 || buf.buf[i] >= 127)
> -				buf.buf[i] = '.';
> -		}
> -		agent = buf.buf;
> +		redact_non_printables(&buf);
> +		agent = strbuf_detach(&buf, NULL);
>  	}
>  
>  	return agent;