Re: [PATCH v2] Refactor recv_sideband()

[Lukas Fleischer <lfleischer@lfos.de>]

On Wed, 15 Jun 2016 at 09:14:54, Lukas Fleischer wrote:
> What we could do is reintroduce the local prefix variable I had in v1
> and use that to store whether a prefix needs to be prepended or not. If
> we do that and if we are fine with strbuf memory being (potentially)
> allocated and re-allocated multiple times during a single
> recv_sideband() invocation, the strbuf could be made local to the part
> that actually needs it and could be used as in asprintf().

When I revamped the patch and looked at similar code I had another idea
that I did not want to keep to myself:

In contexts similar to this patch, we often seem to use statically
allocated string buffers as follows:

-- 8< --
diff --git a/sideband.c b/sideband.c
index 8340a1b..08b75e2 100644
--- a/sideband.c
+++ b/sideband.c
@@ -22,9 +22,10 @@ int recv_sideband(const char *me, int in_stream, int out)
 {
 	const char *term, *suffix;
 	char buf[LARGE_PACKET_MAX + 1];
-	struct strbuf outbuf = STRBUF_INIT;
+	static struct strbuf outbuf = STRBUF_INIT;
 	const char *b, *brk;
 
+	strbuf_reset(&outbuf);
 	strbuf_addf(&outbuf, "%s", PREFIX);
 	term = getenv("TERM");
 	if (isatty(2) && term && strcmp(term, "dumb"))
-- 8< --

The benefits are obvious: No memory (re-)allocation overhead and no need
to take care of freeing on every return path. The downside is that the
function becomes non-thread-safe. After looking at the call sites, it
seems like we do not seem to call recv_sideband() from different threads
yet -- but do we want to rely on that?

The other two options are:

1. As I suggested earlier, introduce a wrapper that could be named
   xwritef() or fprintf_atomic() and allocate a new buffer (i.e., use a
   fresh strbuf) each time something is printed.

2. Keep using a fixed-size buffer size we already know the maximum size
   each of the strings can have.

Which one do you prefer?

Regards,
Lukas