On Thu, Aug 04, 2016 at 01:41:02PM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html