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.