Jeff King [off-list ref] 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.
On Thu, Aug 04, 2016 at 01:41:02PM -0700, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
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