Re: [PATCH 3/7] trace: use warning() for printing trace errors

[Jeff King <peff@peff.net>]

On Thu, Aug 04, 2016 at 01:41:02PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Right now we just fprintf() straight to stderr, which can
> > make the output hard to distinguish. It would be helpful to
> > give it one of our usual prefixes like "error:", "warning:",
> > etc.
> >
> > It doesn't make sense to use error() here, as the trace code
> > is "optional" debugging code. If something goes wrong, we
> > should warn the user, but saying "error" implies the actual
> > git operation had a problem. So warning() is the only sane
> > choice.
> >
> > Note that this does end up calling warn_routine() to do the
> > formatting. So in theory, somebody who tries to trace from
> > their warn_routine() could cause a loop. But nobody does
> > this, and in fact nobody in the history of git has ever
> > replaced the default warn_builtin (there isn't even a
> > set_warn_routine function!).
> 
> I think the last bit is about to change; cf. 545f13c0 (usage: add
> set_warn_routine(), 2016-07-30) on cc/apply-am topic.

Thanks, I meant to check this series against "pu" to make sure there are
no new callers for write_or_whine_pipe(), but forgot to.

It looks like that same topic does add one new caller, and switches the
"fprintf" inside it to use warning().

IMHO the call added by 19a73ac (builtin/apply: make try_create_file()
return -1 on error, 2016-07-30) should just be a regular:

  if (write_in_full(...) < 0)
        error(...);

We don't care about the weird pipe handling there (we know we're writing
to a file we just created), and the way the error message is passed in
just makes things weird. Plus it seems more like an error() than a
warning (e.g., we call error() immediately below when close() fails!).
But 8fab3c6 (write_or_die: use warning() instead of fprintf(stderr,
...), 2016-07-30)  makes it an unconditional warning (that commit, btw,
has a bug in that it retains the trailing newline of the message, even
though warning() will add one itself).

So I'd suggest that series drop the call write_or_whine_pipe() and drop
8fab3c6 entirely.

I wondered if that would then let us drop set_warn_routine(), but it
looks like there are other warning() calls it cares about. So that would
invalidate the last paragraph here, though I still think converting the
trace errors to warning() is a reasonable thing to do.

-Peff