Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors
Jeff King writes:
> if (!dont_change_ref) {
> struct ref_transaction *transaction;
> - struct strbuf err = STRBUF_INIT;
> -
> - transaction = ref_transaction_begin(&err);
> - if (!transaction ||
> - ref_transaction_update(transaction, ref.buf,
> -sha1, forcing ? NULL : null_sha1,
> -0, msg, &err) ||
> - ref_transaction_commit(transaction, &err))
> - die("%s", err.buf);
> +
> + transaction = ref_transaction_begin(&error_die);
> + ref_transaction_update(transaction, ref.buf,
> +sha1, forcing ? NULL : null_sha1,
> +0, msg, &error_die);
> + ref_transaction_commit(transaction, &error_die);
> ref_transaction_free(transaction);
> - strbuf_release(&err);
> }
>
> if (real_ref && track)
>
> which is much shorter and to the point (it does rely on the called
> functions always calling report_error() and never just returning NULL or
> "-1", but that should be the already. If it isn't, we'd be printing
> "fatal: " with no message).
Yes but... grepping for die() got a lot harder, which may not be a
good thing.
I do like the flexibility such a mechanism offers, but
wrapping/hiding die in it is probably an example that the
flexibility went a bit too far.
Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors
On Wed, Sep 28, 2016 at 07:01:38AM +0200, Michael Haggerty wrote:
> > - a global for chaining to error, like:
> >
> >struct error_context print_errors = {
> > error, /* actually a wrapper to handle va_list and NULL data */
> > NULL
> >};
>
> There could also be a global for chaining to `warn()` or `die()`.
I played around a little with this. The latter actually makes a lot of
code cleaner, because we can rely on the functions not returning at all.
So for example, you get:
diff --git a/branch.c b/branch.c
index a5a8dcb..53404b8 100644
--- a/branch.c
+++ b/branch.c
@@ -303,17 +303,13 @@ void create_branch(const char *head,
if (!dont_change_ref) {
struct ref_transaction *transaction;
- struct strbuf err = STRBUF_INIT;
-
- transaction = ref_transaction_begin(&err);
- if (!transaction ||
- ref_transaction_update(transaction, ref.buf,
- sha1, forcing ? NULL : null_sha1,
- 0, msg, &err) ||
- ref_transaction_commit(transaction, &err))
- die("%s", err.buf);
+
+ transaction = ref_transaction_begin(&error_die);
+ ref_transaction_update(transaction, ref.buf,
+ sha1, forcing ? NULL : null_sha1,
+ 0, msg, &error_die);
+ ref_transaction_commit(transaction, &error_die);
ref_transaction_free(transaction);
- strbuf_release(&err);
}
if (real_ref && track)
which is much shorter and to the point (it does rely on the called
functions always calling report_error() and never just returning NULL or
"-1", but that should be the already. If it isn't, we'd be printing
"fatal: " with no message).
Cases that call:
error("%s", err.buf);
can drop the strbuf handling, but of course still need to retain their
conditionals. So they're better, but not as much. I did a half-hearted
conversion of some of the ref code that uses strbufs, and it seems like
it would save a few hundred lines of boilerplate.
There are some cases that are _worse_, because they want to prefix the
error. E.g., in init-db, we have:
struct strbuf err = STRBUF_INIT;
...
if (refs_init_db(&err))
die("failed to set up refs db: %s", err.buf);
which is fairly clean. Using an error_context adds slightly to the
boilerplate:
struct strbuf err_buf = STRBUF_INIT;
struct error_context err = STRBUF_ERR(&err_buf);
...
if (refs_init_db(&err))
die("failed to set up refs db: %s", err_buf.buf);
Though if we wanted to get really magical, the err_buf/err pattern could
be its own single-line macro.
You could solve this more generally with something like:
struct error_prefix_data err;
error_prefix(&err, &error_die, "failed to set up refs db");
refs_init_db(&err.err);
where error_prefix() basically sets us up to call back a function which
concatenates the prefix to the real error, then chains to error_die.
But to cover all cases, error_prefix() would actually have to format the
prefix string. Because some callers would be more like:
error_prefix(&err, &error_print, "unable to frob %s", foo);
do_frob(foo, &err);
We can't just save the va_list passed to error_prefix(), because it's
not valid after we return. So you have to format the prefix into a
buffer, even though in most cases we won't see an error at all (and
doing it completely correctly would involve using a strbuf, which means
there needs to be a cleanup step; yuck).
-Peff
Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors
On Tue, Sep 27, 2016 at 06:57:34PM -0400, David Turner wrote:
> > int report_error(struct error_context *err, const char *fmt, ...)
> > {
> > if (err->fn) {
> > va_list ap;
> > va_start(ap, fmt);
> > err->fn(err->data, fmt, ap);
> > va_end(ap);
> > }
> > return -1;
> > }
> >
> > Then low-level functions just take a context and do:
> >
> > return report_error(&err, "some error: %s", foo);
> >
> > And then the callers would pick one of a few generic error contexts:
> >
> > - passing NULL silences the errors
>
> Overall, +1.
>
> I guess I would rather have a sentinel value for silencing errors,
> because I'm worried that someone might read NULL as "don't handle the
> errors, just die". Of course, code review would hopefully catch this,
> but even so, it would be easier to read foo(x, y, silence_errors) than
> foo(x, y, null).
Yeah, I waffled on that. If you look carefully, you'll note that
the report_error() I showed above would actually require such a
"{ NULL, NULL }" global.
I don't plan to make any patches immediately for this, but I'll let it
percolate and consider whether it makes sense to try out for a future
series.
-Peff
Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors
On 09/27/2016 09:19 PM, Jeff King wrote:
> [...]
> I'm going to ramble for a minute, and I don't think it's worth exploring
> for this patch series in particular, so feel free to ignore me.
>
> I think this error concept could be extended fairly elegantly with
> something like:
>
> typedef void (*err_fn)(void *, const char *fmt, va_list ap)
> struct error_context {
> err_fn fn;
> void *data;
> };
>
> int report_error(struct error_context *err, const char *fmt, ...)
> {
> if (err->fn) {
> va_list ap;
> va_start(ap, fmt);
> err->fn(err->data, fmt, ap);
> va_end(ap);
> }
> return -1;
> }
I like this idea. It's nicely flexible (more so than the `struct strbuf
*err` that is currently used for reference transactions) without being
cumbersome.
> Then low-level functions just take a context and do:
>
> return report_error(&err, "some error: %s", foo);
>
> And then the callers would pick one of a few generic error contexts:
>
> - passing NULL silences the errors
>
> - a global for chaining to error, like:
>
>struct error_context print_errors = {
> error, /* actually a wrapper to handle va_list and NULL data */
> NULL
>};
There could also be a global for chaining to `warn()` or `die()`.
> [...]
Michael
Re: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors
On Tue, 2016-09-27 at 15:19 -0400, Jeff King wrote:
> On Tue, Sep 27, 2016 at 11:19:34AM -0400, David Turner wrote:
>
> > > typedef void (*err_fn)(const char *, ...);
> > >
> > > static int decode_tree_entry(struct tree_desc *desc,
> > >const char *buf, unsigned long size,
> > > err_fn err)
> > > {
> > > ...
> > > if (size < 23 || buf[size - 21]) {
> > > err("too-short tree object");
> > > return -1;
> > >}
> > > }
> > >
> > > I dunno. Maybe that is overengineering. I guess we only hit the strbufs
> > > in the error path (which used to die!), so nobody really cares that much
> > > about the extra allocation.
> >
> > I don't really like err_fn because:
> > (a) without a baton, it's somewhat less general (or less thread-safe)
> > than the strbuf approach and
> > (b) with a baton, it's two arguments instead of one.
>
> I'm going to ramble for a minute, and I don't think it's worth exploring
> for this patch series in particular, so feel free to ignore me.
>
> I think this error concept could be extended fairly elegantly with
> something like:
>
> typedef void (*err_fn)(void *, const char *fmt, va_list ap)
> struct error_context {
> err_fn fn;
> void *data;
> };
>
> int report_error(struct error_context *err, const char *fmt, ...)
> {
> if (err->fn) {
> va_list ap;
> va_start(ap, fmt);
> err->fn(err->data, fmt, ap);
> va_end(ap);
> }
> return -1;
> }
>
> Then low-level functions just take a context and do:
>
> return report_error(&err, "some error: %s", foo);
>
> And then the callers would pick one of a few generic error contexts:
>
> - passing NULL silences the errors
Overall, +1.
I guess I would rather have a sentinel value for silencing errors,
because I'm worried that someone might read NULL as "don't handle the
errors, just die". Of course, code review would hopefully catch this,
but even so, it would be easier to read foo(x, y, silence_errors) than
foo(x, y, null).
