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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html