git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: David Turner <novalis@novalis.org>
Cc: git@vger.kernel.org, mhagger@alum.mit.edu,
	David Turner <dturner@twosigma.com>
Subject: thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors
Date: Tue, 27 Sep 2016 15:19:55 -0400	[thread overview]
Message-ID: <20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net> (raw)
In-Reply-To: <1474989574.26902.7.camel@frank>

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

  reply	other threads:[~2016-09-27 19:20 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-09-26 19:32 [PATCH 0/2] tree-walk improvements David Turner
2016-09-26 19:32 ` [PATCH 1/2] tree-walk: be more specific about corrupt tree errors David Turner
2016-09-27  5:14   ` Jeff King
2016-09-27  5:35     ` Junio C Hamano
2016-09-27 15:21     ` David Turner
2016-09-26 19:32 ` [PATCH 2/2] fsck: handle bad trees like other errors David Turner
2016-09-26 19:51   ` Junio C Hamano
2016-09-26 20:08   ` Junio C Hamano
2016-09-26 20:11     ` Junio C Hamano
2016-09-27  5:27   ` Jeff King
2016-09-27 15:19     ` David Turner
2016-09-27 19:19       ` Jeff King [this message]
2016-09-27 22:57         ` thoughts on error passing, was " David Turner
2016-09-28  6:54           ` Jeff King
2016-09-28  5:01         ` Michael Haggerty
2016-09-28  8:58           ` Jeff King
2016-09-28 18:02             ` Junio C Hamano
2016-09-26 19:39 ` [PATCH 0/2] tree-walk improvements Stefan Beller
2016-09-26 19:43 ` Junio C Hamano
2016-09-26 20:22   ` David Turner
2016-09-27  0:35     ` Junio C Hamano
2016-09-26 21:04 ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=dturner@twosigma.com \
    --cc=git@vger.kernel.org \
    --cc=mhagger@alum.mit.edu \
    --cc=novalis@novalis.org \
    --subject='thoughts on error passing, was Re: [PATCH 2/2] fsck: handle bad trees like other errors' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox