Re: Proposal/Discussion: Turning parts of Git into libraries

[Jeff King <peff@peff.net>]

On Fri, Feb 24, 2023 at 12:31:16PM -0800, Emily Shaffer wrote:

> > One thing that strategy doesn't help with, though, that your
> > libification might want: it's not very machine-readable. The error
> > reporters would still fundamentally be working with strings. So a
> > libified process can know "OK, writing this ref failed, and I have some
> > error messages in a buffer". But the calling code can't know specifics
> > like "it failed because we tried to open file 'foo' and it got EPERM".
> > We _could_ design an error context that stores individual errno values
> > or codes in a list, but having each caller report those specific errors
> > is a much bigger job (and ongoing maintenance burden as we catalogue and
> > give an identifier to each error).
> 
> Is there a reason not to use this kind of struct and provide
> library-specific error code enums, though, I wonder? You're right that
> parsing the error string is really bad for the caller, for anything
> besides just logging it. But it seems somewhat reasonable to expect
> that any call from config library returning an integer error code is
> referring to enum config_errors...

Right, you could definitely layer the two approaches by storing the
enums in the struct. And that might be a good thing to do in the long
run. But I think it's a much harder change, as it implies assigning
those codes (and developing a taxonomy of errors). But that can also be
done incrementally, especially if it's done on top of human-readable
strings.

I.e., I can imagine a world where low-level code reports an error like:

   int read_foo(const char *path, struct error_context *err)
   {
           fd = open(path, ...);
           if (fd < 0)
                   return report_errno(&err, ERR_FOO_OPEN, "unable to open foo file '%s'", path);
           ...read and parse...
           if (some_unexpected_format)
                   return report(&err, ERR_FOO_FORMAT, "unable to parse foo file '%s'", path);
   }

and then the caller has many options:

  - pass in a context that just dumps the human readable errors to
    stderr

  - collect the error strings in a buffer to report by some other
    mechanism

  - check err.type to act on ERR_FOO_OPEN, etc, including errno (which
    I'd imagine report_errno() to record)

The details above are just a sketch, of course. Rather than FOO_OPEN,
callers may actually want to think of things more like a stack of
errors that would mirror the callstack. You could imagine something
like:

  type=ERR_REF_WRITE
  type=ERR_GET_LOCK
  type=ERR_OPEN, errno=EPERM

I do think it's possible to over-engineer this to the point of
absurdity, and end up creating a lot of work. Which is why I'd probably
start with uniformly trying to use an error context struct with strings,
after which adding on fancier features gets easier (and again, is
possibly something that can be done incrementally as various subsystems
support it).

-Peff