Re: [PATCH 2/2] fsck: handle bad trees like other errors

[David Turner <novalis@novalis.org>]

On Tue, 2016-09-27 at 01:27 -0400, Jeff King wrote:
> > -static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size)
> > +static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size, struct strbuf *err)
> >  {
> 
> I know we used the "err" strbuf pattern in the ref code, and it makes
> sense there where we have a lot of different functions with public
> interfaces. But here, we literally just feed the result to die() or
> warning(). I wonder if a nicer interface would be:
> 
>   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.

Thanks for all of the rest of the commentary; I've incorporated it and
will re-roll shortly.