On Wed, Jul 31, 2024 at 7:18 PM Junio C Hamano [off-list ref] wrote:

Christian Couder [off-list ref] writes:

quoted

diff --git a/strbuf.c b/strbuf.c
index 3d2189a7f6..cccfdec0e3 100644
--- a/strbuf.c
+++ b/strbuf.c

@@ -1082,3 +1082,12 @@ void strbuf_strip_file_from_path(struct strbuf *sb)
      char *path_sep = find_last_dir_sep(sb->buf);
      strbuf_setlen(sb, path_sep ? path_sep - sb->buf + 1 : 0);
 }
+
+void strbuf_sanitize(struct strbuf *sb)
+{
+     strbuf_trim(sb);
+     for (size_t i = 0; i < sb->len; i++) {
+             if (sb->buf[i] <= 32 || sb->buf[i] >= 127)
+                     sb->buf[i] = '.';
+     }
+}

This looked a bit _too_ specific for the use of the transport layer
(which raises the question if it should even live in strbuf.[ch]).
It also made me wonder if different callers likely want to have
different variants (e.g., do not trim, only trim at the tail, squash
a run of unprintables into a single '.', use '?'  instead of '.',
etc., etc.).

It turns out that there is only *one* existing caller that gets
replaced with this "common" version, which made it a Meh to me.

Let's hope that there will be many new callers to make this step
worthwhile.

A very similar step was also part of my previous patch series to add
an OS version to the protocol. See:

https://lore.kernel.org/git/20240619125708.3719150-2-christian.couder@gmail.com/ (local)

My opinion is that the code is doing something often needed when
dealing with the protocol, so it is worth it to refactor that code
soon, and then adapt it later when needed with options (to not trim,
only trim at the tail, use '?'  instead of '.', etc).

I am not sure if it should live in strbuf.[ch], but on the other hand
if we indeed adapt it over time with a number of options for different
use cases, it might end up in strbuf.[ch], so it is a reasonable bet
to put it there right away. I must also say that I don't know which
other place(s) would be a good home for it.