Re: [PATCH] refs.c: repack_without_refs may be called without error string buffer

On Thu, Nov 20, 2014 at 10:35 AM, Jonathan Nieder [off-list ref] wrote:

Stefan Beller wrote:

quoted

If we don't pass in the error string buffer, we skip over all
parts dealing with preparing error messages.

Please no.

We tried this with the ref transaction code.  When someone wants
to silence the message, it is cheap enough to do

        struct strbuf ignore = STRBUF_INIT;

        if (thing_that_can_fail_in_an_ignorable_way(..., &ignore)) {
                ... handle the failure ...
        }

The extra lines of code make it obvious that the error message is
being dropped, which is a very good thing.  The extra work to format a
message in the error case is not so bad and can be mitigated if the
error is a common normal case by passing a flag to not consider it an
error.

Silently losing good diagnostic messages when err == NULL would have
the opposite effect: when there isn't a spare strbuf to put errors in
around, it would be tempting for people coding in a hurry to just pass
NULL, and to readers it would look at first glance like "oh, an
optional paramter was not passed and we are getting the good default
behavior".

This is not a theoretical concern --- it actually happened.

Fair enough.
Un-LGTM my message above.

My two cents,
Jonathan