Re: [PATCH v2 1/1] vreportf: Fix interleaving issues, remove 4096 limitation
From: Jeff King <hidden>
Date: 2019-10-25 22:11:21
On Fri, Oct 25, 2019 at 04:02:36PM +0200, Johannes Schindelin wrote:
quoted hunk ↗ jump to hunk
... and indeed, I verified that this patch fixes the problem: -- snip --diff --git a/usage.c b/usage.c index 2fdb20086bd..7f5bdfb0f40 100644 --- a/usage.c +++ b/usage.c@@ -10,13 +10,16 @@ void vreportf(const char *prefix, const char *err, va_list params) { char msg[4096]; char *p; - - vsnprintf(msg, sizeof(msg), err, params); + size_t off = strlcpy(msg, prefix, sizeof(msg)); + int ret = vsnprintf(msg + off, sizeof(msg) - off, err, params); for (p = msg; *p; p++) { if (iscntrl(*p) && *p != '\t' && *p != '\n') *p = '?'; } - fprintf(stderr, "%s%s\n", prefix, msg); + if (ret > 0) { + msg[off + ret] = '\n'; /* we no longer need a NUL */ + write_in_full(2, msg, off + ret + 1); + } }
Heh. This is quite similar to what I posted in: https://public-inbox.org/git/20190828145412.GB14432@sigill.intra.peff.net/ though I missed the cleverness with "we no longer need a NUL" to get an extra byte. ;)
quoted
except truncation to 4096. Then I would expect a patch to increase buffer size to 8192 in the next couple years. And if you also try to solve truncation, it will get you very close to my code.My point is: I don't want to "fix" truncation. I actually think of it as a feature. An error message that is longer than the average news article I read is too long, period.
Yeah. As the person responsible for many of the "avoid truncation" works referenced in the original patch, I have come to the conclusion that it is not worth the complexity. Even when we do manage to produce a gigantic error message correctly, it's generally not very readable. That's basically what I came here to say, and I was pleased to find that you had already argued for it quite well. So I'll just add my support for the direction you've taken the conversation. I _do_ wish we could do the truncation more intelligently. I'd much rather see: error: unable to open 'absurdly-long-file-name...': permission denied than: error: unable to open 'absurdly-long-file-name-that-goes-on-forever-and-ev But I don't think it's possible without reimplementing snprintf ourselves. -Peff