Hi Junio,
On Wed, 30 Oct 2019, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> writes:
>
> > Also please note that we `fflush(stderr)` here to help when running in a
> > Git Bash on Windows: in this case, `stderr` is not actually truly
> > unbuffered, and needs the extra help.
>
> I do not think you have to single out Windows in this case; if we
> are writing directly to file descriptor #2, as long as there have
> been something written by the process to the standard error stream
> before our caller called us, we must flush it to make sure what
> we are going to write out will come after, no matter what platform
> we are on.
>
> The process may have written to the standard error stream and
> there may be something left in the buffer kept in the stdio
> layer. Call fflush(stderr) before writing the message we
> prepare in this function.
>
> or something along that line would be sufficient.
I replaced the paragraph with a slightly edited version of this. Thanks.
> > diff --git a/usage.c b/usage.c
> > index 2fdb20086b..471efb2de9 100644
> > --- a/usage.c
> > +++ b/usage.c
> > @@ -9,14 +9,21 @@
> > void vreportf(const char *prefix, const char *err, va_list params)
> > {
> > char msg[4096];
> > - char *p;
> > + size_t off = strlcpy(msg, prefix, sizeof(msg));
> > + char *p, *pend = msg + sizeof(msg);
> >
> > - vsnprintf(msg, sizeof(msg), err, params);
> > - for (p = msg; *p; p++) {
> > + p = off < pend - msg ? msg + off : pend - 1;
> > + if (vsnprintf(p, pend - p, err, params) < 0)
> > + *p = '\0'; /* vsnprintf() failed, clip at prefix */
> > +
> > + for (; p != pend - 1 && *p; p++) {
>
> It may make the result much simpler to start with something like:
>
> size_t prefix_len = strlen(prefix);
>
> if (sizeof(msg) <= prefix_len) {
> fprintf(stderr, "BUG!!! too long a prefix '%s'\n", prefix);
> abort();
> }
> memcpy(msg, prefix, prefix_len);
> p = msg + prefix_len;
>
> as we agreed that we won't have prefix that will fill or overflow
> msg[] based on your earlier reading of the callers like BUG-fl.
Makes sense. I was a bit blinded by my endeavor to keep the diffstat
small.
> That way, we probably may be able to even lose the pend pointer and
> arithmetic around it.
I tried this, and it still looks more obvious to me with `pend`.
> > if (iscntrl(*p) && *p != '\t' && *p != '\n')
> > *p = '?';
> > }
>
> Thanks. With this flow it is crystal clear that the prefix is shown
> as-is, while the payload is sanitized.
Yes ;-)
Thanks,
Dscho
> > - fprintf(stderr, "%s%s\n", prefix, msg);
> > +
> > + *(p++) = '\n'; /* we no longer need a NUL */
> > + fflush(stderr);
> > + write_in_full(2, msg, p - msg);
> > }
> >
> > static NORETURN void usage_builtin(const char *err, va_list params)
>
>
