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

From: Jeff King <peff@peff.net>
Date: Aug 4, 2016, 21:21 UTC
Message-ID: <20160804212157.cn7ecyxcbyf4okd7@sigill.intra.peff.net>
In-Reply-To: <xmqqk2fw1d41.fsf@gitster.mtv.corp.google.com>

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

Show 22 quoted lines

> 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