thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors

[Jeff King <peff@peff.net>]

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

  - a global for chaining to error, like:

       struct error_context print_errors = {
          error, /* actually a wrapper to handle va_list and NULL data */
          NULL
       };

   - a context that collects errors in a strbuf (or list, etc)

       struct strbuf err_buf = STRBUF_INIT;
       struct error_context = STRBUF_ERR_CONTEXT(&err_buf);

And that error_context can be passed around like a baton through several
functions.

I remember having a big discussion about error-passing patterns around
the ref refactoring, but I don't remember this particular thing coming
up. 

Anyway, this is all way outside the scope of what we should consider for
your current series. If we were to do something like this, it would make
sense to start using it consistently. This discussion just made me think
of it.

-Peff