Re: [PATCH v5 12/16] merge-recursive: flush output buffer before printing error messages

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 1, 2016, 09:18 UTC
Message-ID: <alpine.DEB.2.20.1608011118480.149069@virtualbox>
In-Reply-To: <xmqqh9bavk2x.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Wed, 27 Jul 2016, Junio C Hamano wrote:

Show 13 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > The data structure passed to the recursive merge machinery has a feature
> > where the caller can ask for the output to be buffered into a strbuf, by
> > setting the field 'buffer_output'.
> >
> > Previously, we simply swallowed the buffered output when showing error
> > messages. With this patch, we show the output first, and only then print
> > the error message.
> 
> I didn't quite understand this paragraph until I realized that you
> meant "when showing die message".  We died without flushing, losing
> accumulated output.

I rephrased it, using your explanation.

Show 9 quoted lines

> > +static int err(struct merge_options *o, const char *err, ...)
> > +{
> > +	va_list params;
> > +
> > +	va_start(params, err);
> > +	flush_output(o);
> 
> I would have written the above two swapped; va_start() logically
> is about what happens in the next four lines.

For some reason, I thought that `va_start()` must be the first statement of the function. Fixed.

Show 5 quoted lines

> > +	strbuf_vaddf(&o->obuf, err, params);
> > +	error("%s", o->obuf.buf);
> > +	strbuf_reset(&o->obuf);
> 
> Sneaky ;-)

Thanks ;-) Dscho